search

notaryproject / notation

A CLI tool to sign and verify artifacts
https://notaryproject.dev/
Apache License 2.0
306 stars 84 forks source link

feat: upgrade to OCI 1.1 #916

Closed Two-Hearts closed 2 months ago

Two-Hearts commented 3 months ago

This PR upgrades Notation to OCI 1.1.

Major changes [UPDATE as of 4/2/2024 after community meeting]:

  1. New flag --force-referrers-tag is introduced. And it is only applied to the Sign command. It's default to true, and it's NOT an experimental flag. The original experimental flag --allow-referrers-api will be hidden, i.e., description and example will be hidden from help page. It is kept only for backwards compatibility purpose, and a warning will be printed out when user sets it.

  2. Sign: 

     # Default behavior: Use the referrers tag schema for backwards compatibility.
 notation sign ...
 notation sign --force-referrers-tag ...

# With `--force-referrers-tag=false`: Check the Referrers API first, if not supported, automatically fallback to the referrers tag schema.
 notation sign --force-referrers-tag=false ...
  3. Verify/List/Inspect: They will always use the Referrers API first, if not supported, automatically fallback to the referrers tag schema. Note: for backwards compatibility reason, the experimental flag --allow-referrers-api is still kept and hidden. When set, a warning will be printed out.

Changes are tested in E2E and ACR.

This PR partially resolves issue https://github.com/notaryproject/notation/issues/892. Resolves #893. Resolves #926,

github-advanced-security[bot] commented 3 months ago

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.72%. Comparing base (eaa5fb4) to head (ce4915a). Report is 23 commits behind head on main.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #916 +/- ## ========================================== + Coverage 64.93% 67.72% +2.79% ========================================== Files 45 45 Lines 2729 2169 -560 ========================================== - Hits 1772 1469 -303 + Misses 795 534 -261 - Partials 162 166 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sudo-bmitch commented 3 months ago

This may be too late to change, but if not, I'd suggest renaming the flag to something more clear to end users:

--force-1-1-compatibility: Pushes content to registries required to support notation 1.1.0 and earlier clients

Default the value to true for now, and on a 2.x release, change the default to false.

From an outside view, --allow-referrers-api is confusing since the signing operation isn't calling the API and the client has no say in whether the registry will include the content in the referrers response, that's automatic based on the subject field.

Edit: note this is a focus on the notation version, rather than the OCI version, since users would more likely know the version of their notation clients while they don't necessarily know the version of their registry.

priteshbandi commented 3 months ago

This may be too late to change, but if not, I'd suggest renaming the flag to something more clear to end users:

--force-1-1-compatibility: Pushes content to registries required to support notation 1.1.0 and earlier clients

Default the value to true for now, and on a 2.x release, change the default to false.

From an outside view, --allow-referrers-api is confusing since the signing operation isn't calling the API and the client has no say in whether the registry will include the content in the referrers response, that's automatic based on the subject field.

+1, can we add --force-oci-1-1 and make --allow-referrers-api its alias ?

Two-Hearts commented 3 months ago

I'm actually seeing two suggestions here:

  1. Suggested by @sudo-bmitch : --force-1-1-compatibility, here 1-1 means notation 1.1.0 and before. This flag is default to true. In this case: 
    
Sign with the referrers tag schema by default:
notation sign ...
notation sign --force-1-1-compatibility ...

Sign with the referrers API: notation sign --force-1-1-compatibility=false ...


2. Suggested by @priteshbandi  : `--force-oci-1-1`, an alias of the current `--allow-referrers-api`, default to `false`. Since `--allow-referrers-api` is currently under `experimental`, we could safely rename it to `--force-oci-1-1`. In this case:

Sign with the referrers tag schema by default: notation sign ...

Sign with the referrers API: notation sign --force-oci-1-1 ...



Maybe a vote? @sajayantony @shizhMSFT @toddysm @yizha1 @FeynmanZhou @JeyJeyGao @rgnote and other folks.
shizhMSFT commented 3 months ago

This may be too late to change, but if not, I'd suggest renaming the flag to something more clear to end users:

--force-1-1-compatibility: Pushes content to registries required to support notation 1.1.0 and earlier clients

Default the value to true for now, and on a 2.x release, change the default to false.

From an outside view, --allow-referrers-api is confusing since the signing operation isn't calling the API and the client has no say in whether the registry will include the content in the referrers response, that's automatic based on the subject field.

Edit: note this is a focus on the notation version, rather than the OCI version, since users would more likely know the version of their notation clients while they don't necessarily know the version of their registry.

It makes sense as pushing manifests with subjects does not really call the referrers API as defined in distribution-spec v1.1.0 although it did call as defined in distribution-spec RC versions.

The --force-1-1-compatibility is good but confusing since it means more than registry interactions. Therefore, I'd suggest --force-referrers-tag. In notation 1.x, the default value is --force-referrers-tag=true for backward compatibility. In notation 2.x, the default value can set to false.

yizha1 commented 3 months ago

I vote for the flag name --force-referrers-tag.

priteshbandi commented 3 months ago

As discussed in Today's call, I'm fine with both --force-referrers-tag or --force-tag-schema` with defualt value of true in. (1.2 release). This serves the intent of using tags for storing signature and doesn't uses oci version.

FeynmanZhou commented 3 months ago

I am kind of @shizhMSFT 's proposal, i.e.,

For @sudo-bmitch and @priteshbandi 's proposals above, --force-1-1-compatibility and force-oci-1-1 sound a bit vague and technical because most of end users are not aware of the OCI spec and different versions. In general, users care about which signature format they can use to store the signature in registry.

yizha1 commented 3 months ago

I think --force-referrers-tag is better than --force-tag-schema, as it shows that the tag is related to referrers, and also a short version of the term Referrers Tag Schema used in OCI specification, see the screenshot below

image

JeyJeyGao commented 3 months ago

I vote for --force-referrers-tag with true as the default value. I also want to mention that it conflicts with the original flag --allow-referrers-api. If we also need to keep the original flag, we should ensure they are mutually exclusive.

Two-Hearts commented 3 months ago

Going for --force-referrers-tag as it gets the highest vote.

Note: --allow-referrers-api (default to false) is no longer a valid flag in notation 1.2.0 with oras-go v2.5.0 and OCI 1.1. Because, even though the signer does NOT set this flag, a referrer will still be stored in the registry (given the registry supports the referrers api). However, due to backwards compatibility reason, we need to keep the flag (mark as deprecated) and remove it in a future release.

sudo-bmitch commented 3 months ago

For @sudo-bmitch ... proposals above, --force-1-1-compatibility and force-oci-1-1 sound a bit vague and technical because most of end users are not aware of the OCI spec and different versions.

This reason is why my suggestion wasn't about OCI, referrers, or a tag schema. Users may not know what they are or why they would want it. The 1-1-compatibility flag described that it was needed to support 1.1 versions of notation clients, which tells users when and why they would need to use the flag:

--force-1-1-compatibility: Pushes content to registries required to support notation 1.1.0 and earlier clients

The --force-1-1-compatibility is good but confusing since it means more than registry interactions.

My understanding here is that it got turned down because you have other flags that users will also need to use to support older versions of notation, which I was not aware of. That may be an indication your users won't be aware of that either.