Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

acquireTokenByClientCredential broken for clientCertificate #7082

Closed
2 tasks
ericcan opened this issue May 7, 2024 · 19 comments · Fixed by #7088
Closed
2 tasks

acquireTokenByClientCredential broken for clientCertificate #7082

ericcan opened this issue May 7, 2024 · 19 comments · Fixed by #7088
Assignees
Labels
bug A problem that needs to be fixed for the feature to function as intended. confidential-client Issues regarding ConfidentialClientApplications msal-node Related to msal-node package p1 P1 and P2 are priorities of the bug. P1 bugs should get Fixed/Closed within 4 weeks.

Comments

@ericcan
Copy link

ericcan commented May 7, 2024

Core Library

MSAL Node (@azure/msal-node)

Core Library Version

2.8.0

Wrapper Library

Not Applicable

Wrapper Library Version

n/a

Public or Confidential Client?

Confidential

Description

I upgraded to the latest version (2.8 from 2.7) and my call to acquireTokenByClientCredential fails now.

Error Message

{
"errorCode": "invalid_client",
"errorMessage": "7000216 - [2024-05-07 15:55:13Z]: AADSTS7000216: 'client_assertion', 'client_secret' or 'request' is required for the 'client_credentials' grant type. Trace ID: d1a0baab-1cf3-4a20-a263-605f65ceb200 Correlation ID: a93c308b-63ff-4880-8d07-383a27828034 Timestamp: 2024-05-07 15:55:13Z - Correlation ID: a93c308b-63ff-4880-8d07-383a27828034 - Trace ID: d1a0baab-1cf3-4a20-a263-605f65ceb200",
"subError": "",
"name": "ServerError",
"errorNo": 7000216,
"correlationId": "2bedce44-4ef7-4752-abb9-39cfe913cea3"
}

MSAL Logs

No response

Network Trace (Preferrably Fiddler)

  • Sent
  • Pending

MSAL Configuration

const clientConfig: Configuration = {
      auth: {
                clientCertificate: {
                    thumbprint,
                    privateKey
                },
                clientId: ,
                authority
            }
        }

Relevant Code Snippets

const pca = new ConfidentialClientApplication(clientConfig);
        const clientCredentialRequest: ClientCredentialRequest = {
            scopes: ["https://graph.microsoft.com/.default"]
        };
        const newToken = pca
            .acquireTokenByClientCredential(clientCredentialRequest)

Reproduction Steps

Run the code above

Expected Behavior

A token is returned

Identity Provider

Entra ID (formerly Azure AD) / MSA

Browsers Affected (Select all that apply)

Chrome

Regression

@azure/msal-node @2.7.0

Source

External (Customer)

@ericcan ericcan added bug-unconfirmed A reported bug that needs to be investigated and confirmed question Customer is asking for a clarification, use case or information. labels May 7, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Attention 👋 Awaiting response from the MSAL.js team label May 7, 2024
@github-actions github-actions bot added confidential-client Issues regarding ConfidentialClientApplications msal-node Related to msal-node package labels May 7, 2024
@aleclair-KoAi
Copy link

I have the same problem

@joaquingomez-tc
Copy link

Hi, this started happening to me and my team as well for newer builds, as a workaround we had to downgrade to version 2.6.4

@wenytang-ms
Copy link

wenytang-ms commented May 8, 2024

get same error on version 2.6.*
ServerError: invalid_client: 7000218 - [2024-05-08 14:38:58Z]: AADSTS7000218: The request body must contain the following parameter: 'client_assertion' or 'client_secret'. Trace ID: 43aaea61-54e5-4a8f-8a14-e7e12142f800 Correlation ID: 788b70bc-ed5b-4b63-90a1-f93cf45b04f8 Timestamp: 2024-05-08 14:38:58Z - Correlation ID: 788b70bc-ed5b-4b63-90a1-f93cf45b04f8 - Trace ID: 43aaea61-54e5-4a8f-8a14-e7e12142f800

@neha-bhargava neha-bhargava added bug A problem that needs to be fixed for the feature to function as intended. and removed question Customer is asking for a clarification, use case or information. bug-unconfirmed A reported bug that needs to be investigated and confirmed Needs: Attention 👋 Awaiting response from the MSAL.js team labels May 8, 2024
@bgavrilMS bgavrilMS added the p1 P1 and P2 are priorities of the bug. P1 bugs should get Fixed/Closed within 4 weeks. label May 8, 2024
@bgavrilMS
Copy link
Member

@KarishmaGhiya - please see this bug. I recommend you don't take dependency on MSAL 2.8

@Robbie-Microsoft
Copy link
Collaborator

@ericcan @aleclair-KoAi @joaquingomez-tc @wenytang-ms I can only reproduce this issue when thumbprint and privateKey are both empty strings. Can you all doublecheck your thumbprint and privateKey values and let me know if they're empty strings or not?

@ericcan
Copy link
Author

ericcan commented May 8, 2024

Neither the privateKey or thumbprint are empty strings in my case. The exact same calls do work with 2.7.

Not sure if it's helpful, but this is some call stack info that leads to the error (it's from a bundle, so the line numbers don't mean much but I could get you some data on the specific lines if it helps track this down).

 "    at _ResponseHandler.validateTokenResponse (/var/task/index.js:14121:27)",
    "    at ClientCredentialClient.executeTokenRequest (/var/task/index.js:17286:21)",
    "    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)",
    "    at async ConfidentialClientApplication.acquireTokenByClientCredential (/var/task/index.js:17572:14)"

@Robbie-Microsoft
Copy link
Collaborator

Robbie-Microsoft commented May 8, 2024

Sure, a specific line would help.

I think the regression was introduced in this PR: Client Assertion implementation now accepts an async callback as well as a string argument

@ericcan
Copy link
Author

ericcan commented May 8, 2024

17286 is:
responseHandler.validateTokenResponse(serverTokenResponse, refreshAccessToken);

17572 is:
return await clientCredentialClient.acquireToken(validRequest);

ericcan referenced this issue May 9, 2024
… as a string argument (#7014)

Client assertion can currently be provided by a developer as a string.
This PR allows a developer to provide an async callback (which will
resolve to a string) as the client assertion.

The client assertion initialization has been removed from
ConfidentialClient's constructor and is now initialized inside of
buildOauthClientConfiguration, called during every acquireToken call.

Note: Applied Boy-Scout-Rule on UsernamePasswordClient.spec.ts. Only the
last test in the file is relevant for this review. Now this test file is
up-to-date and mirrors ClientCredentialsClient.spec.ts and
OnBehalfOfClient.spec.ts.
@ericcan
Copy link
Author

ericcan commented May 9, 2024

@Robbie-Microsoft

See this comment for a possible place that might be causing the certificate to get lost.

@wenytang-ms
Copy link

wenytang-ms commented May 9, 2024

@Robbie-Microsoft we use msal-node to get graph token

const config = {
  auth: {
    clientId: clientId,
    authority: `https://login.microsoftonline.com/${tenantId}`,
  },
};

const usernamePasswordRequest = {
  scopes: ["https://graph.microsoft.com/.default"],
  username: username,
  password: encodeURIComponent(password),
};

const pca = new msal.PublicClientApplication(config);
const credential = await pca.acquireTokenByUsernamePassword(
  usernamePasswordRequest
);
const accessToken = credential?.accessToken;
await axios.request({method: get, url: 'https://graph.microsoft.com/v1.0/users',headers: {
        authorization: `Bearer ${accessToken }`,
        "content-type": "application/json",
},})

and get such error, i'm not sure what should i update the code to query graph and get 200 response.

@bgavrilMS
Copy link
Member

@wenytang-ms - this is probably unrelated. A public client application does not need a secret or a certificate. UsernamePassword exists for both public client and for confidential client. It is disabled by default for public client. Go to the portal and enable it.

But! Please realize that UsernamePassword flow is extremely insecure and should be avoided. We only recommend using it in tests, to test web apis. Public Client (CLI, desktop apps) should use interactive authentication.

@wenytang-ms
Copy link

@bgavrilMS indeed!
In our scenario, we will run a lot of E2E cases which may create azure resource, so we finally use this method to clean up these resources like AAD App.
But we still meet such error!
this is our clean up jobs.
https://github.com/OfficeDev/TeamsFx/actions/runs/9004965614/job/24739186953
and the relative code is here.
https://github.com/OfficeDev/TeamsFx/blob/9a420617c310357cfe5a1d76450e9c3ba822ad21/packages/tests/src/utils/cleanHelper.ts#L63
Do I need to create a new issue?

@bgavrilMS
Copy link
Member

@wenytang-ms - I think this is just a setting in the app registration. Please send me an email to discuss further.

@Robbie-Microsoft
Copy link
Collaborator

@Robbie-Microsoft

See this comment for a possible place that might be causing the certificate to get lost.

Excellent, thanks for this @ericcan . I will be working on this today.

@StanislavHT
Copy link

StanislavHT commented May 9, 2024

It works with @azure/msal-node 2.6.4, but doesn't work with 2.8.0, I haven't tested with other versions.
If I strictly set @azure/msal-node version to 2.6.4 in package.json, it works.

At first time, I thought it's Azure Entra ID Application configuration problem.
But for the same configuration, 2.6.4 works.

As my checking the error happens exactly from 2.6.5
It seems the error is related to underlying library @azure/msal-common from 14.8.0

@ericcan
Copy link
Author

ericcan commented May 9, 2024

@Robbie-Microsoft

See this comment for a possible place that might be causing the certificate to get lost.

Excellent, thanks for this @ericcan . I will be working on this today.

Terrific. I did do a local fix that works and would be happy to share it, but wasn't confident that it was consistent with all the cases. Let me know if there is anything that would be helpful.

@Robbie-Microsoft
Copy link
Collaborator

Robbie-Microsoft commented May 9, 2024

@Robbie-Microsoft

See this comment for a possible place that might be causing the certificate to get lost.

Excellent, thanks for this @ericcan . I will be working on this today.

Terrific. I did do a local fix that works and would be happy to share it, but wasn't confident that it was consistent with all the cases. Let me know if there is anything that would be helpful.

Sure, please email the fix to Bogdan and he will forward it to me. In the meantime, I'm able to reproduce this now.

@Robbie-Microsoft
Copy link
Collaborator

Robbie-Microsoft commented May 9, 2024

I was able to reproduce the issue via tinkering with our unit tests and developed a fix via the linked PR. Unit tests are needed before review + merging into our dev branch. I'll be publishing a new version of msal-node with this fix either today or tomorrow.

Robbie-Microsoft added a commit that referenced this issue May 13, 2024
…ient assertion (#7088)

Fixes [acquireTokenByClientCredential broken for clientCertificate
#7082](#7082)

Applied boy-scout-rule to `ConfidentialClientApplication.spec.ts`
(contains unit tests). I've been waiting for a good opportunity to do
this. The ConfidentialClientApplication tests are now in line with the
other test files: All Managed Identity sources, ClientCredentialClient,
OnBehalfOfClient and UsernamePasswordClient.

Co-authored-by: @ericcan
@Robbie-Microsoft
Copy link
Collaborator

Fixed in v2.8.1, which is now available to install.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A problem that needs to be fixed for the feature to function as intended. confidential-client Issues regarding ConfidentialClientApplications msal-node Related to msal-node package p1 P1 and P2 are priorities of the bug. P1 bugs should get Fixed/Closed within 4 weeks.
Projects
None yet
8 participants