Skip to content

POC for using the basic_auth provisions from caddy internals #147

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

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

Conversation

proofrock
Copy link

@proofrock proofrock commented Dec 19, 2024

1. What does this change do, exactly?

Hello!
as suggested, I tried to modify the forwardproxy current authentication to re-use caddy's (main) code. I think I am getting there, but there are a few things that - if you are interested - may need discussion.

In the code, I marked the relevant parts with a [POC] comment tag and a brief discussion.

Some explanations:

  • I tried to reuse Authenticate and parseCaddyFile to do the authentication and parsing, but they do just a little bit too much - the logic is adherent to where they're originally used (of course). For instance, Authenticate writes to the ResponseWriter and I don't think we need that; and parseCaddyFile is internal, uses a httpcaddyfile.Helper that is not available, and expects a slightly different "position" in the config file layout.
  • So I duplicated the minimum amount of code that is necessary in two methods I put in the adapter.go file.
  • Unfortunately, this meant duplicating also the file basicauth.go (as it is, basically) because it contains some internal fields and methods I need to access from those new methods.
  • All this could be avoided and made transparent by minimally patching caddy's code, but for now I just wanted your opinion on all the setup.
  • Code that use all this - in caddyfile.go and forwardproxy.go - should be quite straightforward; the changes in error messages are also marked with [POC]. Please mainly review if these files suits you, what I described in the earlier points are "just" to make these changes work and can be optimized, as I said.
  • Tests don't work 🤔 While xcaddy run... compiles against the latest caddy and my code is done against that version, go test compiles against v2.7.6, and there's a breaking change in basicauth.go#Comparer that makes things difficult.
  • Even after that, in the tests the auth object is provisioned manually, and the code would need a ctx I don't have available at initialization time. I tried to work around this, but the point before makes it really difficult; so I hope that it's possible to update the "base" version before (possibly, eventually) finalizing all this.

To test:

  • Create a caddyfile
:80
route {
  forward_proxy {
    basic_auth
      test1 $2a$14$Zkx19XLiW6VYouLHR5NmfOFU0z2GTNmpkT/5qqR7hx4IjWJPDhjvG
      test2 $2a$14$Zkx19XLiW6VYouLHR5NmfOFU0z2GTNmpkT/5qqR7hx4IjWJPDhjvG
  }
}
  • Run with xcaddy run -- --config caddyfile

  • Try with

curl -x http://test1:hiccup@localhost:80 https://ifconfig.io # Success
curl -x http://test2:hiccup@localhost:80 https://ifconfig.io # Success
curl -x http://test3:hiccup@localhost:80 https://ifconfig.io # This one fails
curl -x http://test1:hiccap@localhost:80 https://ifconfig.io # This one fails

Thanks for the great code and the fun ride! Even if this will go nowhere, which is of course ok, I definitely learned something.

2. Please link to the relevant issues.

None

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

README.md, breaking changes should be explained in the basic_auth section(s).

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

@francislavoie
Copy link
Member

francislavoie commented Jan 15, 2025

This makes sense to me! What changes do you think would need to be made to Caddy's basicauth package? I think we can export some more things if necessary. Re v2.7.6, we can bump forwardproxy to v2.8.4 minimum at this stage I think (see #137 (comment) for my comment on that)

@proofrock
Copy link
Author

proofrock commented Jan 15, 2025

Hi Francis!

This makes sense to me! What changes do you think would need to be made to Caddy's basicauth package? I think we can export some more things if necessary.

Yes, this should do the trick, maybe it's also possible to create the two methods that I had to put in adapters.go in a more elegant way. Of course it should be tested. I'd do it with joy, but I need to compile the whole thing with modified caddy code... currently I'm using xcaddy with the stock caddy code and my custom fork of forwardproxy; how can I make it point to a fork of the caddy code? Or how can I compile the whole thing together?

Re v2.7.6, we can bump forwardproxy to v2.8.4 minimum at this stage I think (see #137 (comment) for my comment on that)

That would be great; IMHO it should be done before merging this PR (if it will be merged, of course). Then I will "adapt" this PR.

Thanks!

PS: I'm running this version since December in my homelab, accessing it everyday, it seems to work reliably. Not conclusive for anything of course, but it reassures me 😁

@francislavoie
Copy link
Member

francislavoie commented Jan 15, 2025

how can I make it point to a fork of the caddy code?

See the xcaddy README, it covers that. You can easily point it to a local copy of Caddy.

Of course any API change to Caddy itself being necessary means this PR would be halted until those changes are in a tagged release of Caddy, at which point this can be merged. (Probably best to mark this PR as a draft for now)

Thanks for working on it!

@proofrock proofrock marked this pull request as draft January 15, 2025 09:19
@proofrock
Copy link
Author

how can I make it point to a fork of the caddy code?

See the xcaddy README, it covers that. You can easily point it to a local copy of Caddy.

Thanks!

Of course any API change to Caddy itself being necessary means this PR would be halted until those changes are in a tagged release of Caddy, at which point this can be merged. (Probably best to mark this PR as a draft for now)

But of course. This PR was never meant (in my mind) to be merged like this, too much duplicated code. I've changed it to draft.

Thanks for your time!

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.

2 participants