process certificate chains presented by the client #9795
Conversation
There was a problem hiding this comment.
or the intermediate in the cert.pem
So with this PR -in contrast to master- #8859 (comment) (haven’t tested yet) should just work?
|
At first glance, this looks more like the change I was expecting for certificate changes. 👍 |
That is correct. The intermediate can either go into the cert.pem or into the ca.pem (on the agent-side) as well. OpenSSL internally picks up all the chain-elements and send them over, so no change is actually needed on the agent-side. Only the master needs to pickup the (untrusted) chain from the wire and tries to validate the whole set against its local root-ca-cert. |
sircubbi
force-pushed
the
certchain_client
branch
from
eb068c5
to
f06672c
Compare
| @@ -69,7 +69,7 @@ String SHA256(const String& s); | |||
| String RandomString(int length); | |||
| String BinaryToHex(const unsigned char* data, size_t length); | |||
|
|
|||
| bool VerifyCertificate(const std::shared_ptr<X509>& caCertificate, const std::shared_ptr<X509>& certificate, const String& crlFile); | |||
| bool VerifyCertificate(const std::shared_ptr<X509>& caCertificate, const std::shared_ptr<X509>& certificate, const String& crlFile, STACK_OF(X509) *chain = nullptr); | |||
There was a problem hiding this comment.
(Maybe I've asked too early for a default value here.) What are the now implicit-nullptr usages and (why) should they (not) also get some chains?
There was a problem hiding this comment.
But, to be honest, one strange error message less is already an improvement. (But not for 2.14.)
@julianbrost Do you agree?
|
Hold on a second, do I understand this correctly: You're saying (also taking information from #9798 into account here) that node certificates signed by an intermediate CA work perfectly fine apart from a log message that doesn't limit the functionality ("Received certificate request for CN ... not signed by our CA:")? So the claim made by #9026 (put that renewal stuff in an if and things work) was indeed correct? And in contrast, this PR would fix it by making the renewal logic aware of the chain which contains the intermediates required for verification? |
|
Yes. IMAO we don’t even need #9026 on every node, instead we could check the root CA name in code or so. But I prefer PRs like this one. |
|
I didn't want to revive #9026 but my intuition about that PR was "how could this possibly make intermediate certificates work", but if it wasn't even that broken, it makes a bit more sense now. |
|
Just to give an update: I am still in the process of reading through your comments (also in #9798). However because of vacation in the next week it will be a little bit delayed, hope that is ok. |
Al2Klimov
added
the
area/distributed
Al2Klimov
force-pushed
the
certchain_client
branch
from
f06672c
to
b1687b3
Compare
Similar to Icinga#8859 this patch works around Icinga#7719 by allowing the intermediate certificate presented by the icinga2-agent. To make this work the icinga2-master only holds to root-ca in its local ca.crt, while the icinga2-agent has the intermediate-cert in its local ca.crt (or the intermediate together with the root in the ca.crt / or the intermediate in the cert.pem - doesn't matter).
Al2Klimov
force-pushed
the
certchain_client
branch
from
b1687b3
to
ffdf278
Compare
Similar to #8859 this patch works around #7719 by allowing the intermediate certificate presented by the icinga2-agent.
To make this work the icinga2-master only holds to root-ca in its local ca.crt, while the icinga2-agent has the intermediate-cert in its local ca.crt (or the intermediate together with the root in the ca.crt / or the intermediate in the cert.pem - doesn't matter).
Edit (@Al2Klimov)
closes #9026