Skip to content

Conversation

@naroraindeed
Copy link
Contributor

Using credentialcomposer plugins forces Claims to be translated as protobuf structs which serializes integers as floats (#4982). AWS rejects validating JWT issued by SPIRE with timestamps that are in scientific notation. AWS STS only accepts integer timestamps as valid. We've discussed this with AWS, and while they agree it's an issue in AWS STS, there's no recourse available with them. This fix helps reset value type for timestamps and also includes unit tests that proves the problem and the limited scope of the fix. This is the minimal change needed for SPIRE to produce verifiable JWT for AWS when using credentialcomposer plugin.

Signed-off-by: narora@indeed.com

Pull Request check list

  • [Y] Commit conforms to CONTRIBUTING.md?
  • [Y] Proper tests/regressions included?
  • [N] Documentation updated?

Affected functionality
BuildWorkloadJWTSVIDClaims resets data type for certain claims.

Description of change
Minimal change needed for SPIRE to produce verifiable JWT for AWS when using credentialcomposer plugin.

Which issue this PR fixes
fixes #4982

…patiblity with AWS

Using credentialcomposer plugins forces Claims to be translated as protobuf structs which serializes integers as floats (spiffe#4982). AWS rejects validating JWT issued by SPIRE with timestamps that are in scientific notation. AWS STS only accepts integer timestamps as valid. We've discussed this with AWS, and while they agree it's an issue in AWS STS, there's no recourse available with them. This fix helps reset value type for timestamps and also includes unit tests that make the problem obvious. This is the minimal change needed for SPIRE to produce verifiable JWT for AWS when using credentialcomposer plugin.

Signed-off-by: Nikhil Arora <narora@indeed.com>
@naroraindeed
Copy link
Contributor Author

fixed lint and DCO workflow failures. Need help to retrigger workflows.

Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Thanks for this quick fix @naroraindeed. This will be great for the upcoming 1.9.5 release. I think for 1.10.0 we'll do something a little more broadly scoped and this PR lays out some important groundwork. I really appreciate the contribution.

@azdagron azdagron added this to the 1.9.5 milestone May 1, 2024
@azdagron azdagron merged commit 9deb317 into spiffe:main May 1, 2024
@naroraindeed
Copy link
Contributor Author

Thank you so much @azdagron . I completely agree, this is a tactical fix to make AWS happy. This doesn't fix the generic problem of representing Claims in a way that preserves the integer value type in composer plugins. That's a good next step.
You've reduced the burden on our end to maintain a separate fork for core SPIRE. 👍🏾

@liwadman
Copy link

liwadman commented May 2, 2024

@azdagron A review of many common JWT validation libraries seemed to indicate that by type validation alone, timestamps as floats/scientific notation wouldn't be supported. I will say I that I didn't test all these with code, but rather the their code for validating IAT/NBF/EXP claims. Thus it would seem there is extremely mixed support in the JWT ecosystem for non-integer timestamp values, and in time likely it would surface more than just AWS STS would be impacted by this issue.

Libraries I Checked:
Nimbus - java (floats not supported by type validation)
Jose - typescript (floats allowed by type validation)
PyJWT - python(floats not supported by type validation)
Python-jose - python(floats not supported by type validation)
Golang-jwt - go (floats allowed by type validation)
g-jose - go (floats not supported by type validation)
java-jwt - java(floats not supported by type validation)
node-jose javascript(floats allowed by type validation)

@amartinezfayo amartinezfayo modified the milestones: 1.9.5, 1.9.6 May 8, 2024
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.

4 participants