Add logic for using Auth.Period when handling auth login/renew requests #3677
Conversation
vault/request_handling.go
Outdated
| @@ -518,6 +531,11 @@ func (c *Core) handleLoginRequest(req *logical.Request) (retResp *logical.Respon | |||
| auth.Accessor = te.Accessor | |||
| auth.Policies = te.Policies | |||
|
|
|||
| // Set auth.TTL if not set, used below in RegisterAuth | |||
| if auth.TTL == 0 { | |||
| auth.TTL = tokenTTL | |||
There was a problem hiding this comment.
This can be set to be the period's value if that's present. Not sure if we want that, or have it set to DefaultLeaseTTL.
There was a problem hiding this comment.
tokenTTL should already be the period's value from above if it's present. Actually, I think what needs to happen here is that in all cases we need to set auth.TTL to the tokenTTL value. That will ensure that the expiration manager always uses the correct value.
There was a problem hiding this comment.
Yea, I'll do auth.TTL = te.TTL since these two should be set to the same value.
…iod against sys values on RenewToken
|
|
|
@calvn Indeed, but I can't think of a use case for having the period (not total lifetime of the token, just the period) be that long. |
|
Likewise. I can change the tests so that they pass, but I am a bit concerned that changing the current behavior might break people's role setup in the cases where they have period > max TTL. Their next token renewal would fail. |
|
@calvn shouldn't it simply cap the value rather than fail it? (Ideally with a warning in the response.) |
|
Yea it should so we're good here, my bad. |
* oss/master: Defer reader.Close that is used to determine sha256 changelog++ Avoid unseal failure if plugin backends fail to setup during postUnseal (#3686) Add logic for using Auth.Period when handling auth login/renew requests (#3677) plugins/database: use context with plugins that use database/sql package (#3691) changelog++ Fix plaintext backup in transit (#3692) Database gRPC plugins (#3666)
I started off with backend changes on approle only, and if the logic looks correct I'll update the other backend that uses period afterwards.