-
Notifications
You must be signed in to change notification settings - Fork 115
Remove @vocab
from the base context.
#1520
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
Conversation
Im just going to highlight for one more time, in case this point has been missed by others. This PR and related issue #1514 has been re-raised because of this security disclosure made around DI. Removing I still think its better from a developer experience to have |
Looking at the minutes of our meeting, many on the call were in favor of adding something about an inline I think we should add (possibly in a separate, editorial note) something along the lines of:
|
Hi @tplooker,
My apologies if I misunderstand it, I am a late arrival to that whole discussion, but my understanding is that "fixing it" would involve changes in the JSON-LD specification. The JSON-LD Working Group is currently in maintenance mode, and I am not sure whether it is chartered to make significant changes to the specification. But even if it is, such a change takes a very long time. This means this is not an option for us imho. At this point, what we can do is to draw the attention of developers to the issue, and this is what this PR tries to do (also based on the WG discussion).
See above. That being said, the group recognized that developers may have valid reasons to use this; see my comment above: #1520 (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 support the removal of @vocab from the base context.
I also support @iherman's language in #1520 (comment) of using @vocab for developmental purposes as an additional in-line context, so anticipate that will be a separate PR that will be raised.
I understand the complexity here @iherman but the in-escapable reality is we have a feature in JSON-LD that creates security issues in DI, so I think we need to focus on fixing that, what ever that may involve rather then simply trying to limit the usage of the problematic feature in certain contexts.
I'm quite uncomfortable if this is the best we think we can do here, there are solution options highlighted in the DI issue that are being considered which would fix this.
Side note, I assumed from comments in other threads that inline contexts were "bad practise" see here for an example. If these aren't and in fact are allowed then that makes security issue highlighted here much harder to mitigate without say integrity protecting the |
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 do not support removal of @vocab
from the base context. The reasons it was put there remain valid.
Among other such reasons, it was documented that some JSON LD implementations omit undefined terms and types from the derived RDF. That means, that, for instance, if a JSON object includes a type error, the entire thing will be removed before signing. That means that attackers can change anything in the removed JSON object without affecting the signature.
Using a default @vocab
prevents these attacks from being possible.
So, I don't want to bring that other issue over here, but I wanted to quickly say that this feature does not create security issues in DI. Consuming terms from contexts that are not understood is not spec conformant and doing so is what creates the security issue. Anyone following the specs does not have this security problem because they know they cannot consume a term by just assuming what it means -- rather than understanding the context that defines it. Currently It will never be the case that using This has not been an obvious logical conclusion, tripping some developers up. Now, just because some developers get confused about a feature does not mean we should ban that feature. However, I'm for removing it from the core VC v2 context because I think it's better for terms to be globally unambiguous in production systems as I've stated elsewhere. Note: Altering JSON-LD itself doesn't allow us to avoid the logical conclusion above. If we were to add a JSON-LD To try and be extra clear again here, it is a logical impossibility to do these two things at once:
You simply cannot know whether a term was defined via +1 to removing |
It is non-conformant to silently drop terms. The test suite will have (or may already have) tests to confirm an implementation does not do this. |
I'm in favor of removing @iherman another option is to use the There could even be a context url provided for this purpose, |
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.
Approving and +1 to @PatStLouis 's idea of using the examples context.
@selfissued wouldn't this require a non-conformant issuer and a non-conformant verifier? Reading the specification:
This addresses your concern directly. The main reason this was added was to make it easier for implementers to do development and add JWT private claims interoperability. The current vocab file used reads that this is for:
Having development features in a production system is not a valid reason in my opinion while you seem to argue that it is. |
As the group highlighted during the call this past week, we do not have consensus to include the Even if there are many more people that step forward and object to the PR, that will not change the fact that there are multiple people that are not supportive of this feature. IOW, we do not have consensus for the feature, so we must remove the feature. Saying that one doesn't support the removal of the feature does nothing for a feature that does not have consensus (other than signal that you're displeased about the fact that we do not have consensus for a feature).
Others in this thread have pointed out why your statement is incorrect.
A default |
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 on the fence with this change. I understand the reason and risk of including @vocab
, but it is a reason I'm still promoting the use of W3C VCs within the ecosystem I work in--to maintain compatibility with the data model while not wanting to do LD processing or create and manage our own contexts.
The comments you made on the last call, @msporny, suggest that you believe that this door has been "closed" and folks who don't want to do LD processing have already moved to the IETF or other groups. I don't see it the same way -- the door is still ajar, before this PR.
Given how many credentials I have encountered (and I'm sure others too) that claim to conform to the 1.1 data model but completely ignore LD processing to the degree that LD processors would break...I am still sympathetic to reducing the burden on these implementers. Making more credentials conformant, and breaking fewer processors is a north star for me. If we achieve that, we'll achieve a successful data model.
Now, I completely see the other side: the data model is an LD data model. If folks don't do LD processing (to some degree..) they're signing themselves up for a bad time. Instead of increasing interoperability we're increasing the illusion of interoperability and potentially, confusion. Bite the LD bullet or go to the IETF. It's a simpler and clearer line in the sand.
I have not come to a conclusion yet, just sharing my thoughts. My instinct is that perhaps we could publish and maintain a context with just @vocab
like
{
"@context": {
"@protected": true,
"@vocab": "https://www.w3.org/ns/credentials/issuer-dependent#"
}
}
That we provide as an option -- with necessary caveats and warnings -- for implementers that want to keep the door ajar. I would not go as far as to say it SHOULD NOT or MUST NOT be used in production use cases.
We do not have consensus to remove |
@PatStLouis wrote:
Please re-read that slide in its full context. If you look below the text you have quoted, I highlight the issues with it as a proposed solution. In general this section of the slide deck is a brief discussion of different possible solutions including whether they actually solve the problem, the tradeoffs AND what else would need to be true for them to potentially be a solution. I express clear doubt about this particular "solution" in the first sentence and go on to cite my reasons why. @PatStLouis wrote:
We finally agree on something :) which is that |
@decentralgabe wrote:
Raised in PR #1525 (to try to keep the decisions on each PR simple and not mix in other concerns/arguments). |
@PatStLouis wrote:
I believe @PatStLouis is saying " @decentralgabe wrote:
Seems like he did do his homework in this case. Also, with the standard CEPC reminder to everyone in the thread, this is "Unacceptable behavior" / "Patronizing language or behavior" / "Intentionally or unintentionally making assumptions about the skills or knowledge of others" https://www.w3.org/policies/code-of-conduct/#unacceptablebehavior ... and I say that knowing that you're one of the more pleasant, even keeled, and thoughtful members of the group, and I can assure you that @PatStLouis matches you on each measure. The written word really is a terrible medium for conveying nuance. :) |
I apologize for my language and have edited my original comment. This is a valuable discussion, and I'd like to contribute to keeping it constructive. Pat, your opinion and contributions are welcome, regardless of your involvement in previous discussions. I'd like to clarify the point I was trying to make regarding:
This statement is uncontroversial. However, the key point is:
In the original issue (#953), a primary motivation was lowering the barrier to entry for developers new to JSON-LD, enabling them to create valid Verifiable Credentials more easily. My advocacy for keeping You correctly note that the original discussion mentioned use cases like development, experimentation, and closed applications. However, I believe the line between these use cases can be blurry, and so can the intention of the text itself. My prior comment—and this comment—are responding to your statements:
This is exactly a use case it was intended for, as evidenced by @OR13's original comment in #953:
There's an important distinction to make here. The goal of 'helping developers' isn't limited to assisting with development-stage work; it extends to supporting developers in creating production-ready systems. When we talk about features that aid developers, we're often referring to tools and approaches that simplify the process of building robust, deployable solutions. In this context, using To be fair, @msporny noted in multiple comments in #953 the risks of using Given this context, I believe your assertions do not accurately reflect the original intent of including Let's move to addressing |
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.
Assuming there's a suitable anchor, "described earlier in this section" should link back to that description.
@tplooker
The only use case I've seen listed here over and over again is "issuers don't want to do json-ld while prototyping". If issuers don't want to bother using the technology they are using (this statement in itself highlights the irony of such use cases), they create issues downstream and should consider other options for their solution, at least momentarily. Or, like the solution proposed, segregate their use cases into a subset of credentials,
This is a discussion about data integrity and the use of these features in conjunction with data integrity. If the goal is to address a change in the json-ld library, these conversations should happen elsewhere. The conversation happening here are about data integrity and the VC data model. Shifting the blame elsewhere is just a deflection to avoid addressing the responses people are making. Concrete solutions identified by your very own research are being proposed to address this specific concern.
I've always partially agreed with you since my first post and never claimed otherwise. Having dangled in pen-testing and cybersecurity in the past, I do find this disclosure valuable and have already demonstrated it in a few places for education purposes (while giving the credit to MATTR). Besides, removing What I do disagree with is labeling this as a vulnerability, as this is an attack vector, there is a difference. I'll play nice and say sure, you demonstrated a low impact vulnerability in a sandbox/development/experimental environment with said attack. There was no harmful impact and you only got a json response which says verified (which is true, because the signature is valid). No further treatment is made, this didn't give you access to any resources, you didn't exploit any system nor run any unauthorized code. If theoretically some measures were in place, the credential would still be verified with a warning of un-trusted context, so a business layer controller can make the proper decisions based on that validation response. This is surely something we can look into as we are currently evaluating verification responses in the vc-api, and what consists of an error and a warning. This has also been acknowledged as something to include in conformance testing. A reminder that the vc-api As an aside, here's a vulnerability that was reported in the QC covid passport verification app. This is a vulnerability describing how you could forge a proof of vaccination of any provincial resident. It was demonstrated using the production ready available mobile wallet app and verification software. Sure, covid circumstances were different as a lot of technology had to be put out quickly, but the service providers were using jwt (smart health card) and did a bad implementation leading to this vulnerability. Is jwt to blame here? No one is making this claim, it's the implementation which is the issue. Here's a blog post about the vulnerability a friend of mine wrote. |
I also apologize if my word choices are interpreted negatively. There are significant claims being made here and surely this will tense up the discussions and create a division, however I can assure you I'm not looking to make any negative assumptions about your professional qualifications or work ethics. I don't know you besides a few posts I've read notably around did:x or maybe a talk at IIW, and I surely don't know what conversations you have with your clients, nor is it any of my business. What I do assume due to your dedication in the space is that you have a good grasp on what is at stake and have the ability to think critically, as you've demonstrated.
I do appreciate the motivation, however I will have to say, as a developer, I've seen many good intentions of "we'll fix it later" that never leads to anything once the temporary fix "works". Maybe your experiences can tell a different story. The solution proposed also keeps this available, whilst giving a nudge to developers to take action on it downstream. I've also advocated the The idea of having a separate url for this use case, which can simply be replaced with a proper context url once the implementers did the work, was never refuted by anyone. This is a simple, elegant solution that would greatly reduce the friction we are seeing here. I also use the The Removing it from the base context at the very least makes it easier to manage its use case through spec text and requires implementers to take deliberate actions to enable it (adding the issuer-dependent url to the context). It also provides information to verifiers who also do not want to do json-ld that this credential contains issuer-dependent terms. The risks around |
I wouldn't frame this usecase as "issuers don't want to do json-ld while prototyping", even if you don't define your own JSON-LD context, but you are still issuing or verifying a JSON-LD based VC, you are "doing json-ld". For example more generally I would say a website that integrates in JSON-LD following googles guidelines for SEO is "doing json-ld" even though they aren't defining their own In fact the most promising direction for this technology is to actually limit the situations whereby someone has to define their own However as I've said earlier in the thread I won't block removal of this from the core context but I will just re-state for the record this does nothing to fix the identified issue it is linked to, it simply avoids fixing the problem. |
@tplooker wrote:
It seems that others in this thread, and in the WG, disagree. This PR is part of a broader solution that is being discussed in w3c/vc-data-integrity#272. "avoids" can be interpreted as a belief that people are "uninformed" at best or "acting in bad faith" at worst (putting their heads in the sand instead of dealing with an issue). "problem" ignores the very long thread of discussion that proposes that the "real problem" was a misconfiguration of software in the security disclosure. I realize that both of these points are some of the points of contention in that very long thread, but repeating one side of that point of contention doesn't accomplish much when it is clear that there is disagreement on "avoiding" and "problem" in the w3c/vc-data-integrity#272 thread. On the contrary, the issue has had vigorous debate and this PR was one of the outcomes of that debate. Let's try to keep discussion in this thread focused on this specific PR and its changes and keep the more general discussion on "what else to do" in w3c/vc-data-integrity#272. |
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.
👍
The issue was discussed in a meeting on 2024-07-17
View the transcript3.2. Remove
|
Co-authored-by: Ivan Herman <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
8cfa55b
to
b2ab43b
Compare
From the telecon today:
This has been implemented in the PR, in these lines. Requesting a re-review from @selfissued. Per the call today, this PR will be merged by Friday if there are no more objections. |
Normative, multiple reviews, changes requested and made, no remaining objections, WG-imposed merge delay requirement met, merging. |
This PR is a partial attempt to address issue #1514 by removing
@vocab
from the base context because we no longer have WG support for this specific usage of the feature.Other forthcoming PRs will provide new guidance on when and how to use
@vocab
safely.Preview | Diff