Closed RoelVB closed 2 months ago
It's about this code, where this.config.auth.clientCertificate
can be undefined
:
https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/2ff18f23f0631507ccbf966478b152b02c9d25af/lib/msal-node/src/client/ConfidentialClientApplication.ts#L223-L226
@RoelVB Thanks for reporting this. What app type are you using? ConfidentialClientApplication
, ClientCredentialClient
, etc
Encountered this using ConfidentialClientApplication
Thanks, I'll be looking into this immediately as well as any tests that may have missed this regression.
@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?
@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:
npm i
.env
file with the contents:
CLIENTID="01234567-89ab-cdef-0123-456789abcdef"
CLIENTSECRET="VGhpcyBpcyB2ZXJ5IHNlY3JldCE="
CLIENTTENANT="contoso"
MAILBOX="test@example.com"
ADDITIONALRECIPIENT="test2@example.com"
npm run test:send
logs/error.log
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.
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.
@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: https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/e6d18c5140359145e0142d4ee4854367fd5da078/lib/msal-node/src/client/ConfidentialClientApplication.ts#L224-L227
And because of this line, the issues is not caught while building (or by your IDE):
https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/aa24330717b5e1f2f98c9d534d3766caefbaa452/lib/msal-node/src/config/Configuration.ts#L192
Because now all auth
properties are typed as "defined".
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:
#msalClient
without clientSecret
or clientCertificate
Config.clientCertificateThumbprint && Config.clientCertificateKeyPath
clientCertificate
to #msalClient
clientSecret
, add clientSecret
to #msalClient
Regardless, the code https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/5eabb42fe253ca40be6ded4158e63da2b8a2c6ec/lib/msal-node/src/config/Configuration.ts#L228-L234
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
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
?
https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/5eabb42fe253ca40be6ded4158e63da2b8a2c6ec/lib/msal-node/src/config/Configuration.ts#L121-L127
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: https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/5eabb42fe253ca40be6ded4158e63da2b8a2c6ec/lib/msal-node/src/config/Configuration.ts#L191-L197
Ok. I see what you're saying now. I could fix your issue by changing
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:
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
.
So this method should transform Configuration
to NodeConfiguration
, but it's not guaranteed:
https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/22651b30bce9244cce2d7ffcfa1a81e4c44be133/lib/msal-node/src/config/Configuration.ts#L210-L243
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
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.
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.
@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
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-MicrosoftError Message
TypeError: Cannot read properties of undefined (reading 'thumbprint')
MSAL Logs
No response
Network Trace (Preferrably Fiddler)
MSAL Configuration
Relevant Code Snippets
Reproduction Steps
Try authenticating without setting the
clientCertificate
property in theauth
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)