-
-
Notifications
You must be signed in to change notification settings - Fork 296
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
Quiet OCSP warnings if the cert has a short lifetime #320
Conversation
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.
Looks good, overall!
One thought I had: might it be a good idea to only stifle the warnings for the specific message of missing OCSP information (in short-lived certs)?
Because, if for some reason a short-lived cert had OCSP, you'd probably still want to know it. But I think specifically what we want to do is make it completely silent/OK if a short-lived cert specifically doesn't support OCSP. But if there was some sort of other error, like a network issue, we'd want to know it.
What do you think? The logic of calling Lifetime()
would probably move into stapleOCSP
and if the lifetime was short, we'd just set the err to nil and return (a no-op).
How's this? |
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.
Yes! That's it! Almost, I think 😄
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
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.
⭐ 🌟 ⭐
Oh, lol, whodathunkit, we have a test for this: |
Oh I guess this broke a test lmao |
Thanks a bunch! |
In Caddy, we often get warnings like these when using internally issued certs:
These generally have a very short lifetime and for that reason warning about them not having OCSP configured is just noise, because OCSP on short certs isn't too useful.
I added a
Lifetime()
method to help here, I usedexpiresAt
for the "1s resolution of ASN.1 UTCTime/GeneralizedTime", hope that's correct.