Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

2uasimojo
Copy link
Member

@2uasimojo 2uasimojo commented Jul 31, 2024

Manual cherry-picks of commits related to #2306, #2356, and #2366. See individual commit messages for details.

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.)
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)
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)
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)
@2uasimojo
Copy link
Member Author

/assign @jstuever
/cc @suhanime

Almost clean backport of these commits.

@openshift-ci openshift-ci bot requested a review from suhanime July 31, 2024 22:11
Copy link
Contributor

openshift-ci bot commented Aug 1, 2024

@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.

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

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

Project coverage is 58.60%. Comparing base (d9cd574) to head (750a486).

Files 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.6    #2389      +/-   ##
===========================================
+ Coverage    58.56%   58.60%   +0.04%     
===========================================
  Files          182      181       -1     
  Lines        25835    25822      -13     
===========================================
+ Hits         15130    15134       +4     
+ Misses        9429     9415      -14     
+ Partials      1276     1273       -3     
Files Coverage Δ
...roller/argocdregister/argocdregister_controller.go 50.50% <100.00%> (-0.22%) ⬇️
...roller/awsprivatelink/awsprivatelink_controller.go 68.09% <100.00%> (+0.27%) ⬆️
...controller/clusterclaim/clusterclaim_controller.go 63.59% <100.00%> (ø)
.../clusterdeployment/clusterdeployment_controller.go 66.78% <100.00%> (+0.12%) ⬆️
...kg/controller/clusterdeployment/clusterinstalls.go 75.57% <100.00%> (ø)
pkg/remoteclient/remoteclient.go 73.18% <100.00%> (+1.07%) ⬆️
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

@jstuever
Copy link
Contributor

jstuever commented Aug 1, 2024

/lgtm
/approve

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

jstuever commented Aug 1, 2024

/approve

Copy link
Contributor

openshift-ci bot commented Aug 1, 2024

[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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 68f8522 into openshift:mce-2.6 Aug 1, 2024
10 checks passed
@2uasimojo 2uasimojo deleted the HIVE-2485/mce-2.6 branch August 1, 2024 19:32
@2uasimojo
Copy link
Member Author

/cherry-pick mce-2.5

@openshift-cherrypick-robot

@2uasimojo: new pull request created: #2391

In response to this:

/cherry-pick mce-2.5

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.

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