adding credential issuer logo parameter in the issuer metadata #170
Conversation
Sakurann
requested review from
jogu,
tlodderstedt,
peppelinux,
tplooker,
paulbastian and
awoie
| @@ -1342,6 +1342,9 @@ This specification defines the following Credential Issuer Metadata: | |||
| * `display`: OPTIONAL. Array of objects, where each object contains display properties of a Credential Issuer for a certain language. Below is a non-exhaustive list of valid parameters that MAY be included: | |||
| * `name`: OPTIONAL. String value of a display name for the Credential Issuer. | |||
| * `locale`: OPTIONAL. String value that identifies the language of this object represented as a language tag taken from values defined in BCP47 [@!RFC5646]. There MUST be only one object for each language identifier. | |||
| * `logo`: OPTIONAL. A JSON object with information about the logo of the Credential Issuer with a following non-exhaustive list of parameters that MAY be included: | |||
| * `uri`: OPTIONAL. String value that contains a URI where the Wallet can obtain a logo of the Credential from the Credential Issuer. Wallet needs to determine the scheme, since the URI value could use `https:` scheme, `data:` scheme, etc. | |||
There was a problem hiding this comment.
| * `uri`: OPTIONAL. String value that contains a URI where the Wallet can obtain a logo of the Credential from the Credential Issuer. Wallet needs to determine the scheme, since the URI value could use `https:` scheme, `data:` scheme, etc. | |
| * `uri`: REQUIRED when the `logo` object is present. String value that contains a URI where the Wallet can obtain a logo of the Credential from the Credential Issuer. Wallet needs to determine the scheme, since the URI value could use `https:` scheme, `data:` scheme, etc. |
If the logo object is present but doesn't contain any properties (like uri), it would indeed result in "logo": {}. This would be nonsensical because the logo object is supposed to contain at least the uri property when it is present. Therefore, it's important to specify that uri is required when logo is present to avoid such situations.
There was a problem hiding this comment.
we should discuss this. logo object is optional. so the issuer can avoid "logo": {} by not including logo parameter at all.
There was a problem hiding this comment.
I think it's also about setting clear expectations for what we expect issuers to do, as well as making it clear what situations we might end up needing to conformance test that wallets correctly parse. So I think it's good to make it clear that we don't expect issuers to use "logo": {} if that is our expectation.
It would probably be clearer if the logo object was defined in a new section. Then uri could I think just be marked as 'REQUIRED' and I think it would be clear that means "if the logo object is present".
There was a problem hiding this comment.
I don't think a separate section is necessary. I changed uri parameter to required. it made more sense from the expectations perspective and from the consistency with the rest of the spec perspective.
Co-authored-by: Joseph Heenan <[email protected]>
|
Jan-04-2024 WG call. agreed on "required" terminology. agreed to change uri to required for credential_configurations_supported too. no objections to merge once @peppelinux's approval is in |
| @@ -1343,6 +1343,9 @@ This specification defines the following Credential Issuer Metadata: | |||
| * `display`: OPTIONAL. Array of objects, where each object contains display properties of a Credential Issuer for a certain language. Below is a non-exhaustive list of valid parameters that MAY be included: | |||
| * `name`: OPTIONAL. String value of a display name for the Credential Issuer. | |||
| * `locale`: OPTIONAL. String value that identifies the language of this object represented as a language tag taken from values defined in BCP47 [@!RFC5646]. There MUST be only one object for each language identifier. | |||
| * `logo`: OPTIONAL. Object with information about the logo of the Credential Issuer with the following non-exhaustive list of parameters that MAY be included: | |||
| * `uri`: REQUIRED. String value that contains a URI where the Wallet can obtain a logo of the Credential Issuer. The Wallet needs to determine the scheme, since the URI value could use `https:` scheme, `data:` scheme, etc. | |||
There was a problem hiding this comment.
As I've raised in other forums before, I'm concerned at the shear number of options this definition permits, both in by value and by reference based resolution of the image based logo, it also allows an open ended number of image formats. Good standards make choices and this definition is avoiding making a choice which could meaningfully improve interoperability.
I also have concerns from a security perspective because many of these URI mechanisms can resolve to much more than just images and because we aren't limiting to certain image formats its likely implementations will leave themselves vulnerable in certain ways such as via RCE through mistakenly downloading and executing something like javascript. At an absolute minimum I'd like to see normative guidance which recommends implementations take precautions in the URI resolution process to improve the likelihood that they actually obtain an image as the result. That might include validating the mediatype of a data URI to check it is indicating its an image OR using headers when resolving an HTTPS based URI to request an image and validate a response.
There was a problem hiding this comment.
I'd suggest raising a separate issue about this, or perhaps two issues (one about the normative security note, one to discuss if we should limit the options) - I think the concerns apply to all logos, not just the one added in this PR.
There was a problem hiding this comment.
I agree with Tobias, wallets already have many responsibilities and options to implement, limiting the logo formats is definitely a good idea
Co-authored-by: Michael B. Jones <[email protected]>
|
@selfissued accepted. please re-review |
PR born out of this comment on PR #141 (comment)