CurtHagenlocher commented on code in PR #3303:
URL: https://github.com/apache/arrow-adbc/pull/3303#discussion_r2291605366
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2StandardConnection.cs:
##########
@@ -112,19 +112,15 @@ protected override TTransport CreateTransport()
TTransport baseTransport;
if (TlsOptions.IsTlsEnabled)
{
- X509Certificate2? trustedCert =
!string.IsNullOrEmpty(TlsOptions.TrustedCertificatePath)
- ? new X509Certificate2(TlsOptions.TrustedCertificatePath!)
- : null;
-
RemoteCertificateValidationCallback certValidator = (sender,
cert, chain, errors) => HiveServer2TlsImpl.ValidateCertificate(cert, errors,
TlsOptions);
if (IPAddress.TryParse(hostName!, out var ipAddress))
{
- baseTransport = new TTlsSocketTransport(ipAddress,
portValue, config: new(), 0, trustedCert, certValidator);
+ baseTransport = new TTlsSocketTransport(ipAddress,
portValue, config: new(), 0, null, certValidator);
Review Comment:
There's similar code in ImpalaStandardConnection.cs. Does that need to be
updated as well?
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2TlsImpl.cs:
##########
@@ -155,14 +180,39 @@ static internal bool ValidateCertificate(X509Certificate?
cert, SslPolicyErrors
return
!policyErrors.HasFlag(SslPolicyErrors.RemoteCertificateChainErrors) ||
(tlsProperties.AllowSelfSigned && IsSelfSigned(cert2));
}
- X509Certificate2 trustedRoot = new
X509Certificate2(tlsProperties.TrustedCertificatePath);
X509Chain customChain = new();
- customChain.ChainPolicy.ExtraStore.Add(trustedRoot);
// "tell the X509Chain class that I do trust this root certs and
it should check just the certs in the chain and nothing else"
customChain.ChainPolicy.VerificationFlags =
X509VerificationFlags.AllowUnknownCertificateAuthority;
+ var collection =
LoadPemCertificates(tlsProperties.TrustedCertificatePath!);
+
+ foreach (var trustedCert in collection)
+ {
+ customChain.ChainPolicy.ExtraStore.Add(trustedCert);
+ }
customChain.ChainPolicy.RevocationMode = X509RevocationMode.Online;
bool chainValid = customChain.Build(cert2);
+ if (chainValid)
+ {
+ bool trustedBy = false;
+ foreach (X509ChainElement element in customChain.ChainElements)
+ {
+ foreach (X509Certificate2 ca in collection)
Review 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?
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2TlsImpl.cs:
##########
@@ -138,6 +139,30 @@ static internal TlsProperties
GetStandardTlsOptions(IReadOnlyDictionary<string,
return tlsProperties;
}
+ public static List<X509Certificate2> LoadPemCertificates(string
pemPath)
Review 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.
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2TlsImpl.cs:
##########
@@ -155,14 +180,39 @@ static internal bool ValidateCertificate(X509Certificate?
cert, SslPolicyErrors
return
!policyErrors.HasFlag(SslPolicyErrors.RemoteCertificateChainErrors) ||
(tlsProperties.AllowSelfSigned && IsSelfSigned(cert2));
}
- X509Certificate2 trustedRoot = new
X509Certificate2(tlsProperties.TrustedCertificatePath);
X509Chain customChain = new();
- customChain.ChainPolicy.ExtraStore.Add(trustedRoot);
// "tell the X509Chain class that I do trust this root certs and
it should check just the certs in the chain and nothing else"
customChain.ChainPolicy.VerificationFlags =
X509VerificationFlags.AllowUnknownCertificateAuthority;
+ var collection =
LoadPemCertificates(tlsProperties.TrustedCertificatePath!);
+
+ foreach (var trustedCert in collection)
+ {
+ customChain.ChainPolicy.ExtraStore.Add(trustedCert);
+ }
customChain.ChainPolicy.RevocationMode = X509RevocationMode.Online;
bool chainValid = customChain.Build(cert2);
+ if (chainValid)
+ {
+ bool trustedBy = false;
+ foreach (X509ChainElement element in customChain.ChainElements)
+ {
+ foreach (X509Certificate2 ca in collection)
+ {
+ if (element.Certificate.Thumbprint == ca.Thumbprint)
Review 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?
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2TlsImpl.cs:
##########
@@ -155,14 +180,39 @@ static internal bool ValidateCertificate(X509Certificate?
cert, SslPolicyErrors
return
!policyErrors.HasFlag(SslPolicyErrors.RemoteCertificateChainErrors) ||
(tlsProperties.AllowSelfSigned && IsSelfSigned(cert2));
}
- X509Certificate2 trustedRoot = new
X509Certificate2(tlsProperties.TrustedCertificatePath);
X509Chain customChain = new();
- customChain.ChainPolicy.ExtraStore.Add(trustedRoot);
// "tell the X509Chain class that I do trust this root certs and
it should check just the certs in the chain and nothing else"
customChain.ChainPolicy.VerificationFlags =
X509VerificationFlags.AllowUnknownCertificateAuthority;
+ var collection =
LoadPemCertificates(tlsProperties.TrustedCertificatePath!);
+
+ foreach (var trustedCert in collection)
+ {
+ customChain.ChainPolicy.ExtraStore.Add(trustedCert);
+ }
customChain.ChainPolicy.RevocationMode = X509RevocationMode.Online;
bool chainValid = customChain.Build(cert2);
+ if (chainValid)
+ {
+ bool trustedBy = false;
+ foreach (X509ChainElement element in customChain.ChainElements)
+ {
+ foreach (X509Certificate2 ca in collection)
+ {
+ if (element.Certificate.Thumbprint == ca.Thumbprint)
+ {
+ trustedBy = true;
+ break;
+ }
+ }
+ if (trustedBy)
+ {
+ break;
+ }
+ }
+ chainValid = chainValid && trustedBy;
+ }
+
return chainValid || (tlsProperties.AllowSelfSigned &&
IsSelfSigned(cert2));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
