eric-wang-1990 commented on code in PR #3962:
URL: https://github.com/apache/arrow-adbc/pull/3962#discussion_r2795854842
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2TlsImpl.cs:
##########
@@ -152,17 +283,46 @@ static internal bool ValidateCertificate(X509Certificate?
cert, SslPolicyErrors
if (string.IsNullOrEmpty(tlsProperties.TrustedCertificatePath))
{
- return
!policyErrors.HasFlag(SslPolicyErrors.RemoteCertificateChainErrors) ||
(tlsProperties.AllowSelfSigned && IsSelfSigned(cert2));
+ // If chain errors exist, provide detailed error information
+ if
(policyErrors.HasFlag(SslPolicyErrors.RemoteCertificateChainErrors))
+ {
+ if (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2))
+ return true;
+
+ // Build a chain to get detailed error information
+ using (X509Chain defaultChain = new X509Chain())
+ {
+ defaultChain.ChainPolicy.RevocationMode =
tlsProperties.RevocationMode;
+ defaultChain.Build(cert2);
+
+ // Throw detailed error with actionable guidance
+ ThrowDetailedCertificateError(defaultChain,
tlsProperties);
+ }
+
+ return false;
+ }
+ return true;
}
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;
- customChain.ChainPolicy.RevocationMode = X509RevocationMode.Online;
+ customChain.ChainPolicy.RevocationMode =
tlsProperties.RevocationMode;
bool chainValid = customChain.Build(cert2);
+
+ // Check for validation failures and provide detailed error message
+ if (!chainValid)
+ {
+ if (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2))
+ return true;
+
+ // Throw detailed error with actionable guidance
+ ThrowDetailedCertificateError(customChain, tlsProperties);
+ }
+
return chainValid || (tlsProperties.AllowSelfSigned &&
IsSelfSigned(cert2));
Review Comment:
✅ Done - Simplified as suggested:
```csharp
if (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2))
return true;
if (!chainValid)
ThrowDetailedCertificateError(customChain, tlsProperties);
return true;
```
This is cleaner and removes the unreachable code after
`ThrowDetailedCertificateError`.
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2TlsImpl.cs:
##########
@@ -152,17 +283,46 @@ static internal bool ValidateCertificate(X509Certificate?
cert, SslPolicyErrors
if (string.IsNullOrEmpty(tlsProperties.TrustedCertificatePath))
{
- return
!policyErrors.HasFlag(SslPolicyErrors.RemoteCertificateChainErrors) ||
(tlsProperties.AllowSelfSigned && IsSelfSigned(cert2));
+ // If chain errors exist, provide detailed error information
+ if
(policyErrors.HasFlag(SslPolicyErrors.RemoteCertificateChainErrors))
+ {
+ if (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2))
+ return true;
+
+ // Build a chain to get detailed error information
+ using (X509Chain defaultChain = new X509Chain())
+ {
+ defaultChain.ChainPolicy.RevocationMode =
tlsProperties.RevocationMode;
+ defaultChain.Build(cert2);
+
+ // Throw detailed error with actionable guidance
+ ThrowDetailedCertificateError(defaultChain,
tlsProperties);
+ }
+
+ return false;
Review Comment:
✅ Good catch! No, that line cannot be reached because
`ThrowDetailedCertificateError` always throws an exception.
I've updated the code to make this explicit by adding:
```csharp
// Unreachable - ThrowDetailedCertificateError always throws
throw new InvalidOperationException("ThrowDetailedCertificateError should
have thrown an exception");
```
This satisfies the compiler's control flow analysis while documenting that
this code should never execute.
--
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]
