From 7f595445ac2eef3d4ae51e9092d22e4be2b2356b Mon Sep 17 00:00:00 2001 From: "Artur M. Wolff" Date: Tue, 31 Aug 2021 18:15:43 +0200 Subject: [PATCH] satellite/metainfo: make subsequent auth validations not perform rate-limiting Currently, requests that were successfully passed through the metainfo endpoints rate-limiter might still fail in the middle of the corresponding response. The problem is that we perform rate-limiting a second time, which means other requests would influence whether the current (already rate-checked) request will fail. This also has other unintended effects, like responding with rpcstatus.PermissionDenied for requests that were successfully rate-checked and did not lack permissions but were rate-checked again in the middle of (*Endpoint).BeginObject. This situation has been happening on the gateway side and might affect other uplink clients. This change, where appropriate, swaps subsequent validateAuth with validateAuthN that performs rate-limiting once. Change-Id: I6fc26dedb8c442dd20acaab5942f751279020b08 --- satellite/metainfo/metainfo.go | 140 ++++++++++------ satellite/metainfo/validation.go | 46 +++++ satellite/metainfo/validation_test.go | 233 ++++++++++++++++++++++++++ 3 files changed, 364 insertions(+), 55 deletions(-) create mode 100644 satellite/metainfo/validation_test.go diff --git a/satellite/metainfo/metainfo.go b/satellite/metainfo/metainfo.go index 2f0b5663a..e5b59ede4 100644 --- a/satellite/metainfo/metainfo.go +++ b/satellite/metainfo/metainfo.go @@ -300,11 +300,35 @@ func (endpoint *Endpoint) DeleteBucket(ctx context.Context, req *pb.BucketDelete now := time.Now() - keyInfo, err := endpoint.validateAuth(ctx, req.Header, macaroon.Action{ - Op: macaroon.ActionDelete, - Bucket: req.Name, - Time: now, - }) + var canRead, canList bool + + keyInfo, err := endpoint.validateAuthN(ctx, req.Header, + verifyPermission{ + action: macaroon.Action{ + Op: macaroon.ActionDelete, + Bucket: req.Name, + Time: now, + }, + }, + verifyPermission{ + action: macaroon.Action{ + Op: macaroon.ActionRead, + Bucket: req.Name, + Time: now, + }, + actionPermitted: &canRead, + optional: true, + }, + verifyPermission{ + action: macaroon.Action{ + Op: macaroon.ActionList, + Bucket: req.Name, + Time: now, + }, + actionPermitted: &canList, + optional: true, + }, + ) if err != nil { return nil, err } @@ -314,20 +338,6 @@ func (endpoint *Endpoint) DeleteBucket(ctx context.Context, req *pb.BucketDelete return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } - _, err = endpoint.validateAuth(ctx, req.Header, macaroon.Action{ - Op: macaroon.ActionRead, - Bucket: req.Name, - Time: now, - }) - canRead := err == nil - - _, err = endpoint.validateAuth(ctx, req.Header, macaroon.Action{ - Op: macaroon.ActionList, - Bucket: req.Name, - Time: now, - }) - canList := err == nil - var ( bucket storj.Bucket convBucket *pb.Bucket @@ -598,12 +608,30 @@ func (endpoint *Endpoint) BeginObject(ctx context.Context, req *pb.ObjectBeginRe endpoint.log.Warn("unable to collect uplink version", zap.Error(err)) } - keyInfo, err := endpoint.validateAuth(ctx, req.Header, macaroon.Action{ - Op: macaroon.ActionWrite, - Bucket: req.Bucket, - EncryptedPath: req.EncryptedPath, - Time: time.Now(), - }) + now := time.Now() + + var canDelete bool + + keyInfo, err := endpoint.validateAuthN(ctx, req.Header, + verifyPermission{ + action: macaroon.Action{ + Op: macaroon.ActionWrite, + Bucket: req.Bucket, + EncryptedPath: req.EncryptedPath, + Time: now, + }, + }, + verifyPermission{ + action: macaroon.Action{ + Op: macaroon.ActionDelete, + Bucket: req.Bucket, + EncryptedPath: req.EncryptedPath, + Time: now, + }, + actionPermitted: &canDelete, + optional: true, + }, + ) if err != nil { return nil, err } @@ -626,14 +654,6 @@ func (endpoint *Endpoint) BeginObject(ctx context.Context, req *pb.ObjectBeginRe return nil, rpcstatus.Error(rpcstatus.NotFound, "bucket not found: non-existing-bucket") } - _, err = endpoint.validateAuth(ctx, req.Header, macaroon.Action{ - Op: macaroon.ActionDelete, - Bucket: req.Bucket, - EncryptedPath: req.EncryptedPath, - Time: time.Now(), - }) - canDelete := err == nil - if canDelete { _, err = endpoint.DeleteObjectAnyStatus(ctx, metabase.ObjectLocation{ ProjectID: keyInfo.ProjectID, @@ -1419,12 +1439,38 @@ func (endpoint *Endpoint) BeginDeleteObject(ctx context.Context, req *pb.ObjectB now := time.Now() - keyInfo, err := endpoint.validateAuth(ctx, req.Header, macaroon.Action{ - Op: macaroon.ActionDelete, - Bucket: req.Bucket, - EncryptedPath: req.EncryptedPath, - Time: now, - }) + var canRead, canList bool + + keyInfo, err := endpoint.validateAuthN(ctx, req.Header, + verifyPermission{ + action: macaroon.Action{ + Op: macaroon.ActionDelete, + Bucket: req.Bucket, + EncryptedPath: req.EncryptedPath, + Time: now, + }, + }, + verifyPermission{ + action: macaroon.Action{ + Op: macaroon.ActionRead, + Bucket: req.Bucket, + EncryptedPath: req.EncryptedPath, + Time: now, + }, + actionPermitted: &canRead, + optional: true, + }, + verifyPermission{ + action: macaroon.Action{ + Op: macaroon.ActionList, + Bucket: req.Bucket, + EncryptedPath: req.EncryptedPath, + Time: now, + }, + actionPermitted: &canList, + optional: true, + }, + ) if err != nil { return nil, err } @@ -1434,22 +1480,6 @@ func (endpoint *Endpoint) BeginDeleteObject(ctx context.Context, req *pb.ObjectB return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } - _, err = endpoint.validateAuth(ctx, req.Header, macaroon.Action{ - Op: macaroon.ActionRead, - Bucket: req.Bucket, - EncryptedPath: req.EncryptedPath, - Time: now, - }) - canRead := err == nil - - _, err = endpoint.validateAuth(ctx, req.Header, macaroon.Action{ - Op: macaroon.ActionList, - Bucket: req.Bucket, - EncryptedPath: req.EncryptedPath, - Time: now, - }) - canList := err == nil - var deletedObjects []*pb.Object if req.GetStatus() == int32(metabase.Pending) { diff --git a/satellite/metainfo/validation.go b/satellite/metainfo/validation.go index a540e7e50..54eda6873 100644 --- a/satellite/metainfo/validation.go +++ b/satellite/metainfo/validation.go @@ -61,6 +61,52 @@ func (endpoint *Endpoint) validateAuth(ctx context.Context, header *pb.RequestHe return keyInfo, nil } +type verifyPermission struct { + action macaroon.Action + actionPermitted *bool + optional bool +} + +// validateAuthN validates things like API keys, rate limit and user permissions +// for each permission from permissions. It returns an error for the first +// required (not optional) permission that the check fails for. There must be at +// least one required (not optional) permission. In case all permissions are +// optional, it will return an error. It always returns valid RPC errors. +func (endpoint *Endpoint) validateAuthN(ctx context.Context, header *pb.RequestHeader, permissions ...verifyPermission) (_ *console.APIKeyInfo, err error) { + defer mon.Task()(&ctx)(&err) + + allOptional := true + + for _, p := range permissions { + if !p.optional { + allOptional = false + break + } + } + + if allOptional { + return nil, rpcstatus.Error(rpcstatus.Internal, "All permissions are optional") + } + + key, keyInfo, err := endpoint.validateBasic(ctx, header) + if err != nil { + return nil, err + } + + for _, p := range permissions { + err = key.Check(ctx, keyInfo.Secret, p.action, endpoint.revocations) + if p.actionPermitted != nil { + *p.actionPermitted = err == nil + } + if err != nil && !p.optional { + endpoint.log.Debug("unauthorized request", zap.Error(err)) + return nil, rpcstatus.Error(rpcstatus.PermissionDenied, "Unauthorized API credentials") + } + } + + return keyInfo, nil +} + func (endpoint *Endpoint) validateBasic(ctx context.Context, header *pb.RequestHeader) (_ *macaroon.APIKey, _ *console.APIKeyInfo, err error) { defer mon.Task()(&ctx)(&err) diff --git a/satellite/metainfo/validation_test.go b/satellite/metainfo/validation_test.go new file mode 100644 index 000000000..bed8cf1d6 --- /dev/null +++ b/satellite/metainfo/validation_test.go @@ -0,0 +1,233 @@ +// Copyright (C) 2021 Storj Labs, Inc. +// See LICENSE for copying information. + +package metainfo + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" + + "storj.io/common/macaroon" + "storj.io/common/pb" + "storj.io/common/testcontext" + "storj.io/storj/satellite/console" + "storj.io/storj/satellite/console/consoleauth" +) + +type mockAPIKeys struct { + secret []byte +} + +func (m *mockAPIKeys) GetByHead(ctx context.Context, head []byte) (*console.APIKeyInfo, error) { + return &console.APIKeyInfo{Secret: m.secret}, nil +} + +var _ APIKeys = (*mockAPIKeys)(nil) + +func TestEndpoint_validateAuthN(t *testing.T) { + ctx := testcontext.New(t) + defer ctx.Cleanup() + + secret, err := macaroon.NewSecret() + require.NoError(t, err) + + key, err := macaroon.NewAPIKey(secret) + require.NoError(t, err) + + keyNoLists, err := key.Restrict(macaroon.Caveat{DisallowLists: true}) + require.NoError(t, err) + + keyNoListsNoDeletes, err := keyNoLists.Restrict(macaroon.Caveat{DisallowDeletes: true}) + require.NoError(t, err) + + endpoint := Endpoint{ + log: zaptest.NewLogger(t), + apiKeys: &mockAPIKeys{secret: secret}, + } + + now := time.Now() + + var canRead, canList, canDelete bool + + set1 := []verifyPermission{ + { + action: macaroon.Action{ + Op: macaroon.ActionDelete, + Time: now, + }, + }, + { + action: macaroon.Action{ + Op: macaroon.ActionRead, + Time: now, + }, + actionPermitted: &canRead, + optional: true, + }, + { + action: macaroon.Action{ + Op: macaroon.ActionList, + Time: now, + }, + actionPermitted: &canList, + optional: true, + }, + } + set2 := []verifyPermission{ + { + action: macaroon.Action{ + Op: macaroon.ActionWrite, + Time: now, + }, + }, + { + action: macaroon.Action{ + Op: macaroon.ActionDelete, + Time: now, + }, + actionPermitted: &canDelete, + optional: true, + }, + } + set3 := []verifyPermission{ + { + action: macaroon.Action{ + Op: macaroon.ActionDelete, + Time: now, + }, + }, + { + action: macaroon.Action{ + Op: macaroon.ActionRead, + Time: now, + }, + actionPermitted: &canRead, + optional: true, + }, + { + action: macaroon.Action{ + Op: macaroon.ActionList, + Time: now, + }, + actionPermitted: &canList, + optional: true, + }, + } + + for i, tt := range [...]struct { + key *macaroon.APIKey + permissions []verifyPermission + wantCanRead, wantCanList, wantCanDelete bool + wantErr bool + }{ + { + key: key, + wantErr: true, + }, + + { + key: key, + permissions: make([]verifyPermission, 2), + wantErr: true, + }, + + { + key: key, + permissions: []verifyPermission{ + { + action: macaroon.Action{ + Op: macaroon.ActionWrite, + Time: now, + }, + optional: true, + }, + { + action: macaroon.Action{ + Op: macaroon.ActionDelete, + Time: now, + }, + optional: true, + }, + }, + wantErr: true, + }, + + { + key: key, + permissions: []verifyPermission{ + { + action: macaroon.Action{ + Op: macaroon.ActionProjectInfo, + Time: now, + }, + }, + }, + }, + + { + key: key, + permissions: set1, + wantCanRead: true, + wantCanList: true, + }, + { + key: keyNoLists, + permissions: set1, + wantCanRead: true, + }, + { + key: keyNoListsNoDeletes, + permissions: set1, + wantErr: true, + }, + + { + key: key, + permissions: set2, + wantCanDelete: true, + }, + { + key: keyNoLists, + permissions: set2, + wantCanDelete: true, + }, + { + key: keyNoListsNoDeletes, + permissions: set2, + }, + + { + key: key, + permissions: set3, + wantCanRead: true, + wantCanList: true, + }, + { + key: keyNoLists, + permissions: set3, + wantCanRead: true, + }, + { + key: keyNoListsNoDeletes, + permissions: set3, + wantErr: true, + }, + } { + canRead, canList, canDelete = false, false, false // reset state + + rawKey := tt.key.SerializeRaw() + ctxWithKey := consoleauth.WithAPIKey(ctx, rawKey) + + _, err := endpoint.validateAuthN(ctxWithKey, &pb.RequestHeader{ApiKey: rawKey}, tt.permissions...) + + assert.Equal(t, err != nil, tt.wantErr, i) + assert.Equal(t, tt.wantCanRead, canRead, i) + assert.Equal(t, tt.wantCanList, canList, i) + assert.Equal(t, tt.wantCanDelete, canDelete, i) + } +}