From 6d97d8d87beb788d19a4084d07ec9157e5705b13 Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Tue, 23 Apr 2024 20:05:57 -0400 Subject: caddyhttp: Address some Go 1.20 features (#6252) Co-authored-by: Francis Lavoie --- cmd/x509rootsfallback.go | 33 ++++++++++++++++++++++++++++ go.mod | 1 + go.sum | 2 ++ modules/caddyhttp/caddyhttp.go | 13 +++++++++-- modules/caddyhttp/caddyhttp_test.go | 26 ++++++++++++++++------ modules/caddyhttp/requestbody/caddyfile.go | 26 +++++++++++++++++++++- modules/caddyhttp/requestbody/requestbody.go | 30 +++++++++++++++++++++++++ modules/caddyhttp/responsewriter_test.go | 10 ++++----- 8 files changed, 125 insertions(+), 16 deletions(-) create mode 100644 cmd/x509rootsfallback.go diff --git a/cmd/x509rootsfallback.go b/cmd/x509rootsfallback.go new file mode 100644 index 00000000..935a48ec --- /dev/null +++ b/cmd/x509rootsfallback.go @@ -0,0 +1,33 @@ +// Copyright 2015 Matthew Holt and The Caddy Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package caddycmd + +import ( + // For running in minimal environments, this can ease + // headaches related to establishing TLS connections. + // "Package fallback embeds a set of fallback X.509 trusted + // roots in the application by automatically invoking + // x509.SetFallbackRoots. This allows the application to + // work correctly even if the operating system does not + // provide a verifier or system roots pool. ... It's + // recommended that only binaries, and not libraries, + // import this package. This package must be kept up to + // date for security and compatibility reasons." + // + // This is in its own file only because of conflicts + // between gci and goimports when in main.go. + // See https://github.com/daixiang0/gci/issues/76 + _ "golang.org/x/crypto/x509roots/fallback" +) diff --git a/go.mod b/go.mod index c3dc1568..6c4fed3c 100644 --- a/go.mod +++ b/go.mod @@ -36,6 +36,7 @@ require ( go.uber.org/zap v1.27.0 go.uber.org/zap/exp v0.2.0 golang.org/x/crypto v0.22.0 + golang.org/x/crypto/x509roots/fallback v0.0.0-20240416174822-0da2a6a1bbc8 golang.org/x/net v0.24.0 golang.org/x/sync v0.7.0 golang.org/x/term v0.19.0 diff --git a/go.sum b/go.sum index bd298867..14434b24 100644 --- a/go.sum +++ b/go.sum @@ -507,6 +507,8 @@ golang.org/x/crypto v0.3.0/go.mod h1:hebNnKkNXi2UzZN1eVRvBB7co0a+JxK6XbPiWVs/3J4 golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/crypto v0.22.0 h1:g1v0xeRhjcugydODzvb3mEM9SQ0HGp9s/nh3COQ/C30= golang.org/x/crypto v0.22.0/go.mod h1:vr6Su+7cTlO45qkww3VDJlzDn0ctJvRgYbC2NvXHt+M= +golang.org/x/crypto/x509roots/fallback v0.0.0-20240416174822-0da2a6a1bbc8 h1:WsZ1vq1wEPrhLWtbblOEshIKDAbG0sYGgbYCFInT0QM= +golang.org/x/crypto/x509roots/fallback v0.0.0-20240416174822-0da2a6a1bbc8/go.mod h1:kNa9WdvYnzFwC79zRpLRMJbdEFlhyM5RPFBBZp/wWH8= golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8 h1:aAcj0Da7eBAtrTp03QXWvm88pSyOt+UgdZw2BFZ+lEw= golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8/go.mod h1:CQ1k9gNrJ50XIzaKCRR2hssIjF07kZFEiieALBM/ARQ= golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go index f15aec5e..26b21b54 100644 --- a/modules/caddyhttp/caddyhttp.go +++ b/modules/caddyhttp/caddyhttp.go @@ -226,13 +226,22 @@ func StatusCodeMatches(actual, configured int) bool { // in the implementation of http.Dir. The root is assumed to // be a trusted path, but reqPath is not; and the output will // never be outside of root. The resulting path can be used -// with the local file system. +// with the local file system. If root is empty, the current +// directory is assumed. If the cleaned request path is deemed +// not local according to lexical processing (i.e. ignoring links), +// it will be rejected as unsafe and only the root will be returned. func SanitizedPathJoin(root, reqPath string) string { if root == "" { root = "." } - path := filepath.Join(root, path.Clean("/"+reqPath)) + relPath := path.Clean("/" + reqPath)[1:] // clean path and trim the leading / + if !filepath.IsLocal(relPath) { + // path is unsafe (see https://github.com/golang/go/issues/56336#issuecomment-1416214885) + return root + } + + path := filepath.Join(root, filepath.FromSlash(relPath)) // filepath.Join also cleans the path, and cleaning strips // the trailing slash, so we need to re-add it afterwards. diff --git a/modules/caddyhttp/caddyhttp_test.go b/modules/caddyhttp/caddyhttp_test.go index 84c0271f..4763c383 100644 --- a/modules/caddyhttp/caddyhttp_test.go +++ b/modules/caddyhttp/caddyhttp_test.go @@ -3,6 +3,7 @@ package caddyhttp import ( "net/url" "path/filepath" + "runtime" "testing" ) @@ -12,9 +13,10 @@ func TestSanitizedPathJoin(t *testing.T) { // %2f = / // %5c = \ for i, tc := range []struct { - inputRoot string - inputPath string - expect string + inputRoot string + inputPath string + expect string + expectWindows string }{ { inputPath: "", @@ -63,7 +65,7 @@ func TestSanitizedPathJoin(t *testing.T) { { inputRoot: "/a/b", inputPath: "/%2e%2e%2f%2e%2e%2f", - expect: filepath.Join("/", "a", "b") + separator, + expect: "/a/b", // inputPath fails the IsLocal test so only the root is returned }, { inputRoot: "/a/b", @@ -81,9 +83,16 @@ func TestSanitizedPathJoin(t *testing.T) { expect: filepath.Join("C:\\www", "foo", "bar"), }, { - inputRoot: "C:\\www", - inputPath: "/D:\\foo\\bar", - expect: filepath.Join("C:\\www", "D:\\foo\\bar"), + inputRoot: "C:\\www", + inputPath: "/D:\\foo\\bar", + expect: filepath.Join("C:\\www", "D:\\foo\\bar"), + expectWindows: filepath.Join("C:\\www"), // inputPath fails IsLocal on Windows + }, + { + // https://github.com/golang/go/issues/56336#issuecomment-1416214885 + inputRoot: "root", + inputPath: "/a/b/../../c", + expect: filepath.Join("root", "c"), }, } { // we don't *need* to use an actual parsed URL, but it @@ -96,6 +105,9 @@ func TestSanitizedPathJoin(t *testing.T) { t.Fatalf("Test %d: invalid URL: %v", i, err) } actual := SanitizedPathJoin(tc.inputRoot, u.Path) + if runtime.GOOS == "windows" && tc.expectWindows != "" { + tc.expect = tc.expectWindows + } if actual != tc.expect { t.Errorf("Test %d: SanitizedPathJoin('%s', '%s') => '%s' (expected '%s')", i, tc.inputRoot, tc.inputPath, actual, tc.expect) diff --git a/modules/caddyhttp/requestbody/caddyfile.go b/modules/caddyhttp/requestbody/caddyfile.go index d2956cae..8378ad7f 100644 --- a/modules/caddyhttp/requestbody/caddyfile.go +++ b/modules/caddyhttp/requestbody/caddyfile.go @@ -15,6 +15,8 @@ package requestbody import ( + "time" + "github.com/dustin/go-humanize" "github.com/caddyserver/caddy/v2/caddyconfig/httpcaddyfile" @@ -44,8 +46,30 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) } rb.MaxSize = int64(size) + case "read_timeout": + var timeoutStr string + if !h.AllArgs(&timeoutStr) { + return nil, h.ArgErr() + } + timeout, err := time.ParseDuration(timeoutStr) + if err != nil { + return nil, h.Errf("parsing read_timeout: %v", err) + } + rb.ReadTimeout = timeout + + case "write_timeout": + var timeoutStr string + if !h.AllArgs(&timeoutStr) { + return nil, h.ArgErr() + } + timeout, err := time.ParseDuration(timeoutStr) + if err != nil { + return nil, h.Errf("parsing write_timeout: %v", err) + } + rb.WriteTimeout = timeout + default: - return nil, h.Errf("unrecognized servers option '%s'", h.Val()) + return nil, h.Errf("unrecognized request_body subdirective '%s'", h.Val()) } } diff --git a/modules/caddyhttp/requestbody/requestbody.go b/modules/caddyhttp/requestbody/requestbody.go index dfc0fd92..d00455a8 100644 --- a/modules/caddyhttp/requestbody/requestbody.go +++ b/modules/caddyhttp/requestbody/requestbody.go @@ -17,6 +17,9 @@ package requestbody import ( "io" "net/http" + "time" + + "go.uber.org/zap" "github.com/caddyserver/caddy/v2" "github.com/caddyserver/caddy/v2/modules/caddyhttp" @@ -31,6 +34,14 @@ type RequestBody struct { // The maximum number of bytes to allow reading from the body by a later handler. // If more bytes are read, an error with HTTP status 413 is returned. MaxSize int64 `json:"max_size,omitempty"` + + // EXPERIMENTAL. Subject to change/removal. + ReadTimeout time.Duration `json:"read_timeout,omitempty"` + + // EXPERIMENTAL. Subject to change/removal. + WriteTimeout time.Duration `json:"write_timeout,omitempty"` + + logger *zap.Logger } // CaddyModule returns the Caddy module information. @@ -41,6 +52,11 @@ func (RequestBody) CaddyModule() caddy.ModuleInfo { } } +func (rb *RequestBody) Provision(ctx caddy.Context) error { + rb.logger = ctx.Logger() + return nil +} + func (rb RequestBody) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error { if r.Body == nil { return next.ServeHTTP(w, r) @@ -48,6 +64,20 @@ func (rb RequestBody) ServeHTTP(w http.ResponseWriter, r *http.Request, next cad if rb.MaxSize > 0 { r.Body = errorWrapper{http.MaxBytesReader(w, r.Body, rb.MaxSize)} } + if rb.ReadTimeout > 0 || rb.WriteTimeout > 0 { + //nolint:bodyclose + rc := http.NewResponseController(w) + if rb.ReadTimeout > 0 { + if err := rc.SetReadDeadline(time.Now().Add(rb.ReadTimeout)); err != nil { + rb.logger.Error("could not set read deadline", zap.Error(err)) + } + } + if rb.WriteTimeout > 0 { + if err := rc.SetWriteDeadline(time.Now().Add(rb.WriteTimeout)); err != nil { + rb.logger.Error("could not set write deadline", zap.Error(err)) + } + } + } return next.ServeHTTP(w, r) } diff --git a/modules/caddyhttp/responsewriter_test.go b/modules/caddyhttp/responsewriter_test.go index 492fcad9..c08ad26a 100644 --- a/modules/caddyhttp/responsewriter_test.go +++ b/modules/caddyhttp/responsewriter_test.go @@ -2,7 +2,6 @@ package caddyhttp import ( "bytes" - "fmt" "io" "net/http" "strings" @@ -75,20 +74,19 @@ func TestResponseWriterWrapperReadFrom(t *testing.T) { // take precedence over our ReadFrom. src := struct{ io.Reader }{strings.NewReader(srcData)} - fmt.Println(name) if _, err := io.Copy(wrapped, src); err != nil { - t.Errorf("Copy() err = %v", err) + t.Errorf("%s: Copy() err = %v", name, err) } if got := tt.responseWriter.Written(); got != srcData { - t.Errorf("data = %q, want %q", got, srcData) + t.Errorf("%s: data = %q, want %q", name, got, srcData) } if tt.responseWriter.CalledReadFrom() != tt.wantReadFrom { if tt.wantReadFrom { - t.Errorf("ReadFrom() should have been called") + t.Errorf("%s: ReadFrom() should have been called", name) } else { - t.Errorf("ReadFrom() should not have been called") + t.Errorf("%s: ReadFrom() should not have been called", name) } } }) -- cgit v1.2.3-70-g09d2