Repository: clouditor/clouditor
Pull request #1076 (2023-04-20 12:17:40)

Add authorization to AssessEvidence endpoint

codecov[bot]: ## [Codecov](https://codecov.io/gh/clouditor/clouditor/pull/1076?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) Report
> Merging [#1076](https://codecov.io/gh/clouditor/clouditor/pull/1076?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) (3fd7c2e) into [main](https://codecov.io/gh/clouditor/clouditor/commit/e80926514d381048b7b41950e731aee6d724bcf6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) (e809265) will **increase** coverage by `0.03%`.
> The diff coverage is `70.00%`.

```diff
@@            Coverage Diff             @@
##             main    #1076      +/-   ##
==========================================
+ Coverage   83.52%   83.55%   +0.03%     
==========================================
  Files          91       91              
  Lines        8193     8202       +9     
==========================================
+ Hits         6843     6853      +10     
+ Misses       1022     1021       -1     
  Partials      328      328              
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `83.55% <70.00%> (+0.03%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/clouditor/clouditor/pull/1076?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor) | Coverage Δ | |
|---|---|---|
| [service/assessment/assessment.go](https://codecov.io/gh/clouditor/clouditor/pull/1076?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor#diff-c2VydmljZS9hc3Nlc3NtZW50L2Fzc2Vzc21lbnQuZ28=) | `89.21% <62.50%> (-0.62%)` | :arrow_down: |
| [api/assessment/assessment.go](https://codecov.io/gh/clouditor/clouditor/pull/1076?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor#diff-YXBpL2Fzc2Vzc21lbnQvYXNzZXNzbWVudC5nbw==) | `100.00% <100.00%> (ø)` | |

... and [1 file with indirect coverage changes](https://codecov.io/gh/clouditor/clouditor/pull/1076/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor)

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clouditor)


Patch:
diff --git a/api/assessment/assessment.go b/api/assessment/assessment.go
index 5bb163af..0747fbf0 100644
--- a/api/assessment/assessment.go
+++ b/api/assessment/assessment.go
@@ -42,3 +42,9 @@ var (
 func (req *AssessEvidenceRequest) GetPayload() proto.Message {
 	return req.Evidence
 }
+
+// GetCloudServiceId is a shortcut to implement CloudServiceRequest. It returns the cloud service ID of the inner
+// object.
+func (req *AssessEvidenceRequest) GetCloudServiceId() string {
+	return req.GetEvidence().GetCloudServiceId()
+}
diff --git a/service/assessment/assessment.go b/service/assessment/assessment.go
index aebb0e3d..96a7e814 100644
--- a/service/assessment/assessment.go
+++ b/service/assessment/assessment.go
@@ -104,6 +104,7 @@ type Service struct {
 	confMutex sync.Mutex
 
 	authorizer api.Authorizer
+	authz      service.AuthorizationStrategy
 
 	// pe contains the actual policy evaluation engine we use
 	pe policies.PolicyEval
@@ -157,15 +158,15 @@ func WithOrchestratorAddress(address string, opts ...grpc.DialOption) service.Op
 
 // WithOAuth2Authorizer is an option to use an OAuth 2.0 authorizer
 func WithOAuth2Authorizer(config *clientcredentials.Config) service.Option[Service] {
-	return func(s *Service) {
-		s.SetAuthorizer(api.NewOAuthAuthorizerFromClientCredentials(config))
+	return func(svc *Service) {
+		svc.SetAuthorizer(api.NewOAuthAuthorizerFromClientCredentials(config))
 	}
 }
 
 // WithAuthorizer is an option to use a pre-created authorizer
 func WithAuthorizer(auth api.Authorizer) service.Option[Service] {
-	return func(s *Service) {
-		s.SetAuthorizer(auth)
+	return func(svc *Service) {
+		svc.SetAuthorizer(auth)
 	}
 }
 
@@ -176,9 +177,16 @@ func WithRegoPackageName(pkg string) service.Option[Service] {
 	}
 }
 
+// WithAuthorizationStrategy is an option that configures an authorization strategy.
+func WithAuthorizationStrategy(authz service.AuthorizationStrategy) service.Option[Service] {
+	return func(svc *Service) {
+		svc.authz = authz
+	}
+}
+
 // NewService creates a new assessment service with default values.
 func NewService(opts ...service.Option[Service]) *Service {
-	s := &Service{
+	svc := &Service{
 		evidenceStoreStreams: api.NewStreamsOf(api.WithLogger[evidence.EvidenceStore_StoreEvidencesClient, *evidence.StoreEvidenceRequest](log)),
 		orchestratorStreams:  api.NewStreamsOf(api.WithLogger[orchestrator.Orchestrator_StoreAssessmentResultsClient, *orchestrator.StoreAssessmentResultRequest](log)),
 		cachedConfigurations: make(map[string]cachedConfiguration),
@@ -186,32 +194,37 @@ func NewService(opts ...service.Option[Service]) *Service {
 
 	// Apply any options
 	for _, o := range opts {
-		o(s)
+		o(svc)
 	}
 
 	// Set to default Evidence Store
-	if s.evidenceStoreAddress.target == "" {
-		s.evidenceStoreAddress = grpcTarget{
+	if svc.evidenceStoreAddress.target == "" {
+		svc.evidenceStoreAddress = grpcTarget{
 			target: DefaultEvidenceStoreAddress,
 		}
 	}
 
 	// Set to default Orchestrator
-	if s.orchestratorAddress.target == "" {
-		s.orchestratorAddress = grpcTarget{
+	if svc.orchestratorAddress.target == "" {
+		svc.orchestratorAddress = grpcTarget{
 			target: DefaultOrchestratorAddress,
 		}
 	}
 
 	// Set to default Rego package
-	if s.evalPkg == "" {
-		s.evalPkg = policies.DefaultRegoPackage
+	if svc.evalPkg == "" {
+		svc.evalPkg = policies.DefaultRegoPackage
 	}
 
 	// Initialize the policy evaluator after options are set
-	s.pe = policies.NewRegoEval(policies.WithPackageName(s.evalPkg))
+	svc.pe = policies.NewRegoEval(policies.WithPackageName(svc.evalPkg))
 
-	return s
+	// Default to an allow-all authorization strategy
+	if svc.authz == nil {
+		svc.authz = &service.AuthorizationStrategyAllowAll{}
+	}
+
+	return svc
 }
 
 // SetAuthorizer implements UsesAuthorizer
@@ -225,7 +238,7 @@ func (svc *Service) Authorizer() api.Authorizer {
 }
 
 // AssessEvidence is a method implementation of the assessment interface: It assesses a single evidence
-func (svc *Service) AssessEvidence(_ context.Context, req *assessment.AssessEvidenceRequest) (resp *assessment.AssessEvidenceResponse, err error) {
+func (svc *Service) AssessEvidence(ctx context.Context, req *assessment.AssessEvidenceRequest) (resp *assessment.AssessEvidenceResponse, err error) {
 	resp = &assessment.AssessEvidenceResponse{}
 
 	// Validate request
@@ -242,6 +255,11 @@ func (svc *Service) AssessEvidence(_ context.Context, req *assessment.AssessEvid
 		return nil, status.Errorf(codes.InvalidArgument, "%v", err)
 	}
 
+	// Check if cloud_service_id in the service is within allowed or one can access *all* the cloud services
+	if !svc.authz.CheckAccess(ctx, service.AccessUpdate, req) {
+		return nil, service.ErrPermissionDenied
+	}
+
 	// Assess evidence
 	_, err = svc.handleEvidence(req.Evidence, resourceId)
 	if err != nil {
@@ -280,7 +298,7 @@ func (svc *Service) AssessEvidences(stream assessment.Assessment_AssessEvidences
 		assessEvidencesReq := &assessment.AssessEvidenceRequest{
 			Evidence: req.Evidence,
 		}
-		_, err = svc.AssessEvidence(context.Background(), assessEvidencesReq)
+		_, err = svc.AssessEvidence(stream.Context(), assessEvidencesReq)
 		if err != nil {
 			// Create response message. The AssessEvidence method does not need that message, so we have to create it here for the stream response.
 			res = &assessment.AssessEvidencesResponse{


Test patch:
diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go
index f29c9f2e..c36e2d21 100644
--- a/internal/logging/logging_test.go
+++ b/internal/logging/logging_test.go
@@ -177,10 +177,15 @@ func TestLogRequest(t *testing.T) {
 			args: args{
 				level:   logrus.DebugLevel,
 				reqType: Store,
-				req:     &assessment.AssessEvidenceRequest{Evidence: &evidence.Evidence{Id: testdata.MockEvidenceID}},
-				params:  []string{fmt.Sprintf("back into queue for %s (%s)", "orchestrator", "localhost")},
+				req: &assessment.AssessEvidenceRequest{
+					Evidence: &evidence.Evidence{
+						Id:             testdata.MockEvidenceID,
+						CloudServiceId: testdata.MockCloudServiceID,
+					},
+				},
+				params: []string{fmt.Sprintf("back into queue for %s (%s)", "orchestrator", "localhost")},
 			},
-			want: "level=debug msg=Evidence with ID '11111111-1111-1111-1111-111111111111' stored back into queue for orchestrator (localhost).\n",
+			want: "level=debug msg=Evidence with ID '11111111-1111-1111-1111-111111111111' stored for Cloud Service '11111111-1111-1111-1111-111111111111' back into queue for orchestrator (localhost).\n",
 		},
 	}
 	for _, tt := range tests {
diff --git a/service/assessment/assessment_test.go b/service/assessment/assessment_test.go
index 5507ef8d..74b662a3 100644
--- a/service/assessment/assessment_test.go
+++ b/service/assessment/assessment_test.go
@@ -43,6 +43,7 @@ import (
 	"clouditor.io/clouditor/internal/testdata"
 	"clouditor.io/clouditor/internal/testutil"
 	"clouditor.io/clouditor/internal/testutil/clitest"
+	"clouditor.io/clouditor/internal/testutil/servicetest"
 	"clouditor.io/clouditor/policies"
 	"clouditor.io/clouditor/service"
 	"clouditor.io/clouditor/voc"
@@ -215,6 +216,7 @@ func TestNewService(t *testing.T) {
 			assert.NotNil(t, s.orchestratorStreams)
 			s.evidenceStoreStreams = nil
 			s.orchestratorStreams = nil
+			s.authz = nil
 
 			if got := s; !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("NewService() = %v, want %v", got, tt.want)
@@ -225,13 +227,17 @@ func TestNewService(t *testing.T) {
 
 // TestAssessEvidence tests AssessEvidence
 func TestService_AssessEvidence(t *testing.T) {
+	type fields struct {
+		authz service.AuthorizationStrategy
+	}
 	type args struct {
 		in0      context.Context
 		evidence *evidence.Evidence
 	}
 	tests := []struct {
-		name string
-		args args
+		name   string
+		fields fields
+		args   args
 		// hasRPCConnection is true when connected to orchestrator and evidence store
 		hasRPCConnection bool
 		wantResp         *assessment.AssessEvidenceResponse
@@ -325,6 +331,26 @@ func TestService_AssessEvidence(t *testing.T) {
 			wantResp:         &assessment.AssessEvidenceResponse{},
 			wantErr:          assert.NoError,
 		},
+		{
+			name: "Assess resource of wrong could service",
+			fields: fields{
+				authz: servicetest.NewAuthorizationStrategy(false, testdata.MockAnotherCloudServiceID),
+			},
+			args: args{
+				in0: context.TODO(),
+				evidence: &evidence.Evidence{
+					Id:             testdata.MockEvidenceID,
+					ToolId:         testdata.MockEvidenceToolID,
+					Timestamp:      timestamppb.Now(),
+					Resource:       toStruct(voc.VirtualMachine{Compute: &voc.Compute{Resource: &voc.Resource{ID: testdata.MockResourceID, Type: []string{"VirtualMachine"}}}}, t),
+					CloudServiceId: testdata.MockCloudServiceID},
+			},
+			hasRPCConnection: true,
+			wantResp:         nil,
+			wantErr: func(tt assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, service.ErrPermissionDenied.Error())
+			},
+		},
 		{
 			name: "Assess resource without resource id",
 			args: args{
@@ -366,6 +392,9 @@ func TestService_AssessEvidence(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			s := NewService()
+			if tt.fields.authz != nil {
+				s.authz = tt.fields.authz
+			}
 			if tt.hasRPCConnection {
 				s.evidenceStoreAddress.opts = []grpc.DialOption{grpc.WithContextDialer(bufConnDialer)}
 				s.orchestratorAddress.opts = []grpc.DialOption{grpc.WithContextDialer(bufConnDialer)}
@@ -392,6 +421,7 @@ func TestService_AssessEvidences(t *testing.T) {
 		results                       map[string]*assessment.AssessmentResult
 		evidenceStoreStreams          *api.StreamsOf[evidence.EvidenceStore_StoreEvidencesClient, *evidence.StoreEvidenceRequest]
 		orchestratorStreams           *api.StreamsOf[orchestrator.Orchestrator_StoreAssessmentResultsClient, *orchestrator.StoreAssessmentResultRequest]
+		authz                         service.AuthorizationStrategy
 		UnimplementedAssessmentServer assessment.UnimplementedAssessmentServer
 	}
 	type args struct {
@@ -449,6 +479,7 @@ func TestService_AssessEvidences(t *testing.T) {
 				results:              make(map[string]*assessment.AssessmentResult),
 				evidenceStoreStreams: api.NewStreamsOf(api.WithLogger[evidence.EvidenceStore_StoreEvidencesClient, *evidence.StoreEvidenceRequest](log)),
 				orchestratorStreams:  api.NewStreamsOf(api.WithLogger[orchestrator.Orchestrator_StoreAssessmentResultsClient, *orchestrator.StoreAssessmentResultRequest](log)),
+				authz:                servicetest.NewAuthorizationStrategy(true),
 			},
 			args: args{
 				streamToServer: createMockAssessmentServerStream(&assessment.AssessEvidenceRequest{
@@ -465,8 +496,10 @@ func TestService_AssessEvidences(t *testing.T) {
 			},
 		},
 		{
-			name:   "Error in stream to client - Send()-err",
-			fields: fields{},
+			name: "Error in stream to client - Send()-err",
+			fields: fields{
+				authz: servicetest.NewAuthorizationStrategy(true),
+			},
 			args: args{
 				streamToClientWithSendErr: createMockAssessmentServerStreamWithSendErr(&assessment.AssessEvidenceRequest{
 					Evidence: &evidence.Evidence{
@@ -479,8 +512,10 @@ func TestService_AssessEvidences(t *testing.T) {
 			wantErrMessage: "rpc error: code = Unknown desc = cannot send response to the client",
 		},
 		{
-			name:   "Error in stream to server - Recv()-err",
-			fields: fields{},
+			name: "Error in stream to server - Recv()-err",
+			fields: fields{
+				authz: servicetest.NewAuthorizationStrategy(true),
+			},
 			args: args{
 				streamToServerWithRecvErr: createMockAssessmentServerStreamWithRecvErr(&assessment.AssessEvidenceRequest{
 					Evidence: &evidence.Evidence{
@@ -511,7 +546,8 @@ func TestService_AssessEvidences(t *testing.T) {
 				orchestratorAddress: grpcTarget{
 					opts: []grpc.DialOption{grpc.WithContextDialer(bufConnDialer)},
 				},
-				pe: policies.NewRegoEval(),
+				pe:    policies.NewRegoEval(),
+				authz: tt.fields.authz,
 			}
 
 			if tt.args.streamToServer != nil {
@@ -721,7 +757,7 @@ func (*mockAssessmentServerStream) SetTrailer(metadata.MD) {
 }
 
 func (*mockAssessmentServerStream) Context() context.Context {
-	return nil
+	return context.TODO()
 }
 
 func (*mockAssessmentServerStream) SendMsg(interface{}) error {
@@ -767,6 +803,10 @@ func (m *mockAssessmentServerStreamWithSendErr) Recv() (req *assessment.AssessEv
 	return req, nil
 }
 
+func (*mockAssessmentServerStreamWithSendErr) Context() context.Context {
+	return context.TODO()
+}
+
 type mockAssessmentServerStreamWithRecvErr struct {
 	grpc.ServerStream
 	RecvToServer   chan *assessment.AssessEvidenceRequest
@@ -783,6 +823,10 @@ func (*mockAssessmentServerStreamWithRecvErr) Recv() (*assessment.AssessEvidence
 	return nil, err
 }
 
+func (*mockAssessmentServerStreamWithRecvErr) Context() context.Context {
+	return context.TODO()
+}
+
 func createMockAssessmentServerStreamWithRecvErr(r *assessment.AssessEvidenceRequest) *mockAssessmentServerStreamWithRecvErr {
 	m := &mockAssessmentServerStreamWithRecvErr{
 		RecvToServer: make(chan *assessment.AssessEvidenceRequest, 1),