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

Verification Plugin spec #165

Merged
merged 14 commits into from
Jul 25, 2022
Merged

Conversation

gokarnm
Copy link
Contributor

@gokarnm gokarnm commented Jul 6, 2022

  • Added verification plugin spec with new verify-signature plugin command.
  • Updated Notary v2 Signature spec and JWS signature spec for new attributes to support Signing Scheme and Verification plugin

Signed-off-by: Milind Gokarn gokarnm@amazon.com

Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
@gokarnm gokarnm requested review from shizhMSFT and a team July 6, 2022 05:08
@gokarnm gokarnm changed the title [DRAFT] Verification Plugin and Signing Scheme spec Verification Plugin and Signing Scheme spec Jul 6, 2022
@gokarnm
Copy link
Contributor Author

gokarnm commented Jul 6, 2022

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

Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
signing-scheme.md Outdated Show resolved Hide resolved
specs/plugin-extensibility.md Outdated Show resolved Hide resolved
Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
signing-scheme.md Outdated Show resolved Hide resolved
2. Validate that plugin version is greater than or equal to *Verification plugin minimum version*
3. Fail signature verification if these validations fail
3. Validate if plugin capabilities contains any `SIGNATURE_VERIFIER` capabilities
1. Fail signature verification if a matching plugin with `SIGNATURE_VERIFIER` capability cannot be found. If signature envelope contains the *Verification Plugin Name* attribute, include it as a hint in error response.
Copy link
Contributor

Choose a reason for hiding this comment

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

If signature envelope contains the Verification Plugin Name attribute, include it as a hint in error response.

If plugin is invoked, doesnt that means signature envelope contains the Verification Plugin Name attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, updated the language Verification Plugin Name attribute will be always present.

Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
specs/plugin-extensibility.md Outdated Show resolved Hide resolved
specs/plugin-extensibility.md Show resolved Hide resolved
specs/plugin-extensibility.md Show resolved Hide resolved
Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
signature-envelope-jws.md Outdated Show resolved Hide resolved
signing-scheme.md Outdated Show resolved Hide resolved
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.

I'm all for extensibility and flexibility, enabling various scenarios that don't require notary maintainers to be involved. The remote signing extensibility has proven to work really well, enabling various cloud providers to meet their product goals for how they support remote signing/key management solutions. This was a key for Notary v2 scenarios and requirements

I have general concerns about a verification plugin that could limit the portability of signatures, as this could be counter to our usability and platform/vendor neutrality goals.

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

See [Extended attributes for *Notation* Plugins](./signature-specification.md#extended-attributes-for-notation-plugins) for detailed description og these headers.

- **`io.cncf.notary.verificationPlugin`**(*string*)(critical): An OPTIONAL header that specifies the name of the verification plugin that MAY be used to verify the signature.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, why do we think we need this?
The notary client should be able to verify any signature format supported. I haven't seen where we need plugins for verification.


#### Extended attributes

Implementations of Notary v2 signature spec MAY include additional signed attributes in the signature envelope. These attributes MAY be marked critical, i.e. the attribute MUST be understood and processed by a verifier, unknown critical attributes MUST cause signature verification to fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like it would impose instability and inconsistency.
A primary goal of Notary v2 is cloud/location independence. If a user copies an image from MAR to their ecr, they shouldn't need any special Microsoft stuff to validate it.
I'm guessing you have some ideas around new headers. Can we discuss those and find a platform neutral way?

Copy link
Contributor

@sajayantony sajayantony Jul 12, 2022

Choose a reason for hiding this comment

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

Are these the critical headers we are discussing here -

Given the statement - basically it means that a signature with a critical header will put a requirement on the client environment to have the plugin available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the links point to how criticality of signed attributes is implemented in JWS and COSE. If an extended attribute is marked critical (not all extended attributes need to be critical) the verifier must process it either using a plugin or equivalent verification logic.

signature-specification.md Outdated Show resolved Hide resolved
signature-specification.md Show resolved Hide resolved
Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
- **[`crit`](https://datatracker.ietf.org/doc/html/rfc7515#section-4.1.11)**(*array of strings*): This OPTIONAL header lists the headers that implementation MUST understand and process. It MUST only contain headers apart from registered headers (e.g. `alg`, `cty`) in JWS specification, therefore this header is only present when the optional `io.cncf.notary.expiry` header is present in the protected headers collection.
If present, the value MUST be `["io.cncf.notary.expiry"]`.
- **`io.cncf.notary.signingScheme`**(*string*)(critical): This REQUIRED header specifies the [Notary v2 Signing Scheme](./signing-scheme.md) used by the signature. Supported values are `notary.default.x509` and `notary.signingAuthority.x509`.
- **`io.cncf.notary.signingTime`**(*string*): This header specifies the time at which the signature was generated. This is an untrusted timestamp, and therefore not used in trust decisions. Its value is a [RFC 3339][rfc3339] formatted date time, the optional fractional second ([time-secfrac][rfc3339][[1](https://datatracker.ietf.org/doc/html/rfc3339#section-5.3)]) SHOULD NOT be used. This claim is REQUIRED and only valid when signing scheme is `notary.default.x509`.
Copy link
Contributor

Choose a reason for hiding this comment

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

r/ .default

@@ -126,9 +126,24 @@ Notary v2 requires the signature envelope to support the following signed attrib

#### Standard attributes

- **Signing time**: A REQUIRED claim that indicates 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).
- **Signing Scheme** (critical): A REQUIRED claim that defines the [Notary v2 Signing Scheme](./signing-scheme.md) used by the signature. This attribute dictates the rest of signature schema - the set of signed and unsigned attributes to be included in the signature. Supported values are `notary.default.x509` and `notary.signingAuthority.x509`.
- **Signing Time**: A claim that indicates 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). This claim is REQUIRED and only valid when signing scheme is `notary.default.x509` .
Copy link
Contributor

Choose a reason for hiding this comment

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

r/ .default

Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
@gokarnm gokarnm changed the title Verification Plugin and Signing Scheme spec Verification Plugin spec Jul 18, 2022
@gokarnm
Copy link
Contributor Author

gokarnm commented Jul 18, 2022

Removed Signing scheme related spec updates in this PR which is now a separate PR (#175).

Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
Signed-off-by: Milind Gokarn <gokarnm@amazon.com>
@gokarnm
Copy link
Contributor Author

gokarnm commented Jul 22, 2022

Resolved merge conflicts.

@SteveLasker
Copy link
Contributor

Thanks @gokarnm for the latest updates.
My LGTM still applies, and believe we should merge with the understanding that we should have time before rc.1 to test any implementation and verify we're happy with the results. This is a baseline to iterate and learn.

Copy link
Contributor

@NiazFK NiazFK left a comment

Choose a reason for hiding this comment

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

LGTM

@NiazFK NiazFK merged commit c4400eb into notaryproject:main Jul 25, 2022
@dtzar dtzar modified the milestones: RC-1, alpha-3 Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.