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

Fixed: ssl conditions in http.py #17

Merged
merged 3 commits into from
Dec 31, 2023
Merged

Conversation

rohitpawar2811
Copy link
Contributor

@rohitpawar2811 rohitpawar2811 commented Dec 27, 2023

Previously, when SSL was enabled, self.proto was not set, causing issues with functions like get_table_names, and get_columns.

  • Fixed the Use_ssl condition, which was previously not supported when using kwargs.
  • By default, session.verify is set to true; now, it is set to false. If verify_ssl is true, then enable it.

@aadel

@@ -90,6 +89,13 @@ def create_connect_args(self, url, **kwargs):

# Prepare a session with proper authorization handling.
session = Session()
#session.verify property which is bydefault true so Handled here
session.verify = False
Copy link
Owner

Choose a reason for hiding this comment

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

Two points here:

  1. I would suggest keeping session.verify to the default True and the parameter can alter that to 'False`, i.e.:
if "verify_ssl" in url.query and url.query["verify_ssl"] in [False, "False", "false"]:
  session.verify = False
 ...
  1. According to the Requests documentation, there are two different session attributes: verify which is a boolean and cert which represents the path to the SSL client certificate. By contrast, only request method accepts verify parameter as either a boolean or string path to CA bundle. How does that apply to the logic below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the [Requests documentation](https://requests.readthedocs.io/en/latest/api), there are two different session attributes: [verify](https://requests.readthedocs.io/en/latest/api/?highlight=verify#requests.Session.verify) which is a boolean and [cert](https://requests.readthedocs.io/en/latest/api/?highlight=verify#requests.Session.cert) which represents the path to the SSL client certificate. By contrast, only request method accepts verify parameter as either a boolean or string path to CA bundle. How does that apply to the logic below?

Actually here I have refered below condition in _solrdbapi.py : I think then should we have to change this also?

if verify_ssl is False:
    session.verify = False
else:
    if ca_certs is not None:
        session.verify = ca_certs
    else:
        session.verify = True

Copy link
Contributor Author

@rohitpawar2811 rohitpawar2811 Dec 28, 2023

Choose a reason for hiding this comment

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

I would suggest keeping session.verify to the default True and the parameter can alter that to 'False`, i.e.:
if "verify_ssl" in url.query and url.query["verify_ssl"] in [False, "False", "false"]:
  session.verify = False 
 

Are you intending to enforce certificate verification by default unless the client explicitly sets verify_ssl to False?

@rohitpawar2811
Copy link
Contributor Author

@aadell,
According to your thoughts, I have made some changes. Please take a look at them and let me know if any modifications are needed.

Thank you.

@aadel
Copy link
Owner

aadel commented Dec 31, 2023

Hi @rohitpawar2811 LGTM. thank you for your contribution!

@rohitpawar2811
Copy link
Contributor Author

Thanks, @aadel, for your support. I'm curious about when these changes will be available in the next release. Could you please provide the expected release date? Also, could you share your LinkedIn ID? I'd like to add you to my connections. It's been great to connect with you.

@aadel aadel merged commit d327770 into aadel:master Dec 31, 2023
@aadel
Copy link
Owner

aadel commented Dec 31, 2023

  1. I estimate the next release will be available by January 5, 2024, which will include SSL updates along with all resolved issues since 0.2.2.1.
  2. Sure! You're welcome to connect on LinkedIn.

Thanks!

@rohitpawar2811
Copy link
Contributor Author

Okay thanks for info.

https://www.linkedin.com/in/rohitpawar2811/ : connect me

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

2 participants