Refs #32383: Configurable client certificate authentication to Pulp #186

ehelms · 2021-04-27T23:41:59Z

…o Pulp as admin

Require a client certificate to be presented with a CN that matches
the supplied auth CN. If this is present, set the REMOTE_USER to
admin to pass along to Pulp. This changes from having to generate
aclient certificate with a valid user (e.g. admin) as the CN
to allowing to use a client certificate generated with a more standard
CN (e.g. FQDN) and act as the admin user.

Example usage:

class { 'pulpcore':
  api_client_auth_cn => $foreman_host
}

Questions:

This would allow us to drop the special purpose pulp_client certificates we generate today. Do we want to go this route?
Is this the best way implement how we enable this and configure it?
This forces all API paths to be behind authentication now. The /pulp/api/v3/status/ does not require authentication by Pulp. Do we want an exception for that route? If so, how do we idenitfy that path in the Apache config to skip the require?

ekohl · 2021-04-28T10:19:00Z

This would allow us to drop the special purpose pulp_client certificates we generate today. Do we want to go this route?

Yes please. I would love to get rid of those special purpose certificates.

This forces all API paths to be behind authentication now. The /pulp/api/v3/status/ does not require authentication by Pulp. Do we want an exception for that route? If so, how do we idenitfy that path in the Apache config to skip the require?

Some other pattern that I saw while reading some docs was this: https://httpd.apache.org/docs/trunk/expr.html#examples. While I haven't tried, it makes me think something like this may work.

Header unset REMOTE_USER
<If "%{SSL_CLIENT_S_DN_CN} == 'MY_CN'">
  Header set REMOTE_USER "admin"
</If>

ehelms · 2021-04-28T14:24:18Z

This would allow us to drop the special purpose pulp_client certificates we generate today. Do we want to go this route?

Yes please. I would love to get rid of those special purpose certificates.

This forces all API paths to be behind authentication now. The /pulp/api/v3/status/ does not require authentication by Pulp. Do we want an exception for that route? If so, how do we idenitfy that path in the Apache config to skip the require?

Some other pattern that I saw while reading some docs was this: https://httpd.apache.org/docs/trunk/expr.html#examples. While I haven't tried, it makes me think something like this may work.
Header unset REMOTE_USER
<If "%{SSL_CLIENT_S_DN_CN} == 'MY_CN'">
  Header set REMOTE_USER "admin"
</If>

Solid find, that did handle both situations.

ehelms · 2021-04-28T16:00:33Z

manifests/init.pp

@@ -200,6 +203,7 @@
  Integer[0] $api_service_worker_count = 1,
  Integer[0] $content_service_worker_timeout = 90,
  Integer[0] $api_service_worker_timeout = 90,
+  String $api_client_auth_cn = 'admin',


By setting this default to admin means that this change can be released without breaking consumers of this, primarily Katello which uses client certificates with the CN set to admin.

Should we use a stricter data type?

Suggested change

String $api_client_auth_cn = 'admin',

Stdlib::Fqdn $api_client_auth_cn = 'admin',

AFAIK, a CN field in a cert should always be a hostname. The FQDN type also allows unqualified names (such as admin).

ekohl · 2021-04-28T16:10:11Z

I like that it's now shaping up as an enhancement. The commit message should reflect that.

ehelms · 2021-04-28T16:11:30Z

I like that it's now shaping up as an enhancement. The commit message should reflect that.

How would you like me to reflect that wording wise?

ekohl · 2021-04-28T16:17:05Z

I'm thinking about:

Configurable Certificate CN matching + mapping

Prior to this, the Certificate CN was passed directly to Pulp. After this, Apache checks for a match and then sends the value admin.

Reading what I just wrote, it's not really backwards compatible for some use cases. Perhaps if the "destination" was also a parameter then it would really be a mapping. You can also think about Hash $api_cn_map = { 'admin' => 'admin' }.

ehelms · 2021-04-29T00:55:35Z

I am confused as to how the current test failure works:

  1) basic installation Curl command "https://centos7-64.example.com/pulp/api/v3/" body is expected to contain "artifacts_list"
     Failure/Error: its(:body) { is_expected.to contain('artifacts_list') }
       expected "openapi: 3.0.3\ninfo:\n  title: Pulp 3 API\n  version: v3\n  description: Fetch, Upload, Organize, a...n      type: apiKey\n      in: cookie\n      name: Session\nservers:\n- url: http://pulpcore-api/\n" to contain "artifacts_list"
       
     # ./spec/acceptance/basic_spec.rb:92:in `block (3 levels) in <top (required)>'

When I look in a Pulp 3 install on a Katello nightly:

[root@pipe-katello-server-nightly-centos8 bats]# curl https://`hostname`/pulp/api/v3/ | grep artifact
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 42473  100 42473    0     0   117k      0 --:--:-- --:--:-- --:--:--  117k
      description: Queues a task that creates a new Collection from an uploaded artifact.
  /pulp_ansible/galaxy/{path}/api/v3/artifacts/collections/:
      operationId: pulp_ansible_galaxy_api_v3_artifacts_collections_create
      description: Create an artifact and trigger an asynchronous task to create Collection
        artifact:
        artifact:
        artifact:

I don't see artifacts_list in there.

ekohl · 2021-04-29T08:22:52Z

puppet-pulpcore/spec/acceptance/basic_spec.rb

Lines 83 to 94 in 5462f34

    
           describe curl_command("https://#{host_inventory['fqdn']}/pulp/api/v3/", cacert: "#{certdir}/ca-cert.pem") do 
        
             its(:response_code) { is_expected.to eq(200) } 
        
             its(:body) { is_expected.not_to contain('artifacts_list') } 
        
             its(:exit_status) { is_expected.to eq 0 } 
        
           end 
        
           describe curl_command("https://#{host_inventory['fqdn']}/pulp/api/v3/", 
        
                                 cacert: "#{certdir}/ca-cert.pem", key: "#{certdir}/client-key.pem", cert: "#{certdir}/client-cert.pem") do 
        
             its(:response_code) { is_expected.to eq(200) } 
        
             its(:body) { is_expected.to contain('artifacts_list') } 
        
             its(:exit_status) { is_expected.to eq 0 } 
        
           end

Note how first it's unauthenticated and expects it not to be there. Then it is authenticated and it's expected to be there. The status endpoint is very different for anonymous and (admin) users.

The client cert is generated here:

puppet-pulpcore/spec/setup_acceptance_node.pp

Line 61 in 5462f34

    
           command   => "openssl req -nodes -new -newkey rsa:2048 -subj '/CN=admin' -out '${client_csr}' -keyout '${client_key}'",

ehelms · 2021-04-29T11:51:52Z

Ahh, I see now. It is not the status API but the root API which returns the OpenAPI spec that differs based on authenticated vs. unauthenticated. That feels a bit odd to get back different API specs based on who you are but that's not what this PR is about -- thanks.

ekohl · 2021-04-29T11:58:04Z

It makes sense that the OpenAPI spec describes what you can actually do vs what you could do if you had permissions. It's also what I used to determine if authentication actually works.

manifests/apache.pp

ehelms · 2021-04-29T20:37:12Z

On a Katello nightly this comes out as:

  <Location "/pulp/api/v3">
    RequestHeader unset REMOTE_USER
    ProxyPass unix:///run/pulpcore-api.sock|http://pulpcore-api/pulp/api/v3 timeout=600
    ProxyPassReverse unix:///run/pulpcore-api.sock|http://pulpcore-api/pulp/api/v3
    <If "%{SSL_CLIENT_S_DN_CN} == 'admin'">
      RequestHeader set REMOTE_USER "admin"
    </If>
  </Location>

On a proxy:

  <Location "/pulp/api/v3">
    Require all granted
      ## Request Header rules
            RequestHeader unset REMOTE_USER
    ProxyPass unix:///run/pulpcore-api.sock|http://pulpcore-api/pulp/api/v3 timeout=600
    ProxyPassReverse unix:///run/pulpcore-api.sock|http://pulpcore-api/pulp/api/v3
              <If "%{SSL_CLIENT_S_DN_CN} == 'admin'">
      RequestHeader set REMOTE_USER "admin"
    </If>

  </Location>

ekohl · 2021-04-30T10:23:48Z

Another thought I had (sorry to keep directing you all over the place) is using the conditional part of mod_headers:

RequestHeader unset REMOTE_USER
RequestHeader set REMOTE_USER "%{SSL_CLIENT_S_DN_CN}s" env=SSL_CLIENT_S_DN_CN
RequestHeader set REMOTE_USER "admin" "expr=%{SSL_CLIENT_S_DN_CN} == 'some-cn.example.com'"

It keeps the option open to use other identities but allows us to remap some. What do you think about this?

ehelms · 2021-04-30T12:29:19Z

RequestHeader set REMOTE_USER "admin" "expr=%{SSL_CLIENT_S_DN_CN} == 'some-cn.example.com'"

I went looking for something like this, glad you found it as that makes the code much simpler. Now I find myself asking:

Do we allow only a single mapping? Or support an array of mappings?
Do we always include "set REMOTE_USER "%{SSL_CLIENT_S_DN_CN}s" env=SSL_CLIENT_S_DN_CN"

ekohl · 2021-04-30T12:40:24Z

Do we allow only a single mapping? Or support an array of mappings?

I was thinking about a simple hash where the key is the source (it matches SSL_CLIENT_S_DN_CN) and the value is the destination (admin)). I don't know if we may end up wanting to support more than a single (admin) user but it would prepare us for that.

Do we always include "set REMOTE_USER "%{SSL_CLIENT_S_DN_CN}s" env=SSL_CLIENT_S_DN_CN"

I think it would make sense. It keeps it backwards compatible and allows using multiple users in the future.

ehelms · 2021-04-30T12:50:05Z

Do we always include "set REMOTE_USER "%{SSL_CLIENT_S_DN_CN}s" env=SSL_CLIENT_S_DN_CN"

I think it would make sense. It keeps it backwards compatible and allows using multiple users in the future.

The last thought I had with this was if we wanted, going forward, to support passing any CN presented to Pulp and letting Pulp handle things. In the pure mapping case we are providing a layer in Apache that both restricts the set of possible users and enforces an explicit declaration of who can get to that endpoint. But perhaps that is over reaching and we should just trust Pulp to handle whatever user is thrown at it.

ekohl · 2021-04-30T12:57:26Z

But perhaps that is over reaching and we should just trust Pulp to handle whatever user is thrown at it.

I'm leaning to this, but I had the same considerations.

ehelms · 2021-04-30T14:56:04Z

The code is much simpler now which I like. This keeps the previous default and adds any specified mappings iteratively.

manifests/apache.pp

manifests/init.pp

Allows a user supplied mapping of certificate CN to Pulp user name. If this is present, set the REMOTE_USER to a Pulp user defined in the parameter to pass along to Pulp. This changes from having to generate a client certificate with a valid user (e.g. admin) as the CN to allowing to use a client certificate generated with a more standard CN (e.g. FQDN) and act as a user in Pulp suppplied to the parameter.

ekohl

I think this is now a pure enhancement rather than an actual change. Thanks!

theforeman-bot added Needs testing Not yet reviewed labels Apr 27, 2021

ekohl added the Backwards incompatible label Apr 28, 2021

ehelms force-pushed the refs-32383 branch 3 times, most recently from f04c112 to ecda8cd Compare April 28, 2021 16:00

ehelms commented Apr 28, 2021

View reviewed changes

ehelms force-pushed the refs-32383 branch from ecda8cd to 2797e4a Compare April 28, 2021 16:01

ehelms mentioned this pull request Apr 28, 2021

Fixes #32383: Set Pulp to expect Foreman host as the client authentic… theforeman/puppet-foreman_proxy_content#350

Merged

ehelms force-pushed the refs-32383 branch from 2797e4a to 2bcccb2 Compare April 28, 2021 16:32

ehelms force-pushed the refs-32383 branch from 2bcccb2 to 2182504 Compare April 29, 2021 19:47

ehelms commented Apr 29, 2021

View reviewed changes

manifests/apache.pp Outdated Show resolved Hide resolved

ehelms force-pushed the refs-32383 branch 2 times, most recently from 12a4a0b to e9be59e Compare April 30, 2021 00:39

ehelms added Enhancement New feature or request and removed Backwards incompatible Not yet reviewed labels Apr 30, 2021

ehelms force-pushed the refs-32383 branch from e9be59e to 377f2e0 Compare April 30, 2021 14:52

ekohl reviewed Apr 30, 2021

View reviewed changes

manifests/apache.pp Outdated Show resolved Hide resolved

manifests/init.pp Outdated Show resolved Hide resolved

ehelms force-pushed the refs-32383 branch from 377f2e0 to 3fc9947 Compare April 30, 2021 16:15

ehelms changed the title ~~Refs #32383: Rely on client certificate CN matching to authenticate t…~~ Refs #32383: Configurable client certificate authentication to Pulp Apr 30, 2021

ehelms mentioned this pull request Apr 30, 2021

Fixes #32487: Use Foreman client certificates to communicate with Pulp Katello/katello#9330

Merged

ekohl approved these changes May 3, 2021

View reviewed changes

ehelms merged commit 1101ab3 into theforeman:master May 3, 2021

evgeni mentioned this pull request Sep 3, 2024

Correct container plugin apache header rewrite #356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #32383: Configurable client certificate authentication to Pulp #186

Refs #32383: Configurable client certificate authentication to Pulp #186

ehelms commented Apr 27, 2021

ekohl commented Apr 28, 2021

ehelms commented Apr 28, 2021

ehelms Apr 28, 2021

ekohl Apr 28, 2021

ekohl commented Apr 28, 2021

ehelms commented Apr 28, 2021

ekohl commented Apr 28, 2021

ehelms commented Apr 29, 2021

ekohl commented Apr 29, 2021

ehelms commented Apr 29, 2021

ekohl commented Apr 29, 2021

ehelms commented Apr 29, 2021

ekohl commented Apr 30, 2021

ehelms commented Apr 30, 2021

ekohl commented Apr 30, 2021

ehelms commented Apr 30, 2021

ekohl commented Apr 30, 2021

ehelms commented Apr 30, 2021

ekohl left a comment

	String $api_client_auth_cn = 'admin',
	Stdlib::Fqdn $api_client_auth_cn = 'admin',

Refs #32383: Configurable client certificate authentication to Pulp #186

Refs #32383: Configurable client certificate authentication to Pulp #186

Conversation

ehelms commented Apr 27, 2021

ekohl commented Apr 28, 2021

ehelms commented Apr 28, 2021

ehelms Apr 28, 2021

Choose a reason for hiding this comment

ekohl Apr 28, 2021

Choose a reason for hiding this comment

ekohl commented Apr 28, 2021

ehelms commented Apr 28, 2021

ekohl commented Apr 28, 2021

ehelms commented Apr 29, 2021

ekohl commented Apr 29, 2021

ehelms commented Apr 29, 2021

ekohl commented Apr 29, 2021

ehelms commented Apr 29, 2021

ekohl commented Apr 30, 2021

ehelms commented Apr 30, 2021

ekohl commented Apr 30, 2021

ehelms commented Apr 30, 2021

ekohl commented Apr 30, 2021

ehelms commented Apr 30, 2021

ekohl left a comment

Choose a reason for hiding this comment