Taught https-download to trust system certstore.

[ggainey]

fixes #3036.

[dralley]

@bmbouter @dkliban Do you have any objections to the current approach, vaguely remember some discussion about it but I don't recall if there was a specific objection. The only thing I can find written down is a different approach suggestion https://bugzilla.redhat.com/show_bug.cgi?id=1993917#c91

[ggainey]

@bmbouter @dkliban Do you have any objections to the current approach, vaguely remember some discussion about it but I don't recall if there was a specific objection. The only thing I can find written down is a different approach suggestion https://bugzilla.redhat.com/show_bug.cgi?id=1993917#c91

The showstopper, iirc, was that prior to python-3.11 we had to set a protected attribute - see the discussion here : #3038 (comment)

With py-3.11, that's now the default and we don't have to hack around it.

[dralley]

I thought there was also some objection about the loading of system certs, maybe #3038 (comment)?

[ggainey]

I thought there was also some objection about the loading of system certs, maybe #3038 (comment)?

I have a memory of a discussion, that I don't think got written down anywhere alas. We make the user give us the CA for a Remote, because "the system" pulp is installed on may not want to trust that connection globally. But Pulp should be allowed to talk to anything "the system" has already decided is legal/allowed, and shouldn't need to "double accept" something the system-admin has already added to the global keystore.

Does anyone else remember that conversation, or did I dream it?

[dralley]

I have vague memory of this discussion, yes. I don't recall the outcome though.

[bmbouter]

I could imagine folks not wanting this always on. I'm guessing here because I'm not one of those folks, but say you generally don't trust the thousands of CAs that are typically loaded...

[bmbouter]

That being said, we should have a way to enable this for sure. This is highly valuable for almost everyone.

[ggainey]

I could imagine folks not wanting this always on. I'm guessing here because I'm not one of those folks, but say you generally don't trust the thousands of CAs that are typically loaded..

In this context, pulp has been told "use this proxy", and that proxy's CA has been defined as trusted by the environment pulp is installed in. I can't actually think of a "hat" to wear, where asking pulp to extra-double-trust the proxy is actually useful.

[dralley]

Well, you could extend that logic to say "we've told Pulp to sync this remote, and the CA used by this remote's URL is already trusted by the environment". Historically though, we've asked that the certificate to verify against be explicitly provided.

I actually think the same logic does make sense there, and that we perhaps should trust system certs automatically when applied to remotes in general (maybe unless a specific one is provided?), but that's not historically what we've done.

[ggainey]

Well, you could extend that logic to say "we've told Pulp to sync this remote, and the CA used by this remote's URL has been defined as trusted by the environment". Historically though, we've asked that the certificate to verify against be explicitly provided.

My contention is (and has been) that proxy-trust is qualitatively different than remote-trust. The Environment pulp is running is, has declared "this proxy can be trusted" - that's not the pulp-admin's purview. For the remotes being sync'd, only pulp cares - which is why we have the admin set the CA for the remote.

I actually think the same logic does make sense there, and that we probably should trust system certs automatically when applied to remotes in general (maybe unless a specific one is provided?), but that's not historically what we've done

Basically, if it's in the system-keystore, we should prob trust it, even for Remotes. But as you say, that isn't what we've been doing - and that is a different question than "do we trust the system's definition for This Proxy".

[dralley]

It's a different question but it would still be nice if we have consistent logic / rules about this.

Anyway, your explanation makes some sense and in any case I don't consider consistency necessarily a blocker for this PR

I think it would be good if we expanded the documentation to include these subtleties though.

[dralley]

Sorry, didn't mean to close.

[dralley]

I think I'm OK with this on the basis of the discussion. Any objections?

I believe we also want to backport this to 3.39 as downstream needs this, but that's perhaps a bit murky. It's not a breaking change but it is perhaps a reasonably significant one?

[ggainey]

I think I'm OK with this on the basis of the discussion. Any objections?

I believe we also want to backport this to 3.39 as downstream needs this, but that's perhaps a bit murky. It's not a breaking change but it is perhaps a reasonably significant one?

Def wants to be backported to 3.39 - it's not a New Feature, so much as Removing a Restriction that was imposed by a combination of python<3.11 and older aiohttp.

[bmbouter]

My claim is probably inconvenient, but this change is really significant and I don't think it's good to backport. Imagine that you upgrade Pulp and now all of a sudden your environment trusts thousands of CAs. That's just my opinion though, don't consider it a blocking statement if you both still want to proceed.

[bmbouter]

I think this kind of a feature should be feature flagged on a remote itself. Say you are using ca_cert and your expectation is this is the only thing I trust. Now you look at your remote and you will trust that a bunch of other things. Users and admins have different expectations around what to trust.

Side question: How do users even know what the are trusting when they have no access to the system store. So an admin decides things and then users also must trust it? It's just confusing, but maybe it's ok? Just some random thoughts here.

[ggainey]

My claim is probably inconvenient, but this change is really significant and I don't think it's good to backport. Imagine that you upgrade Pulp and now all of a sudden your environment trusts thousands of CAs. That's just my opinion though, don't consider it a blocking statement if you both still want to proceed.

Pulp's remotes already trust a lot of CAs - if they didn't, you would have to submit a server-CA every time you specified an https: Remote, which you don't, unless that URL serves a cert not managed by one of the system-accepted CAs (like, say, Red Hat's CDN CA cert).

It's only the proxy connection we implement that doesn't have this in place.

I really don't see this as a "really signifcant" change - other than "basic proxy functionality will start working for Pulp that didn't before". (Also, note that in Pulp2 use of https proxies worked, and did not require specifying the root-CA for such a proxy)

[ggainey]

I think this kind of a feature should be feature flagged on a remote itself. Say you are using ca_cert and your expectation is this is the only thing I trust. Now you look at your remote and you will trust that a bunch of other things. Users and admins have different expectations around what to trust.

I contend that generally proxies aren't trusted on a per-destination basis - every outgoing connection from a system has to go through the same proxy or it's disallowed. And every SSL connection that leaves your system is going to trust what's in the system's CAstore, unless the app you're using very specifically decides NOT to do that.

Side question: How do users even know what the are trusting when they have no access to the system store. So an admin decides things and then users also must trust it? It's just confusing, but maybe it's ok? Just some random thoughts here.

Your system trusts everything in /usr/share/pki/ca-trust-source/ - every time you use curl or wget or httpie with an HTTPS URL, and it works, that's why. That's the store we are trusting.

The common use-case is your network admin decrees "you will use the following proxy for all requests, or they will never leave your machine". In real environments, proxy-use is imposed on systems at a higher level than per-application.

Browsers have only relatively recently started carrying their own certstores. This was a response to browsers running on end-user machines that have no administrators, and which can end up with rootkits that install bogus certs. (Of course, the Bad Guys now just install their certs into the browser certstores when they get access, in addition to the system-one...)

[bmbouter]

I think I misunderstood what was going on here, and you've set me straight. Pulp doesn't trust the system trust store, but it does trust certifi. Given that he scope of what is changing here is way smaller than I thought. +1 to proceeding.

[patchback]

Backport to 3.39: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply e211930 on top of patchback/backports/3.39/e2119307497f401de95e463e370787143c524016/pr-4951

Backporting merged PR #4951 into main

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/pulp/pulpcore.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.39/e2119307497f401de95e463e370787143c524016/pr-4951 upstream/3.39

4. Now, cherry-pick PR Taught https-download to trust system certstore. #4951 contents into that branch:

   $ git cherry-pick -x e2119307497f401de95e463e370787143c524016

   If it'll yell at you with something like fatal: Commit e2119307497f401de95e463e370787143c524016 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x e2119307497f401de95e463e370787143c524016

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Taught https-download to trust system certstore. #4951 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.39/e2119307497f401de95e463e370787143c524016/pr-4951

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.