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

The iss value isn't validated when encoding payload #1039

Open
webknjaz opened this issue Feb 18, 2025 · 1 comment
Open

The iss value isn't validated when encoding payload #1039

webknjaz opened this issue Feb 18, 2025 · 1 comment

Comments

@webknjaz
Copy link

webknjaz commented Feb 18, 2025

I was looking into what PyGitHub does and writing test for it, invoking its JWT methods. Then I attempted decoding that JWT with jwt.decode(..., issuer=123) since there was 'iss': 123 in the object. The validator treats anything non-str as iterables and crashes:

    def _validate_iss(self, payload: dict[str, Any], issuer: Any) -> None:
        if issuer is None:
            return
    
        if "iss" not in payload:
            raise MissingRequiredClaimError("iss")
    
        if isinstance(issuer, str):
            if payload["iss"] != issuer:
                raise InvalidIssuerError("Invalid issuer")
        else:
>           if payload["iss"] not in issuer:
E           TypeError: argument of type 'int' is not iterable

issuer     = 123
payload    = {'exp': 1739881471, 'iat': 1739881111, 'iss': 123}
self       = <jwt.api_jwt.PyJWT object at 0x7f3101bc93a0>

When I attempted jwt.decode(..., issuer='123') it hit another code branch and crashed in the equality check:

    def _validate_iss(self, payload: dict[str, Any], issuer: Any) -> None:
        if issuer is None:
            return
    
        if "iss" not in payload:
            raise MissingRequiredClaimError("iss")
    
        if isinstance(issuer, str):
            if payload["iss"] != issuer:
>               raise InvalidIssuerError("Invalid issuer")
E               jwt.exceptions.InvalidIssuerError: Invalid issuer

issuer     = '123'
payload    = {'exp': 1739881584, 'iat': 1739881224, 'iss': 123}
self       = <jwt.api_jwt.PyJWT object at 0x7ff5d86cd2e0>

I understand why this is happening and I'm not the one creating the payload with iss being of int type. I believe that the root cause is that PyGitHub creates it with an integer and the bug is that PyJWT allows it, not validating the input.

I checked the spec @ https://datatracker.ietf.org/doc/html/rfc7519#section-4.1 and it says this is supposed to be a string:

4.1.1. "iss" (Issuer) Claim

The "iss" (issuer) claim identifies the principal that issued the
JWT. The processing of this claim is generally application specific.
The "iss" value is a case-sensitive string containing a StringOrURI
value. Use of this claim is OPTIONAL.

(emphasis mine)

So here we are — PyJWT allows putting arbitrary values into payload on encoding but expects them to be spec-compliant on decoding.

I think, there are two things necessary to maintain consistency here — input validation with jwt.encode() checking that the values provided are of legal types, and maybe better type checking and error messages in jwt.decode() reporting illegal data types.

@webknjaz webknjaz moved this to 🦉 Inclusion ⚖️ in 📅 Procrastinating in public Feb 18, 2025
webknjaz added a commit to arestlelansibletest/awx-plugins that referenced this issue Feb 19, 2025
This includes saving integer IDs as `iss` strings in JWT.

Refs:
* PyGithub/PyGithub#3213
* PyGithub/PyGithub#3214
* jpadilla/pyjwt#1039
webknjaz added a commit to webknjaz/ansible--awx-plugins that referenced this issue Feb 19, 2025
This patch uses PyGitHub to implement it. It currently only supports
GitHub App Client IDs due to limitations in PyGitHub [[1]]. It also
takes care of passing it as a string into the library because PyGitHub
does not perform type conversion, which sometimes leads to producing
invalid JWTs [[2]] as the underlying library PyJWT lacks validation
[[3]].

The plugin accepts both GitHub App and installation IDs as integers
and normalizes them into strings.

The tests use ephemerally-produced in-memory RSA keys for JWT
verification. As an optimization, they are 1024-bit sized because it
is cheaper and makes the tests more performant compared to bigger
values.

As a temporarily measure, the patch includes a suppression of WPS202
in flake8 for `github_app.py`.

A few type-ignores were added around PyGitHub exception handling due
to them using `Any` [[4]].

The patch also includes a change to the flake8-eradicate config
letting distinguish comments with URL lists.

[1]: PyGithub/PyGithub#3213
[2]: PyGithub/PyGithub#3214
[3]: jpadilla/pyjwt#1039
[4]: PyGithub/PyGithub#3218

Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
webknjaz added a commit to webknjaz/ansible--awx-plugins that referenced this issue Feb 19, 2025
This patch uses PyGitHub to implement it. It currently only supports
GitHub App IDs due to limitations in PyGitHub [[1]]. It also
takes care of passing it as a string into the library because PyGitHub
does not perform type conversion, which sometimes leads to producing
invalid JWTs [[2]] as the underlying library PyJWT lacks validation
[[3]].

The plugin accepts both GitHub App and installation IDs as integers
and normalizes them into strings.

The tests use ephemerally-produced in-memory RSA keys for JWT
verification. As an optimization, they are 1024-bit sized because it
is cheaper and makes the tests more performant compared to bigger
values.

As a temporary measure, the patch includes a suppression of WPS202
in flake8 for `github_app.py`.

A few type-ignores were added around PyGitHub exception handling due
to them using `Any` [[4]].

The patch also includes a change to the flake8-eradicate config
letting distinguish comments with URL lists.

[1]: PyGithub/PyGithub#3213
[2]: PyGithub/PyGithub#3214
[3]: jpadilla/pyjwt#1039
[4]: PyGithub/PyGithub#3218

Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
webknjaz added a commit to webknjaz/ansible--awx-plugins that referenced this issue Feb 19, 2025
This patch uses PyGitHub to implement it. It currently only supports
GitHub App IDs due to limitations in PyGitHub [[1]]. It also
takes care of passing it as a string into the library because PyGitHub
does not perform type conversion, which sometimes leads to producing
invalid JWTs [[2]] as the underlying library PyJWT lacks validation
[[3]].

The plugin accepts both GitHub App and installation IDs as integers
and normalizes them into strings.

The tests use ephemerally-produced in-memory RSA keys for JWT
verification. As an optimization, they are 1024-bit sized because it
is cheaper and makes the tests more performant compared to bigger
values.

As a temporary measure, the patch includes a suppression of WPS202
in flake8 for `github_app.py`.

A few type-ignores were added around PyGitHub exception handling due
to them using `Any` [[4]].

The patch also includes a change to the flake8-eradicate config
letting distinguish comments with URL lists.

[1]: PyGithub/PyGithub#3213
[2]: PyGithub/PyGithub#3214
[3]: jpadilla/pyjwt#1039
[4]: PyGithub/PyGithub#3218

Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
webknjaz added a commit to webknjaz/ansible--awx-plugins that referenced this issue Feb 19, 2025
This patch uses PyGitHub to implement it. It currently only supports
GitHub App IDs due to limitations in PyGitHub [[1]]. It also
takes care of passing it as a string into the library because PyGitHub
does not perform type conversion, which sometimes leads to producing
invalid JWTs [[2]] as the underlying library PyJWT lacks validation
[[3]].

The plugin accepts both GitHub App and installation IDs as integers
and normalizes them into strings.

The tests use ephemerally-produced in-memory RSA keys for JWT
verification. As an optimization, they are 1024-bit sized because it
is cheaper and makes the tests more performant compared to bigger
values.

As a temporary measure, the patch includes a suppression of WPS202
in flake8 for `github_app.py`.

A few type-ignores were added around PyGitHub exception handling due
to them using `Any` [[4]].

The patch also includes a change to the flake8-eradicate config
letting distinguish comments with URL lists.

[1]: PyGithub/PyGithub#3213
[2]: PyGithub/PyGithub#3214
[3]: jpadilla/pyjwt#1039
[4]: PyGithub/PyGithub#3218

Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
webknjaz added a commit to webknjaz/ansible--awx-plugins that referenced this issue Feb 19, 2025
This patch uses PyGitHub to implement it. It currently only supports
GitHub App IDs due to limitations in PyGitHub [[1]]. It also
takes care of passing it as a string into the library because PyGitHub
does not perform type conversion, which sometimes leads to producing
invalid JWTs [[2]] as the underlying library PyJWT lacks validation
[[3]].

The plugin accepts both GitHub App and installation IDs as integers
and normalizes them into strings.

The tests use ephemerally-produced in-memory RSA keys for JWT
verification. As an optimization, they are 1024-bit sized because it
is cheaper and makes the tests more performant compared to bigger
values.

As a temporary measure, the patch includes a suppression of WPS202
in flake8 for `github_app.py`.

A few type-ignores were added around PyGitHub exception handling due
to them using `Any` [[4]].

The patch also includes a change to the flake8-eradicate config
letting distinguish comments with URL lists.

[1]: PyGithub/PyGithub#3213
[2]: PyGithub/PyGithub#3214
[3]: jpadilla/pyjwt#1039
[4]: PyGithub/PyGithub#3218

Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
@pachewise
Copy link
Contributor

pachewise commented Feb 21, 2025

hmm, it looks like we have the logic to support list of str in issuer param through this PR: #913

My proposal is to throw a ValueError 1) if iss is not a str; 2) if issuer is not either a str or a list of str. Given we currently do an "in" check for that, we've implicitly supported tuples/sets/dicts, too. For now, I will punt on stricter type-checking for that (see the PR for details)

pachewise added a commit to pachewise/pyjwt that referenced this issue Feb 21, 2025
pachewise added a commit to pachewise/pyjwt that referenced this issue Feb 23, 2025
pachewise added a commit to pachewise/pyjwt that referenced this issue Feb 26, 2025
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

No branches or pull requests

2 participants