diff --git a/change/@azure-msal-node-b684bbd7-5c63-44e8-902f-876ff9bf2fb7.json b/change/@azure-msal-node-b684bbd7-5c63-44e8-902f-876ff9bf2fb7.json new file mode 100644 index 0000000000..a6f62c4478 --- /dev/null +++ b/change/@azure-msal-node-b684bbd7-5c63-44e8-902f-876ff9bf2fb7.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Deprecated SHA-1 thumbprint for clientCertificate #7185", + "packageName": "@azure/msal-node", + "email": "rginsburg@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/lib/msal-node/apiReview/msal-node.api.md b/lib/msal-node/apiReview/msal-node.api.md index 234bdfc958..fa99729eeb 100644 --- a/lib/msal-node/apiReview/msal-node.api.md +++ b/lib/msal-node/apiReview/msal-node.api.md @@ -169,7 +169,9 @@ export abstract class ClientApplication { // @public export class ClientAssertion { static fromAssertion(assertion: string): ClientAssertion; + // @deprecated (undocumented) static fromCertificate(thumbprint: string, privateKey: string, publicCertificate?: string): ClientAssertion; + static fromCertificateWithSha256Thumbprint(thumbprint: string, privateKey: string, publicCertificate?: string): ClientAssertion; getJwt(cryptoProvider: CryptoProvider, issuer: string, jwtAudience: string): string; static parseCertificate(publicCertificate: string): Array; } @@ -475,7 +477,8 @@ export type NodeAuthOptions = { clientSecret?: string; clientAssertion?: string | ClientAssertionCallback; clientCertificate?: { - thumbprint: string; + thumbprint?: string; + thumbprintSha256?: string; privateKey: string; x5c?: string; }; diff --git a/lib/msal-node/src/client/ClientAssertion.ts b/lib/msal-node/src/client/ClientAssertion.ts index 503194cff0..0364212643 100644 --- a/lib/msal-node/src/client/ClientAssertion.ts +++ b/lib/msal-node/src/client/ClientAssertion.ts @@ -22,6 +22,7 @@ export class ClientAssertion { private jwt: string; private privateKey: string; private thumbprint: string; + private useSha256: boolean; private expirationTime: number; private issuer: string; private jwtAudience: string; @@ -38,6 +39,7 @@ export class ClientAssertion { } /** + * @deprecated Use fromCertificateWithSha256Thumbprint instead, with a SHA-256 thumprint * Initialize the ClientAssertion class from the certificate passed by the user * @param thumbprint - identifier of a certificate * @param privateKey - secret key @@ -51,6 +53,29 @@ export class ClientAssertion { const clientAssertion = new ClientAssertion(); clientAssertion.privateKey = privateKey; clientAssertion.thumbprint = thumbprint; + clientAssertion.useSha256 = false; + if (publicCertificate) { + clientAssertion.publicCertificate = + this.parseCertificate(publicCertificate); + } + return clientAssertion; + } + + /** + * Initialize the ClientAssertion class from the certificate passed by the user + * @param thumbprint - identifier of a certificate + * @param privateKey - secret key + * @param publicCertificate - electronic document provided to prove the ownership of the public key + */ + public static fromCertificateWithSha256Thumbprint( + thumbprint: string, + privateKey: string, + publicCertificate?: string + ): ClientAssertion { + const clientAssertion = new ClientAssertion(); + clientAssertion.privateKey = privateKey; + clientAssertion.thumbprint = thumbprint; + clientAssertion.useSha256 = true; if (publicCertificate) { clientAssertion.publicCertificate = this.parseCertificate(publicCertificate); @@ -109,12 +134,21 @@ export class ClientAssertion { const header: jwt.JwtHeader = { alg: JwtConstants.RSA_256, - x5t: EncodingUtils.base64EncodeUrl(this.thumbprint, "hex"), }; + const thumbprintHeader = this.useSha256 + ? JwtConstants.X5T_256 + : JwtConstants.X5T; + Object.assign(header, { + [thumbprintHeader]: EncodingUtils.base64EncodeUrl( + this.thumbprint, + "hex" + ), + } as Partial); + if (this.publicCertificate) { Object.assign(header, { - x5c: this.publicCertificate, + [JwtConstants.X5C]: this.publicCertificate, } as Partial); } diff --git a/lib/msal-node/src/client/ConfidentialClientApplication.ts b/lib/msal-node/src/client/ConfidentialClientApplication.ts index 26e3c7a197..d2f083b92b 100644 --- a/lib/msal-node/src/client/ConfidentialClientApplication.ts +++ b/lib/msal-node/src/client/ConfidentialClientApplication.ts @@ -19,7 +19,6 @@ import { AuthenticationResult, AzureRegionConfiguration, AuthError, - Constants, IAppTokenProvider, OIDC_DEFAULT_SCOPES, UrlString, @@ -67,7 +66,7 @@ export class ConfidentialClientApplication */ constructor(configuration: Configuration) { super(configuration); - this.setClientCredential(this.config); + this.setClientCredential(); this.appTokenProvider = undefined; } @@ -218,15 +217,13 @@ export class ConfidentialClientApplication } } - private setClientCredential(configuration: Configuration): void { - const clientSecretNotEmpty = !!configuration.auth.clientSecret; - const clientAssertionNotEmpty = !!configuration.auth.clientAssertion; - const certificate = configuration.auth.clientCertificate || { - thumbprint: Constants.EMPTY_STRING, - privateKey: Constants.EMPTY_STRING, - }; + private setClientCredential(): void { + const clientSecretNotEmpty = !!this.config.auth.clientSecret; + const clientAssertionNotEmpty = !!this.config.auth.clientAssertion; const certificateNotEmpty = - !!certificate.thumbprint || !!certificate.privateKey; + (!!this.config.auth.clientCertificate.thumbprint || + !!this.config.auth.clientCertificate.thumbprintSha256) && + !!this.config.auth.clientCertificate.privateKey; /* * If app developer configures this callback, they don't need a credential @@ -247,14 +244,14 @@ export class ConfidentialClientApplication ); } - if (configuration.auth.clientSecret) { - this.clientSecret = configuration.auth.clientSecret; + if (this.config.auth.clientSecret) { + this.clientSecret = this.config.auth.clientSecret; return; } - if (configuration.auth.clientAssertion) { + if (this.config.auth.clientAssertion) { this.developerProvidedClientAssertion = - configuration.auth.clientAssertion; + this.config.auth.clientAssertion; return; } @@ -263,11 +260,19 @@ export class ConfidentialClientApplication ClientAuthErrorCodes.invalidClientCredential ); } else { - this.clientAssertion = ClientAssertion.fromCertificate( - certificate.thumbprint, - certificate.privateKey, - configuration.auth.clientCertificate?.x5c - ); + this.clientAssertion = !!this.config.auth.clientCertificate + .thumbprintSha256 + ? ClientAssertion.fromCertificateWithSha256Thumbprint( + this.config.auth.clientCertificate.thumbprintSha256, + this.config.auth.clientCertificate.privateKey, + this.config.auth.clientCertificate.x5c + ) + : ClientAssertion.fromCertificate( + // guaranteed to be a string, due to prior error checking in this function + this.config.auth.clientCertificate.thumbprint as string, + this.config.auth.clientCertificate.privateKey, + this.config.auth.clientCertificate.x5c + ); } } } diff --git a/lib/msal-node/src/config/Configuration.ts b/lib/msal-node/src/config/Configuration.ts index 2751b1f157..b5ca9f81c9 100644 --- a/lib/msal-node/src/config/Configuration.ts +++ b/lib/msal-node/src/config/Configuration.ts @@ -27,6 +27,7 @@ import { } from "../utils/Constants.js"; import { LinearRetryPolicy } from "../retry/LinearRetryPolicy.js"; import { HttpClientWithRetries } from "../network/HttpClientWithRetries.js"; +import { NodeAuthError } from "../error/NodeAuthError.js"; /** * - clientId - Client id of the application. @@ -34,7 +35,7 @@ import { HttpClientWithRetries } from "../network/HttpClientWithRetries.js"; * - knownAuthorities - Needed for Azure B2C and ADFS. All authorities that will be used in the client application. Only the host of the authority should be passed in. * - clientSecret - Secret string that the application uses when requesting a token. Only used in confidential client applications. Can be created in the Azure app registration portal. * - clientAssertion - A ClientAssertion object containing an assertion string or a callback function that returns an assertion string that the application uses when requesting a token, as well as the assertion's type (urn:ietf:params:oauth:client-assertion-type:jwt-bearer). Only used in confidential client applications. - * - clientCertificate - Certificate that the application uses when requesting a token. Only used in confidential client applications. Requires hex encoded X.509 SHA-1 thumbprint of the certificiate, and the PEM encoded private key (string should contain -----BEGIN PRIVATE KEY----- ... -----END PRIVATE KEY----- ) + * - clientCertificate - Certificate that the application uses when requesting a token. Only used in confidential client applications. Requires hex encoded X.509 SHA-1 or SHA-256 thumbprint of the certificate, and the PEM encoded private key (string should contain -----BEGIN PRIVATE KEY----- ... -----END PRIVATE KEY----- ) * - protocolMode - Enum that represents the protocol that msal follows. Used for configuring proper endpoints. * - skipAuthorityMetadataCache - A flag to choose whether to use or not use the local metadata cache during authority initialization. Defaults to false. * @public @@ -45,7 +46,12 @@ export type NodeAuthOptions = { clientSecret?: string; clientAssertion?: string | ClientAssertionCallback; clientCertificate?: { - thumbprint: string; + /** + * @deprecated Use thumbprintSha2 property instead. Thumbprint needs to be computed with SHA-256 algorithm. + * SHA-1 is only needed for backwards compatibility with older versions of ADFS. + */ + thumbprint?: string; + thumbprintSha256?: string; privateKey: string; x5c?: string; }; @@ -138,6 +144,7 @@ const DEFAULT_AUTH_OPTIONS: Required = { clientAssertion: Constants.EMPTY_STRING, clientCertificate: { thumbprint: Constants.EMPTY_STRING, + thumbprintSha256: Constants.EMPTY_STRING, privateKey: Constants.EMPTY_STRING, x5c: Constants.EMPTY_STRING, }, @@ -217,6 +224,15 @@ export function buildAppConfiguration({ disableInternalRetries: system?.disableInternalRetries || false, }; + // if client certificate was provided, ensure that at least one of the SHA-1 or SHA-256 thumbprints were provided + if ( + !!auth.clientCertificate && + !!!auth.clientCertificate.thumbprint && + !!!auth.clientCertificate.thumbprintSha256 + ) { + throw NodeAuthError.createStateNotFoundError(); + } + return { auth: { ...DEFAULT_AUTH_OPTIONS, ...auth }, broker: { ...broker }, diff --git a/lib/msal-node/src/error/NodeAuthError.ts b/lib/msal-node/src/error/NodeAuthError.ts index 5a5519681a..cfade4c2af 100644 --- a/lib/msal-node/src/error/NodeAuthError.ts +++ b/lib/msal-node/src/error/NodeAuthError.ts @@ -37,6 +37,10 @@ export const NodeAuthErrorMessage = { code: "state_not_found", desc: "State not found. Please verify that the request originated from msal.", }, + thumbprintMissing: { + code: "thumbprint_missing_from_client_certificate", + desc: "Client certificate does not contain a SHA-1 or SHA-256 thumbprint.", + }, }; export class NodeAuthError extends AuthError { @@ -114,4 +118,14 @@ export class NodeAuthError extends AuthError { NodeAuthErrorMessage.stateNotFoundError.desc ); } + + /** + * Creates an error thrown when client certificate was provided, but neither the SHA-1 or SHA-256 thumbprints were provided + */ + static createThumbprintMissingError(): NodeAuthError { + return new NodeAuthError( + NodeAuthErrorMessage.thumbprintMissing.code, + NodeAuthErrorMessage.thumbprintMissing.desc + ); + } } diff --git a/lib/msal-node/src/utils/Constants.ts b/lib/msal-node/src/utils/Constants.ts index dbc9105efa..e84ab39946 100644 --- a/lib/msal-node/src/utils/Constants.ts +++ b/lib/msal-node/src/utils/Constants.ts @@ -146,6 +146,7 @@ export type ApiId = (typeof ApiId)[keyof typeof ApiId]; export const JwtConstants = { ALGORITHM: "alg", RSA_256: "RS256", + X5T_256: "x5t#S256", X5T: "x5t", X5C: "x5c", AUDIENCE: "aud", diff --git a/lib/msal-node/test/client/ClientAssertion.spec.ts b/lib/msal-node/test/client/ClientAssertion.spec.ts index 6e0fabea4f..85c2660bdf 100644 --- a/lib/msal-node/test/client/ClientAssertion.spec.ts +++ b/lib/msal-node/test/client/ClientAssertion.spec.ts @@ -8,8 +8,7 @@ import { EncodingUtils } from "../../src/utils/EncodingUtils"; import { JwtConstants } from "../../src/utils/Constants"; import { getClientAssertionCallback } from "./ClientTestUtils"; import { getClientAssertion } from "@azure/msal-common"; - -const jsonwebtoken = require("jsonwebtoken"); +import jwt from "jsonwebtoken"; jest.mock("jsonwebtoken"); @@ -18,6 +17,11 @@ describe("Client assertion test", () => { const issuer = "client_id"; const audience = "audience"; + let spySign: jest.SpyInstance; + beforeAll(async () => { + spySign = jest.spyOn(jwt, "sign"); + }); + test("creates ClientAssertion from assertion string", () => { const assertion = ClientAssertion.fromAssertion( TEST_CONSTANTS.CLIENT_ASSERTION @@ -44,7 +48,7 @@ describe("Client assertion test", () => { ); }); - test("creates ClientAssertion from certificate", () => { + test("creates ClientAssertion from certificate with SHA-1 thumbprint", () => { const expectedPayload = { [JwtConstants.AUDIENCE]: audience, [JwtConstants.ISSUER]: issuer, @@ -61,21 +65,50 @@ describe("Client assertion test", () => { }, }; - const spySign = jest.spyOn(jsonwebtoken, "sign"); const assertion = ClientAssertion.fromCertificate( TEST_CONSTANTS.THUMBPRINT, TEST_CONSTANTS.PRIVATE_KEY ); assertion.getJwt(cryptoProvider, issuer, audience); - expect(spySign.mock.calls[0][0]).toEqual( + expect(spySign.mock.lastCall[0]).toEqual( + expect.objectContaining(expectedPayload) + ); + expect(spySign.mock.lastCall[1]).toEqual(TEST_CONSTANTS.PRIVATE_KEY); + expect(spySign.mock.lastCall[2]).toEqual(expectedOptions); + }); + + test("creates ClientAssertion from certificate with SHA-256 thumbprint", () => { + const expectedPayload = { + [JwtConstants.AUDIENCE]: audience, + [JwtConstants.ISSUER]: issuer, + [JwtConstants.SUBJECT]: issuer, + }; + + const expectedOptions = { + header: { + [JwtConstants.ALGORITHM]: JwtConstants.RSA_256, + [JwtConstants.X5T_256]: EncodingUtils.base64EncodeUrl( + TEST_CONSTANTS.THUMBPRINT256, + "hex" + ), + }, + }; + + const assertion = ClientAssertion.fromCertificateWithSha256Thumbprint( + TEST_CONSTANTS.THUMBPRINT256, + TEST_CONSTANTS.PRIVATE_KEY + ); + assertion.getJwt(cryptoProvider, issuer, audience); + + expect(spySign.mock.lastCall[0]).toEqual( expect.objectContaining(expectedPayload) ); - expect(spySign.mock.calls[0][1]).toEqual(TEST_CONSTANTS.PRIVATE_KEY); - expect(spySign.mock.calls[0][2]).toEqual(expectedOptions); + expect(spySign.mock.lastCall[1]).toEqual(TEST_CONSTANTS.PRIVATE_KEY); + expect(spySign.mock.lastCall[2]).toEqual(expectedOptions); }); - test("creates ClientAssertion from public certificate for SNI", () => { + test("creates ClientAssertion from public certificate, with SHA-1 thumbprint, for SNI", () => { const expectedPayload = { [JwtConstants.AUDIENCE]: audience, [JwtConstants.ISSUER]: issuer, @@ -89,10 +122,10 @@ describe("Client assertion test", () => { TEST_CONSTANTS.THUMBPRINT, "hex" ), + [JwtConstants.X5C]: TEST_CONSTANTS.X5C_FROM_PUBLIC_CERTIFICATE, }, }; - const spySign = jest.spyOn(jsonwebtoken, "sign"); const assertion = ClientAssertion.fromCertificate( TEST_CONSTANTS.THUMBPRINT, TEST_CONSTANTS.PRIVATE_KEY, @@ -100,11 +133,43 @@ describe("Client assertion test", () => { ); assertion.getJwt(cryptoProvider, issuer, audience); - expect(spySign.mock.calls[0][0]).toEqual( + expect(spySign.mock.lastCall[0]).toEqual( + expect.objectContaining(expectedPayload) + ); + expect(spySign.mock.lastCall[1]).toEqual(TEST_CONSTANTS.PRIVATE_KEY); + expect(spySign.mock.lastCall[2]).toEqual(expectedOptions); + }); + + test("creates ClientAssertion from public certificate, with SHA-256 thumbprint, for SNI", () => { + const expectedPayload = { + [JwtConstants.AUDIENCE]: audience, + [JwtConstants.ISSUER]: issuer, + [JwtConstants.SUBJECT]: issuer, + }; + + const expectedOptions = { + header: { + [JwtConstants.ALGORITHM]: JwtConstants.RSA_256, + [JwtConstants.X5T_256]: EncodingUtils.base64EncodeUrl( + TEST_CONSTANTS.THUMBPRINT256, + "hex" + ), + [JwtConstants.X5C]: TEST_CONSTANTS.X5C_FROM_PUBLIC_CERTIFICATE, + }, + }; + + const assertion = ClientAssertion.fromCertificateWithSha256Thumbprint( + TEST_CONSTANTS.THUMBPRINT256, + TEST_CONSTANTS.PRIVATE_KEY, + TEST_CONSTANTS.PUBLIC_CERTIFICATE + ); + assertion.getJwt(cryptoProvider, issuer, audience); + + expect(spySign.mock.lastCall[0]).toEqual( expect.objectContaining(expectedPayload) ); - expect(spySign.mock.calls[0][1]).toEqual(TEST_CONSTANTS.PRIVATE_KEY); - expect(spySign.mock.calls[0][2]).toEqual(expectedOptions); + expect(spySign.mock.lastCall[1]).toEqual(TEST_CONSTANTS.PRIVATE_KEY); + expect(spySign.mock.lastCall[2]).toEqual(expectedOptions); }); test("parseCertificate finds all valid certs in a chain", () => { diff --git a/lib/msal-node/test/client/ClientTestUtils.ts b/lib/msal-node/test/client/ClientTestUtils.ts index d91dba4ab4..4c51bc58f0 100644 --- a/lib/msal-node/test/client/ClientTestUtils.ts +++ b/lib/msal-node/test/client/ClientTestUtils.ts @@ -45,7 +45,6 @@ import { } from "../test_kit/StringConstants"; import { Configuration } from "../../src/config/Configuration"; import { TEST_CONSTANTS } from "../utils/TestConstants"; -import { CryptoKeys } from "../utils/CryptoKeys"; const ACCOUNT_KEYS = "ACCOUNT_KEYS"; const TOKEN_KEYS = "TOKEN_KEYS"; @@ -396,16 +395,16 @@ export class ClientTestUtils { logLevel: LogLevel.Verbose, }; - const cryptoKeys: CryptoKeys = new CryptoKeys(); - const confidentialClientConfig: Configuration = { auth: { clientId: TEST_CONSTANTS.CLIENT_ID, authority: TEST_CONSTANTS.AUTHORITY, // clientSecret, clientAssertion clientCertificate: { - thumbprint: cryptoKeys.thumbprint, - privateKey: cryptoKeys.privateKey, + // defaults to SHA-256 when both thumbprints are provided + thumbprint: TEST_CONSTANTS.THUMBPRINT, + thumbprintSha256: TEST_CONSTANTS.THUMBPRINT256, + privateKey: TEST_CONSTANTS.PRIVATE_KEY, }, knownAuthorities: [TEST_CONSTANTS.AUTHORITY], cloudDiscoveryMetadata: "", diff --git a/lib/msal-node/test/client/ConfidentialClientApplication.spec.ts b/lib/msal-node/test/client/ConfidentialClientApplication.spec.ts index 571baf0a24..614ad6d1df 100644 --- a/lib/msal-node/test/client/ConfidentialClientApplication.spec.ts +++ b/lib/msal-node/test/client/ConfidentialClientApplication.spec.ts @@ -43,8 +43,16 @@ import { mockNetworkClient } from "../utils/MockNetworkClient"; import { ClientTestUtils, getClientAssertionCallback } from "./ClientTestUtils"; import { buildAccountFromIdTokenClaims } from "msal-test-utils"; import { Constants } from "../../src/utils/Constants"; +import jwt from "jsonwebtoken"; +import { NodeAuthError } from "../../src/error/NodeAuthError"; + +jest.mock("jsonwebtoken"); describe("ConfidentialClientApplication", () => { + beforeAll(() => { + jest.spyOn(jwt, "sign").mockReturnValue("fake_jwt_string"); + }); + let config: Configuration; beforeEach(async () => { config = @@ -354,7 +362,7 @@ describe("ConfidentialClientApplication", () => { ); }); - test("ensures that developer-provided certificate is attached to client assertion", async () => { + test("ensures that developer-provided certificate can be provided with a SHA-256 thumbprint, and is attached to client assertion", async () => { const getClientAssertionSpy: jest.SpyInstance = jest.spyOn( ClientApplication.prototype, "getClientAssertion" @@ -382,6 +390,34 @@ describe("ConfidentialClientApplication", () => { Constants.JWT_BEARER_ASSERTION_TYPE ); }); + + test("ensures that developer-provided certificate can be provided with a SHA-1 thumbprint", async () => { + delete config.auth.clientCertificate?.thumbprintSha256; + + const client: ConfidentialClientApplication = + new ConfidentialClientApplication(config); + + const request: ClientCredentialRequest = { + scopes: TEST_CONSTANTS.DEFAULT_GRAPH_SCOPE, + skipCache: false, + }; + + const authResult = (await client.acquireTokenByClientCredential( + request + )) as AuthenticationResult; + expect(authResult.accessToken).toEqual( + CONFIDENTIAL_CLIENT_AUTHENTICATION_RESULT.body.access_token + ); + }); + + test("ensures that developer-provided certificate must be provided with a SHA-1 or SHA-2 thumbprint", async () => { + delete config.auth.clientCertificate?.thumbprint; + delete config.auth.clientCertificate?.thumbprintSha256; + + expect(() => { + new ConfidentialClientApplication(config); + }).toThrow(NodeAuthError.createStateNotFoundError()); + }); }); test("acquireTokenOnBehalfOf", async () => { diff --git a/lib/msal-node/test/utils/TestConstants.ts b/lib/msal-node/test/utils/TestConstants.ts index 13a3abb1e1..5331fcbc4d 100644 --- a/lib/msal-node/test/utils/TestConstants.ts +++ b/lib/msal-node/test/utils/TestConstants.ts @@ -38,6 +38,8 @@ export const TEST_CONSTANTS = { CACHE_LOCATION: "Test", CLIENT_ASSERTION: "MOCK_CLIENT_ASSERTION", THUMBPRINT: "6182de7d4b84517655fe0bfa97076890d66bf37a", + THUMBPRINT256: + "014811efe8ed5ff69a576d84acd47fdeab37a47aad27c039746e220e58bf93b1", PRIVATE_KEY: "PRIVATE_KEY", PUBLIC_CERTIFICATE: `-----BEGIN CERTIFICATE----- line1 @@ -49,6 +51,7 @@ line3 line4 -----END CERTIFICATE----- `, + X5C_FROM_PUBLIC_CERTIFICATE: ["line1line2", "line3line4"], CLAIMS: `{ "access_token": { "xms_cc":{"values":["cp1"] } }}`, SNI_CERTIFICATE: `-----BEGIN PRIVATE KEY-----\r line1\r