Skip to content

fix: Update revocation spec#249

Merged
priteshbandi merged 2 commits intonotaryproject:mainfrom
priteshbandi:revoke
Jun 28, 2023
Merged

fix: Update revocation spec#249
priteshbandi merged 2 commits intonotaryproject:mainfrom
priteshbandi:revoke

Conversation

@priteshbandi
Copy link
Copy Markdown
Contributor

@priteshbandi priteshbandi commented Mar 27, 2023 •
edited
Loading

Some updates to revocation specs. Also, removed remotes/upstream/main/ADA-fuzzing-audit-22-23.pdf file.

This PR only updates revocation specs and doesn't addresses renaming issue. Renaming is covered by @yizha1 in separate PR #262

References:

  1. https://bugzilla.mozilla.org/show_bug.cgi?id=745747
  2. https://www.ibm.com/docs/en/ibm-mq/8.0?topic=certificates-revoked-ocsp
  3. https://groups.google.com/g/mozilla.dev.security.policy/c/CLg_MOZoG-M
  4. https://docs.oracle.com/middleware/12213/wls/SECMG/ssl_cert_revoc_check.htm#SECMG579
  5. https://www.cisco.com/c/en/us/td/docs/security/ise/2-6/pic_admin_guide/PIC_admin26/PIC_admin26_chapter_0101.html
  6. https://www.sysadmins.lv/retired-msft-blogs/xdot509/pki-design-considerations-certificate-revocation-and-crl-publishing-strategies.aspx
  7. https://www.securew2.com/blog/certificate-revocation-crl-explained
  8. https://blog.cloudflare.com/high-reliability-ocsp-stapling/
  9. https://uptime.netcraft.com/perf/reports/performance/CRL?reverse=1&orderby=avg_total
  10. https://uptime.netcraft.com/perf/reports/performance/OCSP?reverse=1&orderby=avg_total

@priteshbandi priteshbandi changed the title Update revocation spec fix: Update revocation spec Mar 27, 2023
yizha1
yizha1 reviewed Mar 28, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Suggest removing Notary v2 references in this document.

shizhMSFT
shizhMSFT previously approved these changes Mar 28, 2023
View reviewed changes
Copy link
Copy Markdown
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

toddysm
toddysm requested changes Mar 28, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@toddysm toddysm left a comment

Choose a reason for hiding this comment

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

I will need to go over the spec in more detail but one question that immediately pops up is how this revocation will work in air-gapped environments where the OCSP and CDP may not be accessible.

vaninrao10
vaninrao10 previously approved these changes Apr 7, 2023
View reviewed changes
Copy link
Copy Markdown

@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

@priteshbandi
Copy link
Copy Markdown
Contributor Author

priteshbandi commented Apr 11, 2023 •
edited
Loading

I will need to go over the spec in more detail but one question that immediately pops up is how this revocation will work in air-gapped environments where the OCSP and CDP may not be accessible.

Not inherently but user can use plugin verification extensibility.

@yizha1
Copy link
Copy Markdown
Contributor

yizha1 commented Apr 20, 2023

@toddysm Maybe we can discuss further on air-gapped environment requirement. My understanding is that in the air-gapped environment, there could still be PKI infrastructure established, right?

patrickzheng200
patrickzheng200 reviewed Apr 26, 2023
View reviewed changes
@yizha1
Copy link
Copy Markdown
Contributor

yizha1 commented May 6, 2023

@priteshbandi have you considered this https://en.wikipedia.org/wiki/OCSP_stapling

@priteshbandi priteshbandi dismissed stale reviews from vaninrao10 and shizhMSFT via 811de64 May 13, 2023 03:23
@priteshbandi
Copy link
Copy Markdown
Contributor Author

priteshbandi commented May 13, 2023

@priteshbandi have you considered this https://en.wikipedia.org/wiki/OCSP_stapling

Yes but its not directly applicable to our usecase because stapling is done by content provider in our case it would be registry.

@yizha1
Copy link
Copy Markdown
Contributor

yizha1 commented May 18, 2023

@priteshbandi overall is good, could elaborate a bit about the last comment, how user can use plugin verification extensibility?

I will need to go over the spec in more detail but one question that immediately pops up is how this revocation will work in air-gapped environments where the OCSP and CDP may not be accessible.

Not inherently but user can use plugin verification extensibility.

shizhMSFT
shizhMSFT previously approved these changes May 19, 2023
View reviewed changes
Copy link
Copy Markdown
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

shizhMSFT
shizhMSFT reviewed May 19, 2023
View reviewed changes
toddysm
toddysm requested changes May 19, 2023
View reviewed changes
yizha1
yizha1 reviewed May 21, 2023
View reviewed changes
@priteshbandi priteshbandi requested a review from toddysm June 12, 2023 23:16
@priteshbandi
Update revocation spec
cd2f7c4 
Signed-off-by: Pritesh Bandi <pritesb@amazon.com>
@priteshbandi priteshbandi requested a review from vaninrao10 June 12, 2023 23:25
gokarnm
gokarnm reviewed Jun 14, 2023
View reviewed changes
gokarnm
gokarnm previously approved these changes Jun 26, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@gokarnm gokarnm left a comment

Choose a reason for hiding this comment

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

LGTM!

@priteshbandi
updated timeout for CRL and OCSP based on netcraft data
11516c5 
Signed-off-by: Pritesh Bandi <priteshbandi@gmail.com>
gokarnm
gokarnm approved these changes Jun 26, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@gokarnm gokarnm left a comment

Choose a reason for hiding this comment

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

LGTM!

@priteshbandi priteshbandi mentioned this pull request Jun 26, 2023
Merged
shizhMSFT
shizhMSFT reviewed Jun 27, 2023
View reviewed changes
Copy link
Copy Markdown
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 except the term Notary V2.

@yizha1
Copy link
Copy Markdown
Contributor

yizha1 commented Jun 27, 2023

@priteshbandi @shizhMSFT @gokarnm I had another PR covering the naming issue, do you agree that we only fix the revocation related issue in this PR? If yes, maybe @priteshbandi you can update the PR description or add a comment to make it clear about the scope of this PR and point to the PR for naming changes.

@priteshbandi
Copy link
Copy Markdown
Contributor Author

priteshbandi commented Jun 27, 2023 •
edited
Loading

@priteshbandi @shizhMSFT @gokarnm I had another PR covering the naming issue, do you agree that we only fix the revocation related issue in this PR? If yes, maybe @priteshbandi you can update the PR description or add a comment to make it clear about the scope of this PR and point to the PR for naming changes.

Updated PR description to reflect that, this PR address only revocation spec changes and renaming is done is separate PR.

shizhMSFT
shizhMSFT approved these changes Jun 27, 2023
View reviewed changes
Copy link
Copy Markdown
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

toddysm
toddysm approved these changes Jun 27, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@toddysm toddysm left a comment

Choose a reason for hiding this comment

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

I am OK with the changes in this PR but we need to have another one with the proper terminology.

@priteshbandi priteshbandi merged commit b6212a5 into notaryproject:main Jun 28, 2023
priteshbandi added a commit to notaryproject/notation-go that referenced this pull request Jun 28, 2023
@priteshbandi
fix: update timeout for OCSP call to 2 seconds (#327)
432c931 
updated timeout for OCSP call to 2 seconds.

Updates based on updates in notaryproject/specifications#249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toddysm toddysm toddysm approved these changes

@gokarnm gokarnm gokarnm approved these changes

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@justincormack justincormack Awaiting requested review from justincormack

@NiazFK NiazFK Awaiting requested review from NiazFK NiazFK is a code owner

@SteveLasker SteveLasker Awaiting requested review from SteveLasker

@yizha1 yizha1 Awaiting requested review from yizha1 yizha1 is a code owner

@patrickzheng200 patrickzheng200 Awaiting requested review from patrickzheng200

@vaninrao10 vaninrao10 Awaiting requested review from vaninrao10 vaninrao10 is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@priteshbandi @yizha1 @toddysm @gokarnm @shizhMSFT @patrickzheng200 @vaninrao10