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

Removed insecure upstreams check #142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Saiv46
Copy link

@Saiv46 Saiv46 commented Oct 12, 2024

Dockerfile

FROM caddy:builder AS builder
RUN xcaddy build \
    --with github.com/caddyserver/forwardproxy=github.com/saiv46/forwardproxy@patch-1
FROM caddy:latest
COPY --from=builder /usr/bin/caddy /usr/bin/caddy

1. What does this change do, exactly?

Removes insecure schemes are only allowed to localhost upstreams, because it prevents using upstreams in Docker containers.

2. Please link to the relevant issues.

Resolves #116

3. Which documentation changes (if any) need to be made because of this PR?

- Supported schemes to remote host: https.
- Supported schemes to localhost: socks5, http, https (certificate check is ignored).
+ Supported schemes: socks5, http, https (certificate check is ignored for localhost).

4. Checklist

  • I have written tests and verified that they fail without my change
  • I made pull request as minimal and simple as possible. If change is not small or additional dependencies are required, I opened an issue to propose and discuss the design first
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code

@zoulja
Copy link

zoulja commented Nov 14, 2024

Does anybody know what must be done to accept this PR?

@cachius
Copy link

cachius commented Nov 14, 2024

Does anybody know what must be done to accept this PR?

Probably @mholt.

I imagined to put the new behaviour behind a switch, maybe.
Am not sure whether CONNECTing to the upsteam HTTP proxy requires extra implementation.

@mholt
Copy link
Member

mholt commented Nov 14, 2024

Unfortunately I don't think I'm qualified at this time to decide whether this change is a security risk or is the correct thing to do. (see #116 (comment)). I am just not familiar enough with the code base / threat model, and don't really have the time at the moment. Anyone else is welcome to investigate though!

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.

insecure schemes are only allowed to localhost upstreams
4 participants