Support multiple basic_auth statements in caddy2

[klzgrad]

forwardproxy/forwardproxy.go

Lines 729 to 732 in 8d6f47b

	// TODO: Support multiple basicauths.
	if h.BasicauthUser != "" || h.BasicauthPass != "" {
	return d.Err("Multi-user basicauth is not supported")
	}

Multiple basic_auth statements are not supported because it was not clear what json structure you wanted for doing it @mholt.

Some users are complaining klzgrad/naiveproxy#137.

[mholt]

Sure, that's something that can be added I'm pretty sure.

[klzgrad]

I can add it but what json structure you want for doing this?

[klzgrad]

Hi @mholt, can you choose one json format for multiple basicauths from below so I can start making a PR?

"basicauths": [
  {"user": "me", "password": "hunter"}
]

"users": ["me"],
"passwords": ["hunter"],

"basicauths": [
  ["me", "hunter"]
]

[wiserain]

Maybe one more choice for your reference:

In traefik, users can configure multiple basicauths in a list. Below is an example of toml format used in traefik.

# Declaring the user list
[http.middlewares]
  [http.middlewares.test-auth.basicAuth]
    users = [
      "test:$apr1$H6uskkkW$IgXLP6ewTrSuBkTrqE8wj/", 
      "test2:$apr1$d9hr9HBB$4HxwgUir3HP4EsggP/QNo0",
    ]

I don't know implementation details but would be nice to use hashed passwords in conf, which is safe to expose in a plain text.

Thanks.

[mholt]

Thanks for helping with this!

Before we go too far on this, I am quickly skimming the code and realizing that I only brought over the basicauth functionality to get the tests to pass temporarily. Caddy 2 already has a very flexible and capable authentication module that supports basicauth: https://caddyserver.com/docs/modules/http.authentication.providers.http_basic

Ideally we wouldn't have any authentication logic in this proxy module at all, since it exists in that module. However, if it's necessary for probe resistance, then at least we should look into simply embedding that module into this one, rather than re-implementing it.

For an example of this, Caddy's reverse_proxy module embeds the headers module which manipulates request/response headers: https://github.com/caddyserver/caddy/blob/385adf5d878939c381c7f73c771771d34523a1a7/modules/caddyhttp/reverseproxy/reverseproxy.go#L91

What do you think about that? Should be easier and faster right?

[klzgrad]

Forward proxy in Caddy1 implements its own basicauth to try to avoid side channel attacks with this

forwardproxy/forwardproxy.go

Lines 177 to 182 in 247c0ba

	if subtle.ConstantTimeCompare(creds, []byte(pa[1])) == 1 {
	// Please do not consider this to be timing-attack-safe code. Simple equality is almost
	// mindlessly substituted with constant time algo and there ARE known issues with this code,
	// e.g. size of smallest credentials is guessable. TODO: protect from all the attacks! Hash?
	return nil
	}

I'll have to remove this constant time compare if I'm to do it with the authentication module. This mitigation is only theoretical but there are real people waiting to use this feature.

Edit: Ok, the purpose of this constant time compare is to hide the existence of the auth check so to realize probe resistance, i.e. arbitrary auth input regardless of success or not should take the same amount of time. A real test case is that if I craft a Proxy-Authorization header with a very long password, and if it takes more time to process than a short password, this reveals the existence of an auth checking proxy.

I don't think Caddy 2's auth module is capable of constant time authentication. Even hashed password as requested by wiserain above is suspect without audited constant time hashing algorithm.

[ha-ku]

Any update for this?

[Bryan2333]

Any update on this?

[klzgrad]

Fixed after #74