Add support for multiple client certificates#6162
Conversation
roji
left a comment
There was a problem hiding this comment.
LGTM, just see question about .NET limitation below.
| else | ||
| { | ||
| var cert = string.IsNullOrEmpty(password) | ||
| ? X509Certificate2.CreateFromPemFile(certPath, keyPath) |
There was a problem hiding this comment.
So there's no way to read multiple certificates (as above with X509Certificate2Collection.ImportFromPemFile()) if the PEM file happens to be encrypted with a password or key? While with PKCS12 it's possible (below we do X509CertificateLoader.LoadPkcs12CollectionFromFile)?
Is this a weird/arbitrary .NET limitation, is it tracked somewhere?
There was a problem hiding this comment.
Seems to be intentional dotnet/runtime#31944 (comment)
To quote:
Having the collection import support key matching requires a whole lot more work and a whole lot of failure cases, and I'd really like to avoid having a simple-looking method that requires pages and pages of documentation of what it does with unmatched keys, what happens if the same cert is specified twice and a key matches it, are multiple keys even supported, et cetera, at such a low level.
I'm still investigating a few things in regards to how pg actually behaves so there might be some other changes in this pr.
There was a problem hiding this comment.
OK, it's a mess.
A bit of context: you can send certificates from a client to the server which then can validate them either for auth or just in general. That's so called client certificates.
One requirement for them is that client certificate has to have private key. Just having a certificate with public key would not do.
The basic validation pg can do is that that certificate is signed by some other trusted certificate (CA). Here there is no issue, we already support this from our side.
The problem is when there are multiple certificates in between the trusted (CA) and the client (leaf). Let's say we have a CA->intermediate->leaf chain. PG only has CA while we have both intermediate and leaf certificates. In this case we have to send both of them, as otherwise PG can't establish the whole chain and rejects our connection. That's the original issue this pr is fixing.
Now, here's the fun part.
- There is no explicit API in .NET to load the first certificate from the chain with key/password and other with only public keys. You either load them all together with whatever they had in that file, or you somehow parse that file and load each certificate separately. I've managed to work around this thanks to the quirk
X509Certificate2.CreateFromPemFilehas: it loads only the first certificate, ignoring everything else. This way we can load the first certificate with both key and password and then load all other certificates viaX509Certificate2Collection.ImportFromPemFile. This still makes it so we have a duplicate of the first certificate, but it's easy to remove it since we know it's the first one. SslClientAuthenticationOptionshas a propertyClientCertificateswhich we're using to send client certificates. The problem is, it doesn't work for intermediate certificates. If they're not trusted or they don't have a private key,SslStreamseems to completely ignore them, making the chain incomplete. From my understanding, that's due to the way SChannel works (and this behavior was copied to other platforms). Which means that we have to useSslStreamCertificateContext.Createand explicitly specify which certificate is leaf and which are additional. The main issue is thatSslStreamCertificateContextdoesn't exist for .NET Framework, so that means we can't backport that fix to npgsql 8, but I'm personally OK with leaving it to 9 and 10.
There was a problem hiding this comment.
There is no explicit API in .NET to load the first certificate from the chain with key/password and other with only public keys.
OK. I just want to confirm that the thing we're supporting here (loading first certificate with key/password, the rest of the certificates with public key only) is "standard" - or just supported by libpq (if libpq supports it ideally so should we). If so, seems quite smelly that we need to hack around like this in .NET rather than having a more 1st-class API (is it worth raising the problem more explicitly?).
Otherwise at this point doing something that can't be backported to netfx seems completely fine to me.
7342196 to
66da7b8
Compare
|
Backported to 9.0.4 via 5d073da (but should work only when targeting .NET 8 or higher due to API limitations) |
Fixes #6152