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

feat: add support for configuring TLS verification policy #442

Merged
merged 4 commits into from Aug 9, 2022

Conversation

torrefatto
Copy link
Contributor

@torrefatto torrefatto commented Aug 5, 2022

Hi! Thanks for this very nice piece of software. First of all, I would like to compliment you for the high quality python code in this repo. It is refreshing to read it.

I am opening this PR to add support for explicit configuration of the ssl_cert_reqs parameter, in case the connection schema is rediss. I took the liberty to also update the README with an updated iredis --help.

To pass values to the underlying SSLConnection object, the user has now two choices:

  • specify the value of ssl_cert_reqs as a query parameter in the url specified via --url
  • specify such value using the --verify-ssl option

Both check that the value be among none, optional or required, and the former takes precedence over the latter (same behavior as the db parameter).

This should tackle issue 403.

iredis/client.py Outdated Show resolved Hide resolved
iredis/entry.py Outdated Show resolved Hide resolved
Copy link
Owner

@laixintao laixintao left a comment

Choose a reason for hiding this comment

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

Thanks, it's a very nice PR!

I made some (picky) suggestions, let me know what do you think.

@laixintao
Copy link
Owner

You can also leave some notes and your name in https://github.com/laixintao/iredis/blob/master/CHANGELOG.md 🔮

Leonardo Barcaroli and others added 2 commits August 8, 2022 18:36
Replace the `if None` checks with a more concise `or`

Co-authored-by: laixintao <laixintaoo@gmail.com>
@torrefatto
Copy link
Contributor Author

Thanks for the suggestions! I incorporated them in the PR (and validated them with a local test instance 😉).

I also updated CHANGELOG.md.

@laixintao laixintao merged commit d0cafb6 into laixintao:master Aug 9, 2022
@laixintao
Copy link
Owner

thanks 🍰

@laixintao laixintao mentioned this pull request Aug 9, 2022
4 tasks
@introom
Copy link

introom commented Dec 19, 2022

Hi @laixintao , looks like in versin 1.12.1, the --verify-ssl parameter is not supported , even though the document claims so.

@laixintao
Copy link
Owner

Hi @introom

I checked the code, it's supported.

connection_kwargs["ssl_cert_reqs"] = verify_ssl

Can you provide a testcase that --verify-ssl didn't work? thanks

@introom
Copy link

introom commented Dec 20, 2022

Hi @introom

I checked the code, it's supported.

connection_kwargs["ssl_cert_reqs"] = verify_ssl

Can you provide a testcase that --verify-ssl didn't work? thanks

Given

	iredis --verify-ssl none --url rediss://some-path

It will return:

Usage: iredis [OPTIONS] [CMD]...
Try 'iredis --help' for help.

Error: no such option: --verify-ssl

iredis, version 1.12.1

@laixintao
Copy link
Owner

laixintao commented Dec 29, 2022

hi @introom confirmed this is a release issue, we merged but don't include it in release of 1.12.1.

I have released a new version 1.13.0, can you upgrade iredis and try again? thanks.

@introom
Copy link

introom commented Dec 29, 2022

hi @introom confirmed this is a release issue, we merged but don't include it in release of 1.12.1.

I have released a new version 1.13.0, can you upgrade iredis and try again? thanks.

I am on homebrew not pip, so gonna be some delay

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.

None yet

3 participants