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

Fixes #37601 - Add Foreman CA refresh template #10208

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

ShimShtein
Copy link
Member

@ShimShtein ShimShtein commented Jun 16, 2024

In this PR I am introducing a way to refresh CA certificate for Foreman
server. It will have the following parts:

  • Downloadable template to run directly on a server
  • REX script template to be used with SSH REX provider
  • REX Ansible template to be used with Ansible REX provider

All the ways would refresh katello-server-ca.pem file and refresh
CA root store accordingly.

ekohl
ekohl previously requested changes Jun 18, 2024
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Normally when rotating certificates you would make sure both the old and the new CA are in the PEM file. Then all clients are updated. Only once all clients have been updated, you would rotate the actual certificates on Foreman. Then you can drop the old CA on all clients.

In an ideal world you'd rotate the certificate in a way that clients themselves can refresh it. subscription-manager has the refresh command to update the entitlement, but I don't think it has a mechanism to update files in ca_cert_dir. That means you'd use REX/Ansible/Puppet to do this.

I'm looking at maintenance and I think the name implies this is a normal procedure, but IMHO it's more of a recovery procedure. I was thinking about naming it scripts to be generic, but REX also has a Script provider so it may become confusing.

One further thing to note: AFAIK this is retrieved over plain text HTTP. That by definition makes it insecure. So it really is a disaster recovery situation, not regular maintenance.

test/controllers/unattended_controller_test.rb Outdated Show resolved Hide resolved
CA_TRUST_ANCHORS=/etc/pki/ca-trust/source/anchors
fi

# Add the Katello CA certificate to the system-wide CA certificate store
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually do this today? I don't see why we need to.

When we use custom certificates (like when Let's Encrypt is used as a CA) it should already be in the certificate store.

Perhaps this should also check if $CA_TRUST_ANCHORS/katello-server-ca.pem exists.

Copy link
Member

@ares ares Jul 23, 2024

Choose a reason for hiding this comment

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

yes, the katello-rhsm-consumer script does it for some reason today

@ares
Copy link
Member

ares commented Jun 20, 2024

I think we need a PR message template like on Katello. The intention is not clear from the primary comment, I only understand it since I know the context from JIRA and read the code. I think @ekohl found out the reason based on the code.

I believe the template is supposed to be used when CA cert expired and we need to reinstall the CA on the hosts. It could (and should) be probably also used in Anaconda's rhsm command, which today relies on katello-ca-consumer script being available at /var/www/pub, to deploy the rhsm certs. It may be also nice to have a job template, but that is less important now. We need a way to get the script out of the Foreman, that configures the certificates.

I find the maintenance term also a but misleading. The setting description does not help much. If there is no other clear responsibility of the template besides certs setup, let's be explicit about that.

@ShimShtein
Copy link
Member Author

ShimShtein commented Jun 20, 2024

@ares Fixed the message, hopefully it's a bit more clear now.

@ekohl About the maintenance template type: I think we should have a category for "All actions needed to make sure that Foreman and the host play nice together".
The options I can see here:

  1. Put this script to a separate category (feels like an abuse for template kind)
  2. Move it to script category (means we will need to expose this whole category to unauthorized users)
  3. Rename it to unauthorized. It will contain all templates that we want to be accessible without authorization.

Edit:
I will add the job templates as follow-ups in this PR.

As for the ideal use case - I agree that ideally we should orchestrate the CA change in Foreman and the change in hosts, but I am not entirely sure that it would be feasible.
I would suggest having this template (plus the REX jobs) as the "good enough" solution.

@ekohl
Copy link
Member

ekohl commented Jun 20, 2024

Naming is one of the hardest parts. No definitive preference in this comment, but responding to one part.

Rename it to unauthorized. It will contain all templates that we want to be accessible without authorization.

I'd prefer something like anonymous over unauthorized because effectively everyone is authorized to access it.

ShimShtein added a commit to ShimShtein/foreman_remote_execution that referenced this pull request Jun 20, 2024
The main idea is to execute scripts from the Foreman server,
similar to theforeman/foreman#10208.
ShimShtein added a commit to ShimShtein/foreman_ansible that referenced this pull request Jun 20, 2024
The main idea is to execute scripts from the Foreman server, similar to theforeman/foreman#10208.
@ekohl
Copy link
Member

ekohl commented Jun 20, 2024

I noticed you created REX/Ansible jobs to retrieve this script, but if you can already log in on a host via SSH then I'd prefer a complete "configure RHSM according to Foreman" job.

It really surprises me redhat_subscription_module is in community, which means it's not part of base RHEL. That would have been exactly what we want: populate the variables with the info already present in Foreman/Katello.

ShimShtein added a commit to ShimShtein/foreman_remote_execution that referenced this pull request Jun 26, 2024
The main idea is to execute scripts from the Foreman server,
similar to theforeman/foreman#10208.
@ShimShtein ShimShtein changed the title Add Foreman CA refresh template Fixes #37601 - Add Foreman CA refresh template Jun 27, 2024
@ShimShtein
Copy link
Member Author

Removed host verification from the controller. Now the endpoint became entirely open, and it's up to a template to decide if it fail the rendering in case the host is not known.

Specifically in case of CA refresh, there is no sensitive data in the template, and it can be public.

@ekohl
Copy link
Member

ekohl commented Jul 1, 2024

There's now an RFC discussing this: https://community.theforeman.org/t/rfc-replace-pub-katello-rhsm-consumer-static-script-with-a-template/38494.

@ShimShtein ShimShtein force-pushed the replace_ca branch 3 times, most recently from 9d34bda to 4b01c2f Compare July 15, 2024 11:40
@ShimShtein ShimShtein force-pushed the replace_ca branch 2 times, most recently from 87007f8 to 7923618 Compare July 22, 2024 12:26
@ShimShtein
Copy link
Member Author

@ekohl @ares Any comments? The test failures do not seem related.

@ares
Copy link
Member

ares commented Jul 23, 2024

The code looks reasonable to me, let me know if you need a hand with the testing. I'd prefer not merging it myself though.

@shubhamsg199
Copy link

shubhamsg199 commented Jul 29, 2024

Tested manually for templates, both foreman_ca_refresh foreman_raw_ca are working fine.

  • I was able to download the foreman_ca_refresh template directly and the certs were refreshed after executing it as expected.

  • foreman_raw_ca correctly fetches the cert, so the user can directly copy it to the certs file.

ShimShtein added a commit to ShimShtein/foreman_ansible that referenced this pull request Jul 31, 2024
The main idea is to execute scripts from the Foreman server, similar to theforeman/foreman#10208.
In this PR I am introducing a way to refresh CA certificate for Foreman
server. It will have the following parts:
 [X] Downloadable template to run directly on a server
 [ ] REX script template to be used with SSH REX provider
 [ ] REX Ansible template to be used with Ansible REX provider

All the ways would refresh `katello-server-ca.pem` file and refresh
CA root store accordingly.

Also added the certs to the ENC, so every ENC consumer would be able
to use them to refresh Foreman's CA on host.
stejskalleos pushed a commit to theforeman/foreman_ansible that referenced this pull request Aug 1, 2024
The main idea is to execute scripts from the Foreman server, similar to theforeman/foreman#10208.
Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

🍏

  • All comments have been addressed
  • QEs confirmed the functionality of the feature
  • No other issues have been raised

Let's get this in, so we can have it in the next snap.

Thanks @ShimShtein @ekohl @shubhamsg199 @ares and everyone else involved in this effort.

@stejskalleos stejskalleos merged commit d7a8a1e into theforeman:develop Aug 1, 2024
52 of 53 checks passed
ShimShtein added a commit to ShimShtein/foreman_ansible that referenced this pull request Aug 1, 2024
… script

The main idea is to execute scripts from the Foreman server, similar to theforeman/foreman#10208.
ShimShtein added a commit to ShimShtein/foreman_remote_execution that referenced this pull request Aug 1, 2024
The main idea is to execute scripts from the Foreman server,
similar to theforeman/foreman#10208.
ShimShtein added a commit to ShimShtein/foreman_remote_execution that referenced this pull request Aug 1, 2024
The main idea is to execute scripts from the Foreman server,
similar to theforeman/foreman#10208.
ofedoren pushed a commit to theforeman/foreman_remote_execution that referenced this pull request Aug 1, 2024
The main idea is to execute scripts from the Foreman server,
similar to theforeman/foreman#10208.
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