From c64cf218b0a82d144e779454817eb0371a5002a8 Mon Sep 17 00:00:00 2001 From: Tw Date: Mon, 11 Apr 2016 12:16:41 +0800 Subject: [PATCH 01/12] http.CloseNotifier implementation for http.ResponseWriter wrapper Signed-off-by: Tw --- middleware/gzip/gzip.go | 9 +++++++++ middleware/recorder.go | 9 +++++++++ 2 files changed, 18 insertions(+) diff --git a/middleware/gzip/gzip.go b/middleware/gzip/gzip.go index 9d75b351..f24a94aa 100644 --- a/middleware/gzip/gzip.go +++ b/middleware/gzip/gzip.go @@ -141,3 +141,12 @@ func (w *gzipResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { } return nil, nil, fmt.Errorf("not a Hijacker") } + +// CloseNotify implements http.CloseNotifier. +// It just inherits the underlying ResponseWriter's CloseNotify method. +func (w *gzipResponseWriter) CloseNotify() <-chan bool { + if cn, ok := w.ResponseWriter.(http.CloseNotifier); ok { + return cn.CloseNotify() + } + panic("not a CloseNotifier") +} diff --git a/middleware/recorder.go b/middleware/recorder.go index ab30e305..50f4811c 100644 --- a/middleware/recorder.go +++ b/middleware/recorder.go @@ -87,3 +87,12 @@ func (r *ResponseRecorder) Flush() { panic("not a Flusher") // should be recovered at the beginning of middleware stack } } + +// CloseNotify implements http.CloseNotifier. +// It just inherits the underlying ResponseWriter's CloseNotify method. +func (r *ResponseRecorder) CloseNotify() <-chan bool { + if cn, ok := r.ResponseWriter.(http.CloseNotifier); ok { + return cn.CloseNotify() + } + panic("not a CloseNotifier") +} From 0a7ca64f53970dbbf94567ace6c873be9fabb69b Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Mon, 11 Apr 2016 00:24:26 -0600 Subject: [PATCH 02/12] gzipResponseWriter should also be a Flusher To be consistent with 3faad41b437c48cea37863123fab425169bc0c6e and c64cf218b0a82d144e779454817eb0371a5002a8 --- middleware/gzip/gzip.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/middleware/gzip/gzip.go b/middleware/gzip/gzip.go index f24a94aa..4ef65855 100644 --- a/middleware/gzip/gzip.go +++ b/middleware/gzip/gzip.go @@ -142,6 +142,16 @@ func (w *gzipResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { return nil, nil, fmt.Errorf("not a Hijacker") } +// Flush implements http.Flusher. It simply wraps the underlying +// ResponseWriter's Flush method if there is one, or panics. +func (w *gzipResponseWriter) Flush() { + if f, ok := w.ResponseWriter.(http.Flusher); ok { + f.Flush() + } else { + panic("not a Flusher") // should be recovered at the beginning of middleware stack + } +} + // CloseNotify implements http.CloseNotifier. // It just inherits the underlying ResponseWriter's CloseNotify method. func (w *gzipResponseWriter) CloseNotify() <-chan bool { From c86c26a0569589c4b468196eb500896bda944446 Mon Sep 17 00:00:00 2001 From: Thomas Boerger Date: Mon, 11 Apr 2016 19:39:35 +0200 Subject: [PATCH 03/12] Added "go up" link to browse template In order to have directly a link within the browse listing I have added a link to the top of the table to get one level up in the tree. Added that after a chat with @mholt. Signed-off-by: Thomas Boerger --- caddy/setup/browse.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/caddy/setup/browse.go b/caddy/setup/browse.go index 13d42e54..63acaddd 100644 --- a/caddy/setup/browse.go +++ b/caddy/setup/browse.go @@ -210,7 +210,8 @@ td:first-child svg { position: absolute; } -td .name { +td .name, +td .goup { margin-left: 1.75em; word-break: break-all; overflow-wrap: break-word; @@ -342,6 +343,17 @@ footer { {{end}} + {{if .CanGoUp}} + + + + Go up + + + — + — + + {{end}} {{range .Items}} From 004a7f84ef5069517bbb2a28e861fb6882ae5bc3 Mon Sep 17 00:00:00 2001 From: Thomas Boerger Date: Mon, 11 Apr 2016 21:08:30 +0200 Subject: [PATCH 04/12] Make test case less dependent on exact error string (#741) --- middleware/context_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/middleware/context_test.go b/middleware/context_test.go index e0c91f64..a61a3bf9 100644 --- a/middleware/context_test.go +++ b/middleware/context_test.go @@ -52,7 +52,13 @@ func TestInclude(t *testing.T) { fileContent: `str1 {{ .InvalidField }} str2`, expectedContent: "", shouldErr: true, - expectedErrorContent: `InvalidField is not a field of struct type middleware.Context`, + expectedErrorContent: `InvalidField`, + }, + { + fileContent: `str1 {{ .InvalidField }} str2`, + expectedContent: "", + shouldErr: true, + expectedErrorContent: `type middleware.Context`, }, } From e0b63d92f45fd662de21f7acfa42c20c1fa69868 Mon Sep 17 00:00:00 2001 From: Thomas Boerger Date: Mon, 11 Apr 2016 20:04:59 +0200 Subject: [PATCH 05/12] Added breadcrumb map function to browse In order to being able to really build a custom template for the browse directive I have added another function to build even custom breadcrumb paths. The other function `LinkedPath` is not that easy styleable as this map function. That way we are able to build the breadcrumb path matching different CSS frameworks like Bootstrap. Signed-off-by: Thomas Boerger --- middleware/browse/browse.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/middleware/browse/browse.go b/middleware/browse/browse.go index a6e8be86..e6070079 100644 --- a/middleware/browse/browse.go +++ b/middleware/browse/browse.go @@ -96,6 +96,34 @@ func (l Listing) LinkedPath() string { return result } +// BreadcrumbMap returns l.Path where every element is a map +// of URLs and path segment names. +func (l Listing) BreadcrumbMap() map[string]string { + result := map[string]string{} + + if len(l.Path) == 0 { + return result + } + + // skip trailing slash + lpath := l.Path + if lpath[len(lpath)-1] == '/' { + lpath = lpath[:len(lpath)-1] + } + + parts := strings.Split(lpath, "/") + for i, part := range parts { + if i == 0 && part == "" { + // Leading slash (root) + result["/"] = "/" + continue + } + result[strings.Join(parts[:i+1], "/")] = part + } + + return result +} + // FileInfo is the info about a particular file or directory type FileInfo struct { IsDir bool From ef95173827822ca58bbc82cdd9662a28bfddf065 Mon Sep 17 00:00:00 2001 From: Thomas Boerger Date: Tue, 12 Apr 2016 17:08:38 +0200 Subject: [PATCH 06/12] Dropped LinkedPath and updated browse template As discussed with @mholt I have dropped the old LinkedPath function and replaced it within the browse template with the new BreadcrumbMap function. Visually it looks exactly the same as before, now the template functionality is just more powerful. Signed-off-by: Thomas Boerger --- caddy/setup/browse.go | 9 ++++----- middleware/browse/browse.go | 28 ---------------------------- 2 files changed, 4 insertions(+), 33 deletions(-) diff --git a/caddy/setup/browse.go b/caddy/setup/browse.go index 63acaddd..28cb2582 100644 --- a/caddy/setup/browse.go +++ b/caddy/setup/browse.go @@ -264,7 +264,6 @@ footer { - @@ -300,14 +299,14 @@ footer { - -
-

{{.LinkedPath}}

+

+ {{range $url, $name := .BreadcrumbMap}}{{$name}}{{if ne $url "/"}}/{{end}}{{end}} +

-
+
{{.NumDirs}} director{{if eq 1 .NumDirs}}y{{else}}ies{{end}} {{.NumFiles}} file{{if ne 1 .NumFiles}}s{{end}}
diff --git a/middleware/browse/browse.go b/middleware/browse/browse.go index e6070079..6fe37ea3 100644 --- a/middleware/browse/browse.go +++ b/middleware/browse/browse.go @@ -6,7 +6,6 @@ import ( "bytes" "encoding/json" "errors" - "fmt" "net/http" "net/url" "os" @@ -69,33 +68,6 @@ type Listing struct { middleware.Context } -// LinkedPath returns l.Path where every element is a clickable -// link to the path up to that point so far. -func (l Listing) LinkedPath() string { - if len(l.Path) == 0 { - return "" - } - - // skip trailing slash - lpath := l.Path - if lpath[len(lpath)-1] == '/' { - lpath = lpath[:len(lpath)-1] - } - - parts := strings.Split(lpath, "/") - var result string - for i, part := range parts { - if i == 0 && part == "" { - // Leading slash (root) - result += `/` - continue - } - result += fmt.Sprintf(`%s/`, strings.Join(parts[:i+1], "/"), part) - } - - return result -} - // BreadcrumbMap returns l.Path where every element is a map // of URLs and path segment names. func (l Listing) BreadcrumbMap() map[string]string { From b149a86bc2a3fcbdec55ac1fb3e2e757adcb658e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?W=2E=E2=80=89Mark=20Kubacki?= Date: Tue, 12 Apr 2016 18:09:45 +0200 Subject: [PATCH 07/12] server: Rotate TLS ticket "keys" (#742) --- server/server.go | 79 +++++++++++++++++++++++++++++++++++++++++++ server/server_test.go | 60 ++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 server/server_test.go diff --git a/server/server.go b/server/server.go index 7b9856ef..b9fa575a 100644 --- a/server/server.go +++ b/server/server.go @@ -4,9 +4,11 @@ package server import ( + "crypto/rand" "crypto/tls" "crypto/x509" "fmt" + "io" "io/ioutil" "log" "net" @@ -18,6 +20,11 @@ import ( "time" ) +const ( + tlsNewTicketEvery = time.Hour * 10 // generate a new ticket for TLS PFS encryption every so often + tlsNumTickets = 4 // hold and consider that many tickets to decrypt TLS sessions +) + // Server represents an instance of a server, which serves // HTTP requests at a particular address (host and port). A // server is capable of serving numerous virtual hosts on @@ -28,6 +35,7 @@ type Server struct { HTTP2 bool // whether to enable HTTP/2 tls bool // whether this server is serving all HTTPS hosts or not OnDemandTLS bool // whether this server supports on-demand TLS (load certs at handshake-time) + tlsGovChan chan struct{} // close to stop the TLS maintenance goroutine vhosts map[string]virtualHost // virtual hosts keyed by their address listener ListenerFile // the listener which is bound to the socket listenerMu sync.Mutex // protects listener @@ -216,6 +224,11 @@ func serveTLS(s *Server, ln net.Listener, tlsConfigs []TLSConfig) error { return err } + // Setup any goroutines governing over TLS settings + s.tlsGovChan = make(chan struct{}) + timer := time.NewTicker(tlsNewTicketEvery) + go runTLSTicketKeyRotation(s.TLSConfig, timer, s.tlsGovChan) + // Create TLS listener - note that we do not replace s.listener // with this TLS listener; tls.listener is unexported and does // not implement the File() method we need for graceful restarts @@ -258,6 +271,11 @@ func (s *Server) Stop() (err error) { } s.listenerMu.Unlock() + // Closing this signals any TLS governor goroutines to exit + if s.tlsGovChan != nil { + close(s.tlsGovChan) + } + return } @@ -378,6 +396,67 @@ func setupClientAuth(tlsConfigs []TLSConfig, config *tls.Config) error { return nil } +var runTLSTicketKeyRotation = standaloneTLSTicketKeyRotation + +var setSessionTicketKeysTestHook = func(keys [][32]byte) [][32]byte { + return keys +} + +// standaloneTLSTicketKeyRotation governs over the array of TLS ticket keys used to de/crypt TLS tickets. +// It periodically sets a new ticket key as the first one, used to encrypt (and decrypt), +// pushing any old ticket keys to the back, where they are considered for decryption only. +// +// Lack of entropy for the very first ticket key results in the feature being disabled (as does Go), +// later lack of entropy temporarily disables ticket key rotation. +// Old ticket keys are still phased out, though. +// +// Stops the timer when returning. +func standaloneTLSTicketKeyRotation(c *tls.Config, timer *time.Ticker, exitChan chan struct{}) { + defer timer.Stop() + // The entire page should be marked as sticky, but Go cannot do that + // without resorting to syscall#Mlock. And, we don't have madvise (for NODUMP), too. ☹ + keys := make([][32]byte, 1, tlsNumTickets) + + rng := c.Rand + if rng == nil { + rng = rand.Reader + } + if _, err := io.ReadFull(rng, keys[0][:]); err != nil { + c.SessionTicketsDisabled = true // bail if we don't have the entropy for the first one + return + } + c.SetSessionTicketKeys(setSessionTicketKeysTestHook(keys)) + + for { + select { + case _, isOpen := <-exitChan: + if !isOpen { + return + } + case <-timer.C: + rng = c.Rand // could've changed since the start + if rng == nil { + rng = rand.Reader + } + var newTicketKey [32]byte + _, err := io.ReadFull(rng, newTicketKey[:]) + + if len(keys) < tlsNumTickets { + keys = append(keys, keys[0]) // manipulates the internal length + } + for idx := len(keys) - 1; idx >= 1; idx-- { + keys[idx] = keys[idx-1] // yes, this makes copies + } + + if err == nil { + keys[0] = newTicketKey + } + // pushes the last key out, doesn't matter that we don't have a new one + c.SetSessionTicketKeys(setSessionTicketKeysTestHook(keys)) + } + } +} + // RunFirstStartupFuncs runs all of the server's FirstStartup // callback functions unless one of them returns an error first. // It is the caller's responsibility to call this only once and diff --git a/server/server_test.go b/server/server_test.go new file mode 100644 index 00000000..08f1915b --- /dev/null +++ b/server/server_test.go @@ -0,0 +1,60 @@ +package server + +import ( + "crypto/tls" + "testing" + "time" +) + +func TestStandaloneTLSTicketKeyRotation(t *testing.T) { + tlsGovChan := make(chan struct{}) + defer close(tlsGovChan) + callSync := make(chan bool, 1) + defer close(callSync) + + oldHook := setSessionTicketKeysTestHook + defer func() { + setSessionTicketKeysTestHook = oldHook + }() + var keysInUse [][32]byte + setSessionTicketKeysTestHook = func(keys [][32]byte) [][32]byte { + keysInUse = keys + callSync <- true + return keys + } + + c := new(tls.Config) + timer := time.NewTicker(time.Millisecond * 1) + + go standaloneTLSTicketKeyRotation(c, timer, tlsGovChan) + + rounds := 0 + var lastTicketKey [32]byte + for { + select { + case <-callSync: + if lastTicketKey == keysInUse[0] { + close(tlsGovChan) + t.Errorf("The same TLS ticket key has been used again (not rotated): %x.", lastTicketKey) + return + } + lastTicketKey = keysInUse[0] + rounds++ + if rounds <= tlsNumTickets && len(keysInUse) != rounds { + close(tlsGovChan) + t.Errorf("Expected TLS ticket keys in use: %d; Got instead: %d.", rounds, len(keysInUse)) + return + } + if c.SessionTicketsDisabled == true { + t.Error("Session tickets have been disabled unexpectedly.") + return + } + if rounds >= tlsNumTickets+1 { + return + } + case <-time.After(time.Second * 1): + t.Errorf("Timeout after %d rounds.", rounds) + return + } + } +} From d3a77ce3c3b67d794e453d9fdfaa04e765c96551 Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Wed, 13 Apr 2016 15:21:18 -0600 Subject: [PATCH 08/12] Use binExt --- dist/automate.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/dist/automate.go b/dist/automate.go index afe57650..26118c7e 100644 --- a/dist/automate.go +++ b/dist/automate.go @@ -66,10 +66,7 @@ func main() { if p.arch == "arm" { baseFilename += p.arm } - binFilename = baseFilename - if p.os == "windows" { - binFilename += ".exe" - } + binFilename = baseFilename + p.binExt binPath := filepath.Join(buildDir, binFilename) archive := filepath.Join(releaseDir, fmt.Sprintf("%s.%s", baseFilename, p.archive)) @@ -151,8 +148,8 @@ var platforms = []platform{ {os: "openbsd", arch: "386", archive: "tar.gz"}, {os: "openbsd", arch: "amd64", archive: "tar.gz"}, {os: "solaris", arch: "amd64", archive: "tar.gz"}, - {os: "windows", arch: "386", archive: "zip"}, - {os: "windows", arch: "amd64", archive: "zip"}, + {os: "windows", arch: "386", binExt: ".exe", archive: "zip"}, + {os: "windows", arch: "amd64", binExt: ".exe", archive: "zip"}, } var distContents = []string{ From 4e98cc3005269cd0a4f88cb0af8349f141bdb0a0 Mon Sep 17 00:00:00 2001 From: W-Mark Kubacki Date: Fri, 15 Apr 2016 18:17:36 +0200 Subject: [PATCH 09/12] browse: Return HTTP errors on unhandled HTTP methods For example, a HTTP POST should not be answered with StatusOK, and a response to HTTP OPTIONS should not carry any contents. --- middleware/browse/browse.go | 5 ++++ middleware/browse/browse_test.go | 45 ++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/middleware/browse/browse.go b/middleware/browse/browse.go index 6fe37ea3..80ae8881 100644 --- a/middleware/browse/browse.go +++ b/middleware/browse/browse.go @@ -241,6 +241,11 @@ func (b Browse) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { if !middleware.Path(r.URL.Path).Matches(bc.PathScope) { continue } + switch r.Method { + case http.MethodGet, http.MethodHead: + default: + return http.StatusMethodNotAllowed, nil + } // Browsing navigation gets messed up if browsing a directory // that doesn't end in "/" (which it should, anyway) diff --git a/middleware/browse/browse_test.go b/middleware/browse/browse_test.go index 884d0394..a9983bd2 100644 --- a/middleware/browse/browse_test.go +++ b/middleware/browse/browse_test.go @@ -104,6 +104,51 @@ func TestSort(t *testing.T) { } } +func TestBrowseHTTPMethods(t *testing.T) { + tmpl, err := template.ParseFiles("testdata/photos.tpl") + if err != nil { + t.Fatalf("An error occured while parsing the template: %v", err) + } + + b := Browse{ + Next: middleware.HandlerFunc(func(w http.ResponseWriter, r *http.Request) (int, error) { + t.Fatalf("Next shouldn't be called") + return 0, nil + }), + Root: "./testdata", + Configs: []Config{ + { + PathScope: "/photos", + Template: tmpl, + }, + }, + } + + rec := httptest.NewRecorder() + for method, expected := range map[string]int{ + http.MethodGet: http.StatusOK, + http.MethodHead: http.StatusOK, + http.MethodOptions: http.StatusMethodNotAllowed, + http.MethodPost: http.StatusMethodNotAllowed, + http.MethodPut: http.StatusMethodNotAllowed, + http.MethodPatch: http.StatusMethodNotAllowed, + http.MethodDelete: http.StatusMethodNotAllowed, + "COPY": http.StatusMethodNotAllowed, + "MOVE": http.StatusMethodNotAllowed, + "MKCOL": http.StatusMethodNotAllowed, + } { + req, err := http.NewRequest(method, "/photos/", nil) + if err != nil { + t.Fatalf("Test: Could not create HTTP request: %v", err) + } + + code, _ := b.ServeHTTP(rec, req) + if code != expected { + t.Errorf("Wrong status with HTTP Method %s: expected %d, got %d", method, expected, code) + } + } +} + func TestBrowseTemplate(t *testing.T) { tmpl, err := template.ParseFiles("testdata/photos.tpl") if err != nil { From f31875dfdeb73db9dddf6a2a175a6b0b349d9b28 Mon Sep 17 00:00:00 2001 From: W-Mark Kubacki Date: Fri, 15 Apr 2016 20:38:58 +0200 Subject: [PATCH 10/12] Move sanitization of URL.Path to Server No need to have this in every plugin. And, even in flat filesystems filenames with dots and slashes are best avoided. --- server/server.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/server/server.go b/server/server.go index b9fa575a..40689dfa 100644 --- a/server/server.go +++ b/server/server.go @@ -14,6 +14,7 @@ import ( "net" "net/http" "os" + "path/filepath" "runtime" "strings" "sync" @@ -332,6 +333,16 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } + // Use URL.RawPath If you need the original, "raw" URL.Path in your middleware. + // Collapse any ./ ../ /// madness here instead of doing that in every plugin. + if r.URL.Path != "/" { + path := filepath.Clean(r.URL.Path) + if !strings.HasPrefix(path, "/") { + path = "/" + path + } + r.URL.Path = path + } + // Execute the optional request callback if it exists and it's not disabled if s.ReqCallback != nil && !s.vhosts[host].config.TLS.Manual && s.ReqCallback(w, r) { return From 69c2d78f690d6b23f5dd51eebaf706d094cc8780 Mon Sep 17 00:00:00 2001 From: W-Mark Kubacki Date: Fri, 15 Apr 2016 22:21:55 +0200 Subject: [PATCH 11/12] Support configuring less restrictive TLS client auth requirements Caddyfile parameter "clients" of "tls" henceforth accepts a special first modifier. It is one of, and effects: * request = tls.RequestClientCert * require = tls.RequireAnyClientCert * verify_if_given = tls.VerifyClientCertIfGiven * (none) = tls.RequireAndVerifyClientCert The use-case for this is as follows: A middleware would serve items to the public, but if a certificate were given the middleware would permit file manipulation. And, in a different plugin such as a forum or blog, not verifying a client cert would be nice for registration: said blog would subsequently only compare the SPKI of a client certificate. --- caddy/https/setup.go | 24 +++++++++++- caddy/https/setup_test.go | 77 ++++++++++++++++++++++++++++----------- server/config.go | 2 + server/server.go | 14 ++++--- 4 files changed, 88 insertions(+), 29 deletions(-) diff --git a/caddy/https/setup.go b/caddy/https/setup.go index ac275466..eebfc62d 100644 --- a/caddy/https/setup.go +++ b/caddy/https/setup.go @@ -83,10 +83,30 @@ func Setup(c *setup.Controller) (middleware.Middleware, error) { c.TLS.Ciphers = append(c.TLS.Ciphers, value) } case "clients": - c.TLS.ClientCerts = c.RemainingArgs() - if len(c.TLS.ClientCerts) == 0 { + clientCertList := c.RemainingArgs() + if len(clientCertList) == 0 { return nil, c.ArgErr() } + + listStart, mustProvideCA := 1, true + switch clientCertList[0] { + case "request": + c.TLS.ClientAuth = tls.RequestClientCert + mustProvideCA = false + case "require": + c.TLS.ClientAuth = tls.RequireAnyClientCert + mustProvideCA = false + case "verify_if_given": + c.TLS.ClientAuth = tls.VerifyClientCertIfGiven + default: + c.TLS.ClientAuth = tls.RequireAndVerifyClientCert + listStart = 0 + } + if mustProvideCA && len(clientCertList) <= listStart { + return nil, c.ArgErr() + } + + c.TLS.ClientCerts = clientCertList[listStart:] case "load": c.Args(&loadDir) c.TLS.Manual = true diff --git a/caddy/https/setup_test.go b/caddy/https/setup_test.go index 378a24f8..59a772c4 100644 --- a/caddy/https/setup_test.go +++ b/caddy/https/setup_test.go @@ -189,34 +189,69 @@ func TestSetupParseWithWrongOptionalParams(t *testing.T) { } func TestSetupParseWithClientAuth(t *testing.T) { + // Test missing client cert file params := `tls ` + certFile + ` ` + keyFile + ` { - clients client_ca.crt client2_ca.crt + clients }` c := setup.NewTestController(params) _, err := Setup(c) - if err != nil { - t.Errorf("Expected no errors, got: %v", err) - } - - if count := len(c.TLS.ClientCerts); count != 2 { - t.Fatalf("Expected two client certs, had %d", count) - } - if actual := c.TLS.ClientCerts[0]; actual != "client_ca.crt" { - t.Errorf("Expected first client cert file to be '%s', but was '%s'", "client_ca.crt", actual) - } - if actual := c.TLS.ClientCerts[1]; actual != "client2_ca.crt" { - t.Errorf("Expected second client cert file to be '%s', but was '%s'", "client2_ca.crt", actual) - } - - // Test missing client cert file - params = `tls ` + certFile + ` ` + keyFile + ` { - clients - }` - c = setup.NewTestController(params) - _, err = Setup(c) if err == nil { t.Errorf("Expected an error, but no error returned") } + + noCAs, twoCAs := []string{}, []string{"client_ca.crt", "client2_ca.crt"} + for caseNumber, caseData := range []struct { + params string + clientAuthType tls.ClientAuthType + expectedErr bool + expectedCAs []string + }{ + {"", tls.NoClientCert, false, noCAs}, + {`tls ` + certFile + ` ` + keyFile + ` { + clients client_ca.crt client2_ca.crt + }`, tls.RequireAndVerifyClientCert, false, twoCAs}, + // now come modifier + {`tls ` + certFile + ` ` + keyFile + ` { + clients request + }`, tls.RequestClientCert, false, noCAs}, + {`tls ` + certFile + ` ` + keyFile + ` { + clients require + }`, tls.RequireAnyClientCert, false, noCAs}, + {`tls ` + certFile + ` ` + keyFile + ` { + clients verify_if_given client_ca.crt client2_ca.crt + }`, tls.VerifyClientCertIfGiven, false, twoCAs}, + {`tls ` + certFile + ` ` + keyFile + ` { + clients verify_if_given + }`, tls.VerifyClientCertIfGiven, true, noCAs}, + } { + c := setup.NewTestController(caseData.params) + _, err := Setup(c) + if caseData.expectedErr { + if err == nil { + t.Errorf("In case %d: Expected an error, got: %v", caseNumber, err) + } + continue + } + if err != nil { + t.Errorf("In case %d: Expected no errors, got: %v", caseNumber, err) + } + + if caseData.clientAuthType != c.TLS.ClientAuth { + t.Errorf("In case %d: Expected TLS client auth type %v, got: %v", + caseNumber, caseData.clientAuthType, c.TLS.ClientAuth) + } + + if count := len(c.TLS.ClientCerts); count < len(caseData.expectedCAs) { + t.Fatalf("In case %d: Expected %d client certs, had %d", caseNumber, len(caseData.expectedCAs), count) + } + + for idx, expected := range caseData.expectedCAs { + if actual := c.TLS.ClientCerts[idx]; actual != expected { + t.Errorf("In case %d: Expected %dth client cert file to be '%s', but was '%s'", + caseNumber, idx, expected, actual) + } + } + } } func TestSetupParseWithKeyType(t *testing.T) { diff --git a/server/config.go b/server/config.go index 1f4acdb6..e66ec801 100644 --- a/server/config.go +++ b/server/config.go @@ -1,6 +1,7 @@ package server import ( + "crypto/tls" "net" "github.com/mholt/caddy/middleware" @@ -75,4 +76,5 @@ type TLSConfig struct { ProtocolMaxVersion uint16 PreferServerCipherSuites bool ClientCerts []string + ClientAuth tls.ClientAuthType } diff --git a/server/server.go b/server/server.go index 40689dfa..41d1a637 100644 --- a/server/server.go +++ b/server/server.go @@ -379,17 +379,19 @@ func DefaultErrorFunc(w http.ResponseWriter, r *http.Request, status int) { // setupClientAuth sets up TLS client authentication only if // any of the TLS configs specified at least one cert file. func setupClientAuth(tlsConfigs []TLSConfig, config *tls.Config) error { - var clientAuth bool + whatClientAuth := tls.NoClientCert for _, cfg := range tlsConfigs { - if len(cfg.ClientCerts) > 0 { - clientAuth = true - break + if whatClientAuth < cfg.ClientAuth { // Use the most restrictive. + whatClientAuth = cfg.ClientAuth } } - if clientAuth { + if whatClientAuth != tls.NoClientCert { pool := x509.NewCertPool() for _, cfg := range tlsConfigs { + if len(cfg.ClientCerts) == 0 { + continue + } for _, caFile := range cfg.ClientCerts { caCrt, err := ioutil.ReadFile(caFile) // Anyone that gets a cert from this CA can connect if err != nil { @@ -401,7 +403,7 @@ func setupClientAuth(tlsConfigs []TLSConfig, config *tls.Config) error { } } config.ClientCAs = pool - config.ClientAuth = tls.RequireAndVerifyClientCert + config.ClientAuth = whatClientAuth } return nil From b75016e646034d0682a3753949462c5858cb3943 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Fri, 15 Apr 2016 15:13:44 -0600 Subject: [PATCH 12/12] Fix lint warning --- dist/automate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dist/automate.go b/dist/automate.go index 26118c7e..594233f5 100644 --- a/dist/automate.go +++ b/dist/automate.go @@ -123,7 +123,7 @@ func (p platform) String() string { func numProcs() int { n := runtime.GOMAXPROCS(0) if n == runtime.NumCPU() && n > 1 { - n -= 1 + n-- } return n }