Replumb AssumeRole (AWS) #2306

2uasimojo · 2024-06-12T20:30:39Z

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.

As a security measure, we will now check AWS config/credential files for credential_process, and explode if found. However, since existing clusters in the field may still be configured with the old mechanism in the relevant Secrets, we will convert such Secrets to use the new mechanism.

[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

cebarks · 2024-06-12T20:32:30Z

This issue has been assigned CVE-2024-25133 by Red Hat Product Security.

2uasimojo · 2024-06-12T21:38:04Z

/cc @abutcher @jharrington22
/assign @suhanime

2uasimojo · 2024-06-12T22:51:24Z

/retest

codecov · 2024-06-12T23:13:45Z

Codecov Report

Attention: Patch coverage is 6.50407% with 115 lines in your changes missing coverage. Please review.

Project coverage is 58.54%. Comparing base (1fce770) to head (13ea4f4).
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2306      +/-   ##
==========================================
+ Coverage   58.51%   58.54%   +0.02%     
==========================================
  Files         182      181       -1     
  Lines       25884    25863      -21     
==========================================
- Hits        15147    15141       -6     
+ Misses       9460     9447      -13     
+ Partials     1277     1275       -2

Files	Coverage Δ
...roller/awsprivatelink/awsprivatelink_controller.go	`67.81% <100.00%> (ø)`
contrib/pkg/utils/azure/azure.go	`0.00% <0.00%> (ø)`
contrib/pkg/utils/gcp/gcp.go	`0.00% <0.00%> (ø)`
pkg/controller/clusterdeprovision/awsactuator.go	`34.48% <0.00%> (ø)`
pkg/controller/dnszone/dnszone_controller.go	`28.79% <0.00%> (ø)`
pkg/controller/hibernation/aws_actuator.go	`64.36% <0.00%> (ø)`
...g/controller/machinepool/machinepool_controller.go	`53.48% <0.00%> (ø)`
contrib/pkg/utils/openstack/openstack.go	`0.00% <0.00%> (ø)`
contrib/pkg/utils/ovirt/ovirt.go	`0.00% <0.00%> (ø)`
contrib/pkg/utils/vsphere/vsphere.go	`0.00% <0.00%> (ø)`
... and 8 more

2uasimojo · 2024-06-12T23:21:30Z

/override ci/prow/security

Addressed via #2308

openshift-ci · 2024-06-12T23:21:47Z

@2uasimojo: Overrode contexts on behalf of 2uasimojo: ci/prow/security

In response to this:

/override ci/prow/security

Addressed via #2308

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

2uasimojo · 2024-06-12T23:33:18Z

/hold for testing

jharrington22 · 2024-06-19T10:54:37Z

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)


Is this the same path for deporovisions or do we need to generate credentials there too?

Deprovision path is below...

2uasimojo · 2024-06-20T20:12:43Z

pkg/controller/clusterdeprovision/clusterdeprovision_controller.go

@@ -478,6 +478,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)


jharrington22 · 2024-06-24T15:25:25Z

Overall LGTM @2uasimojo

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

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

jstuever · 2024-07-15T20:58:32Z

/cc

2uasimojo · 2024-07-15T20:58:37Z

/assign @jstuever

jstuever · 2024-07-15T20:58:38Z

/assign

jstuever · 2024-07-15T23:15:56Z

/hold for pre-merge testing

jstuever

/lgtm

openshift-ci · 2024-07-15T23:17:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, jstuever

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [2uasimojo,jstuever]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

2uasimojo · 2024-07-16T22:44:51Z

MachinePool scaling, hopefully a flake

/test e2e-gcp

2uasimojo · 2024-07-17T18:29:09Z

@jharrington22 agrees to merge.

/override "Red Hat Konflux / hive-on-pull-request"

/hold cancel

openshift-ci · 2024-07-17T18:29:28Z

@2uasimojo: Overrode contexts on behalf of 2uasimojo: Red Hat Konflux / hive-on-pull-request

In response to this:

@jharrington22 agrees to merge.

/override "Red Hat Konflux / hive-on-pull-request"

/hold cancel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci-robot · 2024-07-17T19:37:02Z

/retest-required

Remaining retests: 0 against base HEAD c5571a9 and 2 for PR HEAD 13ea4f4 in total

openshift-ci · 2024-07-17T22:54:48Z

@2uasimojo: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

A previous commit (openshift#2306 / 13ea4f4) 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

A previous commit (openshift#2306 / 13ea4f4) 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 229f705)

A previous commit (openshift#2306 / 13ea4f4) 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 229f705) (cherry picked from commit 3fc318b)

A previous commit (openshift#2306 / 13ea4f4) 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 229f705) (cherry picked from commit 3fc318b) (cherry picked from commit 26ef2e3)

openshift-ci bot requested review from dlom and suhanime June 12, 2024 20:31

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2024

openshift-ci bot assigned suhanime Jun 12, 2024

openshift-ci bot requested review from abutcher and jharrington22 June 12, 2024 21:38

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2024

jharrington22 reviewed Jun 20, 2024

View reviewed changes

2uasimojo commented Jun 20, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2024

2uasimojo force-pushed the HIVE-2485-2529/nix-credential_process branch from ffeac4a to 4d93e26 Compare July 12, 2024 15:31

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2024

2uasimojo force-pushed the HIVE-2485-2529/nix-credential_process branch from 7a260f9 to 9eeeb60 Compare July 15, 2024 20:21

2uasimojo force-pushed the HIVE-2485-2529/nix-credential_process branch from 9eeeb60 to 13ea4f4 Compare July 15, 2024 20:26

openshift-ci bot requested a review from jstuever July 15, 2024 20:58

openshift-ci bot assigned jstuever Jul 15, 2024

jstuever approved these changes Jul 15, 2024

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2024

openshift-merge-bot bot merged commit 5ba8466 into openshift:master Jul 17, 2024
12 of 13 checks passed

2uasimojo deleted the HIVE-2485-2529/nix-credential_process branch July 18, 2024 16:48

2uasimojo mentioned this pull request Jul 19, 2024

AWS: Forbid case-insensitive credential_process #2366

Merged

2uasimojo mentioned this pull request Jul 31, 2024

Hive 2485/mce 2.6: Backport AssumeRole, credential_process, and kubeconfig exec fixes #2389

Merged

GoVulnBot mentioned this pull request Dec 31, 2024

x/vulndb: potential Go vuln in github.com/openshift/hive: CVE-2024-25133 golang/vulndb#3360

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replumb AssumeRole (AWS) #2306

Replumb AssumeRole (AWS) #2306

2uasimojo commented Jun 12, 2024 •

edited

Loading

cebarks commented Jun 12, 2024 •

edited

Loading

2uasimojo commented Jun 12, 2024

2uasimojo commented Jun 12, 2024

codecov bot commented Jun 12, 2024 •

edited

Loading

2uasimojo commented Jun 12, 2024

openshift-ci bot commented Jun 12, 2024

2uasimojo commented Jun 12, 2024

jharrington22 Jun 19, 2024

2uasimojo Jun 20, 2024

2uasimojo Jun 20, 2024

jharrington22 commented Jun 24, 2024 •

edited

Loading

jstuever commented Jul 15, 2024

2uasimojo commented Jul 15, 2024

jstuever commented Jul 15, 2024

jstuever commented Jul 15, 2024

jstuever left a comment

openshift-ci bot commented Jul 15, 2024

2uasimojo commented Jul 16, 2024

2uasimojo commented Jul 17, 2024

openshift-ci bot commented Jul 17, 2024

openshift-ci-robot commented Jul 17, 2024

openshift-ci bot commented Jul 17, 2024

Replumb AssumeRole (AWS) #2306

Replumb AssumeRole (AWS) #2306

Conversation

2uasimojo commented Jun 12, 2024 • edited Loading

cebarks commented Jun 12, 2024 • edited Loading

2uasimojo commented Jun 12, 2024

2uasimojo commented Jun 12, 2024

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

2uasimojo commented Jun 12, 2024

openshift-ci bot commented Jun 12, 2024

2uasimojo commented Jun 12, 2024

jharrington22 Jun 19, 2024

Choose a reason for hiding this comment

2uasimojo Jun 20, 2024

Choose a reason for hiding this comment

2uasimojo Jun 20, 2024

Choose a reason for hiding this comment

jharrington22 commented Jun 24, 2024 • edited Loading

jstuever commented Jul 15, 2024

2uasimojo commented Jul 15, 2024

jstuever commented Jul 15, 2024

jstuever commented Jul 15, 2024

jstuever left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 15, 2024

2uasimojo commented Jul 16, 2024

2uasimojo commented Jul 17, 2024

openshift-ci bot commented Jul 17, 2024

openshift-ci-robot commented Jul 17, 2024

openshift-ci bot commented Jul 17, 2024

2uasimojo commented Jun 12, 2024 •

edited

Loading

cebarks commented Jun 12, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading

jharrington22 commented Jun 24, 2024 •

edited

Loading