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.

Sign up for GitHub

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
Merged

Fix regenerating recovery codes webauthn scope #40633

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
95b5fd1
Always provide MANAGE_DEVICES webauthn scope for creating privileged …
Joerger Apr 17, 2024
7cee96f
Make challenge scope exclusive to onMfaResponse flow.
Joerger Apr 17, 2024
1ecfbc2
Fix UI lint.
Joerger Apr 22, 2024
220a431
Merge branch 'master' into joerger/fix-cloud-regenerate-account-recov…
Joerger Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion web/packages/teleport/src/Account/Account.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,6 @@ export function Account({
onAuthenticated={setToken}
onClose={hideReAuthenticate}
actionText="registering a new device"
challengeScope={MfaChallengeScope.MANAGE_DEVICES}
/>
)}
{EnterpriseComponent && (
Expand Down
4 changes: 1 addition & 3 deletions web/packages/teleport/src/Account/ManageDevices/AddAuthDeviceWizard/AddAuthDeviceWizard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,15 +130,13 @@ export function ReauthenticateStep({
onClose,
onAuthenticated: onAuthenticatedProp,
}: AddAuthDeviceWizardStepProps) {
const challengeScope = MfaChallengeScope.MANAGE_DEVICES;
const onAuthenticated = (privilegeToken: string) => {
onAuthenticatedProp(privilegeToken);
next();
};
const { attempt, clearAttempt, submitWithTotp, submitWithWebauthn } =
useReAuthenticate({
onAuthenticated,
challengeScope,
});
const mfaOptions = createMfaOptions({
auth2faType,
Expand All @@ -159,7 +157,7 @@ export function ReauthenticateStep({
e.preventDefault();
if (!validator.validate()) return;
if (mfaOption === 'webauthn') {
submitWithWebauthn(challengeScope);
submitWithWebauthn();
}
if (mfaOption === 'otp') {
submitWithTotp(authCode);
Expand Down
1 change: 0 additions & 1 deletion web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,4 @@ const props: State = {
onClose: () => null,
auth2faType: 'on',
actionText: 'performing this action',
challengeScope: MfaChallengeScope.UNSPECIFIED,
};
3 changes: 1 addition & 2 deletions web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ export function ReAuthenticate({
auth2faType,
preferredMfaType,
actionText,
challengeScope,
}: State) {
const [otpToken, setOtpToken] = useState('');
const mfaOptions = createMfaOptions({
Expand All @@ -61,7 +60,7 @@ export function ReAuthenticate({
e.preventDefault();

if (mfaOption?.value === 'webauthn') {
submitWithWebauthn(challengeScope);
submitWithWebauthn();
}
if (mfaOption?.value === 'otp') {
submitWithTotp(otpToken);
Expand Down
19 changes: 10 additions & 9 deletions web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import type { MfaAuthnResponse } from 'teleport/services/mfa';
// token, and after successfully obtaining the token, the function
// `onAuthenticated` will be called with this token.
export default function useReAuthenticate(props: Props) {
const { onClose, actionText = defaultActionText, challengeScope } = props;
const { onClose, actionText = defaultActionText } = props;

// Note that attempt state "success" is not used or required.
// After the user submits, the control is passed back
Expand All @@ -53,12 +53,12 @@ export default function useReAuthenticate(props: Props) {
.catch(handleError);
}

function submitWithWebauthn(scope: MfaChallengeScope) {
function submitWithWebauthn() {
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.

.then(webauthnResponse =>
props.onMfaResponse({ webauthn_response: webauthnResponse })
)
Expand All @@ -67,7 +67,7 @@ export default function useReAuthenticate(props: Props) {
}

auth
.createPrivilegeTokenWithWebauthn(scope)
.createPrivilegeTokenWithWebauthn()
.then(props.onAuthenticated)
.catch((err: Error) => {
// This catches a webauthn frontend error that occurs on Firefox and replaces it with a more helpful error message.
Expand Down Expand Up @@ -97,7 +97,6 @@ export default function useReAuthenticate(props: Props) {
auth2faType: cfg.getAuth2faType(),
preferredMfaType: cfg.getPreferredMfaType(),
actionText,
challengeScope,
onClose,
};
}
Expand All @@ -116,17 +115,17 @@ type BaseProps = {
*
* */
actionText?: string;
/**
* The MFA challenge scope of the action to perform, as defined in webauthn.proto.
*/
challengeScope: MfaChallengeScope;
};

// 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;
};

Expand All @@ -137,6 +136,8 @@ type MfaResponseProps = BaseProps & {
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;
};

export type Props = MfaResponseProps | DefaultProps;
Expand Down
5 changes: 3 additions & 2 deletions web/packages/teleport/src/services/auth/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,9 @@ const auth = {
);
},

createPrivilegeTokenWithWebauthn(scope: MfaChallengeScope) {
return auth.fetchWebAuthnChallenge({ scope }).then(res =>
createPrivilegeTokenWithWebauthn() {
// Creating privilege tokens always expects the MANAGE_DEVICES webauthn scope.
return auth.fetchWebAuthnChallenge({ scope: MfaChallengeScope.MANAGE_DEVICES }).then(res =>
api.post(cfg.api.createPrivilegeTokenPath, {
webauthnAssertionResponse: makeWebauthnAssertionResponse(res),
})
Expand Down
Loading