docs: cosign upgrade design document #1246
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1246 +/- ##
======================================
Coverage ? 65.58%
======================================
Files ? 109
Lines ? 5579
Branches ? 0
======================================
Hits ? 3659
Misses ? 1565
Partials ? 355 ☔ View full report in Codecov by Sentry. |
| - Bob attempts to deploy a pod from the vetted image that has 1 cosign signature, which is signed with key in CertificateStore `inline-key-build`. Pod FAILS verification and blocked. | ||
| - Bob attempts to deploy a pod from the vetted image that has 2 cosign signatures, which is signed with keys in CertificateStore `inline-key-build` & `inline-key-test`. Pod passes verification and is created. | ||
|
|
||
| > **NOTE**: Based on Cosign's own verification implementation, there does not seem to be a way to enforce that ALL signatures found are valid. This behavior is unchartered as far as we can tell and would require Ratify to determine if we want to support this directly. For that reason, the strict ALL signature scenario is not yet included. |
There was a problem hiding this comment.
I feel it's a valid case since we already support this scenario in notation verifier. The question is that if we want to have single cosign verifier to enforce it or have different cosign verifiers with different keys.
There was a problem hiding this comment.
very true. we need to discuss this. I will bring this up as topic to discuss
akashsinghal
requested review from
jimmyraywv,
luisdlp,
susanshi and
toddysm
as code owners
| - add new `keys` field to spec | ||
| - Multitenancy considerations | ||
| - Need a separate keys map for namespaced and global certificate store keys | ||
| #### Renaming CertificateStore to KeyManagementSystem |
There was a problem hiding this comment.
Will renaming CRD be a breaking change?
There was a problem hiding this comment.
We would have to introduce the new CRD KeyManagementSystem as a new and separate resource and then deprecate the existing one. I think removing full support for it would be breaking change... we should discuss this. The simplest option would be to keep the CertificateStore but the name is the only issue to consider.
There was a problem hiding this comment.
Is the new CRD name KeyManagementSystem narrow to the Key Management System? If users use a managed signing service like AWS Signer or Azure Code Signing service, is this CRD applicable for non-KMS service?
| - Image manifest pushed to same repository as subject (tag schema: `sha256-abc123.sig`) | ||
| - Image manifest is updated every time new signature is added. Each layer is a different signature (possibly from different signing keys) | ||
| ```json | ||
| # Sample Cosign OCI Image Manifest |
There was a problem hiding this comment.
| # Sample Cosign OCI Image Manifest | |
| # Sample OCI Image Manifest with Cosign signanture |
| - However, if a user decides to do a mixture of cert stores + local directory then they would need to specify in 2 different sections (cosign plugin config + cert store) | ||
| - **UPDATE 1/18/24**: We will mirror notation experience with a `verificationKeys` field in the cosign verifier config | ||
|
|
||
| ## Support Multiple keys for cosign verification |
There was a problem hiding this comment.
TODO: make sure our docs for cosign + notation call out capabilities of mulit key + cert by default.
|
|
||
| ## Support Multiple keys for cosign verification | ||
|
|
||
| Currently, there is only support for a single key per cosign verifier. |
There was a problem hiding this comment.
TODO: open up an issue for revocation key list?
There was a problem hiding this comment.
who will own the management of revocation keys? Ratify or third-party service?
There was a problem hiding this comment.
For external KMS like akv, I think we rely upon AKV to mark the certs as revoked. This will tie into the work to add periodic certificate refresh from KMS. In that case, if a cert is marked revoked in AKV, then Ratify can remove that cert. For inline scenarios, the user will have to inform Ratify if a certificate is revoked. In this case is a revocation list even necessary? Can't the user just delete the cert reference in the CRD?
There was a problem hiding this comment.
I am also working on this if only key pairs (private key + public key) are used, no certificates. What will be the expiry and revocation experience.
To clarify the difference between Cosign and Notary Project. The key for cosign verification, it means public key. For notary project, we use root CA certificate, not public key. It's different. Cosign does support verification using certificates, but it is not in the scope of cosign support yet. I will bring more clarification on this soon.
There was a problem hiding this comment.
Thanks @yizha1 for context. So in public key scenario, if user wants to revoke public key, they would simply just delete the key in their Key Vault? There's no concept of expiry like in certificates?
There was a problem hiding this comment.
If users manage key pairs in AKV, and the key is compromised, users need to rotate the key, so there will be a new version of the key. If we always retrieve the latest version of the key, then there should be a way to gracefully remove the previous version of the key. There is no concept of key expire, but AKV allows uses to set expiration date, but after expiration, users need to disable the version of key or delete the key totally, then this should be detectable via API, so the question I think is more about how to gracefully remove the old version of key in what context. We can discuss it further in the community meeting and document the behavior.
| - User specifies enforcement type by specifying `count` of verification operations that must succeed. | ||
| - e.g 2 keys specified and 2 signatures on the image. each verification is against both signatures for a single key | ||
| - If count is 2, then 2 verifications must return true | ||
| - Why is `keyEnforcement` necessary? |
There was a problem hiding this comment.
What about 'scope` field by registry similar to notation's trust policy
There was a problem hiding this comment.
do you think if we should introduce a general trust policy to cert/key based verifiers?
There was a problem hiding this comment.
yeah that's a good suggestion. I think from a user standpoint we can try to mirror some of the functionality (trust policy has a lot of useful things for keys too like registryScopes and trustStores) but it also has some fields that are not applicable to keys too like identities and signatureVerification level which are notation specific. I'm concerned that we might start overloading the trust policy schema to mean 2 different things between cosign and notation.
From implementation POV, it might not make sense to unify since we already consume the trust policy doc and pass it directly to notation-go. There's no parsing/processing.
| - Multitenancy considerations | ||
| - Need a separate keys map for namespaced and global certificate store keys | ||
|
|
||
| ### Option 2: Renaming CertificateStore to KeyManagementSystem |
There was a problem hiding this comment.
@fseldow We are planning to deprecate CertificateStore and move to a new resource which will bundle certificate + key management. I'd appreciate your input on this decision as well as any comments you have on the cosign doc.
Description
What this PR does / why we need it:
Overall technical design breakdown of all work proposed to support robust cosign verification support in Ratify. This is an evolving document and PR should be used for discussion.
Focuses on issues in:
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #
Type of change
Please delete options that are not relevant.
mainbranch)How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration
Checklist:
Post Merge Requirements
Helm Chart Change