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

fix!: current CType meta schema ignores additional properties in claim contents #726

Merged
merged 14 commits into from
Feb 21, 2023

Conversation

rflechtner
Copy link
Contributor

@rflechtner rflechtner commented Feb 15, 2023

fixes KILTProtocol/ticket#2378

Introduces a new meta schema to fix the issue that additional properties were not validated.
Newly created CTypes would use this meta schema by default; legacy CTypes are still validated against the old meta schema.

  • Makes sure that no additional properties are present on an IClaim
  • Allows for selective disclosure (does not require all allowable properties to be present)
  • Meta schema id uses content integrity protected URI, ideally uses decentralised solution
  • Comes with a strategy for expanding claims to URIs and for checking the same claims on the credentialSubject of a Verifiable Credential
    • Unfortunately it seems that the only viable strategy so far is deriving a schema from the CType.
    • Better compatibility could be achieved by defining an optional @id property on all CTypes (to avoid failing credentialSubjecct verification on VCs, where this would be present after expanding), but this comes at a cost; any IClaim would pass verification even with an extra @id property.
  • Upgrade path / interoperability strategy for credentials issued for the old CType version
    • Because the currently used json schema version does not allow defining a CType which derives the full set of properties from another CType (you can only add references for each property individually) the recommended upgrade path would be to create a new CType with the same properties and publish proof that it is equivalent to the old e.g. by publishing code that reproduces the derivation. Creating a CType where each property is derived from the old CType is another option, but checking that no properties were omitted and that the property names are identical is still a manual process. Compound/nested CTypes are also a bit more complicated to handle, so this seems like the less preferable option.
    • If all properties on the new CType are identical, verifiers would only need to add the new CType to the set of accepted CTypes. The credential could then be processed as before, no matter which version of the CType was used.

How to test:

Updated tests to cover both the old and new meta schema and CTypes created based on them.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

Comment on lines 68 to 69
title: 'CType Metaschema (draft-01)',
description: `Describes a CType, which is a json schema for validating KILT claim types. This version has known issues, the use of schema ${CTypeModelV1.$id} is recommended instead.`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should update the hosted version if merged

@rflechtner rflechtner marked this pull request as ready for review February 16, 2023 17:26
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

Looks very good! I would like to understand a bit more how it all works, if you answer my questions. Then I would take a second look after that :)

packages/core/src/ctype/CType.schemas.ts Show resolved Hide resolved
packages/core/src/ctype/CType.schemas.ts Show resolved Hide resolved
packages/core/src/ctype/CType.schemas.ts Outdated Show resolved Hide resolved
packages/core/src/ctype/CType.schemas.ts Outdated Show resolved Hide resolved
packages/core/src/ctype/CType.schemas.ts Outdated Show resolved Hide resolved
packages/core/src/ctype/CType.ts Outdated Show resolved Hide resolved
packages/core/src/ctype/CType.spec.ts Outdated Show resolved Hide resolved
packages/core/src/ctype/CType.ts Outdated Show resolved Hide resolved
throw new SDKErrors.NestedClaimUnverifiableError(undefined, {
cause: errors,
})
verifyObjectAgainstSchema(cType, CTypeModel, messages)
Copy link
Member

Choose a reason for hiding this comment

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

Question, probably out of scope for this PR: how is a verifier supposed to know what are the nested CTypes? Are they assumed to have this knowledge? Or might they only know the top-level CType they want to verify a credential against?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same thing but yes, I think it's out of scope. Maybe we need a CType registry in the sdk? Would also let us validate a CType only once (when adding to the registry) and not every time it is used.

Copy link
Member

Choose a reason for hiding this comment

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

That would make our SDK less stateless, which is something we wanted to strive for as much as possible. it might be a good idea to at least reference this in a comment or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess that's true. The way I see it choosing a CType is a very conscious decision for a verifier, so they'd be familiar with it and know whether or not it is nested. At least the need to known what sort of data they will be working with in their application, so they'd look into that for sure.

packages/core/src/ctype/CType.schemas.ts Outdated Show resolved Hide resolved
@rflechtner
Copy link
Contributor Author

As far as I can see all the upgrade paths described above but the one where people simply create a new CType based on the old would break quite a few things. So I'd suggest sticking to that. Besides this, I see only two open issue that we should clarify before putting the new CType out there:

  1. Do we consider it V1 or is it simply draft-02? (maybe a tech board question?)
  2. Do we allow nested properties (objects and arrays as values) even though selective disclosure is only possible for top-level properties? I.e. you can't omit an item from an array or a property from an object.

@ntn-x2
Copy link
Member

ntn-x2 commented Feb 20, 2023

Do we consider it V1 or is it simply draft-02? (maybe a tech board question?)

I think this version should be quite stable, right? At least as far as our credentials currently allow. If we want to support nested selected disclosure, we would have a v2 out there. So i would be in favour of having this a v1.

Do we allow nested properties (objects and arrays as values) even though selective disclosure is only possible for top-level properties? I.e. you can't omit an item from an array or a property from an object.

I think, for the sake of completeness, we could still allow any property, making it clear (either in the JSDocs or in the documentation itself), that only the top-level property can be revealed or hidden as a whole.

@rflechtner
Copy link
Contributor Author

rflechtner commented Feb 20, 2023

Do we allow nested properties (objects and arrays as values) even though selective disclosure is only possible for top-level properties? I.e. you can't omit an item from an array or a property from an object.

I think, for the sake of completeness, we could still allow any property, making it clear (either in the JSDocs or in the documentation itself), that only the top-level property can be revealed or hidden as a whole.

As it turns out, there are quite a few details to figure out if we were to allow this, so I think it should be its own ticket with design phase and all. It would also be something that does not affect existing CTypes at all, so it could be quite simply the next meta schema version. See https://github.com/KILTprotocol/ticket/issues/2456.

@rflechtner rflechtner self-assigned this Feb 21, 2023
@rflechtner rflechtner requested a review from ntn-x2 February 21, 2023 12:58
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

Just a question, otherwise LGTM! Very very very very nice job! 👍

packages/core/src/ctype/CType.spec.ts Show resolved Hide resolved
@rflechtner rflechtner merged commit a8713a3 into develop Feb 21, 2023
@rflechtner rflechtner deleted the rf-fix-ctype branch February 21, 2023 16:17
@rflechtner rflechtner changed the title fix: current CType meta schema ignores additional properties in claim contents fix!: current CType meta schema ignores additional properties in claim contents Mar 13, 2023
@rflechtner rflechtner mentioned this pull request Jun 8, 2023
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.

2 participants