Skip to content

Commit fea8a1c

Browse files
authored
fix(auth): close a certless connection when the API refuses it (#94)
The web surface is anonymous and shares its listener with the API, so a browser's first handshake can complete without a client certificate and HTTP/2 then reuses that connection for every API call, each refused with 401 even after a certificate becomes available. The API still refuses a request with no peer certificate, but now also answers with Connection: close, which makes the server send GOAWAY on HTTP/2 so the client's next request performs a fresh handshake where a certificate can be offered. Authenticated connections are untouched. The refusal is logged with the caller's address, since until now the most common failure left no trace at all. Closes #77
1 parent 5078585 commit fea8a1c

4 files changed

Lines changed: 225 additions & 1 deletion

File tree

‎README.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,10 @@ redirect will send browsers to a port nothing is listening on.
5151

5252
The web surface itself is reachable without an operator certificate: it serves a landing
5353
page with a Log in action, and every API call still requires a certificate that verifies
54-
against the operator CA.
54+
against the operator CA. Because both share one listener, a browser may open its
55+
connection for the page before it offers a certificate; an API call that arrives on such a
56+
connection is refused and the connection is closed, so the next attempt makes a fresh TLS
57+
handshake. The refusal is logged (`authz: refused ...`) with the caller's address.
5558

5659
**Everything under the mount must be readable by uid 65532.** The final image stage is
5760
`gcr.io/distroless/static-debian12:nonroot`, so the process runs as that uid, and that

‎cmd/manager/main_test.go‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,17 @@ import (
2525
"crypto/tls"
2626
"crypto/x509"
2727
"crypto/x509/pkix"
28+
"encoding/asn1"
2829
"encoding/json"
2930
"encoding/pem"
31+
"io"
3032
"math/big"
3133
"net/http"
3234
"net/http/httptest"
35+
"net/http/httptrace"
3336
"os"
3437
"path/filepath"
38+
"strconv"
3539
"strings"
3640
"testing"
3741
"time"
@@ -404,3 +408,179 @@ func TestRootHandler_VersionIsAnonymous(t *testing.T) {
404408
t.Errorf("body = %q, want the build info rather than the SPA", rec.Body.String())
405409
}
406410
}
411+
412+
// writeOperatorCA writes a self-signed operator CA to dir and returns its path
413+
// with the parsed certificate and key, so a test can issue operator leaves.
414+
func writeOperatorCA(t *testing.T, dir string) (string, *x509.Certificate, *ecdsa.PrivateKey) {
415+
t.Helper()
416+
key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
417+
if err != nil {
418+
t.Fatal(err)
419+
}
420+
tmpl := &x509.Certificate{
421+
SerialNumber: big.NewInt(1),
422+
Subject: pkix.Name{CommonName: "operator CA"},
423+
NotBefore: time.Now().Add(-time.Hour),
424+
NotAfter: time.Now().Add(time.Hour),
425+
IsCA: true,
426+
BasicConstraintsValid: true,
427+
KeyUsage: x509.KeyUsageCertSign,
428+
}
429+
der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &key.PublicKey, key)
430+
if err != nil {
431+
t.Fatal(err)
432+
}
433+
ca, err := x509.ParseCertificate(der)
434+
if err != nil {
435+
t.Fatal(err)
436+
}
437+
path := filepath.Join(dir, "operator-ca.pem")
438+
if err := os.WriteFile(path, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), 0o600); err != nil {
439+
t.Fatal(err)
440+
}
441+
return path, ca, key
442+
}
443+
444+
// operatorLeaf issues a client certificate carrying the admin access level.
445+
func operatorLeaf(t *testing.T, ca *x509.Certificate, caKey *ecdsa.PrivateKey) tls.Certificate {
446+
t.Helper()
447+
key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
448+
if err != nil {
449+
t.Fatal(err)
450+
}
451+
value, err := authz.MarshalLevelValue(authz.LevelAdmin)
452+
if err != nil {
453+
t.Fatal(err)
454+
}
455+
oid, err := asn1ObjectIdentifier(authz.AccessLevelOID)
456+
if err != nil {
457+
t.Fatal(err)
458+
}
459+
tmpl := &x509.Certificate{
460+
SerialNumber: big.NewInt(2),
461+
Subject: pkix.Name{CommonName: "op@acme.example"},
462+
NotBefore: time.Now().Add(-time.Hour),
463+
NotAfter: time.Now().Add(time.Hour),
464+
KeyUsage: x509.KeyUsageDigitalSignature,
465+
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
466+
ExtraExtensions: []pkix.Extension{{Id: oid, Value: value}},
467+
}
468+
der, err := x509.CreateCertificate(rand.Reader, tmpl, ca, &key.PublicKey, caKey)
469+
if err != nil {
470+
t.Fatal(err)
471+
}
472+
return tls.Certificate{Certificate: [][]byte{der}, PrivateKey: key}
473+
}
474+
475+
// asn1ObjectIdentifier parses a dotted OID.
476+
func asn1ObjectIdentifier(dotted string) (asn1.ObjectIdentifier, error) {
477+
var oid asn1.ObjectIdentifier
478+
for _, part := range strings.Split(dotted, ".") {
479+
n, err := strconv.Atoi(part)
480+
if err != nil {
481+
return nil, err
482+
}
483+
oid = append(oid, n)
484+
}
485+
return oid, nil
486+
}
487+
488+
// connTracker records whether each request went out on a reused connection.
489+
func connTracker(req *http.Request, reused *bool) *http.Request {
490+
trace := &httptrace.ClientTrace{GotConn: func(info httptrace.GotConnInfo) { *reused = info.Reused }}
491+
return req.WithContext(httptrace.WithClientTrace(req.Context(), trace))
492+
}
493+
494+
// TestServe_CertlessConnectionIsNotReusedForTheAPI reproduces #77 over a real
495+
// TLS listener speaking HTTP/2 with the production TLS posture. The browser's
496+
// first request is for the anonymous web surface, so that handshake completes
497+
// with no client certificate. HTTP/2 then reuses the connection for the API,
498+
// which refuses it -- correctly, and it must keep doing so -- but a connection
499+
// with no certificate can never authenticate, so it must not stay open for the
500+
// next API call either. The server closes it, and the client's next request
501+
// performs a fresh handshake where the certificate can be offered.
502+
func TestServe_CertlessConnectionIsNotReusedForTheAPI(t *testing.T) {
503+
caPath, ca, caKey := writeOperatorCA(t, t.TempDir())
504+
tlsCfg, err := buildTLSConfig(config.Config{OperatorCAPath: caPath})
505+
if err != nil {
506+
t.Fatalf("buildTLSConfig: %v", err)
507+
}
508+
509+
const apiPath = "/cryptos.fleet.v1.FleetService/"
510+
srv := httptest.NewUnstartedServer(newRootHandler(
511+
apiPath,
512+
stubHandler(http.StatusOK, "api"),
513+
stubHandler(http.StatusOK, "spa"),
514+
authz.ClientCertMiddleware,
515+
nil,
516+
))
517+
srv.EnableHTTP2 = true
518+
srv.TLS = tlsCfg
519+
srv.StartTLS()
520+
t.Cleanup(srv.Close)
521+
522+
do := func(t *testing.T, c *http.Client, method, path string) (int, bool) {
523+
t.Helper()
524+
var reused bool
525+
req, err := http.NewRequest(method, srv.URL+path, nil)
526+
if err != nil {
527+
t.Fatal(err)
528+
}
529+
resp, err := c.Do(connTracker(req, &reused))
530+
if err != nil {
531+
t.Fatalf("%s %s: %v", method, path, err)
532+
}
533+
_, _ = io.Copy(io.Discard, resp.Body)
534+
_ = resp.Body.Close()
535+
if resp.ProtoMajor != 2 {
536+
t.Fatalf("%s %s: proto = %s, want HTTP/2", method, path, resp.Proto)
537+
}
538+
return resp.StatusCode, reused
539+
}
540+
541+
t.Run("no certificate", func(t *testing.T) {
542+
c := srv.Client()
543+
t.Cleanup(c.CloseIdleConnections)
544+
545+
if code, _ := do(t, c, http.MethodGet, "/"); code != http.StatusOK {
546+
t.Fatalf("GET / = %d, want 200", code)
547+
}
548+
code, reused := do(t, c, http.MethodPost, apiPath+"WhoAmI")
549+
if code != http.StatusUnauthorized {
550+
t.Fatalf("API = %d, want 401", code)
551+
}
552+
if !reused {
553+
t.Fatal("API call did not reuse the web connection; the test is not exercising #77")
554+
}
555+
// The refused connection is gone: the next request handshakes again.
556+
code, reused = do(t, c, http.MethodPost, apiPath+"WhoAmI")
557+
if code != http.StatusUnauthorized {
558+
t.Fatalf("API retry = %d, want 401", code)
559+
}
560+
if reused {
561+
t.Error("API retry reused the certless connection, want a fresh handshake")
562+
}
563+
})
564+
565+
t.Run("with certificate", func(t *testing.T) {
566+
c := srv.Client()
567+
tr := c.Transport.(*http.Transport).Clone()
568+
tr.TLSClientConfig.Certificates = []tls.Certificate{operatorLeaf(t, ca, caKey)}
569+
c = &http.Client{Transport: tr}
570+
t.Cleanup(c.CloseIdleConnections)
571+
572+
if code, _ := do(t, c, http.MethodGet, "/"); code != http.StatusOK {
573+
t.Fatalf("GET / = %d, want 200", code)
574+
}
575+
// An authenticated connection is left alone and keeps being reused.
576+
for range 2 {
577+
code, reused := do(t, c, http.MethodPost, apiPath+"WhoAmI")
578+
if code != http.StatusOK {
579+
t.Fatalf("API = %d, want 200", code)
580+
}
581+
if !reused {
582+
t.Error("authenticated API call did not reuse its connection")
583+
}
584+
}
585+
})
586+
}

‎internal/authz/middleware.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ limitations under the License.
1919
*/
2020

2121
import (
22+
"log"
2223
"math/big"
2324
"net/http"
2425
"strings"
@@ -50,6 +51,15 @@ func ClientCertMiddleware(next http.Handler) http.Handler {
5051
func ClientCertMiddlewareWithRevocation(revoker serialRevoker, next http.Handler) http.Handler {
5152
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
5253
if r.TLS == nil || len(r.TLS.PeerCertificates) == 0 {
54+
// A connection whose handshake carried no certificate can never
55+
// authenticate, yet HTTP/2 would keep reusing it for every later
56+
// API call -- typically because it was opened for the anonymous web
57+
// surface (#77). Close it (GOAWAY on h2) so the client's next
58+
// attempt makes a fresh handshake where a certificate can be
59+
// offered. The request is refused either way.
60+
w.Header().Set("Connection", "close")
61+
log.Printf("authz: refused %s %q from %s: the TLS connection carries no client certificate; closing it so the client re-handshakes",
62+
r.Method, r.URL.Path, r.RemoteAddr)
5363
http.Error(w, "client certificate required", http.StatusUnauthorized)
5464
return
5565
}

‎internal/authz/middleware_test.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,37 @@ func TestClientCertMiddleware_NoCert401(t *testing.T) {
8686
}
8787
}
8888

89+
// TestClientCertMiddleware_NoCertClosesConnection pins #77. A connection whose
90+
// handshake carried no client certificate can never authenticate, and HTTP/2
91+
// would otherwise keep reusing it for every later API call. Refusing with
92+
// Connection: close makes the server tear it down (GOAWAY on h2) so the client's
93+
// next attempt performs a fresh handshake, where a certificate can be offered.
94+
func TestClientCertMiddleware_NoCertClosesConnection(t *testing.T) {
95+
for _, tc := range []struct {
96+
name string
97+
tls *tls.ConnectionState
98+
}{
99+
{"no TLS", nil},
100+
{"TLS without a peer certificate", &tls.ConnectionState{}},
101+
} {
102+
t.Run(tc.name, func(t *testing.T) {
103+
rec := httptest.NewRecorder()
104+
req := httptest.NewRequest(http.MethodPost, "/rpc", nil)
105+
req.TLS = tc.tls
106+
ClientCertMiddleware(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {
107+
t.Error("next must not be called without a client certificate")
108+
})).ServeHTTP(rec, req)
109+
110+
if rec.Code != http.StatusUnauthorized {
111+
t.Fatalf("status = %d, want 401", rec.Code)
112+
}
113+
if got := rec.Header().Get("Connection"); got != "close" {
114+
t.Errorf("Connection = %q, want close", got)
115+
}
116+
})
117+
}
118+
}
119+
89120
func TestClientCertMiddleware_MissingExtension403(t *testing.T) {
90121
// A verified peer cert that carries no access-level extension must be
91122
// rejected (403), never passed through or defaulted to a privileged level.

0 commit comments

Comments
 (0)