Fixes #30368 - Make Candlepin CA file optional#8832
Conversation
|
Issues: #30368 |
|
Related, and based on some discussion with @jlsherrill , today Pulp communication relies on the the CA being trusted by the system (the whole use of trusted_ca module in the installer) and we should switch to doing this explicitly -- |
6d1bd33 to
8ef3711
Compare
|
I took a stab at this. In doing so, I found 2 settings that were removed but still had references to them. It then also unifies the certificate handling in the certs service. There are a few more changes, documented in the commit message. Please read it and let me hear your thoughts. |
|
[test katello] |
8ef3711 to
85f4f40
Compare
|
One test failure seems relevant |
85f4f40 to
d483104
Compare
|
Rebased to resolve merge conflict and to get recent tests. I've also split of unrelated work to #9268 since it doesn't impact this PR. |
|
Haven't tested yet but the test failure definitely seems related |
|
Given stabilization is coming soon and my lack of time, I'm afraid we'll have to push this to the next Katello release. |
|
@ekohl is this PR still valid ? Rebase if so |
theforeman-bot
added
Waiting on contributor
and removed
Needs testing
Not yet reviewed
labels
|
@ekohl, this pull request is currently not mergeable. Please rebase against the master branch and push again. If you have a remote called 'upstream' that points to this repository, you can do this by running: This message was auto-generated by Foreman's prprocessor |
d483104 to
48289b6
Compare
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
I've tried to rebase it, but I'm a bit unclear about the current Pulp 2 vs Pulp 3 status. I still see traces of Runcible, but isn't that Pulp 2-only? I now think this unifies everything. It does mean the system store isn't really used anymore (unless you don't configure anything I think), but that's probably a good thing. Perhaps @ehelms and @evgeni could also think about what kind of behavior we would prefer to have from a platform point of view. |
Rather than using the default value, this uses the actual value that was passed in. Fixes: d6adef8 ("Fixes #34070 - fix container registry with azure plugin")
48289b6 to
b97646c
Compare
b97646c to
daa57eb
Compare
There was a problem hiding this comment.
I now see there already is ca_cert_file for this. I don't know what other classes can be passes into pulp3_ssl_configuration so I'm looking for guidance here.
There was a problem hiding this comment.
I included a commit to rename it for consistency with other tools and naming for the other ssl_ attrs.
This is more consistent with other classes.
daa57eb to
9213056
Compare
This change makes the Candlepin CA file optional by falling back to the Foreman CA file. The result is a reduced configuration in most deployments. For Pulp the certificate is now read in the same way as Candlepin. Previously it partly relied on the CA being in the global allowed CA. This may be an issue in some cases, but in the default deployment it isn't. Following the general SSL config makes the configuration more predictable for users. It can also be easier in a containerized setup or on a system where the admin is not allowed to modify the system CA certificates. The example config is now also consistent with reality. The verify_ssl option is dropped from the ping model. This isn't respected elsewhere and it's misleading to have a valid ping only to have it fail at runtime.
9213056 to
92b2ded
Compare
| # refers to the apache certificate | ||
| # (typically /etc/pki/tls/certs/localhost.crt) location that is needed | ||
| # to connect to pulp over https. | ||
| # Optional CA file to user to verify HTTPS connections to Candlepin. If not |
There was a problem hiding this comment.
| # Optional CA file to user to verify HTTPS connections to Candlepin. If not | |
| # Optional CA file to user to verify HTTPS connections to Pulp. If not |
ehelms
left a comment
ehelms
left a comment
There was a problem hiding this comment.
Clean re-factoring that helps set up future simplifications.
|
I had hoped @evgeni could test if this resolves his issue in containers before merging. |
I liked the change from a re-factoring stand point. If this doesn't solve his issue, then we can iterate on solving that. |
|
theforeman/foremanctl#43 is not failing on SSL errors anymore, so it certainly helped :) |
|
By looking at puppet-katello I realized I forgot something because of the very long duration of the PR. While we can remove this line for We can actually simplify that all the way to default to the same client cert credentials that Foreman already uses and make the whole block optional. |
7 participants
This change makes the Candlepin CA file optional by falling back to the Foreman CA file. The result is a reduced configuration in most deployments.
For Pulp the certificate is now read in the same way as Candlepin. Previously it partly relied on the CA being in the global allowed CA. This may be an issue in some cases, but in the default deployment it isn't. Following the general SSL config makes the configuration more predictable for users. It can also be easier in a containerized setup or on a system where the admin is not allowed to modify the system CA certificates. The example config is now also consistent with reality.
The verify_ssl option is dropped from the ping model. This isn't respected elsewhere and it's misleading to have a valid ping only to have it fail at runtime.