Signature spec updates #158

gokarnm · 2022-05-16T20:16:11Z

Updates Notary v2 signature envelope and JWS envelope in main, based subset of updates in cose-envelope branch.
JWS envelope does not rely on JWT defined claims.
Payload is standardized so we can use the same payload across different signature envelope formats.
Incorporated some feedback from Address review comments on #148 for JWS #156 by @letmaik, JWT related updates are discarded.
I'll address the merge conflict at the end after other review feedback is addressed.

Signed-off-by: Milind Gokarn [email protected]

cc: @shizhMSFT @roywill @letmaik @priteshbandi @rgnote for review

* cbor payload * cose envelope * finalize payload * remove JWS * refine notations * refine datetime Signed-off-by: Shiwei Zhang <[email protected]> Merging into the remote-siging branch for further iteration, addressing the PR feedback with additional PRs into this branch.

Signed-off-by: Steve Lasker <[email protected]>

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 Signed-off-by: Milind Gokarn <[email protected]>

Signed-off-by: Milind Gokarn <[email protected]>

jondonas · 2022-05-16T21:58:11Z

signature-envelope-jws.md

+    "protected": "<Base64Url(ProtectedHeaders)>",
+    "header": {
+        "timestamp": "<Base64(TimeStampToken)>",
+        "x5c": ["<Base64(DER(leafCert))>", "<Base64(DER(intermediateCACert))>", "<Base64(DER(rootCert))>"]


Is cert ordering in the chain suggested or required?

It's in order from leaf to root, will clarify in description.

Why do we need the root in the envelope? Isn't it redundant?

The root cert isn't generally required for verification. The intent is for signature producers to always include full chain, which will be validated by signing tool (notation), so that consumers can set trusted root as root cert (our recommendation). We want to avoid partial chains in the envelope as tooling cannot determine if the partial chain was only one level below the root. We can relax this requirement if there is a pressing reason in future.

signature-envelope-jws.md

signature-specification.md

signature-envelope-jws.md

gokarnm · 2022-05-18T06:40:55Z

@shizhMSFT would like to get your feedback before finalizing this spec.

signature-envelope-jws.md

shizhMSFT · 2022-05-18T15:14:45Z

signature-specification.md

 - **`subject`** (*descriptor*): A REQUIRED artifact descriptor referencing the signed manifest, including, but not limited to image manifest, image index, oras-artifact manifest.
 - **`annotations`** (*string-string map*): This REQUIRED property contains metadata for the artifact manifest.
  It is being used to store information about the signature.
  Keys using the `io.cncf.notary` namespace are reserved for use in Notary and MUST NOT be used by other specifications.
-  - **`io.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.
+  - **`io.cncf.notary.x509.fingerprint.sha256`**: A REQUIRED annotation whose value contains the list of SHA-256 fingerprint of signing certificate and certificate chain used for signature generation.


How about io.cncf.notary.x5t#S256 in RFC7515 4.1.8?

x5t#S256 looks more common and shorter and it also defines the encoding (i.e. a base64url-encoded SHA-256 thumbprint). However, it seems only defines the cert for the signing key and is not a list.

Renamed to io.cncf.notary.x509chain.thumbprint#S256, let me know if you have another suggestion.

signature-specification.md

shizhMSFT · 2022-05-18T15:28:48Z

signature-specification.md


-### Unsigned Attributes
+- Any claims MUST NOT be stored/appended in the payload itself, as the payload is only parsed and processed once the signature has been verified (signature is valid, and from a trusted key) and trust is established.


This sentence is ambiguous for me. The intention seems to be "the claim set MUST NOT be tampered once verified" but it is hard for me to get from it. Request for rewording.

I've reworded to

Any metadata that is used to verify the payload itself, and establish trust MUST be stored separately from the payload itself, as signed attributes or claims. Such metadata MUST NOT be stored/appended in the payload, as the payload is only parsed and processed once the signature has been verified and trust is established.

shizhMSFT · 2022-05-18T15:31:41Z

signature-specification.md

-### Unsigned Attributes
+- Any claims MUST NOT be stored/appended in the payload itself, as the payload is only parsed and processed once the signature has been verified (signature is valid, and from a trusted key) and trust is established.
+- Specific claims can be either required or optional.
+- Claims that if present, MUST be processed by a verifier MUST be marked as critical.


This sentence is ambiguous for me. Adding a pair of brackets enables me understand it faster:

Claims (that if present, MUST be processed by a verifier) MUST be marked as critical.

Request for rewording.

How about

Claims that are present and MUST be processed by a verifier MUST be marked as critical.

Updated to

Claims that MUST be processed by a verifier MUST be marked as critical. Some claims may be optional and critical, i.e. they MUST be processed by a verifier only if they are present.

shizhMSFT · 2022-05-18T15:33:05Z

signature-specification.md

+- **Signing time**: The time at which the signature was generated. Though this claim is signed by the signing key, it’s considered unauthenticated as a signer can modify local time and manipulate this claim. More details [here](#signing-time).
+- **Expiry** (critical): An OPTIONAL claim that provides a “best by use” time for the artifact, as defined by the signer. More details [here](#expiry).
+- **Content type** (critical): A REQUIRED clain that indicates the content type of the payload. Notary currently a payload that contains the OCI descriptor of a subject manifest as the payload, the supported value is `application/vnd.cncf.notary.payload.v1+json`, other payload types MAY be supported in future.
+- **Client identifier**: The version of a client (e.g. Notation) that produced the signature. This is an OPTIONAL claim. It uses the following format `{client}/{version}` e.g. “notation/1.0.0”. This claim in intended to be used for diagnostic and troubleshooting purposes.


Curious if v is allowed in {version} like notation/v1.0.0?

Should this be a signed attribute? Since it is for debugging purposes only, can it be unsigned?

Curious if v is allowed in {version} like notation/v1.0.0?

Yes, I'll update this section that its an opaque string, and only make a suggestion about the format to be used (formatting is not validated).

Should this be a signed attribute? Since it is for debugging purposes only, can it be unsigned?

Changed this to an unsigned attribute, as this attribute is informational, and cannot be a signed attribute when a envelope-generator plugin is used.

signature-specification.md

rgnote · 2022-05-18T21:20:47Z

signature-specification.md

+- **Signing time**: The time at which the signature was generated. Though this claim is signed by the signing key, it’s considered unauthenticated as a signer can modify local time and manipulate this claim. More details [here](#signing-time).
+- **Expiry** (critical): An OPTIONAL claim that provides a “best by use” time for the artifact, as defined by the signer. More details [here](#expiry).
+- **Content type** (critical): A REQUIRED clain that indicates the content type of the payload. Notary currently a payload that contains the OCI descriptor of a subject manifest as the payload, the supported value is `application/vnd.cncf.notary.payload.v1+json`, other payload types MAY be supported in future.
+- **Client identifier**: The version of a client (e.g. Notation) that produced the signature. This is an OPTIONAL claim. It uses the following format `{client}/{version}` e.g. “notation/1.0.0”. This claim in intended to be used for diagnostic and troubleshooting purposes.


Should this be a signed attribute? Since it is for debugging purposes only, can it be unsigned?

signature-envelope-jws.md

rgnote · 2022-05-18T21:45:54Z

signature-envelope-jws.md

+    "protected": "<Base64Url(ProtectedHeaders)>",
+    "header": {
+        "timestamp": "<Base64(TimeStampToken)>",
+        "x5c": ["<Base64(DER(leafCert))>", "<Base64(DER(intermediateCACert))>", "<Base64(DER(rootCert))>"]


Why do we need the root in the envelope? Isn't it redundant?

signature-envelope-jws.md

letmaik · 2022-05-20T10:10:50Z

signature-envelope-jws.md

+```
+
+- **`timestamp`** (*string*): This OPTIONAL property is used to store time stamp token.
+  Only [RFC3161]([rfc3161](https://datatracker.ietf.org/doc/html/rfc3161#section-2.4.2)) compliant TimeStampToken are supported.


I'm assuming ASN.1 DER encoded, then base64 encoded. Definitely has to be spelled out.
Also, RFC3161 signs an opaque datum, typically a hash of something. So this parameter definition also has to define what is actually being sent to the TSA for signing.

That's a very good point. I'll add a TODO here as this requires some more work.

signature-envelope-jws.md

Signed-off-by: Milind Gokarn <[email protected]>

gokarnm · 2022-05-26T06:51:43Z

Addressed review feedback
Updated EC 512 to EC 521 to address feedback in notation-go PR 46

qmuntal · 2022-05-26T07:12:20Z

@gokarnm this PR changes some pieces of the signature spec that affect the wording of the plugin spec. The plugin spec should also be updated either in this PR or in a follow up. Examples:

keySpec EC_512 is mentioned several times:
https://github.com/notaryproject/notaryproject/blob/cf6113ce44c25f89de67d62feebba67e2be7505b/specs/plugin-extensibility.md?plain=1#L278
payload and envelope types:
https://github.com/notaryproject/notaryproject/blob/cf6113ce44c25f89de67d62feebba67e2be7505b/specs/plugin-extensibility.md?plain=1#L323

gokarnm · 2022-05-26T15:37:45Z

@gokarnm this PR changes some pieces of the signature spec that affect the wording of the plugin spec. The plugin spec should also be updated either in this PR or in a follow up. Examples:
* keySpec `EC_512` is mentioned several times:
  https://github.com/notaryproject/notaryproject/blob/cf6113ce44c25f89de67d62feebba67e2be7505b/specs/plugin-extensibility.md?plain=1#L278

* payload and envelope types:
  https://github.com/notaryproject/notaryproject/blob/cf6113ce44c25f89de67d62feebba67e2be7505b/specs/plugin-extensibility.md?plain=1#L323

@qmuntal, totally agree, I want to merge this PR to unblock implementation, and address plugin spec updates as separate one. I was aware of the keyspec updates, thanks for pointing the payload type inconsistency.

gokarnm · 2022-05-26T15:49:40Z

To be addressed in a subsequent PR

gokarnm · 2022-05-26T16:38:02Z

@shizhMSFT @roywill @letmaik @qmuntal @rgnote @priteshbandi @jondonas thanks for all the great feedback! I've addressed most of it. I've resolved ones which are straightforward, and kept more contentious conversations open if you have more feedback. Would appreciate your review as this is blocking implementation.

priteshbandi

LGTM

rgnote · 2022-05-26T17:43:55Z

LGTM

letmaik · 2022-05-27T08:55:29Z

signature-envelope-jws.md

+{
+    "alg": "PS384",
+    "cty": "application/vnd.cncf.notary.payload.v1+json",
+    "io.cncf.notary.signingTime": "2022-04-06 07:01:20Z",


Not RFC 3339, missing the T.

RFC 3339 allows a space separator [1]. I'll update the example though to commonly used representation.

https://datatracker.ietf.org/doc/html/rfc3339#section-5.6

NOTE: ISO 8601 defines date and time separated by "T".
Applications using this syntax may choose, for the sake of
readability, to specify a full-date and full-time separated by
(say) a space character.

letmaik · 2022-05-27T09:02:05Z

signature-envelope-jws.md

+```jsonc
+{
+    "x5c": ["<Base64(DER(leafCert))>", "<Base64(DER(intermediateCACert))>", "<Base64(DER(rootCert))>"],
+    "io.cncf.notary.timestamptoken": "<Base64(TimeStampToken)>",


Suggested change

"io.cncf.notary.timestamptoken": "<Base64(TimeStampToken)>",

"io.cncf.notary.timestamp": "<Base64(DER(TimeStampToken))>",

Got some more feedback on this, plan to rename to io.cncf.notary.timestampSignature to indicate it's a countersignature.

Isn't the value of RFC3161 TimeStampToken already DER encoded, do we need to specify it additionally here?

I guess it's confusing because we mention DER encoding for the cert chain (x5c), which could originally be in PEM or DER format.

TimeStampToken is defined in terms of ASN.1, and section 3 specifies encodings for multiple transports like email, files, HTTP. All of them use DER encoding, and then some of them base64 encode after that. So yes, it's very likely DER already, but given that we define a new context I think we need to specify the ASN.1 encoding as well.

letmaik · 2022-05-27T09:03:52Z

signature-envelope-jws.md

+    "payload": "<Base64Url(JWSPayload)>",
+    "protected": "<Base64Url(ProtectedHeaders)>",
+    "header": {
+        "io.cncf.notary.timestamp": "<Base64(TimeStampToken)>",


Suggested change

"io.cncf.notary.timestamp": "<Base64(TimeStampToken)>",

"io.cncf.notary.timestamp": "<Base64(DER(TimeStampToken))>",

Time stamp token is a CMS signed-data and is BER encoded. If we require DER here, the client needs to convert the BER encoded timestamp token to DER encoded. Is it intended?

You may be right, I was looking at RFC 3161 which only says DER, not BER. Let's postpone any timestamp-related changes to a follow-up PR.

Time-Stamp Protocol via HTTP in RFC 3161 specifies that the TSA response is ASN.1 DER-encoded. Validated with a freetsa.org example here.

So the current content "io.cncf.notary.timestamp": "<Base64(TimeStampToken)>" is accurate, we don't specifically require DER, it's whichever format TSA servers respond with. I'm going to defer changes to timestamp related details, we can refine when we do the implementation. From the signature format's perspective it's an opaque token that is separately validated.

letmaik · 2022-05-27T09:13:26Z

signature-specification.md

-1. Descriptor MUST contain `mediaType`, `digest`, `size` fields.
-1. Descriptor MAY contain `annotations` and if present it MUST follow the [annotation rules](https://github.com/opencontainers/image-spec/blob/main/annotations.md#rules). Notary v2 uses annotations for storing both Notary specific and user defined signed attributes. The prefix `io.cncf.notary` in annotation keys is reserved for use in Notary v2 and MUST NOT be used outside this specification.
-1. Descriptor MAY contain `artifactType` field for artifact manifests, or the `config.mediaType` for `oci.image` based manifests.
+- `targetArtifact` : Required property whose value is the descriptor of the target artifact manifest that is being signed. Both [OCI descriptor][oci-descriptor] and [ORAS artifact descriptors][artifact-descriptor] are supported.


How do you know which descriptor type is used? If the descriptor was signed directly, then cty of the envelope would indicate it, but now with the additional wrapping it seems this information is gone. Maybe I'm missing something though.

Line 87-89 provide the requirements for the descriptor. Notary only depends on attributes common to both OCI and ORAS descriptor - digest, size, mediaType and annotations.

…ignature` Signed-off-by: Milind Gokarn <[email protected]>

Signed-off-by: Milind Gokarn <[email protected]>

shizhMSFT · 2022-05-30T03:51:37Z

signature-envelope-jws.md

+1. Compute the Base64Url value of ProtectedHeaders, this is the value of `protected` property in the signature envelope.
+1. Compute the Base64Url value of JWSPayload, this is the value of `payload` property in the signature envelope.
+1. Build *JWS Signing Input* to be signed by concatenating the values generated in step 1 and step 2 using '.'
+`ASCII(BASE64URL(UTF8(ProtectedHeaders)) ‘.’ BASE64URL(JWSPayload))`


The Base64Url here includes paddings or not? It seems Base64Url is never defined in this documentation.

Good point, clarified that JWS uses base64url without padding as defined in Base64url Encoding in RFC 7515 section 2.

shizhMSFT · 2022-05-30T04:10:09Z

signature-envelope-jws.md

+    "payload": "<Base64Url(JWSPayload)>",
+    "protected": "<Base64Url(ProtectedHeaders)>",
+    "header": {
+        "io.cncf.notary.timestamp": "<Base64(TimeStampToken)>",


Time stamp token is a CMS signed-data and is BER encoded. If we require DER here, the client needs to convert the BER encoded timestamp token to DER encoded. Is it intended?

shizhMSFT · 2022-05-30T05:01:37Z

signature-specification.md

- **TSA counter signature** : The time stamp token generated for a given signature. Only [RFC3161](ietf-rfc3161) compliant TimeStampToken are supported. This is an OPTIONAL attribute.
+- **Certificate Chain**: This is a REQUIRED attribute that contains the ordered list of X.509 public certificates associated with the signing key used to generate the signature. The ordered list starts with the signing certificates, any intermediate certificates and ends with the root certificate. The certificate chain MUST be authenticated against a trust store as part of signature validation. Specific requirements for the certificates in the chain are provided [here](#certificate-requirements).
+- **Timestamp signature** : An OPTIONAL counter signature which provides [trusted timestamp](#signing-time)e.g. Time Stamp Authority (TSA) generated timestamp signature. Only [RFC3161](ietf-rfc3161) compliant TimeStampToken are currently supported.
+- **Signing Agent**: An OPTIONAL claim that provides the identifier of the software (e.g. Notation) that produced the signature on behalf of the user. It is an opaque string set by the software that produces the signature. It's intended primarily for diagnostic and troubleshooting purposes, this attribute is unsigned, the verifier MUST NOT validate formatting, or fail validation based on the content of this claim. The suggested format is one or more tokens of the form `{id}/{version}` containing identifier and version of the software, seperated by spaces. E.g. “notation/1.0.0”, “notation/1.0.0 com.example.nv2plugin/0.8”.


Should this part be signed?

Signing Agent could be signed, which makes it tamper proof. But it does not play a part in signature verification, only diagnostics, so it's not required to be signed.
Notation's default signing and Signature Generator plugins can support this as a signed attribute, but Envelope generator plugin can't. I went with unsigned for simplicity, but we can change it such that it can be a signed attribute where possible if there is a strong case for it.

Signed-off-by: Milind Gokarn <[email protected]>

shizhMSFT

LGTM

gokarnm · 2022-06-01T18:26:29Z

Resolved merge conflicts.

SteveLasker · 2022-06-02T16:36:21Z

signature-envelope-jws.md

+
+```jsonc
+{
+    "artifactType": "application/vnd.cncf.notary.signature",


need version here: application/vnd.cncf.notary.signature.v2

shizhMSFT and others added 5 commits March 25, 2022 11:36

Support for JWT and COSE envelopes (#145)

33d1ee9

Signed-off-by: Steve Lasker <[email protected]>

Baseline for signature spec update.

f705cb8

Signed-off-by: Milind Gokarn <[email protected]>

Updated JWS signature envelope payload and headers.

59ab1b7

Signed-off-by: Milind Gokarn <[email protected]>

gokarnm requested a review from shizhMSFT May 16, 2022 20:16

jondonas reviewed May 16, 2022

View reviewed changes

priteshbandi reviewed May 16, 2022

View reviewed changes

gokarnm commented May 16, 2022

View reviewed changes

signature-specification.md Outdated Show resolved Hide resolved

gokarnm commented May 16, 2022

View reviewed changes

signature-specification.md Outdated Show resolved Hide resolved

priteshbandi reviewed May 16, 2022

View reviewed changes

signature-specification.md Outdated Show resolved Hide resolved

signature-specification.md Outdated Show resolved Hide resolved

gokarnm changed the title ~~[DRAFT] Signature spec updates~~ Signature spec updates May 16, 2022

roywill reviewed May 17, 2022

View reviewed changes

signature-envelope-jws.md Outdated Show resolved Hide resolved

roywill reviewed May 17, 2022

View reviewed changes

signature-envelope-jws.md Outdated Show resolved Hide resolved

shizhMSFT reviewed May 18, 2022

View reviewed changes

rgnote reviewed May 18, 2022

View reviewed changes

letmaik reviewed May 20, 2022

View reviewed changes

signature-envelope-jws.md Outdated Show resolved Hide resolved

letmaik reviewed May 20, 2022

View reviewed changes

signature-envelope-jws.md Outdated Show resolved Hide resolved

letmaik reviewed May 20, 2022

View reviewed changes

qmuntal suggested changes May 23, 2022

View reviewed changes

signature-envelope-jws.md Show resolved Hide resolved

gokarnm mentioned this pull request May 24, 2022

Implement generate-envelope workflow notaryproject/notation-go#47

Merged

Addressed PR feedback. Updated EC512 to EC521.

868585e

Signed-off-by: Milind Gokarn <[email protected]>

gokarnm requested a review from shizhMSFT May 26, 2022 06:51

qmuntal approved these changes May 26, 2022

View reviewed changes

iamsamirzon mentioned this pull request May 26, 2022

Release Candidate Signature Format notaryproject/roadmap#5

Closed

priteshbandi requested changes May 26, 2022

View reviewed changes

rgnote approved these changes May 26, 2022

View reviewed changes

letmaik suggested changes May 27, 2022

View reviewed changes

gokarnm added 2 commits May 27, 2022 16:32

Renamed io.cncf.notary.timestamptoken to `io.cncf.notary.timestampS…

ca186b4

…ignature` Signed-off-by: Milind Gokarn <[email protected]>

RFC 3339 time formatting

5e1ccf6

Signed-off-by: Milind Gokarn <[email protected]>

shizhMSFT reviewed May 30, 2022

View reviewed changes

Define Base64Url encoding for JWS

2378235

Signed-off-by: Milind Gokarn <[email protected]>

gokarnm requested a review from shizhMSFT May 30, 2022 23:25

roywill approved these changes Jun 1, 2022

View reviewed changes

shizhMSFT approved these changes Jun 1, 2022

View reviewed changes

Merge branch 'main' into signature-spec

49e1687

SteveLasker reviewed Jun 2, 2022

View reviewed changes

SteveLasker merged commit 9cfd6a9 into notaryproject:main Jun 2, 2022

gokarnm deleted the signature-spec branch June 5, 2022 22:05

qmuntal mentioned this pull request Jun 20, 2022

Update data model with latest notation spec notaryproject/notation-go#62

Closed

letmaik mentioned this pull request Jun 22, 2022

Address review comments on #148 for JWS #156

Closed

This was referenced Jul 13, 2022

Signature Payload Format notaryproject/roadmap#2

Closed

Signature Manifest Format notaryproject/roadmap#3

Closed

Support for additional Signature Formats in the signature manifest notaryproject/roadmap#4

Closed


		### Unsigned Attributes
		- Any claims MUST NOT be stored/appended in the payload itself, as the payload is only parsed and processed once the signature has been verified (signature is valid, and from a trusted key) and trust is established.

	"io.cncf.notary.timestamptoken": "<Base64(TimeStampToken)>",
	"io.cncf.notary.timestamp": "<Base64(DER(TimeStampToken))>",

	"io.cncf.notary.timestamp": "<Base64(TimeStampToken)>",
	"io.cncf.notary.timestamp": "<Base64(DER(TimeStampToken))>",

Signature spec updates #158

Signature spec updates #158

Conversation

gokarnm commented May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gokarnm commented May 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gokarnm commented May 26, 2022

qmuntal commented May 26, 2022

gokarnm commented May 26, 2022 • edited Loading

gokarnm commented May 26, 2022

gokarnm commented May 26, 2022

priteshbandi left a comment

Choose a reason for hiding this comment

rgnote commented May 26, 2022

Choose a reason for hiding this comment

gokarnm May 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

gokarnm commented Jun 1, 2022

Choose a reason for hiding this comment

gokarnm commented May 16, 2022 •

edited

Loading

gokarnm commented May 26, 2022 •

edited

Loading

gokarnm May 28, 2022 •

edited

Loading