Skip to content
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

Merged
merged 5 commits into from
Oct 31, 2024

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Oct 22, 2024

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.

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-47826.d3pp5qlev8mo18.amplifyapp.com

Copy link
Contributor

@espadolini espadolini left a 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 Show resolved Hide resolved
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from codingllama October 23, 2024 09:30
lib/auth/webauthntypes/webauthn.go Outdated Show resolved Hide resolved
}

// ChallengeExtensions is a json struct for [mfav1.ChallengeExtensions].
type ChallengeExtensions struct {
Copy link
Contributor

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?

Copy link
Contributor Author

@Joerger Joerger Oct 29, 2024

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?

Copy link
Contributor

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.

Copy link
Contributor

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).

Copy link
Contributor Author

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.

Copy link
Contributor Author

@Joerger Joerger Oct 30, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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!

lib/services/sso_mfa.go Outdated Show resolved Hide resolved
lib/auth/webauthntypes/webauthn.go Outdated Show resolved Hide resolved
@Joerger Joerger requested a review from codingllama October 29, 2024 01:34
@Joerger Joerger force-pushed the joerger/fix-challenge-extension-marshalling branch from 7fe29ea to 8651a04 Compare October 29, 2024 01:39
//
// The UserVerificationRequirement field from [mfav1.ChallengeExtensions]
// has been omitted as it's only relevant to WebAuthn/Passwordless.
type ChallengeExtensions struct {
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

}

// ChallengeExtensions is a json struct for [mfav1.ChallengeExtensions].
type ChallengeExtensions struct {
Copy link
Contributor

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.

@Joerger Joerger requested a review from codingllama October 30, 2024 18:48
@Joerger Joerger force-pushed the joerger/fix-challenge-extension-marshalling branch from 1caa224 to 75753c7 Compare October 30, 2024 19:47
lib/auth/mfa/mfa.go Outdated Show resolved Hide resolved
@Joerger Joerger added this pull request to the merge queue Oct 31, 2024
Merged via the queue into master with commit 030abbd Oct 31, 2024
40 checks passed
@Joerger Joerger deleted the joerger/fix-challenge-extension-marshalling branch October 31, 2024 20:17
@public-teleport-github-review-bot

@Joerger See the table below for backport results.

Branch Result
branch/v17 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants