Custom CA overwritten when certs::server_ca_cert is undefined

[selfsealingstembolts]

When upgrading a smart proxy with a custom certificate bundle from Foreman 3.10, which uses puppet-certs v17.1.1, to Foreman 3.11, which uses puppet-certs v18.0.0, foreman-installer will fail with TLS verification errors.

The root cause is this module overwriting the custom CA in /root/ssl-build/katello-server-ca.crt with the contents of /root/ssl-build/katello-default-ca.crt when certs::server_ca_cert is undefined in the foreman-installer scenario answers file.

This behavior can be seen here. The commit introducing this behavior is 433dadc.

A workaround is to set the following values in /etc/foreman-installer/scenarios.d/foreman-proxy-content-answers.yaml

certs:
  server_cert: "/root/ssl-build/${smart-proxy.example.com}/${smart-proxy.example.com}-foreman-proxy.crt"
  server_key: "/root/ssl-build/${smart-proxy.example.com}/${smart-proxy.example.com}-foreman-proxy.key"
  server_ca_cert: "/root/ssl-build/katello-server-ca.crt"

The values of certs::server_cert and certs:server_key are required along with certs:server_ca_cert otherwise the installer will fail on the katello-certs-check step.

I am not sure what an appropriate fix would look like. It does seem desirable to use the default CA if it is not explicitly defined. The obvious candidate is testing if /root/ssl-build/katello-server-ca.crt already exists before overwriting it with the default CA. Alternatively, applying a migration to explicitly define the values mentioned above. Migrations appear to be outside the scope of this repository however.

[knoppi]

An obvious solution would be to add replace => false to the resource File[$server_ca_path] in manifests/ca.pp. Then it would not overwrite the existing file in /root/ssl_build. This works as I just tested.
And I could probably still enforce it by adding --certs-update-server-ca to foreman installer.

[ekohl]

It's not really clear what the problem is. To me this reads as a user error. I don't see the use case where you want custom certificates but not define a server CA. Perhaps we don't enforce it today, but I think it was always assumed you pass in all 3 values.

I can imagine that we start to enforce that you pass in all 3 values (key, cert, ca) or fail. We already have some code in https://github.com/theforeman/foreman-installer/blob/develop/hooks/pre_commit/20-certs_update.rb that is related to this.

A workaround is to set the following values in /etc/foreman-installer/scenarios.d/foreman-proxy-content-answers.yaml

You can just pass --certs-server-cert, --certs-server-key and --certs-server-ca-cert to achieve the same, right?

An obvious solution would be to add replace => false to the resource File[$server_ca_path] in manifests/ca.pp. Then it would not overwrite the existing file in /root/ssl_build. This works as I just tested.

This is not what we want and a PR will be rejected.

[knoppi]

I ran into this issue when installing a new smart proxy.

[root@test-deploy-jan ~]# /usr/sbin/foreman-proxy-certs-generate \
>   --foreman-proxy-fqdn "test-proxy-jan.example.com" \
>   --server-cert "/root/test-proxy-jan.example.com.cert" \
>   --server-key "/root/test-proxy-jan.example.com.key" \
>   --server-ca-cert "/root/test-proxy-jan.example.com.ca.cert" \
>   --certs-tar "/root/test-proxy-jan.example.com-certs.tar"

Preparing installation Done                                              
  Success!

  To finish the installation, follow these steps:

  If you do not have the Smart Proxy registered to the Katello instance, then please do the following:

  1. yum -y localinstall http://test-deploy-jan.example.com/pub/katello-ca-consumer-latest.noarch.rpm
  2. subscription-manager register --org "My_ORG"

  Once this is completed run the steps below to start the Smart Proxy installation:

  1. Ensure that the foreman-installer-katello package is installed on the system.
  2. Copy the following file /root/test-proxy-jan.example.com-certs.tar to the system test-proxy-jan.example.com at the following location /root/test-proxy-jan.example.com-certs.tar
  scp /root/test-proxy-jan.example.com-certs.tar [email protected]:/root/test-proxy-jan.example.com-certs.tar
  3. Run the following commands on the Smart Proxy (possibly with the customized
     parameters, see foreman-installer --scenario foreman-proxy-content --help and
     documentation for more info on setting up additional services):

  foreman-installer\
                    --scenario foreman-proxy-content\
                    --certs-tar-file                              "/root/test-proxy-jan.example.com-certs.tar"\
                    --foreman-proxy-register-in-foreman           "true"\
                    --foreman-proxy-foreman-base-url              "https://test-deploy-jan.example.com"\
                    --foreman-proxy-trusted-hosts                 "test-deploy-jan.example.com"\
                    --foreman-proxy-trusted-hosts                 "test-proxy-jan.example.com"\
                    --foreman-proxy-oauth-consumer-key            "w8rPStxvMLx4f7xaE4jpJQ8JdCMM3T2i"\
                    --foreman-proxy-oauth-consumer-secret         "b4BZ2pffhEExfySZSWnpXkmbwvCVSRNe"

When running the installer command, I obviously don't set --certs-server-ca-cert. The tar file gets extracted but I cannot prevent the server ca cert from being overwritten with the default ca certificate later (still during installation and, hence, installer will fail). I could manually extract the tar file and pass all files explicitely as parameters but that renders the --certs-tar-file rather useless. BTW: That is a valid workaround for the moment.

So, for me smart proxy installation with custom certificates is currently broken and if I installed a smart proxy prior to this change it cannot be upgraded anymore without adding coordinates of cert, key and ca cert pointing to their destinations.

If I were to replace the CA I would always do so calling it with --certs-update-server-ca and since this option exists, I'd expect the installer not to modify the server ca without explicitely stating it. I know this is foreman-installer and only indirectly related to here. But this is also the reason why I still consider replace => false a valid option.

[knoppi]

I think there are two problems here: one for fresh installations of smart proxies, the other one for upgrading existing proxies.

A) Installing new proxies: I would simply pass the --server-certs-tar. This does not work anymore with an external server ca.

B) Upgrading proxies: Since I called the installation without the --certs-server- trinity the corresponding values in the answers.yaml are empty. The next call of the foreman installer which might be completely unrelated to certificates will overwrite the server ca with the default ca.

This is not what we want and a PR will be rejected.

If you elaborate on what you want, I can come up with a more suitable PR.

[ekohl]

If you elaborate on what you want, I can come up with a more suitable PR.

We must manage the file. It sounds like the problem is that we can't properly come up with the value what it should be.

I'll need to do some further digging to see what's going on.

[knoppi]

Is it safe to assume we don't want to have the server ca overwritten with default ca if tar_file is not an empty string and tar_file points to a valid file?

The tar file is probably the common denominator of the problems in this issue and the threads in the Foreman community board. It is also the marker that foreman-installer has ever been called with this parameter. certs::server_ca_cert should still be higher in hierarchy than the tar file. For a fall-back to default ca you would have to empty the value in your answers file or by passing --reset-certs-tar-file.

[ekohl]

Is it safe to assume we don't want to have the server ca overwritten with default ca if tar_file is not an empty string and tar_file points to a valid file?

The file is always managed. That is the whole model of Puppet: there's a complete definition of the entire state and it will enforce that state. The problem is that somehow the definition of the state is incorrect.

[knoppi]

Yes, I understand your point, you always want to have correctly defined file resource for $server_ca_path. I haven't suggested anything else, have I?
I'm just trying to get some more context on decision points because otherwise

This is not what we want and a PR will be rejected.

[ekohl]

I'm just trying to get some more context on decision points because otherwise

This is not what we want and a PR will be rejected.

To be clear: replace => false is intended for files that get their initial content and are then supposed to be hand managed. The CA files do not fall in that category so it's the wrong solution.

Looking further at the problem, I suspect 433dadc can be a regression. You can see it changes if $server_cert into if $certs::server_ca_cert and looks related.

[ekohl]

I first thought perhaps the tarball itself has the wrong content, but #461 adds a test that tells me otherwise. We don't have any coverage for the content proxy case yet, but such a regression test would be an excellent next step.

[ekohl]

#461 now has a reproducer test.

[knoppi]

To be clear: replace => false is intended for files that get their initial content and are then supposed to be hand managed. The CA files do not fall in that category so it's the wrong solution.

Sorry, I was probably not clear about that and sou you put my comments into a context that does not exist. My intention was not to keep annoying anyone with my initial approach.

I only asked for more input to come up with a more sophisticated solution. But because "what you want" is still quite a blackbox, how you prioritize parameters, what workflows you have in mind, I went on with my questions.

From your posts today I understand you don't want an external contribution to this problem but prefer working on it yourself. Is that correct?

[ekohl]

From your posts today I understand you don't want an external contribution to this problem but prefer working on it yourself. Is that correct?

I wouldn't mind external contributions, but that particular implementation is not the right solution. Now I'm trying to understand what's going on and writing tests is my way of doing so. I think I'm close to a solution, but still experimenting.