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 proxy support #10

Closed
wants to merge 1 commit into from
Closed

add proxy support #10

wants to merge 1 commit into from

Conversation

dojiong
Copy link

@dojiong dojiong commented Aug 11, 2023

support HTTP_PROXY/HTTPS_PROXY/ALL_PROXY environments

@Narsil
Copy link
Collaborator

Narsil commented Aug 11, 2023

Do you mind giving a bit more of motivation ?

This looks like just emulating Python requests which is definitely not desired.
IMHO proxy behavior belongs in user code, not manipulating env variables.

sync and async path need to be in sync too.

@dojiong
Copy link
Author

dojiong commented Aug 11, 2023

I think hf-hub is not a simple request library, it's more like a tool to manage huggingface models.
It is more convenient to transparently support proxy via env in hf-hub than provide a proxy-api to downstream user.

async version will add soon(missed)

@Narsil
Copy link
Collaborator

Narsil commented Aug 11, 2023

Do you mind trying to make this land upstream: algesten/ureq#322

Seems reqwest already has support: seanmonstar/reqwest#1856

@dojiong
Copy link
Author

dojiong commented Aug 11, 2023

Do you mind trying to make this land upstream: algesten/ureq#322

Seems reqwest already has support: seanmonstar/reqwest#1856

I'll try.

@Narsil
Copy link
Collaborator

Narsil commented Aug 11, 2023

TBH I didn't know this variable existed. Seems like a great way to exfiltrate traffic for any attacker..

I don't think env variables should be used for this IMHO, even if it does seems quite widespread.

@dojiong
Copy link
Author

dojiong commented Aug 11, 2023

it's safe enough, because we use tls.

@dojiong
Copy link
Author

dojiong commented Aug 11, 2023

upstream pr opened: algesten/ureq#649

@dojiong dojiong closed this Aug 11, 2023
@Narsil
Copy link
Collaborator

Narsil commented Aug 11, 2023

tls doesn't seem like a great justification. You're still intercepting all traffic.

requests doesn't seem to have a great record .... psf/requests#6071

@dojiong
Copy link
Author

dojiong commented Aug 11, 2023

requests's CURL_CA_BUNDLE issuse tells about a empty-string-bug, it's not about env is insecure.

attackers could modify your traffic in many ways if they can change your env. proxy env or ca-bundle env is in the same security level.

@Narsil
Copy link
Collaborator

Narsil commented Aug 11, 2023

Really ? How ?

@dojiong
Copy link
Author

dojiong commented Aug 11, 2023

If attackers can change your env, that would be the one of ways below

  1. have your system's permission to write file (e.g. shell config file, scripts) or inject machine code in your process
  2. you download a injected script/app to run
  3. someone told you to run the command

all the ways above indicate that the attacker have got the file-write permission, then they could change your script/app to do everything, even got the root privilege.

@Narsil
Copy link
Collaborator

Narsil commented Aug 11, 2023

even got the root privilege.

This is called privilege escalation, it's not an easy feat on a decently setup machine.

user space global proxy seems like a pretty bad vector to me. In any case it's definitely not the purpose of this lib to do this.

@dojiong
Copy link
Author

dojiong commented Aug 11, 2023

This is called privilege escalation, it's not an easy feat on a decently setup machine.

There's lots of things can do at normal user privilege, e.g. read $HF_HOME/token.

when your system is injected, that's not important your env would be changed, there be lots of ways to get/change data they want

@dojiong
Copy link
Author

dojiong commented Aug 11, 2023

It's like a knife, just a tool. just like browser permits javascript be executed.

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.

None yet

2 participants