internal/sessions: fix cookie clear session (#376)

CookieStore's ClearSession now properly clears the user session cookie by setting MaxAge to -1.

internal/sessions: move encoder interface to encoding package, and rename to MarshalUnmarshaler.
internal/encoding: move mock to own package
authenticate: use INFO log level for authZ error.

Signed-off-by: Bobby DeSimone <bobbydesimone@gmail.com>
This commit is contained in:
Bobby DeSimone 2019-11-09 10:49:24 -08:00 committed by GitHub
parent d3d60d1055
commit b9ab49c32c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
19 changed files with 173 additions and 217 deletions

View file

@ -3,10 +3,11 @@ package sessions // import "github.com/pomerium/pomerium/internal/sessions"
import (
"errors"
"fmt"
"net"
"net/http"
"strings"
"time"
"github.com/pomerium/pomerium/internal/encoding"
)
const (
@ -31,8 +32,9 @@ type CookieStore struct {
Expire time.Duration
HTTPOnly bool
Secure bool
encoder Marshaler
decoder Unmarshaler
encoder encoding.Marshaler
decoder encoding.Unmarshaler
}
// CookieOptions holds options for CookieStore
@ -45,70 +47,60 @@ type CookieOptions struct {
}
// NewCookieStore returns a new session with ciphers for each of the cookie secrets
func NewCookieStore(opts *CookieOptions, encoder Encoder) (*CookieStore, error) {
if opts.Name == "" {
return nil, fmt.Errorf("internal/sessions: cookie name cannot be empty")
func NewCookieStore(opts *CookieOptions, encoder encoding.MarshalUnmarshaler) (*CookieStore, error) {
cs, err := NewCookieLoader(opts, encoder)
if err != nil {
return nil, err
}
if encoder == nil {
return nil, fmt.Errorf("internal/sessions: decoder cannot be nil")
}
return &CookieStore{
Name: opts.Name,
Secure: opts.Secure,
HTTPOnly: opts.HTTPOnly,
Domain: opts.Domain,
Expire: opts.Expire,
encoder: encoder,
decoder: encoder,
}, nil
cs.encoder = encoder
return cs, nil
}
// NewCookieLoader returns a new session with ciphers for each of the cookie secrets
func NewCookieLoader(opts *CookieOptions, decoder Unmarshaler) (*CookieStore, error) {
func NewCookieLoader(opts *CookieOptions, dencoder encoding.Unmarshaler) (*CookieStore, error) {
if dencoder == nil {
return nil, fmt.Errorf("internal/sessions: dencoder cannot be nil")
}
cs, err := newCookieStore(opts)
if err != nil {
return nil, err
}
cs.decoder = dencoder
return cs, nil
}
func newCookieStore(opts *CookieOptions) (*CookieStore, error) {
if opts.Name == "" {
return nil, fmt.Errorf("internal/sessions: cookie name cannot be empty")
}
if decoder == nil {
return nil, fmt.Errorf("internal/sessions: decoder cannot be nil")
}
return &CookieStore{
Name: opts.Name,
Secure: opts.Secure,
HTTPOnly: opts.HTTPOnly,
Domain: opts.Domain,
Expire: opts.Expire,
decoder: decoder,
}, nil
}
func (cs *CookieStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie {
domain := req.Host
if cs.Domain != "" {
domain = cs.Domain
}
if h, _, err := net.SplitHostPort(domain); err == nil {
domain = h
}
c := &http.Cookie{
Name: name,
func (cs *CookieStore) makeCookie(value string) *http.Cookie {
return &http.Cookie{
Name: cs.Name,
Value: value,
Path: "/",
Domain: domain,
Domain: cs.Domain,
HttpOnly: cs.HTTPOnly,
Secure: cs.Secure,
Expires: timeNow().Add(cs.Expire),
}
// only set an expiration if we want one, otherwise default to non perm session based
if expiration != 0 {
c.Expires = now.Add(expiration)
}
return c
}
// ClearSession clears the session cookie from a request
func (cs *CookieStore) ClearSession(w http.ResponseWriter, req *http.Request) {
http.SetCookie(w, cs.makeCookie(req, cs.Name, "", time.Hour*-1, time.Now()))
func (cs *CookieStore) ClearSession(w http.ResponseWriter, _ *http.Request) {
c := cs.makeCookie("")
c.MaxAge = -1
c.Expires = timeNow().Add(-time.Hour)
http.SetCookie(w, c)
}
// LoadSession returns a State from the cookie in the request.
@ -127,7 +119,7 @@ func (cs *CookieStore) LoadSession(req *http.Request) (*State, error) {
}
// SaveSession saves a session state to a request's cookie store.
func (cs *CookieStore) SaveSession(w http.ResponseWriter, req *http.Request, x interface{}) error {
func (cs *CookieStore) SaveSession(w http.ResponseWriter, _ *http.Request, x interface{}) error {
var value string
if cs.encoder != nil {
data, err := cs.encoder.Marshal(x)
@ -145,17 +137,12 @@ func (cs *CookieStore) SaveSession(w http.ResponseWriter, req *http.Request, x i
return errors.New("internal/sessions: cannot save non-string type")
}
}
cs.setSessionCookie(w, req, value)
cs.setSessionCookie(w, value)
return nil
}
// makeSessionCookie constructs a session cookie given the request, an expiration time and the current time.
func (cs *CookieStore) makeSessionCookie(req *http.Request, value string, expiration time.Duration, now time.Time) *http.Cookie {
return cs.makeCookie(req, cs.Name, value, expiration, now)
}
func (cs *CookieStore) setSessionCookie(w http.ResponseWriter, req *http.Request, val string) {
cs.setCookie(w, cs.makeSessionCookie(req, val, cs.Expire, time.Now()))
func (cs *CookieStore) setSessionCookie(w http.ResponseWriter, val string) {
cs.setCookie(w, cs.makeCookie(val))
}
func (cs *CookieStore) setCookie(w http.ResponseWriter, cookie *http.Cookie) {

View file

@ -4,17 +4,18 @@ import (
"crypto/rand"
"errors"
"fmt"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/pomerium/pomerium/internal/cryptutil"
"github.com/pomerium/pomerium/internal/encoding"
"github.com/pomerium/pomerium/internal/encoding/ecjson"
"github.com/pomerium/pomerium/internal/encoding/mock"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
)
func TestNewCookieStore(t *testing.T) {
@ -26,7 +27,7 @@ func TestNewCookieStore(t *testing.T) {
tests := []struct {
name string
opts *CookieOptions
encoder Encoder
encoder encoding.MarshalUnmarshaler
want *CookieStore
wantErr bool
}{
@ -60,7 +61,7 @@ func TestNewCookieLoader(t *testing.T) {
tests := []struct {
name string
opts *CookieOptions
encoder Encoder
encoder encoding.MarshalUnmarshaler
want *CookieStore
wantErr bool
}{
@ -86,58 +87,6 @@ func TestNewCookieLoader(t *testing.T) {
}
}
func TestCookieStore_makeCookie(t *testing.T) {
cipher, err := cryptutil.NewAEADCipher(cryptutil.NewKey())
if err != nil {
t.Fatal(err)
}
now := time.Now()
tests := []struct {
name string
domain string
cookieDomain string
cookieName string
value string
expiration time.Duration
want *http.Cookie
wantCSRF *http.Cookie
}{
{"good", "http://httpbin.corp.pomerium.io", "", "_pomerium", "value", 0, &http.Cookie{Name: "_pomerium", Value: "value", Path: "/", Domain: "httpbin.corp.pomerium.io", Secure: true, HttpOnly: true}, &http.Cookie{Name: "_pomerium_csrf", Value: "value", Path: "/", Domain: "httpbin.corp.pomerium.io", Secure: true, HttpOnly: true}},
{"domains with https", "https://httpbin.corp.pomerium.io", "", "_pomerium", "value", 0, &http.Cookie{Name: "_pomerium", Value: "value", Path: "/", Domain: "httpbin.corp.pomerium.io", Secure: true, HttpOnly: true}, &http.Cookie{Name: "_pomerium_csrf", Value: "value", Path: "/", Domain: "httpbin.corp.pomerium.io", Secure: true, HttpOnly: true}},
{"domain with port", "http://httpbin.corp.pomerium.io:443", "", "_pomerium", "value", 0, &http.Cookie{Name: "_pomerium", Value: "value", Path: "/", Domain: "httpbin.corp.pomerium.io", Secure: true, HttpOnly: true}, &http.Cookie{Name: "_pomerium_csrf", Value: "value", Path: "/", Domain: "httpbin.corp.pomerium.io", Secure: true, HttpOnly: true}},
{"expiration set", "http://httpbin.corp.pomerium.io:443", "", "_pomerium", "value", 10 * time.Second, &http.Cookie{Expires: now.Add(10 * time.Second), Name: "_pomerium", Value: "value", Path: "/", Domain: "httpbin.corp.pomerium.io", Secure: true, HttpOnly: true}, &http.Cookie{Expires: now.Add(10 * time.Second), Name: "_pomerium_csrf", Value: "value", Path: "/", Domain: "httpbin.corp.pomerium.io", Secure: true, HttpOnly: true}},
{"good", "http://httpbin.corp.pomerium.io", "pomerium.io", "_pomerium", "value", 0, &http.Cookie{Name: "_pomerium", Value: "value", Path: "/", Domain: "pomerium.io", Secure: true, HttpOnly: true}, &http.Cookie{Name: "_pomerium_csrf", Value: "value", Path: "/", Domain: "httpbin.corp.pomerium.io", Secure: true, HttpOnly: true}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
r := httptest.NewRequest("GET", tt.domain, nil)
s, err := NewCookieStore(
&CookieOptions{
Name: "_pomerium",
Secure: true,
HTTPOnly: true,
Domain: tt.cookieDomain,
Expire: 10 * time.Second,
},
ecjson.New(cipher))
if err != nil {
t.Fatal(err)
}
if diff := cmp.Diff(s.makeCookie(r, tt.cookieName, tt.value, tt.expiration, now), tt.want); diff != "" {
t.Errorf("CookieStore.makeCookie() = \n%s", diff)
}
if diff := cmp.Diff(s.makeSessionCookie(r, tt.value, tt.expiration, now), tt.want); diff != "" {
t.Errorf("CookieStore.makeSessionCookie() = \n%s", diff)
}
})
}
}
func TestCookieStore_SaveSession(t *testing.T) {
c, err := cryptutil.NewAEADCipher(cryptutil.NewKey())
if err != nil {
@ -149,18 +98,21 @@ func TestCookieStore_SaveSession(t *testing.T) {
t.Fatal(err)
}
tests := []struct {
name string
State *State
encoder Encoder
decoder Encoder
name string
// State *State
State interface{}
encoder encoding.Marshaler
decoder encoding.Unmarshaler
wantErr bool
wantLoadErr bool
}{
{"good", &State{Email: "user@domain.com", User: "user"}, ecjson.New(c), ecjson.New(c), false, false},
{"bad cipher", &State{Email: "user@domain.com", User: "user"}, nil, nil, true, true},
{"huge cookie", &State{Subject: fmt.Sprintf("%x", hugeString), Email: "user@domain.com", User: "user"}, ecjson.New(c), ecjson.New(c), false, false},
{"marshal error", &State{Email: "user@domain.com", User: "user"}, encoding.MockEncoder{MarshalError: errors.New("error")}, ecjson.New(c), true, true},
{"marshal error", &State{Email: "user@domain.com", User: "user"}, mock.Encoder{MarshalError: errors.New("error")}, ecjson.New(c), true, true},
{"nil encoder cannot save non string type", &State{Email: "user@domain.com", User: "user"}, nil, ecjson.New(c), true, true},
{"good marshal string directly", cryptutil.NewBase64Key(), nil, ecjson.New(c), false, true},
{"good marshal bytes directly", cryptutil.NewKey(), nil, ecjson.New(c), false, true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@ -171,7 +123,7 @@ func TestCookieStore_SaveSession(t *testing.T) {
Domain: "pomerium.io",
Expire: 10 * time.Second,
encoder: tt.encoder,
decoder: tt.encoder,
decoder: tt.decoder,
}
r := httptest.NewRequest("GET", "/", nil)
@ -182,7 +134,6 @@ func TestCookieStore_SaveSession(t *testing.T) {
}
r = httptest.NewRequest("GET", "/", nil)
for _, cookie := range w.Result().Cookies() {
// t.Log(cookie)
r.AddCookie(cookie)
}

View file

@ -3,6 +3,8 @@ package sessions // import "github.com/pomerium/pomerium/internal/sessions"
import (
"net/http"
"strings"
"github.com/pomerium/pomerium/internal/encoding"
)
const (
@ -15,7 +17,7 @@ const (
type HeaderStore struct {
authHeader string
authType string
encoder Unmarshaler
encoder encoding.Unmarshaler
}
// NewHeaderStore returns a new header store for loading sessions from
@ -23,7 +25,7 @@ type HeaderStore struct {
//
// NOTA BENE: While most servers do not log Authorization headers by default,
// you should ensure no other services are logging or leaking your auth headers.
func NewHeaderStore(enc Unmarshaler, headerType string) *HeaderStore {
func NewHeaderStore(enc encoding.Unmarshaler, headerType string) *HeaderStore {
if headerType == "" {
headerType = defaultAuthType
}

View file

@ -2,6 +2,8 @@ package sessions // import "github.com/pomerium/pomerium/internal/sessions"
import (
"net/http"
"github.com/pomerium/pomerium/internal/encoding"
)
const (
@ -12,8 +14,8 @@ const (
// query strings / query parameters.
type QueryParamStore struct {
queryParamKey string
encoder Marshaler
decoder Unmarshaler
encoder encoding.Marshaler
decoder encoding.Unmarshaler
}
// NewQueryParamStore returns a new query param store for loading sessions from
@ -21,7 +23,7 @@ type QueryParamStore struct {
//
// NOTA BENE: By default, most servers _DO_ log query params, the leaking or
// accidental logging of which should be considered a security issue.
func NewQueryParamStore(enc Encoder, qp string) *QueryParamStore {
func NewQueryParamStore(enc encoding.MarshalUnmarshaler, qp string) *QueryParamStore {
if qp == "" {
qp = defaultQueryParamKey
}

View file

@ -8,6 +8,7 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/pomerium/pomerium/internal/encoding"
"github.com/pomerium/pomerium/internal/encoding/mock"
)
func TestNewQueryParamStore(t *testing.T) {
@ -16,13 +17,13 @@ func TestNewQueryParamStore(t *testing.T) {
name string
State *State
enc Encoder
enc encoding.MarshalUnmarshaler
qp string
wantErr bool
wantURL *url.URL
}{
{"simple good", &State{Email: "user@domain.com", User: "user"}, encoding.MockEncoder{MarshalResponse: []byte("ok")}, "", false, &url.URL{Path: "/", RawQuery: "pomerium_session=ok"}},
{"marshall error", &State{Email: "user@domain.com", User: "user"}, encoding.MockEncoder{MarshalError: errors.New("error")}, "", true, &url.URL{Path: "/"}},
{"simple good", &State{Email: "user@domain.com", User: "user"}, mock.Encoder{MarshalResponse: []byte("ok")}, "", false, &url.URL{Path: "/", RawQuery: "pomerium_session=ok"}},
{"marshall error", &State{Email: "user@domain.com", User: "user"}, mock.Encoder{MarshalError: errors.New("error")}, "", true, &url.URL{Path: "/"}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {

View file

@ -37,19 +37,3 @@ type SessionStore interface {
type SessionLoader interface {
LoadSession(*http.Request) (*State, error)
}
// Encoder can both Marshal and Unmarshal a struct into and from a set of bytes.
type Encoder interface {
Marshaler
Unmarshaler
}
// Marshaler encodes a struct into a set of bytes.
type Marshaler interface {
Marshal(interface{}) ([]byte, error)
}
// Unmarshaler decodes a set of bytes and returns a struct.
type Unmarshaler interface {
Unmarshal([]byte, interface{}) error
}