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

builtins: Marshal JWT encode sign inputs as JSON #6934

Merged

Conversation

charlieegan3
Copy link
Contributor

@charlieegan3 charlieegan3 commented Aug 15, 2024

Fixes #6925

This change modifies the io.jwt.encode_sign built-in to marshal the parameters as JSON before signing the JWT. The previous implementation was using String(), which was a sort of pseudo-JSON that was almost correct but used the wrong JSON syntax for the set type.

I have chosen to marshal the headers in the same way as the payload as the 'crit' header can also be an array type. I marshal the signature the same way for consistency with the other two.

The tokens have changed in the assertions for whitespace adjustments. Before the 'JSON' looked like this {"foo": "bar", "bar": "foo"} and now it looks like this {"foo":"bar","bar":"foo"}. This is a shame, but seemed acceptable.

Copy link
Contributor

@johanfylling johanfylling left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Regarding the whitespaces, this is within the confines of the spec, and arguably the produced JWTs are slightly smaller now 😄. Worst case, we're breaking a couple of unit tests out there.

Fixes open-policy-agent#6925

This change modifies the `io.jwt.encode_sign` built-in to marshal the
parameters as JSON before signing the JWT. The previous implementation
was using String(), which was a sort of pseudo-JSON that was almost
correct but used the wrong JSON syntax for the set type.

I have chosen to marshal the headers in the same way as the payload as
the 'crit' header can also be an array type. I marshal the signature the
same way for consistency with the other two.

Signed-off-by: Charlie Egan <charlie@styra.com>
@ashutosh-narkar ashutosh-narkar merged commit 14ff052 into open-policy-agent:main Aug 15, 2024
28 checks passed
@charlieegan3
Copy link
Contributor Author

Thanks both!

@charlieegan3 charlieegan3 deleted the marshal-jwt-inputs branch August 15, 2024 17:23
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

Successfully merging this pull request may close these issues.

builtins: Error JWT but payload is not JSON when using io.jwt.encode_sign with set as payload value
3 participants