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

EC key invalid decoding #149

Closed
pstojek opened this issue Jul 26, 2022 · 5 comments · Fixed by #153
Closed

EC key invalid decoding #149

pstojek opened this issue Jul 26, 2022 · 5 comments · Fixed by #153

Comments

@pstojek
Copy link

pstojek commented Jul 26, 2022

Signum supposed to be used after base64 decoding:
https://github.com/auth0/jwks-rsa-java/blob/master/src/main/java/com/auth0/jwk/Jwk.java#L196-L197

Sample:
ECPoint ecPoint = new ECPoint(new BigInteger(1, Base64.getUrlDecoder().decode(stringValue("x"))),
new BigInteger(1, Base64.getUrlDecoder().decode(stringValue("y"))));

Similar approach is already used in lines 183-184 for RSA Algorithm.

Issue causes invalid pubKey decoding resulting in signature validation failure.

@poovamraj
Copy link
Contributor

Hi @pstojek, can you provide a sample so that we can reproduce this?

@pstojek
Copy link
Author

pstojek commented Aug 1, 2022

I am attaching sample with wiremock test simulating jwks server written in Kotlin - Failure rate is 3/4 (when signum of x/y matters). Solution provided in first comment fixes the issue. If we use that key later e.g. jwt token validation then we will receive invalid signature, because of improper EC pub key decoding.

import com.auth0.jwk.JwkProviderBuilder
import com.github.tomakehurst.wiremock.client.WireMock.get
import com.github.tomakehurst.wiremock.client.WireMock.ok
import com.github.tomakehurst.wiremock.client.WireMock.stubFor
import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo
import com.github.tomakehurst.wiremock.junit5.WireMockTest
import com.nimbusds.jose.jwk.Curve
import com.nimbusds.jose.jwk.ECKey
import com.nimbusds.jose.jwk.KeyUse
import com.nimbusds.jose.jwk.gen.ECKeyGenerator
import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Test
import java.net.URL
import java.security.interfaces.ECPublicKey
import java.time.Duration
import java.util.UUID


@WireMockTest
internal class JwksTest {
    @Test
    fun `should pull same public key as on jwks server`(wmRuntimeInfo: WireMockRuntimeInfo) {
        // PART 1: Stab JWKS server response with com.nimbusds.jose.jwk library:
        val keyId = UUID.randomUUID().toString()

        val serverJwk: ECKey = ECKeyGenerator(Curve.P_384)
            .keyUse(KeyUse.SIGNATURE)
            .keyID(keyId)
            .generate()

        stubFor(
            get("/.well-known/jwks.json").willReturn(
                ok("{\"keys\":[${serverJwk.toPublicJWK().toJSONString()}]}")
            )
        )

        val serverPublicKey = serverJwk.toPublicJWK().toECPublicKey()

        // PART 2: client side using com.auth0.jwk.JwkProviderBuilder
        val jwkProvider = JwkProviderBuilder(
            URL(wmRuntimeInfo.httpBaseUrl + "/.well-known/jwks.json")
        )
            .cached(5, Duration.ofHours(6))
            .timeouts(1000, 1000)
            .rateLimited(false)
            .build()

        val pulledECPublicKey = jwkProvider.get(keyId).publicKey as ECPublicKey

        // Success rate 1/2 for following condition:
        assertEquals(serverPublicKey.w.affineX, pulledECPublicKey.w.affineX)
        // Success rate 1/2 for following condition:
        assertEquals(serverPublicKey.w.affineY, pulledECPublicKey.w.affineY)
    }
}

@salami
Copy link

salami commented Sep 1, 2022

I'm not certain if I had exactly the same issue, but my EC JWK -> public token was failing with something like a "coordinates out of range" error.

Anyways I switched to https://github.com/fusionauth/fusionauth-jwt to grab the public key. Their logic is really quite similar to the logic in this library so not exactly sure what the difference is.

@jimmyjames
Copy link
Contributor

jimmyjames commented Sep 7, 2022

Thanks for raising this and providing a test case! @pstojek and @salami if you're able to try out #153 and verify it resolves the issue that would be great! 👍

@pstojek
Copy link
Author

pstojek commented Sep 8, 2022

Yes, it fixes the issue. Thanks

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

Successfully merging a pull request may close this issue.

4 participants