[minor][feature]: Add caller-provided keys for AT PoP

[Antonio Alwan (antonioalwan)]

Summary

Adds the public MSAL API for caller-provided RSA keys in Access Token Proof-of-Possession flows. MSAL validates and retains the caller-owned key pair, derives req_cnf from its public key, signs the SHR with its private key, and marks Broker requests for selective rollout control.

Key changes

• Add public MSALExternalKeyPair with a read-only RFC 7638 keyId.
• Add an external-key initializer to MSALAuthenticationSchemePop.
• Use the same external key manager for both token binding and SHR signing.
• Add external_key_pop=1 to scheme parameters for Broker classification.
• Add deterministic validation and cryptographic SHR signature tests.

Dependency

• CommonCore: [minor] [feature]: Add external key support for AT PoP microsoft-authentication-library-common-for-objc#1934

How to validate

• Run MSALExternalKeyPairTests.
• Run MSALAuthSchemeTests.
• Verify the external SHR signature with the supplied RSA public key.

Notes

The caller owns key creation, storage, rotation, and deletion. MSAL does not expose or persist private key material.

[Copilot]

Pull request overview

Adds a new public API surface to support caller-provided RSA key pairs for Access Token Proof-of-Possession (AT PoP) flows, enabling MSAL to derive req_cnf and sign the Signed HTTP Request (SHR) using caller-owned keys while marking broker requests for controlled rollout.

Changes:

• Introduces public MSALExternalKeyPair (with RFC 7638 keyId) and wires it into MSALAuthenticationSchemePop via an additive initializer/property.
• Updates PoP scheme parameter generation and SHR signing to use an injected external PoP manager when an external key pair is provided (including external_key_pop=1 flagging).
• Adds unit coverage for external key validation and SHR signature verification, plus updates project/umbrella headers and changelog.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
MSAL/test/unit/MSALExternalKeyPairTests.m	New unit tests validating external key-pair failure reasons and success path.
MSAL/test/unit/MSALAuthSchemeTests.m	Adds regression coverage ensuring SHR is signed with the external key and broker flagging is set.
MSAL/src/public/MSALExternalKeyPair.h	New public type representing caller-owned RSA key pair + keyId.
MSAL/src/public/MSALError.h	Adds MSALErrorInvalidExternalKeyPair to model validation failures.
MSAL/src/public/MSALAuthenticationSchemePop.h	Adds external-key initializer and exposes externalKeyPair property.
MSAL/src/public/MSAL.h	Exposes MSALExternalKeyPair via the public umbrella header.
MSAL/src/MSALExternalKeyPair+Internal.h	Internal bridge to IdentityCore key-pair representation.
MSAL/src/MSALExternalKeyPair.m	Implements validation + keyId derivation and error mapping.
MSAL/src/MSALAuthenticationSchemePop.m	Uses external PoP manager when provided and adds broker classification parameter.
MSAL/MSAL.xcodeproj/project.pbxproj	Wires new sources/headers/tests into build targets.
CHANGELOG.md	Records the new API and dependency/update notes under [TBD].

Suppressed comments (3)

MSAL/test/unit/MSALAuthSchemeTests.m:279

• Issue: createRSA2048PrivateKey doesn’t release the CFErrorRef returned by SecKeyCreateRandomKey when key creation fails.
  Impact: Unit tests can leak CoreFoundation objects, masking real leak regressions.
  Recommendation: Release error when non-NULL after the assertions.

    CFErrorRef error = NULL;
    SecKeyRef privateKey = SecKeyCreateRandomKey((__bridge CFDictionaryRef)attributes, &error);
    XCTAssertNotEqual(privateKey, NULL);
    XCTAssertEqual(error, NULL);
    return privateKey;

MSAL/src/MSALAuthenticationSchemePop.m:56

• Issue: The initializer declaration in the .m file uses nonnull nonce/additionalParameters, but the public header declares them nullable and the implementation already handles nil by generating defaults.
  Impact: This internal/public nullability mismatch can trigger compiler warnings and makes the implementation signature inconsistent with the public API.
  Recommendation: Update the initializer declaration in the implementation to accept nullable values, matching the header.

- (instancetype)initWithHttpMethod:(MSALHttpMethod)httpMethod
                        requestUrl:(NSURL *)requestUrl
                             nonce:(NSString *)nonce
              additionalParameters:(NSDictionary *)additionalParameters

MSAL/src/MSALAuthenticationSchemePop.m:69

• Issue: The external-key initializer implementation also declares nonce/additionalParameters as nonnull, while the public header declares them nullable and the implementation supports nil.
  Impact: This can lead to nullability-mismatch warnings and makes the implementation inconsistent with the public API contract.
  Recommendation: Mark nonce and additionalParameters as nullable in this initializer declaration as well.

- (instancetype)initWithHttpMethod:(MSALHttpMethod)httpMethod
                        requestUrl:(NSURL *)requestUrl
                             nonce:(NSString *)nonce
              additionalParameters:(NSDictionary *)additionalParameters
                   externalKeyPair:(nullable MSALExternalKeyPair *)externalKeyPair

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

MSAL/src/MSALExternalKeyPair.m:40

• Issue: When external-key validation fails, the returned NSError drops the underlying validationError.
  Impact: Callers lose diagnostic detail (and any structured info in the underlying error), making it harder to troubleshoot why the key pair is rejected.
  Recommendation: Preserve validationError under NSUnderlyingErrorKey when constructing MSALErrorInvalidExternalKeyPair.

static MSALExternalKeyPairFailureReason MSALFailureReasonFromMSIDReason(MSIDExternalKeyPairValidationFailureReason reason)
{
    switch (reason)
    {

MSAL/src/public/MSALExternalKeyPair.h:39

• Issue: The initializer parameters are implicitly nonnull (inside NS_ASSUME_NONNULL_BEGIN), but unit tests intentionally pass NULL key handles to validate error behavior.
  Impact: This will trigger -Wnonnull warnings (and can break builds if warnings are treated as errors), and the header doesn’t reflect the implementation’s ability to gracefully reject NULL.
  Recommendation: Mark the SecKeyRef parameters as nullable (and keep returning nil + MSALErrorInvalidExternalKeyPair for invalid handles).

    MSALExternalKeyPairFailureReasonKeySizeTooSmall,
    MSALExternalKeyPairFailureReasonInvalidKeyClass,
    MSALExternalKeyPairFailureReasonNotSigningCapable,

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

MSAL/src/MSALExternalKeyPair.m:68

• Issue: The implementation signature for initWithPrivateKey:publicKey:error: uses nonnull SecKeyRef types, but the initializer’s behavior supports NULL inputs (it returns MSALErrorInvalidExternalKeyPair).
  Impact: If the public header is updated to accept nullable keys (to reflect actual behavior), the implementation should match to avoid nullability mismatches and warnings.
  Recommendation: Update the method signature here to accept _Nullable SecKeyRef inputs (no behavioral change).

- (nullable instancetype)initWithPrivateKey:(SecKeyRef)privateKey
                                  publicKey:(SecKeyRef)publicKey
                                      error:(NSError * _Nullable __autoreleasing * _Nullable)error

MSAL/src/public/MSALExternalKeyPair.h:57

• Issue: initWithPrivateKey:publicKey:error: is declared under NS_ASSUME_NONNULL_BEGIN with nonnull SecKeyRef parameters, but the API explicitly validates for NULL handles (and unit tests pass NULL).
  Impact: Callers (and tests) can get compile-time warnings for passing NULL to nonnull parameters, which is inconsistent with the intended “failable initializer” contract.
  Recommendation: Mark the SecKeyRef parameters as _Nullable to match the validation behavior and avoid false-positive nonnull diagnostics.

- (nullable instancetype)initWithPrivateKey:(SecKeyRef)privateKey
                                  publicKey:(SecKeyRef)publicKey
                                      error:(NSError * _Nullable __autoreleasing * _Nullable)error NS_DESIGNATED_INITIALIZER;

[Antonio Alwan (antonioalwan)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in 841b74d (merge of origin/dev).

Resolutions:

• CHANGELOG.md: kept the #TBD heading from dev and appended the two AT PoP external-key entries under it.
• MSAL/MSAL.xcodeproj/project.pbxproj: all 13 conflict hunks were additive entries on both sides (PBXBuildFile, PBXFileReference, and Sources/Headers lists), so both sides were kept. Two duplicated AuthorityURLFormat.swift build-file entries created by the union were removed, and I verified there are no duplicate object IDs, no unresolved references, and balanced braces.
• MSAL/IdentityCore: kept this branch's pointer (409867e), which is the head of the dependency PR [minor] [feature]: Add external key support for AT PoP microsoft-authentication-library-common-for-objc#1934 and is based on a newer CommonCore dev than the pointer on dev (9f25721). This pointer will be updated to the merged CommonCore commit once Merge release 1220 into dev #1934 lands.

The PR no longer reports conflicts; remaining blocked state is from required reviews/checks.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

MSAL/src/MSALAuthenticationSchemePop.m:83

• Severity: Medium – This implementation depends on new IdentityCore APIs (MSIDDevicePopManager initWithExternalKeyPair:) that aren’t part of MSAL itself.

Issue: MSALAuthenticationSchemePop now calls [[MSIDDevicePopManager alloc] initWithExternalKeyPair:externalKeyPair.msidKeyPair], so the build requires an IdentityCore update that provides this initializer.
Impact: If the IdentityCore submodule pointer (and any packaging references) aren’t updated in the same PR/release, MSAL will fail to compile/link.
Recommendation: Ensure the IdentityCore submodule commit included in this PR contains initWithExternalKeyPair: (and any related constants like MSID_OAUTH2_EXTERNAL_KEY_POP) and that CI validates with git submodule update --init --recursive.

        _externalKeyPair = externalKeyPair;
        if (externalKeyPair)
        {
            _externalPopManager = [[MSIDDevicePopManager alloc] initWithExternalKeyPair:externalKeyPair.msidKeyPair];
        }

MSAL/src/MSALExternalKeyPair.m:76

• Severity: Medium – This new public API wrapper relies on IdentityCore symbols that must exist in the pinned submodule version.

Issue: MSALExternalKeyPair uses MSIDExternalKeyPairValidationFailureReason and -[MSIDKeyOperationUtil validateExternalRSAKeyPair:publicKey:failureReason:context:error:].
Impact: If the IdentityCore submodule pointer/dependency isn’t updated to a version that defines these, MSAL won’t compile.
Recommendation: Make sure the PR includes (or is paired with) the IdentityCore submodule update referenced in the PR description, and that CI builds with that updated submodule commit.

    MSIDExternalKeyPairValidationFailureReason validationReason = MSIDExternalKeyPairValidationFailureReasonNone;
    NSError *validationError = nil;
    BOOL valid = [[MSIDKeyOperationUtil sharedInstance] validateExternalRSAKeyPair:privateKey
                                                                         publicKey:publicKey
                                                                     failureReason:&validationReason
                                                                           context:nil
                                                                             error:&validationError];

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (2)

MSAL/src/MSALExternalKeyPair+Internal.h:36

• Issue: msidKeyPair is exposed as a read/write property in the internal class extension.
  Impact: This makes it easy for other internal code (or tests) to mutate the key pair after validation, undermining the immutability assumptions of MSALExternalKeyPair and complicating debugging if the key identifier changes unexpectedly.
  Recommendation: Make the internal msidKeyPair property read-only and set it only during initialization.

@property (nonatomic) MSIDAssymetricKeyPair *msidKeyPair;

MSAL/src/MSALExternalKeyPair.m:123

• Issue: The failure reason is always reported as MSALExternalKeyPairFailureReasonPublicKeySerializationFailed when _msidKeyPair is nil or when _msidKeyPair.kid is nil.
  Impact: This can misclassify the validation category exposed via MSALExternalKeyPairFailureReasonKey, making it harder for callers to distinguish “failed to construct/derive key info” vs “constructed but couldn’t serialize/compute kid”.
  Recommendation: Report PublicKeyDerivationFailed when _msidKeyPair is nil, and reserve PublicKeySerializationFailed for cases where the key pair object exists but kid is missing.

        if (!_msidKeyPair || !_msidKeyPair.kid)
        {
            if (error)
            {
                *error = [NSError errorWithDomain:MSALErrorDomain

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (9)

MSAL/test/unit/MSALExternalKeyPairTests.m:87

• Issue: SecKeyCopyPublicKey can return NULL, but this test doesn't assert it before later using/releasing the reference.
  Impact: A NULL publicKey would cause CFRelease(NULL) (crash) and mask the real failure.
  Recommendation: Add an XCTAssertNotEqual(publicKey, NULL) immediately after copying the public key.

    SecKeyRef privateKey = [self createPrivateKeyWithType:kSecAttrKeyTypeRSA size:@2048];
    SecKeyRef publicKey = SecKeyCopyPublicKey(privateKey);
    NSError *error = nil;

MSAL/test/unit/MSALExternalKeyPairTests.m:139

• Issue: SecKeyCopyPublicKey can return NULL, but this test doesn't assert it before later using/releasing the reference.
  Impact: A NULL publicKey can crash the test (CFRelease(NULL)) and obscure the true failure.
  Recommendation: Assert publicKey != NULL right after SecKeyCopyPublicKey.

    SecKeyRef privateKey = [self createPrivateKeyWithType:kSecAttrKeyTypeECSECPrimeRandom size:@256];
    SecKeyRef publicKey = SecKeyCopyPublicKey(privateKey);
    NSError *error = nil;

MSAL/test/unit/MSALExternalKeyPairTests.m:156

• Issue: SecKeyCopyPublicKey can return NULL, but this test doesn't assert it before later using/releasing the reference.
  Impact: If NULL, this can crash the test suite (CFRelease(NULL)) instead of producing a clear assertion failure.
  Recommendation: Add an XCTAssertNotEqual(publicKey, NULL) after SecKeyCopyPublicKey.

    SecKeyRef privateKey = [self createPrivateKeyWithType:kSecAttrKeyTypeRSA size:@1024];
    SecKeyRef publicKey = SecKeyCopyPublicKey(privateKey);
    NSError *error = nil;

MSAL/test/unit/MSALExternalKeyPairTests.m:174

• Issue: SecKeyCopyPublicKey can return NULL, but this test doesn't assert it before later using/releasing the reference.
  Impact: A NULL otherPublicKey would lead to CFRelease(NULL) (crash), hiding the root cause.
  Recommendation: Assert otherPublicKey != NULL immediately after SecKeyCopyPublicKey.

    SecKeyRef privateKey = [self createPrivateKeyWithType:kSecAttrKeyTypeRSA size:@2048];
    SecKeyRef otherPrivateKey = [self createPrivateKeyWithType:kSecAttrKeyTypeRSA size:@2048];
    SecKeyRef otherPublicKey = SecKeyCopyPublicKey(otherPrivateKey);
    NSError *error = nil;

MSAL/test/unit/MSALExternalKeyPairTests.m:192

• Issue: SecKeyCopyPublicKey can return NULL, but this test doesn't assert it before later using/releasing the reference.
  Impact: A NULL publicKey can crash the test (CFRelease(NULL)) and make the failure non-actionable.
  Recommendation: Add an assertion that publicKey is non-NULL immediately after copying it.

    SecKeyRef privateKey = [self createPrivateKeyWithType:kSecAttrKeyTypeRSA size:@2048];
    SecKeyRef publicKey = SecKeyCopyPublicKey(privateKey);
    NSError *error = nil;

MSAL/src/MSALExternalKeyPair.m:119

• Issue: This NSError userInfo sets MSALErrorDescriptionKey but not NSLocalizedDescriptionKey.
  Impact: Callers that read error.localizedDescription won’t see the provided message, which reduces diagnosability for a new public API.
  Recommendation: Add NSLocalizedDescriptionKey with the same string value.

                *error = [NSError errorWithDomain:MSALErrorDomain
                                             code:MSALErrorInvalidExternalKeyPair
                                         userInfo:@{
                                             MSALErrorDescriptionKey : @"Unable to derive a public key thumbprint.",
                                             MSALExternalKeyPairFailureReasonKey : @(MSALExternalKeyPairFailureReasonPublicKeySerializationFailed)
                                         }];

MSAL/test/unit/MSALExternalKeyPairTests.m:52

• Issue: SecKeyCopyPublicKey can return NULL, but these tests don't assert it before using/releasing the returned reference.
  Impact: A NULL publicKey here would lead to CFRelease(NULL) (crash) and make failures harder to diagnose.
  Recommendation: Assert publicKey != NULL immediately after SecKeyCopyPublicKey in each test that uses it.

This issue also appears in the following locations of the same file:

• line 85
• line 137
• line 154
• line 171
• line 190

    SecKeyRef privateKey = [self createPrivateKeyWithType:kSecAttrKeyTypeRSA size:@2048];
    SecKeyRef publicKey = SecKeyCopyPublicKey(privateKey);
    NSError *error = nil;

MSAL/src/MSALExternalKeyPair.m:89

• Issue: The constructed NSError userInfo sets MSALErrorDescriptionKey but not NSLocalizedDescriptionKey.
  Impact: Callers that surface error.localizedDescription (common) will likely get a generic "(MSALErrorDomain error -50009)" message instead of the actionable validation description.
  Recommendation: Populate NSLocalizedDescriptionKey alongside MSALErrorDescriptionKey with the same string.

This issue also appears on line 114 of the same file.

            NSString *description = validationError.localizedDescription ?: @"Invalid external AT PoP key pair.";
            NSMutableDictionary *userInfo = [@{
                MSALErrorDescriptionKey : description,
                MSALExternalKeyPairFailureReasonKey : @(MSALFailureReasonFromMSIDReason(validationReason))
            } mutableCopy];

MSAL/src/public/MSALAuthenticationSchemePop.h:55

• Issue: The new designated initializer can return nil but provides no NSError ** out parameter (it currently fails by returning nil and logging internally).
  Impact: Callers have no actionable way to learn why initialization failed for a new public API (they typically rely on NSError + localizedDescription rather than internal logs).
  Recommendation: Since this initializer is newly introduced, consider adding an optional error:(NSError **)error parameter and populate it when returning nil (mirroring other MSAL fallible initializers).

- (nullable instancetype)initWithHttpMethod:(MSALHttpMethod)httpMethod
                                  requestUrl:(NSURL *)requestUrl
                                       nonce:(nullable NSString *)nonce
                        additionalParameters:(nullable NSDictionary *)additionalParameters
                             externalKeyPair:(nullable MSALExternalKeyPair *)externalKeyPair NS_DESIGNATED_INITIALIZER;

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (2)

MSAL/src/MSALAuthenticationSchemePop.m:147

• Issue: When createSignedAccessToken:... error:error fails, this method unconditionally overwrites any error it produced with a generic MSIDCreateError(...).
  Impact: Callers lose the underlying failure details (e.g., signing/crypto/key-specific errors), making external-key PoP failures harder to diagnose.
  Recommendation: Preserve the error from createSignedAccessToken and only create the generic fallback error when no error was provided.

    MSIDDevicePopManager *manager = self.externalPopManager ?: popManager;
    NSString *signedAccessToken = [manager createSignedAccessToken:accessToken.accessToken
                                                        httpMethod:MSALParameterStringForHttpMethod(self.httpMethod)
                                                        requestUrl:self.requestUrl.absoluteString
                                                             nonce:self.nonce
                                                             error:error];

MSAL/src/MSALExternalKeyPair.m:119

• Issue: This error path says it was "Unable to derive a public key thumbprint", but reports MSALExternalKeyPairFailureReasonPublicKeySerializationFailed.
  Impact: The failure reason exposed to SDK consumers can be misleading (thumbprint derivation vs serialization), making it harder to take the right remediation steps.
  Recommendation: Align the failure reason with the message (or adjust the message). If the failure is specifically about deriving the RFC 7638 thumbprint (kid), use MSALExternalKeyPairFailureReasonPublicKeyDerivationFailed.

                *error = [NSError errorWithDomain:MSALErrorDomain
                                             code:MSALErrorInvalidExternalKeyPair
                                         userInfo:@{
                                             MSALErrorDescriptionKey : @"Unable to derive a public key thumbprint.",
                                             MSALExternalKeyPairFailureReasonKey : @(MSALExternalKeyPairFailureReasonPublicKeySerializationFailed)
                                         }];

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (3)

MSAL/src/public/MSALAuthenticationSchemePop.h:55

• Issue: The new designated initializer can return nil (it’s declared nullable) but provides no NSError out-parameter and the header comment doesn’t state when/why initialization can fail.
  Impact: Callers have no deterministic way to diagnose failures (especially if MSAL logging isn’t enabled), which makes this public API harder to use and support.
  Recommendation: Either (a) add an NSError ** out-parameter to this initializer and document the failure conditions, or (b) make the initializer nonnull and treat external key manager initialization failures as programmer errors (assert) since MSALExternalKeyPair is expected to already be validated.

- (nullable instancetype)initWithHttpMethod:(MSALHttpMethod)httpMethod
                                  requestUrl:(NSURL *)requestUrl
                                       nonce:(nullable NSString *)nonce
                        additionalParameters:(nullable NSDictionary *)additionalParameters
                             externalKeyPair:(nullable MSALExternalKeyPair *)externalKeyPair NS_DESIGNATED_INITIALIZER;

CHANGELOG.md:4

• Issue: The changelog entry states “Update IdentityCore with external RSA key validation and injected PoP manager support.”, but this PR’s diff doesn’t include a submodule pointer bump for MSAL/IdentityCore.
  Impact: Without the submodule update, the new MSAL code that depends on these IdentityCore APIs may not build for consumers/CI, and the changelog becomes misleading.
  Recommendation: Either bump the MSAL/IdentityCore submodule to the commit that contains the referenced CommonCore change (and keep this bullet), or adjust the changelog entry to reflect the actual dependency/update included in this PR.

#TBD
* Add SSPR V2 #3039
* Add `MSALExternalKeyPair` and an additive AT PoP authentication-scheme initializer for caller-owned RSA keys.
* Update IdentityCore with external RSA key validation and injected PoP manager support.

MSAL/src/MSALExternalKeyPair.m:123

• Issue: The failure reason reported when _msidKeyPair initialization fails vs when _msidKeyPair.kid is missing is always MSALExternalKeyPairFailureReasonPublicKeySerializationFailed.
  Impact: Callers relying on MSALExternalKeyPairFailureReasonKey can’t distinguish serialization failure from thumbprint derivation failure, even though the public enum exposes both categories.
  Recommendation: Set the failure reason based on whether _msidKeyPair is nil (serialization) or kid is nil (derivation).

        if (!_msidKeyPair || !_msidKeyPair.kid)
        {
            if (error)

[Yong Zeng (jasoncoolmax)]

This initializer can return nil when the external pop manager fails to init, but we only MSID_LOG the reason in the .m, so the caller just gets nil with nothing to go on. MSALExternalKeyPair's init already surfaces MSALErrorInvalidExternalKeyPair + MSALExternalKeyPairFailureReasonKey, will it be better to add an error: param here too so it is consistent? testExternalKeyPair_whenInternalPairIsMissing_shouldFailInitialization shows the nil path is reachable, and caller-owned keys can go bad after MSALExternalKeyPair was created (deleted / invalidated), so this is likely to be hit in the field :)