dbcli / mycli

A Terminal Client for MySQL with AutoCompletion and Syntax Highlighting.
http://mycli.net
Other
11.32k stars 656 forks source link

Add cli option --tls-version to control tls version used #1073

Closed meldafert closed 3 months ago

meldafert commented 1 year ago

Description

Fixes #1053.

This PR adds a --tls-version option, parallel to the option of the vanilla mysql client. It was implemented by bypassing PyMySQL's ssl logic and creating the SSLContext ourselves.

This PR is currently a draft, as it would require raising the minimum python version to 3.7, as 3.6 has no way to force TLSv1.3. Specifically, SSLContext.minimum_version was only added in 3.7. However, PyMySQL's next update will require 3.7 as well.

This could alternatively be partially implemented inside PyMySQL as well. However, it is a documented option to pass a SSLContext to PyMySQL directly, so this seemed like the easier option. One downside is that the _create_ssl_context method was strongly influenced by the one in PyMySQL - does this need to be noted somewhere for copyright reasons?

Checklist

codecov-commenter commented 1 year ago

Codecov Report

Merging #1073 (1561834) into main (48bd4c9) will decrease coverage by 0.81%. The diff coverage is 14.28%.

@@            Coverage Diff             @@
##             main    #1073      +/-   ##
==========================================
- Coverage   67.92%   67.10%   -0.82%     
==========================================
  Files          26       26              
  Lines        2756     2791      +35     
==========================================
+ Hits         1872     1873       +1     
- Misses        884      918      +34     
Impacted Files Coverage Δ
mycli/sqlexecute.py 74.40% <9.37%> (-9.55%) :arrow_down:
mycli/packages/completion_engine.py 61.90% <50.00%> (-0.17%) :arrow_down:
mycli/main.py 65.32% <100.00%> (+0.04%) :arrow_up:
mycli/sqlcompleter.py 82.53% <0.00%> (-1.75%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dveeden commented 1 year ago

Might be good to consider lists/ranges of TLS versions.

Might be good to by default only allow TLSv1.2 and up. See RFC 8996

meldafert commented 3 months ago

Thanks!