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.

Sign up for GitHub

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

SSL with custom certfiles path #435

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

SSL with custom certfiles path #435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6cf4bd4
feat: #404 SSL with custom certfiles path
polomarcus Jan 31, 2018
c2fc660
refacto: use a method to set SSL certificate
polomarcus Feb 1, 2018
c525596
refacto: use another certificat format for example
polomarcus Feb 1, 2018
46e7125
doc: custom_certfiles_path to null for example
polomarcus Feb 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sparkmagic/example_config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": null,

"session_configs": {
"driverMemory": "1000M",
Expand Down
7 changes: 7 additions & 0 deletions sparkmagic/sparkmagic/livyclientlib/reliablehttpclient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ 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:
self._set_custom_certfiles_path()

def get_headers(self):
return self._headers
Expand Down Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

self.logger.debug(u"Using a custom SSL certificate")
self.verify_ssl = conf.custom_certfiles_path
10 changes: 10 additions & 0 deletions sparkmagic/sparkmagic/tests/test_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,13 @@ def test_share_config_between_pyspark_and_pyspark3():
kpc = { 'username': 'U', 'password': 'P', 'base64_password': 'cGFzc3dvcmQ=', 'url': 'L', 'auth': AUTH_BASIC }
overrides = { conf.kernel_python_credentials.__name__: kpc }
assert_equals(conf.base64_kernel_python3_credentials(), conf.base64_kernel_python_credentials())

@with_setup(_setup)
def test_custom_certfiles_path():
overrides = { }
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for this test.

conf.override_all(overrides)
assert_equals(conf.custom_certfiles_path(), None)
cert = "test.pem"
overrides = { conf.custom_certfiles_path.__name__: cert }
conf.override_all(overrides)
assert_equals(conf.custom_certfiles_path(), cert)
1 change: 1 addition & 0 deletions sparkmagic/sparkmagic/tests/test_reliablehttpclient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

3 changes: 3 additions & 0 deletions sparkmagic/sparkmagic/utils/configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,9 @@ def resource_limit_mitigation_suggestion():
def ignore_ssl_errors():
return False

@_with_override
def custom_certfiles_path():
return None
Copy link
Author

Choose a reason for hiding this comment

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

@aggFTW this is how we handle None value for custom_certfiles_path , so I'm not sure what to do for example_config.json

Copy link
Contributor

Choose a reason for hiding this comment

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

If you did:

"custom_certfiles_path": null,

would it end up returning None?

I think it would. Maybe try that out.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

with json.loads(path) null translates to None

https://docs.python.org/3/library/json.html#encoders-and-decoders


@_with_override
def coerce_dataframe():
Expand Down