-
Notifications
You must be signed in to change notification settings - Fork 0
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
bundling mozilla root CA store for when there are no default certs #45
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.
Bundling the certs sounds good to me, and I've double checked that the ones in this PR are identical to the ones at https://ccadb.my.salesforce-sites.com/mozilla/IncludedRootsPEMTxt?TrustBitsInclude=Websites (modulo whitespace changes).
Note that the certs are 218 KiB, not 18 KiB like I wrote before - I must have either misread/mistyped it. That's still fine by me. But after we merge this PR, we could consider opening a ticket to track the idea to reduce them.
Apologies: I've run out of time right now to review the other changes closely enough for me to approve.
not all machines have system certs such as in busybox containers and so by bundling certs from Mozilla we can fallback to them when system certs are missing
@viega updated to |
I think To answer my own question of "how often are these certs updated?", I had a vague memory of curl having a page for this. From here:
With this PR, building nimutils the output is:
I'm not sure why there's 2 more, unless the certs were updated today. But maybe the curl URL is a slightly preferable upstream? Minor advantages:
|
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, except I think we need to invalidate the cache.
const | ||
opensslCmd = "openssl storeutl -noout -certs /dev/stdin" | ||
(check, checkExitCode) = gorgeEx(opensslCmd, input=contents) | ||
checkLines = check.splitLines() |
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.
Non-blocking comment: this is a relatively costly split, since it allocates a string for each line (3500 ish of them) at compile time - it's calling the splitLines
proc, not the iterator.
It probably doesn't have a significant impact on compile times, but it'd be better to get the index of the start of the last line, and slice from there to the end.
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.
LGTM, although I haven't written a book on OpenSSL :)
I'm happy to defer this question, but did we have any thoughts on using the certs published by curl instead? Aside from the reasons mentioned in #45 (comment), the salesforce URL looks weird to me. Perhaps the curl URL is more likely to stay working?
not sure about grabbing it from curls site. thought to get it from the original link in case curl does any massaging but happy to change. cc @MyNameIsMeerkat @viega thoughts? |
I feel like we are splitting hairs at this point, the salesforce/microsoft link looks weird for sure but is one of the official sources cited by CCADB (https://www.ccadb.org/resources) so I have no concerns using it and would tend to have more trust in that as a source than using curl's website tbh. Let's just get this done and merged |
I did approve, so I'm happy for us to merge this. From my side, I think we can defer any further discussion to a follow-up issue/PR. |
not all machines have system certs such as in busybox containers and so by bundling certs from Mozilla we can fallback to them when system certs are missing