try to allow certificate-chains

[sircubbi]

Currently the verification of certificates uses only the first pem in
the ca-file and the first pem in the certificate-file. This breaks if an
intermediate certificate is needed.

A simple workaround is to put the full chain into the ca-file and give
the ca-file instead of the X509-structure to the VerifyCertificate()
methode. There we can just do the usual business but add the full
ca-file again to OpenSSLs SSL_CTX_load_verify_locations().

While this seems a little bit hackish it should at least allow the
proper verification of a certificate chain without introducing any
security implications for setups with just a single root-ca.
The only downside currently: while the CLI "pki verify" will correctly
check the supplied parameters, it still only shows the topmost
certificate from the ca-file (which I guess is fine for the moment).

see also issue #7719. With this patch you can put the full chain into the local ca.crt-file (order does not matter). This would fix setups where the puppet PKI is used with a recent puppetmaster (PE2019.x/Puppet 6 which uses an intermediate CA).

[lazyfrosch]

I've added a bit of explanation here: #7719 (comment)

[cla-bot]

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @sircubbi

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

• If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

[sircubbi]

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @sircubbi

* If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please [contact us](https://icinga.com/company/contact/) if you think this is the case.

* If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

Done

[bobapple]

@cla-bot check

[SimonHoenscheid]

I stumbled over this issue trying to set up a setup using the puppet CA. I see a function from the puppet module, which is currently not usable with an up to date puppet setup and this feature would solve it. Is there a reason to not accept the patch?

[sircubbi]

I stumbled over this issue trying to set up a setup using the puppet CA. I see a function from the puppet module, which is currently not usable with an up to date puppet setup and this feature would solve it. Is there a reason to not accept the patch?

I guess the developers are concentrating on the build-in CA of Icinga. But yes, that doesn't work with the way Puppet setups Icinga. My patch isn't really invasive I think, so unfortunately I don't know what the holdback is.

As long as you are using an RHEL-based system you can grab patched packages from our Copr-repository at https://copr.fedorainfracloud.org/coprs/relaix/utils/

[julianbrost]

I had a quick look at the code. Do I understand correctly that this basically pushes some intermediate certificates into the trust store so that they are known when a leaf certificate is sent without a complete chain (similar to how browsers cache intermediate certificates and then accept certificates even if the chain is incomplete)?

By the way, have you tried if #9026 solves your issue? I didn't have time to properly test either PR so far unfortunately.

[sircubbi]

I had a quick look at the code. Do I understand correctly that this basically pushes some intermediate certificates into the trust store so that they are known when a leaf certificate is sent without a complete chain (similar to how browsers cache intermediate certificates and then accept certificates even if the chain is incomplete)?

Kind of. The problem with the current icinga-implementation is, that the CA as well as the certificate sent by the client is always only a single PEM. That doesn't work if an intermediate is used, since the server will only read the topmost certificate from its ca-file (so it doesn't matter if you put the root or the intermediate there) and the client will also only send its topmost certificate from its cert-file. OpenSSL needs to verify a whole chain from top to bottom, so with an intermediate you have at least three different certs to check, and the current implementation has no way to supply more than two certs.

What my patch does is not reading the top cert from the ca-file, but all certs that you put in there, and therefore supplying the OpenSSL-verify-function with the whole chain.

By the way, have you tried if #9026 solves your issue? I didn't have time to properly test either PR so far unfortunately.

And no, #9026 does not solve the issue, since it only controls if the icinga-master will try to reissue a certificate with its own CA. The purpose if this PR is explicitly to allow the usage of an external CA with intermediates (so either the Puppet CA or actually any other reallife CA you will encounter nowadays, eg. LetsEncrypt).

[SimonHoenscheid]

@bobapple can you support here and try to figure out if this PR will be merged or closed?

[sircubbi]

Is there any reason why this PR was removed from the next milestone? What can I do to get it picked up faster? Essentially there is currently no proper way to setup icinga2 with an existing puppet-ca, although the solution is kind of simple and also doesn't break anything or introduce any security risks

[julianbrost]

Is there any reason why this PR was removed from the next milestone?

Not much changed for this PR, it's still "if it's ready before the next release, it can be in the next release". I'm just trying to get the milestone into a state were it contains things we definitely want in a release. More can still be added.

What can I do to get it picked up faster?

Preferably, the certificate code should move towards how everything else handles certificates. So for intermediate certificates, this would mean that the trust store should only contain the root certificate and the intermediate certificates are sent with the leaf certificates. I'd prefer a PR that makes things work if the intermediate is put there.

[sircubbi]

What can I do to get it picked up faster?

Preferably, the certificate code should move towards how everything else handles certificates. So for intermediate certificates, this would mean that the trust store should only contain the root certificate and the intermediate certificates are sent with the leaf certificates. I'd prefer a PR that makes things work if the intermediate is put there.

I can check how much efford it would be for the agents to sent a whole chain instead of the leaf-cert only.

However, I kind of disagree that the master should only have (one) root certificate in its store. I still think the master should always have a certificate bundle, to allow for different root-certificates. Consider a split setup, where some of the agents would use the Puppet CA and some of the agents use the icinga-builtin CA. You could even extend that to some agents using Lets-Encrypt for example. I see no reason why it should be restricted to only one CA, especially since all of the bundle-handling is already fully implemented within OpenSSL.

[Al2Klimov]

@julianbrost already said everything pretty clearly.

Apropos, I'm wondering whether you could "just" put the intermediates in the same file as the leaf cert. Not Icinga. You. Or your automation tool. After all the CA, and the leaves of course, are external.

Also, multiple roots or not, if you can easily separate that construction area from this one, please do that.

[sircubbi]

@julianbrost already said everything pretty clearly.

Hmmm, but as said, that would fix only part of the issue. Why not doing the proper thing in the first place?

Apropos, I'm wondering whether you could "just" put the intermediates in the same file as the leaf cert. Not Icinga. You. Or your automation tool. After all the CA, and the leaves of course, are external.

No, you cannot do that, as the current Icinga-code just throws away everything from the store and only extracts one single X509-cert.

And btw: the whole mess results from the official way provided by Icinga on how to roll everything out with Puppet. Using the puppet-ca was officially implemented in the puppet-modules provided by the Icinga-devs themselves. So I really think it is best to fix SSL to work like in any other software, which would mean to properly accept multiple Roots and Intermediates. It is just not common today anymore to use on single root, even if that is the way the internal Icinga-CA works at the moment.

[Al2Klimov]

Apropos, I'm wondering whether you could "just" put the intermediates in the same file as the leaf cert. Not Icinga. You. Or your automation tool. After all the CA, and the leaves of course, are external.

No, you cannot do that, as the current Icinga-code just throws away everything from the store and only extracts one single X509-cert.

Indeed.

Notes for the future myself

openssl req -x509 -newkey rsa:4096 -subj '/CN=Ext. Root CA' -md5 -keyout root.key -out root.crt -nodes


openssl req -newkey rsa:4096 -subj '/CN=Ext. Intm. CA' -keyout intm.key -out intm.csr -nodes

openssl x509 -req -in intm.csr -sha512 -out intm.crt -CA root.crt -CAkey root.key -CAcreateserial -extensions ext -extfile <(printf '[ext]\nbasicConstraints=critical,CA:TRUE,pathlen:0')


openssl req -newkey rsa:4096 -subj '/CN=master1' -keyout master1.key -out master1.csr -nodes

openssl x509 -req -in master1.csr -sha512 -out master1.crt -CA intm.crt -CAkey intm.key -CAcreateserial -extensions SAN -extfile <(printf '[SAN]\nsubjectAltName=DNS:master1')


grep Node /etc/icinga2/constants.conf
const NodeName = "master1"
const ZoneName = NodeName

mkdir /var/lib/icinga2/certs

cat master1.crt intm.crt >/var/lib/icinga2/certs/master1.crt

cp master1.key /var/lib/icinga2/certs

cp root.crt /var/lib/icinga2/certs/ca.crt

chown -R nagios: /var/lib/icinga2/certs


icinga2 feature enable api

icinga2 daemon -C

[Al2Klimov]

But if it doesn’t work right now, we should enable that in the future.

[sircubbi]

OK, so I kind of reworked the whole thing now.

I made a separate PR (#9795) which uses the full chain that will be presented by the client/icinga-agent. In that PR the icinga-master only holds to topmost root-cert of the ca, while the icinga-agent has the intermediate-certs (either in the local ca.crt, or added to the clients-certfile).

I then combined the above changes into this PR, since I still believe that is useful to allow mixed setups of different certificate-origins and therefore have a cabundle-file on the icinga-master. (Just in case the original changes previously in this PR are preserved at https://github.com/sircubbi/icinga2/tree/certchain_server).

[julianbrost]

I'm not sure what exactly the role of this PR is given that you just opened #9795. Looks like both contain some of the same changes, but it's not like one is based on top of the other (which means if both were to be merged, I think there would be conflicts). Is it that this PR includes #9795 but on top, it allows more than one trusted root certificate?

[Al2Klimov]

In this case please indicate this via separate commits. If the other PR is a true subset of this one commit and both share the same parent commit, separate commits should be easy via:

• git checkout BRANCH_WITH_BIG_COMMIT
• git reset --soft BRANCH_WITH_SMALL_COMMIT
• git diff --staged
• git commit

[sircubbi]

In this case please indicate this via separate commits. If the other PR is a true subset of this one commit and both share the same parent commit, separate commits should be easy via:

OK, sure. I will rework (#9795) first with the recommended parameter defaults, and then split this PR into two commits.

[sircubbi]

In this case please indicate this via separate commits. If the other PR is a true subset of this one commit and both share the same parent commit, separate commits should be easy via:

OK, sure. I will rework (#9795) first with the recommended parameter defaults, and then split this PR into two commits.

As requested this PR now consists of two commits. The first commit is just the change from (#9795), while the second commit also allows a whole ca-bundle on the master-side.