setClientCredential() errors when no clientCertificate property is set

[RoelVB]

Core Library

MSAL Node (@azure/msal-node)

Core Library Version

2.11.0

Wrapper Library

Not Applicable

Wrapper Library Version

None

Public or Confidential Client?

Confidential

Description

This commit stops checking if config.auth.clientCertificate is actually defined.
CC: @Robbie-Microsoft

Error Message

TypeError: Cannot read properties of undefined (reading 'thumbprint')

MSAL Logs

No response

Network Trace (Preferrably Fiddler)

• Sent
• Pending

MSAL Configuration

auth: {
    authority: 'url',
    clientId: 'guid',
    clientSecret: 'secret',
}

Relevant Code Snippets

N/A

Reproduction Steps

Try authenticating without setting the clientCertificate property in the auth

Expected Behavior

Auth using clientSecret

Identity Provider

Entra ID (formerly Azure AD) / MSA

Browsers Affected (Select all that apply)

None (Server)

Regression

2.10.0

Source

External (Customer)

[RoelVB]

It's about this code, where this.config.auth.clientCertificate can be undefined:

microsoft-authentication-library-for-js/lib/msal-node/src/client/ConfidentialClientApplication.ts

Lines 223 to 226 in 2ff18f2

	const certificateNotEmpty =
	(!!this.config.auth.clientCertificate.thumbprint ||
	!!this.config.auth.clientCertificate.thumbprintSha256) &&
	!!this.config.auth.clientCertificate.privateKey;

[Robbie-Microsoft]

@RoelVB Thanks for reporting this. What app type are you using? ConfidentialClientApplication, ClientCredentialClient, etc

[RoelVB]

Encountered this using ConfidentialClientApplication

[Robbie-Microsoft]

Thanks, I'll be looking into this immediately as well as any tests that may have missed this regression.

[Robbie-Microsoft]

@RoelVB Sorry, I'm not able to reproduce this yet. Are you acquiring a token via cca.acquireTokenByClientCredential? Can you share your acquire token code with me here?

[RoelVB]

@Robbie-Microsoft
Yes. I'm calling acquireTokenByClientCredential.

This is the code that works on 2.10.0, but not on 2.11.0:
https://github.com/SMTP2Graph/SMTP2Graph/blob/aaa5f5b8db38230b8280eb6714819625b7530ee9/src/classes/Mailer.ts#L21-L31

This is the code that requests the token:
https://github.com/SMTP2Graph/SMTP2Graph/blob/aaa5f5b8db38230b8280eb6714819625b7530ee9/src/classes/Mailer.ts#L151-L161

If you want you can clone that repo to test. From the top of my head:

1. Clone
2. npm i
3. Upgrade @azure/msal-node to 2.11.0
4. Add an .env file with the contents:

CLIENTID="01234567-89ab-cdef-0123-456789abcdef"
CLIENTSECRET="VGhpcyBpcyB2ZXJ5IHNlY3JldCE="
CLIENTTENANT="contoso"
MAILBOX="[email protected]"
ADDITIONALRECIPIENT="[email protected]"

5. Run npm run test:send
6. The error will be in logs/error.log

[Robbie-Microsoft]

Per the code in the first link, you are providing clientSecret AND clientCertificate. You can only provide one OR the other (OR clientAssertion). Can you delete the clientSecret line and try again?

However, I don't know why you'd receive the error you posted above: "TypeError: Cannot read properties of undefined (reading 'thumbprint')". I'll look into this tomorrow.

Let me know if deleting the clientSecret line and using ONLY clientCertificate works.

[RoelVB]

Sorry, the supplied code can be a little confusing. clientSecret or clientCertificate is defined, not both. The other will be undefined.

I will probably be able to do some more tests in about 8-9 hours from now.

[RoelVB]

@Robbie-Microsoft
I did some more testing and found something interesting.

This works:

auth: {
    authority: '<url>',
    clientId: '<guid>',
    clientSecret: '<secret>',
}

This throws the error mentioned:

auth: {
    authority: '<url>',
    clientId: '<guid>',
    clientSecret: '<secret>',
    clientCertificate: undefined,
}

So as of 2.11.0 the code checks if the key exists, but not if it's actually defined.

The 2.10.0 code had this to make sure it's defined:

microsoft-authentication-library-for-js/lib/msal-node/src/client/ConfidentialClientApplication.ts

Lines 224 to 227 in e6d18c5

	const certificate = configuration.auth.clientCertificate || {
	thumbprint: Constants.EMPTY_STRING,
	privateKey: Constants.EMPTY_STRING,
	};

And because of this line, the issues is not caught while building (or by your IDE):

microsoft-authentication-library-for-js/lib/msal-node/src/config/Configuration.ts

Line 192 in aa24330

auth: Required<NodeAuthOptions>;

Because now all auth properties are typed as "defined".

[Robbie-Microsoft]

I came to the same conclusion as you and then came back to this page to see that you posted what I was thinking :) . I was able to reproduce your error by passing this into the config for the ConfidentialClientApplication:

clientSecret: "fake_secret", clientCertificate: undefined,

I think this is technically an error on your end, as clientCertificate should not be passed in as undefined, even if it's not defined. Also, you're technically passing in both clientSecret and clientCertificate (even though it's undefined) when you should only be passing in one or the other (or clientAssertion). If you pass in both clientSecret and clientCertificate (expected values instead of undefined), then you would receive the expected error: "An application should at most have one credential".

I believe you would have prevented this error on your end if you typed your config as Configuration, the type from msal-node that ConfidentialClientApplication expects to be passed into it.

With your current setup, you could do something like:

1. define #msalClient without clientSecret or clientCertificate
2. if Config.clientCertificateThumbprint && Config.clientCertificateKeyPath
3. add clientCertificate to #msalClient
4. else if clientSecret, add clientSecret to #msalClient
5. else, throw error

Regardless, the code

microsoft-authentication-library-for-js/lib/msal-node/src/config/Configuration.ts

Lines 228 to 234 in 5eabb42

	if (
	!!auth.clientCertificate &&
	!!!auth.clientCertificate.thumbprint &&
	!!!auth.clientCertificate.thumbprintSha256
	) {
	throw NodeAuthError.createStateNotFoundError();
	}

is not working as expected, and should throw an error if clientCertificate is passed in as undefined.

I will get a PR out soon that checks if clientCertificate is passed in as undefined and throw an appropriate error if so

[RoelVB]

I believe you would have prevented this error on your end if you typed your config as Configuration, the type from msal-node that ConfidentialClientApplication expects to be passed into it.

You mean this Configuration?

microsoft-authentication-library-for-js/lib/msal-node/src/config/Configuration.ts

Lines 121 to 127 in 5eabb42

	export type Configuration = {
	auth: NodeAuthOptions;
	broker?: BrokerOptions;
	cache?: CacheOptions;
	system?: NodeSystemOptions;
	telemetry?: NodeTelemetryOptions;
	};

That's what I use and it accepts auth.clientCertificate to be undefined.

But for some reason the typing is changed here, and used internally, which will make you miss some potential issues:

microsoft-authentication-library-for-js/lib/msal-node/src/config/Configuration.ts

Lines 191 to 197 in 5eabb42

	export type NodeConfiguration = {
	auth: Required<NodeAuthOptions>;
	broker: BrokerOptions;
	cache: CacheOptions;
	system: Required<NodeSystemOptions>;
	telemetry: Required<NodeTelemetryOptions>;
	};

[Robbie-Microsoft]

Ok. I see what you're saying now. I could fix your issue by changing

microsoft-authentication-library-for-js/lib/msal-node/src/client/ConfidentialClientApplication.ts

Lines 223 to 226 in 5eabb42

	const certificateNotEmpty =
	(!!this.config.auth.clientCertificate.thumbprint ||
	!!this.config.auth.clientCertificate.thumbprintSha256) &&
	!!this.config.auth.clientCertificate.privateKey;

to

const certificateNotEmpty = 
     (!!this.config.auth.clientCertificate?.thumbprint || 
         !!this.config.auth.clientCertificate?.thumbprintSha256) && 
     !!this.config.auth.clientCertificate?.privateKey;

However, I don't believe it's our intention to allow you to pass in clientCertificate as undefined. I think this is a bug on our end. The type Configuration -> NodeAuthOptions doesn't allow clientCertificate to have a value of undefined, it just allows clientCertificate to be optionally defined. If it is defined, it must have this format:

microsoft-authentication-library-for-js/lib/msal-node/src/config/Configuration.ts

Lines 48 to 57 in 5eabb42

	clientCertificate?: {
	/**
	* @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;
	};

We do convert Configuration to NodeConfiguration to assign default values to certain properties that weren't defined on Configuration.

Let me discuss this with @bgavrilMS and confirm that we don't want to allow developers to pass in clientCertificate as undefined.

[RoelVB]

So this method should transform Configuration to NodeConfiguration, but it's not guaranteed:

microsoft-authentication-library-for-js/lib/msal-node/src/config/Configuration.ts

Lines 210 to 243 in 22651b3

	export function buildAppConfiguration({
	auth,
	broker,
	cache,
	system,
	telemetry,
	}: Configuration): NodeConfiguration {
	const systemOptions: Required<NodeSystemOptions> = {
	...DEFAULT_SYSTEM_OPTIONS,
	networkClient: new HttpClient(
	system?.proxyUrl,
	system?.customAgentOptions as http.AgentOptions | https.AgentOptions
	),
	loggerOptions: system?.loggerOptions || DEFAULT_LOGGER_OPTIONS,
	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 },
	cache: { ...DEFAULT_CACHE_OPTIONS, ...cache },
	system: { ...systemOptions, ...system },
	telemetry: { ...DEFAULT_TELEMETRY_OPTIONS, ...telemetry },
	};
	}

In my case this line:

auth: { ...DEFAULT_AUTH_OPTIONS, ...auth },

Actually does this:

auth: { ...{clientCertificate: {/* default config */}}, ...{clientCertificate: undefined} },

Which results in:

auth: { clientCertificate: undefined },

---

A fix for this could be:

auth: { ...DEFAULT_AUTH_OPTIONS, ...auth, clientCertificate: { ...DEFAULT_AUTH_OPTIONS.clientCertificate, ...auth.clientCertificate } },

But to be sure, you would also have to do this for DEFAULT_AUTH_OPTIONS.azureCloudOptions and the same for DEFAULT_TELEMETRY_OPTIONS.application

[ngbrown]

Using new ConfidentialClientApplication was working for me in @azure/msal-node: 2.9.2, but now in @azure/msal-node: 2.11.0 I'm getting errors "TypeError: Cannot read properties of undefined (reading 'thumbprint')".

However, I don't believe it's our intention to allow you to pass in clientCertificate as undefined.

TypeScript rules implies that clientCertificate?: {...} allows for undefined to be directly assigned to to the property. This is relevant when trying to set these values from a configuration and all the values are going to be set to something. The code should check for value existence, not just the object key being defined.

[yuezk]

We encountered the same issue with the BotFramework SDK. We're using the client ID and client secret as the client credential. Below are the call stacks. And it seems there is no way to get around this.

[Robbie-Microsoft]

@RoelVB I agree with your analysis about what the root cause is. However, this behavior (root cause) already existed before the regression, and the PR I linked fixes the regression.

I'm going to keep the PR as is, and have created a new GitHub issue to track the root cause: #7221