Skip to content

Hive 2485/mce 2.3: Backport AssumeRole, credential_process, and kubeconfig exec fixes#2432

Merged
openshift-merge-bot[bot] merged 4 commits intoopenshift:mce-2.3from
2uasimojo:HIVE-2485/mce-2.3
Aug 28, 2024
Merged

Hive 2485/mce 2.3: Backport AssumeRole, credential_process, and kubeconfig exec fixes#2432
openshift-merge-bot[bot] merged 4 commits intoopenshift:mce-2.3from
2uasimojo:HIVE-2485/mce-2.3

Conversation

@2uasimojo
Copy link
Member

@2uasimojo 2uasimojo commented Aug 27, 2024

Manual cherry-pick of #2400.

HIVE-2485

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 8f11ce3)

Conflicts:
        pkg/install/generate.go (hiveutil binary path changed, no longer
        relevant.)
(cherry picked from commit a64f325)
(cherry picked from commit 26d01f4)

Conflicts:
	pkg/installmanager/aws_credentials_test.go (deleted)
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 13ea4f4)
(cherry picked from commit bc783e2)
(cherry picked from commit 8702591)

Conflicts:
	contrib/pkg/utils/aws/aws.go (imports)
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)
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 df1ea18)
(cherry picked from commit b9d2ed9)
(cherry picked from commit 4d38f54)

Conflicts:
	pkg/remoteclient/kubeconfig.go (imports)
	pkg/remoteclient/remoteclient.go (imports)
@openshift-ci openshift-ci bot requested review from jstuever and lleshchi August 27, 2024 21:36
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2024
@codecov
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 27.42857% with 127 lines in your changes missing coverage. Please review.

Project coverage is 57.85%. Comparing base (8d7c444) to head (94f41c3).
Report is 5 commits behind head on mce-2.3.

Files with missing lines Patch % Lines
pkg/install/generate.go 2.08% 47 Missing ⚠️
contrib/pkg/utils/generic.go 0.00% 18 Missing ⚠️
pkg/controller/utils/secrets.go 61.76% 8 Missing and 5 partials ⚠️
pkg/awsclient/client.go 0.00% 12 Missing ⚠️
pkg/installmanager/installmanager.go 0.00% 9 Missing ⚠️
contrib/pkg/utils/aws/aws.go 0.00% 6 Missing ⚠️
.../controller/clusterdeployment/clusterprovisions.go 33.33% 4 Missing ⚠️
...lusterdeprovision/clusterdeprovision_controller.go 20.00% 4 Missing ⚠️
contrib/pkg/utils/openstack/openstack.go 0.00% 2 Missing ⚠️
contrib/pkg/utils/ovirt/ovirt.go 0.00% 2 Missing ⚠️
... and 9 more
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           mce-2.3    #2432      +/-   ##
===========================================
+ Coverage    57.80%   57.85%   +0.04%     
===========================================
  Files          186      185       -1     
  Lines        25335    25322      -13     
===========================================
+ Hits         14645    14649       +4     
+ Misses        9439     9425      -14     
+ Partials      1251     1248       -3     
Files with missing lines Coverage Δ
...roller/argocdregister/argocdregister_controller.go 50.50% <100.00%> (-0.22%) ⬇️
...roller/awsprivatelink/awsprivatelink_controller.go 67.62% <100.00%> (+0.27%) ⬆️
...controller/clusterclaim/clusterclaim_controller.go 63.59% <100.00%> (ø)
.../clusterdeployment/clusterdeployment_controller.go 62.35% <100.00%> (+0.15%) ⬆️
...kg/controller/clusterdeployment/clusterinstalls.go 75.23% <100.00%> (ø)
pkg/remoteclient/remoteclient.go 69.23% <100.00%> (+0.80%) ⬆️
contrib/pkg/utils/azure/azure.go 0.00% <0.00%> (ø)
contrib/pkg/utils/gcp/gcp.go 0.00% <0.00%> (ø)
pkg/controller/awsprivatelink/cleanup.go 46.10% <50.00%> (ø)
pkg/controller/clusterdeprovision/awsactuator.go 34.48% <0.00%> (ø)
... and 15 more

@2uasimojo
Copy link
Member Author

/override ci/prow/security

#2387 backport

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 28, 2024

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

Details

In response to this:

/override ci/prow/security

#2387 backport

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
Copy link
Contributor

openshift-ci bot commented Aug 28, 2024

@2uasimojo: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/security 94f41c3 link true /test security

Full PR test history. Your PR dashboard.

Details

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.

@2uasimojo
Copy link
Member Author

/assign @jstuever

@2uasimojo
Copy link
Member Author

/assign @dlom

@dlom
Copy link
Contributor

dlom commented Aug 28, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-merge-bot openshift-merge-bot bot merged commit 5df8e72 into openshift:mce-2.3 Aug 28, 2024
@2uasimojo 2uasimojo deleted the HIVE-2485/mce-2.3 branch August 29, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants