Allow JWT::decode to accept an empty string as a valid kid

[ryanneufeld]

There are instances when using CachedKeySet where a key is returned with an empty string as the kid. This is a valid use case and should be allowed.

For example Teleport Proxy uses this pattern to allow for a default key.

{
    "keys": [
        {
            "kty": "RSA",
            "alg": "RS256",
            "n": "<redacted>",
            "e": "AQAB",
            "use": "sig",
            "kid": ""
        }
    ]
}

Newer versions of teleport also return:

{
    "keys": [
        {
            "kty": "RSA",
            "alg": "RS256",
            "n": "<same key redacted>",
            "e": "AQAB",
            "use": "sig",
            "kid": "<redacted kid>"
        },
        {
            "kty": "RSA",
            "alg": "RS256",
            "n": "<same key redacted>",
            "e": "AQAB",
            "use": "sig",
            "kid": ""
        }
    ]
}

The getKey method can be simplified, as well as refactored to follow the same pattern as the CachedKeySet class which casts null kids to an empty string.

This change also adds a test to ensure that an empty string kid is a valid kid.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[ryanneufeld]

I've signed the CCA, is there anyone who has time to review this?

[bshaffer]

@ryanneufeld thank you for your submission! I'll take a look.

Do you know if the spec defines this as a valid use of kid? Or is it primarily that you've observed this behavior in the wild as a valid use case?

[tabacco]

It's not explicitly specified, except that it's a case-sensitive string: https://datatracker.ietf.org/doc/html/rfc7517#section-4.5

In the wild, I've observed JWK data with an extra key containing a kid of "" from Teleport, so it's definitely a Thing.

[ryanneufeld]

@ryanneufeld thank you for your submission! I'll take a look.

Do you know if the spec defines this as a valid use of kid? Or is it primarily that you've observed this behavior in the wild as a valid use case?

As Doug mentioned. There are use-cases in the wild where this patch would enable support without special code to handle it at the implementation layer.

[ryanneufeld]

@bshaffer Any updates on this?

[bshaffer]

It's not clear to me this functionality is a valid use of the spec. For that reason I'm reticent to add it.

[bshaffer]

Why are you removing these lines?

[ryanneufeld]

The new lines below cover this test as the CachedKeySet implements ArrayAccess

The resulting logic is the same
https://github.com/firebase/php-jwt/blob/main/src/CachedKeySet.php#L19C1-L19C42

[bshaffer]

If you read the comment of the removed line, you'll see that the isset check triggers a refresh of the credentials, which we don't want to or need to do.

[ryanneufeld]

It's not clear to me this functionality is a valid use of the spec. For that reason I'm reticent to add it.

Can you point to the part of the RFC that forbids this?

https://datatracker.ietf.org/doc/html/rfc7517#section-4.5

The "kid" (key ID) parameter is used to match a specific key. This
is used, for instance, to choose among a set of keys within a JWK Set
during key rollover. The structure of the "kid" value is
unspecified. When "kid" values are used within a JWK Set, different
keys within the JWK Set SHOULD use distinct "kid" values. (One
example in which different keys might use the same "kid" value is if
they have different "kty" (key type) values but are considered to be
equivalent alternatives by the application using them.) The "kid"
value is a case-sensitive string. Use of this member is OPTIONAL.
When used with JWS or JWE, the "kid" value is used to match a JWS or
JWE "kid" Header Parameter value.

So an empty value is a case-sensitive string, though not an ideal one.

[bshaffer]

@ryanneufeld are you being serious?

[ryanneufeld]

@ryanneufeld are you being serious?

I'm not sure how to respond to this question, you said you're not sure if it meets the spec. I'm not sure which spec you're referring to, so if you were to point me in the right direction I could address your concerns.

So, yes, I am being serious, not combative.

[bshaffer]

My concern is not that the spec forbids it, my concern is that this adds logic which is not defined by any spec.

Supporting an empty string as a KID is one thing, but selecting a key by default when the KID is empty is another.

[ryanneufeld]

@bshaffer I see your point. I suppose the idea here is that we need a way to pick a key.

The idea here is that if there are two keys, and one has an empty KID, and no KID is passed in for selection, then allow the one with the empty key to match.

The alternative involves parsing the JWT's multiple times in order to validate them.

I welcome any suggestions/collaboration you have to offer for resolving this situation.

[ryanneufeld]

@bshaffer After looking into more closely, I made a refactor that I think will satisfy your concerns, while maintaining the previous functionality.

Additionally this will increase the code coverage with some more tests.

[bshaffer]

See https://g.co/gemini/share/76e85b5fc2d8. The two main concerns which I also share are

Ambiguity and Collision: An empty string kid means multiple JWKs could theoretically have the same identifier. 
This makes it impossible to uniquely identify the key used to sign a specific JWT. When validating a JWT, the 
library wouldn't know which key from the JWK Set to use.

Interoperability Issues: The JSON Web Key (JWK) specification (RFC 7517) defines the kid parameter as a 
"hint indicating which key was used to secure the JWS/JWE." While it doesn't explicitly forbid an empty string,
the intent is for it to be a unique identifier. 

I think it'd be a better approach for Teleport to start producing standard JWKs rather than have this library support non-standard functionality.