Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Security/Extension] JWT Vendor for extensions #2567
[Security/Extension] JWT Vendor for extensions #2567
Changes from all commits
e11e5f9
d5bfef3
114cbde
6bd8d40
367752b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a good alternative since we need to have a fixed reference point and this is the most common way to get time relative to the Epoch. However, it is worth noting that this is not monotonic so you could get subsequent calls with the same value returned even if the time difference is more than 1 ms. It should not matter if we have some clock skew tolerance but just worth mentioning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be worthwhile to document why we picked some of the defaults in the code next to them,
HMAC was picked because ...
(BTW think HMAC is a good choice for our scenario)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a simple T-chart of HMAC vs RSA based on the research I did:
- Flexibility: HMAC can work with various hash functions, such as SHA-256, SHA-384, or SHA-512, depending on the desired security level and performance.
- Not suitable for non-repudiation, as anyone with the secret key can create a valid signature.
- Public key cryptography: RSA uses a public and private key pair, allowing for easier key management and distribution. The public key can be shared openly, while the private key remains secret.
- Key size: RSA requires larger keys for equivalent security levels compared to symmetric algorithms like HMAC. For example, a 3072-bit RSA key provides roughly the same level of security as a 256-bit symmetric key.
Another algorithm, ECDSA (Elliptic Curve Digital Signature Algorithm), is pretty similar to RSA, but a little bit faster (the performance ranking from most efficiency to less efficiency should be: HMAC > ECDSA > RSA). However, back to our case, I agree that HMAC is good for our scenario, due to its performance and simplicity. More importantly, it meets our requirement of supporting both data integrity and authenticity, so that the payload of our token cannot be tampered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this
else {...}
section. There should be a single code path for the way these tokens are generated, and this is a case where customization shouldn't be allowed.Note; if we have a user-facing usage of token generated, we could allow other options / customizations, but we should expose it via a seperate API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peternied This
else
clause is what makes this flexible to support creating JWKs with any properties that are not the default HMAC SHA512 symmetric signing key.This would allow user's to configure asymmetric encryption for the JWT signing if desired.
i.e.
vs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend we start exposing as little surface area as possible around these new features.
As this is part of flow where generating the sensitive tokens e.g. on-behalf-of user tokens for extensions. I would advocate that we minimize the risk of misconfiguration by locking down these values in code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peternied and @cwperks what was the verdict here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the experimental release we are aiming for sensible defaults. In this case HMAC SHA512 is chosen as the sensible defaults. Customizability will be added, but less of a priority then making sure all key functionality works first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we've fully locked down the design - but I am in the camp that we should not include any mapped roles/backend roles onto this claims. @RyanL1997 Could you find out where we are making this decision and reference it?
If we are not including any AuthZ information we should make sure we remove it from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can continue our discussion over here: #2545. For this one, I was just write up the function we may need for implementing it. 100% we can change/remove it anytime. And also, this function should be locate in a separate class if we choose to go down this path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got another issue about user information parity, that might cover it, but I don't think it clearly calls out that we don't include this information inside the authentication token. Or is there another issue where we have this called out, if not want to make one so we can track?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, thanks for the information! I will create separate issue attach to this for the tracking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peternied @RyanL1997 I'm curious to get your thoughts on an idea of encrypting the value of the claim itself for sensitive claims. For cases with an external IdP, I think we will need to store the roles in a claim of the token because they cannot be looked up when the security plugin receives the token as part of a request. Since they cannot be looked up, they will need to be part of the claims of the token. The current JWT backend (and OIDC and SAML since they also use JWTs) already assumes that roles are included as a claim of the token.
Encryption of the sensitive claim can be done using a utility like (Reference SO post: https://stackoverflow.com/a/57902503):
The
secret
that this function takes is different than thesigning_key
that this PR introduces and would be another setting on the same level. I wrote a test with this behavior here: RyanL1997/security@jwt-generator-for-extensions...cwperks:security:jwt-generator-for-extensionsHere's the output of the test. The first line is the encrypted claim that the extension would see and below is decrypted that the security plugin would be able to decrypt from the claim inside the token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cwperks, thanks for putting this together. This is making sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Assuming we are keeping AuthZ info in the token, otherwise this should be deleted) We shouldn't be attributing any roles based on the remote address because the request will be performed from that address but via the extension - which is elsewhere. We might want to break this out into a separate issue to discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RyanL1997, did this get split into the separate issue? It is the last area where I have a concern around merging. As long as there is an issue, I am fine with following-up in this case but could you share the link please? Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to add further negative test cases down the road. Since you are only looking at the vending this seems fine for now, but once we have verification as well, we will want to test expiration compliance and signature mismatches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% We need an authc backend to do the verification, so that we can test them. I''m working on that.