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

Fix regenerating recovery codes webauthn scope #40633

Merged
merged 4 commits into from
Apr 22, 2024

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Apr 17, 2024

Changelog: Fix regenerating cloud account recovery codes

The CreatePrivilegeToken endpoint always expects the MANAGE_DEVICES webauthn scope. Since regenerating recovery codes uses a privileged token instead of using a webauthn response directly, using the ACCOUNT_RECOVERY scope for this flow would result in an error:

image

Note: ACCOUNT_RECOVERY is exclusively expected by the VerifyAccountRecovery 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.

@Joerger Joerger requested a review from codingllama April 17, 2024 19:13
@Joerger Joerger changed the title Fix regenerating recovery codes Fix regenerating recovery codes webauthn scope Apr 17, 2024
Copy link

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 changelog: followed by the changelog entries for the PR.

Copy link
Collaborator

@zmb3 zmb3 left a 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.

@Joerger
Copy link
Contributor Author

Joerger commented Apr 17, 2024

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 MANAGE_DEVICES scope expected by rpc CreatePrivilegeToken is always used.

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.

Copy link
Contributor

@codingllama codingllama left a 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)
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 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.)

Copy link
Contributor Author

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.

@Joerger Joerger enabled auto-merge April 22, 2024 17:54
@Joerger Joerger added this pull request to the merge queue Apr 22, 2024
Merged via the queue into master with commit 6b69472 Apr 22, 2024
40 checks passed
@Joerger Joerger deleted the joerger/fix-cloud-regenerate-account-recovery-codes branch April 22, 2024 18:10
@public-teleport-github-review-bot

@Joerger See the table below for backport results.

Branch Result
branch/v15 Failed

Joerger added a commit that referenced this pull request Apr 22, 2024
* Always provide MANAGE_DEVICES webauthn scope for creating privileged tokens.

* Make challenge scope exclusive to onMfaResponse flow.

* Fix UI lint.
github-merge-queue bot pushed a commit that referenced this pull request Apr 22, 2024
* Always provide MANAGE_DEVICES webauthn scope for creating privileged tokens.

* Make challenge scope exclusive to onMfaResponse flow.

* Fix UI lint.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recreating recovery codes through the account settings page doesn't work
4 participants