Support pop as optional for full framed apps (#7119)

- This PR signs the POP tokens only if the reqCnf is not passed in as a request parameter. This is to enable any clients that choose to sign their tokens. However, please consider this an advanced feature only. - This PR also addresses the native flow bug where cnf is to be sent a string instead of a hash! - Removes reqCnfHash in the ReqCnfData since we do not use it. It is only internal, so this should not be a breaking change. --------- Co-authored-by: Thomas Norling <[email protected]> Co-authored-by: Lalima Sharda <[email protected]> Co-authored-by: Hector Morales <[email protected]>
AzureAD · Jun 10, 2024 · 25aefea · 25aefea
1 parent 7563498
commit 25aefea
Show file tree

Hide file tree

Showing 44 changed files with 633 additions and 66 deletions.
diff --git a/change/@azure-msal-browser-6d89bcc9-48e1-495e-bdd5-2d7fda8e13f8.json b/change/@azure-msal-browser-6d89bcc9-48e1-495e-bdd5-2d7fda8e13f8.json
@@ -0,0 +1,7 @@
+{
+    "type": "minor",
+    "comment": "Add support for apps to set their own `reqCnf` and correct native flows cnf format #6357",
+    "packageName": "@azure/msal-browser",
+    "email": "[email protected]",
+    "dependentChangeType": "patch"
+}
diff --git a/change/@azure-msal-common-98b3791f-fcf5-4225-a03c-a379d2312455.json b/change/@azure-msal-common-98b3791f-fcf5-4225-a03c-a379d2312455.json
@@ -0,0 +1,7 @@
+{
+    "type": "minor",
+    "comment": "Add support for apps to set their own `reqCnf` and correct native flows cnf format #6357",
+    "packageName": "@azure/msal-common",
+    "email": "[email protected]",
+    "dependentChangeType": "patch"
+}
diff --git a/change/@azure-msal-node-cf83856a-c1aa-4763-8eb2-0b62f5708a27.json b/change/@azure-msal-node-cf83856a-c1aa-4763-8eb2-0b62f5708a27.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Fixed msal-node unit tests for PoP token support #\u0016\u00167119",
+  "packageName": "@azure/msal-node",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/lib/msal-browser/apiReview/msal-browser.api.md b/lib/msal-browser/apiReview/msal-browser.api.md
@@ -234,7 +234,8 @@ declare namespace BrowserAuthErrorCodes {
         nativeConnectionNotEstablished,
         uninitializedPublicClientApplication,
         nativePromptNotSupported,
-        invalidBase64String
+        invalidBase64String,
+        invalidPopTokenRequest
     }
 }
 export { BrowserAuthErrorCodes }
@@ -423,6 +424,10 @@ export const BrowserAuthErrorMessage: {
         code: string;
         desc: string;
     };
+    invalidPopTokenRequest: {
+        code: string;
+        desc: string;
+    };
 };
 
 // Warning: (ae-missing-release-tag) "BrowserAuthOptions" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
@@ -1022,6 +1027,11 @@ const invalidBase64String = "invalid_base64_string";
 // @public (undocumented)
 const invalidCacheType = "invalid_cache_type";
 
+// Warning: (ae-missing-release-tag) "invalidPopTokenRequest" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
+//
+// @public (undocumented)
+const invalidPopTokenRequest = "invalid_pop_token_request";
+
 export { IPerformanceClient }
 
 // Warning: (ae-missing-release-tag) "IPublicClientApplication" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)

diff --git a/lib/msal-browser/docs/access-token-proof-of-possession.md b/lib/msal-browser/docs/access-token-proof-of-possession.md
@@ -151,6 +151,14 @@ The Proof-of-Possession authentication scheme relies on an asymmetric cryptograp
 
 In the event of refreshing a bound access token, MSAL will delete the cryptographic keypair that was generated when requesting the expired bound access token, generate a new cryptographic keypair for the new access token, and store the new keypair in the keystore.
 
+## Advanced feature: Application managed cryptographic keypair
+
+> :warning: We do not recommend using this feature unless you are familiar with the [Proof of Possession protocol](https://oauth.net/2/dpop/) and have a specific requirement to generate your own cryptographic keypair. For most cases, we recommend the PoP usage as described in the rest of this document.
+
+If you choose to generate your own cryptographic keypair, then this feature enables the application to provide the `popKid` as a request parameter. MSAL JS ensures the token issuer embeds the `cnf` in the token but returns the issued token _unsigned_. The onus of signing the access token before it is forwarded to the intended resource will be on the application. 
+
+Please also note to make sure the remaining [pop parameters](#at-pop-request-parameters) except the `AuthenticationScheme` are not set if you choose to leverage this behavior.
+
 ### Why access tokens are saved asynchronously
 
 Most MSAL credentials and cache items, like `ID Tokens` for example, can be stored and removed synchronously. This is because these cache items are stored in either `localStorage` or `sessionStorage` (which can be manipulated synchronously), and they have no dependencies on other stored items that have asynchronous access restrictions.

diff --git a/lib/msal-browser/src/broker/nativeBroker/NativeRequest.ts b/lib/msal-browser/src/broker/nativeBroker/NativeRequest.ts
@@ -31,6 +31,7 @@ export type NativeTokenRequest = {
     extendedExpiryToken?: boolean;
     extraParameters?: StringDict;
     storeInCache?: StoreInCache; // Object of booleans indicating whether to store tokens in the cache or not (default is true)
+    signPopToken?: boolean; // Set to true only if token request deos not contain a PoP keyId
 };
 
 /**

diff --git a/lib/msal-browser/src/crypto/CryptoOps.ts b/lib/msal-browser/src/crypto/CryptoOps.ts
@@ -78,6 +78,23 @@ export class CryptoOps implements ICrypto {
         return base64Decode(input);
     }
 
+    /**
+     * Encodes input string to base64 URL safe string.
+     * @param input
+     */
+    base64UrlEncode(input: string): string {
+        return urlEncode(input);
+    }
+
+    /**
+     * Stringifies and base64Url encodes input public key
+     * @param inputKid
+     * @returns Base64Url encoded public key
+     */
+    encodeKid(inputKid: string): string {
+        return this.base64UrlEncode(JSON.stringify({ kid: inputKid }));
+    }
+
     /**
      * Generates a keypair, stores it and returns a thumbprint
      * @param request

diff --git a/lib/msal-browser/src/error/BrowserAuthError.ts b/lib/msal-browser/src/error/BrowserAuthError.ts
@@ -90,6 +90,8 @@ export const BrowserAuthErrorMessages = {
         "The provided prompt is not supported by the native platform. This request should be routed to the web based flow.",
     [BrowserAuthErrorCodes.invalidBase64String]:
         "Invalid base64 encoded string.",
+    [BrowserAuthErrorCodes.invalidPopTokenRequest]:
+        "Invalid PoP token request. The request should not have both a popKid value and signPopToken set to true.",
 };
 
 /**
@@ -333,6 +335,12 @@ export const BrowserAuthErrorMessage = {
             BrowserAuthErrorCodes.invalidBase64String
         ],
     },
+    invalidPopTokenRequest: {
+        code: BrowserAuthErrorCodes.invalidPopTokenRequest,
+        desc: BrowserAuthErrorMessages[
+            BrowserAuthErrorCodes.invalidPopTokenRequest
+        ],
+    },
 };
 
 /**

diff --git a/lib/msal-browser/src/error/BrowserAuthErrorCodes.ts b/lib/msal-browser/src/error/BrowserAuthErrorCodes.ts
@@ -55,3 +55,4 @@ export const uninitializedPublicClientApplication =
     "uninitialized_public_client_application";
 export const nativePromptNotSupported = "native_prompt_not_supported";
 export const invalidBase64String = "invalid_base64_string";
+export const invalidPopTokenRequest = "invalid_pop_token_request";
diff --git a/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts b/lib/msal-browser/src/interaction_client/NativeInteractionClient.ts
@@ -170,10 +170,12 @@ export class NativeInteractionClient extends BaseInteractionClient {
             );
         }
 
+        const { ...nativeTokenRequest } = nativeRequest;
+
         // fall back to native calls
         const messageBody: NativeExtensionRequestBody = {
             method: NativeExtensionMethod.GetToken,
-            request: nativeRequest,
+            request: nativeTokenRequest,
         };
 
         const response: object = await this.nativeMessageHandler.sendMessage(
@@ -289,9 +291,11 @@ export class NativeInteractionClient extends BaseInteractionClient {
         );
         const nativeRequest = await this.initializeNativeRequest(request);
 
+        const { ...nativeTokenRequest } = nativeRequest;
+
         const messageBody: NativeExtensionRequestBody = {
             method: NativeExtensionMethod.GetToken,
-            request: nativeRequest,
+            request: nativeTokenRequest,
         };
 
         try {
@@ -481,7 +485,7 @@ export class NativeInteractionClient extends BaseInteractionClient {
             request,
             homeAccountIdentifier,
             idTokenClaims,
-            result.accessToken,
+            response.access_token,
             result.tenantId,
             reqTimestamp
         );
@@ -535,7 +539,10 @@ export class NativeInteractionClient extends BaseInteractionClient {
         response: NativeResponse,
         request: NativeTokenRequest
     ): Promise<string> {
-        if (request.tokenType === AuthenticationScheme.POP) {
+        if (
+            request.tokenType === AuthenticationScheme.POP &&
+            request.signPopToken
+        ) {
             /**
              * This code prioritizes SHR returned from the native layer. In case of error/SHR not calculated from WAM and the AT
              * is still received, SHR is calculated locally
@@ -725,7 +732,11 @@ export class NativeInteractionClient extends BaseInteractionClient {
                 responseScopes.printScopes(),
                 tokenExpirationSeconds,
                 0,
-                base64Decode
+                base64Decode,
+                undefined,
+                request.tokenType as AuthenticationScheme,
+                undefined,
+                request.keyId
             );
 
         const nativeCacheRecord = new CacheRecord(
@@ -917,8 +928,16 @@ export class NativeInteractionClient extends BaseInteractionClient {
                 ...request.tokenQueryParameters,
             },
             extendedExpiryToken: false, // Make this configurable?
+            keyId: request.popKid,
         };
 
+        // Check for PoP token requests: signPopToken should only be set to true if popKid is not set
+        if (validatedRequest.signPopToken && !!request.popKid) {
+            throw createBrowserAuthError(
+                BrowserAuthErrorCodes.invalidPopTokenRequest
+            );
+        }
+
         this.handleExtraBrokerParams(validatedRequest);
         validatedRequest.extraParameters =
             validatedRequest.extraParameters || {};
@@ -935,17 +954,29 @@ export class NativeInteractionClient extends BaseInteractionClient {
             };
 
             const popTokenGenerator = new PopTokenGenerator(this.browserCrypto);
-            const reqCnfData = await invokeAsync(
-                popTokenGenerator.generateCnf.bind(popTokenGenerator),
-                PerformanceEvents.PopTokenGenerateCnf,
-                this.logger,
-                this.performanceClient,
-                this.correlationId
-            )(shrParameters, this.logger);
-
-            // to reduce the URL length, it is recommended to send the hash of the req_cnf instead of the whole string
-            validatedRequest.reqCnf = reqCnfData.reqCnfHash;
-            validatedRequest.keyId = reqCnfData.kid;
+
+            // generate reqCnf if not provided in the request
+            let reqCnfData;
+            if (!validatedRequest.keyId) {
+                const generatedReqCnfData = await invokeAsync(
+                    popTokenGenerator.generateCnf.bind(popTokenGenerator),
+                    PerformanceEvents.PopTokenGenerateCnf,
+                    this.logger,
+                    this.performanceClient,
+                    request.correlationId
+                )(shrParameters, this.logger);
+                reqCnfData = generatedReqCnfData.reqCnfString;
+                validatedRequest.keyId = generatedReqCnfData.kid;
+                validatedRequest.signPopToken = true;
+            } else {
+                reqCnfData = this.browserCrypto.base64UrlEncode(
+                    JSON.stringify({ kid: validatedRequest.keyId })
+                );
+                validatedRequest.signPopToken = false;
+            }
+
+            // SPAs require whole string to be passed to broker
+            validatedRequest.reqCnf = reqCnfData;
         }
 
         return validatedRequest;

diff --git a/lib/msal-browser/test/interaction_client/StandardInteractionClient.spec.ts b/lib/msal-browser/test/interaction_client/StandardInteractionClient.spec.ts
@@ -22,6 +22,7 @@ import {
     TEST_URIS,
     DEFAULT_TENANT_DISCOVERY_RESPONSE,
     DEFAULT_OPENID_CONFIG_RESPONSE,
+    TEST_REQ_CNF_DATA,
 } from "../utils/StringConstants";
 import { AuthorizationUrlRequest } from "../../src/request/AuthorizationUrlRequest";
 import { RedirectRequest } from "../../src/request/RedirectRequest";
@@ -141,6 +142,56 @@ describe("StandardInteractionClient", () => {
             await testClient.initializeAuthorizationCodeRequest(request);
         expect(request.codeChallenge).toBe(TEST_CONFIG.TEST_CHALLENGE);
         expect(authCodeRequest.codeVerifier).toBe(TEST_CONFIG.TEST_VERIFIER);
+        expect(authCodeRequest.popKid).toBeUndefined;
+    });
+
+    it("initializeAuthorizationCodeRequest validates the request and does not influence undefined popKid param", async () => {
+        const request: AuthorizationUrlRequest = {
+            redirectUri: TEST_URIS.TEST_REDIR_URI,
+            scopes: ["scope"],
+            loginHint: "[email protected]",
+            state: TEST_STATE_VALUES.USER_STATE,
+            authority: TEST_CONFIG.validAuthority,
+            correlationId: TEST_CONFIG.CORRELATION_ID,
+            responseMode: TEST_CONFIG.RESPONSE_MODE as ResponseMode,
+            nonce: "",
+            authenticationScheme:
+                TEST_CONFIG.TOKEN_TYPE_BEARER as AuthenticationScheme,
+        };
+
+        jest.spyOn(PkceGenerator, "generatePkceCodes").mockResolvedValue({
+            challenge: TEST_CONFIG.TEST_CHALLENGE,
+            verifier: TEST_CONFIG.TEST_VERIFIER,
+        });
+
+        const authCodeRequest =
+            await testClient.initializeAuthorizationCodeRequest(request);
+        expect(authCodeRequest.popKid).toBeUndefined;
+    });
+
+    it("initializeAuthorizationCodeRequest validates the request and adds reqCnf param when user defined", async () => {
+        const request: AuthorizationUrlRequest = {
+            redirectUri: TEST_URIS.TEST_REDIR_URI,
+            scopes: ["scope"],
+            loginHint: "[email protected]",
+            state: TEST_STATE_VALUES.USER_STATE,
+            authority: TEST_CONFIG.validAuthority,
+            correlationId: TEST_CONFIG.CORRELATION_ID,
+            responseMode: TEST_CONFIG.RESPONSE_MODE as ResponseMode,
+            nonce: "",
+            authenticationScheme:
+                TEST_CONFIG.TOKEN_TYPE_BEARER as AuthenticationScheme,
+            popKid: TEST_REQ_CNF_DATA.kid,
+        };
+
+        jest.spyOn(PkceGenerator, "generatePkceCodes").mockResolvedValue({
+            challenge: TEST_CONFIG.TEST_CHALLENGE,
+            verifier: TEST_CONFIG.TEST_VERIFIER,
+        });
+
+        const authCodeRequest =
+            await testClient.initializeAuthorizationCodeRequest(request);
+        expect(authCodeRequest.popKid).toEqual(TEST_REQ_CNF_DATA.kid);
     });
 
     it("getDiscoveredAuthority - request authority only", async () => {

diff --git a/lib/msal-browser/test/interaction_handler/InteractionHandler.spec.ts b/lib/msal-browser/test/interaction_handler/InteractionHandler.spec.ts
@@ -129,6 +129,14 @@ const cryptoInterface = {
     base64Encode: (input: string): string => {
         return "testEncodedString";
     },
+    base64UrlEncode(input: string): string {
+        return Buffer.from(input, "utf-8").toString("base64url");
+    },
+    encodeKid(input: string): string {
+        return Buffer.from(JSON.stringify({ kid: input }), "utf-8").toString(
+            "base64url"
+        );
+    },
     generatePkceCodes: async (): Promise<PkceCodes> => {
         return testPkceCodes;
     },

diff --git a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts
@@ -146,6 +146,15 @@ describe("RedirectHandler.ts Unit Tests", () => {
                 base64Encode: (input: string): string => {
                     return "testEncodedString";
                 },
+                base64UrlEncode(input: string): string {
+                    return Buffer.from(input, "utf-8").toString("base64url");
+                },
+                encodeKid(input: string): string {
+                    return Buffer.from(
+                        JSON.stringify({ kid: input }),
+                        "utf-8"
+                    ).toString("base64url");
+                },
                 getPublicKeyThumbprint: async (): Promise<string> => {
                     return TEST_POP_VALUES.ENCODED_REQ_CNF;
                 },

diff --git a/lib/msal-browser/test/utils/BrowserUtils.spec.ts b/lib/msal-browser/test/utils/BrowserUtils.spec.ts
@@ -7,8 +7,7 @@ import { TEST_CONFIG, TEST_URIS } from "./StringConstants";
 import {
     BrowserUtils,
     BrowserAuthError,
-    BrowserAuthErrorMessage,
-    InteractionType,
+    BrowserAuthErrorCodes,
 } from "../../src";
 
 describe("BrowserUtils.ts Function Unit Tests", () => {
@@ -101,7 +100,7 @@ describe("BrowserUtils.ts Function Unit Tests", () => {
             } catch (e) {
                 const browserAuthError = e as BrowserAuthError;
                 expect(browserAuthError.errorCode).toBe(
-                    BrowserAuthErrorMessage.redirectInIframeError.code
+                    BrowserAuthErrorCodes.redirectInIframe
                 );
                 done();
             }

diff --git a/lib/msal-browser/test/utils/StringConstants.ts b/lib/msal-browser/test/utils/StringConstants.ts
@@ -170,6 +170,11 @@ export const TEST_POP_VALUES = {
         '{"kid":"NzbLsXh8uDCcd-6MNwXF4W_7noWXFZAfHkxZsRGC9Xs","xms_ksl":"sw"}',
 };
 
+export const TEST_REQ_CNF_DATA = {
+    kid: TEST_POP_VALUES.KID,
+    reqCnfString: TEST_POP_VALUES.DECODED_REQ_CNF,
+};
+
 export const TEST_SSH_VALUES = {
     SSH_JWK:
         '{"kty":"RSA","n":"wDJwv083ZhGGkpMPVcBMwtSBNLu7qhT2VmKv7AyPEz_dWb8GQzNEnWT1niNjFI0isDMFWQ7X2O-dhTL9J1QguQ==","e":"AQAB"}',