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

@@ -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{
+	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{
+	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:          time.Now(),
+				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{
+	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) {

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)
 

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)
+	}
+}