caddyserver / forwardproxy

Forward proxy plugin for the Caddy web server
Apache License 2.0
603 stars 228 forks source link

Fix multiauth for caddy2 #99

Closed Mygod closed 11 months ago

Mygod commented 1 year ago

1. What does this change do, exactly?

Add back multiauth support for caddy2.

2. Please link to the relevant issues.

74

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

4. Checklist

Mygod commented 1 year ago

For further context, proxy authentication and http authentication seem to be different (Authorization header vs Proxy-Authorization header) so there is not a clear path towards reusing caddy basicauth.

gaby commented 1 year ago

@Mygod Thanks for the PR! I'm still working out an issue with #74 which also includes adding tests.

Have you noticed that test fail regarding the h3 value? Or is that issue not happening with your changes?

Mygod commented 1 year ago

Sorry I have not actually run any unit tests outside of running caddy directly with the plugin. What's this h3 value you are talking about?

gaby commented 1 year ago

@Mygod This is the error:

Failing test:

=== RUN   TestGETAuthWrongProbeResist
    probe_resist_test.go:52: header "Alt-Svc" is different: different string at position 0: h3=":8888"; ma=2592000 vs h3=":9999"; ma=2592000
--- FAIL: TestGETAuthWrongProbeResist (0.00s)
Mygod commented 1 year ago

@gaby Sorry no idea. I just took a look and I couldn't get the tests running on my end either.

Meanwhile, I added support for adding the user to log entry user_id (used by the basicauth module).

gaby commented 1 year ago

@Mygod Thanks for updating your branch, we still working on getting the tests for Caddy V2 working. :-)

gaby commented 1 year ago

These are new failures:

=== RUN   TestCONNECTViaUpstream
    forwardproxy_test.go:364: expected length: 57. Got thus far: 0. Expected response: I am not a ForwardProxy, but I want to be when I grow up!
        Got: 
--- FAIL: TestCONNECTViaUpstream (0.00s)
=== RUN   TestGETViaUpstream
    forwardproxy_test.go:382: expected length: 57. Got thus far: 0. Expected response: I am not a ForwardProxy, but I want to be when I grow up!
        Got: 
--- FAIL: TestGETViaUpstream (0.00s)
Mygod commented 1 year ago

Hmm I don't think anything I changed was related to that though?

diyism commented 11 months ago

Any progress? It seems only 1 test failed.

gaby commented 11 months ago

@Mygod We have to merge https://github.com/caddyserver/forwardproxy/pull/109 first then we can take a look if that fixes the tests for your PR

gaby commented 11 months ago

@Mygod can you update your branch with the latest changes to caddy2 branch?

gaby commented 11 months ago

The tests in https://github.com/caddyserver/forwardproxy/pull/74 are now passing, update your branch and we can go from there. Thanks!

gaby commented 11 months ago

@Mygod Some of your changes are causing the tests to fail.

Mygod commented 11 months ago

Don't have time to fix them any time soon... Feel free to fix. :)

johnzhanghua commented 11 months ago

Don't have time to fix them any time soon... Feel free to fix. :)

It because the cred is not right, it should be "upstreamtest" and "upstreampass", but it uses the "test:pass". It needs to define another 'upstreambuf' var for this .

https://github.com/caddyserver/forwardproxy/pull/99/files#diff-db8006086a10baf66c667cc9830d20488fe7857f83cc533e0d0b13cbe70d0342L258.

Mygod commented 11 months ago

Wee. :)

Maybe use the squash option to merge? :)

gaby commented 11 months ago

@Mygod Will review today/tomorrow Thanks! 💪

gaby commented 11 months ago

@Mygod If you can fix those two things I will merge it. Thanks!

Mygod commented 11 months ago

Done