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

Subject may be redundant #111

Merged
merged 1 commit into from Dec 16, 2021

Conversation

sudo-bmitch
Copy link
Contributor

The subject field seems to be redundant unless we have plans to have other notary fields in the future. This condenses that into a single top level notary field with the payload to be signed.

Signed-off-by: Brandon Mitchell git@bmitch.net

@gokarnm
Copy link
Contributor

gokarnm commented Nov 18, 2021

The notary field is the top level claim which contains a subject field of type descriptor. This allows us to add additional fields under notary field in future without requiring them to be under the descriptor (which only supports adding them as annotations, and restricts them to key value pairs). Though it seems bit redundant, it allows us to add fields under notary while we discover all of the metadata we want to capture in the payload. I don't see a downside from end customer experience. We can could remove the nesting at RC phase if we end up only requiring the descriptor, and possibly renaming notary to something more descriptive that conveys the claim is about the subject manifest.

@sudo-bmitch
Copy link
Contributor Author

The extensibility requirement in OCI is for implementations to ignore unknown fields. Which means if there's a need to have more than a key/value pair in an annotation, you could add additional fields to your implementation without breaking other users of the descriptor.

@gokarnm
Copy link
Contributor

gokarnm commented Dec 9, 2021

No concerns given that we can extend the descriptor with other sub key for complex shapes that cannot fit into annotations. Suggestion, should we rename the top level notary to subjectDescriptor, a more descriptive name.

cc: @priteshbandi @shizhMSFT @SteveLasker

@sudo-bmitch
Copy link
Contributor Author

Once we have consensus on the name, let me know and I'll rebase it.

@shizhMSFT
Copy link
Contributor

No concerns given that we can extend the descriptor with other sub key for complex shapes that cannot fit into annotations. Suggestion, should we rename the top level notary to subjectDescriptor, a more descriptive name.

notary is good enough to me as long as we have documented it.

...
}
}
"notary": {
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 more like a subject rather than the notary

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, the data stored is subjectDescriptor.

Signed-off-by: Brandon Mitchell <git@bmitch.net>
@sudo-bmitch
Copy link
Contributor Author

I've adjusted to use "subject" instead of "notary" based on our discussion today.

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

Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

LGTM

@gokarnm
Copy link
Contributor

gokarnm commented Dec 16, 2021

LGTM

@SteveLasker SteveLasker merged commit 0949aa1 into notaryproject:main Dec 16, 2021
@sudo-bmitch sudo-bmitch deleted the pr-signature-subject branch December 17, 2021 03:44
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.

None yet

6 participants