-
Couldn't load subscription status.
- Fork 79
HTTPS #1581
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
HTTPS #1581
Changes from 10 commits
c716574
28f9697
15a6ee7
7dbac08
e7b541f
c9a3da0
f60a98d
8739635
18fe70c
1337746
d6f63dd
0b848d8
e28153b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIDVjCCAj4CCQCP4XnDqToF2zANBgkqhkiG9w0BAQUFADBtMQswCQYDVQQGEwJV | ||
| UzETMBEGA1UECBMKQ2FsaWZvcm5pYTESMBAGA1UEBxMJU2FuIERpZWdvMQ0wCwYD | ||
| VQQKEwRVQ1NEMRIwEAYDVQQLEwlLbmlnaHRMYWIxEjAQBgNVBAMTCWxvY2FsaG9z | ||
| dDAeFw0xNTEyMTgyMjE3MzBaFw0xNjEyMTcyMjE3MzBaMG0xCzAJBgNVBAYTAlVT | ||
| MRMwEQYDVQQIEwpDYWxpZm9ybmlhMRIwEAYDVQQHEwlTYW4gRGllZ28xDTALBgNV | ||
| BAoTBFVDU0QxEjAQBgNVBAsTCUtuaWdodExhYjESMBAGA1UEAxMJbG9jYWxob3N0 | ||
| MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAt1ggW4M/l3Wpru4+2Cro | ||
| nnqaUWD0ImLnkdAbmDjhGiCdKqdb8yzLeKipGaRY383gd5vMWHsKB1I3t+EzFWiY | ||
| fxd12Evx6MUIXVZSkdConk+8xlmJ5ba1Hgy7qzErY7+HOtgqm1ylyqTuOZyv3Umv | ||
| 0W6ETLVz/alfzxTlqAkvuJn7I7RrbY81I3b5SOUxJTtj9pPwkZtVOD0ha3FH0LBu | ||
| lE4oi6rQQhzIbUDWLITZRCteplV5ikbC3JqaJ7pDiYnOIPnRR0UF+xdyTiOvSNH8 | ||
| WrKuAdGGN+90PDt8fgQOwptE5l/RGyoJ2on7nlSj5crDtYzXXDYw0DCzuFG12nZV | ||
| FwIDAQABMA0GCSqGSIb3DQEBBQUAA4IBAQBTQJ8WYpSfsXsgmDa2uIYX5E+8ECGn | ||
| patQJuxYfOEp9knnBBe+QcaBMY6E7uH6EZz2QwS/gdhfY8e8QXw9sh9ZrQKQlIAK | ||
| Q5l5qxAtek0C90qdseYWoomBhpmqMUicF0OgecbdZ4X6Tfc4hvN5IXUTMn9ZJEaV | ||
| fduah3c7xEkSbHQl6iHnJswNKTc7Amm+BIwuYJjCZxVgKxAgvYzzg/TFU03gqzfE | ||
| h7ARs1p4WdHH+WTMqCZq8+sju3Lum4uwjYaiLaFE7psDkWWAYOu6Jv/o0V1zER/S | ||
| LzNaDfkm5kq4VURhPMQzdAiVdiTNKDFnLB3erg6wG95q5OiGNO1WYSw2 | ||
| -----END CERTIFICATE----- |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| -----BEGIN CERTIFICATE REQUEST----- | ||
| MIICsjCCAZoCAQAwbTELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWEx | ||
| EjAQBgNVBAcTCVNhbiBEaWVnbzENMAsGA1UEChMEVUNTRDESMBAGA1UECxMJS25p | ||
| Z2h0TGFiMRIwEAYDVQQDEwlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IB | ||
| DwAwggEKAoIBAQC3WCBbgz+Xdamu7j7YKuieeppRYPQiYueR0BuYOOEaIJ0qp1vz | ||
| LMt4qKkZpFjfzeB3m8xYewoHUje34TMVaJh/F3XYS/HoxQhdVlKR0KieT7zGWYnl | ||
| trUeDLurMStjv4c62CqbXKXKpO45nK/dSa/RboRMtXP9qV/PFOWoCS+4mfsjtGtt | ||
| jzUjdvlI5TElO2P2k/CRm1U4PSFrcUfQsG6UTiiLqtBCHMhtQNYshNlEK16mVXmK | ||
| RsLcmponukOJic4g+dFHRQX7F3JOI69I0fxasq4B0YY373Q8O3x+BA7Cm0TmX9Eb | ||
| KgnaifueVKPlysO1jNdcNjDQMLO4UbXadlUXAgMBAAGgADANBgkqhkiG9w0BAQUF | ||
| AAOCAQEAJD/s9Z150miwTYW08DklPoCjR1NijtFBUUkh176GCljNFe+rhnn2OLiV | ||
| skunJ4EiVAZ8O3bjgxGc53FnEMVrwisFugvnORTJAZpp7tKxBqpP++gqXf79wElN | ||
| ySTLjg9vFQgDjUxM2gO/8lH12HBw9xUDHFN2hOZ9ozPFf7KZUa6KUzh2AgHJ4UCF | ||
| 9kFceNAvUamAHjv7BwDqoakLTuEBSZOycimDKiW8aBFGYCHFO2NMCzAKyVxmLtkB | ||
| K7+e5iWwvy3LewiPX/eK+jtRVwY+ujcvOZeCv6Th+oQv1BxstpJwOlca+5s1Rdke | ||
| kUu++nvrQUvWSsdQzxZq855r2TBA/w== | ||
| -----END CERTIFICATE REQUEST----- |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| -----BEGIN RSA PRIVATE KEY----- | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have these been tested when serving from other systems? I want to make sure they don't trigger HTTPS error pages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I try and access any page on the system, I get hte following error: WARNING:tornado.general:SSL Error on 61 ('::1', 58512, 0, 0): [SSL: HTTP_REQUEST] http request (_ssl.c:590)So these may not be good universally. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't test this in another system - that is what I exposed in the PR comments. The error that you're seeing is because you are using http rather than https. Make sure to type https - in the live system the redirect is done through nginx, but in your local machine you need to type the https. Also make sure that your config file includes the https in the base_url, so when redirects happen in the system it works as expected. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, and I now get the error I was expecting from chrome: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you post a screenshot of the error rather than copy paste? I would like to see better the error from chrome. Thanks, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @biocore/qiita-dev can I have some comments here? The only alternative then is to ask each of our users to ask for a certificate to an authority - which can take some time and I don't think this is what we need to do. Without this, we can't use https neither oauth2. I think that we good documentation this is not a problem - and we let the user know that is not a problem. @squirrelo If you don't agree with this, please provide an alternative and constructive feedback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should really default back to HTTP if we don't have a certificate provided, possibly with a warning when you start the webserver. This allows users to use standard HTTP when it's on their laptop and safe, and HTTPS in larger deploys. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This issue is a complicated one, on the one hand we want to maintain the installation process as simple as possible, but on the other hand we also want to have a consistent system that isn't raising flags and worrying our users. It almost feels like these two are incompatible, so these are the possible partial solutions I can think of:
The solution should probably be a combination of some of the points above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just discussed some of this information with Jeff. Few points:
About the comment of defaulting to HTTP: the entire work to use HTTPS has beed done so the RESTapi/plugins work as expected communicating through OAuth2 - if we default to HTTP, we can't use any of the plugins as they're authenticating through OAuth2, which requires HTTPS. My vote goes top add detailed documentation in which we specify that if you are running locally in your machine, you will receive that warning the first time that you log in to the server (per @squirrelo 's observation, maybe every time that you start the server - I did not have that problem). Then, also add documentation that if you want to make your own Qiita server, you should get your own valid certificates from a Certificate Authority. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the solution is to improve documentation and/or try to do some of these suggestions: http://superuser.com/questions/772762/how-can-i-disable-security-checks-for-localhost |
||
| MIIEogIBAAKCAQEAt1ggW4M/l3Wpru4+2CronnqaUWD0ImLnkdAbmDjhGiCdKqdb | ||
| 8yzLeKipGaRY383gd5vMWHsKB1I3t+EzFWiYfxd12Evx6MUIXVZSkdConk+8xlmJ | ||
| 5ba1Hgy7qzErY7+HOtgqm1ylyqTuOZyv3Umv0W6ETLVz/alfzxTlqAkvuJn7I7Rr | ||
| bY81I3b5SOUxJTtj9pPwkZtVOD0ha3FH0LBulE4oi6rQQhzIbUDWLITZRCteplV5 | ||
| ikbC3JqaJ7pDiYnOIPnRR0UF+xdyTiOvSNH8WrKuAdGGN+90PDt8fgQOwptE5l/R | ||
| GyoJ2on7nlSj5crDtYzXXDYw0DCzuFG12nZVFwIDAQABAoIBAAWPwsuD27X4d8E6 | ||
| sOVVx+Cmz2+QhVdVsLiXm36umwVz39CfawahYTqzww1oEtoXIKwAKd61PMSkFdSa | ||
| HDaWCaoxR/3br+bjszFBoReTQMosmQlbRfeV50CWGktDHpHUCYIbdI9p7A2Nmzxc | ||
| KGmKBq9Pruu0xWP4c1OQyraiMLRkyjdPxV42t/cpolzkk+zt94om2DdmaZHgrwfM | ||
| TIC/PkgINlwIQfATFSaZSeTQ3bhfMzlLLaiDicf/ZUF8HnIToQjXwfApLv9/2DQB | ||
| BBjFXVXPODYADG0ddczX4GflO4cdyKeJALooQ2v1R7+F7XbTCHGSewFQsYN1cebP | ||
| Shnw4iECgYEA3t2wik92tLDD9yTw3Gl7KcoafW7yVMVjY7DrQqx5VLNN6H2ToIw0 | ||
| 1eaWx1xvabh8F4GyZ6hLqQJs2yOvuBcdEgvjs7rEAktT9w60MNQHP/9LxC7AloUR | ||
| QCpcp4kgf6QuYNBH5eeDMPXt/AyItlTBu1tLWvIyArZDFyFpQ1R+UFUCgYEA0po7 | ||
| 78yzQ2zTxxg+Zg1oFhojo8eqbBCxcnaeMMobaUvRlYdQn3FEgYEzAi39ajw+ApKO | ||
| megCBEc58mlWRaCEBzmOSdttCL/hZR5QD3tMyE97xZxu4qvty7mS8+Qx7VXi4E+R | ||
| 9iB3QW2IdnO0ny0LL+/FRlseFd9Xi48HPEfGC7sCgYBZ4bC0Z03pm6y8/PIDAquA | ||
| dBjw45DqLV1jNFP9ZM/eIB5YmGS0iDc62S+QIbBxQ80QMeeAtLgm5mp+isrwhKnd | ||
| wFvJ/ZXVkwm/BvgXBTwTTGQDMBPN4MbPSQZrLvujXtSxiO6mQxN+6IBdebSdTFuB | ||
| s1UVzb3rqBQwvPHWDLqQrQKBgEQoemFfZFzb6aYzqG4qxJbymro0smkK9gLPdN7h | ||
| agGn8gpsoqEXbU5kW/fGno81M/RtLg2NUqnDy3MyuCMMke4sqGoqw5zjTLUHHG4G | ||
| P5FMeAg6WoCtl+YpBvA72BSxiR2DYv++/hmEs3dydvBVCZvYA4aDkrcNcy3Nek/+ | ||
| ApBlAoGAQOXrFJa0kMKK89D6luYd7KWTHze7fSNmohxXKm4OW/9V/IxcDFASRasm | ||
| Ld45yQ6CRk42Ry0xA/TiEs9Gv8hPrJEzNXVzBMA+s0h6uY13p5cMgZdMA7XotM95 | ||
| UgtsatiprRqzpYQItbPCYyPmDAscA+jsdoZzbXFjlOYz/9BZRCc= | ||
| -----END RSA PRIVATE KEY----- | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| # ----------------------------------------------------------------------------- | ||
| # Copyright (c) 2014--, The Qiita Development Team. | ||
| # | ||
| # Distributed under the terms of the BSD 3-clause License. | ||
| # | ||
| # The full license is in the file LICENSE, distributed with this software. | ||
| # ----------------------------------------------------------------------------- | ||
|
|
||
| from os.path import join, dirname, abspath | ||
| from os import environ | ||
| from future import standard_library | ||
|
|
||
| import requests | ||
|
|
||
| with standard_library.hooks(): | ||
| from configparser import ConfigParser | ||
|
|
||
|
|
||
| class QiitaClient(object): | ||
| """Client of the Qiita RESTapi | ||
|
|
||
| Parameters | ||
| ---------- | ||
| server_url : str | ||
| The url of the Qiita server | ||
|
|
||
| Methods | ||
| ------- | ||
| get | ||
| post | ||
| """ | ||
| def __init__(self, server_url): | ||
| self._server_url = server_url | ||
|
|
||
| try: | ||
| conf_fp = environ['QP_TARGET_GENE_CONFIG_FP'] | ||
| except KeyError: | ||
| conf_fp = join(dirname(abspath(__file__)), 'support_files', | ||
| 'config_file.cfg') | ||
|
|
||
| config = ConfigParser() | ||
| with open(conf_fp, 'U') as conf_file: | ||
| config.readfp(conf_file) | ||
|
|
||
| server_cert = config.get('main', 'SERVER_CERT') | ||
| if not server_cert: | ||
| # The server certificate is not provided, use standard certificate | ||
| # verification methods | ||
| self._verify = True | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the looks of this, _verify can be bool or string type and hold True or a filepath. This is confusing and will be hard to maintain, especially since both are "truthy" for if statements. Please decompose this better so we are not mixing types in the same variable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I hate this - however this is a limitation from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is pretty weird. If we do want to keep this, this needs a lot more documentation as to what's going on, including a link to that documentation. |
||
| else: | ||
| # The server certificate is provided, use it to verify the identity | ||
| # of the server | ||
| self._verify = server_cert | ||
|
|
||
| def _request_retry(self, req, url, **kwargs): | ||
| """Executes a request retrying it 3 times in case of failure | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should 3 be a parameter in the config file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure. This functionality is hard to test. In case that the communication fails I'm just re-trying such communication up to 3 times, in case that something weird happened. Do you have strong feelings on moving this to a config file? (note that this was like that in the previous version but in a different function) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No strong feelings, just that trying 3 times seems kind of random when you can try once or twice. If nobody else has an issue with this it is fine to leave as is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add documentation in the notes section why 3 specifically. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed to 2 and added documentation on why. |
||
|
|
||
| Parameters | ||
| ---------- | ||
| req : function | ||
| The request to execute | ||
| url : str | ||
| The url to access in the server | ||
| kwargs : dict | ||
| The request kwargs | ||
|
|
||
| Returns | ||
| ------- | ||
| dict | ||
| The JSON information in the request reply | ||
| """ | ||
| url = self._server_url + url | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this use path join? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree as those are not paths and, although we don't support it, on windows it will not work. |
||
| retries = 3 | ||
| json_reply = None | ||
| while retries > 0: | ||
| retries -= 1 | ||
| r = req(url, verify=self._verify, **kwargs) | ||
| r.close() | ||
| if r.status_code == 200: | ||
| json_reply = r.json() | ||
| break | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This break does the trick for success ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I guess I no longer need the success boolean. I think this was a bit of leftover code when I ported from the previous function. |
||
| return json_reply | ||
|
|
||
| def get(self, url, **kwargs): | ||
| """Execute a get against the Qiita server | ||
|
|
||
| Parameters | ||
| ---------- | ||
| url : str | ||
| The url to access in the server | ||
| kwargs : dict | ||
| The request kwargs | ||
|
|
||
| Returns | ||
| ------- | ||
| dict | ||
| The JSON response from the server | ||
| """ | ||
| return self._request_retry(requests.get, url, **kwargs) | ||
|
|
||
| def post(self, url, **kwargs): | ||
| """Execute a post against the Qiita server | ||
|
|
||
| Parameters | ||
| ---------- | ||
| url : str | ||
| The url to access in the server | ||
| kwargs : dict | ||
| The request kwargs | ||
|
|
||
| Returns | ||
| ------- | ||
| dict | ||
| The JSON response from the server | ||
| """ | ||
| return self._request_retry(requests.post, url, **kwargs) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you state these must be filepaths and not the certificate text itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done