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

[PM-7382] Add support for non-UUID credential #11993

Merged
merged 4 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export class Fido2AuthenticatorError extends Error {
}

export interface PublicKeyCredentialDescriptor {
id: BufferSource;
id: Uint8Array;
Copy link
Member

Choose a reason for hiding this comment

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

General question, should be interface be prescriptive enough about the ID to define it as Uint8Array, or use ArrayBuffer? I don't see an issue sticking with Uint8Array though.

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 actually commented this in the commit. I don't think it matters

refactor: change interface to use Uint8Array for simplification
Technically this deviates from the specification, but nobody is going to be using the authenticator directly but us so it shouldn't matter. We're gonna switch to passkey-rs anyways so

transports?: ("ble" | "hybrid" | "internal" | "nfc" | "usb")[];
type: "public-key";
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { compareCredentialIds, parseCredentialId } from "./credential-id-utils";

describe("credential-id-utils", () => {
describe("parseCredentialId", () => {
it("returns credentialId in binary format when given a valid UUID string", () => {
const result = parseCredentialId("08d70b74-e9f5-4522-a425-e5dcd40107e7");

expect(result).toEqual(
new Uint8Array([
0x08, 0xd7, 0x0b, 0x74, 0xe9, 0xf5, 0x45, 0x22, 0xa4, 0x25, 0xe5, 0xdc, 0xd4, 0x01, 0x07,
0xe7,
]),
);
});

it("returns credentialId in binary format when given a valid Base64Url string", () => {
const result = parseCredentialId("b64.CNcLdOn1RSKkJeXc1AEH5w");

expect(result).toEqual(
new Uint8Array([
0x08, 0xd7, 0x0b, 0x74, 0xe9, 0xf5, 0x45, 0x22, 0xa4, 0x25, 0xe5, 0xdc, 0xd4, 0x01, 0x07,
0xe7,
]),
);
});

it("returns undefined when given an invalid Base64 string", () => {
const result = parseCredentialId("b64.#$%&");

expect(result).toBeUndefined();
});

it("returns undefined when given an invalid UUID string", () => {
const result = parseCredentialId("invalid");

expect(result).toBeUndefined();
});
});

describe("compareCredentialIds", () => {
it("returns true when the two credential IDs are equal", () => {
const a = new Uint8Array([0x01, 0x02, 0x03]);
const b = new Uint8Array([0x01, 0x02, 0x03]);

const result = compareCredentialIds(a, b);

expect(result).toBe(true);
});

it("returns false when the two credential IDs are not equal", () => {
const a = new Uint8Array([0x01, 0x02, 0x03]);
const b = new Uint8Array([0x01, 0x02, 0x04]);

const result = compareCredentialIds(a, b);

expect(result).toBe(false);
});

it("returns false when the two credential IDs have different lengths", () => {
const a = new Uint8Array([0x01, 0x02, 0x03]);
const b = new Uint8Array([0x01, 0x02, 0x03, 0x04]);

const result = compareCredentialIds(a, b);

expect(result).toBe(false);
});
});
});
31 changes: 31 additions & 0 deletions libs/common/src/platform/services/fido2/credential-id-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { Fido2Utils } from "./fido2-utils";
import { guidToRawFormat } from "./guid-utils";

export function parseCredentialId(encodedCredentialId: string): Uint8Array {
try {
if (encodedCredentialId.startsWith("b64.")) {
Copy link
Member

Choose a reason for hiding this comment

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

I hate it, but let's run with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% agree

return Fido2Utils.stringToBuffer(encodedCredentialId.slice(4));
}

return guidToRawFormat(encodedCredentialId);
} catch {
return undefined;
}
}

/**
* Compares two credential IDs for equality.
*/
export function compareCredentialIds(a: Uint8Array, b: Uint8Array): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

We may benefit from having a general util function for comparing Uint8Arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but you've always stopped me when adding functions to the general utils in the past so now I'm going by "if we use it twice put it in general" XD

if (a.length !== b.length) {
return false;
}

for (let i = 0; i < a.length; i++) {
if (a[i] !== b[i]) {
return false;
}
}

return true;
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ import {
import { Utils } from "../../misc/utils";

import { CBOR } from "./cbor";
import { parseCredentialId } from "./credential-id-utils";
import { AAGUID, Fido2AuthenticatorService } from "./fido2-authenticator.service";
import { Fido2Utils } from "./fido2-utils";
import { guidToRawFormat } from "./guid-utils";

const RpId = "bitwarden.com";

Expand Down Expand Up @@ -139,7 +139,7 @@ describe("FidoAuthenticatorService", () => {
params = await createParams({
excludeCredentialDescriptorList: [
{
id: guidToRawFormat(excludedCipher.login.fido2Credentials[0].credentialId),
id: parseCredentialId(excludedCipher.login.fido2Credentials[0].credentialId),
type: "public-key",
},
],
Expand Down Expand Up @@ -482,7 +482,7 @@ describe("FidoAuthenticatorService", () => {
credentialId = Utils.newGuid();
params = await createParams({
allowCredentialDescriptorList: [
{ id: guidToRawFormat(credentialId), type: "public-key" },
{ id: parseCredentialId(credentialId), type: "public-key" },
],
rpId: RpId,
});
Expand Down Expand Up @@ -546,7 +546,7 @@ describe("FidoAuthenticatorService", () => {
let params: Fido2AuthenticatorGetAssertionParams;

beforeEach(async () => {
credentialIds = [Utils.newGuid(), Utils.newGuid()];
credentialIds = [Utils.newGuid(), "b64.Lb5SVTumSV6gYJpeWh3laA"];
ciphers = [
await createCipherView(
{ type: CipherType.Login },
Expand All @@ -559,7 +559,7 @@ describe("FidoAuthenticatorService", () => {
];
params = await createParams({
allowCredentialDescriptorList: credentialIds.map((credentialId) => ({
id: guidToRawFormat(credentialId),
id: parseCredentialId(credentialId),
type: "public-key",
})),
rpId: RpId,
Expand Down Expand Up @@ -667,7 +667,7 @@ describe("FidoAuthenticatorService", () => {
selectedCredentialId = credentialIds[0];
params = await createParams({
allowCredentialDescriptorList: credentialIds.map((credentialId) => ({
id: guidToRawFormat(credentialId),
id: parseCredentialId(credentialId),
type: "public-key",
})),
rpId: RpId,
Expand Down Expand Up @@ -723,7 +723,7 @@ describe("FidoAuthenticatorService", () => {
const flags = encAuthData.slice(32, 33);
const counter = encAuthData.slice(33, 37);

expect(result.selectedCredential.id).toEqual(guidToRawFormat(selectedCredentialId));
expect(result.selectedCredential.id).toEqual(parseCredentialId(selectedCredentialId));
expect(result.selectedCredential.userHandle).toEqual(
Fido2Utils.stringToBuffer(fido2Credentials[0].userHandle),
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ import { LogService } from "../../abstractions/log.service";
import { Utils } from "../../misc/utils";

import { CBOR } from "./cbor";
import { compareCredentialIds, parseCredentialId } from "./credential-id-utils";
import { p1363ToDer } from "./ecdsa-utils";
import { Fido2Utils } from "./fido2-utils";
import { guidToRawFormat, guidToStandardFormat } from "./guid-utils";
import { guidToStandardFormat } from "./guid-utils";

// AAGUID: d548826e-79b4-db40-a3d8-11116f7e8349
export const AAGUID = new Uint8Array([
Expand Down Expand Up @@ -178,7 +179,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr

const authData = await generateAuthData({
rpId: params.rpEntity.id,
credentialId: guidToRawFormat(credentialId),
credentialId: parseCredentialId(credentialId),
counter: fido2Credential.counter,
userPresence: true,
userVerification: userVerified,
Expand All @@ -193,7 +194,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
);

return {
credentialId: guidToRawFormat(credentialId),
credentialId: parseCredentialId(credentialId),
attestationObject,
authData,
publicKey: pubKeyDer,
Expand Down Expand Up @@ -313,7 +314,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr

const authenticatorData = await generateAuthData({
rpId: selectedFido2Credential.rpId,
credentialId: guidToRawFormat(selectedCredentialId),
credentialId: parseCredentialId(selectedCredentialId),
counter: selectedFido2Credential.counter,
userPresence: true,
userVerification: userVerified,
Expand All @@ -328,7 +329,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
return {
authenticatorData,
selectedCredential: {
id: guidToRawFormat(selectedCredentialId),
id: parseCredentialId(selectedCredentialId),
userHandle: Fido2Utils.stringToBuffer(selectedFido2Credential.userHandle),
},
signature,
Expand Down Expand Up @@ -412,16 +413,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
credentials: PublicKeyCredentialDescriptor[],
rpId: string,
): Promise<CipherView[]> {
const ids: string[] = [];

for (const credential of credentials) {
try {
ids.push(guidToStandardFormat(credential.id));
// eslint-disable-next-line no-empty
} catch {}
}

if (ids.length === 0) {
if (credentials.length === 0) {
return [];
}

Expand All @@ -432,7 +424,12 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
cipher.type === CipherType.Login &&
cipher.login.hasFido2Credentials &&
cipher.login.fido2Credentials[0].rpId === rpId &&
ids.includes(cipher.login.fido2Credentials[0].credentialId),
credentials.some((credential) =>
compareCredentialIds(
credential.id,
parseCredentialId(cipher.login.fido2Credentials[0].credentialId),
),
),
);
}

Expand Down
28 changes: 28 additions & 0 deletions libs/common/src/platform/services/fido2/guid-utils.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { guidToRawFormat } from "./guid-utils";

describe("guid-utils", () => {
describe("guidToRawFormat", () => {
it.each([
[
"00000000-0000-0000-0000-000000000000",
[
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00,
],
"08d70b74-e9f5-4522-a425-e5dcd40107e7",
[
0x08, 0xd7, 0x0b, 0x74, 0xe9, 0xf5, 0x45, 0x22, 0xa4, 0x25, 0xe5, 0xdc, 0xd4, 0x01, 0x07,
0xe7,
],
],
])("returns UUID in binary format when given a valid UUID string", (input, expected) => {
const result = guidToRawFormat(input);

expect(result).toEqual(new Uint8Array(expected));
});

it("throws an error when given an invalid UUID string", () => {
expect(() => guidToRawFormat("invalid")).toThrow(TypeError);
});
});
});
Loading