Use md5 hash as role session name when assuming into customer's support role #283

AlexVulaj · 2023-12-08T16:55:10Z

What type of PR is this?

feature

What this PR does / Why we need it?

backplane-cli passes the SRE's email address as a role session name when assuming a customer's support role. We should not pass any SRE usernames to a customer when assuming into their account.

Which Jira/Github issue(s) does this PR fix?

https://issues.redhat.com/browse/OSD-19901

Special notes for your reviewer

Pre-checks (if applicable)

Ran unit tests locally
Validated the changes in a cluster
Included documentation changes with PR

codecov-commenter · 2023-12-08T17:01:34Z

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (52a842d) 46.83% compared to head (5d3d480) 45.87%.
Report is 32 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #283      +/-   ##
==========================================
- Coverage   46.83%   45.87%   -0.97%     
==========================================
  Files          53       53              
  Lines        3540     3758     +218     
==========================================
+ Hits         1658     1724      +66     
- Misses       1621     1759     +138     
- Partials      261      275      +14

Files	Coverage Δ
pkg/awsutil/sts.go	`53.38% <50.00%> (ø)`
cmd/ocm-backplane/cloud/common.go	`16.35% <0.00%> (-0.54%)`	⬇️

... and 5 files with indirect coverage changes

samanthajayasinghe · 2023-12-10T20:30:08Z

pkg/awsutil/sts.go

- assumeRoleRetryBackoff = 5 * time.Second
+ assumeRoleMaxRetries = 3
+ assumeRoleRetryBackoff = 5 * time.Second
+ assumeCustomerRoleSessionName = "RH-SRE"


Not a blocker, I feel that bp-cli may use ISV's for connecting to the cluster in the near future. In that case, this logic may not be accurate.
Ideally, we should move some of the assumed role logic to bp-api endpoint where API decide how to determine the session-name based on some other factors.

We've updated the API to determine the role session name that should be used, but the actual AssumeRole logic needs to be done client side since we use individual SRE credentials for the isolated backplane flow.

https://gitlab.cee.redhat.com/service/backplane-api/-/merge_requests/329

cmd/ocm-backplane/cloud/common.go

jharrington22 · 2023-12-11T21:38:52Z

cmd/ocm-backplane/cloud/common.go

+ roleArnSession := awsutil.RoleArnSession{RoleArn: namedRoleArnEntry.Arn}
+ if namedRoleArnEntry.Name == CustomerRoleArnName {
+ data := []byte(email)
+ roleArnSession.RoleSessionName = fmt.Sprintf("%x", md5.Sum(data)) //nolint:gosec


Is it possible to get this from the backplane API? My concern is that an update to this in the CLI or API may mean they diverge in the future, it would be good to keep this consistent.

It's definitely possible - it comes down to how quickly we want this out the door.

To go this route, we would add a new API endpoint that takes a string and returns an md5 hash, using the same existing functions that are used in the API today for performing that conversion. Then the bp cli would call that endpoint instead.

I'm happy to go that route, and then we can keep this PR in a "hold" until that API change gets released. Thoughts?

Actually another thought - since this is only for the new flow, we could update the response object of the existing API call to include a new property for the role session name to be used for the customer's support role. That would be substantially less overhead and allow us quicker turnover.

AlexVulaj · 2023-12-12T20:33:29Z

/hold

See this comment: https://issues.redhat.com/browse/OSD-19901?focusedId=23636518&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-23636518

…rt role

AlexVulaj · 2024-01-03T16:50:45Z

/unhold

API changes have been promoted to production https://gitlab.cee.redhat.com/service/app-interface/-/merge_requests/92517

bmeng · 2024-01-04T02:32:50Z

    "userIdentity": {
        "type": "AssumedRole",
        "principalId": "xxxxxx:RH-SRE-bmeng_sre.openshift",
        "arn": "arn:aws:sts::xxxxx:assumed-role/ManagedOpenShift-Support-98sn7d/RH-SRE-bmeng_sre.openshift",
        "accountId": "xxxxx",
        "accessKeyId": "xxxxx",
        "sessionContext": {
            "sessionIssuer": {
                "type": "Role",
                "principalId": "xxxxxx",
                "arn": "arn:aws:iam::xxxxx:role/ManagedOpenShift-Support-98sn7d",
                "accountId": "xxxxx",
                "userName": "ManagedOpenShift-Support-98sn7d"
            },

I still can see my user name in the cloudtrail, but it is not in the session part. I assume it is expected for now?

AlexVulaj · 2024-01-04T19:24:13Z

@bmeng since the ticket specifically calls out the session name, I think we can go forward from here. I've started a thread here for further guidance on the principal being visible in cloudtrail:
https://redhat-internal.slack.com/archives/C058T1YEDPV/p1704395248986019

bmeng · 2024-01-09T14:34:27Z

As discussed in slack, the scope for this pr should be limit the info from the session, for the rest parts we can cover them separately.
/lgtm

openshift-ci · 2024-01-09T14:40:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexVulaj, bmeng

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~~ [bmeng]

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

openshift-ci · 2024-01-09T14:54:07Z

@AlexVulaj: 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/test-infra repository. I understand the commands that are listed here.

openshift-ci bot requested review from bmeng and supreeth7 December 8, 2023 16:55

samanthajayasinghe reviewed Dec 10, 2023

View reviewed changes

AlexVulaj force-pushed the generic-role-session-name-assume-customer-role branch from 3bd1b49 to 657f5b3 Compare December 11, 2023 19:59

AlexVulaj changed the title ~~Use 'RH-SRE' as role session name when assuming into customer's support role~~ Use md5 hash as role session name when assuming into customer's support role Dec 11, 2023

AlexVulaj force-pushed the generic-role-session-name-assume-customer-role branch 5 times, most recently from 337f8bb to cd24a6d Compare December 11, 2023 20:26

AlexVulaj requested review from jharrington22 and samanthajayasinghe December 11, 2023 20:36

jharrington22 reviewed Dec 11, 2023

View reviewed changes

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

Use md5 hash as role session name when assuming into customer's suppo…

5d3d480

…rt role

AlexVulaj force-pushed the generic-role-session-name-assume-customer-role branch from cd24a6d to 5d3d480 Compare January 2, 2024 17:04

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

openshift-ci bot assigned bmeng Jan 9, 2024

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

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

openshift-merge-bot bot merged commit 4321f0b into openshift:main Jan 9, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use md5 hash as role session name when assuming into customer's support role #283

Use md5 hash as role session name when assuming into customer's support role #283

AlexVulaj commented Dec 8, 2023

codecov-commenter commented Dec 8, 2023 •

edited

Loading

samanthajayasinghe Dec 10, 2023

AlexVulaj Jan 2, 2024

jharrington22 Dec 11, 2023

AlexVulaj Dec 11, 2023

AlexVulaj Dec 11, 2023

AlexVulaj commented Dec 12, 2023

AlexVulaj commented Jan 3, 2024

bmeng commented Jan 4, 2024

AlexVulaj commented Jan 4, 2024

bmeng commented Jan 9, 2024

openshift-ci bot commented Jan 9, 2024

openshift-ci bot commented Jan 9, 2024

Use md5 hash as role session name when assuming into customer's support role #283

Use md5 hash as role session name when assuming into customer's support role #283

Conversation

AlexVulaj commented Dec 8, 2023

What type of PR is this?

What this PR does / Why we need it?

Which Jira/Github issue(s) does this PR fix?

Special notes for your reviewer

Pre-checks (if applicable)

codecov-commenter commented Dec 8, 2023 • edited Loading

Codecov Report

samanthajayasinghe Dec 10, 2023

Choose a reason for hiding this comment

AlexVulaj Jan 2, 2024

Choose a reason for hiding this comment

jharrington22 Dec 11, 2023

Choose a reason for hiding this comment

AlexVulaj Dec 11, 2023

Choose a reason for hiding this comment

AlexVulaj Dec 11, 2023

Choose a reason for hiding this comment

AlexVulaj commented Dec 12, 2023

AlexVulaj commented Jan 3, 2024

bmeng commented Jan 4, 2024

AlexVulaj commented Jan 4, 2024

bmeng commented Jan 9, 2024

openshift-ci bot commented Jan 9, 2024

openshift-ci bot commented Jan 9, 2024

codecov-commenter commented Dec 8, 2023 •

edited

Loading