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

COSE Signature Envelope #139

Merged
merged 11 commits into from
Mar 25, 2022
Merged

Conversation

shizhMSFT
Copy link
Contributor

@shizhMSFT shizhMSFT commented Mar 2, 2022

Replace JWS with COSE for the envelope format.

Highlight: Unlike JWS, alg is not a required header in COSE. Therefore, we can remove it completely, making the signature envelope more secure.

Resolves #117

Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
@shizhMSFT shizhMSFT changed the title From JSON/JOSE to CBOR/COSE Extended Signature Envelope to COSE Mar 2, 2022
@shizhMSFT shizhMSFT changed the title Extended Signature Envelope to COSE COSE Signature Format Mar 2, 2022
@shizhMSFT shizhMSFT requested review from SteveLasker and gokarnm March 2, 2022 00:28
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
@shizhMSFT shizhMSFT changed the title COSE Signature Format COSE Signature Envelope Mar 2, 2022
Signed-off-by: Shiwei Zhang <[email protected]>
@shizhMSFT shizhMSFT mentioned this pull request Mar 9, 2022
The certificate containing the public key corresponding to the key used to digitally sign the JWS MUST be the first certificate.
- **`x5chain`** (*array of byte strings*): This REQUIRED property (label: `33` by [IANA](https://www.iana.org/assignments/cose/cose.xhtml#header-parameters)) contains the list of X.509 certificate or certificate chain ([RFC5280](https://datatracker.ietf.org/doc/html/rfc5280)) corresponding to the key used to digitally sign the COSE.
The certificate containing the public key corresponding to the key used to digitally sign the COSE MUST be the first certificate.
Optionally, this header can be presented in the protected header.
Copy link
Contributor

Choose a reason for hiding this comment

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

If x5chain is not protected, should require that x5t/34 is. There may also be value in simply requiring that x5chain is protected. This guards against a potential cert substitution attack, at the expense of requiring a larger signature envelope size because the header can't be stripped in environments where the value is known externally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@priteshbandi Should we move Certificates attribute from Unsigned Attributes to Signed Attributes?

Copy link
Contributor

Choose a reason for hiding this comment

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

@briankr-ms , @shizhMSFT what's the call here? I can make changes if we can conclude on a decision.

Copy link
Contributor

Choose a reason for hiding this comment

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

@briankr-ms could you provide more details for cert substitution attack. Notation will use x5chain in conjunction with trust store configured by the user. As part of verification, the cert chain is checked to terminate in one of the certs in trust store. @shizhMSFT are we using x5t/thumbprint in any other context, other than signature manifest attributes, for matching signature to policy for evaluation?

Copy link
Contributor

Choose a reason for hiding this comment

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

@shizhMSFT The certs in chain are already signed by the next cert in the chain, and is expected to be authenticated against the trust store. Open to discussing if moving it to signed attributes protects us against certain attacks.

@roywill
Copy link

roywill commented Mar 14, 2022

  • org.cncf.notary.x509certs.fingerprint.sha256: A REQUIRED annotation whose value contains the list of SHA-256 fingerprint of signing certificate and certificate chain used for signature generation.

If we adopt\moving to a Ledger type system, why would we keep the certificate chain anywhere? Is this a hold over from threat?


Refers to: signature-specification.md:25 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)

@roywill
Copy link

roywill commented Mar 14, 2022

        "mediaType": "application/jose+json",

Why Jose?


Refers to: signature-specification.md:33 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)

@roywill
Copy link

roywill commented Mar 14, 2022

The Notary v2 signature artifact's org.cncf.notary.x509certs.fingerprint.sha256 annotations key MUST contain the list of SHA-256 fingerprints of certificate and certificate chain used for signing.

Why do we need this?


Refers to: signature-specification.md:59 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)

@roywill
Copy link

roywill commented Mar 14, 2022

  • Signed attributes v: The signature metadata that is integrity protected - e.g. signature expiration time, signing time, etc.

Why do we keep the expiration time in its own field. Is this part of the support for extending life based on Secure TimeStamp data?


Refers to: signature-specification.md:67 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)

@roywill
Copy link

roywill commented Mar 14, 2022

  • Unsigned attributes u: This OPTIONAL property represents signature metadata that is not integrity protected - e.g. timestamp, certificates, etc.

In the case of our other scenarios, the key thing that we store in unprotected headers is another signed message. How much of this is true here?


Refers to: signature-specification.md:68 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)


Edit from @SteveLasker

Updated to reflect:

  • Unsigned attributes u: This OPTIONAL property represents signature metadata that is not integrity protected by this envelope- e.g. timestamp, certificates, etc.
    We anticipate all unsigned attributes contain signed content.

@roywill
Copy link

roywill commented Mar 14, 2022

  "io.wabbit-networks.buildId": "123"  // user defined signed attribute.

It would be nice to show something under io.cncf.notary


Refers to: signature-specification.md:91 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)


@SteveLasker Commenting in an edit:

We could switch to a CNCF project. Within the notary project, we use two companies as representative users

  • Wabbit Networks is a small ISV, shipping their software
  • Acme Rockets is a small consumer, that consumes public software, such as the net-monitor:v1 image from Wabbit Networks.
    This is just to be consistent with those users.

@roywill
Copy link

roywill commented Mar 14, 2022

"mediaType": "sbom/example",

Media type is usually text\plain, multipart\related, image\jpg .

Did we really define a type sbom/example?


Refers to: signature-specification.md:98 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)


Inlined edit from @SteveLasker

To avoid referencing specific projects, we use [artifactType]/example

@roywill
Copy link

roywill commented Mar 14, 2022

Its value should be numeric representing the number of seconds (not milliseconds) since Epoch.

Is it the time it was signed or the time the payload was created? Thus in a disaster scenario when things come back up is it the time the "operation happened" or the time when we requested a signature?


Refers to: signature-specification.md:109 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)

@roywill
Copy link

roywill commented Mar 14, 2022

Its value should be numeric representing the number of seconds since Epoch.

Isn't this also expected to be > than Creation Time? Or is it truly free form and thus can be earlier than or equal to creation time?


Refers to: signature-specification.md:111 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)

@roywill
Copy link

roywill commented Mar 14, 2022

  • Certificates: The ordered collection of X.509 certificates with a signing certificate as the first entry.

Why are all the intermediaries here and does it include the root?
What happens if we have to support multiple signatures?


Refers to: signature-specification.md:116 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)

Signed-off-by: Shiwei Zhang <[email protected]>
@shizhMSFT
Copy link
Contributor Author

shizhMSFT commented Mar 16, 2022

@priteshbandi @SteveLasker Could you also take a look at @roywill 's comment since his comment is mainly for the signature specification itself and is not specific to this PR?

Signed-off-by: Shiwei Zhang <[email protected]>
@shizhMSFT
Copy link
Contributor Author

        "mediaType": "application/jose+json",

updated to cose.


In reply to: 1067416112


Refers to: signature-specification.md:33 in 644c8cd. [](commit_id = 644c8cd, deletion_comment = False)

@letmaik
Copy link
Contributor

letmaik commented Mar 17, 2022

Highlight: Unlike JWS, alg is not a required header in COSE. Therefore, we can remove it completely, making the signature envelope more secure.

Keeping alg in the header is useful when relying on key discovery systems that don't bind the alg to the key itself. An example is DID where alg can be included in the JWK object. Since different DID methods have more or less guarantees on the integrity of the resolved DID document (DID documents are unsigned), e.g. when using ledgers vs. fetching a URL, it makes sense to reduce reliance on additional metadata as much as possible.

@shizhMSFT
Copy link
Contributor Author

Highlight: Unlike JWS, alg is not a required header in COSE. Therefore, we can remove it completely, making the signature envelope more secure.

Keeping alg in the header is useful when relying on key discovery systems that don't bind the alg to the key itself. An example is DID where alg can be included in the JWK object. Since different DID methods have more or less guarantees on the integrity of the resolved DID document (DID documents are unsigned), e.g. when using ledgers vs. fetching a URL, it makes sense to reduce reliance on additional metadata as much as possible.

The alg header is OPTIONAL and many COSE implementations still set values to it. Unlike JWS, the verification won't fail if alg does not present.

The array MUST contain `3` (`cty`), and `signingtime`. If `expiry` is presented, the array MUST also contain `expiry`.
- **`cty`** (*string*): The REQUIRED property content-type (label: `3`) is used to declare the media type of the secured content (the payload).
- **`signingtime`** (*datetime*): The REQUIRED property identifies the time at which the signature was generated.
- **`expiry`** (*datetime*): This OPTIONAL property contains the expiration time on or after which the signature must not be considered valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add iss (string) to this list as optional property indicating the issuer of the signature? If it follows a well-known scheme like DID (starting with did:) then it may be used for key discovery, together with kid which is also missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, Notary v2 only supports certs. Other key discovery systems like kid or iss or others are not supported, and are not covered in this PR. This PR focuses on changing what we have for JWS to COSE and nothing else. BTW, iss makes it more like a CWT.

@letmaik Could you follow up #95 or open a new issue in case we need to support other key discovery systems so that the community can review?

@SteveLasker
Copy link
Contributor

@shizhMSFT, you’re correct this PR is focused on COSE and x509. And, we’re looking to use DiD for where to discover the public key. Knowing we’ll add other key and identity types, is there anything we would add now to make that support less of a breaking change?

@SteveLasker
Copy link
Contributor

From today's Notary call, we agreed to keep both the current JWT, adding COSE.

We’re hoping to land the COSE format for the initial release, but we’ve said notary will support multiple signature formats over time, for instance, different governments (China has been the common example, but also support for newer fomats over time). If we add multiples now, we validate the spec supports multiples. If we land COSE for v1, we can pull JWT out, knowing the spec supports multiple formats.

@shizhMSFT shizhMSFT changed the base branch from main to cose-envelope March 25, 2022 15:22
@SteveLasker
Copy link
Contributor

To move the spec forward, aleviating Shiwei to focus on the go-cose work, suggesting we merge this into cose-envelope branch.
Please LGTM as the PR target was changed.

Copy link
Contributor

@SteveLasker SteveLasker left a comment

Choose a reason for hiding this comment

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

LGTM to merge into the cose-envelope branch, and we'll continue to iterate the feedback there.

@gokarnm
Copy link
Contributor

gokarnm commented Mar 25, 2022

LGTM to merge into the cose-envelope branch, to unblock work. @priteshbandi and me have yet to review this PR completely.

@SteveLasker SteveLasker merged commit 3d8617b into notaryproject:cose-envelope Mar 25, 2022
@SteveLasker
Copy link
Contributor

The Notary v2 signature artifact's org.cncf.notary.x509certs.fingerprint.sha256 annotations key MUST contain the list of SHA-256 fingerprints of certificate and certificate chain used for signing.

Why do we need this?

@shizhMSFT, @roywill, @briankr-ms what's the final call here?
Do we need an issue to track this?

@SteveLasker
Copy link
Contributor

Signed attributes v: The signature metadata that is integrity protected - e.g. signature expiration time, signing time, etc.

Why do we keep the expiration time in its own field. Is this part of the support for extending life based on Secure TimeStamp data?

Need a call here: @roywill, @shizhMSFT, @gokarnm

@SteveLasker
Copy link
Contributor

Its value should be numeric representing the number of seconds (not milliseconds) since Epoch.

Is it the time it was signed or the time the payload was created? Thus in a disaster scenario when things come back up is it the time the "operation happened" or the time when we requested a signature?

@shizhMSFT, @gokarnm, @priteshbandi, what would you like to make this?

@SteveLasker
Copy link
Contributor

Its value should be numeric representing the number of seconds since Epoch.

Isn't this also expected to be > than Creation Time? Or is it truly free form and thus can be earlier than or equal to creation time?

@shizhMSFT, @gokarnm, @priteshbandi, same question here ^

@SteveLasker
Copy link
Contributor

@roywill

Certificates: The ordered collection of X.509 certificates with a signing certificate as the first entry.
Why are all the intermediaries here and does it include the root?
What happens if we have to support multiple signatures?

Notary doesn't support multiple signatures in the same payload. You can create multiple detached signatures, each with their own subject and each with their own claim.

However, this does call out "signing certificate", which we've since said is not required. Should this be updated to say root certificate as the first entry?

@@ -17,7 +17,7 @@ The signature manifest has an artifact type that specifies it's a Notary V2 sign

- **`artifactType`** (*string*): This REQUIRED property references the Notary version of the signature: `application/vnd.cncf.notary.v2.signature`.
- **`blobs`** (*array of objects*): This REQUIRED property contains collection of only one [artifact descriptor](https://github.com/oras-project/artifacts-spec/blob/main/descriptor.md) referencing signature envelope.
- **`mediaType`** (*string*): This REQUIRED property contains media type of signature envelope blob. The supported value is `application/jose+json`
- **`mediaType`** (*string*): This REQUIRED property contains media type of signature envelope blob. The supported value is `application/cose`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intent to disregard any optional parameter e.g. application/cose; cose-type="cose-sign1" ? Notation should probably include code-type param to be more specific.

"exp": 1234567891011
/ crit / 2: [
/ cty / 3,
'signingtime',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to use headers of type string instead of int? Do we plan to followup and include these in IANA?

https://www.iana.org/assignments/cose/cose.xhtml#header-parameters

The certificate containing the public key corresponding to the key used to digitally sign the JWS MUST be the first certificate.
- **`x5chain`** (*array of byte strings*): This REQUIRED property (label: `33` by [IANA](https://www.iana.org/assignments/cose/cose.xhtml#header-parameters)) contains the list of X.509 certificate or certificate chain ([RFC5280](https://datatracker.ietf.org/doc/html/rfc5280)) corresponding to the key used to digitally sign the COSE.
The certificate containing the public key corresponding to the key used to digitally sign the COSE MUST be the first certificate.
Optionally, this header can be presented in the protected header.
Copy link
Contributor

Choose a reason for hiding this comment

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

@briankr-ms could you provide more details for cert substitution attack. Notation will use x5chain in conjunction with trust store configured by the user. As part of verification, the cert chain is checked to terminate in one of the certs in trust store. @shizhMSFT are we using x5t/thumbprint in any other context, other than signature manifest attributes, for matching signature to policy for evaluation?

The certificate containing the public key corresponding to the key used to digitally sign the JWS MUST be the first certificate.
- **`x5chain`** (*array of byte strings*): This REQUIRED property (label: `33` by [IANA](https://www.iana.org/assignments/cose/cose.xhtml#header-parameters)) contains the list of X.509 certificate or certificate chain ([RFC5280](https://datatracker.ietf.org/doc/html/rfc5280)) corresponding to the key used to digitally sign the COSE.
The certificate containing the public key corresponding to the key used to digitally sign the COSE MUST be the first certificate.
Optionally, this header can be presented in the protected header.
Copy link
Contributor

Choose a reason for hiding this comment

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

@shizhMSFT The certs in chain are already signed by the next cert in the chain, and is expected to be authenticated against the trust store. Open to discussing if moving it to signed attributes protects us against certain attacks.


```json
**ProtectedHeaders**: Notary v2 supports the following protected headers. Other header fields can be included but will be ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd explicitly mention that alg if included will not be considered by Notation during verification.

Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: Capture the intent for x5chain.

@gokarnm
Copy link
Contributor

gokarnm commented Mar 29, 2022

The Notary v2 signature artifact's org.cncf.notary.x509certs.fingerprint.sha256 annotations key MUST contain the list of SHA-256 fingerprints of certificate and certificate chain used for signing.

Why do we need this?

@shizhMSFT, @roywill, @briankr-ms what's the final call here? Do we need an issue to track this?

Multiple signatures can be associated with and OCI artifact, which are can be queried through the ORAS references API. This field is used to filter down signatures that will be considered for signature verification against the customer configured Trust store.

More context in #131

@gokarnm
Copy link
Contributor

gokarnm commented Mar 29, 2022

Signed attributes v: The signature metadata that is integrity protected - e.g. signature expiration time, signing time, etc.

Why do we keep the expiration time in its own field. Is this part of the support for extending life based on Secure TimeStamp data?

Need a call here: @roywill, @shizhMSFT, @gokarnm

The signature can be generated by ephemeral keys, the optional signature expiry field allows the signatures to be valid beyond key expiry without relying on TSA.

Can be renamed to signature-expiry. Clarify that signature-expiry can be shorter or larger than key expiry.

SteveLasker added a commit that referenced this pull request Nov 22, 2022
Updates to signature specification
* Moves signed attributes out of payload
* Defines new signed attributes and marks them as critical/informational
* Introduces signature scheme to support different trust models
* cbor payload
* cose envelope
* finalize payload
* refine notations
* refine datetime
* Align COSE spec with JWS
* update CBOR spec
* spec: update data type of signingTime to data/time
* Update signature-envelope-cose.md
* Update signature-envelope-cose.md
* update to RFC for the fields of the sig_structure
* update description for field context
* COSE Signature Envelope (#139)
* Support for JWT and COSE envelopes (#145)
* Updates to signature specification (#147)
* Update COSE spec (#179)
* Update signature-envelope-cose.md (#153)
* `cose-envelope`: merge changes from `main` (#177)
* spec: update data type of signingTime to date/time (#199)
* spec: update cose and signature spec according to review comments (#204)

Co-authored-by: letmaik <[email protected]>
Co-authored-by: Quim Muntal <[email protected]>
Co-authored-by: Milind Gokarn <[email protected]>
Co-authored-by: Shiwei Zhang <[email protected]>
Co-authored-by: Pritesh Bandi <[email protected]>
Co-authored-by: Sajay Antony <[email protected]>
Co-authored-by: Brian Krell <[email protected]>
Co-authored-by: Maik Riechert <[email protected]>
Co-authored-by: Thomas Fossati <[email protected]>
Co-authored-by: David Tesar <[email protected]>
Co-authored-by: Yi Zha <[email protected]>
Signed-off-by: Steve Lasker <[email protected]>
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.

Implement COSE as signing envelope
8 participants