Support EC Signed OIDC Jwt tokens #7785
Conversation
Add support for EC Signing Algorithms
EC Signatures to be transcoded to DER format
Adjust Signature Exception catching
Add: import java.security.SignatureException;
Adjust catching exceptions
There was a problem hiding this comment.
Thanks for this contribution.
Overall, the patch seems quite reasonable.
There are also some places where (in my view) the code's readability could be improved, but I believe the code is correct. I've marked these comments "[non-blocking]"
Unfortunately, the method transcodeJWTECDSASignatureToDER seems to miss some of the intricacies of DER encoding, particularly in how LENGTH and INTEGER values are encoded. I've tried to indicate these potential issues. Given the complexity of DER-encoding, providing some unit-tests that target this specific method would seem prudent.
it would be helpful if there were also unit-tests that target the overall processing of EC-signed JWTs (see existing unit-tests for possible inspiration). Including some real-life example tokens as unit-tests could be useful, if possible. The existing RSA tests build the JWT dynamically, but doing this for EC-signed JWTs might be awkward.
| // INTEGER R | ||
| der[offset++] = 0x02; | ||
| der[offset++] = (byte) rLen; | ||
| System.arraycopy(jwsSignature, rawLen - rLen, der, offset, rLen); |
There was a problem hiding this comment.
[non-blocking]
Suggest:
System.arraycopy(jwsSignature, rStart, der, offset, rLen).
| // INTEGER S | ||
| der[offset++] = 0x02; | ||
| der[offset++] = (byte) sLen; | ||
| System.arraycopy(jwsSignature, jwsSignature.length - sLen, der, offset, sLen); |
There was a problem hiding this comment.
[non-blocking]
Suggest:
System.arraycopy(jwsSignature, sStart, der, offset, sLen);
| while (sStart < jwsSignature.length && jwsSignature[sStart] == 0) { | ||
| sStart++; | ||
| } | ||
| int sLen = rawLen - (sStart - rawLen); |
There was a problem hiding this comment.
[non-blocking]
This is equivalent to:
int sLen = jwsSignature.length - sStart;
Personally I find the above form easier to understand, but tastes vary, so I offer this only as an alternative.
|
|
||
| // Find the start of R (skip leading zeros) | ||
| int rStart = 0; | ||
| while (rStart < rawLen && jwsSignature[rStart] == 0) { |
There was a problem hiding this comment.
This produces the wrong sequence if the initial non-padding octet is >= 0x80. Under DER encoding, an integer starting with an octet >= 0x80 results in that value accepted as a negative number (2s-complement). Under these circumstances, a proceeding 0x00 octet is needed to ensure the DER encoded integer is understood as a positive number. As a special case, if the R octet sequence has no 0x00 padding and the first octet is >= 0x80 then an additional 0x00 octet (not present in jwsSignature) must be included in the DER encoded output, before the first jwsSignature octet.
The same problem exists for the code calculating the start of S: the DER encoded integer is treated as negative if the initial octet is >= 0x80.
| } | ||
| int sLen = rawLen - (sStart - rawLen); | ||
|
|
||
| int totalLen = 2 + 2 + rLen + 2 + sLen; // SEQUENCE + INTEGER(R) + INTEGER(S) |
There was a problem hiding this comment.
This underestimates the amount of memory needed if (2 + rLen + 2 + sLen) >= 0x80. This is because DER encoding a length n where 0x80 <= n <= 0xff requires two bytes: 0x81, n.
| byte[] der = new byte[totalLen]; | ||
|
|
||
| der[offset++] = 0x30; // SEQUENCE | ||
| der[offset++] = (byte) (totalLen - 2); |
There was a problem hiding this comment.
Encoding the length as a single octet assumes that the number of octets needed to DER-encoded the two integers (i.e., totalLen - 2) is < 0x80. If the number of octets needed n is 0x80 <= n <= 0xff then a two octet sequence is needed: 0x81, n.
| der[offset++] = (byte) (totalLen - 2); | ||
|
|
||
| // INTEGER R | ||
| der[offset++] = 0x02; |
There was a problem hiding this comment.
[non-blocking]
It would be helpful if the meaning of the 0x02 value was explained, similar to the 0x30 above.
| @@ -113,12 +114,57 @@ public String getKeyIdentifier() { | |||
| return kid; | |||
| } | |||
|
|
|||
| private static byte[] transcodeJWTECDSASignatureToDER(byte[] jwsSignature) throws SignatureException { | |||
There was a problem hiding this comment.
Given the complexity of this method, it would be reasonable to provide some unit-tests (tests targeting this specific method) to verify correct behaviour for example byte sequences for each of the supported algorithms.
| @@ -113,12 +114,57 @@ public String getKeyIdentifier() { | |||
| return kid; | |||
| } | |||
|
|
|||
| private static byte[] transcodeJWTECDSASignatureToDER(byte[] jwsSignature) throws SignatureException { | |||
| int rawLen = jwsSignature.length / 2; | |||
There was a problem hiding this comment.
[non-blocking]
It would be better if the code rejected jwsSignature if the length was an odd number.
Better still would be if the code rejected jwsSignature if the length was not the expected value for the chosen algorithm.
|
It may be off-topic: Make it sense to look at https://connect2id.com/products/nimbus-jose-jwt and avoid custom JWT code? |
dCache gplazma2 OIDC was only supporting RSA signed JWT tokens. I had a OIDC OP that issues EC signed tokens.
I added the support for EC Signed Tokens and now my tokens are accepted.