feat(csharp/src/Drivers/Apache): Load certificate bundle from pem file #3303
Conversation
Can you file this as a separate work item? |
There was a problem hiding this comment.
Thanks! This isn't really in my area of expertise so I have a lot of questions. Most are inline, but I'm also curious whether the existing code that's being replaced could never have worked. Is there some file format other then .pem that it would have worked with, and are we now losing support for that? Would it be better to support both and use the file extension to distinguish between them?
| return tlsProperties; | ||
| } | ||
|
|
||
| public static List<X509Certificate2> LoadPemCertificates(string pemPath) |
There was a problem hiding this comment.
How big is this file typically, and how long does it take to read and parse? Does it make sense to keep a cache of these by file path and size/last update time so that it doesn't need to be reloaded regularly?
The standard ~cacert.pem is about 220kb.
| bool trustedBy = false; | ||
| foreach (X509ChainElement element in customChain.ChainElements) | ||
| { | ||
| foreach (X509Certificate2 ca in collection) |
There was a problem hiding this comment.
I know that cert chains typically aren't that long, but this is n^2 behavior. Is it worth loading the certificates as a dictionary keyed by thumbprint in order to speed up the search?
| { | ||
| foreach (X509Certificate2 ca in collection) | ||
| { | ||
| if (element.Certificate.Thumbprint == ca.Thumbprint) |
There was a problem hiding this comment.
Should this be a case-insensitive comparison or are thumbprints always normalized to a specific case? Is comparing just the thumbprint enough for good security?
| chainValid = chainValid && trustedBy; | ||
| } | ||
|
|
||
| return chainValid || (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2)); |
There was a problem hiding this comment.
Consider modifying the check around line 178 to return early for a self-signed certificate even if TrustedCertificatePath is not null. There's no reason to go to the trouble of loading the additional certificates if we already know we have a self-signed cert. Then the check here becomes superfluous and can be removed.
4 participants
No description provided.