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

Use CA certificate from /etc/pki/trust/anchors in rhnpush #7364

Merged

Conversation

mbussolotto
Copy link
Member

What does this PR change?

Use CA certificate from /etc/pki/trust/anchors in rhnpush

GUI diff

No difference.

  • DONE

Documentation

Test coverage

  • Cucumber tests were added

  • DONE

Links

  • DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

  • No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

  • Re-run test "changelog_test"
  • Re-run test "backend_unittests_pgsql"
  • Re-run test "java_pgsql_tests"
  • Re-run test "schema_migration_test_pgsql"
  • Re-run test "susemanager_unittests"
  • Re-run test "javascript_lint"
  • Re-run test "spacecmd_unittests"

@mcalmer
Copy link
Contributor

mcalmer commented Aug 18, 2023

@mbussolotto I wonder if we should maybe use the default trust chain? This would be a change but as this will be a standalone tool in 5.0 for the clients it might be the right move. In this case the path would be /etc/ssl/ca-bundle.pem on SUSE, but would be different on other OSes.

@mbussolotto
Copy link
Member Author

@mcalmer thanks for pointing out! Although this solution workw and it has been already tested in our dev branch, the main goal of this PR is to merge asap changes required for 5.0, in order to prevent possible last minute issue. So if you think the right move is to use the default trust chain, I think we should go in that direction. @cbosdo what do you think?

@cbosdo
Copy link
Contributor

cbosdo commented Aug 21, 2023

Would the default trust bundle include the Uyuni CA? I would bet it doesn't if using self-signed CA. If we want such a change, then yes I would rather have it in a separate issue.

@mcalmer
Copy link
Contributor

mcalmer commented Aug 21, 2023

Would the default trust bundle include the Uyuni CA? I would bet it doesn't if using self-signed CA. If we want such a change, then yes I would rather have it in a separate issue.

If we put the CA in /etc/pki/trust/anchor/ and call update-ca-certificate, it will get included into the bundle.
AFAIK we do exactly this :-)

@cbosdo
Copy link
Contributor

cbosdo commented Aug 21, 2023

Would the default trust bundle include the Uyuni CA? I would bet it doesn't if using self-signed CA. If we want such a change, then yes I would rather have it in a separate issue.

If we put the CA in /etc/pki/trust/anchor/ and call update-ca-certificate, it will get included into the bundle. AFAIK we do exactly this :-)

OK, now I got what you mean, and yes it probably isn't much to do. I'll see if I can squash it in.

@mbussolotto
Copy link
Member Author

In this case the path would be /etc/ssl/ca-bundle.pem on SUSE, but would be different on other OSes.

I'm doing these changes. @mcalmer @cbosdo Just to be sure since https://github.com/openSUSE/ca-certificates/blob/2dae8b77c250506dc1bf862351c3a5de89a08e90/README#L26-L27 ... why README suggests to not use /etc/ssl/ca-bundle.pem ?

@mcalmer
Copy link
Contributor

mcalmer commented Aug 30, 2023

@mbussolotto if you want to include the bundle you should use /etc/ssl/ca-bundle.pem . But when rhnpush is running on a not SUSE OS, this path might be different.

@cbosdo
Copy link
Contributor

cbosdo commented Sep 1, 2023

Looks like we need more work on this one:
image

Copy link
Contributor

@cbosdo cbosdo left a comment

Choose a reason for hiding this comment

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

Acceptance tests going red due to an issue with SSL and rhnpush to be fixed before merging

@mbussolotto
Copy link
Member Author

Yes I noticed it, I'll work on it

@mbussolotto mbussolotto force-pushed the rhnpush_ca_cerfificate_path branch 3 times, most recently from 361fa19 to ebb93af Compare September 4, 2023 13:31
@jordimassaguerpla
Copy link
Contributor

How does the certificate end up in /etc/pki/trust/anchors/LOCAL-RHN-ORG-TRUSTED-SSL-CERT ? It is installed by some RPM? I suspect that is the case and in the containers we are not building the RPMS with the new code but copying the code. We are installing the latest released RPMs, which I assume have the certificate in /usr/share/rhn/RHN-ORG-TRUSTED-SSL-CERT .

@jordimassaguerpla
Copy link
Contributor

Would it be an option to make this code backward compatible ? I mean, check if /etc/pki/trust/anchors/LOCAL-RHN-ORG-TRUSTED-SSL-CERT exist and otherwise use /usr/share/rhn/RHN-ORG-TRUSTED-SSL-CERT ? Why are we doing this change?

@mcalmer
Copy link
Contributor

mcalmer commented Sep 7, 2023

In a Hub scenario both might exist where RHN-... is the one from the Hub and LOCAL-RHN... is the local CA cert.
We hope it is the same, but in principal it could happen that they differ.
In the past we had the problem that one was overwriting the other.

RHN-... comes from bootstrapping the system as a client to another SUMA Server (Perefiral Server connect to Hub)
while LOCAL-RHN... comes from the new import certificates script .

@cbosdo
Copy link
Contributor

cbosdo commented Sep 7, 2023

In a Hub scenario both might exist where RHN-... is the one from the Hub and LOCAL-RHN... is the local CA cert. We hope it is the same, but in principal it could happen that they differ. In the past we had the problem that one was overwriting the other.

RHN-... comes from bootstrapping the system as a client to another SUMA Server (Perefiral Server connect to Hub) while LOCAL-RHN... comes from the new import certificates script .

Ouch! I wasn't aware of this difference... We'll need more thinking into it

@mbussolotto mbussolotto marked this pull request as draft September 13, 2023 15:52
Copy link
Contributor

@cbosdo cbosdo left a comment

Choose a reason for hiding this comment

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

Removing the defaults for ca_chain gets the bundle detection to be used and we don't need to know the CA path at all.

@cbosdo cbosdo marked this pull request as ready for review September 15, 2023 06:35
@cbosdo
Copy link
Contributor

cbosdo commented Sep 15, 2023

Tested to work on the K3S CI:
Screenshot from 2023-09-15 08-29-06

mbussolotto and others added 2 commits September 15, 2023 17:42
In order to test changes in rhnpush we need to deploy the python and
config files to the container.
@cbosdo cbosdo added the ready PR ready, waiting branch is unlocked label Sep 15, 2023
@cbosdo cbosdo merged commit b3e054c into uyuni-project:master Oct 2, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants