-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Use json compatible struct for SSOMFASessionData
#47826
Conversation
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the followup 🙌
lib/services/sso_mfa.go
Outdated
} | ||
|
||
// ChallengeExtensions is a json struct for [mfav1.ChallengeExtensions]. | ||
type ChallengeExtensions struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we place this type into a less generic package? Why lib/services?
Should this be somewhere under api/types, with a single shared type between lib/services and lib/auth/webauthntypes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add lib/auth/mfa/session.go
. We could move the WebauthnSessionData
here too, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in a follow up, but I wouldn't worry too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also worth thinking about the import paths. lib/auth/webauthn pulling from lib/auth/webauthntypes is fine (the latter is a "leaf" package), but pulling from lib/auth/mfa not so much. I think I would leave it as-is (or consider a move to another leaf package, likely in api/, but as-is seems better for now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow, lib/auth/mfa
is a brand new leaf package which only imports the mfav1 proto, right? Also better to keep this out of api/
since it's purely for internal Teleport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually yeah, since this is more of a leaf pacakge than wantypes, I moved ChallengeExtensions
out of wantypes into lib/auth/mfa
. I'm going to move wantypes.SessionData
too, in the final commit, we could move it to a follow up PR if it's too much.
Edit: it's better to keep wantypes.SessionData after all so it's comment and helper functions aren't importing github.com/go-webauthn/webauthn
into lib/auth/mfa
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving SSOMFASessionData
out of lib/services
actually causes issues with /e. If we want to do this still, we can do it in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with a follow up for SSOMFASessionData. Thanks for looking into it, Brian!
7fe29ea
to
8651a04
Compare
lib/auth/mfa/session.go
Outdated
// | ||
// The UserVerificationRequirement field from [mfav1.ChallengeExtensions] | ||
// has been omitted as it's only relevant to WebAuthn/Passwordless. | ||
type ChallengeExtensions struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this duplicated just so it omits UserVerificationRequirement? Instead of duping could we use wantypes.ChallengeExtensions and document that UserVerificationRequirement is not used for SSOMFASessionData? WDYT?
If not, then call this one SSOChallengeExtensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, thinking more about semantics/import paths, calling it SSOChallengeExtensions may be a better choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Friendly ping for this one? Follow-up also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, I was able to move this to lib/auth/mfa
and use it for both session data types.
lib/services/sso_mfa.go
Outdated
} | ||
|
||
// ChallengeExtensions is a json struct for [mfav1.ChallengeExtensions]. | ||
type ChallengeExtensions struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in a follow up, but I wouldn't worry too much.
1caa224
to
75753c7
Compare
Follow up to #47647 in relation to this thread to make
SSOMFASessionData
durable to json changes in protobuf generation.I made the same change to Webauthn
SessionData
and it is backwards compatible.