Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

meldafert
Copy link
Contributor

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

  • I've added this contribution to the changelog.md.
  • I've added my name to the AUTHORS file (or it's already there).

Copy link
Member

@amjith amjith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the feature. I don't personally use ssl context so pardon my questions if they're too basic. I've left some questions inline for own clarification.

mycli/sqlexecute.py Show resolved Hide resolved
mycli/sqlexecute.py Show resolved Hide resolved
mycli/sqlexecute.py Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2022

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%) ⬇️
mycli/packages/completion_engine.py 61.90% <50.00%> (-0.17%) ⬇️
mycli/main.py 65.32% <100.00%> (+0.04%) ⬆️
mycli/sqlcompleter.py 82.53% <0.00%> (-1.75%) ⬇️

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

@dveeden
Copy link
Contributor

dveeden commented Sep 23, 2022

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

  • --tls-version TLSv1.2,TLSv1.3 Allowing these two versions only
  • --min-tls-version TLSv1.2 Allowing TLSv1.2 and up

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

@amjith amjith marked this pull request as ready for review March 25, 2024 03:01
@amjith amjith merged commit dd7bd76 into dbcli:main Mar 25, 2024
3 of 6 checks passed
@meldafert meldafert deleted the option-tls-version branch April 3, 2024 09:23
@meldafert
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding a new "TLS version" argument to mycli command line options ?
4 participants