-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Support for TLS auth #250
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
Support for TLS auth #250
Conversation
…t for SSL in docker-py. Including the ability to specify the expected SSL Version for issues with OpenSSL sslv3/tls1 recognition issues. Added an exception class for repetitive reminders to look at the CLI doc on docker.io.
* Exported TLS configuration in tls.TLSConfig * Merged exceptions packagee into pre-existing errors module * Flake8 fixes * Bug fixes
@denibertovic @momer If you guys can spare a minute to review this, it would help tremendously! |
If tests pass and this is able to connect to daemons with TLS enabled, then I'd say merge it in! Nice tag team action all, Mo |
Also, when doing the procedure described in the docs i get this: AttributeError: 'TLSConfig' object has no attribute 'cert' |
Right - however I was unable to get self.verify to connect even with my own CA file; hence the caveat in comments. If you're able to get it to work, then by all means go for it. I'd done some cleaning up on another branch, re the type error, but other changes in master caused issues that I don't have time to dig into atm. |
I fixed a couple bugs with the last commit (overlooks on my part, sorry about that) and acted upon some of @denibertovic 's comments. Are you guys able to make this work now? I'll see if I can add unit tests that make sense here later. |
Good work guys , hope this function merge in as soon as possible. |
@shin- shoot, I've been swamped the last few days and forgot about this. Will take a look again later today and give feedback. |
@shin- when trying to instantiate the client like so: I get the following error: /home/deni/.virtualenvs/test/src/dockerpy/docker/client.py in init(self, base_url, version, timeout, tls) AttributeError: 'Client' object has no attribute 'ssl_version' |
…e default when mounting adapter
Thanks, fixed that one too. |
@shin- and @denibertovic I strongly disagree with removing that little argument. The entire reason for the SSLAdapter was to address an issue in many clients (I believe Ubuntu 12.04 and 14.04 are affected, at least my 12.04 was) where the SSL Version needed to be specified. The reason the error popped up was because in the changes made since my initial commit (which worked on the branch at the time of it's parent) somewhere was removed the initialization of Please see the comments at the top of https://github.com/momer/docker-py/blob/7ce73de4a710b6ccd334673bd3c4d1ee667addea/docker/ssladapter/ssladapter.py Edit: Also note that without that argument, if a user is affected (as I was), they will be unable to connect to their docker instance using TLS. |
@momer it's not removed, if you provide a TLSConfig() object with the config = docker.tls.TLSConfig(True, ssl_version='...')
c = docker.Client(host='<host>', tls=config) will do the trick. |
Right on! I'd thought it was en route to being removed as a parameter. |
Don't worry, I'm not removing functionality, just squashing bugs at the moment =) |
I can confirm that I can get it to work if I explicitly set tls_verify=False (ie. not check the server certificate with tls_ca_cert). If i try to validate the server certificate with tls_ca_cert I get this error: SSLError: [Errno 1] _ssl.c:510: error:14090086:SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed |
I think we can handle that one as a separate bug / investigation. As discussed with @denibertovic over IRC, the only thing that's blocking this merge is the API itself. We try to stick with the CLI params but those are a mess already, and trying to translate them to python makes it worse still. We might want to do our own thing here. That doesn't invalidate the work done here since most of it will still be good, but making the interface more user-friendly would be great; |
That's the same issue I ran into, which is why TLS Verify was only enabled if the TLS ca cert file was provided (which worked) OR explicitly specified. That should be caveated as I had in the comments, and addressed in the docs. It seems the issue is somewhere up the chain. 👍 for removing the original restriction of docker API mapping |
fix ssl_version exception when urllib3 version <= 1.5
…ved / amended docs
Improved TLSConfig API
Conflicts: docker/client.py
And it's in! Thanks everyone for your contributions and reviews! |
Based on #226