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 #35627 - Use proxy registration_url from Proxy in registration #10315

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

stejskalleos
Copy link
Contributor

@stejskalleos stejskalleos commented Oct 13, 2022

What are the changes introduced in this pull request?

  • Use registration_url from proxy's registration module while registering hosts

Considerations taken when implementing this change?

RFC: Host registration and Load balancers

What are the testing steps for this pull request?

  • Configure proxy & haproxy
  • Try to register hosts to the Foreman
  • Verify that register host is configured to the haproxy & can consume data

Related PRs

@theforeman-bot
Copy link

Issues: #35627

@stejskalleos stejskalleos changed the title Fixes #35627 - Use proxy template URL in registration Fixes #35627 - Use proxy registratio_url from Proxy in registration Oct 19, 2022
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.

I'm going to throw out a completely different solution here, which I already suggested way back in the beginning but what was not implemented. However, it's going to be cleaner I think.

The request coming from the Smart Proxy can be authenticated using client certificates. This is how a Smart Proxy identifies itself to Foreman.

To start, we have the request issues by the Smart Proxy. Luckily, that is already done here.

Then on the receiving end you have to authenticate. We have the SmartProxyAuth concern. I'm not sure if require_smart_proxy_or_login method is correct for this use case (was authentication required or not?), but if it it's then it's simple. After that you get the authenticated detected_proxy property, regardless of whatever URL was used.

@stejskalleos stejskalleos changed the title Fixes #35627 - Use proxy registratio_url from Proxy in registration Fixes #35627 - Use proxy registration_url from Proxy in registration Oct 20, 2022
@stejskalleos
Copy link
Contributor Author

The request coming from the Smart Proxy can be authenticated using client certificates. This is how a Smart Proxy identifies itself to Foreman.

Sorry I don't follow much, can you please explain how is this related to the load balancing and registration and why to do it? Do I understand correctly that it's an another level of security for registration?

@ekohl
Copy link
Member

ekohl commented Oct 20, 2022

A Smart Proxy has SSL client certificates and can use those to authenticate. On the certificate is the name of the Smart Proxy (stored in the commonName). What happens is:

  • Apache validates the client certificate (authentication)
  • Apache passes that information on to Foreman
  • Foreman can use the authentication data to authorize. Most controllers in Foreman ignore the data, but where it does need to the SmartProxyAuth concern is used.

Now you have an authenticated way to determine which Smart Proxy was used, regardless of which URL was passed in.

One example is the config reports endpoint. It first adds a filter that checks, additionally that it has the right features enabled:
https://github.com/theforeman/foreman/blob/debea6c156b9508188e03cabc63093bc19f62956/app/controllers/api/v2/config_reports_controller.rb#L11

Then in the actual method it can use the detected proxy:

https://github.com/theforeman/foreman/blob/debea6c156b9508188e03cabc63093bc19f62956/app/controllers/api/v2/config_reports_controller.rb#L39-L47

Note it's still optional there since a regular user can also log in and use the API endpoint as well.

So it does add an additional layer of security, but it also makes it very reliable in detecting which Smart Proxy since it no longer relies on the passed in URL which could go out of sync. For example, when a user changes the registration URL on a Smart Proxy but forgets to refresh the features. Then your detection would fail, but the certificate authentication still works.

@stejskalleos
Copy link
Contributor Author

@ekohl I added SmartProxyAuth for smart proxy search, however I wasn't able to make the SSL authentication work on my setup, this condition is always false, even when I send the certs, no idea why.

But with mocked condition for the certificate registration works and I've been able to register and subscribe hosts without any problems.

@ekohl
Copy link
Member

ekohl commented Oct 25, 2022

however I wasn't able to make the SSL authentication work on my setup, this condition is always false, even when I send the certs, no idea why.

I guess it depends on your setup. If you use Apache as a reverse proxy in front of it, you do need to set variables to pass it on. These are the settings that are used to determine the environment variables:
https://github.com/theforeman/foreman/blob/b2d06d0ddbf01923d527feef16c4aef97aada881/app/services/foreman/client_certificate.rb#L49-L59
Our installer sets them here:
https://github.com/theforeman/puppet-foreman/blob/master/files/settings-reverse-proxy-headers.yaml

If you use Puma directly, the values may be different. Be sure to pass verify_mode and set it to peer. I think that the raw certificate is passed raw as the env variable puma.peercert. At least if I look at https://github.com/puma/puma/blob/03ed6c8e78b9ba0c76055af2ddb0bdd532807d6c/lib/puma/request.rb#L58-L60 & https://github.com/puma/puma/blob/03ed6c8e78b9ba0c76055af2ddb0bdd532807d6c/lib/puma/const.rb#L194

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.

Code wise 👍 though I'd probably update the commit message to reflect the current change. Describe that it now uses client certificates to identify a Smart Proxy. I'll also admit that I don't have a setup to verify this right now.

Any Katello developer who could verify this?

@@ -35,14 +36,19 @@ def context_urls

def smart_proxy
@smart_proxy ||= begin
proxy = params[:url] ? SmartProxy.unscoped.find_by(url: params[:url]) : SmartProxy.pulp_primary
proxy = params[:url] ? find_smart_proxy : SmartProxy.pulp_primary
Copy link
Member

Choose a reason for hiding this comment

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

Reviewers note: I was a bit tempted to suggest always authenticating the Smart Proxy and seeing if it can, but that can raise warnings. params[:url] should be set if a Smart Proxy sends a request so that's good enough for now. In the future we may want to revisit this.

@stejskalleos
Copy link
Contributor Author

@jeremylenz Looking for Katello dev to do the final ACK, could you add this to your review board?

Copy link
Member

@ehelms ehelms left a comment

Choose a reason for hiding this comment

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

@parthaa or @jeremylenz y'all OK with this?

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

Perhaps I'm missing something, but trying to register a host through a smart proxy via the usual method (just entering a smart proxy in the host registration form) is failing:

21:19:01 rails.1   | 2022-11-03T21:19:01 [I|app|a812306b] Started GET "/register?activation_keys=Default+Library+Key&force=true&location_id=2&operatingsystem_id=3&organization_id=1&setup_remote_execution_pull=true&update_packages=false&url=https%3A%2F%2Fcentos8-proxy-devel.cannolo.example.com%3A9090" for 192.168.122.243 at 2022-11-03 21:19:01 +0000
21:19:01 rails.1   | 2022-11-03T21:19:01 [I|app|a812306b] Processing by Api::V2::RegistrationController#global as HTML
21:19:01 rails.1   | 2022-11-03T21:19:01 [I|app|a812306b]   Parameters: {"activation_keys"=>"Default Library Key", "force"=>"true", "location_id"=>"2", "operatingsystem_id"=>"3", "organization_id"=>"1", "setup_remote_execution_pull"=>"true", "update_packages"=>"false", "url"=>"https://centos8-proxy-devel.cannolo.example.com:9090", "registration"=>{}}
21:19:01 rails.1   | 2022-11-03T21:19:01 [I|app|a812306b] Authorized user admin(Admin User)
21:19:01 rails.1   | 2022-11-03T21:19:01 [D|app|a812306b] Post-login processing for admin
21:19:01 rails.1   | 2022-11-03T21:19:01 [D|app|a812306b] Unpermitted parameters: :operatingsystem_id, :update_packages, :url, :registration
21:19:01 rails.1   | 2022-11-03T21:19:01 [W|app|a812306b] No SSL cert with CN supplied - request from 192.168.122.243
21:19:01 rails.1   | 2022-11-03T21:19:01 [W|app|a812306b] Action failed
21:19:01 rails.1   | 2022-11-03T21:19:01 [I|app|a812306b] Backtrace for 'Action failed' error (Foreman::Exception): ERF42-9394 [Foreman::Exception]: Smart proxy content source not found!
21:19:01 rails.1   |  a812306b | /home/vagrant/katello/app/controllers/katello/concerns/api/v2/registration_controller_extensions.rb:41:in `smart_proxy'

My Foreman and smart proxy have both been updated with the related code.

I'm not totally understanding how the smart proxy URL is automatically decided by auth_smart_proxy. I'm seeing the No SSL cert with CN supplied - request from 192.168.122.243 error... is more setup necessary?

Without this PR, I can register though a set smart proxy just fine.

I'm testing with a Katello development environment and one of the proxy-devel vagrant boxes.

@ekohl
Copy link
Member

ekohl commented Nov 3, 2022

Looking at puppet-katello_devel I think some settings may be missing (which means the Smart Proxy auth never worked in the devel setup?) Can you check if https://github.com/theforeman/puppet-foreman/blob/master/files/settings-reverse-proxy-headers.yaml is present in your settings.yaml?

@ianballou
Copy link
Member

Those settings are indeed missing from my development environment. I'll add them in and see what changes.

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

I didn't test with haproxy, but after adding the correct Foreman settings this worked just fine for me. I tested registering through a smart proxy and changing the content source a couple times.

If you think an actual test with haproxy is needed, I'd appreciate some steps for configuring it how a user actually would.

Anyway, from my standpoint, I'm cool with merging it. We'll just need to update the katello_devel puppet module.

@ekohl
Copy link
Member

ekohl commented Nov 4, 2022

I don't think it's needed to test it out with HAProxy now. Please merge it. And we do indeed also need to update the katello_devel module. Thanks for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants