Skip to content

Update controller document reference #99

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

Closed
wants to merge 2 commits into from

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Jun 11, 2023

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

This text aligns with https://w3c.github.io/did-core/#dfn-publickeyjwk , which we came to after considerable debate. If we want to change this text, that will make the Data Integrity definition deviate from the one in DID Core.

-1 for making the definitions mis-matched unless there is a really good reason to do so. What is the reason that this text is being proposed, which overrides what the DID WG decided the text should be?

Comment on lines +960 to +961
The `publicKeyJwk` property is RECOMMENDED. If present, the value MUST
conform to the <a data-cite="RFC7517#section-4">JSON Web Key (JWK) Format</a>.
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 show preference for JWKs over other formats. If we want to make advice here on not exposing private key information to be more concise that could be ok if we also link to the DID core spec. I also don't think we should lose the advice for fragment identifiers.

In short, I think this should mirror what we did with the DID core spec, but extend it to other documents -- which is what I think the current spec already does.

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

As @dlongley said.

@iherman
Copy link
Member

iherman commented Jun 14, 2023

The issue was discussed in a meeting on 2023-06-14

  • no resolutions were taken
View the transcript

1.5. Update controller document reference (pr vc-data-integrity#99)

See github pull request vc-data-integrity#99.

Manu Sporny: data integrity has 1 pr on it.
… manu we think did core will point to data integrity, there are changes requested.
… there are no prs on vc-di-eddsa.

@OR13
Copy link
Contributor Author

OR13 commented Jun 27, 2023

https://lists.w3.org/Archives/Public/public-vc-wg/2023Jun/0026.html

This is why we should recommend JWK over multibase.

Copy link

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

it's helpful to implementers to have clear guidance like this

@OR13
Copy link
Contributor Author

OR13 commented Jul 12, 2023

Listing 2 key representations, making them both optional and defining a key resolution protocol in a data model document, seems to be not a great way to ensure interoperability.

Instead, we should define a RECOMMENDED way to obtain key material and RECOMMENDED representation for the material, to discourage people from implementing support for every possible key format that might ever exist.... which will not help issuers secure claims about subjects in way that verifiers will be able to consistently understand.

@iherman
Copy link
Member

iherman commented Jul 12, 2023

The issue was discussed in a meeting on 2023-07-12

  • no resolutions were taken
View the transcript

4.3. Update controller document reference (pr vc-data-integrity#99)

See github pull request vc-data-integrity#99.

mnau: there is a stuck PR, 99...

Manu Sporny: need to discuss PR 99 probably.
… we are adding new editors to the specs.
… waiting on invited expert status for those editors.

Ivan Herman: the invitation has been received, but he needs to review documents before possibly signing.

@iherman
Copy link
Member

iherman commented Jul 20, 2023

The issue was discussed in a meeting on 2023-07-18

  • no resolutions were taken
View the transcript

1.2. Update controller document reference (pr vc-data-integrity#99)

See github pull request vc-data-integrity#99.

Brent Zundel: feel like we have a possible path forward.
… Orie, please give an overview of the pr.

Orie Steele: DI has some excellent stuff that is not specific to data integrity, notably controller descriptions especially of did docs.
snark about did working group.
… the did working group did not define key representations or shapes.
… as a result of that almost all did methods have undefined terms.
… this PR addresses one of the predicates and recommends public key JWK as a path.
… there is dispute over the best way of representing keys in json-ld.
… the counter proposal appears to be do the same as did working group and don't specify.

Manu Sporny: think that the group has been clear that we should not prefer one approach or format over another.
… this PR prefers one key format over others, and feels inappropriate.

Orie Steele: key material can secure both proof formats.
Dave Longley: +1 there's no consensus to prefer one key material over the other ... people want different things.
Orie Steele: preferring key formats would help all securing mechanisms.
Dave Longley: +1 let's not spend time on that disagreement again.

Manu Sporny: the other issue is that the did wg did spend a lot of time trying to improve mistakes around publishing key material, and this pr removes some guidance that could permit an implementer to publish private key material.

Orie Steele: its easier to publish multikey material with mistakes... since multikey is not a defined key format...

Manu Sporny: we should have the ability to ensure that errors are thrown when errors or leakage is detected.
… do agree that we should make things better, but don't think this pr makes things better.
… and think it removes sec guidance.

Michael Prorock: I think Manu's speaking to an important point, especially because we're talking about Data Integrity and this spec and security data in general, while I have approved this PR and think that this is a good path forward, I do agree that we should also explicitly call out items and not remove items that deal with accidents we know happen in the wild. Specifically, inclusion of 'd' in a public key, just grep Github.
… There are things we should be careful about, we know people do bad things, we want to add back in some of these language that provides security guidance.

Brent Zundel: is there a path forward for this PR and are there next steps?

Orie Steele: agree with comments regarding avoiding preference - might consider applying those comments more generally.
… for this PR is the sentence regarding encoding of JWK good enough, or should we carry over items from did wg.
… if text comes in as "may" is that actually an improvement.
… concerned regarding standardization path for other formats.
… potentially this pr should have been raised in the core data model.

Manu Sporny: we should be specific in security guidance. the text in red that is being removed is good text and stuff that should be said.
… this pr is not specific enough about what private info accidentally is leaked.
… the kid value items are important, and should be aligned with vc-jose-cose.
… should take a look at fingerprinting and compound key identifiers.
… agree that we should make this better, but deleting this stuff is not the way.

Orie Steele: Sometimes saying less, is "making things better".

Manu Sporny: disagreement is on whether or not this PR makes improvements.
… and over preference between multibase and jwk.
… ... but not in this case. :).

Michael Prorock: There are shades of what turned out to be a painful PR, which was the context integrity related items that came up initially in the JOSE/COSE spec, initially. We said yes, big enough issue to open issue on vc data model side... this feels a bit like that to me, particularly when we're talking about most of this, ZKPs, basically we can refer to what we're doing w/ VCs as dealing with public key crypto... so if that's the case, we should pull top level guidance into security considerations ... take areas that are deleted, put them into core data model explicitly and PR on data integrity that points to that.
… I have similar concerns around multibase, but to me, there are other things in this PR that do merit a bigger look... next steps, pull this stuff up to security guidance and reference it clearly in VC COSE JOSE and then we're setting model on how to do this stuff right, especially when we know there are issues, good thing to do.
… While my preference is to rely on JWK, but rest of WG has to come to consensus on how to talk about that, fine for certain users and not ok for others.

Dave Longley: general comment - if we have 3 diff approaches, we should give the best advice we can about each approach.
… we should be as even handed as possible, and that doesn't mean that we will say the same number of things about each item.

Orie Steele: Our did:key implementation supports both key formats, https://did.key.transmute.industries/ speaking as an implementer... I consider this a huge waste of time, now... given did:jwk is simpler and supports x509 extensions... I think recommending experimental stuff is generally a bad idea.
multikey does not even support expressing private keys for NIST curves.... thats how safe it is.
Manu Sporny: Orie, not true: https://github.com/multiformats/multicodec/blob/master/table.csv#L171.

Manu Sporny: support for good security guidance should stay in, should get better, and we should see if we can get it in the core spec.
… that may be difficult, because then we are talking about securing methods in the core data model, but is worthwhile.
… we should make sure that errors are thrown always when things like private key material is encountered when it shouldn't be.

Brent Zundel: look forward to seeing where this goes and i will be looking for an issue on the main spec.

Copy link
Contributor

@dmitrizagidulin dmitrizagidulin left a comment

Choose a reason for hiding this comment

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

  • 👍 to pointing to the https://www.iana.org/assignments/jose#web-key-parameters registry (though it still wouldn't hurt to provide an example private parameter like d).
  • -1 to removing the recommendation to use public key fingerprint as kid. That is an extremely useful piece of implementer guidance.

I also feel uneasy to RECOMMEND a key representation in this top level spec.

@OR13
Copy link
Contributor Author

OR13 commented Aug 4, 2023

@dmitrizagidulin If I understand your feedback, you want us to include an example where d is used incorrectly, with a comment saying, do not do this?

Do you plan similar guidance on using the multicodec representation for fragments on publicKeyMultibase related verification methods, if that is the plan, then I agree it makes sense to keep the guidance on kid.

@OR13
Copy link
Contributor Author

OR13 commented Aug 4, 2023

I also feel uneasy to RECOMMEND a key representation in this top level spec.

I think the current language misleads implementers regarding key representation maturity and standardization.

I feel similarly about adopting multikey in IETF... given COSE Key and JWK already exist and are good enough, for communicating keys in JSON and CBOR.

I guess we'll continue this debate there when the charter goes up for review.

@OR13
Copy link
Contributor Author

OR13 commented Aug 4, 2023

See comment here: #135 (comment)

Regarding the unsafe guidance we are currently giving regarding private keys and RSA.

@dmitrizagidulin
Copy link
Contributor

@dmitrizagidulin If I understand your feedback, you want us to include an example where d is used incorrectly, with a comment saying, do not do this?

Not quite, but I don't feel strongly about this, I agree with you elsewhere that we should specifically say parameters not properties etc.

Do you plan similar guidance on using the multicodec representation for fragments on publicKeyMultibase related verification methods, if that is the plan, then I agree it makes sense to keep the guidance on kid.

I would like to add similar guidance for multicodec, yeah. Advice to developers on how to form key fragments is super useful.

@OR13
Copy link
Contributor Author

OR13 commented Aug 8, 2023

@dmitrizagidulin propose some text, and I will update this PR to address both cases consistently.

Copy link
Contributor

@peacekeeper peacekeeper 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 it's helpful to deviate from DID Core, which (besides complexities around different representations) has been working quite well so far.

@iherman
Copy link
Member

iherman commented Aug 23, 2023

The issue was discussed in a meeting on 2023-08-23

  • no resolutions were taken
View the transcript

1.1. Update controller document reference (pr vc-data-integrity#99)

See github pull request vc-data-integrity#99.

Manu Sporny: Orie raised the PR; it has had a number of reviews which are approximately 50-50 for and against. It doesn't seem like there will be consensus.

Orie Steele: I think we should copy the content into VC JOSE/COSE, and we should live the vc-data-integrity PR open until that or another strategy is completed. I agree that the vc-data-integrity PR is unlikely to gain consensus.

Manu Sporny: I wonder if we should convert it to an issue in the VC JOSE/COSE specification. I also think that copying-and-pasting is a risky strategy for spec text.

Orie Steele: We could promote the use of Multibase, and not mention JWK at all.
… I don't feel that the content of VC data integrity is acceptable.
… I think there is an opportunity to reduce market confusion by 'picking a side' between Multibase and JWK.

Manu Sporny: I don't want to 'pick a side'; that's not what I think the specification is trying to do. We shouldn't force things on markets, because it would split communities of implementers.

Orie Steele: I don't agree, and I believe we are perpetuating a major reason that we don't see market adoption of these specifications.

Dmitri Zagidulin: @Orie - and you think it'll help adoption to say, in the spec, "Oh, people who implemented Multibase, go ahead and re-do everything in JWK"?

Sebastian Crane: I think there is a philosophical issue and Manu alluded to that. There's the issue of whether or not VCs should essentially force a specific technology to facilitate the whole working of the ecosystem or if it's better to let the implementers decide.
… There's also the technical merits of the different key formats.
… I think those debates are tangential, I think we can talk about those merits separately and I recommend creating an issue to document the differences in a collaborative way that can guide those decisions later on.

@bob420-svg

This comment was marked as off-topic.

@bob420-svg

This comment was marked as off-topic.

@msporny
Copy link
Member

msporny commented Sep 2, 2023

There is now a PR here that attempts to make the changes in this PR: https://github.com/w3c/vc-jose-cose/pull/144/files ... and many more that I expect will lead to objections.

This PR is unlikely to be merged and overtaken by events (another PR in another repo). Closing.

@msporny msporny closed this Sep 2, 2023
@bob420-svg

This comment was marked as off-topic.

@msporny msporny deleted the feat/update-verificaton-methods branch February 24, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.