From ab1f1f821bbd8a5fce5d7d22b35c7f5856f90b7c Mon Sep 17 00:00:00 2001
From: ale <ale@incal.net>
Date: Thu, 19 Dec 2019 17:24:47 +0000
Subject: [PATCH] Do not generate SSO tickets valid for longer than the auth
session
This prevents an error where the keystore will have invalid keys even
in presence of a valid SSO ticket (because the parent auth session has
expired already).
---
server/http.go | 6 ++++--
server/login/login.go | 26 ++++++++++++++++++++------
server/service.go | 14 +++++++++++---
server/service_test.go | 33 ++++++++++++++++++++++++++++++---
4 files changed, 65 insertions(+), 14 deletions(-)
diff --git a/server/http.go b/server/http.go
index b224026..e5638dc 100644
--- a/server/http.go
+++ b/server/http.go
@@ -289,8 +289,10 @@ func (h *Server) handleGrantTicket(w http.ResponseWriter, req *http.Request) {
}
}
- // Make the authorization request.
- token, err := h.loginService.Authorize(username, service, destination, nonce, groups)
+ // Make the authorization request. Tickets should not live
+ // longer than the authentication session.
+ ttl := auth.Deadline.Sub(time.Now().UTC())
+ token, err := h.loginService.Authorize(username, service, destination, nonce, groups, ttl)
if err != nil {
log.Printf("auth error: %v: user=%s service=%s destination=%s nonce=%s groups=%s", err, username, service, destination, nonce, groupsStr)
http.Error(w, err.Error(), http.StatusBadRequest)
diff --git a/server/login/login.go b/server/login/login.go
index ab8272c..aff58ca 100644
--- a/server/login/login.go
+++ b/server/login/login.go
@@ -25,6 +25,9 @@ type Auth struct {
// True if the user is authenticated.
Authenticated bool
+ // Deadline until authentication will need to be renewed.
+ Deadline time.Time
+
// User name and other information (like group membership).
Username string
UserInfo *auth.UserInfo
@@ -198,7 +201,7 @@ func (l *Login) ServeHTTP(w http.ResponseWriter, req *http.Request) {
}
}
-func (l *Login) loginOk(w http.ResponseWriter, req *http.Request, sess *loginSession, password string) {
+func (l *Login) loginOk(w http.ResponseWriter, req *http.Request, sess *loginSession, password string, userinfo *auth.UserInfo) {
if l.callback != nil {
if err := l.callback(req.Context(), sess.Username, password, sess.UserInfo); err != nil {
log.Printf("login callback error: %v", err)
@@ -214,11 +217,23 @@ func (l *Login) loginOk(w http.ResponseWriter, req *http.Request, sess *loginSes
target = l.fallbackRedirect
}
- sess.Authenticated = true
+ // Wipe the temporary state as we don't need it anymore.
sess.Redir = ""
sess.Password = ""
sess.AuthResponse = nil
+ // Save the remaining fields of Auth (Username is already set by
+ // handleLogin) that will persist long-term.
+ //
+ // The authentication deadline will affect the lifespan of the
+ // generated SSO credentials. We are anyway bound by the securecookie
+ // lifespan, which is set at the end of this function when we call
+ // Encode() on the session cookie, so we're guaranteed that it will be
+ // greater or equal to Deadline.
+ sess.Authenticated = true
+ sess.UserInfo = userinfo
+ sess.Deadline = time.Now().UTC().Add(l.sessionTTL)
+
http.Redirect(w, req, target, http.StatusFound)
}
@@ -253,8 +268,7 @@ func (l *Login) handleLogin(w http.ResponseWriter, req *http.Request, sess *logi
switch resp.Status {
case auth.StatusOK:
- sess.UserInfo = resp.UserInfo
- l.loginOk(w, req, sess, password)
+ l.loginOk(w, req, sess, password, resp.UserInfo)
return
case auth.StatusInsufficientCredentials:
sess.Password = password
@@ -303,7 +317,7 @@ func (l *Login) handleLoginOTP(w http.ResponseWriter, req *http.Request, sess *l
return
}
if resp.Status == auth.StatusOK {
- l.loginOk(w, req, sess, sess.Password)
+ l.loginOk(w, req, sess, sess.Password, resp.UserInfo)
return
}
env["Error"] = true
@@ -353,7 +367,7 @@ func (l *Login) handleLoginU2F(w http.ResponseWriter, req *http.Request, sess *l
return
}
if resp.Status == auth.StatusOK {
- l.loginOk(w, req, sess, sess.Password)
+ l.loginOk(w, req, sess, sess.Password, resp.UserInfo)
return
}
env["Error"] = true
diff --git a/server/service.go b/server/service.go
index f03d165..b8a699e 100644
--- a/server/service.go
+++ b/server/service.go
@@ -77,12 +77,20 @@ func NewLoginService(config *Config) (*LoginService, error) {
// Authorize a user to access a service by generating a token for
// it. Note that the user must already be successfully identified by
-// some other means (e.g. passing a login form, etc).
-func (s *LoginService) Authorize(username, service, destination, nonce string, groups []string) (string, error) {
+// some other means (e.g. passing a login form, etc). The 'maxTTL'
+// parameter, if non-zero, caps the time-to-live of the ticket, which
+// is otherwise determined depending on the service configuration.
+func (s *LoginService) Authorize(username, service, destination, nonce string, groups []string, maxTTL time.Duration) (string, error) {
if err := s.validateServiceAccess(service, destination); err != nil {
return "", err
}
- tkt := sso.NewTicket(username, service, s.config.Domain, nonce, groups, s.config.getServiceTTL(service))
+
+ ttl := s.config.getServiceTTL(service)
+ if maxTTL != 0 && maxTTL < ttl {
+ ttl = maxTTL
+ }
+
+ tkt := sso.NewTicket(username, service, s.config.Domain, nonce, groups, ttl)
return s.signer.Sign(tkt)
}
diff --git a/server/service_test.go b/server/service_test.go
index f6a740f..8df66f6 100644
--- a/server/service_test.go
+++ b/server/service_test.go
@@ -6,6 +6,7 @@ import (
"os"
"path/filepath"
"testing"
+ "time"
"golang.org/x/crypto/ed25519"
"gopkg.in/yaml.v2"
@@ -78,7 +79,7 @@ func TestLoginService_Ok(t *testing.T) {
t.Fatal("NewLoginService():", err)
}
- token, err := svc.Authorize("user", "service.example.com/", "https://service.example.com/", "nonce", []string{"group1"})
+ token, err := svc.Authorize("user", "service.example.com/", "https://service.example.com/", "nonce", []string{"group1"}, 0)
if err != nil {
t.Fatal("Authorize():", err)
}
@@ -96,6 +97,32 @@ func TestLoginService_Ok(t *testing.T) {
}
}
+func TestLoginService_MaxTTL(t *testing.T) {
+ tmpdir, _ := ioutil.TempDir("", "")
+ defer os.RemoveAll(tmpdir)
+
+ config := testConfig(t, tmpdir, "")
+ svc, err := NewLoginService(config)
+ if err != nil {
+ t.Fatal("NewLoginService():", err)
+ }
+
+ // Using a negative maxTTL, we create a ticket that expired in the past.
+ token, err := svc.Authorize("user", "service.example.com/", "https://service.example.com/", "nonce", []string{"group1"}, -1*time.Hour)
+ if err != nil {
+ t.Fatal("Authorize():", err)
+ }
+
+ v, err := newValidatorFromConfig(config)
+ if err != nil {
+ t.Fatal("newValidatorFromConfig():", err)
+ }
+ _, err = v.Validate(token, "nonce", "service.example.com/", []string{"group1"})
+ if err == nil {
+ t.Fatal("Validate() succeeded, expected error")
+ }
+}
+
func TestLoginService_SanityChecks(t *testing.T) {
tmpdir, _ := ioutil.TempDir("", "")
defer os.RemoveAll(tmpdir)
@@ -119,7 +146,7 @@ func TestLoginService_SanityChecks(t *testing.T) {
}
for _, td := range testdata {
- _, err := svc.Authorize("user", td.service, td.destination, "nonce", nil)
+ _, err := svc.Authorize("user", td.service, td.destination, "nonce", nil, 0)
if (err == nil) != td.ok {
t.Errorf("Authorize error: s=%s d=%s expected=%v got=%v", td.service, td.destination, td.ok, err)
}
@@ -147,7 +174,7 @@ func TestLoginService_Exchange(t *testing.T) {
}
for _, td := range testdata {
- tkt, err := svc.Authorize("user", td.service, "https://"+td.service, "nonce", nil)
+ tkt, err := svc.Authorize("user", td.service, "https://"+td.service, "nonce", nil, 0)
if err != nil {
t.Errorf("%s: Authorize() error", td.service)
continue
--
GitLab