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 #32678 - katello_ca_consumer in registration template #8563

Merged
merged 1 commit into from
Oct 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions app/controllers/concerns/foreman/controller/registration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ def global_registration_vars
location: (location || User.current.default_location || User.current.my_locations.first),
hostgroup: host_group,
operatingsystem: operatingsystem,
url_host: registration_url.host,
registration_url: registration_url,
setup_insights: ActiveRecord::Type::Boolean.new.deserialize(params['setup_insights']),
setup_remote_execution: ActiveRecord::Type::Boolean.new.deserialize(params['setup_remote_execution']),
packages: params['packages'],
Expand All @@ -40,6 +38,7 @@ def global_registration_vars
.to_h
.symbolize_keys
.merge(context)
.merge(context_urls)
end

def safe_render(template)
Expand Down Expand Up @@ -96,6 +95,10 @@ def registration_url
fail Foreman::Exception.new(msg)
end

def context_urls
{ registration_url: registration_url }
end

def setup_host_params
clean_host_params

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,36 @@ if [ -f /etc/os-release ] ; then
. /etc/os-release
fi

# Choose package manager
# apt-get for Debian & Ubuntu
# dnf for Fedora (version >= 22) & RHEL family (version > 7)
# yum for Fedora (version < 22) & RHEL family (version < 8)
if [ x$ID = xfedora ]; then
if [ "${VERSION_ID%.*}" -gt 21 ]; then
PKG_MANAGER='dnf'
else
PKG_MANAGER='yum'
fi
Comment on lines +47 to +49
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to support Fedora <21

elif [ -f /etc/redhat-release ] ; then
if [ "${VERSION_ID%.*}" -gt 7 ]; then
PKG_MANAGER='dnf'
else
PKG_MANAGER='yum'
fi
elif [ -f /etc/debian_version ]; then
PKG_MANAGER='apt-get'
fi

SSL_CA_CERT=$(mktemp)
cat << EOF > $SSL_CA_CERT
<%= foreman_server_ca_cert %>
EOF

cleanup_and_exit() {
rm -f $SSL_CA_CERT
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't cleanup KATELLO_SERVER_CA_CERT or roll-back the changes to rhsm.conf

is that intentional?

Copy link
Member

Choose a reason for hiding this comment

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

(probably, because it's also called in the success case, but I think we should be nicer in the error case)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this. Today, on uninstall users set back to rhsm.conf.bak. If this fails, it would be nice to restore them to whatever they had originally so they are not left in a broken state. That is, on failure restore the rhsm.conf.bak to rhsm.conf. Use cases:

  1. User is moving a host from being registered to Red Hat CDN to a Foreman, something fails, they would get reverted and remain connected and operational
  2. User is moving a host from one Foreman to another Foreman, this would prevent the host from being in limbo if it failed

Copy link
Member

Choose a reason for hiding this comment

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

Two Foremans won't work, as the first registration has created a .bak file, so the second registration won't create one (and then restore the original rhsm.conf, from before the first registration, on failure).

But this is probably a corner case we can fix later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doesn't cleanup KATELLO_SERVER_CA_CERT or roll-back the changes to rhsm.conf

is that intentional?

But do we want to remove KATELLO_SERVER_CA_CERT? If we remove the cert then all communication between subscription-manager & foreman will fail with SSL error, right?

Today, on uninstall users set back to rhsm.conf.bak

We should do the backup of the rhsm.conf that's for sure.

If this fails, it would be nice to restore them to whatever they had originally so they are not left in a broken state. That is, on failure restore the rhsm.conf.bak to rhsm.conf.

Is this something what we do right now with katello_ca_consumer.rpm? I mean, if registration with rpm fails, does it do all the rollback steps automatically or users have to do them manually?

Because implementing rollback feature (while catching all the possible fail scenarios) won't be easy.
We need to cleanup methods:

  • When script is successful
  • When script failed

IMHO this is out of the scope of this PR, I would suggest to do it in separate PR and not block this one

Copy link
Member

Choose a reason for hiding this comment

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

But do we want to remove KATELLO_SERVER_CA_CERT? If we remove the cert then all communication between subscription-manager & foreman will fail with SSL error, right?

This and the rhsm.conf roll back is tricky, as today we do this if the RPM is removed (https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/lib/puppet/provider/bootstrap_rpm/bootstrap_rpm.rb#L203). But I do not see us doing it if the re-configure script fails. I would agree that could be left for a follow up given this does reflect current state.

exit $1
}

<% unless @repo.blank? -%>
echo '#'
echo '# Adding repository'
Expand All @@ -58,8 +83,8 @@ gpgkey=<%= shell_escape @repo_gpg_key_url %>
EOF

echo "Building yum metadata cache, this may take a few minutes"
yum makecache
elif [ x$ID = xdebian ] || [ x$ID = xubuntu ]; then
$PKG_MANAGER makecache
elif [ -f /etc/debian_version ]; then
cat << EOF > /etc/apt/sources.list.d/foreman_registration.list
<%= shell_escape @repo %>
EOF
Expand All @@ -71,7 +96,7 @@ EOF

else
echo "Unsupported operating system, can't add repository."
exit 1
cleanup_and_exit 1
fi
<% end -%>

Expand Down Expand Up @@ -102,7 +127,7 @@ echo "#"
if [ -f /etc/redhat-release ]; then
register_katello_host(){
UUID=$(subscription-manager identity | head -1 | awk '{print $3}')
curl --silent --show-error --cacert $SSL_CA_CERT --request POST "<%= @registration_url %>" \
curl --silent --show-error --cacert $KATELLO_SERVER_CA_CERT --request POST "<%= @registration_url %>" \
--data "uuid=$UUID" \
<%= headers.join(' ') %> \
<%= " --data 'host[organization_id]=#{@organization.id}' \\\n" if @organization -%>
Expand All @@ -115,42 +140,77 @@ if [ -f /etc/redhat-release ]; then
<%= " --data packages=#{shell_escape(@packages)} \\\n" if @packages.present? -%>
<%= " --data 'update_packages=#{@update_packages}' \\\n" unless @update_packages.nil? -%>

}
}

KATELLO_SERVER_CA_CERT=/etc/rhsm/ca/katello-server-ca.pem
RHSM_CFG=/etc/rhsm/rhsm.conf

# Backup rhsm.conf
if [ -f $RHSM_CFG ] ; then
test -f $RHSM_CFG.bak || cp $RHSM_CFG $RHSM_CFG.bak
fi

# rhn-client-tools conflicts with subscription-manager package
# since rhn tools replaces subscription-manager, we need to explicitly
# install subscription-manager after the rhn tools cleanup
if [ x$ID = xol ]; then
$PKG_MANAGER remove -y rhn-client-tools
$PKG_MANAGER install -y --setopt=obsoletes=0 subscription-manager
fi

# Prepare SSL certificate
mkdir -p /etc/rhsm/ca
cp -f $SSL_CA_CERT $KATELLO_SERVER_CA_CERT
chmod 644 $KATELLO_SERVER_CA_CERT

# Prepare subscription-manager
stejskalleos marked this conversation as resolved.
Show resolved Hide resolved
<% if @force -%>
if [ -x "$(command -v subscription-manager)" ] ; then
subscription-manager unregister || true
subscription-manager clean
fi

yum remove -y katello-ca-consumer\*
$PKG_MANAGER remove -y katello-ca-consumer\*
<% end -%>

# rhn-client-tools conflicts with subscription-manager package
# since rhn tools replaces subscription-manager, we need to explicitly
# install subscription-manager after the rhn tools cleanup
if [ x$ID = xol ]; then
yum remove -y rhn-client-tools
yum install -y --setopt=obsoletes=0 subscription-manager
if ! [ -x "$(command -v subscription-manager)" ] ; then
$PKG_MANAGER install -y subscription-manager
else
$PKG_MANAGER upgrade -y subscription-manager
fi

CONSUMER_RPM=$(mktemp --suffix .rpm)
curl --silent --show-error --output $CONSUMER_RPM <%= subscription_manager_configuration_url(hostname: @url_host) %>
if ! [ -f $RHSM_CFG ] ; then
echo "'$RHSM_CFG' not found, cannot configure subscription-manager"
cleanup_and_exit 1
fi
stejskalleos marked this conversation as resolved.
Show resolved Hide resolved

# Workaround for systems with enabled FIPS,
# where installation of RPM generated on RHEL7 cause 'no digest' error
# See https://projects.theforeman.org/issues/32068
if [ "$(cat /proc/sys/crypto/fips_enabled)" = "1" ]; then
rpm -ivh --nodigest --nofiledigest $CONSUMER_RPM
# Configure subscription-manager
test -f $RHSM_CFG.bak || cp $RHSM_CFG $RHSM_CFG.bak
subscription-manager config \
--server.hostname="<%= @rhsm_url.host %>" \
--server.port="<%= @rhsm_url.port %>" \
--server.prefix="<%= @rhsm_url.path %>" \
--rhsm.repo_ca_cert="$KATELLO_SERVER_CA_CERT" \
--rhsm.baseurl="<%= @pulp_content_url %>"

# Older versions of subscription manager may not recognize
# report_package_profile and package_profile_on_trans options.
# So set them separately and redirect out & error to /dev/null
# to fail silently.
subscription-manager config --rhsm.package_profile_on_trans=1 > /dev/null 2>&1 || true
subscription-manager config --rhsm.report_package_profile=1 > /dev/null 2>&1 || true

# Configuration for EL6
if grep --quiet full_refresh_on_yum $RHSM_CFG; then
stejskalleos marked this conversation as resolved.
Show resolved Hide resolved
sed -i "s/full_refresh_on_yum\s*=.*$/full_refresh_on_yum = 1/g" $RHSM_CFG
else
yum localinstall $CONSUMER_RPM -y
full_refresh_config="#config for on-premise management\nfull_refresh_on_yum = 1"
sed -i "/baseurl/a $full_refresh_config" $RHSM_CFG
fi

rm -f $CONSUMER_RPM

subscription-manager register <%= '--force' if @force %> \
--org='<%= @organization.label %>' \
--activationkey=<%= shell_escape(activation_keys) %> || <%= @ignore_subman_errors ? 'true' : 'exit 1' %>
--org='<%= @organization.label %>' \
--activationkey='<%= activation_keys %>' || <%= @ignore_subman_errors ? 'true' : 'cleanup_and_exit 1' %>
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I should know this, does this mean that I can only register with an activation key? What happens if I want to register with username and password?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, registration with activation key is the only workflow supported right now. If users wants to use login & password they have to edit the template and add parameters manually

Copy link
Member

Choose a reason for hiding this comment

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

Is username and password support on the Roadmap?

Copy link
Member

Choose a reason for hiding this comment

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

fwiw, bootstrap doesn't support username/pass either today.

Copy link
Member

Choose a reason for hiding this comment

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

Bootstrap does so many things and that is one it does not do? TIL. Let me try to just write out the scenario that I am envisioning and ask a question at the end for discussion of how we want things to work for users.

The move to have GR directly re-configure subscription-manager allows us to drop the bootstrap RPM and centralize on the GR API as the way to register. Today, users can and are used to installing the bootstrap RPM and then calling subscription-manager to register a host with username and password or activation key. This current design enforces using an activation key.

Is that a design choice we want to make? And if yes, how would it affect users who are used to username and password?

Copy link
Member

Choose a reason for hiding this comment

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

I think that in our template we shouldn't really care about username/password. AKs do have benefits and I can imagine that at some point we actually generally a new key on the fly and automatically expire it after it's been used when this workflow is used. Like a one-time-activation-key. Not something to implement here, but having a registration workflow inside Foreman that supports it does make that flow possible.

Copy link
Member

Choose a reason for hiding this comment

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

I know this is "registration", but throwing the idea out there anyway. If a user does not supply an activation key we could opt to configure subscription-manager but not register with subscription-manager and output a message saying this must be done manually by calling subscription-manager. That would allow for this existing workflow to work.

I am OK if we drop username/password support. I would want us to be clear about that and call it out as a deprecated or removed (depending on timeline) feature for users in our release notes.

I'd like to also get @jlsherrill and @jturel to think on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am too kinda hesitant to completely remove this functionality. Its generally the scenario that developers use, and i wouldnt' be surprised if users did too. Its even mentioned in the docs: https://docs.theforeman.org/nightly/Content_Management_Guide/index-katello.html

I like the workflow you mention, if an activation key isn't present, print out the sub-man command without the activation key and let the user run it, alternatively, it could run the command and let them input their user/pass directly?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it should print a command to run: the registration should either work (and be complete) or fail for a technical reason. It shouldn't start with a TODO. I'm also hesitant to make the script interactive.

Given bootstrap doesn't support it either, are we really dropping anything? It feels more like a possible future RFE.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the above command has feature-parity with bootstrap, everything else is RFE and/or "not the main workflow we support, you can script it yourself"


register_katello_host | bash
else
Expand All @@ -159,3 +219,5 @@ fi
<% else -%>
register_host | bash
<% end -%>

cleanup_and_exit
3 changes: 3 additions & 0 deletions config/initializers/uri_jail.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class URI::Generic::Jail < Safemode::Jail
allow :host, :path, :port, :query, :scheme
end
7 changes: 7 additions & 0 deletions test/unit/foreman/renderer/scope/macros/base_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,13 @@ class BaseMacrosTest < ActiveSupport::TestCase
end
end

test 'URI::Generic jail test' do
allowed = [:host, :path, :port, :query, :scheme]
allowed.each do |m|
assert URI::HTTP::Jail.allowed?(m), "Method #{m} is not available in URI::HTTP::Jail while should be allowed."
end
end

context 'subnet helpers' do
setup do
host = FactoryBot.build(:host)
Expand Down