Add support for external-id and session-name

[stefansedich]

This PR adds support for overriding the assumed roles session-name by use of a pod annotation, and also adds support for using an external-id when assuming the role.

For now I chose to simply use an annotation for overriding the session name, I felt that adding templating could be a future enhancement if it is desired.

It carries on the work from #399 and should help us finally close off #399 and #38

[stefansedich]

@pingles draft opened here adding support for external id and session naming, yet to write any tests around this but wanted some early input with the direction before I go any further.

[pingles]

Yeah this looks great to me.

I think with session name it would be good to change the default to include pod identifiers? It's been a while since I've looked at it, are there any limits on the size of the session names or formatting with / in them to build paths?

Other than that, looks like a neat solution so thank you!

[stefansedich]

Yeah this looks great to me.

I think with session name it would be good to change the default to include pod identifiers? It's been a while since I've looked at it, are there any limits on the size of the session names or formatting with / in them to build paths?

Other than that, looks like a neat solution so thank you!

I like the idea of using the pod name as a default, something like kiam-POD_NAME could be good, let me dig into naming limits and how that could fit in.

[pingles]

So having read the docs

An identifier for the assumed role session.
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.
The regex used to validate this parameter is a string of characters consisting of upper- and lower-case alphanumeric characters with no spaces. You can also include underscores or any of the following characters: =,.@-

Adding via annotation seems like a reasonable place to start. I also wonder if it's good to coerce the value into the expected format stated via their regex, or to have a default placeholder if it won't pass AWS' expectations.

What do you think @stefansedich?

[stefansedich]

So having read the docs

An identifier for the assumed role session.
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.
The regex used to validate this parameter is a string of characters consisting of upper- and lower-case alphanumeric characters with no spaces. You can also include underscores or any of the following characters: =,.@-

Adding via annotation seems like a reasonable place to start. I also wonder if it's good to coerce the value into the expected format stated via their regex, or to have a default placeholder if it won't pass AWS' expectations.

What do you think @stefansedich?

Thanks, I had that doco handy but linking it here reminds me to look at it deeper to ensure we only use session names that are appropriate 😄!

I like the idea of keeping it simple, just logging a warning if we cannot use the value provided and we can fallback to a default, which should hopefully land to something like kiam-POD-NAME as discussed.

[stefansedich]

@pingles the server already has the --session config option, if I am to use the pod name by default what do we do here? I wonder if we should support templating in this case where you could use something like --session={{ .name }} but an explicit annotation would still override this, this however does introduce complexity.

I am still unsure here what we do about validation, I am wondering if it does not pass the regex test if we just always default to kiam and log a warning as the user could use a bad value in --session still, it looks like there are no length limits on STS session names that I can see and based on their rules pod names should be valid.

I am just trying to avoid removing existing behavior assuming there might be users already using --session in some way or do we want to look at deprecating this config option and move to the pod name default?

[pingles]

I'm not sure. My feeling is that typically it's a question of whether we'd want cluster operators to configure, or people scheduling pods? As you say, does add complexity so maybe another option is that you can enable the use of the annotation via a flag?

Do you have an example of how you'd use this? @tombooth perhaps you have an idea about how we'd likely do the same and then use that to help guide the decisions.

[stefansedich]

I'm not sure. My feeling is that typically it's a question of whether we'd want cluster operators to configure, or people scheduling pods? As you say, does add complexity so maybe another option is that you can enable the use of the annotation via a flag?

Do you have an example of how you'd use this? @tombooth perhaps you have an idea about how we'd likely do the same and then use that to help guide the decisions.

My usecase is simple, we intend to just use pod annotations for things we want to override the session name at a pod level.

I personally am in favor of the simple route for this first release:

1. Keep the --session config option
2. Intrroduce the pod annotation which overrides anything set using --session
3. If the name fails validation based on the AWS regex, default to the string kiam and log a warning to the user

[pingles]

Thanks.

2. So, I'd say let's introduce a flag that enables the pod annotation support (thus allowing pod control to be turned off later) --enable-pod-session-name-annotation?
3. Rather than the regex, what about coercing the value, replacing any invalid characters with a hyphen? That feels easier than reporting an error in the pod events.

[stefansedich]

Thanks.

1. So, I'd say let's introduce a flag that enables the pod annotation support (thus allowing pod control to be turned off later) --enable-pod-session-name-annotation?
2. Rather than the regex, what about coercing the value, replacing any invalid characters with a hyphen? That feels easier than reporting an error in the pod events.

1. We can do that, would it default to off? I question how useful a flag even is, if the user does not annotate the existing behavior is used and things change only when they decide to use the new annotation. What benefit do we get adding a new flag?

2. Yup this works, will corece the value to what AWS wants.

[pingles]

I would say default to on but allow it to be disabled. Use case I’m thinking of is extending to support templates names that could be configured on the server rather than needing annotations in all deployments. But, that could be added later I guess. Good push back.

…

On Wed, 21 Oct 2020 at 18:44, Stefan Sedich ***@***.***> wrote: Thanks. 1. So, I'd say let's introduce a flag that enables the pod annotation support (thus allowing pod control to be turned off later) --enable-pod-session-name-annotation? 2. Rather than the regex, what about coercing the value, replacing any invalid characters with a hyphen? That feels easier than reporting an error in the pod events. 1. We can do that, would it default to off? I question how useful a flag even is, if the user does not annotate the existing behavior is used and things change only when they decide to use the new annotation. What benefit do we get adding a new flag? 2. Yup this works, will corece the value to what AWS wants. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#430 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAI7QLIBAT7QIEUDICTULSL4MYZANCNFSM4STYEEKA> .

[stefansedich]

I would say default to on but allow it to be disabled. Use case I’m thinking of is extending to support templates names that could be configured on the server rather than needing annotations in all deployments. But, that could be added later I guess. Good push back.
…
On Wed, 21 Oct 2020 at 18:44, Stefan Sedich @.***> wrote: Thanks. 1. So, I'd say let's introduce a flag that enables the pod annotation support (thus allowing pod control to be turned off later) --enable-pod-session-name-annotation? 2. Rather than the regex, what about coercing the value, replacing any invalid characters with a hyphen? That feels easier than reporting an error in the pod events. 1. We can do that, would it default to off? I question how useful a flag even is, if the user does not annotate the existing behavior is used and things change only when they decide to use the new annotation. What benefit do we get adding a new flag? 2. Yup this works, will corece the value to what AWS wants. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#430 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAI7QLIBAT7QIEUDICTULSL4MYZANCNFSM4STYEEKA .

👍 okies will get the coercing and tests done and get this open, I am not sure I will get time to wrap this up this week but will aim to have it ready for review next week.

[pingles]

That’s great. No worries on time, appreciate the contribution!! As you’re working let us know if you need a hand on anything, here or on Slack. On Wed, 21 Oct 2020 at 18:48, Stefan Sedich <[email protected]> wrote:

…

I would say default to on but allow it to be disabled. Use case I’m thinking of is extending to support templates names that could be configured on the server rather than needing annotations in all deployments. But, that could be added later I guess. Good push back. … <#m_5178614659173035544_> On Wed, 21 Oct 2020 at 18:44, Stefan Sedich *@*.***> wrote: Thanks. 1. So, I'd say let's introduce a flag that enables the pod annotation support (thus allowing pod control to be turned off later) --enable-pod-session-name-annotation? 2. Rather than the regex, what about coercing the value, replacing any invalid characters with a hyphen? That feels easier than reporting an error in the pod events. 1. We can do that, would it default to off? I question how useful a flag even is, if the user does not annotate the existing behavior is used and things change only when they decide to use the new annotation. What benefit do we get adding a new flag? 2. Yup this works, will corece the value to what AWS wants. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#430 (comment) <#430 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAI7QLIBAT7QIEUDICTULSL4MYZANCNFSM4STYEEKA . 👍 okies will get the coercing and tests done and get this open, I am not sure I will get time to wrap this up this week but will aim to have it ready for review next week. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#430 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAI7TWM7VAI67BZIB3UJLSL4NGLANCNFSM4STYEEKA> .

[pingles]

I suspect this needs to change (and thus also needs some tests to prove the behaviour).

The purpose was originally to find any Pods in the Kubernetes API server filled cache. It should probably take the RoleIdentity type and then use that string for the cache key.

I'd expect the failing test to be something like if we had 2 pods using the same role but different session names, when one of them is no longer active (but the other is) we'd expect the non-active session to no longer be retained. If it's just matching on Role name, then it would keep them active.

The behaviour isn't horrific (it'd just mean refreshing credentials past them needing to be used) but be good to make it match the rest? What do you think?

[stefansedich]

@pingles just so I have this correct, we want to change the pod cache to use something like indexRoleIdentity over the existing indexPodRole? where the existing cache of pods by only the role changes to be pods cached by the identity?

[pingles]

Yep, that's right. At the moment, I would expect us to have incorrect behaviour assuming a state of:

Pod{Name: "foo", Status: "active", Role: "reader", Session: "active-reader"}
Pod{Name: "bar", Status: "stopped", Role: "reader", Session: "stopped-reader"}

IsRoleActive for an identity of Identity{Role: "reader", Session: "stopped-reader"} would still return true (because the active Pod remains) despite the call checking a different identity.

I suspect we should also include ExternalID if that's included also?

[stefansedich]

Ok got it, I am thinking of just changing to build the Identity and using String on that for the key, will pull out constructing an identity to an IdentityFromPod function and go from there. Hold my 🍺

[pingles]

Sounds great to me 🤞

[stefansedich]

Okies hopefully I did this right, refactoring+tests added 🤞

[pingles]

Thanks @stefansedich, super excited to get this added.

[pingles]

Had a quick scan on my phone but looks good to me! I'll have another look through on my laptop later but all looks good to me.