Skip to content

Support EC Signed OIDC Jwt tokens #7785

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Support EC Signed OIDC Jwt tokens #7785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0992f8b
add support for EC Public Keys
Apr 29, 2025
2d22189
Update JsonWebToken.java
HarryKodden Apr 29, 2025
02e99fc
Update JsonWebToken.java
HarryKodden Apr 29, 2025
6e5c91b
Update JsonWebToken.java
HarryKodden Apr 29, 2025
5155d12
Update JsonWebToken.java
HarryKodden Apr 29, 2025
4b771ef
Update JsonWebToken.java
HarryKodden Apr 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/Issuer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,14 @@
import java.security.PublicKey;
import java.security.spec.KeySpec;
import java.security.spec.RSAPublicKeySpec;
import java.time.Duration;
import java.security.interfaces.ECPublicKey;
import java.security.spec.ECPoint;
import java.security.spec.ECParameterSpec;
import java.security.spec.ECPublicKeySpec;
import java.security.spec.ECGenParameterSpec;
import java.security.AlgorithmParameters;
import java.util.Base64;
import java.time.Duration;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -214,6 +220,8 @@ private PublicKey buildPublicKey(JsonNode details) throws BadKeyDescriptionExcep
switch (kty) {
case "RSA":
return buildRSAPublicKey(details);
case "EC":
return buildECPublicKey(details);
default:
throw new BadKeyDescriptionException("Unknown key type " + kty);
}
Expand Down Expand Up @@ -249,6 +257,41 @@ private PublicKey buildRSAPublicKey(JsonNode details) throws BadKeyDescriptionEx
}
}

private ECParameterSpec getECParameterSpec(String crv) throws GeneralSecurityException {
String name;

switch (crv) {
case "P-256":
name = "secp256r1";
break;
case "P-384":
name = "secp384r1";
break;
case "P-521":
name = "secp521r1";
break;
default:
throw new GeneralSecurityException("Unsupported curve: " + crv);
}
AlgorithmParameters parameters = AlgorithmParameters.getInstance("EC");
parameters.init(new ECGenParameterSpec(name));
return parameters.getParameterSpec(ECParameterSpec.class);
}

private PublicKey buildECPublicKey(JsonNode details) throws BadKeyDescriptionException {
try {
String crv = getString(details, "crv");
byte[] x = Base64.getUrlDecoder().decode(getString(details, "x"));
byte[] y = Base64.getUrlDecoder().decode(getString(details, "y"));
ECParameterSpec params = getECParameterSpec(crv);
ECPoint point = new ECPoint(new BigInteger(1, x), new BigInteger(1, y));
ECPublicKeySpec pubSpec = new ECPublicKeySpec(point, params);
return KeyFactory.getInstance("EC").generatePublic(pubSpec);
} catch (GeneralSecurityException e) {
throw new BadKeyDescriptionException("Unable to build EC public key: " + e.toString());
}
}

public void checkIssued(JsonWebToken token) throws AuthenticationException {
Map<String, Result<PublicKey,String>> keyMap = keys.get()
.orElseThrow(msg -> new AuthenticationException(
Expand Down
54 changes: 53 additions & 1 deletion modules/gplazma2/src/main/java/org/dcache/gplazma/util/JsonWebToken.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.nio.charset.StandardCharsets;
import java.security.GeneralSecurityException;
import java.security.NoSuchAlgorithmException;
import java.security.SignatureException;
import java.security.PublicKey;
import java.security.Signature;
import java.time.Instant;
Expand Down Expand Up @@ -113,12 +114,57 @@ public String getKeyIdentifier() {
return kid;
}

private static byte[] transcodeJWTECDSASignatureToDER(byte[] jwsSignature) throws SignatureException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

int rawLen = jwsSignature.length / 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.


// Find the start of R (skip leading zeros)
int rStart = 0;
while (rStart < rawLen && jwsSignature[rStart] == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rStart++;
}
int rLen = rawLen - rStart;

// Find the start of S (skip leading zeros)
int sStart = rawLen;
while (sStart < jwsSignature.length && jwsSignature[sStart] == 0) {
sStart++;
}
int sLen = rawLen - (sStart - rawLen);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.


int totalLen = 2 + 2 + rLen + 2 + sLen; // SEQUENCE + INTEGER(R) + INTEGER(S)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

int offset = 0;
byte[] der = new byte[totalLen];

der[offset++] = 0x30; // SEQUENCE
der[offset++] = (byte) (totalLen - 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// INTEGER R
der[offset++] = 0x02;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[non-blocking]

It would be helpful if the meaning of the 0x02 value was explained, similar to the 0x30 above.

der[offset++] = (byte) rLen;
System.arraycopy(jwsSignature, rawLen - rLen, der, offset, rLen);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[non-blocking]

Suggest:

System.arraycopy(jwsSignature, rStart, der, offset, rLen).

offset += rLen;

// INTEGER S
der[offset++] = 0x02;
der[offset++] = (byte) sLen;
System.arraycopy(jwsSignature, jwsSignature.length - sLen, der, offset, sLen);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[non-blocking]

Suggest:

System.arraycopy(jwsSignature, sStart, der, offset, sLen);


return der;
}

public boolean isSignedBy(PublicKey key) {
try {
Signature signature = getSignature();
signature.initVerify(key);
signature.update(unsignedToken);
return signature.verify(this.signature);
byte[] sig = this.signature;
if (alg.startsWith("ES")) {
sig = transcodeJWTECDSASignatureToDER(sig);
}
return signature.verify(sig);
} catch (SignatureException e) {
LOGGER.warn("Problem verifying signature: {}", e.toString());
return false;
} catch (GeneralSecurityException e) {
LOGGER.warn("Problem verifying signature: {}", e.toString());
return false;
Expand All @@ -129,6 +175,12 @@ private Signature getSignature() throws GeneralSecurityException {
switch (alg) {
case "RS256":
return Signature.getInstance("SHA256withRSA");
case "ES256":
return Signature.getInstance("SHA256withECDSA");
case "ES384":
return Signature.getInstance("SHA384withECDSA");
case "ES512":
return Signature.getInstance("SHA512withECDSA");
default:
throw new NoSuchAlgorithmException("Unknown JWT alg " + alg);
}
Expand Down