superfluous response.WriteHeader after writing from an authentication module

[freman]

I'd argue there's plenty of reasons to do a redirect in a request to login...

2020/05/18 20:41:10 http: superfluous response.WriteHeader call from github.com/caddyserver/caddy/v2/modules/caddyhttp.(*Server).ServeHTTP (server.go:245)

package testheader

import (
    "net/http"

    "github.com/caddyserver/caddy/v2"
    "github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyauth"
)

func init() {
    caddy.RegisterModule(TestWriteHeader{})
}

type TestWriteHeader struct {
}

// CaddyModule returns the Caddy module information.
func (TestWriteHeader) CaddyModule() caddy.ModuleInfo {
    return caddy.ModuleInfo{
        ID:  "http.authentication.providers.testheader",
        New: func() caddy.Module { return new(TestWriteHeader) },
    }
}

// Authenticate the request
func (t TestWriteHeader) Authenticate(w http.ResponseWriter, r *http.Request) (caddyauth.User, bool, error) {
    http.Redirect(w, r, "http://example.org", 303)
    return caddyauth.User{}, false, nil
}

[francislavoie]

That error is coming from Go, not from Caddy. Once headers are written, they can't be written again.

I was working on a PR recently which ran into the same problem. What you'll need to do is wrap the ResponseWriter so that any write operations are delegated.

https://github.com/francislavoie/caddy/blob/statuscode-handler/modules/caddyhttp/statuscode.go#L76

Hopefully this helps you push forwards :smile:

I'll close this as it's not a bug or feature request, but feel free to continue discussion.

[freman]

The authentication handlers don't get the chance to wrap the http response writer, the superfluous header is being triggered by caddy after the authentication method returns. Perhaps we could return a type of error that indicates that the header writing has already been handled.

Or caddy can wrap the writer before passing to authentication handlers

[mholt]

This happens because the Authenticate() method is handling the request (i.e. writing the response), while returning false and nil values, indicating to the caller that the request has not been handled yet.

If we need Authenticate() to be able to write to the response, then we probably need a sentinel return value that can indicate that. Maybe an error value like var ErrHandled = fmt.Errorf("request handled") - any thoughts @greenpau or @roblabla?

[francislavoie]

FWIW that sounds reasonable to me :+1:

[freman]

And there's already precedent in go stdlib, http.ErrUseLastResponse, so not such a hacky solution

[roblabla]

So, what I did in my JWT plugin is handle the redirection separately from the authentication, through the use of an error route. This is much better IMO, as it allows the end-user to have full flexibility over what happens in the case of an authentication failure if they so wish. You can easily generate a default error route in the JSON yourself in the parseDirective.

While I do see value in making this work, I think authentication plugins should shy away from answering the HTTP request from within the Authenticate, as it ultimately allows for a much more flexible system.

[greenpau]

If we need Authenticate() to be able to write to the response, then we probably need a sentinel return value that can indicate that. Maybe an error value like var ErrHandled = fmt.Errorf("request handled") - any thoughts @greenpau or @roblabla?

@mholt , I think this is happening because authentication and authorization functions are being blended in one plugin.

[mholt]

I like @roblabla's approach of using error routes to handle auth errors, it just means the behavior isn't self-contained (at least, in the JSON -- it could probably be bundled up in the Caddyfile somehow).

I also like the idea of separating authentication and authorization.

Proposals?

[greenpau]

http.Redirect(w, r, "http://example.org", 303)

@freman, do not use http.Redirect. Rather, use something like this:

https://github.com/greenpau/caddy-auth-forms/blob/99388d341f1461af7de00aa48b4c19263719635a/plugin.go#L469-L472

[greenpau]

I also like the idea of separating authentication and authorization.

@mholt , side note ... I think it makes sense to document how caddyauth.User can be used by downstream plugins.

[freman]

http.Redirect(w, r, "http://example.org", 303)

@freman, do not use http.Redirect. Rather, use something like this:

https://github.com/greenpau/caddy-auth-forms/blob/99388d341f1461af7de00aa48b4c19263719635a/plugin.go#L469-L472

This code still has a writeheader warning and results in the secret information being displayed (curl -v do a show the location header and 303 working but also shows the static content I configured in caddy being rendered. So seems like people accidentally configure caddy-auth-forms to leak secret data)

shannon@mac ~ $ curl -v http://[::]:9080/secret
*   Trying ::...
* TCP_NODELAY set
* Connected to :: (::1) port 9080 (#0)
> GET /secret HTTP/1.1
> Host: [::]:9080
> User-Agent: curl/7.64.1
> Accept: */*
>
< HTTP/1.1 303 See Other
< Location: http://www.example.org
< Server: Caddy
< Date: Mon, 18 May 2020 21:35:44 GMT
< Content-Length: 11
< Content-Type: text/plain; charset=utf-8
<
* Connection #0 to host :: left intact
tell no-one* Closing connection 0

If I can't do this the way the users of my plugin expect it to work (based on the features they asked for in the V1 version) I'll just rewrite it as middleware as it was in V1.

[mholt]

It's because writing a response does not terminate the middleware chain -- currently, an error would have to be returned to terminate it.

Before you rewrite things differently, let's find a design that better accommodates all the various authentication mechanisms.

[roblabla]

@freman what's wrong with using an error route? I suppose your users are using a Caddyfile (instead of using the JSON directly). When you go from caddyfile to JSON, it's possible to create multiple routes, such that your authentication module is wrapped in an error handling routine.

Look at https://github.com/caddyserver/caddy/issues/2894#issuecomment-589303660 and https://github.com/roblabla/caddy-jwt/blob/caddy2/config.go#L294

[freman]

@roblabla I suppose my concern is that it's easier to misconfigure it and it takes the control out of the plugin generating that error which may only relate to that plugin. If that's how it's to be done I'll document it and let em complain or go back to being middleware so it behaves as they expect.

[mholt]

I do agree with @freman in that failed auth should be handled close by, rather than requiring the user to configure it.

Tbh, I haven't delved into know what's really going on here. Someone will need to sit down and figure it out if they have a chance, I may not be able to get around to it for a while.

[greenpau]

So seems like people accidentally configure caddy-auth-forms to leak secret data)

@freman , not sure what you mean by leaking. The plugin does not do authorization. It authenticates users and issues JWT token. You are not supposed to protect anything with it. It works in conjunction with caddy-auth-jwt.

[roblabla]

@freman I don't understand why you're talking about configuration. My strategy doesn't require the user to configure anything unless they're using raw JSON (and if they're doing this, I expect them to want this kind of flexibility).

My strategy involves the plugin adding the error handling route itself, when converting from Caddyfile to JSON. The user doesn't have to configure anything in this case. The additional route is only user-visible if they dump the internal JSON route representation. In the parseCaddyfile:

        authRoute := caddyhttp.Route{
            HandlersRaw: []json.RawMessage{
                caddyconfig.JSONModuleObject(authHandler, "handler", authHandler.CaddyModule().ID.Name(), nil),
            },
        }

        redirectHandler := caddyhttp.StaticResponse{
            StatusCode: "303",
            Headers: http.Header{
                "Location": {*shouldRedirect},
            },
        }
        redirectRoute := caddyhttp.Route{
            HandlersRaw: []json.RawMessage{
                caddyconfig.JSONModuleObject(redirectHandler, "handler", redirectHandler.CaddyModule().ID.Name(), nil),
            },
            Terminal: true,
        }
        subroute := caddyhttp.Subroute{
            Routes: []caddyhttp.Route{authRoute},
            Errors: &caddyhttp.HTTPErrorConfig{
                Routes: []caddyhttp.Route{redirectRoute},
            },
        }

        return h.NewRoute(matcherSet, &subroute), nil

[freman]

So seems like people accidentally configure caddy-auth-forms to leak secret data)

@freman , not sure what you mean by leaking. The plugin does not do authorization. It authenticates users and issues JWT token. You are not supposed to protect anything with it. It works in conjunction with caddy-auth-jwt.

Sorry mate, might have come across a little more aggressive than intended. What I mean is that in testing simpler similar code to what you linked me to in caddy-auth-forms without caddy-auth-jwt I can get caddy to do a 303 redirect and still print out the content of the page that should be protected.

I realise this is a configuration error, and I haven't actually tested it with caddy-auth-forms or caddy-auth-jwt but I'd rather avoid a configuration error being capable of causing a leak.

package test

import (
        "net/http"

        "github.com/caddyserver/caddy/v2"
        "github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyauth"
)

func init() {
        caddy.RegisterModule(TestWriteHeader{})
}

type TestWriteHeader struct {
}

// CaddyModule returns the Caddy module information.
func (TestWriteHeader) CaddyModule() caddy.ModuleInfo {
        return caddy.ModuleInfo{
                ID:  "http.authentication.providers.test",
                New: func() caddy.Module { return new(TestWriteHeader) },
        }
}

// Authenticate the request
func (t TestWriteHeader) Authenticate(w http.ResponseWriter, r *http.Request) (caddyauth.User, bool, error) {
        w.Header().Set("Location", "http://www.example.org/login")
        w.WriteHeader(303)
        return caddyauth.User{}, true, nil
}

My strategy involves the plugin adding the error handling route itself, when converting from Caddyfile to JSON. The user doesn't have to configure anything in this case. The additional route is only user-visible if they dump the internal JSON route representation. In the parseCaddyfile:

I am not using registering a directive, but error handling (or in this case failure mode) is it's own thing in code... I've gone CaddyModule(), Provision(), Validate(), Authenticate() - relying on json parsing so maybe that's something I need to look at.

[greenpau]

I can get caddy to do a 303 redirect and still print out the content of the page that should be protected.

return caddyauth.User{}, true, nil

@freman , it should be false, not true. In my example, the user who gets redirect is an authenticated user.

[freman]

it should be false, not true. In my example, the user who gets redirect is an authenticated user.

If it's false I still get the warning caddy is sending headers.

[mholt]

That could use some investigation, if you have a chance @freman. I'm a bit swamped this week...

PS. I super-duper appreciate both of your willingness to figure this out and make it work!

[greenpau]

https://github.com/freman/caddy2-reauth/blob/master/reauth.go

@freman , looking at this I think you read “forms” a little bit more that you willing to admit :-)

[freman]

@freman , looking at this I think you read “forms” a little bit more that you willing to admit :-)

I looked around, that looked like the most straightforward implementation, so I based the caddy2 entry point around that yes.

Seemed like the quickest and simplest way to port.

[greenpau]

I looked around, that looked like the most straightforward implementation, so I based the caddy2 entry point around that yes.

@freman , I like how you expanded on the interface idea in backend for “failures“ 👍

[freman]

@freman , I like how you expanded on the interface idea in backend for “failures“ 👍

https://github.com/freman/caddy-reauth/blob/master/failure.go

Was always there, just wasn't as defined. I really enjoy the direct from JSON building of the module

[greenpau]

really enjoy the direct from JSON building of the module

@freman , What do you mean by “the direct”?

[greenpau]

@freman , it is worth reading README in caddy-auth-jwt. You may have some challenges porting considering that each endpoint is separate instance.

[freman]

@freman , it is worth reading README in caddy-auth-jwt. You may have some challenges porting considering that each endpoint is separate instance.

Yeh I expect I'll have to revisit the ldap code at some point but I was just rushing it out the door

[mholt]

Would it be helpful if you joined us in our developer Slack, @freman? Might make quick back-and-forths a little easier :) Just let me know which email address to invite if you want in.

[mholt]

Is this still a problem, and is it a problem in Caddy's core or standard modules? If so, we can reopen this.