Support Client Side Cert for Livy #576
Conversation
There was a problem hiding this comment.
Thank you for the patch! This seems like a useful feature.
Nice to have: Besides the inline comments, I'm starting to think that we really need some sort of end-to-end test framework this could run in... but I realize that's a big change given there aren't any at the moment.
Required: At the very minimum, though, I don't think there's any new unittests for the new codepaths, which would be good. And I'd like a bit added to the README explaining how to use this new feature.
Thank you!
|
|
||
| def __ne__(self, other): | ||
| return not self == other | ||
|
|
||
| def __str__(self): | ||
| return u"Endpoint({})".format(self.url) | ||
|
|
||
| class SSLInfo(object): |
There was a problem hiding this comment.
Add a docstring explain what this class does, and what the parameters are.
There was a problem hiding this comment.
Also, this seems like a good place to start using attrs (http://www.attrs.org/en/stable/), it'll make this class much much shorter.
| @@ -389,7 +389,12 @@ def matplot(self, line, cell="", local_ns=None): | |||
| def refresh_configuration(self): | |||
| credentials = getattr(conf, 'base64_kernel_' + self.language + '_credentials')() | |||
| (username, password, auth, url) = (credentials['username'], credentials['password'], credentials['auth'], credentials['url']) | |||
| self.endpoint = Endpoint(url, auth, username, password) | |||
| (ssl_client_cert, ssl_client_key, ssl_verify) = (credentials.get('ssl_client_cert'), credentials.get('ssl_client_key'), credentials.get('ssl_verify'),) | |||
There was a problem hiding this comment.
Could we say "TLS" instead of "SSL" everywhere? It's been TLS for 20 years now :)
| @@ -61,15 +61,37 @@ def _send_request_helper(self, url, accepted_status_codes, function, data, retry | |||
| try: | |||
| if self._endpoint.auth == constants.NO_AUTH: | |||
| if data is None: | |||
There was a problem hiding this comment.
This now has a huge amount of duplicate paths and code, which makes me worry about errors. Can it be simplified? E.g.
if data is not None:
data = json.dumps(data)
at the top would simplify it quite a lot, and so on.
|
|
||
| def __ne__(self, other): | ||
| return not self == other | ||
|
|
||
| def __str__(self): | ||
| return u"Endpoint({})".format(self.url) | ||
|
|
||
| class SSLInfo(object): | ||
| def __init__(self, client_cert, client_key, ssl_verify): |
There was a problem hiding this comment.
Having two different places where youo set ssl_verify, this and the config, bothers me a little. Is there a reason to have it here and not just rely on the config?
itamarst
added
the
awaiting-submitter-response
Our livy is deployed in such a way:
It has a whitelist of CNs and require client to present a client side certificate, only client cert whose CN match the whitelist will be allow to access livy.
I am adding 3 configs to kernel_{language}_credentials:
ssl_client_cert, ssl_client_key and ssl_verify
I have the following config:
Tested it and the change worked for me.