Handle cookie signing errors

pkg/encryption/utils.go

@@ -2,19 +2,16 @@ package encryption
 
 import (
 	"crypto/hmac"
-	"crypto/rand"
+	// TODO (@NickMeves): Remove SHA1 signed cookie support in V7
 	"crypto/sha1" // #nosec G505
 	"crypto/sha256"
 	"encoding/base64"
 	"fmt"
 	"hash"
-	"io"
 	"net/http"
 	"strconv"
 	"strings"
 	"time"
-
-	"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
 )
 
 // SecretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary
@@ -69,40 +66,44 @@ func Validate(cookie *http.Cookie, seed string, expiration time.Duration) (value
 }
 
 // SignedValue returns a cookie that is signed and can later be checked with Validate
-func SignedValue(seed string, key string, value []byte, now time.Time) string {
+func SignedValue(seed string, key string, value []byte, now time.Time) (string, error) {
 	encodedValue := base64.URLEncoding.EncodeToString(value)
 	timeStr := fmt.Sprintf("%d", now.Unix())
-	sig := cookieSignature(sha256.New, seed, key, encodedValue, timeStr)
+	sig, err := cookieSignature(sha256.New, seed, key, encodedValue, timeStr)
+	if err != nil {
+		return "", err
+	}
 	cookieVal := fmt.Sprintf("%s|%s|%s", encodedValue, timeStr, sig)
-	return cookieVal
+	return cookieVal, nil
 }
 
-func cookieSignature(signer func() hash.Hash, args ...string) string {
+func cookieSignature(signer func() hash.Hash, args ...string) (string, error) {
 	h := hmac.New(signer, []byte(args[0]))
 	for _, arg := range args[1:] {
 		_, err := h.Write([]byte(arg))
-		// If signing fails, fail closed and return something that won't validate
 		if err != nil {
-			garbage := make([]byte, 16)
-			if _, err := io.ReadFull(rand.Reader, garbage); err != nil {
-				logger.Fatal("HMAC & RNG functions both failing. Shutting down for security")
-			}
-			return base64.URLEncoding.EncodeToString(garbage)
+			return "", err
 		}
 	}
 	var b []byte
 	b = h.Sum(b)
-	return base64.URLEncoding.EncodeToString(b)
+	return base64.URLEncoding.EncodeToString(b), nil
 }
 
 func checkSignature(signature string, args ...string) bool {
-	checkSig := cookieSignature(sha256.New, args...)
+	checkSig, err := cookieSignature(sha256.New, args...)
+	if err != nil {
+		return false
+	}
 	if checkHmac(signature, checkSig) {
 		return true
 	}
 
-	// TODO: After appropriate rollout window, remove support for SHA1
-	legacySig := cookieSignature(sha1.New, args...)
+	// TODO (@NickMeves): Remove SHA1 signed cookie support in V7
+	legacySig, err := cookieSignature(sha1.New, args...)
+	if err != nil {
+		return false
+	}
 	return checkHmac(signature, legacySig)
 }
 

pkg/encryption/utils_test.go

@@ -88,8 +88,10 @@ func TestSignAndValidate(t *testing.T) {
 	value := base64.URLEncoding.EncodeToString([]byte("I am soooo encoded"))
 	epoch := "123456789"
 
-	sha256sig := cookieSignature(sha256.New, seed, key, value, epoch)
-	sha1sig := cookieSignature(sha1.New, seed, key, value, epoch)
+	sha256sig, err := cookieSignature(sha256.New, seed, key, value, epoch)
+	assert.NoError(t, err)
+	sha1sig, err := cookieSignature(sha1.New, seed, key, value, epoch)
+	assert.NoError(t, err)
 
 	assert.True(t, checkSignature(sha256sig, seed, key, value, epoch))
 	// This should be switched to False after fully deprecating SHA1

pkg/sessions/cookie/session_store.go

@@ -44,8 +44,7 @@ func (s *SessionStore) Save(rw http.ResponseWriter, req *http.Request, ss *sessi
 	if err != nil {
 		return err
 	}
-	s.setSessionCookie(rw, req, value, *ss.CreatedAt)
-	return nil
+	return s.setSessionCookie(rw, req, value, *ss.CreatedAt)
 }
 
 // Load reads sessions.SessionState information from Cookies within the
@@ -114,24 +113,33 @@ func sessionFromCookie(v []byte, c encryption.Cipher) (s *sessions.SessionState,
 }
 
 // setSessionCookie adds the user's session cookie to the response
-func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Request, val []byte, created time.Time) {
-	for _, c := range s.makeSessionCookie(req, val, created) {
+func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Request, val []byte, created time.Time) error {
+	cookies, err := s.makeSessionCookie(req, val, created)
+	if err != nil {
+		return err
+	}
+	for _, c := range cookies {
 		http.SetCookie(rw, c)
 	}
+	return nil
 }
 
 // makeSessionCookie creates an http.Cookie containing the authenticated user's
 // authentication details
-func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now time.Time) []*http.Cookie {
+func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now time.Time) ([]*http.Cookie, error) {
 	strValue := string(value)
 	if strValue != "" {
-		strValue = encryption.SignedValue(s.Cookie.Secret, s.Cookie.Name, value, now)
+		var err error
+		strValue, err = encryption.SignedValue(s.Cookie.Secret, s.Cookie.Name, value, now)
+		if err != nil {
+			return nil, err
+		}
 	}
 	c := s.makeCookie(req, s.Cookie.Name, strValue, s.Cookie.Expire, now)
 	if len(c.String()) > maxCookieLength {
-		return splitCookie(c)
+		return splitCookie(c), nil
 	}
-	return []*http.Cookie{c}
+	return []*http.Cookie{c}, nil
 }
 
 func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie {

pkg/sessions/persistence/manager.go

@@ -48,7 +48,11 @@ func (m *Manager) Save(rw http.ResponseWriter, req *http.Request, s *sessions.Se
 	if err != nil {
 		return err
 	}
-	tckt.setCookie(rw, req, s)
+
+	err = tckt.setCookie(rw, req, s)
+	if err != nil {
+		return err
+	}
 
 	return nil
 }

pkg/sessions/persistence/ticket.go

@@ -148,33 +148,42 @@ func (t *ticket) clearSession(clearer clearFunc) error {
 }
 
 // setCookie sets the encoded ticket as a cookie
-func (t *ticket) setCookie(rw http.ResponseWriter, req *http.Request, s *sessions.SessionState) {
-	ticketCookie := t.makeCookie(
+func (t *ticket) setCookie(rw http.ResponseWriter, req *http.Request, s *sessions.SessionState) error {
+	ticketCookie, err := t.makeCookie(
 		req,
 		t.encodeTicket(),
 		t.options.Expire,
 		*s.CreatedAt,
 	)
+	if err != nil {
+		return err
+	}
 
 	http.SetCookie(rw, ticketCookie)
+	return nil
 }
 
 // clearCookie removes any cookies that would be where this ticket
 // would set them
 func (t *ticket) clearCookie(rw http.ResponseWriter, req *http.Request) {
-	clearCookie := t.makeCookie(
+	http.SetCookie(rw, cookies.MakeCookieFromOptions(
 		req,
+		t.options.Name,
 		"",
+		t.options,
 		time.Hour*-1,
 		time.Now(),
-	)
-	http.SetCookie(rw, clearCookie)
+	))
 }
 
 // makeCookie makes a cookie, signing the value if present
-func (t *ticket) makeCookie(req *http.Request, value string, expires time.Duration, now time.Time) *http.Cookie {
+func (t *ticket) makeCookie(req *http.Request, value string, expires time.Duration, now time.Time) (*http.Cookie, error) {
 	if value != "" {
-		value = encryption.SignedValue(t.options.Secret, t.options.Name, []byte(value), now)
+		var err error
+		value, err = encryption.SignedValue(t.options.Secret, t.options.Name, []byte(value), now)
+		if err != nil {
+			return nil, err
+		}
 	}
 	return cookies.MakeCookieFromOptions(
 		req,
@@ -183,7 +192,7 @@ func (t *ticket) makeCookie(req *http.Request, value string, expires time.Durati
 		t.options,
 		expires,
 		now,
-	)
+	), nil
 }
 
 // makeCipher makes a AES-GCM cipher out of the ticket's secret

pkg/sessions/tests/session_store_tests.go

@@ -311,11 +311,12 @@ func SessionStoreInterfaceTests(in *testInput) {
 			BeforeEach(func() {
 				By("Using a valid cookie with a different providers session encoding")
 				broken := "BrokenSessionFromADifferentSessionImplementation"
-				value := encryption.SignedValue(in.cookieOpts.Secret, in.cookieOpts.Name, []byte(broken), time.Now())
+				value, err := encryption.SignedValue(in.cookieOpts.Secret, in.cookieOpts.Name, []byte(broken), time.Now())
+				Expect(err).ToNot(HaveOccurred())
 				cookie := cookiesapi.MakeCookieFromOptions(in.request, in.cookieOpts.Name, value, in.cookieOpts, in.cookieOpts.Expire, time.Now())
 				in.request.AddCookie(cookie)
 
-				err := in.ss().Save(in.response, in.request, in.session)
+				err = in.ss().Save(in.response, in.request, in.session)
 				Expect(err).ToNot(HaveOccurred())
 			})