Skip to content

ca-certificates 2021-07-05 (new formula)#86304

Closed
Bo98 wants to merge 2 commits intoHomebrew:masterfrom
Bo98:openssl-old-no-keychain
Closed

ca-certificates 2021-07-05 (new formula)#86304
Bo98 wants to merge 2 commits intoHomebrew:masterfrom
Bo98:openssl-old-no-keychain

Conversation

@Bo98
Copy link
Copy Markdown
Member

@Bo98 Bo98 commented Oct 2, 2021

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Also, stop using the keychain on Yosemite and El Capitan, since these versions don't have the Let's Encrypt root.

I have not tested this on a 10.10 VM yet, but will before merging to make sure the PEM route actually works.

@Bo98 Bo98 added do not merge CI-skip-recursive-dependents Pass --skip-recursive-dependents to brew test-bot. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-skip-dependents Pass --skip-dependents to brew test-bot. CI-no-bottles Merge without publishing bottles labels Oct 2, 2021
@BrewTestBot BrewTestBot added the CI-linux-self-hosted Build on Linux self-hosted runner label Oct 2, 2021
@Bo98 Bo98 removed the CI-linux-self-hosted Build on Linux self-hosted runner label Oct 2, 2021
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's just make this a separate formula. It's in use in multiple formulae anyway.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure of the best strategy for that.

Ideally we'd handle the keychain stuff there and then the file can be used by multiple formula. But the problem is we currently use openssl in that processing. A bit of a chicken and egg problem.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Or maybe /usr/bin/openssl is enough actually. I forget that still exists.

Copy link
Copy Markdown
Member

@carlocab carlocab Oct 2, 2021

Choose a reason for hiding this comment

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

I don't follow -- the cacert resource is used only in post_install_from_pem, where no openssl binary is needed. Where do we need openssl here?

Or, I guess alternatively we can just use /etc/ssl/cert.pem. Is there any reason why we don't want that one?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't follow -- the cacert resource is used only in post_install_from_pem, where no openssl binary is needed. Where do we need openssl here?

The keychain part. I've just pushed what I was thinking of.

Or, I guess alternatively we can just use /etc/ssl/cert.pem

I can't recall the differences between that and the keychain, but we're going to want to avoid that on older macOS anyway since it's severely outdated.

@Bo98 Bo98 removed the CI-no-bottles Merge without publishing bottles label Oct 2, 2021
@Bo98 Bo98 force-pushed the openssl-old-no-keychain branch from 7af4003 to 9f51be7 Compare October 2, 2021 02:51
@Bo98 Bo98 changed the title openssl@1.1: don't use keychain on Yosemite and El Capitan cacert 2021-07-05 (new formula) Oct 2, 2021
Copy link
Copy Markdown
Member

@carlocab carlocab Oct 2, 2021

Choose a reason for hiding this comment

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

I was thinking of making this a Linux- and old-macOS-only dependency, so that we can keep the keychain post_install in this formula. This way we can still do depends_on "cacert" in gnutls, which currently has its own logic for generating the CA cert bundle.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

gnutls does the exact same Keychain stuff. I'm not sure I follow how there wouldn't be a conditional in gnutls as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not exactly the same:

# XXX And drop Kerberos certs which may invalidate the whole trust store
# due to bug in gnutls (https://gitlab.com/gnutls/gnutls/-/issues/1255)
IO.popen("openssl x509 -inform pem -issuer -noout 2>/dev/null", "r+") do |openssl_io|
openssl_io.write(cert)
openssl_io.close_write
cn = openssl_io.read
openssl_io.close_read
cn.exclude? "CN=com.apple.kerberos.kdc"
end

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Honestly, I see no reason why that can't apply to OpenSSL as well. It's a self-signed Kerberos certificate - not a SSL root certificate.

@Bo98 Bo98 force-pushed the openssl-old-no-keychain branch 3 times, most recently from 46fc08d to 6a73295 Compare October 2, 2021 03:02
@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Oct 2, 2021

The general idea I have here is I would also like to get rid of the copy paste chaos we have where I have to make the same change in 3 different places. Plus decoupling CA cert updates from an OpenSSL & GnuTLS revision bump is nice.

@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Oct 2, 2021

Oh and to resolve #54235 finally.

@Bo98 Bo98 force-pushed the openssl-old-no-keychain branch from 6a73295 to a7fdbe7 Compare October 2, 2021 03:20
@carlocab
Copy link
Copy Markdown
Member

carlocab commented Oct 2, 2021

Oh and to resolve #54235 finally.

@FiloSottile will be pleased to learn about this. (Apologies for the ping, but this stuff seems to be in your wheelhouse, so it'd be nice to hear your thoughts here too.)

@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Oct 2, 2021

@FiloSottile will be pleased to learn about this.

Just to summarise, the idea is:

HOMEBREW_PREFIX/etc/cacert/cert.pem is the only copy on the system (or whatever we call the formula).

HOMEBREW_PREFIX/etc/openssl@1.1/cert.pem is a symlink to the above. Same with those in gnutls, openssl@3 etc.

@Bo98 Bo98 force-pushed the openssl-old-no-keychain branch from a7fdbe7 to dd59599 Compare October 2, 2021 03:29
@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Oct 2, 2021

@Homebrew/core I know this topic has come up on a number of occasions on Slack so here's a ping if you're interested (and sorry if you're not).

@carlocab carlocab added the maintainer feedback Additional maintainers' opinions may be needed label Oct 2, 2021
@Bo98 Bo98 force-pushed the openssl-old-no-keychain branch from dd59599 to 508bfbe Compare October 2, 2021 23:55
@Bo98 Bo98 changed the title cacert 2021-07-05 (new formula) ca-certificates 2021-07-05 (new formula) Oct 2, 2021
@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Oct 3, 2021

I’m not sure I understand the choice of merging the Mozilla and Apple root stores. Is it just to bring in the Let’s Encrypt root on older macOS versions?

Yes.

Could it be scoped to only systems that are not getting updates from Apple?

Apple seem to rarely update older trust stores. The Mojave cert store, for example, has not been modified since the 2019-002 security update (December 2019). Catalina's cert store has not been modified since June 2020 (presumably 10.15.6). Monterey (beta) meanwhile was updated a month ago.

So "not getting update from Apple" might be anything that's not the latest version.

I did a quick skim of the logic, but it seems to ignore the root purposes: in a store you can have roots trusted for email, code signing, Kerberos, TLS… you usually only want to extract the latter.

Yep I absolutely agree. This should have been added when the keychain was expanded beyond Apple roots. I've added a restriction to the "SSL Client CA" and "SSL Server CA" purpose.

-p ssl would probably also work with verify-cert but apparently that starts making network calls.

The Mozilla store also has purposes, but what you get depends on where you download it. Maybe curl did the filtering for you.

Curl store filters to only include SERVER_AUTH.

@Bo98 Bo98 force-pushed the openssl-old-no-keychain branch 2 times, most recently from ddf8111 to b08ff2d Compare October 3, 2021 16:57
@FiloSottile
Copy link
Copy Markdown
Contributor

Yep I absolutely agree. This should have been added when the keychain was expanded beyond Apple roots. I've added a restriction to the "SSL Client CA" and "SSL Server CA" purpose.

You probably want only "SSL Server CA", which are the ones actually bound by the Baseline Requirements. (It also matches curl's SERVER_AUTH.)

@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Oct 3, 2021

Yep I absolutely agree. This should have been added when the keychain was expanded beyond Apple roots. I've added a restriction to the "SSL Client CA" and "SSL Server CA" purpose.

You probably want only "SSL Server CA", which are the ones actually bound by the Baseline Requirements. (It also matches curl's SERVER_AUTH.)

Yeah kinda just realised after pushing. Fixed now!

@Bo98 Bo98 force-pushed the openssl-old-no-keychain branch from b08ff2d to ca7ae59 Compare October 3, 2021 17:09
Copy link
Copy Markdown
Member

@carlocab carlocab Oct 3, 2021

Choose a reason for hiding this comment

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

Won't this make everyone have the post_install done twice when they brew upgrade or brew install openssl@1.1 once this is merged? I can see why we need it, but I wonder if there's a way to avoid doing this twice in a single brew invocation.

We can save this for later though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can probably drop this in favour of regular ca-certificates updates & revision bumps, which is what we should be doing.

@Bo98 Bo98 force-pushed the openssl-old-no-keychain branch from ca7ae59 to d321d6f Compare October 3, 2021 17:19
@Bo98 Bo98 added the new formula PR adds a new formula to Homebrew/homebrew-core label Oct 3, 2021
@carlocab
Copy link
Copy Markdown
Member

carlocab commented Oct 4, 2021

Just noticed we used to ship a CA bundle: Homebrew/legacy-homebrew#28658

This doesn't necessarily have any bearing on what we're doing here today, but I thought I'd link this here for anyone who needs to look up things we've done before and the reasons for them.

@Bo98 Bo98 removed the do not merge label Oct 4, 2021
@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Oct 4, 2021

So many chickens, so many eggs.

10.14 audit of course fails because it needs Homebrew/brew#12167, but Homebrew/brew#12167 in turn needs this PR.

Anyhow, this should be good to go.

@BrewTestBot
Copy link
Copy Markdown
Contributor

:shipit: @Bo98 has triggered a merge.

@carlocab
Copy link
Copy Markdown
Member

carlocab commented Oct 4, 2021

🐔 🥚 🐔

@carlocab
Copy link
Copy Markdown
Member

carlocab commented Oct 4, 2021

Not an all bottle. Very strange.

@carlocab
Copy link
Copy Markdown
Member

carlocab commented Oct 4, 2021

Timestamps are off:

❯ diffoscope ca-certificates--2021-09-30.arm64_big_sur.bottle.tar.gz ca-certificates--2021-09-30.mojave.bottle.tar.gz
--- ca-certificates--2021-09-30.arm64_big_sur.bottle.tar.gz
+++ ca-certificates--2021-09-30.mojave.bottle.tar.gz
├── filetype from file(1)
│ @@ -1 +1 @@
│ -gzip compressed data, was "ca-certificates-bottle.tar", last modified: Thu Sep 30 21:42:41 2021, from Unix
│ +gzip compressed data, was "ca-certificates-bottle.tar", last modified: Mon Oct  4 02:43:56 2021, from Unix
│   --- ca-certificates--2021-09-30.arm64_big_sur.bottle.tar
├── +++ ca-certificates--2021-09-30.mojave.bottle.tar
│ ├── file list
│ │ @@ -1,6 +1,6 @@
│ │ -drwxr-xr-x   0        0        0        0 2021-09-30 21:42:41.000000 ca-certificates/2021-09-30/
│ │ -drwxr-xr-x   0        0        0        0 2021-09-30 21:42:41.000000 ca-certificates/2021-09-30/.brew/
│ │ --rw-r--r--   0        0        0     4226 2021-09-30 21:42:41.000000 ca-certificates/2021-09-30/.brew/ca-certificates.rb
│ │ -drwxr-xr-x   0        0        0        0 2021-09-30 21:42:41.000000 ca-certificates/2021-09-30/share/
│ │ -drwxr-xr-x   0        0        0        0 2021-09-30 21:42:41.000000 ca-certificates/2021-09-30/share/ca-certificates/
│ │ --rw-r--r--   0        0        0   203007 2021-09-30 21:42:41.000000 ca-certificates/2021-09-30/share/ca-certificates/cacert.pem
│ │ +drwxr-xr-x   0        0        0        0 2021-10-04 02:43:56.000000 ca-certificates/2021-09-30/
│ │ +drwxr-xr-x   0        0        0        0 2021-10-04 02:43:56.000000 ca-certificates/2021-09-30/.brew/
│ │ +-rw-r--r--   0        0        0     4226 2021-10-04 02:43:56.000000 ca-certificates/2021-09-30/.brew/ca-certificates.rb
│ │ +drwxr-xr-x   0        0        0        0 2021-10-04 02:43:56.000000 ca-certificates/2021-09-30/share/
│ │ +drwxr-xr-x   0        0        0        0 2021-10-04 02:43:56.000000 ca-certificates/2021-09-30/share/ca-certificates/
│ │ +-rw-r--r--   0        0        0   203007 2021-10-04 02:43:56.000000 ca-certificates/2021-09-30/share/ca-certificates/cacert.pem

@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Oct 4, 2021

That is really weird and makes no sense. I could try a rebottle? It definitely was working in one of the attempts here, and I've only been fiddling with postinstall since.

@carlocab
Copy link
Copy Markdown
Member

carlocab commented Oct 4, 2021

I'll poke at it; go to bed.

Copy link
Copy Markdown
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.

Great work here!

Comment on lines +16 to +18
macos_post_install
else
linux_post_install
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd be tempted to just inline these.

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Oct 10, 2021
This change aligns the formula with OpenSSL 1.1. See Homebrew#86304.

Also, make this keg-only on Linux too while we're still primarily using
`openssl@1.1`, and remove `enable-md2`.
BrewTestBot pushed a commit that referenced this pull request Oct 10, 2021
This change aligns the formula with OpenSSL 1.1. See #86304.

Also, make this keg-only on Linux too while we're still primarily using
`openssl@1.1`, and remove `enable-md2`.

Closes #87023.

Signed-off-by: Bo Anderson <mail@boanderson.me>
Signed-off-by: BrewTestBot <1589480+BrewTestBot@users.noreply.github.com>
@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

CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-skip-dependents Pass --skip-dependents to brew test-bot. CI-skip-recursive-dependents Pass --skip-recursive-dependents to brew test-bot. maintainer feedback Additional maintainers' opinions may be needed new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants