Remove and replace various com.nimbusds utils #941
Conversation
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenRequestExecutor.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ClientAssertion.java
Outdated
Show resolved
Hide resolved
neha-bhargava
left a comment
neha-bhargava
left a comment
There was a problem hiding this comment.
Approve with comments.
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ClientAssertion.java
Outdated
Show resolved
Hide resolved
| String idTokenJson; | ||
|
|
||
| try { | ||
| idTokenJson = new String(Base64.getDecoder().decode(idToken.split("\\.")[1]), StandardCharsets.UTF_8); |
There was a problem hiding this comment.
jwt's are base64url encoded, not base64. THis already results in a P1.
Can we add some tests around it?
There was a problem hiding this comment.
This was mistakenly copied from a branch that had not yet been updated with the fix for the previous issue.
In the latest commit the logic both here and in TokenRequestExecutor has been moved to a helper method in JsonHelper, and new tests were added to confirm the behavior.
| String idTokenJson; | ||
|
|
||
| try { | ||
| idTokenJson = new String(Base64.getDecoder().decode(idToken.split("\\.")[1]), StandardCharsets.UTF_8); |
There was a problem hiding this comment.
This is a very strange operation (extracting only the body). Why are you doing this?
There was a problem hiding this comment.
This was copied from similar code in TokenRequestExecutor, which had been parsing the body since that file was first added: https://github.com/AzureAD/microsoft-authentication-library-for-java/blame/bbdc93d59d54916cdbac4016f1b174822e9bb51c/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenRequestExecutor.java#L123
An IdToken is created from the JSON of claims in the body: https://github.com/AzureAD/microsoft-authentication-library-for-java/blob/dev/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IdToken.java
I'm not sure exactly why just the body is decoded, however from some testing now it seems like Java's built-in base64 URL decoded cannot parse the signature so that may be why.
|
|
||
| // Build header | ||
| Map<String, Object> header = new HashMap<>(); | ||
| header.put("alg", "RS256"); |
There was a problem hiding this comment.
The alg is PS256 not RS256 to suggest PSS padding instead of PKCS1 padding. Please have a look at the .NET code for this.
There was a problem hiding this comment.
The current version of the library uses RS256 here, and has apparently done so since the 'sendX5C' API was first added in 2020 by PR #285:
So this might need a more thorough investigation. Pretty much every integration test sends that "alg=RS256" header when retrieving the token to access ID labs, so the endpoint is either accepting or ignoring it.
|
|
||
| // Decode and verify headers | ||
| String headerJson = new String(Base64.getUrlDecoder().decode(jwtParts[0])); | ||
| assertTrue(headerJson.contains("\"alg\":\"RS256\""), "Header should specify RS256 algorithm"); |
There was a problem hiding this comment.
It should be PS256 . RS256 means PKCS1 padding, which is not approved.
There was a problem hiding this comment.
These comments have more context: #941 (comment)
…crosoft-authentication-library-for-java into avdunn/nimbus-utils # Conflicts: # msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/HelperAndUtilityTests.java
Avery-Dunn
changed the base branch from
avdunn/nimbus-grants
to
avdunn/nimbus-removal
This PR removes various com.nimbusds helpers and utils, as per #909. In short, the following changes were made:
It also removes unused code in
IdToken, and the unused classSAML11BearerGrant(no longer needed due to the changes in #926)