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

Truncate AssumeRole session name to API limits #44836

Conversation

joaoubaldo
Copy link
Contributor

The issue describes the problem in more detail but essentially Teleport does not have any role session name restriction when calling AWS AssumeRole API. This is causing an AWS API error to propagate to the client and prevents calling the AWS API through Teleport in some conditions (the issue is likely more frequent in trusted clusters configurations due to the way the role session name is computed).

This PR proposes a potential fix regarding the limitation above by truncating the role session name to match AWS AssumeRole API limits.

The error that is being propagated to the client is:

... roleSessionName - failed to satisfy constraint: Member must have length less than or equal to 64...

Issue #44833

@CLAassistant
Copy link

CLAassistant commented Jul 30, 2024

CLA assistant check
All committers have signed the CLA.

@joaoubaldo joaoubaldo force-pushed the 44833-truncate-assumerole-session-name branch 4 times, most recently from d1341ab to 16b65e8 Compare July 30, 2024 17:38
@zmb3 zmb3 changed the title 44833 Truncate AssumeRole session name to API limits Truncate AssumeRole session name to API limits Jul 30, 2024
@r0mant r0mant requested a review from greedy52 August 6, 2024 15:48
Copy link
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaoubaldo 👏

Thanks for making the fix. I can even reproduce this with a local cluster but with a very long username.

The fix is reasonable. The only issue is you have to use shortened name when searching cloud trails. We will do some improvements separately from this PR:

  • put a sts_session_name value in Teleport audit log for easier cross referencing
  • update our AWS guide on cloud trail search for leaf cluster and long names

(You don't have to worry about these. We can do it afterwards.)

As for the fix itself, it fixes the AWS console, but tsh aws still fails for the same reason. Could you apply the same fix here?

func (g *credentialsGetter) Get(_ context.Context, request GetCredentialsRequest) (*credentials.Credentials, error) {
logrus.Debugf("Creating STS session %q for %q.", request.SessionName, request.RoleARN)
return stscreds.NewCredentials(request.Provider, request.RoleARN,
func(cred *stscreds.AssumeRoleProvider) {
cred.RoleSessionName = request.SessionName
cred.Expiry.SetExpiration(request.Expiry, 0)

I will open a buddy PR once ready.

Thanks again for contributing!

lib/utils/aws/aws.go Outdated Show resolved Hide resolved
@joaoubaldo
Copy link
Contributor Author

@joaoubaldo 👏

Thanks for making the fix. I can even reproduce this with a local cluster but with a very long username.

The fix is reasonable. The only issue is you have to use shortened name when searching cloud trails. We will do some improvements separately from this PR:

  • put a sts_session_name value in Teleport audit log for easier cross referencing
  • update our AWS guide on cloud trail search for leaf cluster and long names

(You don't have to worry about these. We can do it afterwards.)

As for the fix itself, it fixes the AWS console, but tsh aws still fails for the same reason. Could you apply the same fix here?

func (g *credentialsGetter) Get(_ context.Context, request GetCredentialsRequest) (*credentials.Credentials, error) {
logrus.Debugf("Creating STS session %q for %q.", request.SessionName, request.RoleARN)
return stscreds.NewCredentials(request.Provider, request.RoleARN,
func(cred *stscreds.AssumeRoleProvider) {
cred.RoleSessionName = request.SessionName
cred.Expiry.SetExpiration(request.Expiry, 0)

I will open a buddy PR once ready.

Thanks again for contributing!

@greedy52 thanks for checking. Your suggestion about applying the fix to tsh aws was already in place right?

@joaoubaldo joaoubaldo force-pushed the 44833-truncate-assumerole-session-name branch from de163b8 to 247df92 Compare August 7, 2024 08:24
Co-authored-by: STeve (Xin) Huang <[email protected]>
@joaoubaldo joaoubaldo force-pushed the 44833-truncate-assumerole-session-name branch from 247df92 to 11d2169 Compare August 7, 2024 08:25
@joaoubaldo joaoubaldo requested a review from greedy52 August 7, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants