prober/tls: fix probe_ssl_last_chain_expiry_timestamp_seconds #681
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This metric should report the earliest expiry of the chain that expires the latest out of all the verified chains. Presently, it reports the earliest expiry of the chain that expires first. The current test for this metric was using an expired root certificate which is omitted from the verified chain, so the test was passing despite this bug. I've changed it to use a root that is still valid but expires before a root held by the client. Signed-off-by: Rob Best <robertbest89@gmail.com>
ribbybibby
force-pushed
the
last-chain-expiry-fix
branch
from
e4fcf2d
to
9b3d63f
Compare
|
Your change seems correct, @itkq did you miss this in your testing? The unittest still doesn't seem right, we should have a test with different values for the two metrics. |
Include the older root certificate in the chain presented by the server as well as in the client root CAs. This ensures that the peer certificate metric identifies the older root CA as the earliest expiry while it is ignored by the verified metric in favour of the longer-lived chain. Signed-off-by: Rob Best <robertbest89@gmail.com>
|
@brian-brazil I've tweaked the test. I think this captures the situation this metric was put in place to address now: the server presents a root cert that expires before the server cert, despite the client holding another, valid, longer-lived root. |
|
This change seems right for me. I'm sorry, I should have noticed that |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Apologies if I've misunderstood the purpose of this metric but I believe that it should be reporting the earliest expiry date of the chain that expires last out of all the verified chains. Presently it's reporting the earliest expiry of the chain that expires first.
This wasn't identified by the test for this metric because the test is using an expired root certificate which is not included in the
VerifiedChains, so there was only one chain for the method to iterate over and thereforegetLastChainExpiryreturned the same regardless of the logic.