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

Support JWK without alg. #624

Merged
merged 7 commits into from Mar 22, 2021
Merged

Conversation

dajiaji
Copy link
Contributor

@dajiaji dajiaji commented Mar 7, 2021

I don't think it is the best way but for now it is a reasonable solution for supporting JWKs which don't have "alg" (e.g., Azure AD public JWKs).

Considering backward compatibility, I made kty validation (and alg guessing) activated only when a JWK does not have "alg".
Since all of existing tests passed, I made kty parameter mandatory compliant with RFC7517.

In addition, test coverage is not perfect because Ed25519Algorithm does not have from_jwk() function.
if you merge PR #623, I can send another PR for that.

I'm very happy if you check and merge it.

Close #603
Close #616

Copy link
Contributor

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

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

jwi.io lists the following possibilities, only some of which are supported in this pull request

  • HS256
  • HS384
  • HS512
  • PS256
  • PS384
  • PS512
  • RS256
  • RS384
  • RS512
  • ES256
  • ES256K
  • ES384
  • ES512
  • EdDSA

TBH I'm not sure where to get sample tokens for all of the algorithms
(jwt.io has sample tokens for most, but not all)

@dimaqq
Copy link
Contributor

dimaqq commented Mar 11, 2021

jwt.io tokens are here: https://github.com/jsonwebtoken/jsonwebtoken.github.io/blob/bf9a99c8f37aacbcac0b58b9877179073aea32a1/src/editor/default-tokens.js
the repo is MIT license, so I guess the test vectors can be borrowed?

@dajiaji
Copy link
Contributor Author

dajiaji commented Mar 13, 2021

@dimaqq Thanks for your comments and information.

At the first place, keys in JWKs are independent of hash algorithms, at least in "RSA"(RS*, PS*) and "oct"(HS*).
As someone mentioned in #603, "a single RSA key could be used to sign with RS256, RS384, and RS512"

Therefore, the hash algorithm should not be specified when the JWK is loaded (especially if there is no alg parameter), but should be specified with the signature during verification.

However, in the current implementation of pyjwt, the hash algorithm must be specified during the JWK decoding phase due to pyjwt class structure, and this PR is a workaround to implicitly set the default value of alg.

In case of kty=RSA, I adopted RS256 as a default alg value as it is "Recommended" in RFC7518. Again, because it is not possible to determine whether it is RS256, RS384, RS512 from the information contained in JWK (kty=RSA).

Similarly, In case of kty=oct, I adopted HS256 as a default alg value. as it is a unique "Required" algorithm in RFC7518.

On the other hand, in case of kty=EC, crv can be used to determine the `alg' value.

ES256K

At least, we need another PR to support it (RFC8812).

@dajiaji dajiaji changed the title Support JWK without alg. [WIP] Support JWK without alg. Mar 18, 2021
@dajiaji
Copy link
Contributor Author

dajiaji commented Mar 18, 2021

After #623 merged, I'll add support for ES256K, add tests and remove WIP.

@dajiaji dajiaji changed the title [WIP] Support JWK without alg. Support JWK without alg. Mar 19, 2021
@auvipy auvipy merged commit 81d5829 into jpadilla:master Mar 22, 2021
@dajiaji dajiaji deleted the support-jwk-without-alg branch March 22, 2021 11:15
dajiaji added a commit to dajiaji/pyjwt that referenced this pull request Mar 22, 2021
auvipy pushed a commit that referenced this pull request Mar 22, 2021
@agentgonzo
Copy link

@auvipy @jpadilla - is it possible to issue a new release of the library? 2.0.1 was released in January and doesn't include this fix (that we'd like to use!)

@MarcinBMD
Copy link

@auvipy @jpadilla - is it possible to issue a new release of the library? 2.0.1 was released in January and doesn't include this fix (that we'd like to use!)

We also would like to use this and are affected by it, as our source of JWT also only uses 'kty'. A release would be great.

@agentgonzo
Copy link

agentgonzo commented Apr 21, 2021

For anyone else (@MarcinBMD) facing this, here's my workaround that you can apply to a 2.0.1 (or before) version of the library that fixes the bug. It just takes monkey-patches the __init__ method with the one from the fix.

def monkey_patch(self, jwk_data, algorithm=None):
    self._algorithms = get_default_algorithms()
    self._jwk_data = jwk_data

    kty = self._jwk_data.get("kty", None)
    if not kty:
        raise InvalidKeyError("kty is not found: %s" % self._jwk_data)

    if not algorithm and isinstance(self._jwk_data, dict):
        algorithm = self._jwk_data.get("alg", None)

    if not algorithm:
        # Determine alg with kty (and crv).
        crv = self._jwk_data.get("crv", None)
        if kty == "EC":
            if crv == "P-256" or not crv:
                algorithm = "ES256"
            elif crv == "P-384":
                algorithm = "ES384"
            elif crv == "P-521":
                algorithm = "ES512"
            elif crv == "secp256k1":
                algorithm = "ES256K"
            else:
                raise InvalidKeyError("Unsupported crv: %s" % crv)
        elif kty == "RSA":
            algorithm = "RS256"
        elif kty == "oct":
            algorithm = "HS256"
        elif kty == "OKP":
            if not crv:
                raise InvalidKeyError("crv is not found: %s" % self._jwk_data)
            if crv == "Ed25519":
                algorithm = "EdDSA"
            else:
                raise InvalidKeyError("Unsupported crv: %s" % crv)
        else:
            raise InvalidKeyError("Unsupported kty: %s" % kty)

    self.Algorithm = self._algorithms.get(algorithm)

    if not self.Algorithm:
        raise PyJWKError("Unable to find a algorithm for key: %s" % self._jwk_data)

    self.key = self.Algorithm.from_jwk(self._jwk_data)


# PyJWKClient 2.0.1 (latest release at time of writing) contains a bug: https://github.com/jpadilla/pyjwt/pull/624. This monkey patch fixes it.
PyJWK.__init__ = monkey_patch

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