Skip to content
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

breaking change: .well-known/jwt-issuer renamed to .well-known/jwt-vc-issuer #118

Closed
wants to merge 4 commits into from

Conversation

peppelinux
Copy link
Contributor

@peppelinux peppelinux commented Jun 8, 2023

this PR resolves #101

more discussion is needed

@peppelinux peppelinux requested a review from awoie as a code owner June 8, 2023 16:19
@tlodderstedt
Copy link
Contributor

might be a stupid question: but given the latest change from VC-SD-JWT to SD-JWT VC, shouldn’t the metadata location name be jwt-vc-issuer instead of vc-jwt-issuer?

Copy link
Contributor

@tlodderstedt tlodderstedt left a comment

Choose a reason for hiding this comment

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

please also see my comment

draft-terbu-sd-jwt-vc.md Outdated Show resolved Hide resolved
@peppelinux peppelinux changed the title breaking change: .well-known/jwt-issuer renamed to .well-known/vc-jwt-issuer breaking change: .well-known/jwt-issuer renamed to .well-known/jwt-vc-issuer Jun 8, 2023
Copy link
Collaborator

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

I don't think this helps with readability

Copy link
Collaborator

@awoie awoie left a comment

Choose a reason for hiding this comment

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

lgtm

@awoie
Copy link
Collaborator

awoie commented Jun 21, 2023

Potentially fixes #101

@danielfett
Copy link
Member

I propose that we wait to resolve the discussion in #101 before merging this.

@awoie
Copy link
Collaborator

awoie commented Jun 22, 2023

Marked as DO NOT MERGE since we have to wait for the discussion in #101 to resolve first.

@awoie awoie added the discuss Discuss label Jun 22, 2023
configuration available at the path formed by concatenating the string
`/.well-known/jwt-issuer` to the `iss` claim value in the JWT. The `iss` MUST
`/.well-known/jwt-vc-issuer` to the `iss` claim value in the JWT. The `iss` MUST
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we intentionally breaking compatibility with oidc?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OIDC uses a different path, so I don't think it breaks compatibility with it. (this path is being defined in this spec, because we were surprised to find that a generic jwt-issuer .well-known path has not been defined, but at the same time maybe it was not defined because it is too generic and this path needs to be scoped to jwt-vcs, not sure yet)

Copy link
Contributor

Choose a reason for hiding this comment

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

@OR13 A SD-JWT VC is not an ID Token and a verifier does not know how a SD-JWT VC was issued. So I don't see the relationship to OIDC: Could you please explain what "breaking compatibility with OIDC" means?

configuration including `jwks_uri`:

```
{
"issuer":"https://example.com",
"jwks_uri":"https://jwt-issuer.example.org/my_public_keys.jwks"
"jwks_uri":"https://jwt-vc-issuer.example.org/my_public_keys.jwks"
Copy link
Collaborator

Choose a reason for hiding this comment

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

provide a full example of the dereferenced resource.... something like:

{
  "keys": [
  {
     "kid": "..."
  }
   ]
}

Copy link
Collaborator

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

This seems to break compatibility with both DIDs and OIDC.

@OR13
Copy link
Collaborator

OR13 commented Jun 26, 2023

I added a related PR here: w3c/vc-jose-cose#111

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

i do not think we have enough data points to make this decision, yet. this mechanism /.well-known/jwt-issuer/ is meant to be a generic mechanism, applicable beyond only VCs and I don't think it conflicts with other .well-known paths like in federation (partially because we do not have clarity around what other metadata to put under that path other thank JWK...)

Copy link

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

I would leave the name .well-known/jwt-issuer. This is a general-purpose capability usable by other kinds of JWTs. It just happens to being defined in this spec.

There's nothing wrong with that. In fact, specs often do that, to the benefit of others wanting to use these more-general-purpose facilities. They get put in IANA registries, from which their definitions are discoverable.

@peppelinux
Copy link
Contributor Author

@selfissued what about having jwt_issuer as a oidc federation metadata type?

If this well known resource Is a generic purpose metadata that every kind of entity can expose, how to check the protocol metadata It carries?

It might be and OpenID provider and a relying party and an RS or whatever, without a unique identifier It wouldnt be good for metadata but Just for Key attestation

So the difference between this and x509 would be just the data format.

Or, differently, in jwt-issuer there would space for multiple metadata with unique type identifiers like the federation EC?

@OR13
Copy link
Collaborator

OR13 commented Jun 30, 2023

Why not define, .well-known/jwks.json... doesn't seem like jwt or issuer are needed...

@peppelinux
Copy link
Contributor Author

Why not define, .well-known/jwks.json... doesn't seem like jwt or issuer are needed...

That's the same though I had, if It Is Just for Key attestation let's give to it the semantic It should

@peppelinux
Copy link
Contributor Author

Ok dears, may I change the endpoint name to .well-known/jwks.json in this PR and ask again your kindly revision?

@selfissued
Copy link

.well-known/jwt-issuer is a better name than .well-known/jwks.json. The latter implies that the only claim that will be in the metadata is the jwks_uri claim, which won't be the case for some use cases. Just like .well-known/openid-configuration and .well-known/oauth-authorization-server, yes, we'll have jwks_uri, but we also want the metadata to be extensible.

The existing name is good.

@peppelinux
Copy link
Contributor Author

peppelinux commented Jun 30, 2023

names are very often semantical or cosmetic and driven by personal tastes or objective meanings, that's just a name.

I would like to ask which are the scopes of this endpoint, if it is just for key attestation or also for metadata.
I assume the second. Since it is also for metadata, I assume here, how the metadata of a formless and abstract entity could be carried in that endpoint?

my answer is, by defining several metadata types, as already done in federation entity configuration

but this seems not clear, or out of scope for now, or let them think about this later on.
Well, without the big picture I find very difficult giving a name to an endpoint, since the name should represent its scopes and values

@tplooker
Copy link

Why not define, .well-known/jwks.json... doesn't seem like jwt or issuer are needed...

I like this more, having a well known location to associate a set of JWKs to seems generally useful, if you have additional metadata to communicate to parties that should be served from higher level well-known endpoints that are defined for that protocol / application.

@peppelinux
Copy link
Contributor Author

if the purpose of this endpoint is only for serving jwks, I think .well-known/jwk-set is a good choice

@TakahikoKawasaki
Copy link

Any path name would be fine (e.g., /.well-known/jwks.json, /.well-known/jwk-set, /.well-known/jwks, ...), but I think it's a good idea to assign a well-known path to the endpoint that returns the JWK Set document.

Defining such a well-known path will not prevent the definition of /.well-known/jwt-issuer. Both paths can coexist. The discussion for /.well-known/jwt-issuer can be revisisted in the future if the need arises.

@OR13
Copy link
Collaborator

OR13 commented Jul 10, 2023

You could remove the content type from the resource identifier, and use the accept header.

GET /.well-known/keys

--accept application/jwk-set+json
--accept application/cose-key-set

@peppelinux
Copy link
Contributor Author

You could remove the content type from the resource identifier, and use the accept header.

GET /.well-known/keys

--accept application/jwk-set+json --accept application/cose-key-set

I love this, however I'd prefer two separate endpoints for two different format, since without the definition of any content-type in the request would return a default value, then: Jose or cose Key set?

@OR13
Copy link
Collaborator

OR13 commented Jul 10, 2023

Server reserves the right to set the content type header or throw a content type unsupported error, but you can also probably say jwk-set+json is the default when requesting the endpoint be reserved....

openid-configuration is known to be application/json, and didn't reserve

/.well-known/openid-configuration.json

@peppelinux
Copy link
Contributor Author

overseeded by #189

@peppelinux peppelinux closed this Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants