From 16878a22ead58d1ef5282188c52f432b44bc8ebe Mon Sep 17 00:00:00 2001 From: Michal Niewrzal Date: Tue, 10 Mar 2020 10:58:14 +0100 Subject: [PATCH] satellite/metainfo: stops hiding real validateAuth Metainfo method validateAuth checks things like API key, user permission and rate limit but at the end all errors were returned as rpcstatus.Unauthenticated. Old Metainfo is not touched to avoid backward compatibility issues. Change-Id: I78eb276210fc50151da58a5c84e13ecd0961da29 --- satellite/metainfo/metainfo.go | 42 ++++++++++++------------- satellite/metainfo/metainfo_old_test.go | 10 ++++++ satellite/metainfo/metainfo_test.go | 42 ++++++++++++------------- satellite/metainfo/validation.go | 1 + 4 files changed, 53 insertions(+), 42 deletions(-) diff --git a/satellite/metainfo/metainfo.go b/satellite/metainfo/metainfo.go index 087026d38..08a8f78a6 100644 --- a/satellite/metainfo/metainfo.go +++ b/satellite/metainfo/metainfo.go @@ -716,7 +716,7 @@ func (endpoint *Endpoint) ProjectInfo(ctx context.Context, req *pb.ProjectInfoRe Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } salt := sha256.Sum256(keyInfo.ProjectID[:]) @@ -736,7 +736,7 @@ func (endpoint *Endpoint) GetBucket(ctx context.Context, req *pb.BucketGetReques Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } bucket, err := endpoint.metainfo.GetBucket(ctx, req.GetName(), keyInfo.ProjectID) @@ -768,7 +768,7 @@ func (endpoint *Endpoint) CreateBucket(ctx context.Context, req *pb.BucketCreate Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } err = endpoint.validateBucket(ctx, req.Name) @@ -818,7 +818,7 @@ func (endpoint *Endpoint) DeleteBucket(ctx context.Context, req *pb.BucketDelete Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } err = endpoint.validateBucket(ctx, req.Name) @@ -848,7 +848,7 @@ func (endpoint *Endpoint) ListBuckets(ctx context.Context, req *pb.BucketListReq } keyInfo, err := endpoint.validateAuth(ctx, req.Header, action) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } allowedBuckets, err := getAllowedBuckets(ctx, req.Header, action) @@ -932,7 +932,7 @@ func (endpoint *Endpoint) setBucketAttribution(ctx context.Context, header *pb.R Time: time.Now(), }) if err != nil { - return rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return err } partnerID, err := endpoint.resolvePartnerID(ctx, header, partnerIDBytes) @@ -1084,7 +1084,7 @@ func (endpoint *Endpoint) BeginObject(ctx context.Context, req *pb.ObjectBeginRe Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } if !req.ExpiresAt.IsZero() && !req.ExpiresAt.After(time.Now()) { @@ -1142,7 +1142,7 @@ func (endpoint *Endpoint) commitObject(ctx context.Context, req *pb.ObjectCommit err = signing.VerifyStreamID(ctx, endpoint.satellite, streamID) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } if streamID.CreationDate.Before(time.Now().Add(-satIDExpiration)) { @@ -1156,7 +1156,7 @@ func (endpoint *Endpoint) commitObject(ctx context.Context, req *pb.ObjectCommit Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } streamMeta := pb.StreamMeta{} @@ -1223,7 +1223,7 @@ func (endpoint *Endpoint) GetObject(ctx context.Context, req *pb.ObjectGetReques Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } err = endpoint.validateBucket(ctx, req.Bucket) @@ -1322,7 +1322,7 @@ func (endpoint *Endpoint) ListObjects(ctx context.Context, req *pb.ObjectListReq Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } err = endpoint.validateBucket(ctx, req.Bucket) @@ -1373,7 +1373,7 @@ func (endpoint *Endpoint) BeginDeleteObject(ctx context.Context, req *pb.ObjectB Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } err = endpoint.validateBucket(ctx, req.Bucket) @@ -1428,7 +1428,7 @@ func (endpoint *Endpoint) FinishDeleteObject(ctx context.Context, req *pb.Object err = signing.VerifyStreamID(ctx, endpoint.satellite, streamID) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, rpcstatus.Error(rpcstatus.InvalidArgument, err.Error()) } if streamID.CreationDate.Before(time.Now().Add(-satIDExpiration)) { @@ -1442,7 +1442,7 @@ func (endpoint *Endpoint) FinishDeleteObject(ctx context.Context, req *pb.Object Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } // we don't need to do anything for shim implementation @@ -1466,7 +1466,7 @@ func (endpoint *Endpoint) BeginSegment(ctx context.Context, req *pb.SegmentBegin Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } // no need to validate streamID fields because it was validated during BeginObject @@ -1550,7 +1550,7 @@ func (endpoint *Endpoint) commitSegment(ctx context.Context, req *pb.SegmentComm Time: time.Now(), }) if err != nil { - return nil, nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, nil, err } if numResults := len(req.UploadResult); numResults < int(streamID.Redundancy.GetSuccessThreshold()) { @@ -1699,7 +1699,7 @@ func (endpoint *Endpoint) makeInlineSegment(ctx context.Context, req *pb.Segment Time: time.Now(), }) if err != nil { - return nil, nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, nil, err } if req.Position.Index < 0 { @@ -1778,7 +1778,7 @@ func (endpoint *Endpoint) BeginDeleteSegment(ctx context.Context, req *pb.Segmen Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } pointer, path, err := endpoint.getPointer(ctx, keyInfo.ProjectID, int64(req.Position.Index), streamID.Bucket, streamID.EncryptedPath) @@ -1838,7 +1838,7 @@ func (endpoint *Endpoint) FinishDeleteSegment(ctx context.Context, req *pb.Segme Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } // at the moment logic is in BeginDeleteSegment @@ -1862,7 +1862,7 @@ func (endpoint *Endpoint) ListSegments(ctx context.Context, req *pb.SegmentListR Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } limit := req.Limit @@ -2029,7 +2029,7 @@ func (endpoint *Endpoint) DownloadSegment(ctx context.Context, req *pb.SegmentDo Time: time.Now(), }) if err != nil { - return nil, rpcstatus.Error(rpcstatus.Unauthenticated, err.Error()) + return nil, err } bucketID := createBucketID(keyInfo.ProjectID, streamID.Bucket) diff --git a/satellite/metainfo/metainfo_old_test.go b/satellite/metainfo/metainfo_old_test.go index aaeeb1e88..1028e5187 100644 --- a/satellite/metainfo/metainfo_old_test.go +++ b/satellite/metainfo/metainfo_old_test.go @@ -63,6 +63,16 @@ func TestInvalidAPIKeyOld(t *testing.T) { }) } +func assertUnauthenticated(t *testing.T, err error, allowed bool) { + t.Helper() + + // If it's allowed, we allow any non-unauthenticated error because + // some calls error after authentication checks. + if !allowed { + assert.True(t, errs2.IsRPC(err, rpcstatus.Unauthenticated)) + } +} + func TestRestrictedAPIKey(t *testing.T) { testplanet.Run(t, testplanet.Config{ SatelliteCount: 1, StorageNodeCount: 0, UplinkCount: 1, diff --git a/satellite/metainfo/metainfo_test.go b/satellite/metainfo/metainfo_test.go index 5de1fdd49..d26faf05b 100644 --- a/satellite/metainfo/metainfo_test.go +++ b/satellite/metainfo/metainfo_test.go @@ -44,43 +44,43 @@ func TestInvalidAPIKey(t *testing.T) { client.SetRawAPIKey([]byte(invalidAPIKey)) _, err = client.BeginObject(ctx, metainfo.BeginObjectParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, err = client.BeginDeleteObject(ctx, metainfo.BeginDeleteObjectParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, err = client.ListBuckets(ctx, metainfo.ListBucketsParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, _, err = client.ListObjects(ctx, metainfo.ListObjectsParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) err = client.CommitObject(ctx, metainfo.CommitObjectParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, err = client.CreateBucket(ctx, metainfo.CreateBucketParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) err = client.DeleteBucket(ctx, metainfo.DeleteBucketParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, err = client.BeginDeleteObject(ctx, metainfo.BeginDeleteObjectParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) err = client.FinishDeleteObject(ctx, metainfo.FinishDeleteObjectParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, err = client.GetBucket(ctx, metainfo.GetBucketParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, err = client.GetObject(ctx, metainfo.GetObjectParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) err = client.SetBucketAttribution(ctx, metainfo.SetBucketAttributionParams{}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, err = client.GetProjectInfo(ctx) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) // these methods needs StreamID to do authentication @@ -98,19 +98,19 @@ func TestInvalidAPIKey(t *testing.T) { require.NoError(t, err) _, _, _, err = client.BeginSegment(ctx, metainfo.BeginSegmentParams{StreamID: streamID}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, _, _, err = client.BeginDeleteSegment(ctx, metainfo.BeginDeleteSegmentParams{StreamID: streamID}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) err = client.MakeInlineSegment(ctx, metainfo.MakeInlineSegmentParams{StreamID: streamID}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, _, err = client.ListSegments(ctx, metainfo.ListSegmentsParams{StreamID: streamID}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) _, _, err = client.DownloadSegment(ctx, metainfo.DownloadSegmentParams{StreamID: streamID}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) // these methods needs SegmentID @@ -127,18 +127,18 @@ func TestInvalidAPIKey(t *testing.T) { require.NoError(t, err) err = client.CommitSegment(ctx, metainfo.CommitSegmentParams{SegmentID: segmentID}) - assertUnauthenticated(t, err, false) + assertInvalidArgument(t, err, false) } }) } -func assertUnauthenticated(t *testing.T, err error, allowed bool) { +func assertInvalidArgument(t *testing.T, err error, allowed bool) { t.Helper() // If it's allowed, we allow any non-unauthenticated error because // some calls error after authentication checks. if !allowed { - assert.True(t, errs2.IsRPC(err, rpcstatus.Unauthenticated)) + assert.True(t, errs2.IsRPC(err, rpcstatus.InvalidArgument)) } } diff --git a/satellite/metainfo/validation.go b/satellite/metainfo/validation.go index 84df04f93..341511828 100644 --- a/satellite/metainfo/validation.go +++ b/satellite/metainfo/validation.go @@ -136,6 +136,7 @@ func getAPIKey(ctx context.Context, header *pb.RequestHeader) (key *macaroon.API return macaroon.ParseAPIKey(string(keyData)) } +// validateAuth validates things like API key, user permissions and rate limit and always returns valid rpc error. func (endpoint *Endpoint) validateAuth(ctx context.Context, header *pb.RequestHeader, action macaroon.Action) (_ *console.APIKeyInfo, err error) { defer mon.Task()(&ctx)(&err)