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

JWT Assertion payload does not match RFC example when generated with OIDC relying party #452

Open
1 of 2 tasks
Kunde21 opened this issue Oct 4, 2023 · 8 comments
Open
1 of 2 tasks
Labels
auth enhancement New feature or request

Comments

@Kunde21
Copy link

Kunde21 commented Oct 4, 2023

Preflight Checklist

  • I could not find a solution in the documentation, the existing issues or discussions
  • I have joined the ZITADEL chat

Version

v2.11.0

Describe the problem caused by this bug

JWT assertion payload does not match format from example in rfc 7523 - section 4 causing errors when connecting to systems that require audience be a string value.

To reproduce

Create a relying party with the WithJWTProfile option, attempt a token exchange.

Generated assertion body:

{
  "iss": "2da2387b-f75f-4c86-a0fe-3109d8aa6943",
  "sub": "2da2387b-f75f-4c86-a0fe-3109d8aa6943",
  "aud": [
    "https://127.0.0.1/"
  ],
  "iat": 1696409593,
  "exp": 1696413193
}

Screenshots

No response

Expected behavior

Expected assertion body:

{
  "iss": "2da2387b-f75f-4c86-a0fe-3109d8aa6943",
  "sub": "2da2387b-f75f-4c86-a0fe-3109d8aa6943",
  "aud": "https://127.0.0.1/",
  "iat": 1696409593,
  "exp": 1696413193
}

Additional Context

No response

@Kunde21 Kunde21 added the bug Something isn't working label Oct 4, 2023
@hifabienne
Copy link
Member

@muhlemmer @livio-a Is there a reason why we have an array in the aud?
Does that change with the new version of the OIDC lib?

@livio-a
Copy link
Member

livio-a commented Oct 9, 2023

I've just checked section 4 and can't see a conflict in the implementation.
note the could:

Below is an example JSON object that could be encoded to produce the JWT Claims Set for a JWT:

Further the JWT RFC 7519 (https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.3) states that

[...] In the general case, the "aud" value is an array of case-
sensitive strings, each containing a StringOrURI value. In the
special case when the JWT has one audience, the "aud" value MAY be a
single case-sensitive string containing a StringOrURI value.

We certainly improve the serialization of a single string audience into a string, but i wouldn't classify it as a bug then.

@muhlemmer
Copy link
Collaborator

I second @livio-a his conclusion that our current implementation is valid. I just checked it without knowing he already replied.

causing errors when connecting to systems that require audience be a string value.

@Kunde21, can you be specific which systems are breaking on this? How would those systems handle multiple audience values? Would be strange if they work properly with multiple values (which they should) and break on a single value in an array.

Have you considered raising a bug or ticket at the "other" system?

We certainly improve the serialization of a single string audience into a string, but i wouldn't classify it as a bug then.

@livio-a we would end up again with more custom json marshalling on any claims bearing object, if we want to be consistent. Or use a separate type for all claims that allow the switch between string or array and have a marshal method on that. Either way it sounds like more code to maintain. I would abstain until it's proven to be absolutely necessary by the OP or others.

@muhlemmer muhlemmer added enhancement New feature or request and removed bug Something isn't working labels Oct 9, 2023
@muhlemmer muhlemmer changed the title [Bug]: JWT Assertion payload does not match RFC example when generated with OIDC relying party JWT Assertion payload does not match RFC example when generated with OIDC relying party Oct 9, 2023
@livio-a
Copy link
Member

livio-a commented Oct 10, 2023

We certainly improve the serialization of a single string audience into a string, but i wouldn't classify it as a bug then.

@livio-a we would end up again with more custom json marshalling on any claims bearing object, if we want to be consistent. Or use a separate type for all claims that allow the switch between string or array and have a marshal method on that. Either way it sounds like more code to maintain. I would abstain until it's proven to be absolutely necessary by the OP or others.

You're right. As the RFC even uses MAY and not SHOULD it's even less necessary.

@Kunde21
Copy link
Author

Kunde21 commented Oct 10, 2023

The context here are FAPI compliant auth servers at financial institutions that take a very strict reading of the FAPI part 2 Section 5.2.3:

In addition, the confidential client:
...
10. shall send the aud claim in the request object as the OP's Issuer Identifier URL
...

The token exchange fails for any value of aud that does not match the issuer (in some cases) or the token endpoint (in the rest).

@muhlemmer
Copy link
Collaborator

And a FAPI compliance test is failing with a single value was set to an array? Or is this a theoretical discussion?

@Kunde21
Copy link
Author

Kunde21 commented Oct 12, 2023

This is an active, production system that is being integrated upon. The oidc server is not going to change in this scenario.

@muhlemmer
Copy link
Collaborator

After some digging we found this conformance suite discussion.

Practically, the FAPI test suite introduces the error as it gives the idea the Audience should be a string. But that's a bug is the suite. Meanwhile, what I get from this comment is that the conformance test automatically flattens the array and therefore accepting implicitly a single element array during RP testing.


If it helps, I would be open to a PR that implements a Marshal method on the oidc.Audience type. And hope there are no OPs out there that only accept arrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth enhancement New feature or request
Projects
Status: 📨 Product Backlog
Development

No branches or pull requests

4 participants