[VAULT-1986] Cap AWS Token TTL based on Default Lease TTL #12026
Conversation
|
|
vinay-gopalan
requested review from
jasonodonnell,
austingebauer,
fairclothjm and
MilenaHC
builtin/credential/aws/path_role.go
Outdated
| @@ -892,9 +892,11 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request | |||
| defaultLeaseTTL := b.System().DefaultLeaseTTL() | |||
| systemMaxTTL := b.System().MaxLeaseTTL() | |||
| if roleEntry.TokenTTL > defaultLeaseTTL { | |||
| roleEntry.TokenTTL = defaultLeaseTTL | |||
There was a problem hiding this comment.
How come we clamp TTL to at most the default? I would only expect clamping to be needed for max. Is .DefaultLeaseTTL() just a poorly named function?
There was a problem hiding this comment.
The function is correctly named, but this is existing behavior within the plugin that was never actually implemented. We can either remove it and only cap on max, or implement the original intent. Thoughts?
There was a problem hiding this comment.
In that case, personally I would err towards removing the warning rather than adding capping - it's the more correct and safer change.
There was a problem hiding this comment.
I agree with Tom's recommendation
There was a problem hiding this comment.
@jasonodonnell what do you mean by never implemented? The value from b.System().DefaultLeaseTTL() is determined by this method:
vault/vault/dynamic_system_view.go
Lines 124 to 127 in 2e0dcb7
There was a problem hiding this comment.
Sync'ed up with Vinay on this and it turns out that TTL calculation caps the max TTL but doesn't care about the ttl (or token_ttl) value as long as that's under the max. I'm fine with removing this. At some point we were capping the default but then removed it when we changed things to use tokenutil.
| resp.AddWarning(fmt.Sprintf("Given ttl of %d seconds greater than current mount/system default of %d seconds; ttl will be capped at login time", roleEntry.TokenTTL/time.Second, defaultLeaseTTL/time.Second)) | ||
| } | ||
| if roleEntry.TokenMaxTTL > systemMaxTTL { | ||
| roleEntry.TokenMaxTTL = systemMaxTTL | ||
| resp.AddWarning(fmt.Sprintf("Given max ttl of %d seconds greater than current mount/system default of %d seconds; max ttl will be capped at login time", roleEntry.TokenMaxTTL/time.Second, systemMaxTTL/time.Second)) |
There was a problem hiding this comment.
max ttl will be capped at login time
Is this now slightly inaccurate? I believe if you read the config back for the role, it will probably have stored the system max TTL instead of the specified max TTL right? So it's actually already been capped.
There was a problem hiding this comment.
Right, we should remove the about "at login". We're now setting this on the role.
There was a problem hiding this comment.
I believe if you read the config back for the role, it will probably have stored the system max TTL instead of the specified max TTL right?
I don't think that this is the case because we're not actually modifying/overriding roleEntry.TokenMaxTTL so the cap is actually determined at login time. The outdated diff looks like we were at one point but then we reverted back (which I think is the right call).
| t.Fatalf("resp: %#v, err: %v", resp, err) | ||
| } | ||
|
|
||
| roleReq.Operation = logical.ReadOperation |
There was a problem hiding this comment.
nit: It does make the tests more verbose, but I think as a general rule it's nice to treat structs as immutable in tests so that the definition of your requests is always fully defined near the call site, and you reduce the risk of surprising bugs from dependencies between parts of the test.
There was a problem hiding this comment.
LGTM, I do echo @tomhjp's question about whether or not we should have the capping code and raised this internally.
|
@jasonodonnell Here are the new updates after our Sync. |
changelog/12026.txt
Outdated
| @@ -0,0 +1,3 @@ | |||
| ```release-note:bug | |||
| auth/aws: Fix a bug where the Token TTL for AWS is not capped by Default Lease TTL. | |||
There was a problem hiding this comment.
Forgot about that, fixed now :)
* fix: cap token TTL at login time based on default lease TTL * add changelog file * patch: update warning messages to not include 'at login' * patch: remove default lease capping and test * update changelog * patch: revert warning message
…12252) * fix: cap token TTL at login time based on default lease TTL * add changelog file * patch: update warning messages to not include 'at login' * patch: remove default lease capping and test * update changelog * patch: revert warning message
…12026) * fix: cap token TTL at login time based on default lease TTL * add changelog file * patch: update warning messages to not include 'at login' * patch: remove default lease capping and test * update changelog * patch: revert warning message
This PR fixes a bug in capping the Token TTL for AWS based on the Default Lease TTL. Before, the Token TTL was not being capped by the default lease even though a warning displayed in the console stating that it will be capped.
This PR also includes a test for the bug fix, including some typo fixes in error messages.