Skip to content

Commit c13d5b2

Browse files
authored
Merge commit from fork
Closes two independent draft advisories in a single PR. Each fix is narrow, defense-in-depth, and verified by a dedicated test. GHSA-rqfj-vv8r-xhqc (medium, Secure cookies): - Add ServerConfig.CookieSecure *bool with CookieSecureResolved() that resolves explicit value first, otherwise infers true when both tls_cert and tls_key are set. - Thread the flag through SessionManager and OIDC via SetCookieSecure setters; Web.WithCookieSecure propagates to both (nil-safe for OIDC). - Update every cookie write site to set Secure: sm.cookieSecure / o.cookieSecure. Logout and OIDC state-clear cookies also pick up the full HttpOnly/Secure/SameSite=Lax fingerprint so browsers reliably replace the live cookies (RFC 6265 attribute matching). GHSA-6vgg-xhvh-38ff (low, mobile-bundle Cache-Control): - handleMobileBundle now sets Cache-Control: no-store, Pragma: no-cache, Expires: 0 alongside Content-Type. The endpoint returns a freshly minted X25519 private key inline, so any cache between server and operator must drop the response. X-Content-Type-Options: nosniff is already set globally by the securityHeaders middleware in cli/serve.go (verified by cli/security_headers_test.go), so the handler does not duplicate it. Note: GHSA-wx87-29cj-m659 (constant-time legacy-key compare) was part of the original bundle but is now obsoleted on main — the legacy config-file API key fallback was removed in 56c07b7, eliminating the timing oracle entirely. The corresponding advisory should be closed with reference to that commit rather than fixed here.
1 parent 8f495a8 commit c13d5b2

10 files changed

Lines changed: 210 additions & 3 deletions

File tree

internal/api/mobile_bundle.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,15 @@ func (s *Server) handleMobileBundle(w http.ResponseWriter, r *http.Request) {
6969
return
7070
}
7171

72-
// Return YAML bundle with proper content-type
72+
// Return YAML bundle with proper content-type. The bundle inlines a
73+
// freshly-minted X25519 private key, so suppress every layer of cache
74+
// between server and operator (intermediate proxies/CDNs, browser disk
75+
// cache). X-Content-Type-Options is set globally by the securityHeaders
76+
// middleware in cli/serve.go. Closes GHSA-6vgg-xhvh-38ff.
7377
w.Header().Set("Content-Type", "application/yaml; charset=utf-8")
78+
w.Header().Set("Cache-Control", "no-store")
79+
w.Header().Set("Pragma", "no-cache")
80+
w.Header().Set("Expires", "0")
7481
w.WriteHeader(http.StatusOK)
7582
if _, err := w.Write(bundle); err != nil {
7683
s.logger.Error("write mobile bundle response", "error", err)

internal/api/mobile_bundle_test.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,20 @@ func TestHandleMobileBundle_Success(t *testing.T) {
5252
t.Errorf("Content-Type = %q, want 'application/yaml; charset=utf-8'", ct)
5353
}
5454

55+
// Bundle inlines a freshly-minted X25519 private key — every cache
56+
// between server and operator must drop the response. X-Content-Type-Options
57+
// is covered by the securityHeaders middleware (cli/security_headers_test.go).
58+
// Regression for GHSA-6vgg-xhvh-38ff.
59+
for header, want := range map[string]string{
60+
"Cache-Control": "no-store",
61+
"Pragma": "no-cache",
62+
"Expires": "0",
63+
} {
64+
if got := w.Header().Get(header); got != want {
65+
t.Errorf("%s = %q, want %q", header, got, want)
66+
}
67+
}
68+
5569
// Verify body is valid YAML with expected keys
5670
var yamlData map[string]interface{}
5771
if err := yaml.Unmarshal(w.Body.Bytes(), &yamlData); err != nil {

internal/cli/serve.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,11 @@ func Serve(configPath string) error {
223223
logger.Info("oidc enabled", "issuer", cfg.OIDC.Issuer)
224224
}
225225

226+
// Resolve cookie_secure AFTER any OIDC wiring so the OIDC state cookie
227+
// picks up the same flag (the helper propagates to both session manager
228+
// and OIDC). Closes GHSA-rqfj-vv8r-xhqc.
229+
webUI.WithCookieSecure(cfg.CookieSecureResolved())
230+
226231
mux := buildMux(webUI, apiSrv)
227232

228233
httpServer := &http.Server{

internal/config/server.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,25 @@ type ServerConfig struct {
7878
// CAAutoRotate configures the periodic CA auto-rotation scanner (issue #110).
7979
// Disabled by default — operators must opt in by setting Enabled=true.
8080
CAAutoRotate CAAutoRotateConfig `yaml:"ca_auto_rotate,omitempty"`
81+
82+
// CookieSecure controls the `Secure` attribute on session and OIDC
83+
// state cookies (GHSA-rqfj-vv8r-xhqc). When unset, the effective
84+
// value is inferred from the TLS configuration: true if both
85+
// tls_cert and tls_key are populated, false otherwise. Operators
86+
// terminating TLS at a reverse proxy must set this to true
87+
// explicitly — `rate_limit.trust_proxy_header` is not a reliable
88+
// signal that the proxy speaks TLS to clients.
89+
CookieSecure *bool `yaml:"cookie_secure,omitempty"`
90+
}
91+
92+
// CookieSecureResolved returns the effective Secure-cookie flag for this
93+
// configuration. Explicit value wins; if unset, infer from the presence
94+
// of TLS material on the server itself.
95+
func (c ServerConfig) CookieSecureResolved() bool {
96+
if c.CookieSecure != nil {
97+
return *c.CookieSecure
98+
}
99+
return c.TLSCert != "" && c.TLSKey != ""
81100
}
82101

83102
// EnrollmentTokenTTLDuration returns the configured default token TTL parsed

internal/config/server_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,3 +288,29 @@ ca_auto_rotate:
288288
t.Fatal("expected error for interval < 1m, got nil")
289289
}
290290
}
291+
292+
// TestCookieSecureResolved covers GHSA-rqfj-vv8r-xhqc's resolution rules:
293+
// explicit cookie_secure wins; otherwise the value is inferred from the
294+
// presence of TLS material on the server.
295+
func TestCookieSecureResolved(t *testing.T) {
296+
ptr := func(b bool) *bool { return &b }
297+
cases := []struct {
298+
name string
299+
cfg ServerConfig
300+
want bool
301+
}{
302+
{"explicit_true", ServerConfig{CookieSecure: ptr(true)}, true},
303+
{"explicit_false_overrides_tls", ServerConfig{CookieSecure: ptr(false), TLSCert: "c", TLSKey: "k"}, false},
304+
{"unset_no_tls", ServerConfig{}, false},
305+
{"unset_with_tls", ServerConfig{TLSCert: "c", TLSKey: "k"}, true},
306+
{"unset_partial_tls_cert_only", ServerConfig{TLSCert: "c"}, false},
307+
{"unset_partial_tls_key_only", ServerConfig{TLSKey: "k"}, false},
308+
}
309+
for _, c := range cases {
310+
t.Run(c.name, func(t *testing.T) {
311+
if got := c.cfg.CookieSecureResolved(); got != c.want {
312+
t.Errorf("CookieSecureResolved() = %v, want %v", got, c.want)
313+
}
314+
})
315+
}
316+
}

internal/web/oidc.go

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,27 @@ type OIDC struct {
3939
stateMu sync.Mutex
4040
states map[string]time.Time // state token → expiry
4141

42+
// cookieSecure controls the Secure attribute on the OIDC state cookie.
43+
// Set via SetCookieSecure from the server's resolved config value.
44+
// Closes GHSA-rqfj-vv8r-xhqc.
45+
cookieSecure bool
46+
4247
// provisionCA is invoked after a new operator is successfully created
4348
// during OIDC first-time login. Enables auto-provisioning of default CA
4449
// for user-role operators without coupling OIDC to Web struct.
4550
// If nil, auto-provision is skipped.
4651
provisionCA func(ctx context.Context, op *models.Operator) error
4752
}
4853

54+
// SetCookieSecure controls the Secure attribute on the OIDC state cookie.
55+
// Called at startup from cli/serve.go with the resolved server-config value.
56+
func (o *OIDC) SetCookieSecure(secure bool) {
57+
if o == nil {
58+
return
59+
}
60+
o.cookieSecure = secure
61+
}
62+
4963
// NewOIDC builds an OIDC integration from the given config. It contacts the
5064
// issuer to fetch the OIDC discovery document. Returns (nil, nil) if OIDC is
5165
// disabled or unconfigured.
@@ -100,6 +114,7 @@ func (o *OIDC) HandleLogin(rw http.ResponseWriter, r *http.Request) {
100114
Value: state,
101115
Path: "/",
102116
HttpOnly: true,
117+
Secure: o.cookieSecure,
103118
SameSite: http.SameSiteLaxMode,
104119
MaxAge: int(oidcStateTTL.Seconds()),
105120
})
@@ -122,7 +137,18 @@ func (o *OIDC) HandleCallback(rw http.ResponseWriter, r *http.Request) {
122137
http.Error(rw, "invalid oidc state", http.StatusBadRequest)
123138
return
124139
}
125-
http.SetCookie(rw, &http.Cookie{Name: oidcStateCookieName, Value: "", Path: "/", MaxAge: -1})
140+
// Match every attribute of the live state cookie so the browser
141+
// replaces it reliably (RFC 6265). Without HttpOnly/SameSite/Secure
142+
// matching, some CDNs and corporate proxies keep the original around.
143+
http.SetCookie(rw, &http.Cookie{
144+
Name: oidcStateCookieName,
145+
Value: "",
146+
Path: "/",
147+
HttpOnly: true,
148+
Secure: o.cookieSecure,
149+
SameSite: http.SameSiteLaxMode,
150+
MaxAge: -1,
151+
})
126152

127153
code := r.URL.Query().Get("code")
128154
if code == "" {

internal/web/oidc_test.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,3 +129,25 @@ func TestOIDC_HandleCallbackBadState(t *testing.T) {
129129
t.Errorf("status = %d, want 400 for invalid state", rec.Code)
130130
}
131131
}
132+
133+
// TestOIDC_SetCookieSecure covers GHSA-rqfj-vv8r-xhqc's OIDC arm. The
134+
// setter must be nil-safe (it is called unconditionally from Web.WithCookieSecure
135+
// even when OIDC is not configured) and must propagate the flag into
136+
// every state-cookie write — both the live-state set in HandleLogin and
137+
// the delete-cookie in HandleCallback.
138+
func TestOIDC_SetCookieSecure(t *testing.T) {
139+
// nil-safety: must not panic when no OIDC is configured.
140+
var nilOIDC *OIDC
141+
nilOIDC.SetCookieSecure(true)
142+
143+
// Setter writes to the field.
144+
o := &OIDC{}
145+
o.SetCookieSecure(true)
146+
if !o.cookieSecure {
147+
t.Error("SetCookieSecure(true) did not propagate")
148+
}
149+
o.SetCookieSecure(false)
150+
if o.cookieSecure {
151+
t.Error("SetCookieSecure(false) did not propagate")
152+
}
153+
}

internal/web/session.go

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,22 @@ const (
2323

2424
// SessionManager handles DB-backed cookie sessions for operator users.
2525
type SessionManager struct {
26-
store store.Store
26+
store store.Store
27+
cookieSecure bool
2728
}
2829

2930
// NewSessionManager creates a new session manager backed by the given store.
3031
func NewSessionManager(s store.Store) *SessionManager {
3132
return &SessionManager{store: s}
3233
}
3334

35+
// SetCookieSecure controls the Secure attribute on session cookies. Called
36+
// at startup from cli/serve.go with the resolved server-config value.
37+
// Closes GHSA-rqfj-vv8r-xhqc.
38+
func (sm *SessionManager) SetCookieSecure(secure bool) {
39+
sm.cookieSecure = secure
40+
}
41+
3442
// LoginResult is the outcome of the first authentication step.
3543
type LoginResult struct {
3644
Operator *models.Operator
@@ -92,6 +100,7 @@ func (sm *SessionManager) Login(w http.ResponseWriter, r *http.Request, username
92100
Value: token,
93101
Path: "/",
94102
HttpOnly: true,
103+
Secure: sm.cookieSecure,
95104
SameSite: http.SameSiteLaxMode,
96105
MaxAge: cookieMaxAge,
97106
})
@@ -123,6 +132,7 @@ func (sm *SessionManager) StartAuthenticatedSession(w http.ResponseWriter, r *ht
123132
Value: token,
124133
Path: "/",
125134
HttpOnly: true,
135+
Secure: sm.cookieSecure,
126136
SameSite: http.SameSiteLaxMode,
127137
MaxAge: int(sessionDuration.Seconds()),
128138
})
@@ -162,6 +172,7 @@ func (sm *SessionManager) CompleteTwoFactor(w http.ResponseWriter, r *http.Reque
162172
Value: cookie.Value,
163173
Path: "/",
164174
HttpOnly: true,
175+
Secure: sm.cookieSecure,
165176
SameSite: http.SameSiteLaxMode,
166177
MaxAge: int(sessionDuration.Seconds()),
167178
})
@@ -176,11 +187,17 @@ func (sm *SessionManager) Logout(w http.ResponseWriter, r *http.Request) {
176187
slog.Debug("delete session", "error", err)
177188
}
178189
}
190+
// Match every attribute of the live session cookie so browsers reliably
191+
// replace it. RFC 6265 requires (Name, Domain, Path) to match; setting
192+
// SameSite and Secure to the same values prevents fingerprint mismatches
193+
// that have caused stale-cookie bugs in production CDNs.
179194
http.SetCookie(w, &http.Cookie{
180195
Name: sessionCookieName,
181196
Value: "",
182197
Path: "/",
183198
HttpOnly: true,
199+
Secure: sm.cookieSecure,
200+
SameSite: http.SameSiteLaxMode,
184201
MaxAge: -1,
185202
})
186203
}

internal/web/web.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,14 @@ func (w *Web) recordLogin(result, factor string) {
131131
// before ServeHTTP is invoked. Default is false.
132132
func (w *Web) AllowSelfRegistration(allow bool) { w.allowSelfRegistration = allow }
133133

134+
// WithCookieSecure threads the resolved cookie-secure flag through to the
135+
// session manager and (if attached) OIDC. Call after WithOIDC so the OIDC
136+
// state cookie picks up the same flag. Closes GHSA-rqfj-vv8r-xhqc.
137+
func (w *Web) WithCookieSecure(secure bool) {
138+
w.session.SetCookieSecure(secure)
139+
w.oidc.SetCookieSecure(secure) // nil-safe inside SetCookieSecure
140+
}
141+
134142
// WithOIDC attaches an OIDC provider and registers its login/callback routes.
135143
// Must be called before ServeHTTP is invoked.
136144
func (w *Web) WithOIDC(o *OIDC) {

internal/web/web_test.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,69 @@ func TestLogin_Success(t *testing.T) {
9999
}
100100
}
101101

102+
// TestSession_CookieSecureFlag covers GHSA-rqfj-vv8r-xhqc: when the
103+
// resolved cookie_secure flag is true, both the live session cookie and
104+
// the logout-delete cookie must carry Secure. Mirrors the OIDC test for
105+
// the state cookie.
106+
func TestSession_CookieSecureFlag(t *testing.T) {
107+
w, _ := newTestWeb(t)
108+
w.WithCookieSecure(true)
109+
110+
cookies := loginSession(t, w)
111+
var live *http.Cookie
112+
for _, c := range cookies {
113+
if c.Name == sessionCookieName {
114+
live = c
115+
break
116+
}
117+
}
118+
if live == nil {
119+
t.Fatal("session cookie not set after login")
120+
}
121+
if !live.Secure {
122+
t.Error("session cookie missing Secure attribute")
123+
}
124+
if !live.HttpOnly {
125+
t.Error("session cookie missing HttpOnly attribute")
126+
}
127+
if live.SameSite != http.SameSiteLaxMode {
128+
t.Errorf("session cookie SameSite = %v, want Lax", live.SameSite)
129+
}
130+
131+
// Re-issuing the logout cookie with mismatched attributes leaves
132+
// browsers holding the original — assert the delete cookie matches
133+
// the live cookie's fingerprint.
134+
logoutReq := httptest.NewRequest("GET", "/ui/logout", nil)
135+
logoutReq.AddCookie(live)
136+
logoutRec := httptest.NewRecorder()
137+
w.ServeHTTP(logoutRec, logoutReq)
138+
var del *http.Cookie
139+
for _, c := range logoutRec.Result().Cookies() {
140+
if c.Name == sessionCookieName {
141+
del = c
142+
}
143+
}
144+
if del == nil {
145+
t.Fatal("logout did not emit a session-cookie delete")
146+
}
147+
if !del.Secure || !del.HttpOnly || del.SameSite != http.SameSiteLaxMode {
148+
t.Errorf("logout cookie attribute drift: Secure=%v HttpOnly=%v SameSite=%v",
149+
del.Secure, del.HttpOnly, del.SameSite)
150+
}
151+
}
152+
153+
// TestSession_CookieSecureDefault confirms that without an explicit
154+
// WithCookieSecure call, the cookie is NOT marked Secure — required so
155+
// plain-HTTP local development stays usable.
156+
func TestSession_CookieSecureDefault(t *testing.T) {
157+
w, _ := newTestWeb(t)
158+
for _, c := range loginSession(t, w) {
159+
if c.Name == sessionCookieName && c.Secure {
160+
t.Error("session cookie unexpectedly Secure when flag not set")
161+
}
162+
}
163+
}
164+
102165
func TestLogin_WrongPassword(t *testing.T) {
103166
w, _ := newTestWeb(t)
104167
form := url.Values{"username": {testUsername}, "password": {"wrong"}}

0 commit comments

Comments
 (0)