From a64f325c7117006b3fedece30d8c6fd287e8fd27 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Fri, 7 Jun 2024 17:47:30 -0500 Subject: [PATCH 1/4] Replumb AssumeRole (AWS) Simplify the AssumeRole flow: Rather than doing it via `credential_process` as a callback from within the creds file used by the provision pod, flatten this out so the AssumeRole is done implicitly by the AWS SDK. This flow remains unchanged: The clusterdeployment controller: - Copies the service provider secret into the CD namespace - Creates an AWS credentials secret - Creates the provision pod The provision pod: - Loads the credentials secret - Projects the AWS config therein onto the file system - Invokes the installer The installer: - Creates an AWS client using that config file - Proceeds with installation Before this commit: The AWS config contained a `credential_process` which invoked `hiveutil install-manager aws-credentials` which... - Loaded the service provider secret - Created an AWS client - Used the client to AssumeRole and generate credentials with a 15m expiration - Printed the credentials to stdout in the format expected by AWS. Per AWS docs[1], the SDK will automatically rerun the `credential_process` before the expiration time to refresh the creds. With this commit: The clusterdeployment controller loads the service provider secret and folds it into the AWS config as a separate profile, referenced from the default via `source_profile`: ``` [default] source_profile = source role_arn = arn:aws:iam::123456789012:role/assume-role-customer [profile source] aws_access_key_id: ABCDEFGHIJKLMNOPQRST aws_secret_access_key: 1234567890abcdefghijklmnopqrstuvwxyz0123 role_arn = arn:aws:iam::210987654321:role/assume-role-provider ``` Per AWS docs[2], the SDK will use the source creds to AssumeRole to generate temporary creds, which it will automatically refresh as they expire -- i.e. natively performing the same function as `hiveutil install-manager aws-credentials`. [1] https://docs.aws.amazon.com/cli/v1/userguide/cli-configure-sourcing-external.html [2] https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-role.html HIVE-2485 HIVE-2529 (cherry picked from commit 8f11ce30f4b6e9e3d174a0e728012c7ea00eddcf) Conflicts: pkg/install/generate.go (hiveutil binary path changed, no longer relevant.) --- contrib/pkg/utils/aws/aws.go | 2 - docs/awsassumerolecreds.md | 2 +- .../clusterdeployment/clusterprovisions.go | 2 +- .../clusterdeprovision_controller.go | 2 +- pkg/install/generate.go | 48 ++++-- pkg/installmanager/aws_credentials.go | 157 ------------------ pkg/installmanager/aws_credentials_test.go | 34 ---- pkg/installmanager/installmanager.go | 1 - 8 files changed, 36 insertions(+), 212 deletions(-) delete mode 100644 pkg/installmanager/aws_credentials.go delete mode 100644 pkg/installmanager/aws_credentials_test.go diff --git a/contrib/pkg/utils/aws/aws.go b/contrib/pkg/utils/aws/aws.go index 8c4536ab642..45b52705eb1 100644 --- a/contrib/pkg/utils/aws/aws.go +++ b/contrib/pkg/utils/aws/aws.go @@ -262,8 +262,6 @@ func ConfigureCreds(c client.Client) { utils.ProjectToDir(credsSecret, constants.AWSCredsMount, constants.AWSConfigSecretKey) os.Setenv("AWS_CONFIG_FILE", filepath.Join(constants.AWSCredsMount, constants.AWSConfigSecretKey)) } - // Allow credential_process in the config file - os.Setenv("AWS_SDK_LOAD_CONFIG", "true") // Install cluster proxy trusted CA bundle utils.InstallCerts(constants.TrustedCABundleDir) } diff --git a/docs/awsassumerolecreds.md b/docs/awsassumerolecreds.md index f25330be715..ea1195e0013 100644 --- a/docs/awsassumerolecreds.md +++ b/docs/awsassumerolecreds.md @@ -26,7 +26,7 @@ Let's create a Secret in `hive` (target namespace in HiveConfig) with credential ```console $ cat aws-service-provider-config -[default] +[profile source] aws_access_key_id = XXXXXX aws_secret_access_key = XXXXX role_arn = arn:aws:iam::123456:role/hive-aws-service-provider diff --git a/pkg/controller/clusterdeployment/clusterprovisions.go b/pkg/controller/clusterdeployment/clusterprovisions.go index 0d819224d61..0fa59e3bfec 100644 --- a/pkg/controller/clusterdeployment/clusterprovisions.go +++ b/pkg/controller/clusterdeployment/clusterprovisions.go @@ -774,7 +774,7 @@ func (r *ReconcileClusterDeployment) setupAWSCredentialForAssumeRole(cd *hivev1. return nil } - return install.AWSAssumeRoleCLIConfig(r.Client, cd.Spec.Platform.AWS.CredentialsAssumeRole, install.AWSAssumeRoleSecretName(cd.Name), cd.Namespace, cd, r.scheme) + return install.AWSAssumeRoleConfig(r.Client, cd.Spec.Platform.AWS.CredentialsAssumeRole, install.AWSAssumeRoleSecretName(cd.Name), cd.Namespace, cd, r.scheme) } func (r *ReconcileClusterDeployment) watchClusterProvisions(mgr manager.Manager, c controller.Controller) error { diff --git a/pkg/controller/clusterdeprovision/clusterdeprovision_controller.go b/pkg/controller/clusterdeprovision/clusterdeprovision_controller.go index cc243c3b563..8220a216dbf 100644 --- a/pkg/controller/clusterdeprovision/clusterdeprovision_controller.go +++ b/pkg/controller/clusterdeprovision/clusterdeprovision_controller.go @@ -457,6 +457,6 @@ func (r *ReconcileClusterDeprovision) setupAWSCredentialForAssumeRole(cd *hivev1 return nil } - return install.AWSAssumeRoleCLIConfig(r.Client, cd.Spec.Platform.AWS.CredentialsAssumeRole, install.AWSAssumeRoleSecretName(cd.Name), cd.Namespace, cd, r.scheme) + return install.AWSAssumeRoleConfig(r.Client, cd.Spec.Platform.AWS.CredentialsAssumeRole, install.AWSAssumeRoleSecretName(cd.Name), cd.Namespace, cd, r.scheme) } diff --git a/pkg/install/generate.go b/pkg/install/generate.go index cc940116994..70ada73d727 100644 --- a/pkg/install/generate.go +++ b/pkg/install/generate.go @@ -74,23 +74,40 @@ func CopyAWSServiceProviderSecret(client client.Client, destNamespace string, en return controllerutils.CopySecret(client, src, dest, owner, scheme) } -// AWSAssumeRoleCLIConfig creates a secret that can assume the role using the hiveutil -// credential_process helper. -func AWSAssumeRoleCLIConfig(client client.Client, role *hivev1aws.AssumeRole, secretName, secretNamespace string, owner metav1.Object, scheme *runtime.Scheme) error { - cmd := "/usr/bin/hiveutil" - args := []string{"install-manager", "aws-credentials"} - args = append(args, []string{"--namespace", secretNamespace}...) - args = append(args, []string{"--role-arn", role.RoleARN}...) - if role.ExternalID != "" { - args = append(args, []string{"--external-id", role.ExternalID}...) +// AWSAssumeRoleConfig creates a secret with an AWS credentials file containing: +// - Role configuration for AssumeRole, pointing to... +// - A profile containing the source credentials for AssumeRole. +func AWSAssumeRoleConfig(client client.Client, role *hivev1aws.AssumeRole, secretName, secretNamespace string, owner metav1.Object, scheme *runtime.Scheme) error { + + // Credentials source + credsSecret := &corev1.Secret{} + if err := client.Get( + context.TODO(), + types.NamespacedName{ + Namespace: secretNamespace, + // TODO: DRY this string construction + Name: fmt.Sprintf("%s-%s", owner.GetName(), os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar)), + }, + credsSecret); err != nil { + return err } + // The old credential_process flow documented creating this with [default]. + // For backward compatibility, accept that, but convert to [profile source]. + sourceProfile := strings.Replace(string(credsSecret.Data[constants.AWSConfigSecretKey]), `[default]`, `[profile source]`, 1) - cmd = fmt.Sprintf("%s %s", cmd, strings.Join(args, " ")) + extID := "" + if role.ExternalID != "" { + extID = fmt.Sprintf("external_id = %s\n", role.ExternalID) + } - template := `[default] -credential_process = %s -` - data := fmt.Sprintf(template, cmd) + // Build the config file + configFile := fmt.Sprintf(`[default] +source_profile = source +role_arn = %s +%s +%s +`, + role.RoleARN, extID, sourceProfile) secret := &corev1.Secret{ TypeMeta: metav1.TypeMeta{ @@ -102,9 +119,10 @@ credential_process = %s Name: secretName, }, Data: map[string][]byte{ - constants.AWSConfigSecretKey: []byte(data), + constants.AWSConfigSecretKey: []byte(configFile), }, } + if err := controllerutil.SetOwnerReference(owner, secret, scheme); err != nil { return nil } diff --git a/pkg/installmanager/aws_credentials.go b/pkg/installmanager/aws_credentials.go deleted file mode 100644 index 3b1f497bd71..00000000000 --- a/pkg/installmanager/aws_credentials.go +++ /dev/null @@ -1,157 +0,0 @@ -package installmanager - -import ( - "bytes" - "context" - "encoding/json" - "fmt" - "io" - "os" - "time" - - "github.com/aws/aws-sdk-go/aws/credentials" - "github.com/aws/aws-sdk-go/aws/credentials/stscreds" - "github.com/pkg/errors" - "github.com/spf13/cobra" - corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - - contributils "github.com/openshift/hive/contrib/pkg/utils" - "github.com/openshift/hive/pkg/awsclient" - "github.com/openshift/hive/pkg/constants" -) - -// AWSCredentials is a supported external process credential provider as detailed in -// https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-sourcing-external.html -type AWSCredentials struct { - output io.WriteCloser - kubeClient client.Client - - ServiceProviderSecretName string - ServiceProviderSecretNamespace string - - RoleARN string - ExternalID string -} - -// NewInstallManagerAWSCredentials is the entrypoint to load credentials for AWS SDK -// using the service provider credentials. It supports the external process credential -// provider as mentioned in https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-sourcing-external.html -func NewInstallManagerAWSCredentials() *cobra.Command { - options := &AWSCredentials{} - cmd := &cobra.Command{ - Use: "aws-credentials", - Short: "Loads AWS credentials using the service provider credentials", - Long: "This loads the AWS credentials using the service provider credentials and then returns them in format defined in https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-sourcing-external.html", - Run: func(cmd *cobra.Command, args []string) { - if err := options.Complete(args); err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) - return - } - - if err := options.Validate(); err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) - return - } - - var err error - options.kubeClient, err = contributils.GetClient() - if err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) - return - } - - if err := options.Run(); err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) - return - } - }, - } - flags := cmd.Flags() - flags.StringVar(&options.ServiceProviderSecretNamespace, "namespace", "", "The namespace where the service provider secret is stored") - cmd.MarkFlagRequired("namespace") - flags.StringVar(&options.RoleARN, "role-arn", "", "The IAM role that should be assumed") - cmd.MarkFlagRequired("role-arn") - flags.StringVar(&options.ExternalID, "external-id", "", "External identifier required to assume the role specified.") - - return cmd -} - -// Validate the options -func (options *AWSCredentials) Validate() error { return nil } - -// Complete the options using the args -func (options *AWSCredentials) Complete(args []string) error { - options.output = os.Stdout - options.ServiceProviderSecretName = os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar) - return nil -} - -// Run runs the command using the options. -func (options *AWSCredentials) Run() error { - var secret *corev1.Secret - if options.ServiceProviderSecretName != "" { - secret = &corev1.Secret{} - if err := options.kubeClient.Get(context.TODO(), - client.ObjectKey{Namespace: options.ServiceProviderSecretNamespace, Name: options.ServiceProviderSecretName}, - secret); err != nil { - return errors.Wrap(err, "failed to get the service provider secret") - } - } - - sess, err := awsclient.NewSessionFromSecret(secret, "") - if err != nil { - return errors.Wrap(err, "failed to create AWS session") - } - - duration := stscreds.DefaultDuration - creds := stscreds.NewCredentials(sess, options.RoleARN, func(p *stscreds.AssumeRoleProvider) { - p.Duration = duration - if options.ExternalID != "" { - p.ExternalID = &options.ExternalID - } - }) - v, err := creds.Get() - if err != nil { - return errors.Wrap(err, "failed to Assume the require role") - } - - resp, err := newCredentialProcessResponse(v, time.Now().Add(-1*time.Minute).Add(duration)) - if err != nil { - return errors.Wrap(err, "failed to create response for credential process") - } - _, err = fmt.Fprint(options.output, resp) - return err -} - -func newCredentialProcessResponse(v credentials.Value, expiry time.Time) (string, error) { - resp := &credentialProcessResponse{ - Version: 1, - AccessKeyID: v.AccessKeyID, - SecretAccessKey: v.SecretAccessKey, - SessionToken: v.SessionToken, - Expiration: &expiry, - } - outRaw, err := json.Marshal(resp) - if err != nil { - return "", errors.Wrap(err, "failed to create the credential process response") - } - outRawCompact := &bytes.Buffer{} - if err := json.Compact(outRawCompact, outRaw); err != nil { - return "", errors.Wrap(err, "failed to compact the JSON response") - } - - return outRawCompact.String(), nil -} - -type credentialProcessResponse struct { - Version int - AccessKeyID string `json:"AccessKeyId"` - SecretAccessKey string - SessionToken string - Expiration *time.Time -} diff --git a/pkg/installmanager/aws_credentials_test.go b/pkg/installmanager/aws_credentials_test.go deleted file mode 100644 index 9e3a141337b..00000000000 --- a/pkg/installmanager/aws_credentials_test.go +++ /dev/null @@ -1,34 +0,0 @@ -package installmanager - -import ( - "testing" - "time" - - "github.com/aws/aws-sdk-go/aws/credentials" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestNewCredentialProcessResponse(t *testing.T) { - - cases := []struct { - name string - creds credentials.Value - resp string - }{{ - name: "valid credentials", - creds: credentials.Value{ - AccessKeyID: "ASX..ID...", - SecretAccessKey: "ASX..SECRET...", - SessionToken: "ASX..TOKEN...", - }, - resp: `{"Version":1,"AccessKeyId":"ASX..ID...","SecretAccessKey":"ASX..SECRET...","SessionToken":"ASX..TOKEN...","Expiration":"0001-01-01T00:00:00Z"}`, - }} - for _, test := range cases { - t.Run(test.name, func(t *testing.T) { - got, err := newCredentialProcessResponse(test.creds, time.Time{}) - require.NoError(t, err) - assert.Equal(t, test.resp, got) - }) - } -} diff --git a/pkg/installmanager/installmanager.go b/pkg/installmanager/installmanager.go index 0a0dbc98e6e..35d00df579b 100644 --- a/pkg/installmanager/installmanager.go +++ b/pkg/installmanager/installmanager.go @@ -207,7 +207,6 @@ SSH_PRIV_KEY_PATH: File system path of a file containing the SSH private key cor flags.StringVar(&im.WorkDir, "work-dir", "/output", "directory to use for all input and output") flags.StringVar(&im.LogsDir, "logs-dir", "/logs", "directory to use for all installer logs") - cmd.AddCommand(NewInstallManagerAWSCredentials()) return cmd } From bc783e263ee34f83d3e2f48ecd0f2f652cff96c3 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Sun, 14 Jul 2024 16:53:40 -0500 Subject: [PATCH 2/4] Forbid and convert credential_process As a security measure, check AWS config/credential files for `credential_process`, and explode if found. We used to use `credential_process` deliberately to AssumeRole for STS clusters. A prior commit switched this over to use a different mechanism, but existing clusters in the field may still be configured with the old mechanism in the relevant Secrets. Convert such Secrets to use the new mechanism. HIVE-2485 (cherry picked from commit 13ea4f477531cffcd351ef8b93786a3c94de1a84) --- contrib/pkg/utils/aws/aws.go | 20 ++++++- contrib/pkg/utils/azure/azure.go | 2 +- contrib/pkg/utils/gcp/gcp.go | 2 +- contrib/pkg/utils/generic.go | 48 +++++++++++++-- contrib/pkg/utils/openstack/openstack.go | 4 +- contrib/pkg/utils/ovirt/ovirt.go | 4 +- contrib/pkg/utils/vsphere/vsphere.go | 4 +- pkg/awsclient/client.go | 24 ++++++-- .../awsprivatelink_controller.go | 2 +- .../clusterdeployment/clusterprovisions.go | 12 ++-- .../clusterdeprovision/awsactuator.go | 5 +- .../clusterdeprovision_controller.go | 12 ++-- pkg/controller/dnszone/dnszone_controller.go | 3 +- pkg/controller/hibernation/aws_actuator.go | 4 +- .../machinepool/machinepool_controller.go | 3 +- pkg/controller/utils/secrets.go | 21 ++++++- pkg/install/generate.go | 60 ++++++++++++------- pkg/installmanager/installmanager.go | 16 ++--- 18 files changed, 171 insertions(+), 75 deletions(-) diff --git a/contrib/pkg/utils/aws/aws.go b/contrib/pkg/utils/aws/aws.go index 45b52705eb1..b8de1633b74 100644 --- a/contrib/pkg/utils/aws/aws.go +++ b/contrib/pkg/utils/aws/aws.go @@ -1,12 +1,14 @@ package aws import ( + "errors" "fmt" - corev1 "k8s.io/api/core/v1" "os" "path/filepath" "strings" + corev1 "k8s.io/api/core/v1" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" @@ -242,6 +244,16 @@ func GetAWSCreds(credsFile, defaultCredsFile string) (string, string, error) { return accessKeyIDValue.String(), secretAccessKeyValue.String(), nil } +var awsConfigForbidCredentialProcess utils.ProjectToDirFileFilter = func(key string, contents []byte) (basename string, newContents []byte, err error) { + // First, only process aws_config + bn, newContents, err := utils.ProjectOnlyTheseKeys(constants.AWSConfigSecretKey)(key, contents) + // If that passed, scrub for credential_process + if err == nil && bn != "" && awsclient.ContainsCredentialProcess(newContents) { + return "", nil, errors.New("credential_process is insecure and thus forbidden") + } + return bn, newContents, err +} + // ConfigureCreds loads a secret designated by the environment variables CLUSTERDEPLOYMENT_NAMESPACE // and CREDS_SECRET_NAME and configures AWS credential environment variables and config files // accordingly. @@ -258,10 +270,12 @@ func ConfigureCreds(c client.Client) { os.Setenv("AWS_SECRET_ACCESS_KEY", secret) } if config := credsSecret.Data[constants.AWSConfigSecretKey]; len(config) != 0 { - // Lay this down as a file - utils.ProjectToDir(credsSecret, constants.AWSCredsMount, constants.AWSConfigSecretKey) + // Lay this down as a file, but forbid credential_process + utils.ProjectToDir(credsSecret, constants.AWSCredsMount, awsConfigForbidCredentialProcess) os.Setenv("AWS_CONFIG_FILE", filepath.Join(constants.AWSCredsMount, constants.AWSConfigSecretKey)) } + // This would normally allow credential_process in the config file, but we checked for that above. + os.Setenv("AWS_SDK_LOAD_CONFIG", "true") // Install cluster proxy trusted CA bundle utils.InstallCerts(constants.TrustedCABundleDir) } diff --git a/contrib/pkg/utils/azure/azure.go b/contrib/pkg/utils/azure/azure.go index bf252533d97..c26ac7bc150 100644 --- a/contrib/pkg/utils/azure/azure.go +++ b/contrib/pkg/utils/azure/azure.go @@ -36,7 +36,7 @@ func ConfigureCreds(c client.Client) { if credsSecret == nil { return } - utils.ProjectToDir(credsSecret, constants.AzureCredentialsDir) + utils.ProjectToDir(credsSecret, constants.AzureCredentialsDir, nil) os.Setenv(constants.AzureCredentialsEnvVar, constants.AzureCredentialsDir+"/"+constants.AzureCredentialsName) // Install cluster proxy trusted CA bundle utils.InstallCerts(constants.TrustedCABundleDir) diff --git a/contrib/pkg/utils/gcp/gcp.go b/contrib/pkg/utils/gcp/gcp.go index 94693e1f980..b3be4616ad3 100644 --- a/contrib/pkg/utils/gcp/gcp.go +++ b/contrib/pkg/utils/gcp/gcp.go @@ -37,7 +37,7 @@ func ConfigureCreds(c client.Client) { if credsSecret == nil { return } - utils.ProjectToDir(credsSecret, constants.GCPCredentialsDir) + utils.ProjectToDir(credsSecret, constants.GCPCredentialsDir, nil) os.Setenv("GOOGLE_CREDENTIALS", constants.GCPCredentialsDir+"/"+constants.GCPCredentialsName) // Install cluster proxy trusted CA bundle utils.InstallCerts(constants.TrustedCABundleDir) diff --git a/contrib/pkg/utils/generic.go b/contrib/pkg/utils/generic.go index 31240ad2241..09403f235a8 100644 --- a/contrib/pkg/utils/generic.go +++ b/contrib/pkg/utils/generic.go @@ -150,16 +150,56 @@ func loadOrDie(c client.Client, nameEnvKey string, obj client.Object) bool { } +// ProjectToDirFileFilter is run by ProjectToDir for each key found in the obj. +// If the second return is an error, ProjectToDir will panic with it. Otherwise: +// If ProjectToDir should create the file, the first return should be the base +// name of the file in which the newContents should be written. +// If the file should be skipped, the first return should be the empty string. +type ProjectToDirFileFilter func(key string, contents []byte) (basename string, newContents []byte, err error) + +// projectDefault is a ProjectToDirFileFilter that causes ProjectToDir to create +// files for all keys in the obj, naming each file the same as its key. +var projectDefault ProjectToDirFileFilter = func(key string, contents []byte) (string, []byte, error) { + return key, contents, nil +} + +// ProjectOnlyTheseKeys returns a ProjectToDirFileFilter that instructs ProjectToDir +// to create only the files with the specified keys. Each file's name will be the +// same as the key. The error return is always nil. +func ProjectOnlyTheseKeys(keys ...string) ProjectToDirFileFilter { + return func(key string, contents []byte) (string, []byte, error) { + if len(keys) == 0 { + // Caller should use nil (projectDefault) instead, but meh. + return key, contents, nil + } + if slice.ContainsString(keys, key, nil) { + // A match, project the file with the key as the basename and unchanged contents + return key, contents, nil + } + // No match; skip this file + return "", nil, nil + } +} + // ProjectToDir simulates what happens when you mount a secret or configmap as a volume on a pod, creating // files named after each key under `dir` and populating them with the contents represented by the values. -func ProjectToDir(obj client.Object, dir string, keys ...string) { - write := func(filename string, bytes []byte) { - if len(keys) != 0 && !slice.ContainsString(keys, filename, nil) { +// This default behavior can be modified by specifying a non-nil filter to validate, skip, and/or rename +// the file corresponding to each key. +func ProjectToDir(obj client.Object, dir string, filter ProjectToDirFileFilter) { + write := func(key string, bytes []byte) { + if filter == nil { + filter = projectDefault + } + filename, newBytes, err := filter(key, bytes) + if err != nil { + panic(err) + } + if filename == "" { // Skip this key return } path := filepath.Join(dir, filename) - if err := os.WriteFile(path, bytes, 0400); err != nil { + if err := os.WriteFile(path, newBytes, 0400); err != nil { log.WithError(err).WithField("path", path).Fatal("Failed to write file") } } diff --git a/contrib/pkg/utils/openstack/openstack.go b/contrib/pkg/utils/openstack/openstack.go index 48c33bde3df..26e2d313700 100644 --- a/contrib/pkg/utils/openstack/openstack.go +++ b/contrib/pkg/utils/openstack/openstack.go @@ -39,10 +39,10 @@ func GetCreds(credsFile string) ([]byte, error) { // CREDS_SECRET_NAME, and CERTS_SECRET_NAME and configures OpenStack credential config files accordingly. func ConfigureCreds(c client.Client) { if credsSecret := utils.LoadSecretOrDie(c, "CREDS_SECRET_NAME"); credsSecret != nil { - utils.ProjectToDir(credsSecret, constants.OpenStackCredentialsDir) + utils.ProjectToDir(credsSecret, constants.OpenStackCredentialsDir, nil) } if certsSecret := utils.LoadSecretOrDie(c, "CERTS_SECRET_NAME"); certsSecret != nil { - utils.ProjectToDir(certsSecret, constants.OpenStackCertificatesDir) + utils.ProjectToDir(certsSecret, constants.OpenStackCertificatesDir, nil) utils.InstallCerts(constants.OpenStackCertificatesDir) } // Install cluster proxy trusted CA bundle diff --git a/contrib/pkg/utils/ovirt/ovirt.go b/contrib/pkg/utils/ovirt/ovirt.go index cc44a3feb59..e859220423a 100644 --- a/contrib/pkg/utils/ovirt/ovirt.go +++ b/contrib/pkg/utils/ovirt/ovirt.go @@ -36,11 +36,11 @@ func GetCreds(credsFile string) ([]byte, error) { // and config files accordingly. func ConfigureCreds(c client.Client) { if credsSecret := utils.LoadSecretOrDie(c, "CREDS_SECRET_NAME"); credsSecret != nil { - utils.ProjectToDir(credsSecret, constants.OvirtCredentialsDir) + utils.ProjectToDir(credsSecret, constants.OvirtCredentialsDir, nil) os.Setenv(constants.OvirtConfigEnvVar, constants.OvirtCredentialsDir+"/"+constants.OvirtCredentialsName) } if certsSecret := utils.LoadSecretOrDie(c, "CERTS_SECRET_NAME"); certsSecret != nil { - utils.ProjectToDir(certsSecret, constants.OvirtCertificatesDir) + utils.ProjectToDir(certsSecret, constants.OvirtCertificatesDir, nil) utils.InstallCerts(constants.OvirtCertificatesDir) } // Install cluster proxy trusted CA bundle diff --git a/contrib/pkg/utils/vsphere/vsphere.go b/contrib/pkg/utils/vsphere/vsphere.go index 2bc77a62691..accf3290442 100644 --- a/contrib/pkg/utils/vsphere/vsphere.go +++ b/contrib/pkg/utils/vsphere/vsphere.go @@ -21,10 +21,10 @@ func ConfigureCreds(c client.Client) { } // NOTE: I think this is only used for terminateWhenFilesChange(), which we don't really // care about anymore. Can we get rid of it? - utils.ProjectToDir(credsSecret, constants.VSphereCredentialsDir) + utils.ProjectToDir(credsSecret, constants.VSphereCredentialsDir, nil) } if certsSecret := utils.LoadSecretOrDie(c, "CERTS_SECRET_NAME"); certsSecret != nil { - utils.ProjectToDir(certsSecret, constants.VSphereCertificatesDir) + utils.ProjectToDir(certsSecret, constants.VSphereCertificatesDir, nil) utils.InstallCerts(constants.VSphereCertificatesDir) } // Install cluster proxy trusted CA bundle diff --git a/pkg/awsclient/client.go b/pkg/awsclient/client.go index 78d460e964f..c52ca98fec4 100644 --- a/pkg/awsclient/client.go +++ b/pkg/awsclient/client.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "os" + "regexp" "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" @@ -597,7 +598,10 @@ func NewSessionFromSecret(secret *corev1.Secret, region string) (*session.Sessio // Special case to not use a secret to gather credentials. if secret != nil { - config := awsCLIConfigFromSecret(secret) + config, err := awsCLIConfigFromSecret(secret) + if err != nil { + return nil, err + } f, err := os.CreateTemp("", "hive-aws-config") if err != nil { return nil, err @@ -626,18 +630,30 @@ func NewSessionFromSecret(secret *corev1.Secret, region string) (*session.Sessio return s, nil } +var credentialProcessRE = regexp.MustCompile(`\bcredential_process\b`) + +func ContainsCredentialProcess(config []byte) bool { + return len(credentialProcessRE.Find(config)) != 0 +} + // awsCLIConfigFromSecret returns an AWS CLI config using the data available in the secret. -func awsCLIConfigFromSecret(secret *corev1.Secret) []byte { +func awsCLIConfigFromSecret(secret *corev1.Secret) ([]byte, error) { if config, ok := secret.Data[constants.AWSConfigSecretKey]; ok { - return config + if ContainsCredentialProcess(config) { + return nil, errors.New("credential_process is insecure and thus forbidden") + } + return config, nil } buf := &bytes.Buffer{} fmt.Fprint(buf, "[default]\n") for k, v := range secret.Data { + if k == "credential_process" { + return nil, errors.New("credential_process is insecure and thus forbidden") + } fmt.Fprintf(buf, "%s = %s\n", k, v) } - return buf.Bytes() + return buf.Bytes(), nil } func awsChinaEndpointResolver(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { diff --git a/pkg/controller/awsprivatelink/awsprivatelink_controller.go b/pkg/controller/awsprivatelink/awsprivatelink_controller.go index 04afd70b8b9..4cbad7abdde 100644 --- a/pkg/controller/awsprivatelink/awsprivatelink_controller.go +++ b/pkg/controller/awsprivatelink/awsprivatelink_controller.go @@ -1268,7 +1268,7 @@ func newAWSClient(r *ReconcileAWSPrivateLink, cd *hivev1.ClusterDeployment) (*aw }, AssumeRole: &awsclient.AssumeRoleCredentialsSource{ SecretRef: corev1.SecretReference{ - Name: os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar), + Name: controllerutils.AWSServiceProviderSecretName(""), Namespace: controllerutils.GetHiveNamespace(), }, Role: cd.Spec.Platform.AWS.CredentialsAssumeRole, diff --git a/pkg/controller/clusterdeployment/clusterprovisions.go b/pkg/controller/clusterdeployment/clusterprovisions.go index 0fa59e3bfec..32d6223965d 100644 --- a/pkg/controller/clusterdeployment/clusterprovisions.go +++ b/pkg/controller/clusterdeployment/clusterprovisions.go @@ -215,12 +215,8 @@ func (r *ReconcileClusterDeployment) startNewProvision( } if err := r.setupAWSCredentialForAssumeRole(cd); err != nil { - if !apierrors.IsAlreadyExists(err) { - // Couldn't create the assume role credential secret for a reason other than it already exists. - // If the secret already exists, then we should just use that secret. - logger.WithError(err).Error("could not create AWS assume role credential secret") - return reconcile.Result{}, err - } + logger.WithError(err).Error("could not create or update AWS assume role credential secret") + return reconcile.Result{}, err } r.expectations.ExpectCreations(types.NamespacedName{Namespace: cd.Namespace, Name: cd.Name}.String(), 1) @@ -750,7 +746,7 @@ func getInstallLogEnvVars(secretPrefix string) ([]corev1.EnvVar, error) { func getAWSServiceProviderEnvVars(cd *hivev1.ClusterDeployment, secretPrefix string) []corev1.EnvVar { var extraEnvVars []corev1.EnvVar - spSecretName := os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar) + spSecretName := controllerutils.AWSServiceProviderSecretName(secretPrefix) if spSecretName == "" { return extraEnvVars } @@ -761,7 +757,7 @@ func getAWSServiceProviderEnvVars(cd *hivev1.ClusterDeployment, secretPrefix str extraEnvVars = append(extraEnvVars, corev1.EnvVar{ Name: constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar, - Value: secretPrefix + "-" + spSecretName, + Value: spSecretName, }) return extraEnvVars } diff --git a/pkg/controller/clusterdeprovision/awsactuator.go b/pkg/controller/clusterdeprovision/awsactuator.go index b6c12229316..7786664a95a 100644 --- a/pkg/controller/clusterdeprovision/awsactuator.go +++ b/pkg/controller/clusterdeprovision/awsactuator.go @@ -1,8 +1,6 @@ package clusterdeprovision import ( - "os" - log "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -11,7 +9,6 @@ import ( hivev1 "github.com/openshift/hive/apis/hive/v1" awsclient "github.com/openshift/hive/pkg/awsclient" - "github.com/openshift/hive/pkg/constants" controllerutils "github.com/openshift/hive/pkg/controller/utils" ) @@ -61,7 +58,7 @@ func getAWSClient(cd *hivev1.ClusterDeprovision, c client.Client, logger log.Fie }, AssumeRole: &awsclient.AssumeRoleCredentialsSource{ SecretRef: corev1.SecretReference{ - Name: os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar), + Name: controllerutils.AWSServiceProviderSecretName(""), Namespace: controllerutils.GetHiveNamespace(), }, Role: cd.Spec.Platform.AWS.CredentialsAssumeRole, diff --git a/pkg/controller/clusterdeprovision/clusterdeprovision_controller.go b/pkg/controller/clusterdeprovision/clusterdeprovision_controller.go index 8220a216dbf..7099326435e 100644 --- a/pkg/controller/clusterdeprovision/clusterdeprovision_controller.go +++ b/pkg/controller/clusterdeprovision/clusterdeprovision_controller.go @@ -272,12 +272,8 @@ func (r *ReconcileClusterDeprovision) Reconcile(ctx context.Context, request rec } if err := r.setupAWSCredentialForAssumeRole(instance); err != nil { - if !errors.IsAlreadyExists(err) { - // Couldn't create the assume role credentials secret for a reason other than it already exists. - // If the secret already exists, then we should just use that secret. - rLog.WithError(err).Error("could not create assume role AWS secret") - return reconcile.Result{}, err - } + rLog.WithError(err).Error("could not create or update assume role AWS secret") + return reconcile.Result{}, err } if err := controllerutils.SetupClusterUninstallServiceAccount(r, cd.Namespace, rLog); err != nil { @@ -433,7 +429,7 @@ func (r *ReconcileClusterDeprovision) getActuator(cd *hivev1.ClusterDeprovision) func getAWSServiceProviderEnvVars(cd *hivev1.ClusterDeprovision, secretPrefix string) []corev1.EnvVar { var extraEnvVars []corev1.EnvVar - spSecretName := os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar) + spSecretName := controllerutils.AWSServiceProviderSecretName(secretPrefix) if spSecretName == "" { return extraEnvVars } @@ -444,7 +440,7 @@ func getAWSServiceProviderEnvVars(cd *hivev1.ClusterDeprovision, secretPrefix st extraEnvVars = append(extraEnvVars, corev1.EnvVar{ Name: constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar, - Value: secretPrefix + "-" + spSecretName, + Value: spSecretName, }) return extraEnvVars } diff --git a/pkg/controller/dnszone/dnszone_controller.go b/pkg/controller/dnszone/dnszone_controller.go index dc1413473f0..14bdc8ce6a6 100644 --- a/pkg/controller/dnszone/dnszone_controller.go +++ b/pkg/controller/dnszone/dnszone_controller.go @@ -16,7 +16,6 @@ import ( hivev1 "github.com/openshift/hive/apis/hive/v1" awsclient "github.com/openshift/hive/pkg/awsclient" "github.com/openshift/hive/pkg/azureclient" - "github.com/openshift/hive/pkg/constants" hivemetrics "github.com/openshift/hive/pkg/controller/metrics" controllerutils "github.com/openshift/hive/pkg/controller/utils" gcpclient "github.com/openshift/hive/pkg/gcpclient" @@ -399,7 +398,7 @@ func (r *ReconcileDNSZone) getActuator(dnsZone *hivev1.DNSZone, dnsLog log.Field AssumeRole: &awsclient.AssumeRoleCredentialsSource{ SecretRef: corev1.SecretReference{ Namespace: controllerutils.GetHiveNamespace(), - Name: os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar), + Name: controllerutils.AWSServiceProviderSecretName(""), }, Role: dnsZone.Spec.AWS.CredentialsAssumeRole, }, diff --git a/pkg/controller/hibernation/aws_actuator.go b/pkg/controller/hibernation/aws_actuator.go index 67a64891f1b..2455a6d3974 100644 --- a/pkg/controller/hibernation/aws_actuator.go +++ b/pkg/controller/hibernation/aws_actuator.go @@ -3,7 +3,6 @@ package hibernation import ( "context" "fmt" - "os" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" @@ -18,7 +17,6 @@ import ( hivev1 "github.com/openshift/hive/apis/hive/v1" awsclient "github.com/openshift/hive/pkg/awsclient" - "github.com/openshift/hive/pkg/constants" controllerutils "github.com/openshift/hive/pkg/controller/utils" ) @@ -261,7 +259,7 @@ func getAWSClient(cd *hivev1.ClusterDeployment, c client.Client, logger log.Fiel }, AssumeRole: &awsclient.AssumeRoleCredentialsSource{ SecretRef: corev1.SecretReference{ - Name: os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar), + Name: controllerutils.AWSServiceProviderSecretName(""), Namespace: controllerutils.GetHiveNamespace(), }, Role: cd.Spec.Platform.AWS.CredentialsAssumeRole, diff --git a/pkg/controller/machinepool/machinepool_controller.go b/pkg/controller/machinepool/machinepool_controller.go index 225b90e7802..288db1b977b 100644 --- a/pkg/controller/machinepool/machinepool_controller.go +++ b/pkg/controller/machinepool/machinepool_controller.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" "fmt" - "os" "reflect" "sort" "strconv" @@ -1162,7 +1161,7 @@ func (r *ReconcileMachinePool) createActuator( AssumeRole: &awsclient.AssumeRoleCredentialsSource{ SecretRef: corev1.SecretReference{ Namespace: controllerutils.GetHiveNamespace(), - Name: os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar), + Name: controllerutils.AWSServiceProviderSecretName(""), }, Role: cd.Spec.Platform.AWS.CredentialsAssumeRole, }, diff --git a/pkg/controller/utils/secrets.go b/pkg/controller/utils/secrets.go index 45bd125eda8..c5ad9f5ac37 100644 --- a/pkg/controller/utils/secrets.go +++ b/pkg/controller/utils/secrets.go @@ -3,13 +3,15 @@ package utils import ( "context" "fmt" + "os" + "github.com/openshift/hive/pkg/constants" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) -// LoadSecretData loads a given secret key and returns it's data as a string. +// LoadSecretData loads a given secret key and returns its data as a string. func LoadSecretData(c client.Client, secretName, namespace, dataKey string) (string, error) { s := &corev1.Secret{} err := c.Get(context.TODO(), types.NamespacedName{Name: secretName, Namespace: namespace}, s) @@ -22,3 +24,20 @@ func LoadSecretData(c client.Client, secretName, namespace, dataKey string) (str } return string(retStr), nil } + +// Generate the name of the Secret containing AWS Service Provider configuration. The +// prefix should be specified when the env var is known to refer to the global (hive +// namespace) Secret to convert it to the local (CD-specific) name. +// Beware of recursion: we're overloading the env var to refer to both the secret in the +// hive namespace and that in the CD-specific namespace. +func AWSServiceProviderSecretName(prefix string) string { + spSecretName := os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar) + // If no secret is given via env, this is n/a + if spSecretName == "" { + return "" + } + if prefix == "" { + return spSecretName + } + return prefix + "-" + spSecretName +} diff --git a/pkg/install/generate.go b/pkg/install/generate.go index 70ada73d727..930fe7afc5d 100644 --- a/pkg/install/generate.go +++ b/pkg/install/generate.go @@ -3,7 +3,7 @@ package install import ( "context" "fmt" - "os" + "reflect" "strconv" "strings" "time" @@ -15,6 +15,7 @@ import ( batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -50,7 +51,7 @@ func AWSAssumeRoleSecretName(secretPrefix string) string { func CopyAWSServiceProviderSecret(client client.Client, destNamespace string, envVars []corev1.EnvVar, owner metav1.Object, scheme *runtime.Scheme) error { hiveNS := controllerutils.GetHiveNamespace() - spSecretName := os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar) + spSecretName := controllerutils.AWSServiceProviderSecretName("") if spSecretName == "" { // If the src secret reference wasn't found, then don't attempt to copy the secret. return nil @@ -74,22 +75,22 @@ func CopyAWSServiceProviderSecret(client client.Client, destNamespace string, en return controllerutils.CopySecret(client, src, dest, owner, scheme) } -// AWSAssumeRoleConfig creates a secret with an AWS credentials file containing: +// AWSAssumeRoleConfig creates or updates a secret with an AWS credentials file containing: // - Role configuration for AssumeRole, pointing to... // - A profile containing the source credentials for AssumeRole. func AWSAssumeRoleConfig(client client.Client, role *hivev1aws.AssumeRole, secretName, secretNamespace string, owner metav1.Object, scheme *runtime.Scheme) error { // Credentials source credsSecret := &corev1.Secret{} + credsSecretName := controllerutils.AWSServiceProviderSecretName(owner.GetName()) if err := client.Get( context.TODO(), types.NamespacedName{ Namespace: secretNamespace, - // TODO: DRY this string construction - Name: fmt.Sprintf("%s-%s", owner.GetName(), os.Getenv(constants.HiveAWSServiceProviderCredentialsSecretRefEnvVar)), + Name: credsSecretName, }, credsSecret); err != nil { - return err + return errors.Wrapf(err, "failed to load credentials source secret %s", credsSecretName) } // The old credential_process flow documented creating this with [default]. // For backward compatibility, accept that, but convert to [profile source]. @@ -109,25 +110,44 @@ role_arn = %s `, role.RoleARN, extID, sourceProfile) - secret := &corev1.Secret{ - TypeMeta: metav1.TypeMeta{ - APIVersion: corev1.SchemeGroupVersion.String(), - Kind: "Secret", - }, - ObjectMeta: metav1.ObjectMeta{ - Namespace: secretNamespace, - Name: secretName, - }, - Data: map[string][]byte{ - constants.AWSConfigSecretKey: []byte(configFile), - }, + // Load the config secret if it already exists + configSecret := &corev1.Secret{} + if err := client.Get(context.TODO(), types.NamespacedName{Namespace: secretNamespace, Name: secretName}, configSecret); err != nil { + if !apierrors.IsNotFound(err) { + return errors.Wrapf(err, "failed to load config secret %s", secretName) + } + // Not found -- build it + configSecret = &corev1.Secret{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Secret", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: secretNamespace, + Name: secretName, + }, + Data: map[string][]byte{ + constants.AWSConfigSecretKey: []byte(configFile), + }, + } + if err := controllerutil.SetOwnerReference(owner, configSecret, scheme); err != nil { + return err + } + return client.Create(context.TODO(), configSecret) } - if err := controllerutil.SetOwnerReference(owner, secret, scheme); err != nil { + // Secret exists -- do we need to update it? Compare data and owner references. + origSecret := configSecret.DeepCopy() + configSecret.Data[constants.AWSConfigSecretKey] = []byte(configFile) + // SetOwnerReference is a no-op if the owner is already registered + if err := controllerutil.SetOwnerReference(owner, configSecret, scheme); err != nil { + return err + } + if reflect.DeepEqual(origSecret, configSecret) { return nil } - return client.Create(context.TODO(), secret) + return client.Update(context.TODO(), configSecret) } // InstallerPodSpec generates a spec for an installer pod. diff --git a/pkg/installmanager/installmanager.go b/pkg/installmanager/installmanager.go index 35d00df579b..a5a0bae3af0 100644 --- a/pkg/installmanager/installmanager.go +++ b/pkg/installmanager/installmanager.go @@ -550,26 +550,28 @@ func loadSecrets(m *InstallManager, cd *hivev1.ClusterDeployment) { } // Load up the install config and pull secret. These env vars are required; else we'll panic. - contributils.ProjectToDir(contributils.LoadSecretOrDie(m.DynamicClient, "INSTALLCONFIG_SECRET_NAME"), "/installconfig") - contributils.ProjectToDir(contributils.LoadSecretOrDie(m.DynamicClient, "PULLSECRET_SECRET_NAME"), "/pullsecret") + contributils.ProjectToDir(contributils.LoadSecretOrDie(m.DynamicClient, "INSTALLCONFIG_SECRET_NAME"), "/installconfig", nil) + contributils.ProjectToDir(contributils.LoadSecretOrDie(m.DynamicClient, "PULLSECRET_SECRET_NAME"), "/pullsecret", nil) // Additional manifests? Could come in on a Secret or a ConfigMap if manSecret := contributils.LoadSecretOrDie(m.DynamicClient, "MANIFESTS_SECRET_NAME"); manSecret != nil { - contributils.ProjectToDir(manSecret, "/manifests") + contributils.ProjectToDir(manSecret, "/manifests", nil) } else if manCM := contributils.LoadConfigMapOrDie(m.DynamicClient, "MANIFESTS_CONFIGMAP_NAME"); manCM != nil { - contributils.ProjectToDir(manCM, "/manifests") + contributils.ProjectToDir(manCM, "/manifests", nil) } // Custom BoundServiceAccountSigningKey if bsask := contributils.LoadSecretOrDie(m.DynamicClient, "BOUND_TOKEN_SIGNING_KEY_SECRET_NAME"); bsask != nil { - contributils.ProjectToDir(bsask, constants.BoundServiceAccountSigningKeyDir, constants.BoundServiceAccountSigningKeyFile) + contributils.ProjectToDir( + bsask, constants.BoundServiceAccountSigningKeyDir, + contributils.ProjectOnlyTheseKeys(constants.BoundServiceAccountSigningKeyFile)) os.Setenv(constants.BoundServiceAccountSigningKeyEnvVar, constants.BoundServiceAccountSigningKeyDir+"/"+constants.BoundServiceAccountSigningKeyFile) } // SSH private key if sshkey := contributils.LoadSecretOrDie(m.DynamicClient, "SSH_PRIVATE_KEY_SECRET_PATH"); sshkey != nil { - contributils.ProjectToDir(sshkey, constants.SSHPrivateKeyDir) + contributils.ProjectToDir(sshkey, constants.SSHPrivateKeyDir, nil) // TODO: Collapse this in initSSHKey os.Setenv(constants.SSHPrivKeyPathEnvVar, constants.SSHPrivateKeyDir+"/"+constants.SSHPrivateKeySecretKey) @@ -577,7 +579,7 @@ func loadSecrets(m *InstallManager, cd *hivev1.ClusterDeployment) { // BareMetal Libvirt SSH private key if sshkey := contributils.LoadSecretOrDie(m.DynamicClient, "LIBVIRT_SSH_KEYS_SECRET_NAME"); sshkey != nil { - contributils.ProjectToDir(sshkey, constants.LibvirtSSHPrivateKeyDir) + contributils.ProjectToDir(sshkey, constants.LibvirtSSHPrivateKeyDir, nil) os.Setenv(constants.LibvirtSSHPrivKeyPathEnvVar, constants.LibvirtSSHPrivateKeyDir+"/"+constants.SSHPrivateKeySecretKey) } From 3fc318b42fb6b89763713e990c7018ba9c84749c Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Fri, 19 Jul 2024 15:03:17 -0500 Subject: [PATCH 3/4] AWS: Forbid case-insensitive `credential_process` A previous commit (#2306 / 13ea4f47) put in checks to forbid the use of `credential_process` in AWS config/credentials files. It turns out that AWS accepts this key case-insensitively, so this commit updates our checks accordingly. HIVE-2485 (cherry picked from commit 229f7057b746e5713c84da96c244d74e4667ba4a) --- pkg/awsclient/client.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/awsclient/client.go b/pkg/awsclient/client.go index c52ca98fec4..252123ee70e 100644 --- a/pkg/awsclient/client.go +++ b/pkg/awsclient/client.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "regexp" + "strings" "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" @@ -630,7 +631,7 @@ func NewSessionFromSecret(secret *corev1.Secret, region string) (*session.Sessio return s, nil } -var credentialProcessRE = regexp.MustCompile(`\bcredential_process\b`) +var credentialProcessRE = regexp.MustCompile(`(?i)\bcredential_process\b`) func ContainsCredentialProcess(config []byte) bool { return len(credentialProcessRE.Find(config)) != 0 @@ -648,7 +649,7 @@ func awsCLIConfigFromSecret(secret *corev1.Secret) ([]byte, error) { buf := &bytes.Buffer{} fmt.Fprint(buf, "[default]\n") for k, v := range secret.Data { - if k == "credential_process" { + if strings.ToLower(k) == "credential_process" { return nil, errors.New("credential_process is insecure and thus forbidden") } fmt.Fprintf(buf, "%s = %s\n", k, v) From b9d2ed9acec3d266c16d6825f4c651fa6dc21ef2 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Wed, 17 Jul 2024 17:14:21 -0500 Subject: [PATCH 4/4] Forbid kubeconfig exec Users with write access to the admin kubeconfig Secret for a given ClusterDeployment should not be able to execute arbitrary code in the privileged environment in which we run the controllers that use those Secrets. Funnel all code paths that load such Secrets through a validator to ensure that the AuthInfos[].Exec path is not used. HIVE-2485 (cherry picked from commit df1ea18fbe5e57fca5f98383da0feb606b9afdc1) --- .../argocdregister_controller.go | 17 +----- .../awsprivatelink_controller.go | 30 +++------- pkg/controller/awsprivatelink/cleanup.go | 4 +- .../clusterclaim/clusterclaim_controller.go | 2 +- .../clusterdeployment_controller.go | 10 +++- .../clusterdeployment/clusterinstalls.go | 2 +- .../clusterdeployment/clusterprovisions.go | 2 +- pkg/controller/utils/secrets.go | 56 ++++++++++++++++++- pkg/remoteclient/kubeconfig.go | 3 +- pkg/remoteclient/remoteclient.go | 18 +----- 10 files changed, 83 insertions(+), 61 deletions(-) diff --git a/pkg/controller/argocdregister/argocdregister_controller.go b/pkg/controller/argocdregister/argocdregister_controller.go index 927d36ef394..602c1d25016 100644 --- a/pkg/controller/argocdregister/argocdregister_controller.go +++ b/pkg/controller/argocdregister/argocdregister_controller.go @@ -211,7 +211,7 @@ func (r *ArgoCDRegisterController) reconcileCluster(cd *hivev1.ClusterDeployment return reconcile.Result{}, nil } - kubeConfigSecretName := cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name + kubeConfigSecretName := cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name // HIVE-2485 ✓ argoCDServerConfigBytes, err := r.generateArgoCDServerConfig(kubeConfigSecretName, cd.Namespace, argoCDNamespace, cdLog) if err != nil { return reconcile.Result{}, err @@ -320,7 +320,7 @@ func (r *ArgoCDRegisterController) loadArgoCDServiceAccountToken(argoCDNamespace } func (r *ArgoCDRegisterController) generateArgoCDServerConfig(kubeconfigSecretName, kubeConfigSecretNamespace, argoCDNamespace string, cdLog log.FieldLogger) ([]byte, error) { - kubeconfig, err := r.loadSecretData(kubeconfigSecretName, kubeConfigSecretNamespace, adminKubeConfigKey) + kubeconfig, err := controllerutils.LoadSecretData(r.Client, kubeconfigSecretName, kubeConfigSecretNamespace, adminKubeConfigKey) if err != nil { cdLog.WithError(err).Error("unable to load cluster admin kubeconfig") return nil, err @@ -375,19 +375,6 @@ func tlsClientConfigBuilderFunc(kubeConfig clientcmd.ClientConfig, cdLog log.Fie return tlsClientConfig, nil } -func (r *ArgoCDRegisterController) loadSecretData(secretName, namespace, dataKey string) (string, error) { - s := &corev1.Secret{} - err := r.Get(context.TODO(), types.NamespacedName{Name: secretName, Namespace: namespace}, s) - if err != nil { - return "", err - } - retStr, ok := s.Data[dataKey] - if !ok { - return "", fmt.Errorf("secret %s did not contain key %s", secretName, dataKey) - } - return string(retStr), nil -} - // getPredictableSecretName generates a unique secret name by hashing the server API URL, // which is required as all cluster secrets land in the argocd namespace. // This code matches what is presently done in ArgoCD (util/db/cluster.go). However the actual diff --git a/pkg/controller/awsprivatelink/awsprivatelink_controller.go b/pkg/controller/awsprivatelink/awsprivatelink_controller.go index 4cbad7abdde..347342cb366 100644 --- a/pkg/controller/awsprivatelink/awsprivatelink_controller.go +++ b/pkg/controller/awsprivatelink/awsprivatelink_controller.go @@ -25,8 +25,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/rest" - "k8s.io/client-go/tools/clientcmd" "k8s.io/client-go/util/flowcontrol" "k8s.io/client-go/util/retry" "k8s.io/client-go/util/workqueue" @@ -301,7 +299,13 @@ func (r *ReconcileAWSPrivateLink) Reconcile(ctx context.Context, request reconci return reconcile.Result{}, nil } - return r.reconcilePrivateLink(cd, &hivev1.ClusterMetadata{InfraID: *cp.Spec.InfraID, AdminKubeconfigSecretRef: *cp.Spec.AdminKubeconfigSecretRef}, logger) + return r.reconcilePrivateLink( + cd, + &hivev1.ClusterMetadata{ + InfraID: *cp.Spec.InfraID, + AdminKubeconfigSecretRef: *cp.Spec.AdminKubeconfigSecretRef, // HIVE-2585: via ClusterMetadata + }, + logger) } // shouldSync returns if we should sync the desired ClusterDeployment. If it returns false, it also returns @@ -533,7 +537,7 @@ func (r *ReconcileAWSPrivateLink) reconcilePrivateLink(cd *hivev1.ClusterDeploym // Figure out the API address for cluster. apiDomain, err := initialURL(r.Client, - client.ObjectKey{Namespace: cd.Namespace, Name: clusterMetadata.AdminKubeconfigSecretRef.Name}) + client.ObjectKey{Namespace: cd.Namespace, Name: clusterMetadata.AdminKubeconfigSecretRef.Name}) // HIVE-2485 ✓ if err != nil { logger.WithError(err).Error("could not get API URL from kubeconfig") @@ -1303,7 +1307,7 @@ func initialURL(c client.Client, key client.ObjectKey) (string, error) { ); err != nil { return "", err } - cfg, err := restConfigFromSecret(kubeconfigSecret) + cfg, err := controllerutils.RestConfigFromSecret(kubeconfigSecret, true) if err != nil { return "", errors.Wrap(err, "failed to load the kubeconfig") } @@ -1315,22 +1319,6 @@ func initialURL(c client.Client, key client.ObjectKey) (string, error) { return strings.TrimSuffix(u.Hostname(), "."), nil } -func restConfigFromSecret(kubeconfigSecret *corev1.Secret) (*rest.Config, error) { - kubeconfigData := kubeconfigSecret.Data[constants.RawKubeconfigSecretKey] - if len(kubeconfigData) == 0 { - kubeconfigData = kubeconfigSecret.Data[constants.KubeconfigSecretKey] - } - if len(kubeconfigData) == 0 { - return nil, errors.New("kubeconfig secret does not contain necessary data") - } - config, err := clientcmd.Load(kubeconfigData) - if err != nil { - return nil, err - } - kubeConfig := clientcmd.NewDefaultClientConfig(*config, &clientcmd.ConfigOverrides{}) - return kubeConfig.ClientConfig() -} - // awsErrCodeEquals returns true if the error matches all these conditions: // - err is of type awserr.Error // - Error.Code() equals code diff --git a/pkg/controller/awsprivatelink/cleanup.go b/pkg/controller/awsprivatelink/cleanup.go index caa35d21bb5..d458a634272 100644 --- a/pkg/controller/awsprivatelink/cleanup.go +++ b/pkg/controller/awsprivatelink/cleanup.go @@ -61,7 +61,7 @@ func (r *ReconcileAWSPrivateLink) cleanupPreviousProvisionAttempt(cd *hivev1.Clu } metadata := &hivev1.ClusterMetadata{ InfraID: *cp.Spec.PrevInfraID, - AdminKubeconfigSecretRef: cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef, + AdminKubeconfigSecretRef: cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef, // HIVE-2485: via ClusterMetadata } if err := r.cleanupPrivateLink(cd, metadata, logger); err != nil { @@ -138,7 +138,7 @@ func (r *ReconcileAWSPrivateLink) cleanupHostedZone(awsClient awsclient.Client, if hzID == "" { // since we don't have the hz ID, we try to discover it to prevent leaks apiDomain, err := initialURL(r.Client, - client.ObjectKey{Namespace: cd.Namespace, Name: metadata.AdminKubeconfigSecretRef.Name}) + client.ObjectKey{Namespace: cd.Namespace, Name: metadata.AdminKubeconfigSecretRef.Name}) // HIVE-2485 ✓ if apierrors.IsNotFound(err) { logger.Info("no hostedZoneID in status and admin kubeconfig does not exist, skipping hosted zone cleanup") return nil diff --git a/pkg/controller/clusterclaim/clusterclaim_controller.go b/pkg/controller/clusterclaim/clusterclaim_controller.go index 13791378db0..d7a20c94a9e 100644 --- a/pkg/controller/clusterclaim/clusterclaim_controller.go +++ b/pkg/controller/clusterclaim/clusterclaim_controller.go @@ -549,7 +549,7 @@ func (r *ReconcileClusterClaim) applyHiveClaimOwnerRole(claim *hivev1.ClusterCla APIGroups: []string{corev1.GroupName}, Resources: []string{"secrets"}, ResourceNames: []string{ - cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name, + cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name, // HIVE-2485: n/a cd.Spec.ClusterMetadata.AdminPasswordSecretRef.Name, }, Verbs: []string{"get"}, diff --git a/pkg/controller/clusterdeployment/clusterdeployment_controller.go b/pkg/controller/clusterdeployment/clusterdeployment_controller.go index 9cf0c016b44..357356eea14 100644 --- a/pkg/controller/clusterdeployment/clusterdeployment_controller.go +++ b/pkg/controller/clusterdeployment/clusterdeployment_controller.go @@ -437,7 +437,14 @@ func (r *ReconcileClusterDeployment) addAdditionalKubeconfigCAs(cd *hivev1.Clust cdLog log.FieldLogger) error { adminKubeconfigSecret := &corev1.Secret{} - if err := r.Get(context.Background(), types.NamespacedName{Namespace: cd.Namespace, Name: cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name}, adminKubeconfigSecret); err != nil { + if err := r.Get( + context.Background(), + types.NamespacedName{ + Namespace: cd.Namespace, + // HIVE-2485: No need to scrub here + Name: cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name, + }, + adminKubeconfigSecret); err != nil { cdLog.WithError(err).Error("failed to get admin kubeconfig secret") return err } @@ -639,6 +646,7 @@ func (r *ReconcileClusterDeployment) reconcile(request reconcile.Request, cd *hi } // Add cluster deployment as additional owner reference to admin secrets + // HIVE-2485: No need to scrub here if err := r.addOwnershipToSecret(cd, cdLog, cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name); err != nil { return reconcile.Result{}, err } diff --git a/pkg/controller/clusterdeployment/clusterinstalls.go b/pkg/controller/clusterdeployment/clusterinstalls.go index f741209c578..ec7a51b1a99 100644 --- a/pkg/controller/clusterdeployment/clusterinstalls.go +++ b/pkg/controller/clusterdeployment/clusterinstalls.go @@ -54,7 +54,7 @@ func (r *ReconcileClusterDeployment) reconcileExistingInstallingClusterInstall(c if met := ci.Spec.ClusterMetadata; met != nil && met.InfraID != "" && met.ClusterID != "" && - met.AdminKubeconfigSecretRef.Name != "" && + met.AdminKubeconfigSecretRef.Name != "" && // HIVE-2485: via ClusterMetadata met.AdminPasswordSecretRef != nil && met.AdminPasswordSecretRef.Name != "" { if !reflect.DeepEqual(cd.Spec.ClusterMetadata, ci.Spec.ClusterMetadata) { diff --git a/pkg/controller/clusterdeployment/clusterprovisions.go b/pkg/controller/clusterdeployment/clusterprovisions.go index 32d6223965d..e138d4cd24f 100644 --- a/pkg/controller/clusterdeployment/clusterprovisions.go +++ b/pkg/controller/clusterdeployment/clusterprovisions.go @@ -419,7 +419,7 @@ func (r *ReconcileClusterDeployment) reconcileExistingProvision(cd *hivev1.Clust clusterMetadata.ClusterID = *provision.Spec.ClusterID } if provision.Spec.AdminKubeconfigSecretRef != nil { - clusterMetadata.AdminKubeconfigSecretRef = *provision.Spec.AdminKubeconfigSecretRef + clusterMetadata.AdminKubeconfigSecretRef = *provision.Spec.AdminKubeconfigSecretRef // HIVE-2485: via ClusterMetadata } if provision.Spec.AdminPasswordSecretRef != nil { clusterMetadata.AdminPasswordSecretRef = provision.Spec.AdminPasswordSecretRef diff --git a/pkg/controller/utils/secrets.go b/pkg/controller/utils/secrets.go index c5ad9f5ac37..b17138fe725 100644 --- a/pkg/controller/utils/secrets.go +++ b/pkg/controller/utils/secrets.go @@ -2,12 +2,16 @@ package utils import ( "context" + "errors" "fmt" "os" "github.com/openshift/hive/pkg/constants" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/clientcmd" + "k8s.io/client-go/tools/clientcmd/api" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -18,11 +22,59 @@ func LoadSecretData(c client.Client, secretName, namespace, dataKey string) (str if err != nil { return "", err } - retStr, ok := s.Data[dataKey] + retBytes, ok := s.Data[dataKey] if !ok { return "", fmt.Errorf("secret %s did not contain key %s", secretName, dataKey) } - return string(retStr), nil + switch dataKey { + case constants.KubeconfigSecretKey, constants.RawKubeconfigSecretKey: + if _, err := validateKubeconfig(retBytes); err != nil { + return "", err + } + } + return string(retBytes), nil +} + +// RestConfigFromSecret accepts a Secret containing `kubeconfig` and/or `raw-kubeconfig` keys +// and returns a rest.Config loaded therefrom. +// If tryRaw is true, we will look for `raw-kubeconfig` first and use it if present, falling +// back to `kubeconfig` otherwise. +// The error return is non-nil if: +// - The Secret's Data does not contain the [raw-]kubeconfig key(s) +// - The kubeconfig data cannot be Load()ed +// - The kubeconfig is insecure +func RestConfigFromSecret(kubeconfigSecret *corev1.Secret, tryRaw bool) (*rest.Config, error) { + var kubeconfigData []byte + if tryRaw { + kubeconfigData = kubeconfigSecret.Data[constants.RawKubeconfigSecretKey] + } + if len(kubeconfigData) == 0 { + kubeconfigData = kubeconfigSecret.Data[constants.KubeconfigSecretKey] + } + if len(kubeconfigData) == 0 { + return nil, errors.New("kubeconfig secret does not contain necessary data") + } + config, err := validateKubeconfig(kubeconfigData) + if err != nil { + return nil, err + } + kubeConfig := clientcmd.NewDefaultClientConfig(*config, &clientcmd.ConfigOverrides{}) + return kubeConfig.ClientConfig() +} + +// validateKubeconfig ensures the kubeconfig represented by kc does not use insecure paths. +// HIVE-2485 +func validateKubeconfig(kc []byte) (*api.Config, error) { + config, err := clientcmd.Load(kc) + if err != nil { + return nil, err + } + for k, ai := range config.AuthInfos { + if ai.Exec != nil { + return nil, fmt.Errorf("insecure exec in AuthInfos[%s]", k) + } + } + return config, nil } // Generate the name of the Secret containing AWS Service Provider configuration. The diff --git a/pkg/remoteclient/kubeconfig.go b/pkg/remoteclient/kubeconfig.go index e14061d72fa..205d438d61f 100644 --- a/pkg/remoteclient/kubeconfig.go +++ b/pkg/remoteclient/kubeconfig.go @@ -1,6 +1,7 @@ package remoteclient import ( + "github.com/openshift/hive/pkg/controller/utils" "github.com/openshift/hive/pkg/util/scheme" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/discovery" @@ -81,5 +82,5 @@ func (b *kubeconfigBuilder) UseSecondaryAPIURL() Builder { } func (b *kubeconfigBuilder) RESTConfig() (*rest.Config, error) { - return restConfigFromSecret(b.secret) + return utils.RestConfigFromSecret(b.secret, false) } diff --git a/pkg/remoteclient/remoteclient.go b/pkg/remoteclient/remoteclient.go index 6139e666c53..852e3564d99 100644 --- a/pkg/remoteclient/remoteclient.go +++ b/pkg/remoteclient/remoteclient.go @@ -18,11 +18,9 @@ import ( kubeclient "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "k8s.io/client-go/restmapper" - "k8s.io/client-go/tools/clientcmd" "sigs.k8s.io/controller-runtime/pkg/client" hivev1 "github.com/openshift/hive/apis/hive/v1" - "github.com/openshift/hive/pkg/constants" "github.com/openshift/hive/pkg/controller/utils" "github.com/openshift/hive/pkg/util/scheme" ) @@ -291,23 +289,11 @@ func unadulteratedRESTConfig(c client.Client, cd *hivev1.ClusterDeployment) (*re kubeconfigSecret := &corev1.Secret{} if err := c.Get( context.Background(), + // HIVE-2485 ✓ client.ObjectKey{Namespace: cd.Namespace, Name: cd.Spec.ClusterMetadata.AdminKubeconfigSecretRef.Name}, kubeconfigSecret, ); err != nil { return nil, errors.Wrap(err, "could not get admin kubeconfig secret") } - return restConfigFromSecret(kubeconfigSecret) -} - -func restConfigFromSecret(kubeconfigSecret *corev1.Secret) (*rest.Config, error) { - kubeconfigData, ok := kubeconfigSecret.Data[constants.KubeconfigSecretKey] - if !ok { - return nil, errors.Errorf("kubeconfig secret does not contain %q data", constants.KubeconfigSecretKey) - } - config, err := clientcmd.Load(kubeconfigData) - if err != nil { - return nil, err - } - kubeConfig := clientcmd.NewDefaultClientConfig(*config, &clientcmd.ConfigOverrides{}) - return kubeConfig.ClientConfig() + return utils.RestConfigFromSecret(kubeconfigSecret, false) }