[feat] Enable hostname verification by default#20268
Conversation
|
+1. I think it's appropriate that the default secure behavior is better for protecting users. |
…-verification-by-default
…erification-by-default
…erification-by-default
…erification-by-default
…erification-by-default
| @Parameter(names = "--tls-allow-insecure", description = "Allow insecure tls connection #Java, Python") | ||
| protected boolean tlsAllowInsecureConnection; | ||
| // for backwards compatibility purposes | ||
| @Parameter(names = "--hostname_verification_enabled", |
There was a problem hiding this comment.
It seems we should keep the --hostname_verification_enabled flag for backward compatibility.
There was a problem hiding this comment.
The backwards compatibility has to break because we are moving --hostname-verification-enabled to arity 1. As such, users will need to update scripts in order anyway, so it seems like a reasonable time to drop the deprecated parameter.
Note: we have to switch to arity 1 because otherwise JCommander will invert the meaning of the boolean flag and changing the default value will be the opposite of what users expect. See https://jcommander.org/#_boolean
There was a problem hiding this comment.
Thank you for your explanation! When the --hostname-verification-enabled with arity = 1, we must set a value after this flag. This change is incompatible with the old usage.
LGTM
There was a problem hiding this comment.
Could you add this change and usage to PR body?
| cmd.add("--tls-enable-hostname-verification"); | ||
| } | ||
| cmd.add("--tls-enable-hostname-verification"); | ||
| cmd.add(String.valueOf(authConfig.isTlsHostnameVerificationEnable())); |
There was a problem hiding this comment.
Could you add the default value to AuthenticationConfig?
@Builder.Default
private boolean tlsHostnameVerificationEnable = true;
There was a problem hiding this comment.
Great point, I should update that field. Does updating that definition change these lines of code too?
There was a problem hiding this comment.
Your current approach is correct.
|
The pr had no activity for 30 days, mark with Stale label. |
|
Closed as stale and conflict. |
PIP: This will require a PIP. It is a draft for now while I get tests passing.
Motivation
It is recommended to use hostname verification in most use cases for TLS. In order to have more secure defaults, I propose that we enable TLS hostname verification by default.
This change will not affect any users that do not have TLS enabled. It will only be a breaking change for users that want to use TLS with hostname verification disabled.
Modifications
Verifying this change
This is a trivial change from a configuration perspective. I expect many tests will fail though, so those will also verify the changes.
Does this pull request potentially affect one of the following parts:
Documentation
doc-requiredMatching PR in forked repository
PR in forked repository: michaeljmarshall#44