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

chore: removed TODOs, updated outdated specs etc #231

Merged
merged 2 commits into from Feb 2, 2023
Merged

chore: removed TODOs, updated outdated specs etc #231

merged 2 commits into from Feb 2, 2023

Conversation

priteshbandi
Copy link
Contributor

Change log

  1. Moved various requirements related doc to requirements folder
  2. Updated the missing critical attributes in verify-signature command which was part of fix(plugin): update critical attributes list in verify-signature #184 but that's not getting updated by Milind
  3. We are tracking threat model and other open issues in support "attach"ing signatures generated by a separate crypto mechanism notation#151, Plugin related issues #230 and thus removed it from spec
  4. Add signingAuthority to trust store specs.
    5.Removed verification extensibility TODO and replaced it with link to plugin spec link

Signed-off-by: Pritesh Bandi <pritesb@amazon.com>
@priteshbandi priteshbandi changed the title housekeeping: removed todos, updated TODOs in specs etc housekeeping: removed TODOs, updated outdated specs etc Feb 1, 2023
@priteshbandi priteshbandi changed the title housekeeping: removed TODOs, updated outdated specs etc chore: removed TODOs, updated outdated specs etc Feb 1, 2023
Copy link
Contributor

@yizha1 yizha1 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 the update. I left a few comments.

@@ -206,7 +210,7 @@ The following table shows the resultant validation action, either *enforced* (ve

**Revocation check** : Guarantees that the signing identity is still trusted at signature verification time. Events such as key or system compromise can make a signing identity that was previously trusted, to be subsequently untrusted. This guarantee typically requires a verification-time call to an external system, which may not be consistently reliable. The `permissive` verification level only logs failures of revocation check and does not enforce it. If a particular revocation mechanism is reliable, use `strict` verification level instead.

- **NOTE** : `notation` RC1 will not have in built support for Revocation Check feature, see [Key Management - Rescinding Signatures (CRL)](https://github.com/notaryproject/notaryproject/issues/72) for details. When this feature is subsequently implemented, the `strict` verification level will **enforce** this validation, and will fail signatures which would previously pass signature verification when revocation checks were not supported. This is considered a **breaking change**, but is the appropriate behavior for a `strict` verification level.
- **NOTE** : `notation` RC2 will not have in built support for Revocation Check feature, see [Key Management - Rescinding Signatures (CRL)](https://github.com/notaryproject/notaryproject/issues/72) for details. When this feature is subsequently implemented, the `strict` verification level will **enforce** this validation, and will fail signatures which would previously pass signature verification when revocation checks were not supported. This is considered a **breaking change**, but is the appropriate behavior for a `strict` verification level.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about not mentioning the release version, I am afraid we will update it again in the future.

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, I've removed this section from here as this is notation limitation(implementation missing) not notary v2 spec limitation.
There are two ways in which we can surface this limitations to users

  1. Create PR in notation's README.md to call out its limitation.
  2. Call them as part of release notes

I think we should should do both(which should be done separately as release activity.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you to remove it, since it is more about implementation progress, not specification itself. I did notice that there is a release for this repo https://github.com/notaryproject/notaryproject/releases/tag/v1.0.0-rc.1, which is also called rc.1. Will this cause confusion that the spec rc.1 matched the notation rc.1?

I also agree with you that we should put the limitations in both Readme.md and release notes. In the future, we should also link the release note or create a different format in notaryproject website. Currently we don't have release blogs yet. see example https://oras.land/blog/oras-0.14-and-future/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did notice that there is a release for this repo https://github.com/notaryproject/notaryproject/releases/tag/v1.0.0-rc.1, which is also called rc.1. Will this cause confusion that the spec rc.1 matched the notation rc.1?

IMO, that should not be an issue. Spec version and implementation versions can (and will) diverge, its implementations responsibility to call out which spec version it supports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking it in #233

specs/trust-store-trust-policy.md Show resolved Hide resolved
specs/plugin-extensibility.md Show resolved Hide resolved
specs/plugin-extensibility.md Show resolved Hide resolved
@shizhMSFT
Copy link
Contributor

shizhMSFT commented Feb 1, 2023

Can we split the PR into smaller PRs although this PR overall LGTM? Basically, one PR one purpose. Otherwise, the combine PR may take longer time to review and may be stale in the end.

Copy link

@vaninrao10 vaninrao10 left a comment

Choose a reason for hiding this comment

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

LGTM - PR#184 is taken care.

Signed-off-by: Pritesh Bandi <pritesb@amazon.com>
@priteshbandi
Copy link
Contributor Author

Can we split the PR into smaller PRs although this PR overall LGTM? Basically, one PR one purpose. Otherwise, the combine PR may take longer time to review and may be stale in the end.

Yes there are multiple changes but at high level there are only two changes

  1. move requirements related doc to requirements directory, which are shown as moved files in PR
  2. Update outdated specs.

Although I can split above two changes into separate PRs but that wouldn't help much

Copy link
Contributor

@yizha1 yizha1 left a comment

Choose a reason for hiding this comment

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

LGTM. Suggest creating an issue to track the limitations in rc.2 release notes.

@priteshbandi priteshbandi merged commit a56d1d9 into notaryproject:main Feb 2, 2023
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

4 participants