Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add cached OCSP client support to Cert Auth #17093
Add cached OCSP client support to Cert Auth #17093
Changes from 11 commits
8f34df9
5b63032
b0798a7
87bd5a3
0b9f785
fb65fd5
5d768c0
6e00633
44d808d
2b73de7
0f9cda6
fca24ee
59a3769
ae8423c
ea1feb4
e7d790d
75244c9
68c3308
acc2c29
81e8d6a
ca7e8c0
b5f2b03
6ba87f9
4abdc5a
1be9cf5
51db45f
8b39c75
0642d7e
0ff724c
7e50dd0
222bc58
f192134
b8fb6ed
9933c3b
6912f06
bf4e630
4021908
8143803
e91c2cf
ff60c7d
65bb99a
9a2acb3
899f36d
ec3d5ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure this is correct down in the OCSP code, but its easier to explain here.
tCert
comes viatrusted
off of the storage backend. Its order doesn't matter.cCert
comes viatrustedChains
off of thePeerCertificates
:vault/builtin/credential/cert/path_login.go
Lines 653 to 680 in 7e50dd0
x509.Verify(...)
.Per docs,
trustedChains
includes the leaf cert here, which shouldn't matter as we've hopefully excluded non-CA certs from chains before storage... But is in TLS order (leaf->[ICA->...]root
)...This is correct, as we might have some intermediates that aren't in
trusted
but are intrustedChains
(confusing!)...However, the call to
checkForCertInOCSP
is incorrect (and in particular, its[len(x) - 1]
is incorrect!) -- because we're passing ittrust
coming from the TLS-sortedtrustedChains
. In particular, we're using the last cert in the chain, which isn't the leaf's issuer, but could be some arbitrary parent issuer. OCSP expects to get passed(cert, cert's issuer)
, not(cert, root)
in particular.This should fail verification, as the OCSP responder doesn't necessarily know about the ultimate Root CA this leaf is issued under, and may only know about the ICA that actually issued this leaf.
Hence I believe down there, given that
trust
includes the leaf in spot 0, the code should usechain[1]
.To reproduce, I think if you use two separate CA mount points (not sure if you do in your tests), and don't tell the ICA mount about the root, you should see this fail OCSP with an error like not authoritative or unknown issuer or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, great analysis. This was code I was very unsure about.