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

Adding cleaned up version of key management requirements #67

Merged
merged 28 commits into from
Sep 14, 2021

Conversation

NiazFK
Copy link
Contributor

@NiazFK NiazFK commented Apr 30, 2021

No description provided.

@SteveLasker
Copy link
Contributor

@NiazFK, can you please resolve the DCO

@NiazFK NiazFK force-pushed the master branch 2 times, most recently from 6f36f25 to 34758e1 Compare June 8, 2021 00:40
NiazFK and others added 24 commits June 7, 2021 18:02
Added in initial set of uses cases documented so far in meeting notes.

Signed-off-by: Niaz Khan <[email protected]>
Co-authored-by: Trishank Karthik Kuppusamy <[email protected]>
Signed-off-by: Niaz Khan <[email protected]>
Co-authored-by: Trishank Karthik Kuppusamy <[email protected]>
Signed-off-by: Niaz Khan <[email protected]>
Diagrams to highlight new components based on preliminary discussions.

Signed-off-by: Niaz Khan <[email protected]>
Added diagrams, new personas, and placeholders for additional definitions.

Signed-off-by: Niaz Khan <[email protected]>
Fixed image rendering

Signed-off-by: Niaz Khan <[email protected]>
Fixes for image rendering

Signed-off-by: Niaz Khan <[email protected]>
Co-authored-by: Brandon Mitchell <[email protected]>
Signed-off-by: Niaz Khan <[email protected]>
Co-authored-by: Brandon Mitchell <[email protected]>
Signed-off-by: Niaz Khan <[email protected]>
Removed links for additional requirements docs.

Signed-off-by: Niaz Khan <[email protected]>
Signed-off-by: Niaz Khan <[email protected]>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sudo-bmitch sudo-bmitch left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up the DCO. The new png's aren't referenced in the markdown, is that intentional?

I think most of my comments are old but got lost in the cleanup or were never included in the PR. Since this is a new file, I'm fine with either iterating here, or merging and iterating in separate PRs.

}
}
```
- Trust Store: The trust store defines the relationship between signing keys and artifacts that are used at validation time to determine whether to trust an artifact with a crytptographically valid signature. The trust store will relate a scope (any source, specific registry, specific repository, or specific target) with a certificate (for root key, intermediate, or signing key) or key repository (for automated key distribtuion). It is not recommended to use a signing key as this will cause signature validation to fail if the signing key is rotated. An example trustStore where the first root is trusted for artifacts from any registry and the second root is only trusted for artifacts from "registry.wabbit-networks.io" :
Copy link
Contributor

Choose a reason for hiding this comment

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

It would make more sense to me to have "scope" moved into the trust policy rather than the trust store. Leaving the trust store as simply a set of named keys. The policy could then be used to define things like permitted registry mirrors, or fine grained scopes for specific public keys in the trust store.

- Trust Store Key Information: The key information configured in the trust store will be cryptographically verifiable and contain:
- Public Key: Will be used to verify signed artifacts.
- Signed Identity: Identity of creator of the signing key.
- (Optional) Issuer: If the key is an intermediate this will describe the owner of the root.
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 like to see more definition on what it means to "describe the owner of the root". I'm looking for something that can't be easily broken by a bad key (e.g. a lone developer makes their key claim to be the root for all of Docker Hub to make their local mirror work, and doesn't realize how a user configurable field in their key breaks other notary clients). Will this reference point to another key in the trust store? Would it make sense to simply include the full certificate chain any time a key other than a root is added to the store?

]
}
```
- Trust Store Key Information: The key information configured in the trust store will be cryptographically verifiable and contain:
Copy link
Contributor

Choose a reason for hiding this comment

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

To make policies on keys in the trust store, I think it may help to include a client configurable name for the key. E.g. I may name one key "Docker Hub" and another "GCR", and configure my trust policies with just that name.

- Publishers SHOULD be able to generate multiple signatures for a single artifact.
- Publisher admins MUST have a mechanism to revoke signatures to indicate they are no longer trusted.
- Trust stores MUST be configurable by the deployer admin.
- Deployer admins MUST be able to configure trusted entities for individual repositories and targets.
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 add something along the lines that "Deployer MUST be able to trust a signing key with a certificate chain that leads to a trusted root or intermediate certificate according to the trust policy". We're still missing the detail of how to communicate that certificate chain (e.g. does it get pushed to the registry with the signing artifact, along side the signature, or from an external discovery system).

@SteveLasker SteveLasker merged commit 61822da into notaryproject:main Sep 14, 2021
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