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

Make CA Persistent #4935

Closed
wants to merge 5 commits into from
Closed

Make CA Persistent #4935

wants to merge 5 commits into from

Conversation

AndTheDaysGoBy
Copy link

Was trying to get docker-py working with a remote host, i.e. using a key.pem, cert.pem, and ca.pem. Had SSL issues. Ultimately figured out that it was due to the APIClient being derived from requests.sessions. In particular, even if verify is set for the session, it will not actually be used since the kwargs for the get() are directly passed into the request() method (which defaults verify to None).
As additional info., curling worked when passing the cert.pem, ca.pem, and key.pem, requests worked when merely using requests.get(url, cert=(cert, key), verify=ca), and sessions worked as long as verify was always passed into the get() method, i.e. s.get(url, verify=ca, ...)

Uses the default cacert (e.g. set via s = requests.Sessions(), s.verify = cacert) when it's not passed in via s.get(url, verify=cacert).
@AndTheDaysGoBy
Copy link
Author

AndTheDaysGoBy commented Jan 11, 2019

By the looks of it, I think the tests aren't passing because the tests expect verify=None (as it would be set in the requests() method should it not be passed into the kwargs from the get()). However, is this the intended behavior desired? Since it's a session, it should be using the self.verify when it isn't explicitly passed into the method. Therefore, under this assumption, wouldn't the tests be testing incorrect behavior since they expect the session's verify to be None as opposed to the session's default (self.verify = True set in the init)?

Edit:
Running python27 locally. I tried just running pytest on master and it fails due to the MarkDecorator has no len() error (that being the only one).

@AndTheDaysGoBy
Copy link
Author

I noticed that there is an actual merge_settings function that is used which supposedly performs the behavior I desire. I will have to look into why it doesn't appear to work for me for verify. I'll open an actual issue once I discover it, and update the proposed solution (if one is needed).

@kennethreitz
Copy link
Contributor

thanks!

@AndTheDaysGoBy
Copy link
Author

@kennethreitz Thank you for responding, but it seems this issue was never fixed. I.e., the corresponding issue: #4938 had been closed without any discussion or resolution. Luckily, someone else found the same or related issue in: home-assistant-ecosystem/home-assistant-cli#184 (comment)
However, that issue has gone stale (thankful some activity did occur there, so perhaps this issue has entered the public light more).
I would hope either my fix or any fix for this could be implemented. In that other branch they state stuff about WebSocket API. If waiting for that will fix this, good, but I would hope this issue to be kept in the light somehow.

@kennethreitz kennethreitz reopened this Aug 20, 2019
@AndTheDaysGoBy
Copy link
Author

AndTheDaysGoBy commented Aug 21, 2019

@kennethreitz OK, re-merged with master, however, the CI fails on 2.7 since super() is expecting an argument despite the fact that the present code in master seems to be making use of Python 3.X super(). Since all the other builds pass, I presume my code change to not be the source of the issue--in particular since I've not modified anything in regards super.

@AndTheDaysGoBy
Copy link
Author

AndTheDaysGoBy commented Aug 21, 2019

Actually, it seems this should have worked at some point considering it was 7 years ago that:

kwargs.setdefault('verify', self.verify)
was added. I can only presume that with the introduction of
settings = self.merge_environment_settings(
which merges the verify flag (amongst other things for that matter) that things got out of wack. After all, this makes there to be 2 places where some interaction is going on between the session set values and the passed in values. The original which was in the .send() should have take care of everything since, in theory, back before the merge_settings stuff entered the picture, the fact that verify wasn't in the call to request(...) would result in it not being in the call to send(...) consequently, it would use the default value.

In light of this I will state that my current PR just has the .get() fixed, however, these fixes of adding .setdefault() of the verify everywhere is more so a hack fix in light of this new info. The true fix would be to either ensure we don't perform additional magic regarding the passed in value and the default value between the def request(...) and the def send(...) so that the existing kwargs.setdefault(...) in send may perform their job as required. The alternative would be to extract said defaults out of send and have them exist purely in requests. I personally thing the prior to be the better option because I'm unsure how much destruction would occur from taking the setdefaults out of send. After all, who knows what's calling the send function and what is expected to be passed along.

When I get the chance, I'll git rebase this branch and attempt that approach. The issue of Python 2.7 super() though still remains a separate issue though, I believe.

This reverts commit dc35933.
@AndTheDaysGoBy
Copy link
Author

Closing in favor of: #5172

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants