hubcio commented on code in PR #4304:
URL: https://github.com/apache/iggy/pull/4304#discussion_r4122564490
##########
core/common/src/types/configuration/quic_config/quic_connection_string_options.rs:
##########
@@ -175,7 +175,13 @@ impl ConnectionStringOptions for
QuicConnectionStringOptions {
return Err(IggyError::InvalidConnectionString);
}
},
- "validate_certificate" => {
+ "tls_validate_certificate" | "validate_certificate" => {
+ // TODO: Remove the deprecated `validate_certificate`
alias after the compatibility release.
+ if option_parts[0] == "validate_certificate" {
+ tracing::warn!(
+ "Connection string option 'validate_certificate'
is deprecated; use 'tls_validate_certificate'"
+ );
+ }
validate_certificate = option_parts[1] == "true";
Review Comment:
warning: any value other than an exact `true` reads as false, so a typo like
`tls_validate_certificate=ture` turns certificate verification off and lands on
`SkipServerVerification` at `quic_client.rs:1198`. parse the value with
`parse::<bool>()` and reject the rest, like tcp does at
`tcp_connection_string_options.rs:97`.
also at `websocket_connection_string_options.rs:191`.
##########
core/common/src/types/configuration/tcp_config/tcp_connection_string_options.rs:
##########
@@ -67,6 +72,7 @@ impl ConnectionStringOptions for TcpConnectionStringOptions {
let mut tls_enabled = false;
let mut tls_domain = "".to_string();
let mut tls_ca_file = None;
+ let mut tls_validate_certificate = true;
Review Comment:
warning: this key defaults to `true` on tcp and `false` on quic and
websocket (`quic_connection_string_options.rs:100`,
`websocket_connection_string_options.rs:215`), so one key now means two
security postures. the rustdoc covers both, but
`foreign/cpp/include/iggy.hpp:2067` states it for tcp only.
##########
core/common/src/types/configuration/quic_config/quic_connection_string_options.rs:
##########
@@ -175,7 +175,13 @@ impl ConnectionStringOptions for
QuicConnectionStringOptions {
return Err(IggyError::InvalidConnectionString);
}
},
- "validate_certificate" => {
+ "tls_validate_certificate" | "validate_certificate" => {
Review Comment:
warning: the key is `tls_validate_certificate` now, but the quic field and
builder still say `validate_certificate` (`quic_client_config.rs:54`,
`quic_client_config_builder.rs:147`), while tcp and websocket say
`tls_validate_certificate`. renaming breaks the api, so either note it on #4210
or leave it for a follow-up.
--
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]
