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 "criticalProperty" field #158

Closed
msporny opened this issue Apr 8, 2018 · 27 comments
Closed

Add "criticalProperty" field #158

msporny opened this issue Apr 8, 2018 · 27 comments
Assignees

Comments

@msporny
Copy link
Member

msporny commented Apr 8, 2018

We may want to add a "criticalProperties" field to the specification to match the security properties provided by x509 Critical extensions flag: http://www.securesenses.net/2013/05/x509-certificates-critical-vs-non.html

It would specify terms that MUST be understood and processed by a verifier. The field would prevent the usage of credentials that are only partially understandable by a verifier.

The drawback of this approach is that some issuers may mark fields as critical that are not for a particular verifier. Ignoring this field is always up to the verifier.

Here's an example:

{
  "id": "http://dmv.example.gov/credentials/3732",
  "type": ["Credential", "ProofOfAgeCredential"],
  // the property below is the suggested addition to the data model
  "criticalProperty": [
    "ageOver", "termsOfUse", "assigner", "assignee", 
    "prohibition", "target", "action"
  ],
  "issuer": "https://dmv.example.gov/issuers/14",
  "issued": "2010-01-01T19:73:24Z",
  "claim": {
    "id": "did:example:ebfeb1f712ebc6f1c276e12ec21",
    "ageOver": 21
  },
  "termsOfUse": [{
    "type": "Policy",
    "uid": "http://example.com/policies/credential/4",
    "profile": "http://example.com/profiles/credential",
    "prohibition": [{
      "assigner": "https://dmv.example.gov/issuers/14",
      "assignee": "AllVerifiers",
      "target": "http://dmv.example.gov/credentials/3732",
      "action": ["Archival"]
    }]
  },
  "proof": { ... }
}
@msporny msporny changed the title Add "criticalProperties" field Add "criticalProperty" field Apr 8, 2018
@David-Chadwick
Copy link
Contributor

It is true that any verifier can ignore any field in an X.509 certificate or a verifiable credential, and accept it as valid, even if it has been revoked, or contains unknown properties. There is a significant difference however between obeying the fields in a certificate/credential, and ignoring them. In the former case the issuer takes the liability, in the latter case the verifier takes the liability.

@David-Chadwick
Copy link
Contributor

Whilst I support the idea of having critical extensions, I would rather define them in a different way to the one being suggested here i.e. we have critical extensions to the current data model, and not critical properties in the data model.
I suggest that the critical is a boolean parameter that may be added to any property, and takes the default value of false. In this way, only critical extensions that must be understood need to be flagged as true. This means that:

  1. every property and type that is fully defined in the verifiable credentials data model document does not need to be flagged as critical because they are not extensions. They are already defined in the data model document therefore everyone knows what they mean.
  2. every property that is only partially defined in the data model document, such as terms of use, would only need the critical extension parameter in terms of use properties that are not part of the standard data model document.
  3. all properties that are not defined in the data model document will need to contain the critical extension property if the issuer is not prepared to take any liability from verifiers who ignore it.
    The above should minimise the need for the critical extension flag

@dlongley
Copy link
Contributor

dlongley commented Apr 9, 2018

I don't think we need "critical" flags at all, I don't think they are actually useful.

An issuer that does not want liability will mark all properties critical. An issuer that wants their VCs to be accepted (i.e. interoperability) will only mark fields critical based on the consensus of at least a majority verifiers (maybe more). In short, whether or not the flag is relied upon determines whether or not the flag exists. This defeats the purpose of the flag.

Why not simply say that all properties are critical and the liability is entirely on verifiers to understand a VC's properties and decide what to accept or reject? If enough VCs are being rejected, then market forces will cause issuers to omit properties that are not actually critical/well understood. If there are properties that are somehow critical for some verifiers and not others, market forces will cause new credential types (or different bundled claims) to arise. So far, I haven't seen an argument that convinces me that the added complexity of a "critical" flag is warranted.

@David-Chadwick
Copy link
Contributor

@dlongley "An issuer that does not want liability will mark all properties critical."

This does not stop liability. If a verifier understands all the properties the issuer is still liable. Furthermore any issuer that will not take any responsibility for anything it says wont be trusted by any verifier. So this is not an argument against a critical flag, it is against untrustworthy issuers.

@dlongley "If there are properties that are somehow critical for some verifiers and not others"

this was never the intention. The critical flag applies to all verifiers who receive the VC.

I think you must have misunderstood the purpose of the critical flag which is to tell verifiers "I am issuing this VC for verifiers who trust me, and I have included some non-standard properties that are essential for you to understand before accepting this VC"

@David-Chadwick
Copy link
Contributor

@dlongley "say that all properties are critical and the liability is entirely on verifiers"

This is a more restrictive data model and would necessitate the following statement to be added to the data model document:
"all properties must be understood by the verifier. If a verifier receives a verifiable credential containing one or more properties that are not recognised or understood then the verifier should reject the credential, unless it is willing to accept all liability for accepting the credential"

The above satisfies me from a security perspective. But I think it could negatively impact utility. Adding the criticality flag increases utility whilst not impacting security.

@dlongley
Copy link
Contributor

dlongley commented Apr 9, 2018

@David-Chadwick,

I think you must have misunderstood the purpose of the critical flag which is to tell verifiers "I am issuing this VC for verifiers who trust me, and I have included some non-standard properties that are essential for you to understand before accepting this VC"

No, I understand that. I'm saying that verifiers have a say in it because an issuer's VCs won't be used if verifiers refuse to accept them. That will drive what the issuer is willing to put into the VC (or mark "critical").

@msporny
Copy link
Member Author

msporny commented Apr 18, 2018

From @David-Chadwick in #159:

Concerning extensibility, I think the standard should do one of two things, Either:
a) mandate, with a MUST statement, that a verifier must discard any VC containing any field it does not understand, OR
b) for each parameter in the envelope that is not specified in the base standard, mandate with a MUST statement, that it must say whether it is mandatory or optional for the verifier to understand it, and if it is mandatory, the verifier must discard the VC if it does not understand it.

@David-Chadwick
Copy link
Contributor

@msporny I dont think your critical property can work as specified, since there is no guarantee that each property name will be unique, especially since properties can be several levels deep and specified by different organisations. Thus the individual properties should be tagged as critical or not, with the default being not critical so that most properties dont need to be tagged.

@burnburn
Copy link
Contributor

Looks like there is disagreement here but the PR was merged anyway. @msporny @David-Chadwick please discuss and resolve or we will need to back out the PR.

@msporny
Copy link
Member Author

msporny commented Apr 25, 2018

@burnburn @David-Chadwick PR #159 wasn't the PR for this issue. It was the PR for a different issue regarding putting a section into the specification that explains how extensibility works for VCs. It's a largely non-normative section. I'll pick up the conversation on PR #159 on that PR thread, but this issue and that PR barely have anything to do with one another.

@dlongley
Copy link
Contributor

dlongley commented Apr 30, 2018

@David-Chadwick,

I dont think your critical property can work as specified, since there is no guarantee that each property name will be unique, especially since properties can be several levels deep and specified by different organisations.

Our extensibility mechanism (JSON-LD) guarantees uniqueness. That being said, I still think a critical flag is unnecessary. I think we should say that if a property is not understood, the VC MUST be rejected and leave it at that.

I say let market forces govern the proliferation of useful properties -- and let protocols allow relying parties to ask for what they need/support. For simple/atomic VCs and data minimization/selective disclosure VCs, this makes a lot of sense, and for more complex ones, vocabularies like the CTDL can be adopted and understood by verifiers or niche vocabs for smaller communities can be created -- all using the same extensibility mechanism.

@dlongley
Copy link
Contributor

dlongley commented Apr 30, 2018

Exploring further into the concept of differentiating "critical" from "non-critical" fields...

Our data model is designed around making statements (claims) like this:

subject predicate object

It would be good to have some examples where such a statement could be invalidated by another statement. If we can't come up with use cases for this or it's unreasonable for this occur, then there's no reason, IMO, to mark some statements "critical" and others not. This allows for recipients to simply ignore what they don't understand.

Again, if such use cases do exist, then I recommend rejecting what you don't understand, as stated above -- thereby keeping things simple. Either way, I am skeptical of the need for a "critical" flag and would like to see some examples of where it is necessary if we're going to add such complexity to the spec.

@David-Chadwick
Copy link
Contributor

@dlongley " I think we should say that if a property is not understood, the VC MUST be rejected and leave it at that" This is clearly one solution that is secure and will work. But I think it unnecessarily reduces usability and interworking. For example, an issuer could not even add a 'description' property, or 'useful hint' property without invalidating its VCs until every verifier can understand them. The critical flag draws a balance between usability and security, by allowing new properties to be added that sophisticated verifiers can use whilst simple (or older) verifiers can safely ignore.

@David-Chadwick
Copy link
Contributor

Here is an example. I am assuming the processing of VCs is automated without human intervention (although if the properties are in a foreign language then human intervention wont necessarily help).

A verifier wants to know if the subject is married so receives this VC which it understands


 "claim": {
        "id": "did:example:ebfeb1f712ebc6f1c276e12ec21",
        "dateOfMarriage": "1/1/2000"
  },

Sometime later it receives another VC


 "claim": {
    "id": "did:example:ebfeb1f712ebc6f1c276e12ec21",
    "dateOfMarriage": "1/1/2000"
    "xxxxxx": "yyyyy"
  },

Is the subject still married or not?
Without the criticality field the verifier cannot tell if xxxx is significant or not.

xxxxx could be the date of divorce, in which case the subject is no longer married, or it could be the number of children, in which case the subject is still married. Or it could simply be the name of the town the couple were married in - a purely advisory field.

One can think of many such cases where the verifier simply does not know whether to ignore a property and continue processing the VC, or reject the VC.

@dlongley
Copy link
Contributor

@David-Chadwick,

Is the subject still married or not?
Without the criticality field the verifier cannot tell if xxxx is significant or not.

IMO, this is bad modeling and it reveals more information than the verifier needs to know, which runs counter to data minimization principles, nevermind the additional complexity. There should be a boolean claim that says whether or not the subject is married -- and this is what the verifier should request and use.

In general, the useful of a critical flag implies that the field on which it appears is a "modifier" field. That is, that field causes a change, somehow, in the meaning of other fields. Otherwise, its scope would be limited to the field itself, meaning the verifier could safely ignore it if it was not understood without it having any effect on any other field.

It seems to me that any field of that sort should either:

  1. Be in the core data model (e.g. expires),
  2. Be modeled in an extension such that a critical flag is unnecessary,
  3. Not exist because it violates data minimization principles.

@dlongley
Copy link
Contributor

dlongley commented Apr 30, 2018

I think VCs should be modeled such that they can reveal "more" information, the more that fields are understood, rather than having existing fields get "modified" to no longer have the same meaning. This is inline with data minimization principles -- and verifiers should be programmed with this in mind.

There should be a sort of monotonicity of logic. "Are you married?" is a fundamentally different question from "When did you get married?". The latter should not be used to answer the former and -- the former doesn't change by adding more information. This creates a simpler system for verifiers to understand and implement.

@David-Chadwick
Copy link
Contributor

@dlongley . I agree that data minimisation is preferrable, and that atomic VCs or selective disclosure VCs using IBM's anonymous credentials is better than VCs containing a lot of properties. But I think it is perhaps unrealistic to expect all users of VCs to know how to model VCs 'correctly' as you suggest so as to not violate data minimisation principles. You have already argued that we cannot control what users will put in VCs, and in fact, we do not want to, as this will stop innovation. Thus we should provide an easy way of allowing them to extend VCs in a controlled way that tells the verifier what to do about the new extensions. Even if the extensions are standardised, due to the lag in implementations migrating from old to new standards, then we will have a mixed economy during the migration period (which could be many years). There needs to be a way for "old" verifiers to still accept VCs from "new" issuers.

@dlongley
Copy link
Contributor

dlongley commented Apr 30, 2018

@David-Chadwick,

Thus we should provide an easy way of allowing them to extend VCs in a controlled way that tells the verifier what to do about the new extensions.

IMO, I think the "easy way" is to tell authors that they should offer boolean attributes to verifiers so that verifiers do not need to write complex, error prone code that attempts to guess at what the issuer (who is meant to be the trusted party) was trying to indicate. If the verifier trusts the issuer to tell them whether or not someone is currently married, then the issuer should issue a credential with the boolean flag "married". This is the common case and it should be that simple. I believe this is the right advice to give in the spec.

I think providing a "critical" flag goes in the other direction. It encourages issuers to output a hodgepodge of attributes for verifiers to try and reason through in order to get to the real answer they want -- because, somehow, the "critical" flag will save them. It won't. It will just be a mess, IMO, and is therefore the wrong advice. Requiring verifiers to put together a bunch of different "critical" pieces of information in order to determine whether or not someone possesses a simple attribute sounds like one kind of problem we're trying to improve upon with this work.

Part of an issuer's value is in being able to immediately and clearly tell verifiers what they want to know. If an issuer knows what the verifier wants, then they can set a simple attribute without any need for external modifiers that may change its meaning. If the issuer doesn't know what the verifier wants to know, then they have no business setting a "critical" flag.

Verifiable Credentials that have a lot of data, e.g. resumes, transcripts, etc., that are worth anything will be based off of well defined vocabularies that are shared across the appropriate market vertical (and probably standardized themselves, at least in a de facto manner). The use of a "critical" flag here makes no sense -- issuers are essentially data sharing for too wide a variety of purposes.

There needs to be a way for "old" verifiers to still accept VCs from "new" issuers.

If we're concerned about having a way to allow "old" verifiers to accept VCs from "new" issuers, the advice we should give is something along the lines of what I said above: keep the attributes simple and use monotonic logic to add more information. Then we can allow fields to be ignored. If an issuer can't do that for their use case, then they should issue a different VC that the old verifiers will reject (because it lacks the core credential type or attribute they are looking for) and force them to upgrade. There's no need to mark this attribute as "critical" -- it is the very thing the verifier requested.

I think giving any other advice will encourage an ecosystem where "critical" will lose its meaning (because its often hard to define to begin with if verifiers have to "calculate" whether or not a subject has a certain attribute!) and we'll have messy VCs.

@David-Chadwick
Copy link
Contributor

@dlongley you appear to be arguing both ways, viz:
i) " I think we should say that if a property is not understood, the VC MUST be rejected and leave it at that"
ii) "keep the attributes simple and use monotonic logic to add more information. Then we can allow fields to be ignored. "
You cannot have your cake and eat it.

@dlongley
Copy link
Contributor

@David-Chadwick,

I'm offering two possibilities that I would find acceptable, neither of which involve a "critical" flag.

@stonematt
Copy link
Contributor

@dlongley

IMO, I think the "easy way" is to tell authors that they should offer boolean attributes to verifiers so that verifiers do not need to write complex, error prone code that attempts to guess at what the issuer (who is meant to be the trusted party) was trying to indicate. If the verifier trusts the issuer to tell them whether or not someone is currently married, then the issuer should issue a credential with the boolean flag "married". This is the common case and it should be that simple. I believe this is the right advice to give in the spec.

The idea that VC claims should be boolean is a bit out of left field, and maybe you're just trying to be provocative. Except in the overused cased of "ageOver" the question a Verifier is asking is almost never a yes/no question. and @David-Chadwick the marriage question really introduces the "point in time" problem, where a VC from last month may indicate that an individual was married, but a VC from today wouldn't. Last months presentation was valid and would still decode/verify. Unless we have a "refresh/get current" mechanism this line of argument is tricky.

Perhaps in this case, the claim that includes "dateOfMarriage": "1/1/2000". is actually evidence for a self issued claim that includes '"isMarried": "True"' -- is that what you meant?

@dlongley
Copy link
Contributor

dlongley commented May 1, 2018

@stonematt,

The idea that VC claims should be boolean is a bit out of left field, and maybe you're just trying to be provocative.

I'm sorry I've created some confusion here -- let me try to clarify. I'm not suggesting that all claims be booleans. I think the "critical" flag is really about making a decision that may change from "yes" to "no" (or vice versa) based on its presence. Therefore, boolean attributes are the only ones that make sense to discuss here.

So, I'm suggesting that if the purpose of a claim is to help a verifier know a simple boolean attribute, then the issuer should just specify it as such (e.g. "married: true", "isUsCitizen: true", etc). If the issuer knows the question and the answer, then the verifier should not be left to try and decipher whether or not someone is married or a US citizen based on a variety of dates and other proxy information that may or may not indicate the presence of the attribute.

If the issuer doesn't know the answer, then I believe it is hopeless for them to tell the verifier that several fields are critical. The verifier is the one who has to make up their mind using whatever information they understand. And if the issuer doesn't know what the question is, how can they possibly tell the verifier what's critical with respect to it?

I say no to all of this. If the verifier knows what they are asking for then goal should be to give it a simple answer, not leave it guessing. If the verifier has to guess using a variety of information from issuers, then it doesn't matter what is marked "critical" or not, the verifier is the one making the determination based on what they understand. If the issuer does know the answer, they should say married: true. They may also give marriage and divorce dates if that's something a verifier may need or ask for and that's fine... but those fields should not invalidate married: true by design.

I believe this is a problem that is really only solved with proper modeling, not with flags that say whether or not a verifier should let some claims invalidate the meaning or conclusions that can be drawn from others (which presumes the issuer knows those conclusions). If the issuer knows what should be paid attention to, then let them make clear claims, not obfuscated ones that require the verifier to jump through hoops to determine what they really want to know. Furthermore, if data can be minimized (e.g. only what the verifier needs to know), let that be what is sent.

If the advice we give is that the logic in claims should be monotonic in nature (i.e. new claims do not invalidate the meaning of other claims) then we don't need a "critical" flag. The use of a "critical" flag is a signal, IMO, that we're trying to patch a sinking ship ... actually, that we're asking someone with less knowledge (the verifier) to patch the sinking ship the issuer created. Let's recommend how to build the ship properly instead.

My understanding is that the main thrust behind this thread and the other related to extensibility was to give advice in our specification about how to create secure, trustworthy, useful Verifiable Credentials, regardless of whether or not that advice is adhered to by implementers. I think giving the modeling advice I've recommended above is the way to do that. I think using a "critical" flag is instead to encourage that such advice is ok to be ignored because the flag can set everything right. I don't believe it will and that it seems to be an anti-pattern.

@dlongley
Copy link
Contributor

I believe the group decided that this issue can be closed now given our adoption of a SHOULD + a reason when not to (verifier acceptable risk):

#159 (comment)

If that's the case, let's close this in 7 days if no one objects.

@David-Chadwick
Copy link
Contributor

@dlongley Actually the proposed text was MUST + a reason when not to (verifier accepts the risk).
see #159

@msporny
Copy link
Member Author

msporny commented May 16, 2018

@David-Chadwick are you objecting to closing this issue, or clarifying what the change was?

This is the current spec text: 42c0426#diff-eacf331f0ffc35d4b482f1d15a887d3bR881

@David-Chadwick
Copy link
Contributor

This issue can be closed because the existing text does say MUST.

@msporny
Copy link
Member Author

msporny commented May 16, 2018

Thanks @David-Chadwick, closing the issue.

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

No branches or pull requests

5 participants