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

First draft of the CIP on Certification Metadata #18

Closed

Conversation

RSoulatIOHK
Copy link
Collaborator

Creation of the first draft for a CIP on Certification Metadata based on the comments and proposal done by @simonjohnthompson on the CIP72 DApp Registration & Discovery pull request

Creation of the file that lists the list of fields we see as mandatory for a certificate on-chain
Changed mandatory fields following discussion with what Legal team wants to see (Disclaimer), some discussions with the WG (hash of reports) and what the DApp store would like to be able to fetch (Summary of the report)
Removed the date of issuance from the metadata (the slot number should be enough) and sync the mandatory fields from MandatoryFields.md with CIP draft
Copy link

@aleeusgr aleeusgr left a comment

Choose a reason for hiding this comment

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

To summarize:
An end user interacts with a dApp through a dApp Store or a wallet. This software implements a mechanism by which a user is alerted to the level of assurance that the dApp in question was subject to, and more importantly - if it was not at all.
To certify an application with the dApp store, the developer has to reach out to an auditor.
The auditor will perform the audit, create metadata that conforms to the standard we are developing, sign it with their key and submit a transaction that will store this information on-chain. It is possible to update certification metadata if needed.

Some consequences to think about:

  1. Additionally to L1,2,3 what happens is a vulnerability is discovered? How does metadata gets updated? If someone performed L3, and then suddenly it turns out something is wrong. Does in invalidate L3 and set the level to L2? Or maybe something else. I think there should be additionally a state that notifies that something went wrong after QA was performed: an additional 0/1 flag field "it's certified, but review is necessary". Maybe it could be set to 0 by submitting an audit metadata, but automatically changed by the wallet if, for example, several users reported the app?
  2. Auditor may get power over the developer, e.g. blackmail or extort them under the threat of revoking the certificate. How do we make auditors accountable? Maybe revocation must be done thorough consensus of several auditors?
  3. Related to CPV-2023-011, parametrization..... erased, thinking.

@RSoulatIOHK
Copy link
Collaborator Author

Thanks @aleeusgr for this comment.

I don't think there is a mechanism currently described in this CIP to update the metadata but that could be something we can implement. Registration has something like that to allow for new version of DApps.

  1. I don't understand what you mean by "invalidate L3 and set to L2". Those levels are the one described here:
    https://www.essentialcardano.io/article/new-certification-levels-for-smart-contracts-on-cardano
    I thought they were kind of canonical in the eco-system that's what I was always refering. So a manual audit would be L2. That being said, we could design something to take a new vulnerability into account. This could be done by either having the certificate issuer, issuing some correction or revoking it. As before, we could draw some inspiration from what the Registration CIP is doing. We could also reference a known list of vulnerability audited at the time of audit and inform the user that any new vulnerability might be present in the product.

  2. I don't think an auditor should be able to revoke a certificate to be honnest. The certificate should clearly display when it was done, what was audited, what was looked for, which version of the DApp, all the tools used to produce the DApp and test it so that if a new vulnerability is discovered, the auditor cannot be held responsible, or if a vulnerability is found somewhere outside the Target of Evaluation.

  3. We need to be able to accomodate parametrization at some point. I agree but I also don't have a good way of doing it. Maybe we could organize a brainstorming session with the Working Group in the near future to discuss possible solutions

Copy link
Collaborator

@mmontin mmontin left a comment

Choose a reason for hiding this comment

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

Review from Tweag.io
Nicolas & Mathieu

### Use-cases and Stakeholders
DApp developers will seek certification from one of the providers, according to their desired level of certification and will refer to the certificate on several platforms (e.g. on their website or in their DApp registration on a DApp store).

Certification is to be provided by certification providers including: testing services (level 1), auditors (level 2), and verification services (level 3). Certification providers will issue these certificates refering to a particular version of a DApp that has succesfully gone through a certification process.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would refine the notion of successfully going through a certification process. Usually, audits will yield findings that should be addressed afterwards.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point but I think this should be defined in a certification standard. This on-chain certificate should only be the result of a succesful certification process no matter what it means at the end.

<!-- The technical specification should describe the proposed improvement in sufficient technical detail. In particular, it should provide enough information that an implementation can be performed solely on the basis of the design in the CIP. This is necessary to facilitate multiple, interoperable implementations. -->

### Certification providers
Certification providers will present which level of certification was reached and present evidence of the work done. Certification providers will sign the certificate to attest that they have done the work and prevent certificate forgeries.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should refine what it means to sign a certificate:

  • should the party sign a message prior to putting it onchain with a crypto key?
  • should the party sign the transaction that will create the utxo containing the certificate?

Copy link
Collaborator Author

@RSoulatIOHK RSoulatIOHK Mar 16, 2023

Choose a reason for hiding this comment

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

That's a good point. I think either signing the transaction, but that would mean having a "certification" wallet for each certification provider or just signing the content in the metadata. I went with the latter in the new version of the CIP that I will push soon but I'm definitely open to both solutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, in this second option, it does not matter what wallet signs the transaction as long as the metadata are signed by a cryptographic key known to be associated to an auditor?

Copy link
Collaborator Author

@RSoulatIOHK RSoulatIOHK Mar 30, 2023

Choose a reason for hiding this comment

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

Yes exactly. So it could be send by the auditor, the DApp developer or any third parties

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds fine by me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I do have one question: we happen to be on a blockchain that has native support for signatures and such; what's the motivation behind the decision to not use that native support but to use another level of signatures instead? Could the metadata file be stored elsewhere as well, eg. on IPFS?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes having the auditor directly sign the transaction is the other choice. I picked the same one as registration did but both solutions are fine by me.



### Definitions
- **dApp** - A decentralised application that is described by the combination of metadata, certificate and a set of used scripts.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we explicitly mention minting policies or are they included into scripts by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I think we need to be more explicit and include them into scripts.

- **dApp Store** - A dApp aggregator application which follows on-chain data looking for and verifying dApp metadata claims, serving their users linked dApp metadata.


### Properties
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about putting these fields as an array with:

  • field name
  • type
  • mandatory?
  • depiction

Copy link
Contributor

@Niols Niols Apr 5, 2023

Choose a reason for hiding this comment

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

Seeing that this comment led to confusion, let me rephrase. What about describing these properties in a Markdown table, with four columns, one for field name, one for type, one for whether it is mandatory and one for a description of the entry. For instance:

| Field name                 | Type                        | Mandatory? | Description
|----------------------------|-----------------------------|------------|---------------
| `subject`                  | UTF-8 encoded string        | Yes        | Identifier of the claim subject (i.e dApp)
| `type`                     | JSON object                 | Yes        | The type of the claim
| `type.action`              | `CERTIFY` or `AUDIT`        | Yes        | The action that the certificate is asserting. In the case of `CERTIFY`, the certificate is asserting that the dApp is being certified. In the case of `AUDIT`, the certificate is asserting that an audit has been performed for this DApp. `AUDIT` shall come with a certification level `certificationLevel` of `0`. [IDEA: Revoke? Update?]
| `type.certificationLevel`  | Integer between `0` and `3` | Yes        | The level of certification. `0` is reserved for audits and reports that are not compliant with the certification standards. `1`, `2`, and `3` refer to the certification certificates referring to the three level of certifications as defined by the certification working group.
| `type.certificationIssuer` | UTF-8 encoded string        | Yes        | The name of the certification issuer.

I don't know how much better than would be. Alternatively, maybe a list formatted a bit more strictly:

- `subject`: Identifier of the claim subject (i.e. dApp).
  - **mandatory**: yes
  - **type**: UTF-8 encoded string

- `type`: The type of the claim.
  - **mandatory**: yes
  - **type**: JSON object

- `type.action`: The action that the certificate is asserting.
  - **mandatory**: yes
  - **type**: `"CERTIFY"` or `"AUDIT"`
  - In the case of `CERTIFY`, the certificate is asserting that the dApp is being certified. In the case of `AUDIT`, the certificate is asserting that an audit has been performed for this DApp. `AUDIT` shall come with a certification level `certificationLevel` of `0`. [IDEA: Revoke? Update?]

- `type.certificationLevel`: Certification level.
  - **mandatory**: yes
  - **type**: integer between 0 and 3
  - 0 is reserved for audits and reports that are not compliant with the certification standards. 1, 2, and 3 refers to the certification certificates referring to the three level of certifications as defined by the certification working group.

- `type.certificationIssuer`: The name of the certification issuer.
  - **mandatory**: yes
  - **type**: UTF-8 encoded string

A pretty visualisation could otherwise simply be generated from the JSON schema.

Metadata/CIP-Certification-Metadata.md Outdated Show resolved Hide resolved

**certificateIssuer**, a string, is a mandatory field that represents the company that has issued the certificate. It will be used with a public list of public keys to ensure that the certificate originates from a trusted certification provider.

**reportURLs**, an array of URLs, is a mandatory field that link to the actual certification report for anyone to read. This ensures transparency in the findings, what was and was not considered in the certification process.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
**reportURLs**, an array of URLs, is a mandatory field that link to the actual certification report for anyone to read. This ensures transparency in the findings, what was and was not considered in the certification process.
**reportURLs**, an array of URLs, is a field where each link points to the same actual certification report for anyone to read. This ensures transparency in the findings, what was and was not considered in the certification process.

Maybe this should not be mandatory. There might not be a report (at all, or yet), depending on the contract between the dApp developer and the certification party. Additionally, links can die.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think there could be a certification with no kind of report to be honnest but I can move that to non-mandatory for the moment.


**reportURLs**, an array of URLs, is a mandatory field that link to the actual certification report for anyone to read. This ensures transparency in the findings, what was and was not considered in the certification process.

**reportHash**, a string, is a mandatory field that is the blake2b-256 hash of the audit file pointed by the links in **reportURLs**.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
**reportHash**, a string, is a mandatory field that is the blake2b-256 hash of the audit file pointed by the links in **reportURLs**.
**reportHash**, a string, is a field that is the blake2b-256 hash of the report pointed by the links in **reportURLs**.

Comment on lines 68 to 74
**summaryURLs**, an array of URLs, is a mandatory field that link to the actual certification summary for anyone to read.

**summaryHash**, a string, is a mandatory field that is the blake2b-256 hash of the summary file pointed by the links in **summaryURLs**.

**disclaimerURLs**, an array of URLs, is a mandatory field that link to the legal disclaimer about the content of the report pointed by **reportURLs** links.

**disclaimerHash**, a string, is a mandatory field that is the blake2b-256 hash of the summary file pointed by the links in **disclaimerURLs**.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these fields really necessary? Shouldn't they be embedded in the report?
Instead, we could have a summary that is a short string summarizing the work that has been conducted. This could be show directly on the dApp store.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They should be embedded into the report in my opinion but there is a demand from DApp store to be able to display them directly at the DApp store level. I don't think we'll be able to standardized the format of the report to a point where they could extract it automatically which is why I made them as fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this suggestion, though, they are not embedded in the metadata which only contains a link (or several links) to the summary or the disclaimer. Is the dApp store going to pull those links, hoping one of them resolves, and show the result? Does that imply constraints on what's at the end of the link (as in plain text for instance, with a size limit)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved those to strings. Thanks for the suggestion.


**disclaimerHash**, a string, is a mandatory field that is the blake2b-256 hash of the summary file pointed by the links in **disclaimerURLs**.

**scriptHashes**, containing an ordered list of all scripts that make the DApp, is a mandatory field which can be used as a link to the actual DApp running on-chain.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
**scriptHashes**, containing an ordered list of all scripts that make the DApp, is a mandatory field which can be used as a link to the actual DApp running on-chain.
**scriptHashes**, containing an array of script hashes that make the DApp, is a mandatory field which can be used as a link to the actual DApp running on-chain.

Why does it have to be an ordered list instead of an array?
Additionally, who is gonna compute those hashes?
What about parameterized scripts with unpredictable parameters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea behind ordering them is to make it easy to compare them with registration metadata versions for example. But this is not really useful so I changed to an array and I removed the ordering.

The certificate issuer needs to be able to compute the script hash and script address. My understanding was that it only requires the parameters to be instantiated and the full compilation chain which is not too much to request when getting certified.

Parametrized scripts is still a challenge with this version of metadata description.

RSoulatIOHK and others added 3 commits March 16, 2023 16:13
Added the suggestions from Matthieu and Nicolas (hopefully without missing anything).
Reworked everything to match the registration metadata format.

There are probably mistakes made along the way but at least we can start discussing this version.
Fixed signature description -> Signature is done by signing the rootHash of the metadata.

Added suggestion for audit (unrelated to L2) for certificate type
Included an example and added Levels in the table
Copy link
Contributor

@Niols Niols left a comment

Choose a reason for hiding this comment

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

Tiny details. It might be worth running a tool such as markdownlint to clean this up and makes spacing around titles in particular more consistent, eg.

nix run nixpkgs#nodePackages.markdownlint-cli -- *.md

- `action`: The action that the certificate is asserting. It can take the following values:
- `CERTIFY`: The certificate is asserting that the dApp is being certified.
- `AUDIT`: The certificate is asserting that an audit has been performed for this DApp. `AUDIT` shall come with a certification level `certificationLevel` of `0`.
[IDEA: Revoke? Update?]
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
[IDEA: Revoke? Update?]

We should probably not include those in the CIP that we send. We should however create issues to maybe add such things in a future version of the standard.

- `CERTIFY`: The certificate is asserting that the dApp is being certified.
- `AUDIT`: The certificate is asserting that an audit has been performed for this DApp. `AUDIT` shall come with a certification level `certificationLevel` of `0`.
[IDEA: Revoke? Update?]
- `certificationLevel`: The certification level is an integer between 0 and 3. 0 is reserved for audits and reports that are not compliant with the certification standards. 1, 2, and 3 refers to the certification certificates refering to the three level of certifications as defined by the certification working group.
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
- `certificationLevel`: The certification level is an integer between 0 and 3. 0 is reserved for audits and reports that are not compliant with the certification standards. 1, 2, and 3 refers to the certification certificates refering to the three level of certifications as defined by the certification working group.
- `certificationLevel`: The certification level is an integer between 0 and 3. 0 is reserved for audits and reports that are not compliant with the certification standards. 1, 2, and 3 refers to the certification certificates referring to the three level of certifications as defined by the certification working group.

Comment on lines 184 to 185
- `1304`: DApp Certification
[TODO: Pick a number and register it in accordance to [CIP10](https://cips.cardano.org/cips/cip10/)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This TODO should not appear in the final document.

Comment on lines 190 to 213
### Properties


**certificateIssuer**, a string, is a mandatory field that represents the party that has issued the certificate. It will be used with a public list of public keys to ensure that the certificate originates from a trusted certification issuer.

**report**, an object that contains:
- **reportURLs**, an array of URLs, is a field where each link points to the same actual certification report for anyone to read. This ensures transparency in the findings, what was and was not considered in the certification process..

- **reportHash**, a string, is a field that is the blake2b-256 hash of the report pointed by the links in **reportURLs**.

**summary**, an object that contains:
- **summaryURLs**, an array of URLs, is a mandatory field that link to the actual certification summary for anyone to read.

- **summaryHash**, a string, is a mandatory field that is the blake2b-256 hash of the summary file pointed by the links in **summaryURLs**.

**disclaimer**, an object that contains:
- **disclaimerURLs**, an array of URLs, is a mandatory field that link to the legal disclaimer about the content of the report pointed by **reportURLs** links.

- **disclaimerHash**, a string, is a mandatory field that is the blake2b-256 hash of the summary file pointed by the links in **disclaimerURLs**.

**script** an array of objects that represents the whole DApp's on-chain script. Each object is comprised of:
- **fullScriptHash**, a string, is the prefix and script hash or script hash+staking key
- **scriptHash**, a string, is the script hash or script hash+staking key
- **contractAddress**, a string, the script address
Copy link
Contributor

Choose a reason for hiding this comment

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

Properties for on-chain are in code (between backquotes) while properties for off-chain are in bold (between **). We should unify those.

Comment on lines 353 to 355
}

```
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
}
```
}
```

Comment on lines 463 to 466
## Copyright
<!-- The CIP must be explicitly licensed under acceptable copyright terms. -->

[CC-BY-4.0]: https://creativecommons.org/licenses/by/4.0/legalcode
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
## Copyright
<!-- The CIP must be explicitly licensed under acceptable copyright terms. -->
[CC-BY-4.0]: https://creativecommons.org/licenses/by/4.0/legalcode
## Copyright
This CIP is licensed under [CC-BY-4.0].
[CC-BY-4.0]: https://creativecommons.org/licenses/by/4.0/legalcode

if the CWG agrees of course.

Comment on lines 458 to 461
### Implementation Plan
- [ ] This CIP will be discussed and agreed by the Cardano Certification Working Group where multiple auditors are being represented. This will ensure that certification issuer have agreed on the content and are ready to issue certificates under this format.

- [ ] This CIP will be presented to the IOG Lace team and Cardano Fans team which will display certificates for DApps. This is to ensure that the format contains all the necessary information for a DApp store or an aggregator to correctly link and display a certificate from the on-chain and off-chain metadata.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should check the first box before submitting. Also, shouldn't we full-fill the second condition?

Comment on lines 446 to 447
**Updates to registration entries**
This would have required DApp owner or certification issuers to add certificates to every registration making it harder to maintain a shared state between all stakeholders. The chosen design requires to follow the chain to discover the certificates which should be expected from stakeholders.
Copy link
Contributor

Choose a reason for hiding this comment

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

These two lines will appear after one another in HTML, and therefore we should have a separator. A colon maybe?

Comment on lines 430 to 432
| Certification issuer | URL | Contact email | Certification levels | Public key |
|----------------------|----- |--------------- |----------------------|------------ |
| Example Ltd. | http://example.com | [email protected] | 1,2 | EXAMPLEPUBLICKEYPGPED25519 (PGP, ed25519) |
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
| Certification issuer | URL | Contact email | Certification levels | Public key |
|----------------------|----- |--------------- |----------------------|------------ |
| Example Ltd. | http://example.com | [email protected] | 1,2 | EXAMPLEPUBLICKEYPGPED25519 (PGP, ed25519) |
| Certification issuer | URL | Contact email | Certification levels | Cardano address |
|----------------------|----- |--------------- |----------------------|------------ |
| Example Ltd. | http://example.com | [email protected] | 1,2 | EXAMPLEADDRESS |

@RSoulatIOHK
Copy link
Collaborator Author

Todo:

Took into consideration most of the comments.
Added description in the json schema.
@RSoulatIOHK RSoulatIOHK mentioned this pull request Sep 5, 2023
2 tasks
@RSoulatIOHK
Copy link
Collaborator Author

CIP-0096 is on the CIP editor repo now:
cardano-foundation/CIPs#499

@RSoulatIOHK RSoulatIOHK closed this Sep 6, 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.

4 participants