feat(csharp): Add configurable certificate revocation mode#3962
Closed
eric-wang-1990 wants to merge 7 commits into
Closed
feat(csharp): Add configurable certificate revocation mode#3962eric-wang-1990 wants to merge 7 commits into
eric-wang-1990 wants to merge 7 commits into
Conversation
Add support for configurable certificate revocation checking to address connection failures in restricted network environments (AWS PrivateLink, corporate firewalls) where CRL/OCSP servers are unreachable. Changes: - Add revocation_mode parameter to HttpTlsOptions and StandardTlsOptions - Support three modes: online (default), offline, nocheck - Add ParseRevocationMode helper for case-insensitive parsing - Apply RevocationMode to X509Chain validation in ValidateCertificate - Add 40 comprehensive unit tests (20 for HTTP, 20 for Standard) - All tests passing (42/42) Parameters: - adbc.http_options.tls.revocation_mode - adbc.standard_options.tls.revocation_mode Values: - "online": Check via network (default, requires port 80 to CRL/OCSP) - "offline": Check using locally cached CRL only - "nocheck": Skip revocation checking (recommended for PrivateLink) Fixes connection timeouts when port 80 access to CRL servers (crl3.digicert.com, ocsp.digicert.com) is blocked. Provides feature parity with ODBC (CheckCertRevocation) and JDBC (checkCertificateRevocation) drivers.
eric-wang-1990
commented
Feb 11, 2026
| chain.ChainPolicy.ExtraStore.Add(issuer); | ||
| chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllowUnknownCertificateAuthority; | ||
| chain.ChainPolicy.RevocationMode = X509RevocationMode.Online; | ||
| chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; |
Contributor
Author
There was a problem hiding this comment.
Why this is hardcode to NoCheck?
Contributor
Author
There was a problem hiding this comment.
✅ Done - Added clarifying comment explaining why NoCheck is intentional here.
This helper method only validates cryptographic signatures (used by IsSelfSigned()), not full certificate chain validation. The actual configurable revocation checking happens in ValidateCertificate() on line 200 using tlsProperties.RevocationMode.
Checking revocation in this helper would be:
- Redundant (already done in main validation)
- Incorrect for self-signed cert detection
- Performance overhead
Explain why RevocationMode.NoCheck is intentionally hardcoded in the IsSignedBy helper method - it only validates cryptographic signatures for self-signed cert detection, not full chain validation with revocation. Addresses PR review comment. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The compiler doesn't understand that IsNullOrWhiteSpace check guarantees value is not null. Add ! operator to suppress the warning. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
jadewang-db
approved these changes
Feb 11, 2026
Contributor
|
@eric-wang-1990, should the Databricks driver switch over to consuming https://github.com/adbc-drivers/hiveserver2 instead of this repo? (I'll happily review it regardless.) |
Contributor
Author
It should but not yet, I plan to cherry pick this change to the hiveserver2 repo as well, so no matter when we cutoff we will always have this fix |
Contributor
Author
|
Converted to draft for now since I will add code to override error message to make it clear |
Provide detailed, actionable error messages for different certificate validation failures instead of generic TLS errors. Each error type now includes: - Clear explanation of what went wrong - Why it happened (common scenarios) - Specific solutions with parameter names Error types covered: - UntrustedRoot: Root CA not in trusted store - PartialChain: Missing intermediate certificates - RevocationStatusUnknown: CRL/OCSP servers unreachable - OfflineRevocation: Revocation server offline (port 80 blocked) - Revoked: Certificate actually revoked (critical security issue) - NotTimeValid: Certificate expired or not yet valid - NotSignatureValid: Invalid signature (possible tampering) - InvalidNameConstraints: Hostname violations - InvalidBasicConstraints: Misconfigured certificate hierarchy - Default: Generic fallback with raw error details This greatly improves UX for users in restricted environments (AWS PrivateLink, corporate firewalls) by providing clear guidance instead of cryptic SSL errors. Example error message: "Certificate validation failed: The revocation server is offline or unreachable. The certificate contains revocation URLs (CRL/OCSP) that require outbound HTTP (port 80) access, but the connection failed. This is common in AWS PrivateLink environments or networks with restrictive egress rules. To resolve this: Set 'adbc.http_options.tls.revocation_mode=nocheck' to disable revocation checking, or set 'revocation_mode=offline' to use only locally cached revocation data." All 42 tests passing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changed all certificate validation error exceptions from HttpRequestException to AuthenticationException to maintain backward compatibility with existing error handling code. Background: - ValidateCertificate is used as a callback in two contexts: 1. HTTP transport: ServerCertificateCustomValidationCallback 2. Standard transport (Thrift): RemoteCertificateValidationCallback - The callback signature provides 4 parameters (request, cert, chain, errors) but we only use cert and errors, building our own chain with custom RevocationMode and trusted root settings - When callback returns false, .NET throws different exceptions: * HTTP transport → HttpRequestException (generic message) * Standard transport → AuthenticationException (generic message) - When callback throws exception, that exception propagates to caller Why AuthenticationException: - Semantically correct for TLS/certificate authentication failures - Maintains backward compatibility with Standard transport callers that catch AuthenticationException - HttpClient will wrap it appropriately for HTTP transport callers - More appropriate than HttpRequestException since certificate validation failures are authentication errors, not HTTP protocol errors Impact: - Existing code catching AuthenticationException continues to work ✅ - No breaking changes for downstream callers ✅ - Detailed error messages still provided ✅ - All 42 unit tests passing ✅ Example error message: "Certificate validation failed: The revocation server is offline or unreachable. The certificate contains revocation URLs (CRL/OCSP) that require outbound HTTP (port 80) access, but the connection failed. This is common in AWS PrivateLink environments or networks with restrictive egress rules. To resolve this: Set 'adbc.http_options.tls.revocation_mode=nocheck' to disable revocation checking, or set 'revocation_mode=offline' to use only locally cached revocation data." Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
eric-wang-1990
commented
Feb 11, 2026
Comment on lines
317
to
326
| if (!chainValid) | ||
| { | ||
| if (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2)) | ||
| return true; | ||
|
|
||
| // Throw detailed error with actionable guidance | ||
| ThrowDetailedCertificateError(customChain, tlsProperties); | ||
| } | ||
|
|
||
| return chainValid || (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2)); |
Contributor
Author
There was a problem hiding this comment.
Can you simplify to
if (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2))
return true;
if (!chainValid)
ThrowDetailedCertificateError(customChain, tlsProperties);
return true;
Contributor
Author
There was a problem hiding this comment.
✅ Done - Simplified as suggested:
if (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2))
return true;
if (!chainValid)
ThrowDetailedCertificateError(customChain, tlsProperties);
return true;This is cleaner and removes the unreachable code after ThrowDetailedCertificateError.
| ThrowDetailedCertificateError(defaultChain, tlsProperties); | ||
| } | ||
|
|
||
| return false; |
Contributor
Author
There was a problem hiding this comment.
Can this line be reached?
Contributor
Author
There was a problem hiding this 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:
// 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.
Address PR review feedback to simplify the certificate validation code:
1. Simplified the validation logic flow:
- Check self-signed allowance first
- Then check if chain is invalid and throw
- Finally return true (clean, linear flow)
2. Removed unreachable code:
- Previous: return chainValid || (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2))
- This was unreachable because ThrowDetailedCertificateError always throws
- Added explicit InvalidOperationException for compiler control flow
3. Applied same simplification to both validation paths:
- No TrustedCertificatePath (default chain)
- With TrustedCertificatePath (custom chain)
Before:
```csharp
if (!chainValid) {
if (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2))
return true;
ThrowDetailedCertificateError(customChain, tlsProperties);
}
return chainValid || (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2));
```
After:
```csharp
if (tlsProperties.AllowSelfSigned && IsSelfSigned(cert2))
return true;
if (!chainValid)
ThrowDetailedCertificateError(customChain, tlsProperties);
return true;
```
Benefits:
- Cleaner, more readable code
- No redundant checks
- Explicit handling of unreachable code
- Same behavior, simpler logic
All 42 tests passing.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Previous run failed due to flaky timing test in Databricks auth module (MandatoryTokenExchangeDelegatingHandlerTests), unrelated to our changes in HiveServer2TlsImpl certificate validation.
Contributor
Author
|
@CurtHagenlocher I will close this one and instead make the changes here: adbc-drivers/hiveserver2#19. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for configurable certificate revocation checking to address connection failures in restricted network environments (AWS PrivateLink, corporate firewalls) where CRL/OCSP servers are unreachable.
Changes:
Parameters:
Values:
Fixes connection timeouts when port 80 access to CRL servers (crl3.digicert.com, ocsp.digicert.com) is blocked.
Provides feature parity with ODBC (CheckCertRevocation) and JDBC (checkCertificateRevocation) drivers.