feat: SSO MFA - support for OIDC MaxAge#47292
Conversation
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
Joerger
added
the
no-changelog
| // Prompt is an optional OIDC prompt. An empty string omits prompt. | ||
| // If not specified, it defaults to select_account for backwards compatibility. | ||
| string prompt = 5; | ||
| // MaxAge is the amount of time in nanoseconds that an IdP session is valid for. Defaults to |
There was a problem hiding this comment.
Can we make this a proper duration instead of requiring the value be defined in nanoseconds?
There was a problem hiding this comment.
It seems to cause issues - #29815 (comment)
I'll give it a try though.
There was a problem hiding this comment.
google.protobuf.Duration max_age = 6 [(gogoproto.stdduration) = true];Resulted in it not parsing from a string like 10s.
google.protobuf.Duration max_age = 6;Would not parse from an int or a string like 10s, seems completely broken. Looks like we map - Mgoogle/protobuf/duration.proto=github.com/gogo/protobuf/types, and gogoproto ofc has issues so I'm guessing changing this would be a huge pain.
google.protobuf.Duration max_age = 6 [
(gogoproto.stdduration) = true,
(gogoproto.casttype) = "Duration"
];This works, though I don't think it's any better than the int64 as it still relies on gogoproto to overrule the actual type. I think it's essentially the same, but we can go with this.
Edit: nevermind, this breaks the protobuf gen file, might work without the (gogoproto.stdduration) = true
Edit2: Nope that breaks too. I don't see a way forward other than int64.
@codingllama any opinion on this?
There was a problem hiding this comment.
codingllama any opinion on this?
Yes, gogo was a terrible idea. :P
Why does it not parse? In what way does it break?
No strong opinions on my part about using int64 here - I would prefer the stronger type but I know how much of a pain it can be on public-facing types (and on legacy/ to boot).
| return trace.BadParameter("max_age cannot be negative") | ||
| } | ||
| if maxAge.Round(time.Second) != maxAge { | ||
| return trace.BadParameter("max_age must be a multiple of seconds") |
There was a problem hiding this comment.
This error message might be a bit confusing to reason about for users
Joerger
force-pushed
the
joerger/oidc-mfa-max-age
branch
2 times, most recently
from
d9262bf to
607d120
Compare
|
🤖 Vercel preview here: https://docs-ekff81dyr-goteleport.vercel.app/docs/ver/preview |
Joerger
force-pushed
the
joerger/oidc-mfa-max-age
branch
from
607d120 to
ac5e044
Compare
|
🤖 Vercel preview here: https://docs-51ivk3mmk-goteleport.vercel.app/docs/ver/preview |
|
@eriktate friendly ping to review |
Joerger
force-pushed
the
joerger/oidc-mfa-max-age
branch
from
ac5e044 to
3771cdb
Compare
|
🤖 Vercel preview here: https://docs-gfjkm7cf4-goteleport.vercel.app/docs/ver/preview |
|
@eriktate reminder to review, it's a pretty small change and I have some other PRs depending on it |
Joerger
force-pushed
the
joerger/oidc-mfa-max-age
branch
from
3771cdb to
9899204
Compare
|
🤖 Vercel preview here: https://docs-c7vhdfccb-goteleport.vercel.app/docs/ver/preview |
eriktate
left a comment
eriktate
left a comment
There was a problem hiding this comment.
lgtm! Sorry about the delay 🙇
|
🤖 Vercel preview here: https://docs-ow5bgzbij-goteleport.vercel.app/docs/ver/preview |
4 participants
Part of the implementation of SSO MFA
Unlike with SAML, we won't add a way to turn off max_age completely. Instead admins can set max_age to a reasonable number like
8hor match whatever the IdP session TTL is for their configuration.