jwks endpoint for X/Y coordinates in EC keypair can return less bytes than expected (#14952)

Closes #14933

core/src/main/java/org/keycloak/jose/jwk/JWKBuilder.java

@@ -124,8 +124,8 @@ public class JWKBuilder {
         k.setAlgorithm(algorithm);
         k.setPublicKeyUse(DEFAULT_PUBLIC_KEY_USE);
         k.setCrv("P-" + fieldSize);
-        k.setX(Base64Url.encode(toIntegerBytes(ecKey.getW().getAffineX())));
+        k.setX(Base64Url.encode(toIntegerBytes(ecKey.getW().getAffineX(), fieldSize)));
-        k.setY(Base64Url.encode(toIntegerBytes(ecKey.getW().getAffineY())));
+        k.setY(Base64Url.encode(toIntegerBytes(ecKey.getW().getAffineY(), fieldSize)));
         
         return k;
     }

core/src/main/java/org/keycloak/jose/jwk/JWKUtil.java

@@ -18,35 +18,46 @@
 package org.keycloak.jose.jwk;
 
 import java.math.BigInteger;
+import java.util.Arrays;
 
 public class JWKUtil {
 
     /**
-     * Convert BigInteger to 64-byte integer array
+     * Coverts {@code BigInteger} to 64-byte array removing the sign byte if
+     * necessary.
      *
-     * Copied from org.apache.commons.codec.binary.Base64
+     * @param bigInt {@code BigInteger} to be converted
+     * @return Byte array representation of the BigInteger parameter
      */
     public static byte[] toIntegerBytes(final BigInteger bigInt) {
-        int bitlen = bigInt.bitLength();
+        return toIntegerBytes(bigInt, bigInt.bitLength());
-        // round bitlen
-        bitlen = ((bitlen + 7) >> 3) << 3;
-        final byte[] bigBytes = bigInt.toByteArray();
-
-        if (((bigInt.bitLength() % 8) != 0) && (((bigInt.bitLength() / 8) + 1) == (bitlen / 8))) {
-            return bigBytes;
     }
-        // set up params for copying everything but sign bit
-        int startSrc = 0;
-        int len = bigBytes.length;
 
-        // if bigInt is exactly byte-aligned, just skip signbit in copy
+    /**
-        if ((bigInt.bitLength() % 8) == 0) {
+     * Coverts {@code BigInteger} to 64-byte array but maintaining the length
-            startSrc = 1;
+     * to bitlen as specified in rfc7518 for certain fields (X and Y parameter
-            len--;
+     * for EC keys).
-        }
+     *
-        final int startDst = bitlen / 8 - len; // to pad w/ nulls as per spec
+     * @param bigInt {@code BigInteger} to be converted
-        final byte[] resizedBytes = new byte[bitlen / 8];
+     * @param bitlen The bit length size of the integer (for example 521 for EC P-521)
-        System.arraycopy(bigBytes, startSrc, resizedBytes, startDst, len);
+     * @return Byte array representation of the BigInteger parameter with length (bitlen + 7) / 8
+     * @throws IllegalStateException if the big integer is longer than bitlen
+     */
+    public static byte[] toIntegerBytes(final BigInteger bigInt, int bitlen) {
+        assert bigInt.bitLength() <= bitlen : "Incorrect big integer with bit length " + bigInt.bitLength() + " for " + bitlen;
+        final int bytelen = (bitlen + 7) / 8;
+        final byte[] array = bigInt.toByteArray();
+        if (array.length == bytelen) {
+            // expected number of bytes, return them
+            return array;
+        } else if (bytelen < array.length) {
+            // if array is greater is because the sign bit (it can be only 1 byte more), remove it
+            return Arrays.copyOfRange(array, array.length - bytelen, array.length);
+        } else {
+            // if array is smaller fill it with zeros
+            final byte[] resizedBytes = new byte[bytelen];
+            System.arraycopy(array, 0, resizedBytes, bytelen - array.length, array.length);
             return resizedBytes;
         }
     }
+}

core/src/test/java/org/keycloak/jose/jwk/JWKTest.java

@@ -135,15 +135,14 @@ public abstract class JWKTest {
         verify(data, sign, JavaAlgorithm.RS256, publicKeyFromJwk);
     }
 
-    @Test
+    private void testPublicEs256(String algorithm) throws Exception {
-    public void publicEs256() throws Exception {
         KeyPairGenerator keyGen = CryptoIntegration.getProvider().getKeyPairGen(KeyType.EC);
         SecureRandom randomGen = new SecureRandom();
-        ECGenParameterSpec ecSpec = new ECGenParameterSpec("secp256r1");
+        ECGenParameterSpec ecSpec = new ECGenParameterSpec(algorithm);
         keyGen.initialize(ecSpec, randomGen);
         KeyPair keyPair = keyGen.generateKeyPair();
 
-        PublicKey publicKey = keyPair.getPublic();
+        ECPublicKey publicKey = (ECPublicKey) keyPair.getPublic();
 
         JWK jwk = JWKBuilder.create().kid(KeyUtils.createKeyId(keyPair.getPublic())).algorithm("ES256").ec(publicKey);
 
@@ -162,28 +161,36 @@ public abstract class JWKTest {
         byte[] xBytes = Base64Url.decode(ecJwk.getX());
         byte[] yBytes = Base64Url.decode(ecJwk.getY());
 
-        assertTrue(publicKey instanceof ECPublicKey);
+        final int expectedSize = (publicKey.getParams().getCurve().getField().getFieldSize() + 7) / 8;
-        ECPoint ecPoint = ((ECPublicKey) publicKey).getW();
+        assertEquals(expectedSize, xBytes.length);
-        assertNotNull(ecPoint);
+        assertEquals(expectedSize, yBytes.length);
-
-        int lengthAffineX = JWKUtil.toIntegerBytes(ecPoint.getAffineX()).length;
-        int lengthAffineY = JWKUtil.toIntegerBytes(ecPoint.getAffineY()).length;
-
-        assertEquals(lengthAffineX, xBytes.length);
-        assertEquals(lengthAffineY, yBytes.length);
 
         String jwkJson = JsonSerialization.writeValueAsString(jwk);
 
         JWKParser parser = JWKParser.create().parse(jwkJson);
-        PublicKey publicKeyFromJwk = parser.toPublicKey();
+        ECPublicKey publicKeyFromJwk = (ECPublicKey) parser.toPublicKey();
-
+        assertEquals(publicKey.getW(), publicKeyFromJwk.getW());
-        assertArrayEquals(publicKey.getEncoded(), publicKeyFromJwk.getEncoded());
 
         byte[] data = "Some test string".getBytes(StandardCharsets.UTF_8);
         byte[] sign = sign(data, JavaAlgorithm.ES256, keyPair.getPrivate());
         verify(data, sign, JavaAlgorithm.ES256, publicKeyFromJwk);
     }
 
+    @Test
+    public void publicEs256P256() throws Exception {
+        testPublicEs256("secp256r1");
+    }
+
+    @Test
+    public void publicEs256P521() throws Exception {
+        testPublicEs256("secp521r1");
+    }
+
+    @Test
+    public void publicEs256P384() throws Exception {
+        testPublicEs256("secp384r1");
+    }
+
     @Test
     public void parse() {
         String jwkJson = "{" +

core/src/test/java/org/keycloak/jose/jwk/JWKUtilTest.java

@@ -0,0 +1,103 @@
+/*
+ * Copyright 2022 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.keycloak.jose.jwk;
+
+import java.math.BigInteger;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * <p>Test class for <em>JWKUtil.toIntegerBytes</em> methods.</p>
+ *
+ * @author rmartinc
+ */
+public class JWKUtilTest {
+
+    @Test
+    public void testBigInteger256bit33bytes() {
+        // big integer that is 256b/32B (P-256) but positive sign adds one more byte
+        BigInteger bi = new BigInteger("106978455244904118504029146852168092303170743300495577837424194202315290288011");
+        Assert.assertEquals(256, bi.bitLength());
+        Assert.assertEquals(33, bi.toByteArray().length);
+        byte[] bytes = JWKUtil.toIntegerBytes(bi, 256);
+        Assert.assertEquals(32, bytes.length);
+        Assert.assertEquals(bi, new BigInteger(1, bytes));
+    }
+
+    @Test
+    public void testBigInteger521bit66bytes() {
+        // big integer that is 521b/66B (P-521)
+        BigInteger bi = new BigInteger("6734373674814691396115132088653791161514881890352734019594374673014557152383502505390504647094584246525242385854438954847939940255492102589858760446395824148");
+        Assert.assertEquals(521, bi.bitLength());
+        Assert.assertEquals(66, bi.toByteArray().length);
+        byte[] bytes = JWKUtil.toIntegerBytes(bi, 521);
+        Assert.assertEquals(66, bytes.length);
+        Assert.assertEquals(bi, new BigInteger(1, bytes));
+    }
+
+    @Test
+    public void testBigInteger519bit65bytes() {
+        // big integer is 519b/65B (P-521)
+        BigInteger bi = new BigInteger("1056406612537758216307284361941630998827278875643943164504783316640832530092186610655845467862847840003942818620330993843247554843391332954698064457598103921");
+        Assert.assertEquals(519, bi.bitLength());
+        Assert.assertEquals(65, bi.toByteArray().length);
+        byte[] bytes = JWKUtil.toIntegerBytes(bi, 521);
+        Assert.assertEquals(66, bytes.length);
+        Assert.assertEquals(bi, new BigInteger(1, bytes));
+        bytes = JWKUtil.toIntegerBytes(bi);
+        Assert.assertEquals(65, bytes.length);
+        Assert.assertEquals(bi, new BigInteger(1, bytes));
+    }
+
+    @Test
+    public void testBigInteger509bit65bytes() {
+        // big integer is 509b/64B (P-521)
+        BigInteger bi = new BigInteger("1020105336060806799317581876370378670178920448263046037385822665297838480884942245045412789346716977404456327079571798657084244307627713218035021026706753");
+        Assert.assertEquals(509, bi.bitLength());
+        Assert.assertEquals(64, bi.toByteArray().length);
+        byte[] bytes = JWKUtil.toIntegerBytes(bi, 521);
+        Assert.assertEquals(66, bytes.length);
+        Assert.assertEquals(bi, new BigInteger(1, bytes));
+        bytes = JWKUtil.toIntegerBytes(bi);
+        Assert.assertEquals(64, bytes.length);
+        Assert.assertEquals(bi, new BigInteger(1, bytes));
+    }
+
+    @Test
+    public void testBigInteger380bit48bytes() {
+        // big integer is 380b/48B (P-384)
+        BigInteger bi = new BigInteger("1318324198847573133767761135109898830134893480775680898178696604234765693579204018161102886445531980641666395659568");
+        Assert.assertEquals(380, bi.bitLength());
+        Assert.assertEquals(48, bi.toByteArray().length);
+        byte[] bytes = JWKUtil.toIntegerBytes(bi, 384);
+        Assert.assertEquals(48, bytes.length);
+        Assert.assertEquals(bi, new BigInteger(1, bytes));
+        bytes = JWKUtil.toIntegerBytes(bi);
+        Assert.assertEquals(48, bytes.length);
+        Assert.assertEquals(bi, new BigInteger(1, bytes));
+    }
+
+    @Test
+    public void testBigInteger380bit48bytesErrorFor256() {
+        // big integer is 380b/48B (P-384) not valid for 256b (P-256)
+        BigInteger bi = new BigInteger("1318324198847573133767761135109898830134893480775680898178696604234765693579204018161102886445531980641666395659568");
+        Assert.assertEquals(380, bi.bitLength());
+        Assert.assertEquals(48, bi.toByteArray().length);
+        AssertionError e = Assert.assertThrows(AssertionError.class, () -> JWKUtil.toIntegerBytes(bi, 256));
+        Assert.assertEquals("Incorrect big integer with bit length 380 for 256", e.getMessage());
+    }
+}