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 authenticated proxies #1129

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

mikoim
Copy link

@mikoim mikoim commented Sep 3, 2020

This PR allows s3cmd to use an authenticated proxy server.
fix #1128

Changed behavior

  • proxy_host accepts username and password (e.g., proxy_host = username:password@hostname)
  • s3cmd --configure can handle a credential from environment variable http_proxy or https_proxy (if use_https = True)

TODO

@fviard
Copy link
Contributor

fviard commented Sep 8, 2020

Thank you for your contribution!
Here are my suggestions for a few improvements:

  • don't use encode/decode but, "deunicodise_s" and "unicodise_s" from Utils.
    And so, there will be no need to specify the encoding.
    (as there is no issue to decode ascii with utf-8).

  • Instead of creating new config entries for proxy_username, proxy_password, it would be better to reuse the proxy_host entry so, configuration is not changed.
    the id would be to support a syntax like:
    proxy_host = login:pass@myhost
    For that, nothing special to do, just at the time of usage, you have 2 choices:

    • Just detect the @ in proxy_host and split by @ and : to extract login/pass/host.

    • Use urlparse to extract the info.

  • For use_https = False, it looks like that there is no nice way to do that, but I'm still thinking about it.

@mikoim
Copy link
Author

mikoim commented Sep 15, 2020

@fviard Thank you for your advice. For backward-compatibility, proxy_host and proxy_port should be kept, but I guess that the format "login:pass@myhost" is not common. How about introducing new parameter like a proxy_url, which supports "schema://username:password@host:port/"?

# python 3 support
from urlparse import urlparse
except ImportError:
from urllib.parse import urlparse
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a detail, but you can probably exchange the imports and have the python3 "from urlib.parse import urlparse" case first as it should now be the default case.

headers = {}
proxy_hostname = cfg.proxy_host
if '@' in cfg.proxy_host:
credential, proxy_hostname = cfg.proxy_host.split('@')
Copy link
Contributor

Choose a reason for hiding this comment

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

it is probably better to use split('@', 1) to avoid unexpected issues.

if '@' in cfg.proxy_host:
credential, proxy_hostname = cfg.proxy_host.split('@')
# FIXME: Following line can't handle username or password including colon
proxy_username, proxy_password = credential.split(':')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same a previous, split(':', 1)

@fviard
Copy link
Contributor

fviard commented Oct 12, 2020

Thank your for your update and sorry for the long delay of my reply, sadly it was a very busy month :-s

@fviard Thank you for your advice. For backward-compatibility, proxy_host and proxy_port should be kept, but I guess that the format "login:pass@myhost" is not common. How about introducing new parameter like a proxy_url, which supports "schema://username:password@host:port/"?

Your point makes sense, but I'm a little bit reluctant to add a new option because of the confusion that it would induce.
(As I think that s3cmd is already a little bit scary as it has a lot of options)
And we would have still to support the proxy_host/proxy_port option for a long time for backward compatibility.

But something that could be done, I think, is to already accept url with that pattern in proxy_host.
Like, if there is a port in proxy_host, it overrides proxy_port.

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

Successfully merging this pull request may close these issues.

HTTP Proxy Authentication
2 participants