diff --git a/cmd/server/service.go b/cmd/server/service.go index e7ae8a2..0f386bd 100644 --- a/cmd/server/service.go +++ b/cmd/server/service.go @@ -316,7 +316,10 @@ skipConfigPrompt: certLine = "# tls_cert: /var/lib/nadir/tls/cert.pem" keyLine = "# tls_key: /var/lib/nadir/tls/key.pem" } else if isTrustProxy { - secureTLSVal = "false" + // The proxy terminates TLS, so the browser<->proxy leg is still HTTPS - + // the cookie stays Secure. Only the proxy<->nadir hop is plaintext, and + // the browser never sees it. + secureTLSVal = "true" trustProxyLine = "trust_proxy: true" certLine = "# tls_cert: /var/lib/nadir/tls/cert.pem" keyLine = "# tls_key: /var/lib/nadir/tls/key.pem" diff --git a/internal/auth/pam.go b/internal/auth/pam.go index 117edca..ed299d5 100644 --- a/internal/auth/pam.go +++ b/internal/auth/pam.go @@ -2,6 +2,14 @@ package auth import "github.com/msteinert/pam" +// Authenticate verifies a username/password against the nadir PAM service. +// +// Both stacks are run. Authenticate checks the password; AcctMgmt runs the +// `account` stack (pam_unix), which is what enforces account and password +// expiry - the shadow sp_expire/sp_max fields an admin sets with `chage -E` or +// `usermod --expiredate` to disable an account without deleting it. Skipping it +// would let a disabled account keep logging in here long after ssh and console +// reject it. func Authenticate(username, password string) error { t, err := pam.StartFunc(PAMService, username, func(s pam.Style, msg string) (string, error) { if s == pam.PromptEchoOff { @@ -13,6 +21,10 @@ func Authenticate(username, password string) error { if err != nil { return err } + // No explicit close: msteinert/pam ends the transaction via a finalizer. - return t.Authenticate(0) + if err := t.Authenticate(0); err != nil { + return err + } + return t.AcctMgmt(0) } diff --git a/internal/config/config.go b/internal/config/config.go index 5e8ec3d..c0e2c98 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -69,10 +69,13 @@ type Server struct { } // SecureCookie reports whether the session cookie should carry the Secure -// attribute, defaulting to false when server.secure_tls is omitted (plain HTTP). +// attribute. An omitted server.secure_tls defaults to true: the safe default is +// a cookie that only travels over HTTPS, so a config that forgets the key can't +// silently ship a session cookie a plaintext request would leak. Local +// plain-HTTP development opts out explicitly with secure_tls: false. func (f *File) SecureCookie() bool { if f.Server.SecureTLS == nil { - return false + return true } return *f.Server.SecureTLS } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 795546f..5f48cee 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -30,13 +30,15 @@ func mods() []module.Module { } } -func TestSecureCookieDefaultsFalse(t *testing.T) { - if (&File{}).SecureCookie() { - t.Error("omitted secure_tls should default to false") +func TestSecureCookieDefaultsTrue(t *testing.T) { + // Fail closed: a config that omits secure_tls must still mark the session + // cookie Secure, so a forgotten key can't leak it over plaintext. + if !(&File{}).SecureCookie() { + t.Error("omitted secure_tls should default to true") } - yes := true - if !(&File{Server: Server{SecureTLS: &yes}}).SecureCookie() { - t.Error("secure_tls: true should enable the Secure flag") + no := false + if (&File{Server: Server{SecureTLS: &no}}).SecureCookie() { + t.Error("secure_tls: false should clear the Secure flag") } }