Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

certificate verification improvements #1122

Merged
merged 5 commits into from
Oct 29, 2020
Merged

certificate verification improvements #1122

merged 5 commits into from
Oct 29, 2020

Conversation

mikehelmick
Copy link
Contributor

Fixes #1121

Proposed Changes

  • Cache not found issuers the same as found issuers
  • Give an advisory warning for mismatched audience fields
  • Prep for next release where audience fields are missing
  • also, considerably speeds up the pha verify tests by reusing the database for all test cases

Release Note

ATTENTION SERVER OPERATORS:
This release will print ERROR level log messages if there are verification certificates where the issuer matches, but the audience field does not match. In the next release these mismatches will be hard failures. If you find a mismatch, work with your verification server operators to resolve any issues before deploying the release after this one.

@google-oss-robot google-oss-robot added the approved Auto: added by prow when enough reviewers approve. label Oct 29, 2020
@google-cla google-cla bot added the cla: yes Auto: added by CLA bot when all committers have signed a CLA. label Oct 29, 2020
@google-oss-robot google-oss-robot added the size/M Auto: Medium number of changes. label Oct 29, 2020
sethvargo
sethvargo reviewed Oct 29, 2020
View reviewed changes
internal/verification/phaverify.go
@@ -94,8 +102,19 @@ func (v *Verifier) VerifyDiagnosisCertificate(ctx context.Context, authApp *aamo
return nil, err
}

if cacheVal == nil {
Copy link
Member

Choose a reason for hiding this comment

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

This should never happen? I don't think we cache nil values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See line 93 - the primary lookup function can return nil as a valid cached value now
the in memory cache implementation allows it.

internal/verification/phaverify.go Outdated Show resolved Hide resolved
@google-oss-robot google-oss-robot added size/L Auto: large number of changes. and removed size/M Auto: Medium number of changes. labels Oct 29, 2020
sethvargo
sethvargo approved these changes Oct 29, 2020
View reviewed changes
Copy link
Member

@sethvargo sethvargo left a comment

Choose a reason for hiding this comment

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

/lgtm

@mikehelmick can you create an issue to remove this in the next release and block it on that milestone so we don't forget?

@google-oss-robot google-oss-robot added the lgtm Auto: added by prown with a reviewer LGTMs label Oct 29, 2020
@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikehelmick, sethvargo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [mikehelmick,sethvargo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-robot google-oss-robot merged commit d6b6371 into google:main Oct 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2020
@mikehelmick mikehelmick deleted the enforceAud branch November 6, 2020 22:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sethvargo sethvargo sethvargo approved these changes

@jeremyfaller jeremyfaller Awaiting requested review from jeremyfaller

Assignees

@sethvargo sethvargo

Labels
approved Auto: added by prow when enough reviewers approve. cla: yes Auto: added by CLA bot when all committers have signed a CLA. lgtm Auto: added by prown with a reviewer LGTMs size/L Auto: large number of changes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Verification certificate aud not checked
3 participants
@mikehelmick @google-oss-robot @sethvargo