-
-
Notifications
You must be signed in to change notification settings - Fork 907
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
base: master
Are you sure you want to change the base?
Conversation
Thank you for your contribution!
|
@fviard Thank you for your advice. For backward-compatibility, |
# python 3 support | ||
from urlparse import urlparse | ||
except ImportError: | ||
from urllib.parse import urlparse |
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.
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('@') |
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.
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(':') |
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.
Same a previous, split(':', 1)
Thank your for your update and sorry for the long delay of my reply, sadly it was a very busy month :-s
Your point makes sense, but I'm a little bit reluctant to add a new option because of the confusion that it would induce. But something that could be done, I think, is to already accept url with that pattern in proxy_host. |
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 variablehttp_proxy
orhttps_proxy
(ifuse_https = True
)TODO
use_https = True
-> SetProxy-Authenticate
header via HTTPConnection.set_tunnel()use_https = False
-> Need helpsplit()
withurlparse()
https://github.com/s3tools/s3cmd/pull/1129/files#diff-a33e8f45f499529c23e3ee67788fb744R237-R238