From 234eb5ff5c0c2c6a72b6634facca37e47581f88b Mon Sep 17 00:00:00 2001 From: Keith Payne Date: Fri, 22 Nov 2024 09:04:20 -0800 Subject: [PATCH] Implements API scope full access check for Status OTE PiperOrigin-RevId: 699190596 --- internal/onetime/status/status.go | 15 +++ internal/onetime/status/status_test.go | 41 +++++--- protos/instanceinfo/instanceinfo.proto | 1 + protos/status/status.proto | 1 + shared/gce/metadataserver/metadataserver.go | 31 ++++-- .../gce/metadataserver/metadataserver_test.go | 18 ++++ shared/statushelper/statushelper.go | 7 ++ shared/statushelper/statushelper_test.go | 95 +++++++++++++++---- 8 files changed, 172 insertions(+), 37 deletions(-) diff --git a/internal/onetime/status/status.go b/internal/onetime/status/status.go index e8c9e947..a15c9069 100644 --- a/internal/onetime/status/status.go +++ b/internal/onetime/status/status.go @@ -30,6 +30,7 @@ import ( "github.com/GoogleCloudPlatform/sapagent/internal/onetime" "github.com/GoogleCloudPlatform/sapagent/internal/onetime/supportbundle" "github.com/GoogleCloudPlatform/sapagent/shared/commandlineexecutor" + "github.com/GoogleCloudPlatform/sapagent/shared/gce/metadataserver" "github.com/GoogleCloudPlatform/sapagent/shared/log" "github.com/GoogleCloudPlatform/sapagent/shared/statushelper" @@ -43,6 +44,8 @@ const ( fetchLatestVersionError = "Error: could not fetch latest version" ) +var requiredScopes = []string{"https://www.googleapis.com/auth/cloud-platform"} + // Status stores the status subcommand parameters. type Status struct { ConfigFilePath string @@ -55,6 +58,7 @@ type Status struct { readFile configuration.ReadConfigFile exec commandlineexecutor.Execute exists commandlineexecutor.Exists + fetchCloudProps metadataserver.GetCloudProperties } // Name implements the subcommand interface for status. @@ -111,6 +115,7 @@ func (s *Status) Run(ctx context.Context, opts *onetime.RunOptions) (*spb.AgentS s.readFile = os.ReadFile s.exec = commandlineexecutor.ExecuteCommand s.exists = commandlineexecutor.CommandExists + s.fetchCloudProps = metadataserver.FetchCloudProperties status, err := s.statusHandler(ctx) if err != nil { log.CtxLogger(ctx).Errorw("Could not get agent status", "error", err) @@ -148,6 +153,16 @@ func (s *Status) agentStatus(ctx context.Context) (*spb.AgentStatus, *cpb.Config agentStatus.AvailableVersion = fetchLatestVersionError } + metadataCloudProps := s.fetchCloudProps() + hasRequiredScopes, err := statushelper.CheckScopes(ctx, metadataCloudProps.Scopes, requiredScopes) + agentStatus.CloudApiAccessFullScopesGranted = spb.State_FAILURE_STATE + if err != nil { + log.CtxLogger(ctx).Errorw("Could not fetch scopes", "error", err) + agentStatus.CloudApiAccessFullScopesGranted = spb.State_ERROR_STATE + } else if hasRequiredScopes { + agentStatus.CloudApiAccessFullScopesGranted = spb.State_SUCCESS_STATE + } + enabled, running, err := statushelper.CheckAgentEnabledAndRunning(ctx, agentPackageName, runtime.GOOS, s.exec) agentStatus.SystemdServiceEnabled = spb.State_FAILURE_STATE agentStatus.SystemdServiceRunning = spb.State_FAILURE_STATE diff --git a/internal/onetime/status/status_test.go b/internal/onetime/status/status_test.go index d27a8dac..191e24b0 100644 --- a/internal/onetime/status/status_test.go +++ b/internal/onetime/status/status_test.go @@ -33,6 +33,7 @@ import ( ipb "github.com/GoogleCloudPlatform/sapagent/protos/instanceinfo" spb "github.com/GoogleCloudPlatform/sapagent/protos/status" "github.com/GoogleCloudPlatform/sapagent/shared/commandlineexecutor" + "github.com/GoogleCloudPlatform/sapagent/shared/gce/metadataserver" "github.com/GoogleCloudPlatform/sapagent/shared/log" ) @@ -121,15 +122,21 @@ func TestStatusHandler(t *testing.T) { exists: func(string) bool { return true }, + fetchCloudProps: func() *metadataserver.CloudProperties { + return &metadataserver.CloudProperties{ + Scopes: []string{"https://www.googleapis.com/auth/cloud-platform"}, + } + }, }, want: &spb.AgentStatus{ - AgentName: agentPackageName, - InstalledVersion: fmt.Sprintf("%s-%s", configuration.AgentVersion, configuration.AgentBuildChange), - AvailableVersion: "enabled", - SystemdServiceEnabled: spb.State_SUCCESS_STATE, - SystemdServiceRunning: spb.State_SUCCESS_STATE, - ConfigurationFilePath: configuration.LinuxConfigPath, - ConfigurationValid: spb.State_SUCCESS_STATE, + AgentName: agentPackageName, + InstalledVersion: fmt.Sprintf("%s-%s", configuration.AgentVersion, configuration.AgentBuildChange), + AvailableVersion: "enabled", + SystemdServiceEnabled: spb.State_SUCCESS_STATE, + SystemdServiceRunning: spb.State_SUCCESS_STATE, + ConfigurationFilePath: configuration.LinuxConfigPath, + ConfigurationValid: spb.State_SUCCESS_STATE, + CloudApiAccessFullScopesGranted: spb.State_SUCCESS_STATE, Services: []*spb.ServiceStatus{ { Name: "Host Metrics", @@ -232,15 +239,21 @@ func TestStatusHandler(t *testing.T) { exists: func(string) bool { return false }, + fetchCloudProps: func() *metadataserver.CloudProperties { + return &metadataserver.CloudProperties{ + Scopes: []string{}, + } + }, }, want: &spb.AgentStatus{ - AgentName: agentPackageName, - InstalledVersion: fmt.Sprintf("%s-%s", configuration.AgentVersion, configuration.AgentBuildChange), - AvailableVersion: fetchLatestVersionError, - SystemdServiceEnabled: spb.State_ERROR_STATE, - SystemdServiceRunning: spb.State_ERROR_STATE, - ConfigurationFilePath: configuration.LinuxConfigPath, - ConfigurationValid: spb.State_SUCCESS_STATE, + AgentName: agentPackageName, + InstalledVersion: fmt.Sprintf("%s-%s", configuration.AgentVersion, configuration.AgentBuildChange), + AvailableVersion: fetchLatestVersionError, + SystemdServiceEnabled: spb.State_ERROR_STATE, + SystemdServiceRunning: spb.State_ERROR_STATE, + ConfigurationFilePath: configuration.LinuxConfigPath, + ConfigurationValid: spb.State_SUCCESS_STATE, + CloudApiAccessFullScopesGranted: spb.State_FAILURE_STATE, Services: []*spb.ServiceStatus{ { Name: "Host Metrics", diff --git a/protos/instanceinfo/instanceinfo.proto b/protos/instanceinfo/instanceinfo.proto index 03d1ce8b..daf9570d 100644 --- a/protos/instanceinfo/instanceinfo.proto +++ b/protos/instanceinfo/instanceinfo.proto @@ -29,6 +29,7 @@ message CloudProperties { string region = 7; // This is needed only for baremtal systems and is not // used for GCE instances. string machine_type = 8; + repeated string scopes = 9; } message Disk { diff --git a/protos/status/status.proto b/protos/status/status.proto index fc5f958a..56c82c14 100644 --- a/protos/status/status.proto +++ b/protos/status/status.proto @@ -30,6 +30,7 @@ message AgentStatus { repeated ServiceStatus services = 8; repeated Reference references = 9; string agent_name = 10; + State cloud_api_access_full_scopes_granted = 11; } message ServiceStatus { diff --git a/shared/gce/metadataserver/metadataserver.go b/shared/gce/metadataserver/metadataserver.go index 01b980af..94f36b2d 100644 --- a/shared/gce/metadataserver/metadataserver.go +++ b/shared/gce/metadataserver/metadataserver.go @@ -43,6 +43,9 @@ const ( MachineTypeUnknown = "unknown" ) +// GetCloudProperties abstracts metadataserver.FetchCloudProperties function for testability. +type GetCloudProperties func() *CloudProperties + var ( zonePattern = regexp.MustCompile("zones/([^/]*)") machineTypePattern = regexp.MustCompile("machineTypes/([^/]*)") @@ -73,16 +76,26 @@ type ( } instanceInfo struct { - ID int64 `json:"id"` - Zone string `json:"zone"` - Name string `json:"name"` - Image string `json:"image"` - MachineType string `json:"machineType"` + ID int64 `json:"id"` + Zone string `json:"zone"` + Name string `json:"name"` + Image string `json:"image"` + MachineType string `json:"machineType"` + ServiceAccounts serviceAccounts `json:"serviceAccounts"` + } + + serviceAccounts struct { + DefaultInfo defaultInfo `json:"default"` + } + + defaultInfo struct { + Scopes []string `json:"scopes"` } // CloudProperties contains the cloud properties of the instance. CloudProperties struct { ProjectID, NumericProjectID, InstanceID, Zone, InstanceName, Image, MachineType string + Scopes []string } ) @@ -216,6 +229,7 @@ func requestCloudProperties() (*instancepb.CloudProperties, error) { machineType := parseMachineType(instance.MachineType) instanceName := instance.Name image := instance.Image + scopes := instance.ServiceAccounts.DefaultInfo.Scopes if image == "" { image = ImageUnknown @@ -225,7 +239,7 @@ func requestCloudProperties() (*instancepb.CloudProperties, error) { } log.Logger.Debugw("Default Cloud Properties from metadata server", - "projectid", projectID, "projectnumber", numericProjectID, "instanceid", instanceID, "zone", zone, "instancename", instanceName, "image", image, "machinetype", machineType) + "projectid", projectID, "projectnumber", numericProjectID, "instanceid", instanceID, "zone", zone, "instancename", instanceName, "image", image, "machinetype", machineType, "scopes", scopes) if projectID == "" || numericProjectID == "0" || instanceID == "0" || zone == "" || instanceName == "" { return nil, fmt.Errorf("metadata server responded with incomplete information") @@ -239,6 +253,7 @@ func requestCloudProperties() (*instancepb.CloudProperties, error) { InstanceName: instanceName, Image: image, MachineType: machineType, + Scopes: scopes, }, nil } @@ -262,6 +277,7 @@ func requestProperties() (*CloudProperties, error) { machineType := parseMachineType(instance.MachineType) instanceName := instance.Name image := instance.Image + scopes := instance.ServiceAccounts.DefaultInfo.Scopes if image == "" { image = ImageUnknown @@ -271,7 +287,7 @@ func requestProperties() (*CloudProperties, error) { } log.Logger.Debugw("Default Cloud Properties from metadata server", - "projectid", projectID, "projectnumber", numericProjectID, "instanceid", instanceID, "zone", zone, "instancename", instanceName, "image", image, "machinetype", machineType) + "projectid", projectID, "projectnumber", numericProjectID, "instanceid", instanceID, "zone", zone, "instancename", instanceName, "image", image, "machinetype", machineType, "scopes", scopes) if projectID == "" || numericProjectID == "0" || instanceID == "0" || zone == "" || instanceName == "" { return nil, fmt.Errorf("metadata server responded with incomplete information") @@ -285,6 +301,7 @@ func requestProperties() (*CloudProperties, error) { InstanceName: instanceName, Image: image, MachineType: machineType, + Scopes: scopes, }, nil } diff --git a/shared/gce/metadataserver/metadataserver_test.go b/shared/gce/metadataserver/metadataserver_test.go index 1d0e0fef..98368f5d 100644 --- a/shared/gce/metadataserver/metadataserver_test.go +++ b/shared/gce/metadataserver/metadataserver_test.go @@ -242,6 +242,11 @@ func TestCloudPropertiesWithRetry(t *testing.T) { Name: "test-instance-name", Image: "test-image", MachineType: "projects/test-project/machineTypes/test-machine-type", + ServiceAccounts: serviceAccounts{ + DefaultInfo: defaultInfo{ + Scopes: []string{"scope1", "scope2"}, + }, + }, }, })}, want: &instancepb.CloudProperties{ @@ -252,6 +257,7 @@ func TestCloudPropertiesWithRetry(t *testing.T) { InstanceName: "test-instance-name", Image: "test-image", MachineType: "test-machine-type", + Scopes: []string{"scope1", "scope2"}, }, }, { @@ -399,6 +405,11 @@ func TestReadCloudPropertiesWithRetry(t *testing.T) { Name: "test-instance-name", Image: "test-image", MachineType: "projects/test-project/machineTypes/test-machine-type", + ServiceAccounts: serviceAccounts{ + DefaultInfo: defaultInfo{ + Scopes: []string{"scope1", "scope2"}, + }, + }, }, })}, want: &CloudProperties{ @@ -409,6 +420,7 @@ func TestReadCloudPropertiesWithRetry(t *testing.T) { InstanceName: "test-instance-name", Image: "test-image", MachineType: "test-machine-type", + Scopes: []string{"scope1", "scope2"}, }, }, { @@ -497,6 +509,11 @@ func TestFetchCloudProperties(t *testing.T) { Name: "test-instance-name", Image: "test-image", MachineType: "projects/test-project/machineTypes/test-machine-type", + ServiceAccounts: serviceAccounts{ + DefaultInfo: defaultInfo{ + Scopes: []string{"scope1", "scope2"}, + }, + }, }, })} want := &CloudProperties{ @@ -507,6 +524,7 @@ func TestFetchCloudProperties(t *testing.T) { InstanceName: "test-instance-name", Image: "test-image", MachineType: "test-machine-type", + Scopes: []string{"scope1", "scope2"}, } ts := mockMetadataServer(t, url) diff --git a/shared/statushelper/statushelper.go b/shared/statushelper/statushelper.go index 1ebf81eb..c024c73a 100644 --- a/shared/statushelper/statushelper.go +++ b/shared/statushelper/statushelper.go @@ -21,6 +21,7 @@ package statushelper import ( "context" "fmt" + "slices" "sort" "strings" @@ -149,6 +150,11 @@ func CheckIAMRoles(ctx context.Context, projectID string, requiredRoles []string return nil } +// CheckScopes validates that the scopes granted are equal to the required scopes. +func CheckScopes(ctx context.Context, scopes []string, requiredScopes []string) (hasRequiredScopes bool, err error) { + return slices.Equal(scopes, requiredScopes), nil +} + // packageVersionLinux returns the latest version of the agent package // available on the linux OS's package manager. func packageVersionLinux(ctx context.Context, packageName string, repoName string, exec commandlineexecutor.Execute, exists commandlineexecutor.Exists) (string, error) { @@ -217,6 +223,7 @@ func PrintStatus(ctx context.Context, status *spb.AgentStatus) { if status.GetConfigurationValid() != spb.State_SUCCESS_STATE { printColor(failure, " %s\n", status.GetConfigurationErrorMessage()) } + printState(ctx, " Cloud API Access Full Scopes Granted", status.GetCloudApiAccessFullScopesGranted()) for _, service := range status.GetServices() { printServiceStatus(ctx, service) diff --git a/shared/statushelper/statushelper_test.go b/shared/statushelper/statushelper_test.go index de49d38e..75cfe2da 100644 --- a/shared/statushelper/statushelper_test.go +++ b/shared/statushelper/statushelper_test.go @@ -399,6 +399,64 @@ func TestAgentEnabledAndRunningLinux(t *testing.T) { } } +func TestCheckScopes(t *testing.T) { + tests := []struct { + name string + scopes []string + requiredScopes []string + wantHasRequiredScopes bool + wantErr error + }{ + { + name: "nilScopesMatchNilRequiredScopes", + scopes: nil, + requiredScopes: nil, + wantHasRequiredScopes: true, + wantErr: nil, + }, + { + name: "zeroLengthScopesMatchZeroLengthRequiredScopes", + scopes: []string{}, + requiredScopes: []string{}, + wantHasRequiredScopes: true, + wantErr: nil, + }, + { + name: "scopesLengthDoesNotMatchRequiredScopesLength", + scopes: []string{"1"}, + requiredScopes: []string{"1", "2"}, + wantHasRequiredScopes: false, + wantErr: nil, + }, + { + name: "scopesDoNotMatchRequiredScopes", + scopes: []string{"1", "2"}, + requiredScopes: []string{"1", "3"}, + wantHasRequiredScopes: false, + wantErr: nil, + }, + { + name: "scopesMatchRequiredScopes", + scopes: []string{"1", "2"}, + requiredScopes: []string{"1", "2"}, + wantHasRequiredScopes: true, + wantErr: nil, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + hasRequiredScopes, err := CheckScopes(context.Background(), test.scopes, test.requiredScopes) + if !cmp.Equal(err, test.wantErr, cmpopts.EquateErrors()) { + t.Errorf("checkScopes(%s) returned err: %v, wantErr: %v", test.name, err, test.wantErr) + } + if diff := cmp.Diff(test.wantHasRequiredScopes, hasRequiredScopes); diff != "" { + t.Errorf("checkScopes(%s) returned unexpected hasRequiredScopes status diff (-want +got):\n%s", test.name, diff) + } + }) + } +} + func TestPrintStatus(t *testing.T) { tests := []struct { name string @@ -419,6 +477,7 @@ Agent Status: Configuration File: Configuration Valid: Error: could not determine status + Cloud API Access Full Scopes Granted: Error: could not determine status `, @@ -426,14 +485,15 @@ Agent Status: { name: "fullStatusAllSuccess", status: &spb.AgentStatus{ - AgentName: "Agent for SAP", - InstalledVersion: "3.6", - AvailableVersion: "3.6", - SystemdServiceEnabled: spb.State_SUCCESS_STATE, - SystemdServiceRunning: spb.State_SUCCESS_STATE, - ConfigurationFilePath: "/etc/google-cloud-sap-agent/configuration.json", - ConfigurationValid: spb.State_SUCCESS_STATE, - ConfigurationErrorMessage: "error: proto: (line 6:44): invalid value for bool field value: 2", + AgentName: "Agent for SAP", + InstalledVersion: "3.6", + AvailableVersion: "3.6", + SystemdServiceEnabled: spb.State_SUCCESS_STATE, + SystemdServiceRunning: spb.State_SUCCESS_STATE, + ConfigurationFilePath: "/etc/google-cloud-sap-agent/configuration.json", + ConfigurationValid: spb.State_SUCCESS_STATE, + ConfigurationErrorMessage: "error: proto: (line 6:44): invalid value for bool field value: 2", + CloudApiAccessFullScopesGranted: spb.State_SUCCESS_STATE, Services: []*spb.ServiceStatus{ { Name: "Process Metrics", @@ -511,6 +571,7 @@ Agent Status: Systemd Service Running: True Configuration File: /etc/google-cloud-sap-agent/configuration.json Configuration Valid: True + Cloud API Access Full Scopes Granted: True -------------------------------------------------------------------------------- Process Metrics: Enabled Status: Fully Functional @@ -536,14 +597,15 @@ What's New: https://cloud.google.com/solutions/sap/docs/agent-for-sap/whats-new { name: "fullStatusWithFailures", status: &spb.AgentStatus{ - AgentName: "Agent for SAP", - InstalledVersion: "3.5", - AvailableVersion: "3.6", - SystemdServiceEnabled: spb.State_FAILURE_STATE, - SystemdServiceRunning: spb.State_FAILURE_STATE, - ConfigurationFilePath: "/etc/google-cloud-sap-agent/configuration.json", - ConfigurationValid: spb.State_FAILURE_STATE, - ConfigurationErrorMessage: "error: proto: (line 6:44): invalid value for bool field value: 2", + AgentName: "Agent for SAP", + InstalledVersion: "3.5", + AvailableVersion: "3.6", + SystemdServiceEnabled: spb.State_FAILURE_STATE, + SystemdServiceRunning: spb.State_FAILURE_STATE, + ConfigurationFilePath: "/etc/google-cloud-sap-agent/configuration.json", + ConfigurationValid: spb.State_FAILURE_STATE, + ConfigurationErrorMessage: "error: proto: (line 6:44): invalid value for bool field value: 2", + CloudApiAccessFullScopesGranted: spb.State_FAILURE_STATE, Services: []*spb.ServiceStatus{ { Name: "Process Metrics", @@ -620,6 +682,7 @@ Agent Status: Configuration File: /etc/google-cloud-sap-agent/configuration.json Configuration Valid: False error: proto: (line 6:44): invalid value for bool field value: 2 + Cloud API Access Full Scopes Granted: False -------------------------------------------------------------------------------- Process Metrics: Enabled Status: Error: Cannot write to Cloud Monitoring, check IAM permissions