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.

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

add support for HTTPS protocol #307

Merged
merged 12 commits into from Dec 10, 2016
Merged

add support for HTTPS protocol #307

merged 12 commits into from Dec 10, 2016

Conversation

pigay
Copy link
Contributor

@pigay pigay commented Dec 5, 2016

Proof of concept for #303

  • Allows to pass arguments to tornado.HTTPServer constructor through app.create_server.
  • support for HTTPS protocol

Enable SSL transport by calling :

args = {}
args['ssl_options'] = {'certfile' : 'path-to-crtfile', 'keyfile' : 'path-to-keyfile'}
app.create_server(**args)

app.serve(Example, 'Example')
app.run()

@almarklein
Copy link
Member

Thanks!

I think it would be nice to document this feature. I think the best place might be to add a few lines to the docstring of create_server() on how server_kwargs can be used to enable SSL.

Is it also possible to add an example script to demo this? Not sure what cert and keyfile to point to though. What do you think?

I see a few more places that use http:

  • The url prop in app/_app.py
  • init_interactive() in app/_funcs.py

@almarklein
Copy link
Member

Travis fails because of style errors. You can run the style test locally by running python make test style in the root flexx directory.

@Korijn
Copy link

Korijn commented Dec 6, 2016

Should the port be set to 443 automatically if port=None for the https protocol? I'm not sure if it will work on other ports.

@pigay
Copy link
Contributor Author

pigay commented Dec 6, 2016

@Korijn from my tests, it works fine for default port (49190)

@alexveden
Copy link
Contributor

@pigay thx for your work. Have you added new config setting handling for HTTPS connections?

@pigay
Copy link
Contributor Author

pigay commented Dec 7, 2016

This error (https://travis-ci.org/zoofIO/flexx/jobs/181720106) seems to happen in python2.

I don't know much about python testing. How can I debug this error?

@pigay
Copy link
Contributor Author

pigay commented Dec 7, 2016

@alexveden not yet. I was thinking of it, though.

However, this configuration is specific to Tornado. Should I add tornado_certfile and tornado_keyfile in flexx._config.py?

@alexveden
Copy link
Contributor

@pigay It would be great, because the problem is also related to remote Ipython notebooks too #299

@alexveden
Copy link
Contributor

alexveden commented Dec 8, 2016

@pigay Am I correct, do we need both key and cert to connect SSL, but according your last commit ccfa4b1 it's allowed to add one of those files.

@pigay
Copy link
Contributor Author

pigay commented Dec 8, 2016

@alexveden I think so. I found easier to handle each file independently and error messages from tornado should still be meaningful.

@pigay
Copy link
Contributor Author

pigay commented Dec 8, 2016

A question: I'm not really fluent with Github. Do I need to create another PR after rebase to solve the merge conflict?

@almarklein
Copy link
Member

@pigay you can create a new branch if you prefer, but it it nor necessary. On a merge conflict in a PR you can either merge your branch with master, or rebase it on master. Preference depends on the project, I slightly prefer the latter:

git checkout master
git pull
git checkout your-branch
git rebase master
... resolve conflicts
git add ... files that were in conflict
git rebase --continue
git push your-remote your-branch -f

The -f force-pushes, i.e. overwrites history. Never ever use that on a master branch :P

@pigay
Copy link
Contributor Author

pigay commented Dec 8, 2016

@almarklein the -f option was what I was missing. Thanks.

@alexveden
Copy link
Contributor

@almarklein how long could it take to merge this pull request to the master? Thx

Copy link
Member

@almarklein almarklein left a comment

Choose a reason for hiding this comment

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

This is great! Just made a few small remarks.



# handle ssl, wether from configuration or given args
if 'ssl_certfile' in config and config.ssl_certfile:
Copy link
Member

Choose a reason for hiding this comment

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

That first test ssl_certfile' in config should not be necessary

app.run()

Alternately, cert and key files can be provided through
``ssl_certfile`` and ``ssl_keyfile`` configuration variables.
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of setting ssl options via the config. In fact, I think we should advocate that as the way to do it. We can keep the server_kwargs, but can you please please remove the example in this docstring (I know I asked you to add it earlier, sorry!).

# configure web server for SSL
app.create_server(ssl_options={'certfile' : CERTFILE,
'keyfile' : KEYFILE})

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 please use flexx.config here instead? I think that should be the approach to put forward as the way to enable https.

KEYFILE = '/tmp/self-signed.key'

os.system('openssl req -x509 -nodes -days 1 -batch -newkey rsa:2048 '
'-keyout %s -out %s' % (KEYFILE, CERTFILE))
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, if Flexx ever supports a web framework alternative to Tornado, would the same key and cert files be usable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as this new web framework uses a certificate and key files, it should be fine. Since tornado's ssl_options is built from flex config variables id done in _tornadoserver.py, you would just need to use the same variables in the new server _open method.

@almarklein almarklein merged commit 1fa5b58 into flexxui:master Dec 10, 2016
@almarklein
Copy link
Member

Thanks @pigay !

@pigay
Copy link
Contributor Author

pigay commented Dec 11, 2016

My pleasure!

@pigay pigay deleted the ssl branch January 3, 2017 16:10
@almarklein almarklein added this to the v0.4.2 milestone Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants