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

Jump to bottom

Changed the default protocol #4531

Merged
merged 3 commits into from
Apr 4, 2019
Merged

Changed the default protocol #4531

merged 3 commits into from
Apr 4, 2019

Conversation

xmnlab
Copy link
Contributor

@xmnlab xmnlab commented Mar 30, 2019

Resolves #4427

Changed PROTOCOL_TLSv1 to PROTOCOL_TLS

@takluyver
Copy link
Member

The idea seems good, but the name is new in Python 3.6, and we're still supporting 3.5 at present. Can you include some compatibility code so it uses PROTOCOL_SSLv23 if PROTOCOL_TLS is not found?

@xmnlab
Copy link
Contributor Author

xmnlab commented Apr 1, 2019

thanks @takluyver I did the changes you suggested. it is done for a review.

takluyver
takluyver reviewed Apr 1, 2019
View reviewed changes
notebook/notebookapp.py Outdated
ssl_options.setdefault('ssl_version', ssl.PROTOCOL_TLSv1)
ssl_options.setdefault(
'ssl_version',
ssl.PROTOCOL_TLS if hasattr(ssl, PROTOCOL_TLS) else ssl.PROTOCOL_SSLv23
Copy link
Member

Choose a reason for hiding this comment

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

hasattr() takes a string as the second argument. Though it might be neater to use getattr() with a default.

Copy link
Contributor Author

@xmnlab xmnlab Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

hasattr() takes a string as the second argument.

you're right. thanks for catch that up. I fixed that it is done for review.

Though it might be neater to use getattr() with a default.

sorry I didn't understand how to use this default

Copy link
Member

Choose a reason for hiding this comment

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

Like getattr(ssl, 'PROTOCOL_TLS', ssl.PROTOCOL_SSLv23). But the hasattr version is fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I see ... that is much better thanks. I forgot getattr has default argument. thanks. I will change that.

@xmnlab
Changed the default protocol
c5264b6 
Applied suggestion from review

Fixed hasattr issue
takluyver
takluyver reviewed Apr 3, 2019
View reviewed changes
notebook/notebookapp.py Outdated
@@ -1421,7 +1421,11 @@ def init_webapp(self):
# SSL may be missing, so only import it if it's to be used
import ssl
# Disable SSLv3 by default, since its use is discouraged.
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this accompanying comment to describe what we're now using and why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure I am going to do that now

@xmnlab
Copy link
Contributor Author

xmnlab commented Apr 3, 2019

@takluyver I am reviewing the documentations here and python35 already supports ssl.PROTOCOL_TLS and PROTOCOL_SSLv23 is deprecated.

ssl.PROTOCOL_TLS
Selects the highest protocol version that both the client and server support. Despite the name, this option can select “TLS” protocols as well as “SSL”.

New in version 3.5.3.

ssl.PROTOCOL_SSLv23
Alias for data:PROTOCOL_TLS.

Deprecated since version 3.5.3: Use data:PROTOCOL_TLS instead.

and as I could find here python 3.5.5 is available on conda-forge and pkgs/main

I will keep PROTOCOL_SSLv23 (because maybe it should need a pinning on the libraries that depend on notebook) but if you think it is ok to remove that, let me know.

@takluyver
Copy link
Member

Ah, OK. As it's pretty easy to fall back to PROTOCOL_SSLv23, let's leave that in for now just in case anyone has an older Python 3.5 for some reason. It would be good to mention in the comment what version needs that, so it's clear when the code can be simplified again.

takluyver
takluyver reviewed Apr 3, 2019
View reviewed changes
notebook/notebookapp.py Outdated
# server support. When PROTOCOL_TLS is not available use PROTOCOL_SSLv23
ssl_options.setdefault(
'ssl_version',
getattr('ssl', 'PROTOCOL_TLS', ssl.PROTOCOL_SSLv23)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, the first parameter for getattr should be the module object (i.e. ssl with no quotes). It's only the attribute name that needs to be a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're totally right. sorry about that. I am still trying to have a functional dev environment here.
that is why I am pushing things without tested locally. sorry again T_T

@takluyver
Copy link
Member

Thanks, that looks good now. I'm about to head out, but I'll merge this tomorrow assuming the CI passes (I suspect this code path isn't tested, so...)

@xmnlab
Copy link
Contributor Author

xmnlab commented Apr 3, 2019

thank you so much @takluyver ! yes it seems it is not tested ...

@takluyver takluyver added this to the 5.8 milestone Apr 4, 2019
@takluyver takluyver merged commit 1a6fc8e into jupyter:master Apr 4, 2019
@takluyver
Copy link
Member

Thanks for your patience on getting this in.

@xmnlab
Copy link
Contributor Author

xmnlab commented Apr 4, 2019

Thanks @takluyver for your patience and your guidance :)

@xmnlab xmnlab deleted the change-default-ssl branch April 4, 2019 13:53
This was referenced Sep 24, 2019
Closed
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@takluyver takluyver takluyver left review comments

Assignees
No one assigned
Labels
status:resolved-locked
Projects
None yet
Milestone
5.8
Development

Successfully merging this pull request may close these issues.

SSL Version
2 participants
@xmnlab @takluyver