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
Add comments on key discovery #111
Add comments on key discovery #111
Changes from 3 commits
c5741bd
b86dda3
07427cb
ea70a52
1dc05ff
dad4ff7
79e45d1
6b0288d
aeb49e2
b9beba9
b04c4b2
7c7edbd
74ec3e4
47fb837
89d431b
65741c1
a4bed8c
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.
Does this mean in the payload? Like if
iss
were to appear in the Verifiable Credential expressed asapplication/vc+ld+json
? Or does it mean ifiss
appears alongside avc
claim in the 1.1 style?I would think we'd want to avoid both cases in 2.0 and require these things to be in the protected header to keep layers separate for this external securing mechanism. Wouldn't getting key information from something in the payload also be incompatible with many existing JWT libraries?
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.
avoid both cases would mean banning the json member
iss
fromapplication/vc+ld+json
right?I don't think that would get consensus... we tried similar things with
proof
... which is still both allowed to be present or optional.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.
There's a clear (and I think, agreed upon) way to do the security in this particular case, right? We want people to move on from the VC JWT 1.1 stuff and get a clean separation? If so, it seems we might have consensus here to clearly direct people not to put
iss
directly into the Verifiable Credential, but instead, if they are going to use it, put it in the protected header.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, I think so, the authority is IETF for terms registered with IANA that are from IETF RFCs... They define this behavior we profile on top of it.
Yes, we made
typ
andcty
values that solved this.Disagree, we are not going to constrain the open world model... people WILL put
iss
wherever they are allowed to (which is everywhere)... We have to explain what doing that means (document existing usage).We won't get consensus to "ban terms in specific places", so it will happen in the payload and the header.
By default it should be interpreted by the security conventions from the relevant RFCs, that is what this PR does. That is why the language just mirrors the RFCs, and doesn't constrain further.
This PR does not change anything an implementer can do today, it acknowledges that there are thing they can do that have special meaning today (registered names in header and claimset), and reminds them to be aware of how those terms are used today.
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.
Does this mean
kid
could be in the payload? It's defined as a header parameter:https://datatracker.ietf.org/doc/html/rfc7515#section-4.1.4
Or is this just referring to
kid
appearing in the header andiss
in the payload? How should "also present" be interpreted 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.
Also, how is
kid
to beuseful to distinguish the specific key used
? This cries out for an example, or a fair amount of additional prose.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.
AFAIK,
kid
is legal in payload, in the same way thatbaz
oralg
is legal in payload.The sentence is not attempting to be specific about where
kid
can be found, butkid
does have special meaning when its in the protected header.can you make a concrete change suggestion?
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, I've made a suggestion to clarify that we're talking about
kid
being present in the protected header.