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

Add normative requirements regarding media type and proof #1014

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions index.html
Original file line number Diff line number Diff line change
Expand Up @@ -5132,6 +5132,12 @@ <h2>The <code>application/credential+ld+json</code> Media Type</h2>
the use of a specific media type.
</p>

<p>
This media type MUST NOT be used to describe a verifiable credential with an <dfn>embedded proof</dfn>.
</p>
Comment on lines +5135 to +5137
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p>
This media type MUST NOT be used to describe a verifiable credential with an <dfn>embedded proof</dfn>.
</p>

@OR13,

My objection was marked resolved without addressing my concern: #1014 (comment)

I'm merging #1014 (comment)

based on the support I can see for it.

I see that some people support that statement. I object to it. We don't have consensus yet.

I currently think it's the wrong thing to do and I've asked for a technical basis for it and one or more clear concrete examples -- otherwise I think it should be removed.

Copy link
Contributor Author

@OR13 OR13 Feb 10, 2023

Choose a reason for hiding this comment

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

I see that some people support that statement. I object to it. We don't have consensus yet.

Agreed, but I raised this PR, and I can merge suggestions from WG members that I agree with.

You are welcome to raise a separate PR that implements the consensus you wish the WG to adopt.

Or request changes on this PR given it is heading a direction you do not agree with.

@dlongley I opened #1033 for your review, its like this PR but with your suggestion applied

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically speaking, a W3C verifiable credential is also a W3C credential. It does not make sense to me to say that you can't tag a VC with an embedded proof with the media type application/credential+ld+json.

I've heard an argument that to do this would be similar to making a mistake like JOSE's alg: none, but this argument is not technically clear. I've also argued that perhaps the opposite is true:

  1. If the alg: none argument is metaphorical, then I say that no one should depend on a media type to determine whether a credential is to be trusted. So if the statement banning using this media type with content with an embedded proof is meant to imply that a credential tagged with this media type can therefore be trusted -- that is bad security practice. To me, that bad practice would be metaphorically similar to alg: none.
  2. If the alg: none argument refers to a literal problem, then I say that alg: none itself should be prohibited regardless of anything we do here; therefore, this should be an orthogonal concern.

I've heard an argument that using application/credential+ld+json "tells an implementer something important and to treat it differently". But this offers nothing specific nor does it surface any particular choices as to how things would be different if someone did happen to tag content with an embedded proof with that media type.

In fact, as an implementer, I would not expect to change any behavior whether an embedded proof was present or not -- other than potentially having the option to try and verify such a credential. But, if my software never tries that anyway (which would have to be the case if the statement were accepted), I do not expect to start rejecting credentials just because they have embedded proofs on them in these circumstances. That seems harmful and like I would be missing out on processing otherwise useful credentials.

It also seems like there may be use cases that are made more complex (having to handle two different media types when it's otherwise unnecessary) if embedded proofs are disallowed. As an example, a use case that comes to mind involves signing VCs with multiple proofs via proof sets. The first time a proof is attached to the credential, a caller would need to use media type 1, and the second time, media type 2. But existing implementations today that attach such proofs do not need to care about this, so this adds unnecessary complexity. Given my experience with what developers tend to do in these situations, I'd expect some of them to just start ignoring the prohibition statement, leading to interoperability problems. So, this prohibition statement needs to have real value, IMO.

Now, if this statement is not present, we don't have to worry about any of that. If we're going to have such a restriction it should be clearly articulated why -- both the benefits and the drawbacks, and a technical discussion should proceed from that followed by arrival at consensus. Otherwise, it's not worth including it at the moment and doing so could be harmful, IMO.

Copy link
Contributor Author

@OR13 OR13 Feb 10, 2023

Choose a reason for hiding this comment

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

@dlongley can you please request changes so it is clear that this PR cannot be merged without addressing / discussing them?

To be clear, I don't intend to alter the PR, but I would like it to be clear to WG members that you are objecting to the proposed text they have suggested which I have merged here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dlongley can you please request changes so it is clear that this PR cannot be merged without addressing / discussing them?

You may have missed this -- I did that above as the first thing in this particular subthread: #1014 (review)

Copy link
Contributor

@decentralgabe decentralgabe Feb 10, 2023

Choose a reason for hiding this comment

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

@dlongley that is a suggestion. Under "add a review" there's a "request changes" button that is a clear "do not merge" signal

Screenshot 2023-02-10 at 3 32 21 PM

Copy link
Member

@msporny msporny Feb 11, 2023

Choose a reason for hiding this comment

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

@OR13 wrote:

@dlongley can you please request changes so it is clear that this PR cannot be merged without addressing / discussing them?

decentralgabe wrote:

@dlongley that is a suggestion. Under "add a review" there's a "request changes" button that is a clear "do not merge" signal

lols

image

@dlongley perhaps try firing a few flares into the sky? Waving a giant "I OBJECT" flag?

It's clear that "Request Changes" and repeatedly saying you don't agree with the PR and then having the conversation arbitrarily marked as resolved to "make reviews easier" has created some level of confusion about your position on the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean it's literally called "add a suggestion" - in every project I've worked on I haven't taken it to be a blocking request. only official reviews of which the options are [approve, comment, request changes]

Screenshot 2023-02-10 at 4 10 56 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

@OR13, @decentralgabe,

:) ... as I said above, that's the first thing I did here. To be clear, I submitted a review where I chose "Request Changes" and included specific suggested changes (as can be seen above). I do appreciate all of the screen shots of what I did though! :P

I think what you may not be expecting is that only code owners / required reviewers get the red change icon and big red merge blocking warnings. It doesn't matter how hard I clicked the "Request Changes" button (and I did), it only shows a gray icon and treats my objection as coming from a mere peon worthy of dismissal. :)

Copy link
Member

@msporny msporny Feb 11, 2023

Choose a reason for hiding this comment

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

it only shows a gray icon and treats my objection as coming from a mere peon worthy of dismissal

Editor hat on -- it's typically bad form for any Editor to dismiss change requests OR suggestions in a way that hides the fact that there continues to be disagreement on a particular line in a PR.

@OR13 was acting in good faith in an attempt to make the PR easier to read, however, in dismissing the change request and the resulting debate, no one else coming into this PR can see that there was clearly disagreement on a particular line.

Additionally, as Editors, it's our job to ensure that if we are not to take suggestions from WG Members (and the public at large) that we have the consent of the individual that raised the change request in the first place... because if we don't have consent, the PR will just be blocked upon merge with a request to roll back the PR (causing extra and unnecessary churn).

As a general rule, as an Editor, I won't merge a PR that has any pending suggestions (note, NOT change requests) that have not explicitly been closed/resolved by the individual that raised them OR where there isn't clear agreement that they're fine w/ their change suggestion or request not being merged.

When you resolve controversial change requests without the consent of the person that raised the change request, it almost always leads to further conflict in the PR.

We have been able to merge PRs on this spec for 9+ years w/o resorting to using the "Resolve conversation" button on threads containing disagreement.

That said, different Editors have different PR management styles... and the WG empowers the Editors to manage PRs in the way they feel will lead to reaching consensus of the group.

in every project I've worked on I haven't taken it to be a blocking request.

Open standards are not open source, incubation, or corporate projects, they're managed differently because the bar for reaching consensus is much higher at W3C (much to the dismay of a number of our WG members) :P.

Comment on lines +5135 to +5137
Copy link
Contributor

@mprorock mprorock Feb 28, 2023

Choose a reason for hiding this comment

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

Suggested change
<p>
This media type MUST NOT be used to describe a verifiable credential with an <dfn>embedded proof</dfn>.
</p>
<p>
If this media type is used any <dfn>embedded proof</dfn> MUST be ignored for the purposes of verification.
</p>

Copy link
Contributor

Choose a reason for hiding this comment

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

-1 to this. VCs and the three party model generally do not create a "control structure". Neither the issuer (nor this WG) get to tell a verifier what they can or cannot accept. I think media types are not the right way to accomplish the goals here.

If an API does not want to accept a particular field, it should say so / have a schema that rejects it, etc. Those are the right tools for that job.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dlongley are you ok with "should not" language for inclusion of an embedded proof?

Copy link
Member

@msporny msporny Mar 1, 2023

Choose a reason for hiding this comment

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

I'm wondering if we can flip the language around and say something like this:

If application/credential+ld+json is specified, verifiers MAY ignore top-level embedded proofs. Implementers are strongly urged to use a more specific media type, such as application/verifiable-credential+ld+json if the credential is secured using an embedded proof.

I'm offering the text above not because I think it should be in the spec, but to do a temperature check on compromise language that might work.

IOW, the statement above says: "Use this 'unsecured' media type and embedded proofs together at your peril... DO use a more specific media type for expressing that the content is secured."

That said, we will probably also need language that effectively states: "DO NOT just trust media types... that's an easily exploitable attack vector". If it says the media type is secured, you MUST run algorithm XYZ to ensure that it is.

Another way to look at this is when MUST an implementation throw an error. I can think of two instances where it definitely MUST throw an error:

  • application/verifiable-credential+ld+json is specified, but the VC contains no embedded proofs.
  • application/verifiable-credential+ld+json is specified, and it contains embedded chained proofs, where one of the proofs in the chain is missing/invalid.

... I'm sure there are other cases where we can all agree that an error MUST be thrown (at some layer).

To see if other media type registrations contained language like this, I started with application/jwt -- https://www.rfc-editor.org/rfc/rfc7519#section-10.3.1

It does not contain any language about what properties MUST/SHOULD or MUST NOT/SHOULD NOT be included in the media type registration... so, that got me wondering... are there other media types that have this sort of language?

Update: I just looked through the entire structured syntax suffix registry and found no language in each IANA registration section that prohibited certain content based on media type.

What inspired the language in this PR? Was it from another media type registration? If so, which one?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mprorock,

are you ok with "should not" language for inclusion of an embedded proof?

No, I'm afraid not. I don't think it's the right use of media types. IMO, the language is unnecessarily restrictive, ineffectual, and likely to create more confusion and unnecessary complexity as a result.

However, I am not against more specific media types (other than that I think we should have a strong reason for every new media type we create). Nor am I against recommending that more specific media types be used if there's a desire to communicate that some data meets some subclass constraints. But that just seems to be general media type advice that shouldn't need to be said in a particular media type registration.

@msporny,

IOW, the statement above says: "Use this 'unsecured' media type and embedded proofs together at your peril... DO use a more specific media type for expressing that the content is secured."

I think language like that is unnecessarily fearful. There's no reason to bring concepts like "use this as your own peril" into this. We don't need to create situations where people are afraid to consume certain data because of the media type that accompanies it. That further proliferates the wrong idea about media types, how to decide whether data is authentic (and what to do / not do about it), and how to work with data in the ecosystem generally, IMO.

In the three party model, there is no single party that tells other parties what to accept and then they just ... accept it. That's a core difference to some common two party models. There is no "Authorization Server" here. Instead, there is self-describing data that conforms to certain specs. Independent parties implement to those specs and when they see data that they understand -- they know how to consume it and what it means. If they don't understand it, they ignore it or reject it -- their choice. What they understand and are willing to accept is up to their business rules. They don't need to get together with other parties or have some special, elevated authorization party tell them what to do; that's just not how this technology works at all.

Copy link
Contributor

@mprorock mprorock Mar 1, 2023

Choose a reason for hiding this comment

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

@msporny This seems closer...

If application/credential+ld+json is specified, verifiers MAY ignore top-level embedded proofs. Implementers are strongly urged to use a more specific media type, such as application/verifiable-credential+ld+json if the credential is secured using an embedded proof.

How about this variation:

If application/credential+ld+json is specified, verifiers MAY ignore top-level embedded proofs. Implementers SHOULD use a more specific media type, such as application/verifiable-credential+ld+json if the credential is secured using an embedded proof.

To your point re the registration side - I think you are correct there, in that I don't think the registration block is the right place for this. I would like clear normative guidance about how to use the media types, and believe that the correct location for language to this effect does not need to be noted right inline below the table as it is done here, and probably could be located elsewhere in the spec.

edit: to add a notes on few things that get into multiple representations that then point out to external specs for normative and interop purposes such as csvm+json

If we are going to have any guidance in this section it should likely be in one (or aspects of the guidance in both of ) the Additional information and Interoperability considerations - there is some normative 'like' language in many docs, usually in the interop section, e.g. in H224, but normative language as to usage in this case is probably better located in a section detailing use of media types with VCs - this then could provide guidance for WG and other specs that utilize media types with VCs (like 'don't register everything, only what is required')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If an API does not want to accept a particular field, it should say so / have a schema that rejects it, etc. Those are the right tools for that job.

@dlongley

The v1 context says proof can be a member of the RDF type VerifiableCredential

Perhaps the mistake we have made was to define credential+ld+json instead of vc+ld+json... since the concept of what a "credential" is seems to not have strong agreement, I thought it was obvious that a credential is not a verifiable credential and would not have an embedded or external proof, after reading the v1 spec... but it seems that there is no agreement on the interpretation of the v1 spec text:

Screen Shot 2023-03-01 at 8 34 45 AM

https://w3c.github.io/vc-data-model/#proofs-signatures

What inspired the language in this PR? Was it from another media type registration? If so, which one?

@msporny confusion over what a "credential" is... and wanting to define a media type so that the bytes for a representation of a "credential" can be secured and referred to consistently... As I noted above, perhaps there is no concept of credential, there is only VerifiableCredential... Certainly this is the case regarding the RDF interpretation of the core data model.

In either case, this dialog is emphasizing a critical flaw in the spec... it is clear the current language is being interpreted in drastically different ways by readers.

A few paths forward.

  1. Remove proof from the core spec (rely on other specs, like data integrity to define it's presence in VCs, Capabilities, or other LD Objects)
  2. Register vc+ld+json instead of credential+ld+json, since proof can be present in vc+ld+json
  3. Remove the language from core spec about credential and just define verifiable credential

Copy link
Contributor

@andresuribe87 andresuribe87 Mar 1, 2023

Choose a reason for hiding this comment

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

Perhaps the mistake we have made was to define credential+ld+json instead of vc+ld+json

Strong +1 to this. It's the point I was trying to convey in #1044 (comment)

AFAIK there is no normative definition of what a Credential is. We only have a normative definition for a VerifiableCredential.

I am against 1, and in favor of doing 2 and 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not necessarily against what @andresuribe87 proposes (and the spirit of his comment) ... provided that we can make everything hang together.

Copy link
Contributor

Choose a reason for hiding this comment

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

I should also note that @andresuribe87's comment I think somewhat captures what we tried do to simplify things in the 1.1 work.

<p>
This media type MAY be used with <dfn>external proof</dfn>.
OR13 marked this conversation as resolved.
Show resolved Hide resolved
</p>
</section>


Expand Down