-
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
Fix regenerating recovery codes webauthn scope #40633
Fix regenerating recovery codes webauthn scope #40633
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 |
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.
This has regressed more than once, so it would be great to figure out a way to test it.
I've refactored the privilege-token-based re-authentication flow to ensure the Other than that, we'd need to add an e2e test that checks that the scope provided on the front end matches the scope expected on the backend, which I'm not sure is necessary. |
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.
Seems reasonable to me, but I think a frontend person should take a look.
setAttempt({ status: 'processing' }); | ||
|
||
if ('onMfaResponse' in props) { | ||
auth | ||
.getWebauthnResponse(scope) | ||
.getWebauthnResponse(props.challengeScope) |
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 document the challengeScope
field as irrelevant if we are going this way? (Ideally, the presence of an event handler should not even affect the logic here, but for the sake of making minimal changes, we can omit this.)
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.
The challengeScope is relevant here for getWebauthnResponse
, it's just not relevant for createPrivilegeTokenWithWebauthn
.
I agree that using the event handler's presence as a logic switch is odd, but the way it actually works is that props
can only be one of two different mutually exclusive types:
// MfaResponseProps defines a function
// that accepts a MFA response. No
// authentication has been done at this point.
type MfaResponseProps = BaseProps & {
onMfaResponse(res: MfaAuthnResponse): void;
/**
* The MFA challenge scope of the action to perform, as defined in webauthn.proto.
*/
challengeScope: MfaChallengeScope;
onAuthenticated?: never;
};
// DefaultProps defines a function that
// accepts a privilegeTokenId that is only
// obtained after MFA response has been
// validated.
type DefaultProps = BaseProps & {
onAuthenticated(privilegeTokenId: string): void;
onMfaResponse?: never;
// TODO(Joerger): change type to 'never' once it is no longer expected in /e
challengeScope?: MfaChallengeScope;
};
I'm not sure what additional info to document, so feel free to leave any specific suggestions, or resolve this comment if you're ok with merging as is.
* Always provide MANAGE_DEVICES webauthn scope for creating privileged tokens. * Make challenge scope exclusive to onMfaResponse flow. * Fix UI lint.
Changelog: Fix regenerating cloud account recovery codes
The
CreatePrivilegeToken
endpoint always expects theMANAGE_DEVICES
webauthn scope. Since regenerating recovery codes uses a privileged token instead of using a webauthn response directly, using theACCOUNT_RECOVERY
scope for this flow would result in an error:Note:
ACCOUNT_RECOVERY
is exclusively expected by theVerifyAccountRecovery
endpoint. It should be used for other one-shot recovery endpoints as well.Fixes #40528
TODO: follow up on
TODO
once https://github.com/gravitational/teleport.e/pull/3980 is merged as well.