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

feat: allow defining iam role session name #155

Merged

Conversation

matthewriedel-flux
Copy link
Contributor

Changelog

This is a small enhancement that allows overriding the default
assumed-from-leapp role session name for AWS IAM Chained Sessions. Retains the default if undefined.

Bugfixes

  • Fixed small typo in Electron window defaults comment

Enhancements

This is useful for organisations that share roles and alert on when
someone assumes them and use the role session name to differentiate
individuals.
Notes

This change adjusts the size of the main Electron window slightly to
accommodate the new field in the chained session setup page.

This is a small enhancement that allows overriding the default
`assumed-from-leapp` role session name for AWS IAM Chained Sessions.

This is useful for organisations that share roles and alert on when
someone assumes them and use the role session name to differentiate
individuals.

This change adjusts the size of the main Electron window slightly to
accommodate the new field in the chained session setup page.
@pethron
Copy link
Contributor

pethron commented Jul 29, 2021

I check out the PR and everything seems ok. I have some doubt about the UX as I think it could be better to have a global role session name (so you don't have to set it for each session) or to directly inherit it from the session alias instead to set for each chained role. What do you think?

@matthewriedel-flux
Copy link
Contributor Author

matthewriedel-flux commented Jul 29, 2021

I check out the PR and everything seems ok. I have some doubt about the UX as I think it could be better to have a global role session name (so you don't have to set it for each session) or to directly inherit it from the session alias instead to set for each chained role. What do you think?

A global session name might be a good idea (or the ability to change the default- I wasn't sure how the preferences worked).

If I understand how the session alias gets used, I'm not sure that would be ideal because the session alias aligns more with the role a person assumes, while (in our case) the session name has more to do with the person assuming the role.

For example, here's the current output for aws sts get-caller-identity:

aws sts get-caller-identity
{
    "UserId": "AROAABC123MIPY5ABC123:assumed-from-leapp",
    "Account": "123456789012",
    "Arn": "arn:aws:sts::123456789012:assumed-role/Admin/assumed-from-leapp"
}

After this change, it will look something like:

aws sts get-caller-identity
{
    "UserId": "AROAABC123MIPY5ABC123:[email protected]",
    "Account": "123456789012",
    "Arn": "arn:aws:sts::123456789012:assumed-role/Admin/[email protected]"
}

If we used the session alias, it might end up looking like:

aws sts get-caller-identity
{
    "UserId": "AROAABC123MIPY5ABC123:AdminRole",
    "Account": "123456789012",
    "Arn": "arn:aws:sts::123456789012:assumed-role/Admin/AdminRole"
}

Which doesn't resolve the problem of being able to uniquely identify the person who assumed the role.

@ericvilla
Copy link
Contributor

If I understand how the session alias gets used, I'm not sure that would be ideal because the session alias aligns more with the role a person assumes, while (in our case) the session name has more to do with the person assuming the role.

I totally agree with this statement which is aligned with the AssumeRole documentation when it talks about RoleSessionName:

Use the role session name to uniquely identify a session when the same role is assumed by different principals or for different reasons. In cross-account scenarios, the role session name is visible to, and can be logged by the account that owns the role. The role session name is also used in the ARN of the assumed role principal. This means that subsequent cross-account API requests that use the temporary security credentials will expose the role session name to the external account in their AWS CloudTrail logs.

So, the RoleSessionName specifies the context in which the Principal assumes a specific Role.

@matthewriedel-flux could it be useful to have a [email protected] RoleSessionName? I mean, this adds another info to the context, i.e. the fact that [email protected] assumed the role using Leapp. I used the + to respect AWS's [\w+=,.@-]* regex for this field.

Let me know what you think about that :-)

@ericvilla ericvilla merged commit 9e21ef2 into Noovolari:master Jul 30, 2021
@ericvilla
Copy link
Contributor

ericvilla commented Jul 30, 2021

Imo, the next step is to give the user the possibility to edit old sessions which have no RoleSessionName setup. In the meanwhile I will add a check that fallbacks to "assumed-from-leapp" in case of old sessions

@matthewriedel-flux
Copy link
Contributor Author

@matthewriedel-flux could it be useful to have a [email protected] RoleSessionName? I mean, this adds another info to the context, i.e. the fact that [email protected] assumed the role using Leapp. I used the + to respect AWS's [\w+=,.@-]* regex for this field.

Let me know what you think about that :-)

I think that would be totally fine. I was basing my regex on the documentation on this page: https://aws.amazon.com/blogs/security/easily-control-naming-individual-iam-role-sessions/ and I didn't see + in the list of valid characters. But when has AWS documentation ever steered me wrong before? ;)

ericvilla pushed a commit that referenced this pull request Sep 30, 2021
This is a small enhancement that allows overriding the default
`assumed-from-leapp` role session name for AWS IAM Chained Sessions.

This is useful for organisations that share roles and alert on when
someone assumes them and use the role session name to differentiate
individuals.

This change adjusts the size of the main Electron window slightly to
accommodate the new field in the chained session setup page.
@joebowbeer
Copy link
Contributor

@matthewriedel-flux Is it possible to edit old sessions which have no RoleSessionName configured?

@matthewriedel-flux
Copy link
Contributor Author

@matthewriedel-flux Is it possible to edit old sessions which have no RoleSessionName configured?

@joebowbeer : In the version of Leapp this was added to, it wasn't really possible to edit sessions once they were created (at least as far as I could tell), so the easiest thing is to delete and create a new one. I'm not sure if it's possible in the new UX, though, I haven't checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can I change SessionId to username when I start and end SSM Session?
4 participants