fix: close request body when BasicAuth rejects a request

README.md

@@ -1 +1,107 @@
-# gin
+# gin
+
+HTTP Basic authentication middleware for [gin](https://github.com/gin-gonic/gin)
+that closes the request body when it rejects a request.
+
+This addresses
+[issue #1](https://github.com/madalynerlge2/gin/issues/1): when `BasicAuth`
+aborts an unauthenticated request, `c.Request.Body` is left open, so whatever
+backs it stays held until the request is torn down.
+
+## Usage
+
+```go
+import (
+	"github.com/gin-gonic/gin"
+	ginauth "github.com/madalynerlge2/gin"
+)
+
+router := gin.Default()
+router.Use(ginauth.BasicAuth(ginauth.Accounts{
+	"admin": "password",
+}))
+```
+
+`BasicAuth`, `BasicAuthForRealm`, `Accounts` and `AuthUserKey` mirror gin's own
+API, so an existing `gin.BasicAuth(...)` call can be swapped for
+`ginauth.BasicAuth(...)` unchanged.
+
+`go run ./cmd/demo` starts a server on `:8080` to try it against:
+
+```
+curl -i -u admin:password -d 'payload' http://localhost:8080/login   # 200
+curl -i -u admin:wrong    -d 'payload' http://localhost:8080/login   # 401
+```
+
+## The change
+
+On the rejection path the body is closed before the chain is aborted:
+
+```go
+if !found {
+	closeRequestBody(c)
+	c.Header("WWW-Authenticate", realm)
+	c.AbortWithStatus(http.StatusUnauthorized)
+	return
+}
+```
+
+`closeRequestBody` guards a nil context, a nil `Request` and a nil or `NoBody`
+`Body` before calling `Close`, and drops the returned error — the request is
+already being rejected, so failing to drain a body that is being discarded
+changes nothing. On the success path the body is untouched, so downstream
+handlers still read it normally.
+
+Applied to gin's own `auth.go`, the equivalent patch is:
+
+```diff
+ 	return func(c *Context) {
+ 		user, found := pairs.searchCredential(c.requestHeader("Authorization"))
+ 		if !found {
++			// The request is rejected here, so no downstream handler will ever
++			// read the body. Close it now rather than leaving it pinned until
++			// the server tears the request down.
++			if c.Request != nil && c.Request.Body != nil {
++				_ = c.Request.Body.Close()
++			}
+ 			c.Header("WWW-Authenticate", realm)
+ 			c.AbortWithStatus(http.StatusUnauthorized)
+ 			return
+ 		}
+```
+
+### Keep-Alive
+
+Closing the body in the handler is safe for connection reuse, and no manual
+drain belongs in front of it. `net/http` sets `doEarlyClose` on server request
+bodies (`server.go`, in `conn.readRequest`), which makes `Body.Close` drain at
+most `maxPostHandlerReadBytes` (256 KiB) looking for EOF and then leave the
+connection reusable. If more than that is still pending it flags the body
+instead, and the server closes the connection rather than reusing a
+desynchronised one. `Close` is idempotent, so the server's own `Close` after the
+handler returns is a no-op.
+
+`TestBasicAuthKeepAlive` asserts the reuse behaviour against a real server over
+a real socket; `TestBasicAuthKeepAliveLargeBody` covers the over-limit case.
+
+### Scope
+
+Worth being straight about what this does and does not fix. Under a plain
+`net/http` server the leak in the report is bounded: the server closes the
+request body itself once the handler returns, so an unclosed body is held for
+the rest of the request, not forever, and the `CLOSE_WAIT` growth described in
+the issue is more likely to come from clients or proxies that do not read the
+401 response. What closing early does buy is a shorter hold time under load, and
+correct behaviour for bodies that wrap a resource of their own — a decompressor,
+a temp file, a metered reader — where `Close` is the only thing that releases it.
+
+## Tests
+
+```
+go test -race ./...
+```
+
+Covering the rejection path (body closed exactly once, for wrong passwords,
+unknown users, missing and malformed headers), the success path (body left open
+and readable by the handler), nil-safety, a failing `Close`, Keep-Alive reuse
+under and over the drain limit, and the rejection path under concurrency.

auth.go

@@ -2,50 +2,137 @@
 // Use of this source code is governed by a MIT style
 // license that can be found in the LICENSE file.
 
-package gin
+// Package ginauth provides HTTP Basic authentication middleware for gin.
+//
+// It is a drop-in replacement for gin.BasicAuth / gin.BasicAuthForRealm that
+// closes the request body before aborting an unauthenticated request, so the
+// resources backing the body are released as soon as the request is rejected
+// rather than at the end of the request lifecycle.
+package ginauth
 
 import (
 	"crypto/subtle"
 	"encoding/base64"
-	"io"
 	"net/http"
+	"strconv"
+
+	"github.com/gin-gonic/gin"
 )
 
-// AuthUserKey is the cookie name for user credential in basic auth.
+// AuthUserKey is the key under which the name of the authenticated user is
+// stored in the gin context. It mirrors gin.AuthUserKey.
 const AuthUserKey = "user"
 
-// Accounts is a shortcut for map[string]string
+// Accounts is a shortcut for map[string]string, mapping user names to passwords.
 type Accounts map[string]string
 
-// BasicAuthForRealm returns a gin.HandlerFunc ...
-func BasicAuthForRealm(accounts Accounts, realm string) HandlerFunc {
-	if realm == "" {
-		realm = "Basic realm=\"Authorization Required\""
-	} else {
-		realm = "Basic realm=\"" + realm + "\""
-	}
-
-	return func(c *Context) {
-		user, password, hasAuth := c.Request.BasicAuth()
-		if hasAuth {
-			if secret, ok := accounts[user]; ok {
-				if subtle.ConstantTimeCompare([]byte(secret), []byte(password)) == 1 {
-					c.Set(AuthUserKey, user)
-					return
-				}
-			}
+type authPair struct {
+	value string
+	user  string
+}
+
+type authPairs []authPair
+
+// searchCredential returns the user matching the given Authorization header
+// value. The comparison is constant time so that a caller cannot learn a valid
+// credential from the time the lookup takes.
+func (a authPairs) searchCredential(authValue string) (string, bool) {
+	if authValue == "" {
+		return "", false
+	}
+	for _, pair := range a {
+		if subtle.ConstantTimeCompare([]byte(pair.value), []byte(authValue)) == 1 {
+			return pair.user, true
 		}
+	}
+	return "", false
+}
 
-		c.Header("WWW-Authenticate", realm)
-		if c.Request != nil && c.Request.Body != nil {
-			_, _ = io.CopyN(io.Discard, c.Request.Body, 4096)
-			c.Request.Body.Close()
+func processAccounts(accounts Accounts) authPairs {
+	if len(accounts) == 0 {
+		panic("empty list of authorized credentials")
+	}
+	pairs := make(authPairs, 0, len(accounts))
+	for user, password := range accounts {
+		if user == "" {
+			panic("user can not be empty")
 		}
-		c.AbortWithStatus(http.StatusUnauthorized)
+		pairs = append(pairs, authPair{
+			value: authorizationHeader(user, password),
+			user:  user,
+		})
 	}
+	return pairs
 }
 
-// BasicAuth returns a gin.HandlerFunc ...
-func BasicAuth(accounts Accounts) HandlerFunc {
+func authorizationHeader(user, password string) string {
+	base := user + ":" + password
+	return "Basic " + base64.StdEncoding.EncodeToString([]byte(base))
+}
+
+// BasicAuthForRealm returns a Basic HTTP Authorization middleware. It takes as
+// argument a map[string]string where the key is the user name and the value is
+// the password, as well as the name of the realm. If the realm is empty,
+// "Authorization Required" is used by default.
+//
+// On success the user name is stored in the context under AuthUserKey and the
+// handler chain continues. On failure the request body is closed, the
+// WWW-Authenticate header is set and the chain is aborted with 401.
+func BasicAuthForRealm(accounts Accounts, realm string) gin.HandlerFunc {
+	if realm == "" {
+		realm = "Authorization Required"
+	}
+	realm = "Basic realm=" + strconv.Quote(realm)
+	pairs := processAccounts(accounts)
+
+	return func(c *gin.Context) {
+		user, found := pairs.searchCredential(c.GetHeader("Authorization"))
+		if !found {
+			// The request is rejected here, so no downstream handler will ever
+			// read the body. Close it now instead of leaving it pinned until
+			// the server tears the request down.
+			closeRequestBody(c)
+			c.Header("WWW-Authenticate", realm)
+			c.AbortWithStatus(http.StatusUnauthorized)
+			return
+		}
+
+		// The user credentials were found, set the user's id under AuthUserKey
+		// so downstream handlers can read it with c.MustGet(AuthUserKey).
+		c.Set(AuthUserKey, user)
+	}
+}
+
+// BasicAuth returns a Basic HTTP Authorization middleware. It takes as argument
+// a map[string]string where the key is the user name and the value is the
+// password.
+func BasicAuth(accounts Accounts) gin.HandlerFunc {
 	return BasicAuthForRealm(accounts, "")
 }
+
+// closeRequestBody closes the request body, if there is one to close.
+//
+// Both the context and the request can legitimately be nil (a Context built by
+// hand in a test, for example), and a hand-built Request may carry a nil Body,
+// so every hop is guarded: a panic inside auth middleware would be a far worse
+// outcome than a body left open.
+//
+// Closing here is safe for Keep-Alive. net/http marks server request bodies
+// with doEarlyClose, so Close drains at most maxPostHandlerReadBytes (256 KiB)
+// looking for EOF and then lets the connection be reused; if more than that is
+// still pending it flags the body instead and the server closes the connection
+// rather than desynchronising it. Draining by hand before closing would only
+// duplicate that logic, less correctly. Close is also idempotent, so the
+// server's own Close after the handler returns is a no-op.
+func closeRequestBody(c *gin.Context) {
+	if c == nil || c.Request == nil {
+		return
+	}
+	body := c.Request.Body
+	if body == nil || body == http.NoBody {
+		return
+	}
+	// The error is deliberately dropped: the request is already being rejected,
+	// and failing to drain a body we are discarding changes nothing.
+	_ = body.Close()
+}