Remove Link headers for management interface#254
Conversation
| wfe.HandleFunc(m, rootKeyPath, wfe.handleKey(wfe.ca.GetRootKey, rootKeyPath), "GET") | ||
| wfe.HandleFunc(m, intermediateCertPath, wfe.handleCert(wfe.ca.GetIntermediateCert, intermediateCertPath), "GET") | ||
| wfe.HandleFunc(m, intermediateKeyPath, wfe.handleKey(wfe.ca.GetIntermediateKey, intermediateKeyPath), "GET") | ||
| wfe.HandleManagementFunc(m, RootCertPath, wfe.handleCert(wfe.ca.GetRootCert, RootCertPath), "GET") |
There was a problem hiding this comment.
I think you could probably remove HandleManagementFunc and just call m.Handle directly:
| wfe.HandleManagementFunc(m, RootCertPath, wfe.handleCert(wfe.ca.GetRootCert, RootCertPath), "GET") | |
| m.Handle(RootCertPath, wfe.handleCert(wfe.ca.GetRootCert, RootCertPath)) |
Most of the stuff that's in HandleFunc (and your copy of it in HandleManagementFunc) is specific to quirks of ACME, and we don't really need it in Pebble. For that matter, we probably don't even need most of it for ACME - for instance, in RFC8555 everything is a POST except for new-nonce and directory; we could clean up the code that deals with tying things to specific HTTP methods.
There was a problem hiding this comment.
It would have to be
| wfe.HandleManagementFunc(m, RootCertPath, wfe.handleCert(wfe.ca.GetRootCert, RootCertPath), "GET") | |
| wfe.HandleManagementFunc(m, RootCertPath, wfe.handleCert(wfe.ca.GetRootCert, RootCertPath), "GET") | |
| m.Handle(RootCertPath, http.StripPrefix(RootCertPath, wfeHandlerFunc(wfe.handleCert(wfe.ca.GetRootCert, RootCertPath)))) |
StripPrefix is needed since otherwise RootCertPath needs to cut off the path prefix itself, and the wfeHandlerFunc() cast is needed since otherwise there's a compile error. While this works, it is pretty explicit (and that has to be repeated for every management endpoint). Having a small wfe.HandleManagementFunc like
func (wfe *WebFrontEndImpl) HandleManagementFunc(
mux *http.ServeMux,
pattern string,
handler wfeHandlerFunc) {
mux.Handle(pattern, http.StripPrefix(pattern, handler))
}makes this already much nicer.
One disadvantage this approach has is that the method checking is gone, i.e. the handlers reply to all HTTP methods independently of HTTP semantics. Also, there's no longer a no-cache header automatically added.
There was a problem hiding this comment.
Your simplified wrapper function looks nicer. I'm not too worried about losing the method checking and no-cache header.
There was a problem hiding this comment.
I've updated the code with the simplified wrapper.
| func (wfe *WebFrontEndImpl) HandleManagementFunc( | ||
| mux *http.ServeMux, | ||
| pattern string, | ||
| handler wfeHandlerFunc) { |
There was a problem hiding this comment.
handler can be net/http.Handler (from interfacer)
There was a problem hiding this comment.
When I do that, I get
wfe/wfe.go:359:58: cannot use wfe.handleCert(wfe.ca.GetRootCert, RootCertPath) (type func(context.Context, http.ResponseWriter, *http.Request)) as type http.Handler in argument to wfe.HandleManagementFunc:
func(context.Context, http.ResponseWriter, *http.Request) does not implement http.Handler (missing ServeHTTP method)
wfe/wfe.go:360:56: cannot use wfe.handleKey(wfe.ca.GetRootKey, rootKeyPath) (type func(context.Context, http.ResponseWriter, *http.Request)) as type http.Handler in argument to wfe.HandleManagementFunc:
func(context.Context, http.ResponseWriter, *http.Request) does not implement http.Handler (missing ServeHTTP method)
wfe/wfe.go:361:66: cannot use wfe.handleCert(wfe.ca.GetIntermediateCert, intermediateCertPath) (type func(context.Context, http.ResponseWriter, *http.Request)) as type http.Handler in argument to wfe.HandleManagementFunc:
func(context.Context, http.ResponseWriter, *http.Request) does not implement http.Handler (missing ServeHTTP method)
wfe/wfe.go:362:64: cannot use wfe.handleKey(wfe.ca.GetIntermediateKey, intermediateKeyPath) (type func(context.Context, http.ResponseWriter, *http.Request)) as type http.Handler in argument to wfe.HandleManagementFunc:
func(context.Context, http.ResponseWriter, *http.Request) does not implement http.Handler (missing ServeHTTP method)
There was a problem hiding this comment.
Yeah I think golangcibot may be confused here and can probably be ignored.
That said, Travis jobs are failing, so let's see whether golangcibot still reports this once those tests are fixed.
There was a problem hiding this comment.
Both Travis and AppVeyor fail because of
$ golangci-lint run
wfe/wfe.go:227:2: `handler` can be `net/http.Handler` (interfacer)
handler wfeHandlerFunc) {
^
So exactly the same "problem". Every other test passed.
There was a problem hiding this comment.
Can you add interfacer to the disable list in .golangci.yml?
There was a problem hiding this comment.
That sounds too general for me. I've first tried to disable this particular test for this line (according to https://github.com/golangci/golangci-lint#nolint). I'm not sure whether it's the correct line, but since GolangCI seems already to be happy, I guess it was the correct one :)
|
@jsha thanks for handling this! |
Fixes #253.
I'm not sure that this is the best implementation. Another way would be to add a boolean argument (I don't think this is good, though), or to have a more generic function which is used by
HandleFuncandHandleManagementFunc.Any opinions on this?