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
Fix and test adding new issuers #260
base: main
Are you sure you want to change the base?
Fix and test adding new issuers #260
Changes from 2 commits
3659f05
e2539f4
37618bc
a2359f2
92020eb
9497639
9c2cf3e
fa65601
ab9f92b
d039514
824775c
d3a2843
e7cd184
4252e2f
685aad3
a8192dc
3a79912
9c572f3
08e69e9
d93bd4c
c78a9c6
e70301a
68b9668
34b7d0b
d932624
d74e9b3
6bc2264
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.
Can you explain the difference between these two things? They're both URLs and I presume they both issue tokens so maybe we can think of a better naming scheme?
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.
Also not super keen on JWT_TOKEN because the T in JWT stands for token.
So we have JSON Web Token Token Issuer
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.
JWT_ISSUER is the name used in CCF for registering the JWT issuer.
JWT_TOKEN_ISSUER_URL is the actual url of the endpoint.
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.
it is very common to talk about JWT tokens. Just do a search for "JWT 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.
Ah thanks for the explanation, that makes sense
Being common doesn't make it correct or less redundant, and I wouldn't claim it's more common that just JWT, jwt.io always just says JWT.
How about we use
JWT_ISSUER_URL
which is the URL which issues JWTsJWT_ISSUER_NAME
which is the name of the thing which issues JWTs (for CCF)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 believe CCF requires this to be a url. Need to check this.
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.
If it needs to be a URL, could it just be the actual URL so we only need one variable instead of two?
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.
indeed.
{
bool validate_issuer(
const std::string& iss,
const std::optionalstd::string& tid,
std::string constraint)
{
LOG_DEBUG_FMT(
"Verify token.iss {} and token.tid {} against published key issuer {}",
iss,
tid,
constraint);
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.
See #260 (comment)
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 understand it's a test JWT issuer, but why can't both values be
http://localhost:3000/token
? In which case the JWT issuer up script only needs to emit one value not two? Isn't it also kind of wrong to have a pretend URL for the issuer which doesn't match it's actual URL?I've tested this locally with a couple of small changes to the issuer and it works fine
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 my own understanding, the scheme is:
If my understanding is correct I really like this scheme!
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 an external one we need to set JWT_TOKEN_ISSUER_URL to the token endpoint.
--iss will the name for the iss property in the token. CCF uses this to register the issuer.
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.
So is it equivalent to setting
JWT_ISSUER
? In which case why are we mixing methods? I think we either allow fully specifying which JWT issuer we're trusting via command line params or we just require the env variables to be changed?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 added a test adding a new issuer implemented by our test token issuer.
For this case I needed to specify the issuer JWT_ISSUER and keep the url to the test token url.
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 that addresses my point about mixed methods, for the test you could set both env variables for the test, or you could provider command line params for both and specify them for the test?
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.
Now we've decided to unify JWT_ISSUER this goes away
This file was deleted.