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

[Bug] Null ref thrown when cca built with no AuthenticationType #1795

Closed
jennyf19 opened this issue Apr 30, 2020 · 0 comments
Closed

[Bug] Null ref thrown when cca built with no AuthenticationType #1795

jennyf19 opened this issue Apr 30, 2020 · 0 comments
Assignees
Labels
Milestone

Comments

@jennyf19
Copy link
Collaborator

Related to this issue in ms identity web

Repro:

  • Create a confidential client using application options (or without, based on my testing), and do not include a client secret, certificate or assertion.

  • In ClientCredentialWrapper the default enum value is ClientCertificate, so when no value is provided in creating the cca, the default AuthenticationType is ClientCertificate.

  • Later, in ClientCredentialHelper, when making the body parameters, we go the path of ClientCertificate and MSAL throws a null ref on line 80 when trying to sign something it doesn't have:
    clientCredential.CachedAssertion = jwtToken.Sign(clientCredential, sendX5C);

MS.Identity.Web had customers reporting a null ref when forgetting to set the client secret in the web app, MS.Identity.Web relies on MSAL for getting the cca tokens. We can successful create a cca without a value in the ClientSecret...later we try to do to .AcquireTokenByAuthorizationCode using the cca we made earlier and we get a null ref, which is actually coming from MSAL .NET (I did some hacking in your unit tests to figure this out).

I have a fix for us in Identity Web to make sure we send options without null or empty strings. Here's a fix to have a default enum value as none, to catch the case where the developer has created a cca, but doesn't passed in any AuthenticationTypes.

Another fix would be to ensure that one of the four AuthenticationType options are used for the cca. We check that not more than one are used, but not that at least one is used. I have not opened an issue for that, will let the team decide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants