Skip to content

[v17] Fix WebUI Admin Action infinite retry with no MFA devices #51513

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

Merged
Joerger merged 1 commit into from
Jan 30, 2025
Merged

[v17] Fix WebUI Admin Action infinite retry with no MFA devices #51513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export function ChangePasswordWizard({
const reauthState = useReAuthenticate({
challengeScope: MfaChallengeScope.CHANGE_PASSWORD,
onMfaResponse: async mfaResponse =>
setWebauthnResponse(mfaResponse?.webauthn_response),
setWebauthnResponse(mfaResponse.webauthn_response),
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Before we were tolerant of null/undefined values, now we are not. Is that intended?

Copy link
Copy Markdown
Contributor Author

@Joerger Joerger Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, returning null/undefined is what caused this bug - causing the admin mfa retry logic to think we haven't tried MFA already. Here's the original PR description:

#49679 and subsequently #50570 introduced a change where getMfaChallengeResponse could return null | undefined when the user had no MFA challenges (no devices or not required). fetchJsonWithMfaAuthnRetry expects getMfaChallengeResponse to return {} in these cases, and will retry with MFA until it gets either an empty or non-empty object.

This PR fixes the issue by:

  1. addressing the recursive fetchJsonWithMfaAuthnRetry which could result in an infinite loop. It's no longer recursive so it will only retry once even if null or undefined is received.
  2. Reverting part of https://github.com/gravitational/teleport/pull/50570/files and instead making getMfaChallengeResponse return {} so we can properly determine at any point whether an mfa response is undefined or an empty response resulting from a no-op challenge attempt (no devices or not required).

});

const [reauthMethod, setReauthMethod] = useState<ReauthenticationMethod>();
Expand Down
6 changes: 3 additions & 3 deletions web/packages/teleport/src/lib/term/tty.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,16 +80,16 @@ class Tty extends EventEmitterMfaSender {
this.socket.send(bytearray.buffer);
}

sendChallengeResponse(resp: MfaChallengeResponse) {
sendChallengeResponse(data: MfaChallengeResponse) {
// we want to have the backend listen on a single message type
// for any responses. so our data will look like data.webauthn, data.sso, etc
// but to be backward compatible, we need to still spread the existing webauthn only fields
// as "top level" fields so old proxies can still respond to webauthn challenges.
// in 19, we can just pass "data" without this extra step
// TODO (avatus): DELETE IN 19.0.0
const backwardCompatibleData = {
...resp?.webauthn_response,
...resp,
...data.webauthn_response,
...data,
};
const encoded = this._proto.encodeChallengeResponse(
JSON.stringify(backwardCompatibleData)
Expand Down
26 changes: 16 additions & 10 deletions web/packages/teleport/src/services/api/api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { MfaChallengeResponse } from '../mfa';
import api, {
defaultRequestOptions,
getAuthHeaders,
Expand All @@ -24,9 +25,18 @@ import api, {
} from './api';

describe('api.fetch', () => {
const mockedFetch = jest.spyOn(global, 'fetch').mockResolvedValue({} as any); // we don't care about response
let mockedFetch: jest.SpiedFunction<typeof fetch>;
beforeEach(() => {
mockedFetch = jest
.spyOn(global, 'fetch')
.mockResolvedValue({ json: async () => ({}), ok: true } as Response); // we don't care about response
});

afterEach(() => {
jest.resetAllMocks();
});

const mfaResp = {
const mfaResp: MfaChallengeResponse = {
webauthn_response: {
id: 'some-id',
type: 'some-type',
Expand All @@ -43,18 +53,14 @@ describe('api.fetch', () => {
},
};

const customOpts = {
const customOpts: RequestInit = {
method: 'POST',
// Override the default header from `defaultRequestOptions`.
headers: {
Accept: 'application/json',
},
};

afterEach(() => {
jest.resetAllMocks();
});

test('default (no optional params provided)', async () => {
await api.fetch('/something');
expect(mockedFetch).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -156,7 +162,7 @@ describe('api.fetch', () => {
});
});

// The code below should guard us from changes to api.fetchJson which would cause it to lose type
// The code below should guard us from changes to api.fetchJsonWithMfaAuthnRetry which would cause it to lose type
// information, for example by returning `any`.

const fooService = {
Expand All @@ -171,13 +177,13 @@ const makeFoo = (): { foo: string } => {

// This is a bogus test to satisfy Jest. We don't even need to execute the code that's in the async
// function, we're interested only in the type system checking the code.
test('fetchJson does not return any', () => {
test('fetchJsonWithMfaAuthnRetry does not return any', () => {
async () => {
const result = await fooService.doSomething();
// Reading foo is correct. We add a bogus expect to satisfy Jest.
result.foo;

// @ts-expect-error If there's no error here, it means that api.fetchJson returns any, which it
// @ts-expect-error If there's no error here, it means that api.fetchJsonWithMfaAuthnRetry returns any, which it
// shouldn't.
result.bar;
};
Expand Down
109 changes: 54 additions & 55 deletions web/packages/teleport/src/services/api/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,66 +150,31 @@ const api = {
customOptions: RequestInit,
mfaResponse?: MfaChallengeResponse
): Promise<any> {
const response = await api.fetch(url, customOptions, mfaResponse);

let json;
try {
json = await response.json();
return await api.fetch(url, customOptions, mfaResponse);
} catch (err) {
// error reading JSON
const message = response.ok
? err.message
: `${response.status} - ${response.url}`;
throw new ApiError({ message, response, opts: { cause: err } });
}

if (response.ok) {
return json;
}

/** This error can occur in the edge case where a role in the user's certificate was deleted during their session. */
const isRoleNotFoundErr = isRoleNotFoundError(parseError(json));
if (isRoleNotFoundErr) {
websession.logoutWithoutSlo({
/* Don't remember location after login, since they may no longer have access to the page they were on. */
rememberLocation: false,
/* Show "access changed" notice on login page. */
withAccessChangedMessage: true,
});
return;
// Retry with MFA if we get an admin action MFA error.
if (!mfaResponse && isAdminActionRequiresMfaError(err)) {
mfaResponse = await api.getAdminActionMfaResponse();
return api.fetch(url, customOptions, mfaResponse);
} else {
throw err;
}
}
},

// Retry with MFA if we get an admin action missing MFA error.
const isAdminActionMfaError = isAdminActionRequiresMfaError(
parseError(json)
);
const shouldRetry = isAdminActionMfaError && !mfaResponse;
if (!shouldRetry) {
throw new ApiError({
message: parseError(json),
response,
proxyVersion: parseProxyVersion(json),
messages: json.messages,
});
}
async getAdminActionMfaResponse() {
const challenge = await auth.getMfaChallenge({
scope: MfaChallengeScope.ADMIN_ACTION,
});

let mfaResponseForRetry;
try {
const challenge = await auth.getMfaChallenge({
scope: MfaChallengeScope.ADMIN_ACTION,
});
mfaResponseForRetry = await auth.getMfaChallengeResponse(challenge);
} catch {
if (!challenge) {
throw new Error(
'Failed to fetch MFA challenge. Please connect a registered hardware key and try again. If you do not have a hardware key registered, you can add one from your account settings page.'
'This is an admin-level API request and requires MFA verification. Please try again with a registered MFA device. If you do not have an MFA device registered, you can add one in the account settings page.'
);
}

return api.fetchJsonWithMfaAuthnRetry(
url,
customOptions,
mfaResponseForRetry
);
return auth.getMfaChallengeResponse(challenge);
},

/**
Expand Down Expand Up @@ -254,7 +219,7 @@ const api = {
* @param mfaResponse if defined (eg: `fetchJsonWithMfaAuthnRetry`)
* will add a custom MFA header field that will hold the mfaResponse.
*/
fetch(
async fetch(
url: string,
customOptions: RequestInit = {},
mfaResponse?: MfaChallengeResponse
Expand All @@ -280,7 +245,41 @@ const api = {
}

// native call
return fetch(url, options);
const response = await fetch(url, options);

let json;
try {
json = await response.json();
} catch (err) {
// error reading JSON
const message = response.ok
? err.message
: `${response.status} - ${response.url}`;
throw new ApiError({ message, response, opts: { cause: err } });
}

if (response.ok) {
return json;
}

/** This error can occur in the edge case where a role in the user's certificate was deleted during their session. */
const isRoleNotFoundErr = isRoleNotFoundError(parseError(json));
if (isRoleNotFoundErr) {
websession.logoutWithoutSlo({
/* Don't remember location after login, since they may no longer have access to the page they were on. */
rememberLocation: false,
/* Show "access changed" notice on login page. */
withAccessChangedMessage: true,
});
return;
}

throw new ApiError({
message: parseError(json),
response,
proxyVersion: parseProxyVersion(json),
messages: json.messages,
});
},
};

Expand Down Expand Up @@ -326,8 +325,8 @@ export function getHostName() {
return location.hostname + (location.port ? ':' + location.port : '');
}

function isAdminActionRequiresMfaError(errMessage) {
return errMessage.includes(
function isAdminActionRequiresMfaError(err: Error) {
return err.message.includes(
'admin-level API request requires MFA verification'
);
}
Expand Down
17 changes: 9 additions & 8 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 @@ -238,7 +238,7 @@ const auth = {
.then(res => {
const request = {
action: 'accept',
webauthnAssertionResponse: res?.webauthn_response,
webauthnAssertionResponse: res.webauthn_response,
};

return api.put(cfg.getHeadlessSsoPath(transactionId), request);
Expand All @@ -254,11 +254,11 @@ const auth = {
},

// getChallenge gets an MFA challenge for the provided parameters. If is_mfa_required_req
// is provided and it is found that MFA is not required, returns null instead.
// is provided and it is found that MFA is not required, returns undefined instead.
async getMfaChallenge(
req: CreateAuthenticateChallengeRequest,
abortSignal?: AbortSignal
) {
): Promise<MfaAuthenticateChallenge | undefined> {
return api
.post(
cfg.api.mfaAuthnChallengePath,
Expand All @@ -274,13 +274,14 @@ const auth = {
},

// getChallengeResponse gets an MFA challenge response for the provided parameters.
// If is_mfa_required_req is provided and it is found that MFA is not required, returns null instead.
// If challenge is undefined or has no viable challenge options, returns empty response.
async getMfaChallengeResponse(
challenge: MfaAuthenticateChallenge,
mfaType?: DeviceType,
totpCode?: string
): Promise<MfaChallengeResponse | undefined> {
if (!challenge) return;
): Promise<MfaChallengeResponse> {
// No challenge, return empty response.
if (!challenge) return {};

// TODO(Joerger): If mfaType is not provided by a parent component, use some global context
// to display a component, similar to the one used in useMfa. For now we just default to
Expand Down Expand Up @@ -310,7 +311,7 @@ const auth = {
}

// No viable challenge, return empty response.
return;
return {};
},

async getWebAuthnChallengeResponse(
Expand Down Expand Up @@ -439,7 +440,7 @@ const auth = {
return auth
.getMfaChallenge({ scope, allowReuse, isMfaRequiredRequest }, abortSignal)
.then(challenge => auth.getMfaChallengeResponse(challenge, 'webauthn'))
.then(res => res?.webauthn_response);
.then(res => res.webauthn_response);
},

getMfaChallengeResponseForAdminAction(allowReuse?: boolean) {
Expand Down