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

Assert HTTPS through redirects on client request (GET, POST, ...) #3416

Open
almeida-raphael opened this issue Nov 30, 2018 · 29 comments
Open

Comments

@almeida-raphael
Copy link

I need to send some critical data on a post request, but the URL is given by a client, so I need to ensure that the data will be sent under SSL. I check the URL for https schema, but if the host send me 301 and redirect to an HTTP website my data will be sent through an insecure channel.

I think it would be nice if I could set "ensure_https" or "ensure_ssl" on a post request for example. If the host redirected me from https to http and "ensure_https" was set, it should raise an exception.

@aio-libs-bot
Copy link

GitMate.io thinks the contributor most likely able to help you is @asvetlov.

Possibly related issues are #3390 (redirects?), #1557 (POST request always follow redirects), #141 (body for HTTP GET request), #3082 ([client] 301 in POST gets redirected to GET), and #852 (Http client HEAD request failed to release).

@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2018

You can achieve the same result right now without waiting for a new feature.
Just disable automatic redirection (allow_redirects=False) and process 301 responses on your own.

@yjqiang
Copy link

yjqiang commented Dec 1, 2018

@asvetlov I think you can delete the labels since it is not a bug or enhancement issue.

@asvetlov asvetlov removed the bug label Dec 1, 2018
@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2018

Well, it is not a bug but a possible request for an enhancement.

@webknjaz
Copy link
Member

webknjaz commented Dec 1, 2018

I think it's impossible to automatically ensure such case. Web server can be doing such redirects conditionally and even if we'll send pre-flight HEAD request it doesn't guarantee that the server's behavior will be redirecting to https. There's no way to check this without sending an actual request with actual data.
OTOH if the client changes http to https before sending a request and disables redirect it could give some level of projection.

I'm not sure whether this feature should be in the framework but if yes, I'd add a flag as forbid_insecure or forbid_http probably. Insecure version is better because it has a potential to be extended to non-http protocols. Like ws vs. wss.

@yjqiang
Copy link

yjqiang commented Dec 1, 2018

I think it's impossible to automatically ensure such case. Web server can be doing such redirects conditionally and even if we'll send pre-flight HEAD request it doesn't guarantee that the server's behavior will be redirecting to https. There's no way to check this without sending an actual request with actual data.
OTOH if the client changes http to https before sending a request and disables redirect it could give some level of projection.

I'm not sure whether this feature should be in the framework but if yes, I'd add a flag as forbid_insecure or forbid_http probably. Insecure version is better because it has a potential to be extended to non-http protocols. Like ws vs. wss.

Yeah. Just like https://www.chromium.org/hsts, and it's different from 301.

@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2018

In other words, aiohttp client should raise an exception if secure mode is requested and the connection is not protected.

Any champion for the feature?

@HeavenVolkoff
Copy link

HeavenVolkoff commented Dec 1, 2018

Hello. I also have an interest in this feature and can attempt to implement it.

I think it's impossible to automatically ensure such case. Web server can be doing such redirects conditionally and even if we'll send pre-flight HEAD request it doesn't guarantee that the server's behavior will be redirecting to https. There's no way to check this without sending an actual request with actual data.
OTOH if the client changes http to https before sending a request and disables redirect it could give some level of projection.

I'm not sure whether this feature should be in the framework but if yes, I'd add a flag as forbid_insecure or forbid_http probably. Insecure version is better because it has a potential to be extended to non-http protocols. Like ws vs. wss.

I may be oversimplifying the issue, but restricting the schemes accepted by the r_url should solve this, no?

aiohttp/aiohttp/client.py

Lines 479 to 481 in 66d1f9c

scheme = r_url.scheme
if scheme not in ('http', 'https', ''):
resp.close()

What would be the necessity for a pre-flight HEAD request?

@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2018

I suggest checking conn.transtort.get_extra_info('sslcontext') in ClientSession._request() / ClientSession._ws_connect() instead, it is stricter and cleaner flag for secured TSL connection.

@HeavenVolkoff
Copy link

I suggest checking conn.transtort.get_extra_info('sslcontext') in ClientSession._request() / ClientSession._ws_connect() instead, it is stricter and cleaner flag for secured TSL connection.

Something like this?

aiohttp/aiohttp/client.py

Lines 402 to 404 in 66d1f9c

assert conn.transport is not None
tcp_nodelay(conn.transport, True)
tcp_cork(conn.transport, False)

if redirects > 0 and forbid_insecure and conn.transport.get_extra_info("sslcontext") is None:
  raise InsecureRedirect()

@asvetlov
Copy link
Member

asvetlov commented Dec 2, 2018

Yes, but without redirects > 0.
If a user wants to enforce TLS -- it should be checked regardless HTTP redirection.

Minor neat: forbid_insecure is a little long name. I have no strong opinion but enforce_tls is shorter. Does everybody know what TLS means? Maybe we can find something better?

@yjqiang
Copy link

yjqiang commented Dec 2, 2018

Yes, but without redirects > 0.
If a user wants to enforce TLS -- it should be checked regardless HTTP redirection.

Minor neat: forbid_insecure is a little long name. I have no strong opinion but enforce_tls is shorter. Does everybody know what TLS means? Maybe we can find something better?

https://tools.ietf.org/html/rfc6797
How about hsts(HTTP Strict Transport Security)? But with hsts, aiohttp should refuse the http requests while users enforce this. Just like hsts on chrome.

@asvetlov
Copy link
Member

asvetlov commented Dec 2, 2018

@yjqiang please elaborate

@yjqiang
Copy link

yjqiang commented Dec 2, 2018

@yjqiang please elaborate

Ok. Edited.

@HeavenVolkoff
Copy link

HeavenVolkoff commented Dec 2, 2018

Yes, but without redirects > 0.
If a user wants to enforce TLS -- it should be checked regardless HTTP redirection.

Fair point

https://tools.ietf.org/html/rfc6797
How about hsts(HTTP Strict Transport Security)? But with hsts, aiohttp should refuse the http requests while users enforce this. Just like hsts on chrome.

I support the idea of implementing hsts...

Would you guys mind if I open a pull request with the mentioned changes?

@yjqiang
Copy link

yjqiang commented Dec 2, 2018

@HeavenVolkoff "The policy is declared by websites via the Strict-Transport-Security HTTP response header field and/or by other means, such as user agent configuration, for example." So maybe we need to handle it when the server returns hsts response?

@HeavenVolkoff
Copy link

HeavenVolkoff commented Dec 2, 2018

I think implementing HSTS spec would be an important security feature to aiohttp. However, after understanding it a bit better, I do not see it as being a solution for this feature request. If what I understood from @Raphael-C-Almeida request is correct. What he (and I, for that matter) want is some form of enforcement to prevent cases when the webserver, being contacted through tls, sends a redirect status code to another insecure URL (eg. http://foo.bar). HSTS would not prevent this as it protects only against insecure access of secure-only domains and sub-domains that could lead to data leak or become a phishing entrypoint.

@Raphael-C-Almeida is my understanding of your request correct?

@yjqiang
Copy link

yjqiang commented Dec 2, 2018

I think implementing HSTS spec would be an important security feature to aiohttp. However, after understanding it a bit better, I do not see it as being a solution for this feature request. If what I understood from @Raphael-C-Almeida request is correct. What he (and me, for that matter) want is some form of enforcement to prevent cases when the webserver, being contacted throught tls, sends a redirect status code to another insecure URL (eg. http://foo.bar). HSTS would not prevent this as it protects only against insecure access of secure-only domains and sub-domains that could lead to data leak or become a phishing entrypoint.

@Raphael-C-Almeida is my understanding of your request correct?

You are right. HSTS only supports when we are at the same domain. And the feature request from @Raphael-C-Almeida is that we should keep tls while redirecting.(eg: url_from_google_result -> actual_url)

@yjqiang
Copy link

yjqiang commented Dec 2, 2018

Yes, but without redirects > 0.
If a user wants to enforce TLS -- it should be checked regardless HTTP redirection.

Minor neat: forbid_insecure is a little long name. I have no strong opinion but enforce_tls is shorter. Does everybody know what TLS means? Maybe we can find something better?

Maybe "tls_redirects" is more readable? We ensure that during redirections, every point is url with https.

@asvetlov
Copy link
Member

asvetlov commented Dec 2, 2018

  1. HSTS is nice to have feature but it is a different beast: TLS enforcement is negotiated by a server side. If you want to add it -- please create a new issue.
  2. In my vision, the discussed option should restrict non-TLS usage for all client connections, not only redirects from a server.

@almeida-raphael
Copy link
Author

almeida-raphael commented Dec 2, 2018

I think implementing HSTS spec would be an important security feature to aiohttp. However, after understanding it a bit better, I do not see it as being a solution for this feature request. If what I understood from @Raphael-C-Almeida request is correct. What he (and me, for that matter) want is some form of enforcement to prevent cases when the webserver, being contacted throught tls, sends a redirect status code to another insecure URL (eg. http://foo.bar). HSTS would not prevent this as it protects only against insecure access of secure-only domains and sub-domains that could lead to data leak or become a phishing entrypoint.

@Raphael-C-Almeida is my understanding of your request correct?

Yes @HeavenVolkoff this is what I meant. It's not exactly HTST.

About the name, @yjqiang, I think the aiohttp user should be able to restrict the connection to a secure one even for in case where no redirects happen, so I think that something like forbid_insecure is better than anything that contains redirect in the name.

@asvetlov also commented about that a few posts above: #3416 (comment)

@yjqiang
Copy link

yjqiang commented Dec 2, 2018

@Raphael-C-Almeida But the way we are going to use is just forbidding http redirection and http requests. Maybe there will be other ways to hack it? So it is not good for "forbid_insecure".

@almeida-raphael
Copy link
Author

Makes sense.

@HeavenVolkoff
Copy link

HeavenVolkoff commented Dec 2, 2018

IMHO enforce_ssl is better than enforce_tls due to the fact that aiohttp uses the argument ssl for passing in a SSLContext and Python, in general, still uses the ssl name...

@asvetlov
Copy link
Member

asvetlov commented Dec 2, 2018

The proposal is forbidding not SSL/TSL transport. The intention is clear, the implementation is obvious.
I don't see a way to hack it

@yjqiang
Copy link

yjqiang commented Dec 3, 2018

The proposal is forbidding not SSL/TSL transport. The intention is clear, the implementation is obvious.
I don't see a way to hack it

forbid_insecure means aiohttp will ban all ways that people can hack connection. So it means aiohttp will be a 100% safe module even in the future. Even if it might be 100% safe right now. But who knows about the future?

@webknjaz
Copy link
Member

webknjaz commented Dec 6, 2018

IMHO enforce_ssl is better than enforce_tls

The modern thing is called TLS, SSL is a deprecated: https://tools.ietf.org/html/rfc7568

@HeavenVolkoff
Copy link

The modern thing is called TLS, SSL is a deprecated: https://tools.ietf.org/html/rfc7568

Yes, I know. Just saying that aiohttp and Python still use the term ssl in several places. But I'm ok with either one...

@webknjaz
Copy link
Member

@HeavenVolkoff actually, there's a PEP with is seeking to fix that https://www.python.org/dev/peps/pep-0543/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants