Log certificate errors in isCertValid (OpenSSL 4.0 only)#4064
Conversation
📝 WalkthroughWalkthrough
ChangesTLS Certificate Time Validation Logging
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/tls.c (1)
472-477: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
else ifto make mutual exclusivity explicit.
X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELDandX509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELDare distinct values, so both branches cannot fire for the sameerror. Usingelse ifbetter expresses this intent and avoids a subtle maintenance hazard if additional branches are added later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tls.c` around lines 472 - 477, The two certificate error checks in the tls.c logic are mutually exclusive, so make the branching explicit by changing the second condition after the first `serverLog` call to `else if` in the error handling around the `X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD` and `X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD` cases. Keep the same log messages, but ensure the `error` variable is handled in a single exclusive chain so future maintenance doesn’t accidentally allow both branches to be considered independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tls.c`:
- Around line 475-477: The notAfter error log in the tls.c branch is passing the
error value without consuming it in the format string. Update the serverLog call
in the notAfter handling path to include a %d placeholder so the error variable
is actually printed, matching the surrounding notBefore/error handling in the
same function.
- Around line 472-474: The serverLog call in the
X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD branch is passing error as a variadic
argument without a matching printf placeholder. Update the log message in the
tls.c certificate-validation path so the format string consumes error with %d or
%i, or remove the argument if it is not needed; use the serverLog/_serverLog
call site to locate it.
---
Nitpick comments:
In `@src/tls.c`:
- Around line 472-477: The two certificate error checks in the tls.c logic are
mutually exclusive, so make the branching explicit by changing the second
condition after the first `serverLog` call to `else if` in the error handling
around the `X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD` and
`X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD` cases. Keep the same log messages,
but ensure the `error` variable is handled in a single exclusive chain so future
maintenance doesn’t accidentally allow both branches to be considered
independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
7c3630f to
ebbdf9a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #4064 +/- ##
============================================
- Coverage 76.73% 76.67% -0.06%
============================================
Files 162 162
Lines 81029 81029
============================================
- Hits 62175 62130 -45
- Misses 18854 18899 +45
🚀 New features to boost your workflow:
|
zuiderkwast
left a comment
There was a problem hiding this comment.
Thanks, I misunderstood the return value of X509_check_certificate_times. This follow-up PR looks like what I expected. Reference: https://docs.openssl.org/master/man3/X509_check_certificate_times/#return-values
@sarthakaggarwal97 Sorry for merging the other PR to fast.
If the cert is not yet valid or expired, no extra logs are output and there are no changes. But if the cert's notBefore or notAfter fields are invalid, a warning is issued. Signed-off-by: Petr Khartskhaev <pkhartsk@redhat.com>
ebbdf9a to
2f9f91f
Compare
|
@zuiderkwast I think we can merge this. |
|
@madolson I was going to merge this but I see you assigned yourself. Any concerns? |
|
Oops, this was the pr I showed during the meeting for assigning. I thought I had removed myself. |
If the cert is not yet valid or expired, no extra logs are output and there are no changes.
But if the cert's notBefore or notAfter fields are invalid, a warning is issued.
Changed also due to the previous logging code being unreachable.
Follow-up of #4016