Skip to content

Conversation

@rhafer
Copy link
Contributor

@rhafer rhafer commented Nov 21, 2024

golang-jwt/jwt/v5 saw quite some restructing with regards to the 'Claims' Interface. This commit tries to follow the migration guide from https://github.com/golang-jwt/jwt/blob/bc8bdca5cced1caa9787e4a1c313a3538544c877/MIGRATION_GUIDE.md

Also this removes the no longer used 'signing' package.

@longsleep longsleep self-requested a review November 26, 2024 09:43
Copy link
Collaborator

@longsleep longsleep left a comment

Choose a reason for hiding this comment

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

Looks almost good, just the Audience now a slice seems to be problematic. Generally I think in certain cases we might need to look at all values to be really compatible with cases when there actually are multiple aud values, but the issue with the empty slice needs to be dealt with imo.

@rhafer
Copy link
Contributor Author

rhafer commented Nov 26, 2024

Generally I think in certain cases we might need to look at all values to be really compatible with cases when there actually are multiple aud values, but the issue with the empty slice needs to be dealt with imo.

Are we ever generation a token that has more than one value in aud? I don't think so. And all the tokens we're dealing with are also generated by us, or am I missing something?

I am fine adding checks for the slice being empty, Though as far as I can see the tokens generated by lico are guaranteed to have a value in aud.

@longsleep
Copy link
Collaborator

I am fine adding checks for the slice being empty, Though as far as I can see the tokens generated by lico are guaranteed to have a value in aud.

I think there is no guarantee about how a token might have been generated when it is presented for validation. Any token can be presented and is validated - so expecting certain things for aud would rely on other previous checks to fail - seems a bad thing to rely on.

@rhafer
Copy link
Contributor Author

rhafer commented Nov 26, 2024

I think there is no guarantee about how a token might have been generated when it is presented for validation.

True. But I guess it would have failed the signature validation already if the presented token wasn't generated by us, right?

Any token can be presented and is validated - so expecting certain things for aud would rely on other previous checks to fail - seems a bad thing to rely on.

Ok, agreed. So I guess we could just err on the safe side and only consider tokens where len(claims.Audience) is exactly 1.

@longsleep
Copy link
Collaborator

signature validation already if the presented token wasn't generated by us, right?

That depends who has one of the valid private keys. That is probably exotic, but technically there is nothing wrong with multiple systems creating lico compatible access tokens. For this it matters little, one of the checks will fail and as long as we don't crash lico when the audience check comes in all should be fine.

golang-jwt/jwt/v5 saw quite some restructing with regards to the 'Claims'
Interface. This commit tries to follow the migration guide from
https://github.com/golang-jwt/jwt/blob/bc8bdca5cced1caa9787e4a1c313a3538544c877/MIGRATION_GUIDE.md

Also this removes the no longer used 'signing' package.
@rhafer
Copy link
Contributor Author

rhafer commented Dec 4, 2024

So I guess we could just err on the safe side and only consider tokens where len(claims.Audience) is exactly 1.

Ok. I've implemented it this way now. I tried to use the jwt.Validators where possible (Access- and RefreshTokens)

@rhafer rhafer requested a review from longsleep December 5, 2024 15:26
Copy link
Collaborator

@longsleep longsleep left a comment

Choose a reason for hiding this comment

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

LGTM. I wanted to release version 1.0 anyways - and this looks like a good enough reason to finally bump the major version for the next release.

@longsleep longsleep merged commit 704f6ef into libregraph:master Dec 17, 2024
1 check passed
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.

2 participants