Skip to content
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

Install and use Homebrew's ca-certificates on macOS <= 10.15.5 #12167

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Oct 1, 2021

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The final step. Must be merged after #12166.

@Bo98 Bo98 added do not merge critical Critical change which should be shipped as soon as possible. labels Oct 1, 2021
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@Bo98 Bo98 changed the title brew.sh: required brewed curl on macOS <= 10.15.5 brew.sh: require brewed curl on macOS <= 10.15.5 Oct 1, 2021
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😭 👍🏻

@Bo98
Copy link
Member Author

Bo98 commented Oct 3, 2021

Following discussion Homebrew/homebrew-core#86291 (comment), I've changed this PR so that we only install one formula: ca-certificates (arriving soon to homebrew-core) and then set SSL_CERT_FILE. This allows system curl to still be used, which greatly simplifies the situation with homebrew-test-bot.

@Bo98 Bo98 changed the title brew.sh: require brewed curl on macOS <= 10.15.5 Install and use Homebrew's ca-certificates on macOS <= 10.15.5 Oct 3, 2021
@Bo98 Bo98 requested a review from carlocab October 3, 2021 01:31
@Bo98 Bo98 force-pushed the brewed-curl-old-macos branch 2 times, most recently from 4d29c33 to 6618497 Compare October 3, 2021 01:42
@Bo98
Copy link
Member Author

Bo98 commented Oct 3, 2021

One caveat with this new approach is that the SSL bug is still present so this issue can resurface in the future.

If this happens, it can be fixed by brew postinstall ca-certificates or we can revision bump the formula to force this to happen. Though this can sometimes be paired with a cert bundle update anyway - Curl/Mozilla released a new version on 2021-09-30, the day of the expiry of the old Let's Encrypt root certificate, to remove it from Curl/Mozilla's copy (and the postinstall should filter it out from the keychain too, if the update is done post-expiry).

Library/Homebrew/cmd/update.sh Outdated Show resolved Hide resolved
@Bo98 Bo98 force-pushed the brewed-curl-old-macos branch 2 times, most recently from cdb134a to d8e89d7 Compare October 3, 2021 04:04
@Bo98
Copy link
Member Author

Bo98 commented Oct 3, 2021

Hmm SecureTransport on 10.10 still doesn't seem to work even with this. Might end up with a hybrid here where some really old versions need a full brewed curl but Mojave etc can live with just ca-certificates - will investigate more tomorrow.

Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. At some point, it will be great to check that the brew install calls here work with HOMEBREW_INSTALL_FROM_API but that isn't a priority at the moment.

@carlocab
Copy link
Member

carlocab commented Oct 3, 2021

Hmm SecureTransport on 10.10 still doesn't seem to work even with this. Might end up with a hybrid here where some really old versions need a full brewed curl but Mojave etc can live with just ca-certificates - will investigate more tomorrow.

Given @EricFromCanada's tests this seems to work on Sierra or newer. I suggest we ship this as soon as we can to fix users who are on those systems. We can always work on a fix for older OSs after.

@Rylan12
Copy link
Member

Rylan12 commented Oct 3, 2021

I suggest we ship this as soon as we can

We need to wait for Homebrew/homebrew-core#86304 gets through, first

Also just a reminder that this will need a new brew tag

@carlocab
Copy link
Member

carlocab commented Oct 3, 2021

Also, I wonder if this will allow us to get rid of using: :homebrew_curl.

@Bo98
Copy link
Member Author

Bo98 commented Oct 3, 2021

Also, I wonder if this will allow us to get rid of using: :homebrew_curl.

I wasn't around when this was added. Why was this necessary? Which platforms did this affect?

@carlocab
Copy link
Member

carlocab commented Oct 3, 2021

Cert errors on Mojave, I think. @iMichka should know the details.

Let's find out ourselves, shall we? Homebrew/homebrew-core#86378.

Edit: Hmm, no, it's not a cert error:

❯ /usr/bin/curl https://www.theora.org
curl: (35) error:1400442E:SSL routines:CONNECT_CR_SRVR_HELLO:tlsv1 alert protocol version

@Bo98
Copy link
Member Author

Bo98 commented Oct 3, 2021

Ok xiph seems to require TLS 1.3, which macOS LibreSSL does not support. Neither does Secure Transport.

@Bo98
Copy link
Member Author

Bo98 commented Oct 3, 2021

Ok so I've added an extra < Sierra check, which will install a full brewed curl. 10.12-10.15.5 will have the lightweight ca-certificates route.

@Rylan12
Copy link
Member

Rylan12 commented Oct 3, 2021

Ok so I've added an extra < Sierra check, which will install a full brewed curl. 10.12-10.15.5 will have the lightweight ca-certificates route.

Sorry if I just missed this, but why won't the ca-certificates route work on < Sierra?

@Bo98
Copy link
Member Author

Bo98 commented Oct 3, 2021

It seems there's some Secure Transport bug or something. Tests on 10.10 & 10.11 reveal that ftp.gnu.org just returns "not trusted" even when passed the ISRG Root X1 alone. Hard to get any meaningful information from Secure Transport - it has poor debugging support.

My guess is it doesn't handle alernative chains properly.

Eric tested all macOS versions and found it works on 10.12 and later.

@Bo98
Copy link
Member Author

Bo98 commented Oct 3, 2021

Basically we're seeing curl/curl#976 on < Sierra.

@Bo98
Copy link
Member Author

Bo98 commented Oct 4, 2021

Must be merged after #12166.

I hate chickens, and I hate eggs.

This can't happen because it needs the nghttp2 PR, but the nghttp2 PR fails because it needs to revision bump formulae outside the curl dep tree and they use HTTPS, which is broken and needs this PR, and the brewed curl test-bot workaround also doesn't work because the nghttp2 PR revision bumps brewed curl itself.

Lost? I don't blame you.

Luckily nghttp2 and its remaining deps all use GitHub so it should be ok.

@Bo98 Bo98 removed the do not merge label Oct 4, 2021
@Bo98 Bo98 merged commit ebc0783 into Homebrew:master Oct 4, 2021
@Bo98 Bo98 deleted the brewed-curl-old-macos branch October 4, 2021 04:30
carlocab added a commit to carlocab/homebrew-test-bot that referenced this pull request Oct 4, 2021
@carlocab
Copy link
Member

carlocab commented Oct 4, 2021

Here's the first successful Homebrew/core CI job from this change: https://github.com/Homebrew/homebrew-core/pull/86421/checks?check_run_id=3788099599

I needed Homebrew/homebrew-test-bot@4fd658e before it worked though. Not sure why.

@Bo98
Copy link
Member Author

Bo98 commented Oct 4, 2021

I needed Homebrew/homebrew-test-bot@4fd658e before it worked though. Not sure why.

This'll be because we never export HOMEBREW_FORCE_BREWED_CA_CERTIFICATES, so it can't be checked in Ruby.

Either we start exporting it or we instead check DevelopmentTools.ca_file_handles_most_https_certificates?.

(Same scenario with curl, even though we don't really need it for any of our CI.)

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

# we cannot install Homebrew CA certificates if homebrew/core is unavailable.
if [[ -d "${HOMEBREW_LIBRARY}/Taps/homebrew/homebrew-core" || -n "${HOMEBREW_INSTALL_FROM_API}" ]]
then
brew install ca-certificates || true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the || true seems undesirable here, we want to handle if this fails (with at least a message), no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a reason in my head before but so much has changed that I can't remember it and it probably no longer applies.

I'm not sure what the message would be though. The solution for brewed curl and git is to install it yourself and put it in the PATH. This doesn't apply to CA certificates.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth just failing in this case, then.

@carlocab
Copy link
Member

carlocab commented Oct 4, 2021

Either we start exporting it or we instead check DevelopmentTools.ca_file_handles_most_https_certificates?.

Homebrew/homebrew-test-bot#674

@github-actions github-actions bot added the outdated PR was locked due to age label Nov 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants