SSL with custom certfiles path #435
Conversation
sparkmagic/example_config.json
Outdated
| @@ -48,6 +48,7 @@ | |||
| "fatal_error_suggestion": "The code failed because of a fatal error:\n\t{}.\n\nSome things to try:\na) Make sure Spark has enough available resources for Jupyter to create a Spark context.\nb) Contact your Jupyter administrator to make sure the Spark magics library is configured correctly.\nc) Restart the kernel.", | |||
|
|
|||
| "ignore_ssl_errors": false, | |||
| "custom_certfiles_path": "cacerts.pem", | |||
There was a problem hiding this comment.
Let's change default value so that it ends up with None value in python.
| @@ -33,6 +33,10 @@ def __init__(self, endpoint, headers, retry_policy): | |||
| if not self.verify_ssl: | |||
| self.logger.debug(u"ATTENTION: Will ignore SSL errors. This might render you vulnerable to attacks.") | |||
| requests.packages.urllib3.disable_warnings() | |||
| else: | |||
| if conf.custom_certfiles_path is not None: | |||
There was a problem hiding this comment.
This should be a method call
|
LGTM otherwise! @apetresc, do you have any comments? |
| @@ -164,6 +164,9 @@ def resource_limit_mitigation_suggestion(): | |||
| def ignore_ssl_errors(): | |||
| return False | |||
|
|
|||
| @_with_override | |||
| def custom_certfiles_path(): | |||
| return None | |||
There was a problem hiding this comment.
@aggFTW this is how we handle None value for custom_certfiles_path , so I'm not sure what to do for example_config.json
There was a problem hiding this comment.
If you did:
"custom_certfiles_path": null,
would it end up returning None?
I think it would. Maybe try that out.
There was a problem hiding this comment.
What do you think about not using "custom_certfiles_path": null in the configuration file but telling in the readme that we can set this value ?
I, personally, don't like to have null value set somewhere
There was a problem hiding this comment.
The purpose of having an example_config.json in the repo is to have a place that lists all the configurable options for the library and show what the default values are, so this option would need to be there and match the python None value. Let's add it so that it translates to None if used.
There was a problem hiding this comment.
with json.loads(path) null translates to None
https://docs.python.org/3/library/json.html#encoders-and-decoders
| @@ -94,3 +96,8 @@ def _send_request_helper(self, url, accepted_status_codes, function, data, retry | |||
| raise HttpClientException(u"Invalid status code '{}' from {} with error payload: {}" | |||
| .format(status, url, text)) | |||
| return r | |||
|
|
|||
| def _set_custom_certfiles_path(self): | |||
| if conf.custom_certfiles_path is not None: | |||
There was a problem hiding this comment.
conf.custom_certfiles_path is not a method call. This needs to invoke the method to retrieve the value of the configuration. So it would change to conf.custom_certfiles_path(). That's what I meant with my previous comment 😃
There was a problem hiding this comment.
So just put this method in line 37, like you had it before, but make sure to actually retrieve the value of the configuration. Does that make sense?
|
|
||
| @with_setup(_setup) | ||
| def test_custom_certfiles_path(): | ||
| overrides = { } |
There was a problem hiding this comment.
There's no need for this test.
| @@ -232,3 +232,4 @@ def test_kerberos_auth_check_auth(): | |||
| client = ReliableHttpClient(endpoint, {}, retry_policy) | |||
| assert_is_not_none(client._auth) | |||
| assert isinstance(client._auth, HTTPKerberosAuth) | |||
|
|
|||
There was a problem hiding this comment.
Adding a test here to see that the verify parameter in the request is honoring the value of conf.custom_certfiles_path() when it's set would be useful and would have caught the lack of method call I mentioned in the other file. Let's add it.
|
Hi @polomarcus! I came across this thread and thought your PR can be helpful. Do you still plan to complete this PR? |
Regarding #404
Using verify to pass the certificate path as shown here
Configure it using
custom_certfiles_path