-
Notifications
You must be signed in to change notification settings - Fork 6
DIIP v5 #84
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
base: develop
Are you sure you want to change the base?
DIIP v5 #84
Conversation
…ft 08 to draft 13 and IETF Token Status List from draft 10 to draft 13
|
🚀 Preview deployment successful! 📄 Preview URL: https://FIDEScommunity.github.io/DIIP/pr/pr-84 This preview will be updated automatically when you push new commits to this PR.
|
spec/federation-profile.md
Outdated
|
|
||
| **Requirement: The Credential Issuer MUST use the value of the `credential_issuer` in its OID4VCI issuer metadata as its Entity Identifier.** | ||
|
|
||
| The Credential Issuer's Entity Configuration can be found by appending the string `/.well-known/openid-federation` to the Entity Identifier. |
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.
Shouldn't we point towards the method(s) listed under https://openid.net/specs/openid-federation-1_0.html#name-obtaining-federation-entity ? It is part of openid-federation, and adding this here could potentially deviate and/or confuse people
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.
Fixed in b3d496d
spec/federation-profile.md
Outdated
|
|
||
| The Credential Issuer MAY place additional metadata into the `federation_entity` Entity Type Identifier. | ||
|
|
||
| **Requirement: The metadata in the `openid_credential_issuer` property MUST override the metadata in the `federation_entity` property.** For example, if both `openid_credential_issuer.display.name` and `federation_entity.organization_name` exist, the Wallet MUST show the value of `openid_credential_issuer.display.name` as the name of 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.
I'm still not sure what this is supposed to mean. organization_name and display.name have different semantic meanings according to the specification (https://openid.net/specs/openid-federation-1_0.html#name-informational-metadata-exte and https://openid.net/specs/openid-4-verifiable-credential-issuance-1_0.html#section-12.2.4-2.10.2.1).
In VCI we have for display.name
String value of a display name for the Credential Issuer.
In oidf organization_name is defined as:
A human-readable name representing the organization owning this Entity. If the owner is a physical person, this MAY be, for example, the person's name. Note that this information will be publicly available
It is even more confusing, as oidf does have a semantically similar claim display_name:
A human-readable name of the Entity to be presented to the End-User.
If we want to have either organization_name or display.name then it should not be for example but explicitly define that this particular property will be overwritten.
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 agree that we should remove the optionality and specify how all implementations should work.
(When I originally wrote the proposal, I was leaning towards just specifying priority/precedence rules for data coming from various sources. When the wallet needs to show the user "Org X is requesting information from your wallet", it could fetch the name of the requesting organization from any of the attributes listed above – even if they have different definitions.)
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.
Should we just require Issuers and Verifiers to populate display_name as specified in https://openid.net/specs/openid-federation-1_0.html#name-informational-metadata-exte amd Wallets to show that to the users?
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.
Or is it simpler to use openid_credential_issuer.display.name since that's what's being used if there is no OpenID Federation involved?
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.
OID4VCI metadata supports localization, but there seems to be no similar metadata for the Verifiers...
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.
Decided to use *federation_entity.display_name` in b3d496d.
spec/federation-profile.md
Outdated
|
|
||
| **Requirement: The Credential Issuer MUST place the OpenID4VCI issuer metadata into the Entity Configuration, in the `openid_credential_issuer` property.** | ||
|
|
||
| **Requirement: The Credential Issuer MUST place the public key material of the keys it uses to sign Digital Credentials in the `jwt_vc_issuer` property. The `jwt_vc_issuer` property MUST include the `jwks` property that contains the Issuer's JSON Web Key Set as defined in RFC7517.** (The `jwt_vc_issuer` property MUST NOT include the `jwks_uri` property.) |
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.
Suggest to call this vc_issuer and not jwt_vc_issuer.
See comment here: #79 (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.
And @samuelmr I think we should actually depend on the JWKS extensions from OpenID Federation instead of leaning on SD-JWT spec. And since we only allow jwks (for now), we can just mention that only jwks should be used for DIIP-compliant federations. But that doesn't mean an entity cannot use others. E.g. if you look at the federation spec:
It is RECOMMENDED that an Entity Configuration use only one of jwks, jwks_uri, and signed_jwks_uri in its OpenID Connect or OAuth 2.0 metadata. However, there may be circumstances in which it is desirable to use multiple JWK Set representations, such as when an Entity is in multiple federations and the federations have different policies about the JWK Set representation to be used. Also note that some implementations might not understand all these representations. For instance, while jwks_uri will certainly be understood in OpenID Connect OP metadata, signed_jwks_uri might not be understood by all OpenID Connect implementations, and so a JWK Set representation that is understood also needs to be present.
TBH we don't really use anything from the JWT VC Issuer of SD-JWT spec, since the resolving is handled by Federation, so we just leave the JWKS document, which is fairly common.
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 my suggestion:
- Do not lean on JWT VC Issuer from SD-JWT VC spec, but rather on the existing JWKs feature in OpenID Federation. There's a section on common metadata parameters and are perfect for what we're trying to do: https://openid.net/specs/openid-federation-1_0.html#name-extensions-for-jwk-sets-in-
- Optionally limit the common JWK parameters to only
jwks(so MUST be present) to simplify implementations. But not disallow the presence of other fields (to allow an entity to be part of multiple federations) - Rename the
jwt_vc_issuertovc_issuerto make it more generic (even though we define this profile for DIIP, I think we should think about application to non-DIIP credential formats and this is a very simple change).
cc @nklomp
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.
Fixed in b3d496d as suggested.
|
@TimoGlastra, @eklaver, @k00ij, @nklomp: can you review the latest version so that we can publish it on Jan 15th as planned? |
|
Would it be possible that we publish DIIP on Jan 15 as a 'release candidate', and we give e.g one or 2 months of time to implement and gather feedback and make adjustments based on that. Since no one already has this implemented on day one, I think giving a window to iterate on the document makes sense, and it gives implementers a heads up that things may change if they were incorrect. It's not intended to be used to push new features |
Thanks for the suggestion. I'd be interested to hear what others think. Remember that there are no sanctions on anyone if they are not compliant on day one. The main requirements (especially, OID4VC 1.0, also status lists and SD-JWT VC version updates) have been public since the end of October (#78). All vendors should have already had plenty of time to implement those. In the last DIIP meetup, we decided to make the OpenID Federation requirements optional because the specification for them was written so late that nobody would have had time to implement them by January 15th. |
|
Thanks for the update! I will review it today. I agree with Timo that during implementation the real questions and discussions come up, but on the other hand I don't mind just releasing the new version. Updates can always be done in the next version. |
selfissued
left a 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 suggest referencing the OpenID Federation Wallet Architectures specification for the definitions from it that this specification is using, such as openid_wallet_provider, openid_credential_issuer, and openid_credential_verifier.
Thanks for the suggestion! This specification doesn't use the definition One could argue that this specification and the OpenID Federation Wallet Architectures specification are conflicting since both define the same Entity Types Another argument is that since both specifications use the Entity Type definitions similarly, it would be beneficial for one spec to refer to the other. (I believe this is the argument you're making.) Currently, the chapters 6.2, 6.3, and 7.1 of OpenID Federation Wallet Architectures are quite brief. Referring to that specification as a normative definition for the abovementioned Entity Types would not make this specification easier to understand or maintain. It would make it somewhat more difficult for implementers to look at another spec instead of having everything in one place. It would also introduce a risk that the OpenID Federation Wallet Architectures specification changes the requirements for the metadata of these Entity Types in a way that is no longer aligned with this spec. Would you be happy with a non-normative reference? (To give credit where it's due.) |
|
I think the chances of anyone deciding to change the Entity Type Identifers As long as we're talking about references, you might also update the OpenID Federation 1.0 reference either to draft 46 or omit the draft number, since it's essentially done. |
|
I must apologies if I didn't get which kind of conflict we may have between the two specification. Federation Wallet has born before and I suppose that DIIP federation was inspired by federation wallet if we may individuate the extensions that DIIP proposes we may converge in a single openid specification. I would like to get them summarized (with reference to DIIP specs) to facilitate their developments |
This PR updates the OID4VC, SD-JWT VC, and Token Status List versions.
It also adds OPTIONAL requirements for issuer and verifier agents to support OpenID Federation (publish Entity Configuration files).
This version includes a draft of OpenID Federation Digital Credentials Profile – likely to be carved out as a separate specification in the future.