openid / OpenID4VCI

60 stars 16 forks source link

Inline credential offer lacks required metadata #77

Closed TimoGlastra closed 8 months ago

TimoGlastra commented 9 months ago

When using inline credential offers (so using an object directly in the credentials array in an offer), the only required general property is the format. All other fields are declared by the format.

The credentials_supported from the issuer metadata however has a lot more properties that are supported by default. Most notably: cryptographic_binding_methods_supported, cryptographic_suites_supported and display.

I think this makes it very hard to use inline credential offers, as you have to guess which binding methods and cryptosuites the issuer supports.

How I'd first understood the different between referenced offers (with id) and inline is that inline offers may be used for one-off credentials, or credentials you don't want displayed in the public issuer metadata.

But there's a certain limitation to using the inline format, which I don't necessarily understand.

So my questions are:

nklomp commented 9 months ago

What you see is that in a lot of cases the credential offer types and format are mapped onto the supported metadata (and the examples also imply that). We also do that, but IMO it should indeed be the one or the other. Or you provide an id and map that onto credentials supported (scopes in latest dev draft version), or you provide inline objects, but then allow for everything in there. The problem with current approach is you cannot map for instance two OpenBadge Credentials with the same format. Let's say I want to offer a slightly different version of a credential, but with the same type and format. Obviously I can have multiple definitions in the metadata and assign IDs/scopes to them. That also works with using ID/scopes in the offer. It would fail with the examples and how both we and Mattr have implemented it, as the types and format will never definitively identify which credential we would be talking about.

IMO scopes should just be made mandatory in the definition. Use scope strings in the offer, or have an inline credential definition, but then allow for everything in there (which means you will have to resort to a URI because of the size)

TimoGlastra commented 9 months ago

IMO scopes should just be made mandatory in the definition. Use scope strings in the offer, or have an inline credential definition, but then allow for everything in there (which means you will have to resort to a URI because of the size)

Yes, 100% agreed with this. I think you put it more clearly than me 😄

TimoGlastra commented 9 months ago

What you see is that in a lot of cases the credential offer types and format are mapped onto the supported metadata (and the examples also imply that)

I wasn't aware of this, and find that quite strange. This is exactly what the purpose of the id mapping is right?

nklomp commented 9 months ago

Yes and no. The id value and its mapping is gone in the latest dev spec. It has been replaced by scopes. But scopes can also span multiple definitions. Which then means you could not identify a single definition anymore. I do see the value of using scopes for multiple credentials, but at the same time I want full flexibility. Similar credential types/formats handled differently by the issuer, should be uniquely identifiable in metadata and thus also in the credential offer. Mapping types/formats from the offer onto the metadata is just prone to errors. Especially if a VC has multiple contexts for instance

paulbastian commented 9 months ago

I was about to open an issue on the same/similar topic, but I'm not quite sure I follow your point. Wouldn't it be easier to just refer to the id value from the metadata in the credential offer credentials? That makes it very easy... This would also be unified solution for all credential types, if I'm not mistaken

TimoGlastra commented 9 months ago

Wouldn't it be easier to just refer to the id value from the metadata in the credential offer credentials?

I think @nklomp is on the same side here. As I understand it he's also arguing to either refer using the id, or use an inline offer that supports all metadata properties as supported by the issuer metadata credentials supported:

Mapping types/formats from the offer onto the metadata is just prone to errors

I think we're all on the same page here! :)

nklomp commented 9 months ago

Yes, I am on the same page. I was merely trying to say that in the latest development version, there is no notion of an id anymore. It is replaced by scopes. A single scope can also be assigned to multiple credential types and format. So it is similar in a sense but also different. Since the scope is defined in metadata, how would I have a single credential type and format that can be part of a dedicated scope for this credential, but also part of a scope together with other credentials? Right now, that is not possible AFAIK as the scope is a singular string in credentials supported in the metadata.

So you could have individually identified credential types, combined credential formats and/or types, but not both at the same time. Let's say I have 2 credential types in 2 formats. Sometimes, I want to issue them together and let the user determine whether they want jwt or jsonld, then I could model that in the server metadata and use the appropriate scope parameter in the offer. However I am not able to create an offer for one of these credentials for instance, only for a jwt format.

Sakurann commented 9 months ago

would it be better to revert to a previous option where a string passed in the credential offer's credentials object is identifier, instead of scopes?

TimoGlastra commented 9 months ago

Maybe, but that's another topic I think. I opened this issue specifically targeted at inline credential offers, not the reference to the issuer metadata. It feels a bit weird that you can't indicate at all with the inline offers the binding methods/crypto graphic suites/display metadata.

How is that supposed to work when I'm using inline offers, do these values need to be negotiated upfront/out-of-band? I would like to see a way in the spec to do this. So that credentials_supported becomes publicly available metadata for everyone to resolve, where inline offers are used for more custom credentials / credentials you don't want in your public metadata. Sure you could also add them to the credentials_supported, but then I don't understand what the purpose of the inline offers is.

paulbastian commented 9 months ago

@TimoGlastra what are you referring to with inline offers? That's not a term in the spec?

In my opinion, it is bad to duplicate data and the credential offer should be rather small. Additionally we should aim to limit too much optionality to simplify implementations.

Therefore the credential offer should only refer to the identifier from the metadata. Or am I missing something here?

nklomp commented 9 months ago

Then the whole object vs string for an offer wouldn't make any sense. Simply use ids/scopes and be done with it. Also ensures the QR/link if used by value is smaller.

@Sakurann I do like the fact that scopes can include multiple credentials. But yes IMO using ids makes for an easy implementation that probably will serve a lot of use cases

Sakurann commented 9 months ago

@paulbastian I think what is referred to as inline offer is using format/doctype to specify the credential being offered in the example below from the spec.

   "credentials": [
      "UniversityDegree_JWT",
      {
         "format": "mso_mdoc",
         "doctype": "org.iso.18013.5.1.mDL"
      }
   ]

I think what I am learning from this discussion is that it would be beneficial to change the current offer structure to have the following two options (@TimoGlastra I understand the original issue was about inline offer but I think suggested change also has impact on the credentials param structure itself):

please let me know if I misunderstood/mischaracterized the discussion. Our implementation would also be supportive of this approach because we are learning that it is more useful to put identifiers value in the credential offer than scopes value.

so the updated credential offer will look as following:

   "credentials": [
      "university_degree_jwt_computer_science", // matches with `c_identifier` value in the `credentials_supported` metadata entry
      {
        // entire `credentials_supported` entry for that credential that cannot be found from the issuer metadata
          "format": "mso_mdoc",
          "doctype": "org.iso.18013.5.1.mDL",
          "cryptographic_binding_methods_supported": [
              "mso"
          ],
          "cryptographic_suites_supported": [
              "ES256", "ES384", "ES512"
          ],
          "display": [
              {
                  "name": "Mobile Driving License",
                  "locale": "en-US",
                  "logo": {
                      "url": "https://examplestate.com/public/mdl.png",
                      "alt_text": "a square figure of a mobile driving license"
                  },
                  "background_color": "#12107c",
                  "text_color": "#FFFFFF"
              },
              {
                  "name": "在籍証明書",
                  "locale": "ja-JP",
                  "logo": {
                      "url": "https://examplestate.com/public/mdl.png",
                      "alt_text": "大学のロゴ"
                  },
                  "background_color": "#12107c",
                  "text_color": "#FFFFFF"
              }
          ],
          "claims": {
              "org.iso.18013.5.1": {
                  "given_name": {
                      "display": [
                          {
                              "name": "Given Name",
                              "locale": "en-US"
                          },
                          {
                              "name": "名前",
                              "locale": "ja-JP"
                          }
                      ]
                  },
                  "family_name": {
                      "display": [
                          {
                              "name": "Surname",
                              "locale": "en-US"
                          }
                      ]
                  },
                  "birth_date": {}
              },
              "org.iso.18013.5.1.aamva": {
                  "organ_donor": {}
              }
          }
      }
   ]

cc @pmhsfelix

TimoGlastra commented 9 months ago

I think what is referred to as inline offer is using format/doctype to specify the credential being offered in the example below from the spec.

Yes indeed that's what I mean with inline offer. I couldn't find a clear name for it in the spec, so this is how I've started to call it.

(@TimoGlastra I understand the original issue was about inline offer but I think suggested change also has impact on the credentials param structure itself):

The suggestion you listed in the two bullet points @Sakurann sounds like a good solution to me, and would solve my issues. It would also simplify the spec a bit, as the structure for the issuer metadata credentials_supported and the object entries in the credentials array from the offer (inline offers) would be the same, and thus less complexity when adding new formats.

paulbastian commented 9 months ago

Yes, your proposal matches my suggestion and I'm in favour of the first option.

What would be the advantage of the second option?

nklomp commented 9 months ago

@Sakurann Indeed that would be the 2 options and I agree it makes sense to have the inline version be signed. On the other hand, these will become so big that they will have to be done by reference anyway and thus should be hosted. Probably makes sense to require same origin for that

Sakurann commented 9 months ago

What would be the advantage of the second option?

@paulbastian, from what I understand "when the issuer wants to offer the issuance of one-off credentials, or credentials it does not want displayed in the public issuer metadata."

TimoGlastra commented 9 months ago

from what I understand "when the issuer wants to offer the issuance of one-off credentials, or credentials it does not want displayed in the public issuer metadata."

Yes that'd be the main case I see for it. TBH I don't have any actual requirement for this feature. I think my initial comment was that in it's current state i don't think the inline offers provide a lot of value. As it's lacking a lot of (needed) metadata compared to the issuer metadata credentials supported.

So I'd either vote to remove them completely (the inline offers, objects in the credentials property from offers), or to 'upgrade' them and make them actually useful (and make it match the structure of credentials_supported).

Sakurann commented 9 months ago

I would be in favor of removing inline offer completely for now. unless @tlodderstedt you could remember an appealing reason why it needs to be kept..

I started to think whether inline offer needs to be signed and ended up ratholing... what is the attack being prevented by signing inline offer? the mismatch between the content of an issued credential and the display information to trick user consent? attacker issuing victim issuer's credential? attacker tricking the victim issuer to issue attacker's credential? bottomline really not sure whether the complexity that signing brings is worth the benefit. (in general when we discussed formal security analysis of OID4VC at oauth security workshop, the learning was that signing the credential offer does not bring any security benefit (cc @fabian-hk @danielfett). not sure this changes.)

also if inline offer is being hosted because of the size, I am really not sure it fulfills the benefits of "issuance of one-off credentials, or credentials it does not want displayed in the public issuer metadata."

Sakurann commented 9 months ago

also the question would arise how this identifier passed in the credential offer is being used throughout the protocol (PR #65, originally raised by Torsten in this comment).

my current thinking is:

The rest is as already defined. if the AS returns identifier from the token endpoint, wallet includes it in the credential request.

tlodderstedt commented 9 months ago

@Sakurann not sure how this is related to the inline or by reference metadata discussion in this issue. It seems from your standpoint the identifier and the scope value are the same. In my mental model, a scope value would represent a certain type of credential while an identifier (as described in PR 65) refers to a certain instance of such a type. In this mental model, a scope value would not be a equivalent representation of the credential being request thus the identifier would need to go into the authz request with the scope value (or without a scope as the identifier will most likely be enough from the AS perspective to determine the credential type, too).

nklomp commented 9 months ago

Does it make sense to require an id on the credentials_supported then on the offer, either have a ids or scopes included, completely dropping current object with types etc?

pmhsfelix commented 9 months ago

IMO, there are (at least) two distinct questions being discussed here:

1) One of them (which seems the original question of the issue) is - should the spec allow for complete dynamic offers that do not use any information from a static metadata credentials_ supported entry. That is, offers that include all the information required by a wallet to do a complete flow (authorization request + token request + credential request). The metadata would only be used to discover the authorization server and the credential endpoint URL. I've created an distinct issue for us to have this discussion, following the 2023-09-29 DCP call - https://github.com/openid/OpenID4VCI/issues/82

2) If an offer requires information from a credentials_supported entry in order to be usable, then how does the wallet finds out which entry to use. Note that the wallet will need things such as cryptographic_binding_methods_supported (obtained from the credentials_supported entry) in order to perform a credential request.

Sakurann commented 9 months ago

I opened a new issue #83 focusing on the identifiers in the Credential Offer. the connection between identifiers and inline credential offer in this issue seemed clear to me, but looks like it was only to me :)

not sure how this is related to the inline or by reference metadata discussion in this issue.

I already opened a separate issue, but to elaborate a little why I thought identifiers are relevant (which might have been wrong, but I did that in good faith, not trying to divert the discussion...) right now, there is an option for the issuer to pass format/type by value in an object in the request, if that option is removed, using a string value becomes the only option to identify a credential in the offer; if that option is expanded, the question is why pass the copy of what is already in the issuer metadata in the offer, and if the value of that is to offer a credential not expressed in the metadata, than the string value becomes the only link to the metadata. and since that identifier is currently a scope value, that becomes limiting. Please see the proposal in this issuer comment

It seems from your standpoint the identifier and the scope value are the same.

I elaborated in #83, but they are not. I don't think 1:1 mapping between identifier and scope can be assumed and I am not sure where I implied that. It is totally possible for the issuer to have same scope value for multiple credential types, one scope value for each type, etc. while identifier can point to the credentials of the same type but different display property. so I think it is totally valid for the issuer to have multiple identifier values for the same scope.

Sakurann commented 9 months ago

agreed at pre-IIW DCP WG:

https://docs.google.com/document/d/1q_vcI7PGJLtM3j0jvpLfmiBsj69jCilTzDQCjKRT2xY/edit

Sakurann commented 8 months ago

PR merged