add ssl verification option #269
Conversation
Introduces new CLI options for SSL certificate handling: - --insecure: Disable SSL verification - --ssl-ca-file: Specify custom CA certificate file Options can also be configured via config file or environment variables.
- Improve SSL verification logic and configuration options - Add proper ssl_ca_file path handling with escaping - Implement comprehensive SSL options test coverage - Add warning message for insecure SSL usage - Fix environment variable parsing for SSL verification
mickeey2525
changed the title
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| * Update password symbol for user creation #262 | |||
| * Add support for td-client-ruby 2.x.x #267 | |||
| * Add SSL certificate options (--insecure, --ssl-ca-file) for proxy environments | |||
There was a problem hiding this comment.
This section is for current release, please remove. We will add the changelog during release process
| * Specify an alternative endpoint to use updating the JAR file (default: https://repo1.maven.org): | ||
| === SSL Options | ||
|
|
||
| $ TD_TOOLBELT_JARUPDATE_ROOT="https://repo1.maven.org" |
There was a problem hiding this comment.
Any reason to remove these lines?
lib/td/command/common.rb
Outdated
| # SSL verification options | ||
| if Config.ssl_verify == false | ||
| opts[:verify] = false | ||
| elsif Config.ssl_ca_file | ||
| opts[:verify] = Config.ssl_ca_file | ||
| end |
There was a problem hiding this comment.
Why can we unify ssl_verify and ssl_ca_file into a single ssl_verify option similar to td-client-ruby's verify option?
After all, if ssl_verify is false, ssl_ca_file value does not apply anyway.
lib/td/command/runner.rb
Outdated
| @@ -104,16 +105,24 @@ def run(argv=ARGV) | |||
| import_endpoint = e | |||
| } | |||
|
|
|||
| op.on('--insecure', "Insecure access: disable SSL (enabled by default)") {|b| | |||
| op.on('--insecure', "Insecure access: disable SSL certificate verification (enabled by default)") {|b| | |||
There was a problem hiding this comment.
I'm not sure it's a good idea to reuse --insecure here. As it just means enable/disable ssl, not certification verification. Changing semantic for this --insecure option can cause confusion to both users and maintainers. We can consider deprecate --insecure option and remove it in the near future
IMO, it's better to introduce new --ssl-verify option that handles both ssl-verify and ssl-ca-file
There was a problem hiding this comment.
--insecure itself was causing a lot of confusion for our customers in the history.
Some customers try to use insecure option to disable ssl verification and it didn't work as expected as it just change the protocol.
So for me, it is natural to use --insecure for this option. but not that strong desire.
And --insecure is already meaning less as TD enforces https communication in the most case.
it's better to introduce new --ssl-verify option that handles both ssl-verify and ssl-ca-file
I think this would cause some confusion to merge options into one options as they need to understand one option has 2 meanings.
Purpose
To address this issue, I added ssl verification options.
#256
About --insecure option
This does not work anymore and does not match with the users' expectations.