-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
lib/mk-ca-bundle.pl: skip certs passed Not Valid After date #7801
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
Conversation
With this change applied, the now expired 'DST Root CA X3' cert will no longer be included in the output. Details: https://letsencrypt.org/docs/dst-root-ca-x3-expiration-september-2021/
@@ -436,9 +436,25 @@ (%) | |||
last if (/\*\*\*\*\* END LICENSE BLOCK \*\*\*\*\*/); | |||
} | |||
} | |||
elsif(/^# (Issuer|Serial Number|Subject|Not Valid Before|Not Valid After |Fingerprint \(MD5\)|Fingerprint \(SHA1\)):/) { | |||
# Not Valid After : Thu Sep 30 14:01:15 2021 | |||
elsif(/^# Not Valid After : (.*)/) { |
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.
why is this no longer pushed to precert, also if there is no Not Valid After
in the header (which shouldn't happen afaict) then the last valid state is reused?
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.
why is this no longer pushed to precert
That's a mistake I think. I never use -m
myself and we have no tests for this...
also if there is no Not Valid After in the header
I thought about it but I could not come up with a quick fix so I left it like this since I wanted to land this and run the script asap. Any suggestions on how to improve this? The script is a little quirky...
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.
The logic a few lines down of resetting precert doesn't make sense to me:
elsif(/^#|^\s*$/) {
undef @precert;
next;
}
Which is to say that if # is detected and does not match any known header (because otherwise this code wouldn't be reached) that it resets precert, or if there is a blank line it resets precert. For the former case wouldn't we want to keep precert even if some unknown header is found?
To your question I think that valid should start off as 0 and reset to 0 at the beginning of processing every certificate but I'm having trouble finding exactly where it makes that determination. I thought it could be in the code quoted above but a # Certificate
check would make more sense imo
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.
Feel free to work on improvements!
.. and bump version to 1.29. This change makes the script properly ignore unknown blocks and otherwise fail when Mozilla changes the certdata format in ways we don't expect. Though this is less flexible behavior it makes it far less likely that an invalid certificate can slip through. Prior to this change the state machine did not always properly reset, and it was possible that a certificate marked as invalid could then later be marked as valid when there was conflicting trust info or an unknown block was erroneously processed as part of the certificate. Ref: curl#7801 (review) Closes #xxxx
.. and bump version to 1.29. This change makes the script properly ignore unknown blocks and otherwise fail when Mozilla changes the certdata format in ways we don't expect. Though this is less flexible behavior it makes it far less likely that an invalid certificate can slip through. Prior to this change the state machine did not always properly reset, and it was possible that a certificate marked as invalid could then later be marked as valid when there was conflicting trust info or an unknown block was erroneously processed as part of the certificate. Ref: #7801 (review) Closes #8411
With this change applied, the now expired 'DST Root CA X3' cert will no
longer be included in the output.
Details: https://letsencrypt.org/docs/dst-root-ca-x3-expiration-september-2021/