CurtHagenlocher commented on code in PR #2745:
URL: https://github.com/apache/arrow-adbc/pull/2745#discussion_r2070852173
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2TlsImpl.cs:
##########
@@ -97,5 +97,49 @@ static internal HttpClientHandler
NewHttpClientHandler(TlsProperties tlsProperti
}
return httpClientHandler;
}
+
+ static internal TlsProperties
GetStandardTlsOptions(IReadOnlyDictionary<string, string> properties)
+ {
+ TlsProperties tlsProperties = new();
+ if (properties.TryGetValue(StandardTlsOptions.IsTlsEnabled, out
string? isTlsEnabled) && bool.TryParse(isTlsEnabled, out bool isTlsEnabledBool))
+ {
+ tlsProperties.IsTlsEnabled = isTlsEnabledBool;
+ }
+ if (!tlsProperties.IsTlsEnabled)
+ {
+ return tlsProperties;
+ }
+
+ if
(properties.TryGetValue(StandardTlsOptions.DisableServerCertificateValidation,
out string? disableServerCertificateValidation) &&
bool.TryParse(disableServerCertificateValidation, out bool
disableServerCertificateValidationBool) &&
disableServerCertificateValidationBool)
+ {
+ tlsProperties.DisableServerCertificateValidation = true;
+ return tlsProperties;
+ }
+ tlsProperties.DisableServerCertificateValidation = false;
+ tlsProperties.AllowHostnameMismatch =
properties.TryGetValue(StandardTlsOptions.AllowHostnameMismatch, out string?
allowHostnameMismatch) && bool.TryParse(allowHostnameMismatch, out bool
allowHostnameMismatchBool) && allowHostnameMismatchBool;
+ tlsProperties.AllowSelfSigned =
properties.TryGetValue(StandardTlsOptions.AllowSelfSigned, out string?
allowSelfSigned) && bool.TryParse(allowSelfSigned, out bool
allowSelfSignedBool) && allowSelfSignedBool;
+ if (tlsProperties.AllowSelfSigned)
+ {
+ if
(!properties.TryGetValue(StandardTlsOptions.TrustedCertificatePath, out string?
trustedCertificatePath)) return tlsProperties;
+ tlsProperties.TrustedCertificatePath = trustedCertificatePath
!= "" && File.Exists(trustedCertificatePath) ? trustedCertificatePath : throw
new FileNotFoundException("Trusted certificate path is invalid or file does not
exist.");
+ }
+ return tlsProperties;
+ }
+
+ static internal RemoteCertificateValidationCallback
getCertificateValidator(TlsProperties tlsProperties)
Review Comment:
```suggestion
static internal RemoteCertificateValidationCallback
GetCertificateValidator(TlsProperties tlsProperties)
```
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2TlsImpl.cs:
##########
@@ -97,5 +97,49 @@ static internal HttpClientHandler
NewHttpClientHandler(TlsProperties tlsProperti
}
return httpClientHandler;
}
+
+ static internal TlsProperties
GetStandardTlsOptions(IReadOnlyDictionary<string, string> properties)
+ {
+ TlsProperties tlsProperties = new();
+ if (properties.TryGetValue(StandardTlsOptions.IsTlsEnabled, out
string? isTlsEnabled) && bool.TryParse(isTlsEnabled, out bool isTlsEnabledBool))
Review Comment:
A failure to parse the value as boolean should probably cause an error
instead of silently taking the default. (At least the default value is `true`,
so a user error is more likely to lead to a failure than a silent security
issue.)
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2TlsImpl.cs:
##########
@@ -97,5 +97,49 @@ static internal HttpClientHandler
NewHttpClientHandler(TlsProperties tlsProperti
}
return httpClientHandler;
}
+
+ static internal TlsProperties
GetStandardTlsOptions(IReadOnlyDictionary<string, string> properties)
Review Comment:
There's no way to share this setup logic with the HTTP code path? They are
practically identical.
In hindsight, it would have been better to have a TLS property set that was
independent of HTTP vs TCP transport.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
