-
Notifications
You must be signed in to change notification settings - Fork 13
feat: Add username/password authentication for forward proxies #18
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
base: main
Are you sure you want to change the base?
feat: Add username/password authentication for forward proxies #18
Conversation
…roxies 1) Now there are 3 additional cli flags, controlling authentication for forward proxies. Authentication is realized via RFC1929 and RFC7235. 2) Improve a little bit error handling for HTTP headers 3) Make some types optional for clearness
JonSnowWhite
left a comment
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.
Thanks a lot for the PR, looks good, I only have minor comments
| if args.forward_proxy_mode.name == "SOCKSv4" and (username or password): | ||
| print("[warn] Credentials are ignored for SOCKS4 forward proxy", file=sys.stderr) | ||
|
|
||
| if args.forward_proxy_socks5_auth == 'no_auth' and (username or password): |
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.
add a warning for incomplete credentials for HTTPS proxy mode and a warning for a configured SOCKSv5 proxy auth when the mode is HTTPS
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.
I noticed I should add sanity checks to the rest of the parameters as well... thanks for the implicit heads up!
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.
@JonSnowWhite I think cli is starting to bloat significantly due to the large number of command flags. Maybe migrate to some sort of config? Probably as part of a separate issue.
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.
Definitely an option for the future, yes
| The proxy type of the forward proxy | ||
| --forward_proxy_resolve_address, --no-forward_proxy_resolve_address | ||
| Whether to resolve domains before including them in the HTTP CONNECT request to the second proxy (default: False) | ||
| --forward_proxy_username FORWARD_PROXY_USERNAME |
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.
Please add warnings to the Readme, help command, and execution when any kind of authentication is used that passwords are transmitted in cleartext. Users should be aware, but I would still like to provide an explicit hint.
| # receive SOCKSv4 OK | ||
| answer = server_socket.recv(STANDARD_SOCKET_RECEIVE_SIZE) | ||
| if not answer.upper().startswith(Socksv4.socks4_ok()) and len(answer) != 8: | ||
| if not answer.startswith(Socksv4.socks4_ok()) or len(answer) != 8: |
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.
please add parantheses here
Description
This PR adds support for using authentication in forward proxies, using RFC1929 and RFC7235.
How this can be tested?
Used some socks5 & http proxies with authentication.