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

P-521 Coordinates (x,y) are expected to be the same length and equal which is not always true #709

Open
mehmetg opened this issue Nov 1, 2021 · 10 comments

Comments

@mehmetg
Copy link

mehmetg commented Nov 1, 2021

when decoding JWT tokens issued by keycloak I have come across x and y coordinate lengths 65 and 66 respectively and when this happens pyjwt throws this error.

raise InvalidKeyError("Coords should be 66 bytes for curve P-521")

Expected Result

It should allow coordinate lengths in [64, 66]
Ref: https://stackoverflow.com/questions/50002149/why-p-521-public-key-x-y-some-time-is-65-bytes-some-time-is-66-bytes

Actual Result

The library raised an invalid key error

Reproduction Steps

This example pointing to an internal keycloak deployment.

import jwt
from jwt import PyJWKClient
token = "eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6Ik5FRTFRVVJCT1RNNE16STVSa0ZETlRZeE9UVTFNRGcyT0Rnd1EwVXpNVGsxUWpZeVJrUkZRdyJ9.eyJpc3MiOiJodHRwczovL2Rldi04N2V2eDlydS5hdXRoMC5jb20vIiwic3ViIjoiYVc0Q2NhNzl4UmVMV1V6MGFFMkg2a0QwTzNjWEJWdENAY2xpZW50cyIsImF1ZCI6Imh0dHBzOi8vZXhwZW5zZXMtYXBpIiwiaWF0IjoxNTcyMDA2OTU0LCJleHAiOjE1NzIwMDY5NjQsImF6cCI6ImFXNENjYTc5eFJlTFdVejBhRTJINmtEME8zY1hCVnRDIiwiZ3R5IjoiY2xpZW50LWNyZWRlbnRpYWxzIn0.PUxE7xn52aTCohGiWoSdMBZGiYAHwE5FYie0Y1qUT68IHSTXwXVd6hn02HTah6epvHHVKA2FqcFZ4GGv5VTHEvYpeggiiZMgbxFrmTEY0csL6VNkX1eaJGcuehwQCRBKRLL3zKmA5IKGy5GeUnIbpPHLHDxr-GXvgFzsdsyWlVQvPX2xjeaQ217r2PtxDeqjlf66UYl6oY6AqNS8DH3iryCvIfCcybRZkc_hdy-6ZMoKT6Piijvk_aXdm7-QQqKJFHLuEqrVSOuBqqiNfVrG27QzAPuPOxvfXTVLXL2jek5meH6n-VWgrBdoMFH93QEszEDowDAEhQPHVs0xj7SIzA"
kid = "NEE1QURBOTM4MzI5RkFDNTYxOTU1MDg2ODgwQ0UzMTk1QjYyRkRFQw"
url = "https://dev-87evx9ru.auth0.com/.well-known/jwks.json"
jwks_client = PyJWKClient(url)
signing_key = jwks_client.get_signing_key_from_jwt(token)
data = jwt.decode(token, signing_key.key, algorithms=["RS256"], audience="https://expenses-api", options={"verify_exp": False},)
print(data)

System Information

$ python -m jwt.help
{
  "cryptography": {
    "version": "35.0.0"
  },
  "implementation": {
    "name": "CPython",
    "version": "3.9.7"
  },
  "platform": {
    "release": "20.6.0",
    "system": "Darwin"
  },
  "pyjwt": {
    "version": "2.3.0"
  }
}

This command is only available on PyJWT v1.6.3 and greater. Otherwise,
please provide some basic information about your system.

@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Issues without activity for more than 60 days label May 16, 2022
@DylanYoung
Copy link

This looks like a valid issue to me.

@DylanYoung
Copy link

Seems like the proper fix is in to_jwk, not the validation.

@mehmetg
Copy link
Author

mehmetg commented Jul 10, 2022

Can we reopen?

@jpadilla jpadilla reopened this Jul 10, 2022
@jpadilla jpadilla removed the stale Issues without activity for more than 60 days label Jul 10, 2022
@DylanYoung
Copy link

In case it helps, here's a rouch sketch of how I solved it for to_jwk. I'm not sure if there's anywhere else it needs to be fixed, but the JWK standard does specify that it should be 66 bytes, so if validation of another library's implementation is failing, I think that's an issue with those libraries:


ES_CURVES_MINIMUM_PARAMETER_LENGTH = {
    'P-521': 66,
}

def bytes_from_int(val: int, min_length=None) -> bytes:
    remaining = val
    byte_length = 0

    while remaining != 0:
        remaining >>= 8
        byte_length += 1

    if min_length and byte_length < min_length:
        byte_length = min_length

    return val.to_bytes(byte_length, "big", signed=False)


def to_base64url_uint(val: int, min_length=None) -> bytes:
    # See https://github.com/jpadilla/pyjwt/issues/709
    if val < 0:
        raise ValueError("Must be a positive integer")

    int_bytes = bytes_from_int(val, min_length=min_length)

    if len(int_bytes) == 0:
        int_bytes = b"\x00"

    return jwt.utils.base64url_encode(int_bytes)


class ECAlgorithm(with_metaclass(MonkeyPatchMeta, jwt.algorithms.ECAlgorithm)):
    @staticmethod
    def to_jwk(public_key):
        ...
        min_length = ES_CURVES_MINIMUM_PARAMETER_LENGTH.get(curve)

        return json.dumps({
            "crv": curve,
            "x": to_base64url_uint(numbers.x, min_length=min_length).decode("utf-8"),
            "y": to_base64url_uint(numbers.y, min_length=min_length).decode("utf-8"),
            "kty": "EC",
        })

@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Issues without activity for more than 60 days label Sep 14, 2022
@jpadilla jpadilla added bug help wanted keep and removed stale Issues without activity for more than 60 days labels Sep 21, 2022
@BeryJu
Copy link

BeryJu commented Jan 18, 2023

The same thing can happen for P-256 too, except with the length having to be 32

For testing I'm using this self-signed certificate

-----BEGIN CERTIFICATE-----
MIIB6jCCAZCgAwIBAgIRAOsdE3N7zETzs+7shTXGj5wwCgYIKoZIzj0EAwIwHjEc
MBoGA1UEAwwTYXV0aGVudGlrIDIwMjIuMTIuMjAeFw0yMzAxMTYyMjU2MjVaFw0y
NDAxMTIyMjU2MjVaMHgxTDBKBgNVBAMMQ0NsbDR2TzFJSGxvdFFhTGwwMHpES2tM
WENYdzRPUFF2eEtZN1NrczAuc2VsZi1zaWduZWQuZ29hdXRoZW50aWsuaW8xEjAQ
BgNVBAoMCWF1dGhlbnRpazEUMBIGA1UECwwLU2VsZi1zaWduZWQwWTATBgcqhkjO
PQIBBggqhkjOPQMBBwNCAAQAwOGam7AKOi5LKmb9lK1rAzA2JTppqrFiIaUdjqmH
ZICJP00Wt0dfqOtEjgMEv1Hhu1DmKZn2ehvpxwPSzBr5o1UwUzBRBgNVHREBAf8E
RzBFgkNCNkw4YlI0UldJRU42NUZLamdUTzV1YmRvNUZWdkpNS2lxdjFZeTRULnNl
bGYtc2lnbmVkLmdvYXV0aGVudGlrLmlvMAoGCCqGSM49BAMCA0gAMEUCIC/JAfnl
uC30ihqepbiMCaTaPMbL8Ka2Lk92IYfMhf46AiEAz9Kmv6HF2D4MK54iwhz2WqvF
8vo+OiGdTQ1Qoj7fgYU=
-----END CERTIFICATE-----
-----BEGIN EC PRIVATE KEY-----
MHcCAQEEIKy6mPLJc5v71InMMvYaxyXI3xXpwQTPLyAYWVFnZHVioAoGCCqGSM49
AwEHoUQDQgAEAMDhmpuwCjouSypm/ZStawMwNiU6aaqxYiGlHY6ph2SAiT9NFrdH
X6jrRI4DBL9R4btQ5imZ9nob6ccD0swa+Q==
-----END EC PRIVATE KEY-----

BeryJu added a commit to goauthentik/authentik that referenced this issue Jan 18, 2023
Signed-off-by: Jens Langhammer <jens@goauthentik.io>
BeryJu added a commit to goauthentik/authentik that referenced this issue Jan 18, 2023
Signed-off-by: Jens Langhammer <jens@goauthentik.io>
BeryJu added a commit to goauthentik/authentik that referenced this issue Jan 18, 2023
* add option to exclude x5*

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

#4082

* cleanup jwks, add flaky test

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

* add workaround based on jpadilla/pyjwt#709

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

* don't rstrip hashes

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

* keycloak seems to strip equals

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

Signed-off-by: Jens Langhammer <jens@goauthentik.io>
@vancouverwill
Copy link

In case it helps, here's a rouch sketch of how I solved it for to_jwk. I'm not sure if there's anywhere else it needs to be fixed, but the JWK standard does specify that it should be 66 bytes, so if validation of another library's implementation is failing, I think that's an issue with those libraries:


ES_CURVES_MINIMUM_PARAMETER_LENGTH = {
    'P-521': 66,
}

def bytes_from_int(val: int, min_length=None) -> bytes:
    remaining = val
    byte_length = 0

    while remaining != 0:
        remaining >>= 8
        byte_length += 1

    if min_length and byte_length < min_length:
        byte_length = min_length

    return val.to_bytes(byte_length, "big", signed=False)


def to_base64url_uint(val: int, min_length=None) -> bytes:
    # See https://github.com/jpadilla/pyjwt/issues/709
    if val < 0:
        raise ValueError("Must be a positive integer")

    int_bytes = bytes_from_int(val, min_length=min_length)

    if len(int_bytes) == 0:
        int_bytes = b"\x00"

    return jwt.utils.base64url_encode(int_bytes)


class ECAlgorithm(with_metaclass(MonkeyPatchMeta, jwt.algorithms.ECAlgorithm)):
    @staticmethod
    def to_jwk(public_key):
        ...
        min_length = ES_CURVES_MINIMUM_PARAMETER_LENGTH.get(curve)

        return json.dumps({
            "crv": curve,
            "x": to_base64url_uint(numbers.x, min_length=min_length).decode("utf-8"),
            "y": to_base64url_uint(numbers.y, min_length=min_length).decode("utf-8"),
            "kty": "EC",
        })

this solution worked for me. Would be good to get it merge in. Note I generated my key with

openssl ecparam -genkey -name secp521r1 -noout -out ecdsa-p521-private.pem

@vancouverwill
Copy link

Seems like the proper fix is in to_jwk, not the validation.

I get this issue when converting a p-512 jwt to key with from_key, the public key in question was generated with https://bitbucket.org/b_c/jose4j/wiki/Home

@DylanYoung
Copy link

DylanYoung commented Mar 20, 2023

@vancouverwill I suspect that's an issue with jose4j, but I can't seem to find the source code to check (is it proprietary?).

It would probably be helpful to have a "loose" mode for from_key that accepts keys that don't respect the minimum length requirements of the spec (since so many implementations seem to miss it).

EDIT: Found the source code, and if I'm reading it correctly, it looks like they have the same bug here: https://github.com/RbkGh/Jose4j/blob/c66ffa2859db44a86f064d2251e639204ceabbb7/src/main/java/org/jose4j/jwk/PublicJsonWebKey.java#L243

(Notice how they don't add any padding)

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

No branches or pull requests

5 participants