From d3423408c7d5091f47e5b788dc39a3a9c0288cbe Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sun, 18 Apr 2021 10:25:57 -0700 Subject: [PATCH 001/433] Add a clock package for better time mocking (#1136) * Add a clock package for better time mocking * Make Clock a struct so it doesn't need initialization * Test clock package * Use atomic for live time tests * Refer to same clock.Mock throughout methods --- CHANGELOG.md | 1 + go.mod | 1 + go.sum | 2 + pkg/clock/clock.go | 164 +++++++++++++++ pkg/clock/clock_suite_test.go | 17 ++ pkg/clock/clock_test.go | 380 ++++++++++++++++++++++++++++++++++ 6 files changed, 565 insertions(+) create mode 100644 pkg/clock/clock.go create mode 100644 pkg/clock/clock_suite_test.go create mode 100644 pkg/clock/clock_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 88baaafea9..a0f663a072 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ## Changes since v7.1.2 +- [#1136](https://github.com/oauth2-proxy/oauth2-proxy/pull/1136) Add clock package for better time mocking in tests (@NickMeves) - [#947](https://github.com/oauth2-proxy/oauth2-proxy/pull/947) Multiple provider ingestion and validation in alpha options (first stage: [#926](https://github.com/oauth2-proxy/oauth2-proxy/issues/926)) (@yanasega) # V7.1.2 diff --git a/go.mod b/go.mod index 9cb43c0801..b4ced24b98 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ go 1.16 require ( github.com/Bose/minisentinel v0.0.0-20200130220412-917c5a9223bb github.com/alicebob/miniredis/v2 v2.13.0 + github.com/benbjohnson/clock v1.1.1-0.20210213131748-c97fc7b6bee0 github.com/bitly/go-simplejson v0.5.0 github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869 // indirect github.com/coreos/go-oidc v2.2.1+incompatible diff --git a/go.sum b/go.sum index 0a1b46f9b6..d1f2d89a44 100644 --- a/go.sum +++ b/go.sum @@ -38,6 +38,8 @@ github.com/aryann/difflib v0.0.0-20170710044230-e206f873d14a/go.mod h1:DAHtR1m6l github.com/aws/aws-lambda-go v1.13.3/go.mod h1:4UKl9IzQMoD+QF79YdCuzCwp8VbmG4VAQwij/eHl5CU= github.com/aws/aws-sdk-go v1.27.0/go.mod h1:KmX6BPdI08NWTb3/sm4ZGu5ShLoqVDhKgpiN924inxo= github.com/aws/aws-sdk-go-v2 v0.18.0/go.mod h1:JWVYvqSMppoMJC0x5wdwiImzgXTI9FuZwxzkQq9wy+g= +github.com/benbjohnson/clock v1.1.1-0.20210213131748-c97fc7b6bee0 h1:ROGOOFsMU1fh3kR94itIWlWiPLtgd4TA/qWi4+lL0GM= +github.com/benbjohnson/clock v1.1.1-0.20210213131748-c97fc7b6bee0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA= github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q= github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+CedLV8= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= diff --git a/pkg/clock/clock.go b/pkg/clock/clock.go new file mode 100644 index 0000000000..34b7bf2387 --- /dev/null +++ b/pkg/clock/clock.go @@ -0,0 +1,164 @@ +package clock + +import ( + "errors" + "sync" + "time" + + clockapi "github.com/benbjohnson/clock" +) + +var ( + globalClock = clockapi.New() + mu sync.Mutex +) + +// Set the global clock to a clockapi.Mock with the given time.Time +func Set(t time.Time) { + mu.Lock() + defer mu.Unlock() + mock, ok := globalClock.(*clockapi.Mock) + if !ok { + mock = clockapi.NewMock() + } + mock.Set(t) + globalClock = mock +} + +// Add moves the mocked global clock forward the given duration. It will error +// if the global clock is not mocked. +func Add(d time.Duration) error { + mu.Lock() + defer mu.Unlock() + mock, ok := globalClock.(*clockapi.Mock) + if !ok { + return errors.New("time not mocked") + } + mock.Add(d) + return nil +} + +// Reset sets the global clock to a pure time implementation. Returns any +// existing Mock if set in case lingering time operations are attached to it. +func Reset() *clockapi.Mock { + mu.Lock() + defer mu.Unlock() + existing := globalClock + globalClock = clockapi.New() + + mock, ok := existing.(*clockapi.Mock) + if !ok { + return nil + } + return mock +} + +// Clock is a non-package level wrapper around time that supports stubbing. +// It will use its localized stubs (allowing for parallelized unit tests +// where package level stubbing would cause issues). It falls back to any +// package level time stubs for non-parallel, cross-package integration +// testing scenarios. +// +// If nothing is stubbed, it defaults to default time behavior in the time +// package. +type Clock struct { + mock *clockapi.Mock + sync.Mutex +} + +// Set sets the Clock to a clock.Mock at the given time.Time +func (c *Clock) Set(t time.Time) { + c.Lock() + defer c.Unlock() + if c.mock == nil { + c.mock = clockapi.NewMock() + } + c.mock.Set(t) +} + +// Add moves clock forward time.Duration if it is mocked. It will error +// if the clock is not mocked. +func (c *Clock) Add(d time.Duration) error { + c.Lock() + defer c.Unlock() + if c.mock == nil { + return errors.New("clock not mocked") + } + c.mock.Add(d) + return nil +} + +// Reset removes local clock.Mock. Returns any existing Mock if set in case +// lingering time operations are attached to it. +func (c *Clock) Reset() *clockapi.Mock { + c.Lock() + defer c.Unlock() + existing := c.mock + c.mock = nil + return existing +} + +func (c *Clock) After(d time.Duration) <-chan time.Time { + m := c.mock + if m == nil { + return globalClock.After(d) + } + return m.After(d) +} + +func (c *Clock) AfterFunc(d time.Duration, f func()) *clockapi.Timer { + m := c.mock + if m == nil { + return globalClock.AfterFunc(d, f) + } + return m.AfterFunc(d, f) +} + +func (c *Clock) Now() time.Time { + m := c.mock + if m == nil { + return globalClock.Now() + } + return m.Now() +} + +func (c *Clock) Since(t time.Time) time.Duration { + m := c.mock + if m == nil { + return globalClock.Since(t) + } + return m.Since(t) +} + +func (c *Clock) Sleep(d time.Duration) { + m := c.mock + if m == nil { + globalClock.Sleep(d) + return + } + m.Sleep(d) +} + +func (c *Clock) Tick(d time.Duration) <-chan time.Time { + m := c.mock + if m == nil { + return globalClock.Tick(d) + } + return m.Tick(d) +} + +func (c *Clock) Ticker(d time.Duration) *clockapi.Ticker { + m := c.mock + if m == nil { + return globalClock.Ticker(d) + } + return m.Ticker(d) +} + +func (c *Clock) Timer(d time.Duration) *clockapi.Timer { + m := c.mock + if m == nil { + return globalClock.Timer(d) + } + return m.Timer(d) +} diff --git a/pkg/clock/clock_suite_test.go b/pkg/clock/clock_suite_test.go new file mode 100644 index 0000000000..f12825ba03 --- /dev/null +++ b/pkg/clock/clock_suite_test.go @@ -0,0 +1,17 @@ +package clock_test + +import ( + "testing" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +func TestClockSuite(t *testing.T) { + logger.SetOutput(GinkgoWriter) + logger.SetErrOutput(GinkgoWriter) + + RegisterFailHandler(Fail) + RunSpecs(t, "Clock") +} diff --git a/pkg/clock/clock_test.go b/pkg/clock/clock_test.go new file mode 100644 index 0000000000..5dc8c5faf6 --- /dev/null +++ b/pkg/clock/clock_test.go @@ -0,0 +1,380 @@ +package clock_test + +import ( + "sync" + "sync/atomic" + "time" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/clock" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +const ( + testGlobalEpoch = 1000000000 + testLocalEpoch = 1234567890 +) + +var _ = Describe("Clock suite", func() { + var testClock = clock.Clock{} + + AfterEach(func() { + clock.Reset() + testClock.Reset() + }) + + Context("Global time not overridden", func() { + It("errors when trying to Add", func() { + err := clock.Add(123 * time.Hour) + Expect(err).To(HaveOccurred()) + }) + + Context("Clock not mocked via Set", func() { + const ( + outsideTolerance = int32(0) + withinTolerance = int32(1) + ) + + It("uses time.After for After", func() { + var tolerance int32 + go func() { + time.Sleep(10 * time.Millisecond) + atomic.StoreInt32(&tolerance, withinTolerance) + }() + go func() { + time.Sleep(30 * time.Millisecond) + atomic.StoreInt32(&tolerance, outsideTolerance) + }() + + Expect(atomic.LoadInt32(&tolerance)).To(Equal(outsideTolerance)) + + <-testClock.After(20 * time.Millisecond) + Expect(atomic.LoadInt32(&tolerance)).To(Equal(withinTolerance)) + + <-testClock.After(20 * time.Millisecond) + Expect(atomic.LoadInt32(&tolerance)).To(Equal(outsideTolerance)) + }) + + It("uses time.AfterFunc for AfterFunc", func() { + var tolerance int32 + go func() { + time.Sleep(10 * time.Millisecond) + atomic.StoreInt32(&tolerance, withinTolerance) + }() + go func() { + time.Sleep(30 * time.Millisecond) + atomic.StoreInt32(&tolerance, outsideTolerance) + }() + + Expect(atomic.LoadInt32(&tolerance)).To(Equal(outsideTolerance)) + + var wg sync.WaitGroup + wg.Add(1) + testClock.AfterFunc(20*time.Millisecond, func() { + wg.Done() + }) + wg.Wait() + Expect(atomic.LoadInt32(&tolerance)).To(Equal(withinTolerance)) + + wg.Add(1) + testClock.AfterFunc(20*time.Millisecond, func() { + wg.Done() + }) + wg.Wait() + Expect(atomic.LoadInt32(&tolerance)).To(Equal(outsideTolerance)) + }) + + It("uses time.Now for Now", func() { + a := time.Now() + b := testClock.Now() + Expect(b.Sub(a).Round(10 * time.Millisecond)).To(Equal(0 * time.Millisecond)) + }) + + It("uses time.Since for Since", func() { + past := time.Now().Add(-60 * time.Second) + Expect(time.Since(past).Round(10 * time.Millisecond)). + To(Equal(60 * time.Second)) + }) + + It("uses time.Sleep for Sleep", func() { + var tolerance int32 + go func() { + time.Sleep(10 * time.Millisecond) + atomic.StoreInt32(&tolerance, withinTolerance) + }() + go func() { + time.Sleep(30 * time.Millisecond) + atomic.StoreInt32(&tolerance, outsideTolerance) + }() + + Expect(atomic.LoadInt32(&tolerance)).To(Equal(outsideTolerance)) + + testClock.Sleep(20 * time.Millisecond) + Expect(atomic.LoadInt32(&tolerance)).To(Equal(withinTolerance)) + + testClock.Sleep(20 * time.Millisecond) + Expect(atomic.LoadInt32(&tolerance)).To(Equal(outsideTolerance)) + }) + + It("uses time.Tick for Tick", func() { + var tolerance int32 + go func() { + time.Sleep(10 * time.Millisecond) + atomic.StoreInt32(&tolerance, withinTolerance) + }() + go func() { + time.Sleep(50 * time.Millisecond) + atomic.StoreInt32(&tolerance, outsideTolerance) + }() + + ch := testClock.Tick(20 * time.Millisecond) + Expect(atomic.LoadInt32(&tolerance)).To(Equal(outsideTolerance)) + <-ch + Expect(atomic.LoadInt32(&tolerance)).To(Equal(withinTolerance)) + <-ch + Expect(atomic.LoadInt32(&tolerance)).To(Equal(withinTolerance)) + <-ch + Expect(atomic.LoadInt32(&tolerance)).To(Equal(outsideTolerance)) + }) + + It("uses time.Ticker for Ticker", func() { + var tolerance int32 + go func() { + time.Sleep(10 * time.Millisecond) + atomic.StoreInt32(&tolerance, withinTolerance) + }() + go func() { + time.Sleep(50 * time.Millisecond) + atomic.StoreInt32(&tolerance, outsideTolerance) + }() + + ticker := testClock.Ticker(20 * time.Millisecond) + Expect(atomic.LoadInt32(&tolerance)).To(Equal(outsideTolerance)) + <-ticker.C + Expect(atomic.LoadInt32(&tolerance)).To(Equal(withinTolerance)) + <-ticker.C + Expect(atomic.LoadInt32(&tolerance)).To(Equal(withinTolerance)) + <-ticker.C + Expect(atomic.LoadInt32(&tolerance)).To(Equal(outsideTolerance)) + }) + + It("errors if Add is used", func() { + err := testClock.Add(100 * time.Second) + Expect(err).To(HaveOccurred()) + }) + }) + + Context("Clock mocked via Set", func() { + var now = time.Unix(testLocalEpoch, 0) + + BeforeEach(func() { + testClock.Set(now) + }) + + It("mocks After", func() { + var after int32 + ready := make(chan struct{}) + ch := testClock.After(10 * time.Second) + go func(ch <-chan time.Time) { + close(ready) + <-ch + atomic.StoreInt32(&after, 1) + }(ch) + <-ready + + err := testClock.Add(9 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&after)).To(Equal(int32(0))) + + err = testClock.Add(1 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&after)).To(Equal(int32(1))) + }) + + It("mocks AfterFunc", func() { + var after int32 + testClock.AfterFunc(10*time.Second, func() { + atomic.StoreInt32(&after, 1) + }) + + err := testClock.Add(9 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&after)).To(Equal(int32(0))) + + err = testClock.Add(1 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&after)).To(Equal(int32(1))) + }) + + It("mocks AfterFunc with a stopped timer", func() { + var after int32 + timer := testClock.AfterFunc(10*time.Second, func() { + atomic.StoreInt32(&after, 1) + }) + timer.Stop() + + err := testClock.Add(11 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&after)).To(Equal(int32(0))) + }) + + It("mocks Now", func() { + Expect(testClock.Now()).To(Equal(now)) + err := testClock.Add(123 * time.Hour) + Expect(err).ToNot(HaveOccurred()) + Expect(testClock.Now()).To(Equal(now.Add(123 * time.Hour))) + }) + + It("mocks Since", func() { + Expect(testClock.Since(time.Unix(testLocalEpoch-100, 0))). + To(Equal(100 * time.Second)) + }) + + It("mocks Sleep", func() { + var after int32 + ready := make(chan struct{}) + go func() { + close(ready) + testClock.Sleep(10 * time.Second) + atomic.StoreInt32(&after, 1) + }() + <-ready + + err := testClock.Add(9 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&after)).To(Equal(int32(0))) + + err = testClock.Add(1 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&after)).To(Equal(int32(1))) + }) + + It("mocks Tick", func() { + var ticks int32 + ready := make(chan struct{}) + go func() { + close(ready) + tick := testClock.Tick(10 * time.Second) + for ticks < 5 { + <-tick + atomic.AddInt32(&ticks, 1) + } + }() + <-ready + + Expect(atomic.LoadInt32(&ticks)).To(Equal(int32(0))) + + err := testClock.Add(9 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&ticks)).To(Equal(int32(0))) + + err = testClock.Add(1 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&ticks)).To(Equal(int32(1))) + + err = testClock.Add(30 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&ticks)).To(Equal(int32(4))) + + err = testClock.Add(10 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&ticks)).To(Equal(int32(5))) + }) + + It("mocks Ticker", func() { + var ticks int32 + ready := make(chan struct{}) + go func() { + ticker := testClock.Ticker(10 * time.Second) + close(ready) + for ticks < 5 { + <-ticker.C + atomic.AddInt32(&ticks, 1) + } + }() + <-ready + + Expect(atomic.LoadInt32(&ticks)).To(Equal(int32(0))) + + err := testClock.Add(9 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&ticks)).To(Equal(int32(0))) + + err = testClock.Add(1 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&ticks)).To(Equal(int32(1))) + + err = testClock.Add(30 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&ticks)).To(Equal(int32(4))) + + err = testClock.Add(10 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&ticks)).To(Equal(int32(5))) + }) + + It("mocks Timer", func() { + var after int32 + ready := make(chan struct{}) + go func() { + timer := testClock.Timer(10 * time.Second) + close(ready) + <-timer.C + atomic.AddInt32(&after, 1) + }() + <-ready + + err := testClock.Add(9 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&after)).To(Equal(int32(0))) + + err = testClock.Add(1 * time.Second) + Expect(err).ToNot(HaveOccurred()) + Expect(atomic.LoadInt32(&after)).To(Equal(int32(1))) + }) + }) + }) + + Context("Global time overridden", func() { + var ( + globalNow = time.Unix(testGlobalEpoch, 0) + localNow = time.Unix(testLocalEpoch, 0) + ) + + BeforeEach(func() { + clock.Set(globalNow) + }) + + Context("Clock not mocked via Set", func() { + It("uses globally mocked Now", func() { + Expect(testClock.Now()).To(Equal(globalNow)) + err := clock.Add(123 * time.Hour) + Expect(err).ToNot(HaveOccurred()) + Expect(testClock.Now()).To(Equal(globalNow.Add(123 * time.Hour))) + }) + + It("errors when Add is called on the local Clock", func() { + err := testClock.Add(100 * time.Hour) + Expect(err).To(HaveOccurred()) + }) + }) + + Context("Clock is mocked via Set", func() { + BeforeEach(func() { + testClock.Set(localNow) + }) + + It("uses the local mock and ignores the global", func() { + Expect(testClock.Now()).To(Equal(localNow)) + + err := clock.Add(456 * time.Hour) + Expect(err).ToNot(HaveOccurred()) + + err = testClock.Add(123 * time.Hour) + Expect(err).ToNot(HaveOccurred()) + + Expect(testClock.Now()).To(Equal(localNow.Add(123 * time.Hour))) + }) + }) + }) +}) From 7eeaea0b3f563c7e14f213e47dfda82b40f3fca1 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Wed, 21 Apr 2021 02:33:27 -0700 Subject: [PATCH 002/433] Support nonce checks in OIDC Provider (#967) * Set and verify a nonce with OIDC * Create a CSRF object to manage nonces & cookies * Add missing generic cookie unit tests * Add config flag to control OIDC SkipNonce * Send hashed nonces in authentication requests * Encrypt the CSRF cookie * Add clarity to naming & add more helper methods * Make CSRF an interface and keep underlying nonces private * Add ReverseProxy scope to cookie tests * Align to new 1.16 SameSite cookie default * Perform SecretBytes conversion on CSRF cookie crypto * Make state encoding signatures consistent * Mock time in CSRF struct via Clock * Improve InsecureSkipNonce docstring --- CHANGELOG.md | 4 + docs/docs/configuration/alpha_config.md | 1 + docs/docs/configuration/overview.md | 1 + main_test.go | 10 +- oauthproxy.go | 141 ++++++++--------- oauthproxy_test.go | 40 ++++- pkg/apis/options/legacy_options.go | 16 +- pkg/apis/options/legacy_options_test.go | 1 + pkg/apis/options/load_test.go | 13 +- pkg/apis/options/providers.go | 7 + pkg/apis/sessions/session_state.go | 7 + pkg/apis/sessions/session_state_test.go | 4 + pkg/cookies/cookies.go | 65 ++++---- pkg/cookies/cookies_suite_test.go | 35 +++++ pkg/cookies/cookies_test.go | 79 ++++++++++ pkg/cookies/csrf.go | 199 ++++++++++++++++++++++++ pkg/cookies/csrf_test.go | 190 ++++++++++++++++++++++ pkg/encryption/nonce.go | 36 ++++- pkg/validation/options.go | 1 + pkg/validation/sessions.go | 4 +- providers/azure.go | 2 +- providers/azure_test.go | 2 +- providers/logingov.go | 2 +- providers/logingov_test.go | 2 +- providers/oidc.go | 36 ++++- providers/oidc_test.go | 28 +++- providers/provider_data.go | 13 ++ providers/provider_data_test.go | 61 ++++++++ providers/provider_default.go | 14 +- providers/provider_default_test.go | 4 +- providers/providers.go | 2 +- 31 files changed, 855 insertions(+), 165 deletions(-) create mode 100644 pkg/cookies/cookies_suite_test.go create mode 100644 pkg/cookies/cookies_test.go create mode 100644 pkg/cookies/csrf.go create mode 100644 pkg/cookies/csrf_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a0f663a072..bb257c7e9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,14 @@ ## Important Notes +- [#967](https://github.com/oauth2-proxy/oauth2-proxy/pull/967) `--insecure-oidc-skip-nonce` is currently `true` by default in case + any existing OIDC Identity Providers don't support it. The default will switch to `false` in a future version. + ## Breaking Changes ## Changes since v7.1.2 +- [#967](https://github.com/oauth2-proxy/oauth2-proxy/pull/967) Set & verify a nonce with OIDC providers (@NickMeves) - [#1136](https://github.com/oauth2-proxy/oauth2-proxy/pull/1136) Add clock package for better time mocking in tests (@NickMeves) - [#947](https://github.com/oauth2-proxy/oauth2-proxy/pull/947) Multiple provider ingestion and validation in alpha options (first stage: [#926](https://github.com/oauth2-proxy/oauth2-proxy/issues/926)) (@yanasega) diff --git a/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md index 7f855b22dd..d2442a69af 100644 --- a/docs/docs/configuration/alpha_config.md +++ b/docs/docs/configuration/alpha_config.md @@ -264,6 +264,7 @@ make up the header value | `issuerURL` | _string_ | IssuerURL is the OpenID Connect issuer URL
eg: https://accounts.google.com | | `insecureAllowUnverifiedEmail` | _bool_ | InsecureAllowUnverifiedEmail prevents failures if an email address in an id_token is not verified
default set to 'false' | | `insecureSkipIssuerVerification` | _bool_ | InsecureSkipIssuerVerification skips verification of ID token issuers. When false, ID Token Issuers must match the OIDC discovery URL
default set to 'false' | +| `insecureSkipNonce` | _bool_ | InsecureSkipNonce skips verifying the ID Token's nonce claim that must match
the random nonce sent in the initial OAuth flow. Otherwise, the nonce is checked
after the initial OAuth redeem & subsequent token refreshes.
default set to 'true'
Warning: In a future release, this will change to 'false' by default for enhanced security. | | `skipDiscovery` | _bool_ | SkipDiscovery allows to skip OIDC discovery and use manually supplied Endpoints
default set to 'false' | | `jwksURL` | _string_ | JwksURL is the OpenID Connect JWKS URL
eg: https://www.googleapis.com/oauth2/v3/certs | | `emailClaim` | _string_ | EmailClaim indicates which claim contains the user email,
default set to 'email' | diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 9c3ada2117..4702f3217c 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -75,6 +75,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/ | `--login-url` | string | Authentication endpoint | | | `--insecure-oidc-allow-unverified-email` | bool | don't fail if an email address in an id_token is not verified | false | | `--insecure-oidc-skip-issuer-verification` | bool | allow the OIDC issuer URL to differ from the expected (currently required for Azure multi-tenant compatibility) | false | +| `--insecure-oidc-skip-nonce` | bool | skip verifying the OIDC ID Token's nonce claim | true | | `--oidc-issuer-url` | string | the OpenID Connect issuer URL, e.g. `"https://accounts.google.com"` | | | `--oidc-jwks-url` | string | OIDC JWKS URI for token verification; required if OIDC discovery is disabled | | | `--oidc-email-claim` | string | which OIDC claim contains the user's email | `"email"` | diff --git a/main_test.go b/main_test.go index 5c7d5ed13e..e40243ef5b 100644 --- a/main_test.go +++ b/main_test.go @@ -71,6 +71,7 @@ providers: groupsClaim: groups emailClaim: email userIDClaim: email + insecureSkipNonce: true ` const testCoreConfig = ` @@ -138,9 +139,10 @@ redirect_url="http://localhost:4180/oauth2/callback" Tenant: "common", }, OIDCConfig: options.OIDCOptions{ - GroupsClaim: "groups", - EmailClaim: "email", - UserIDClaim: "email", + GroupsClaim: "groups", + EmailClaim: "email", + UserIDClaim: "email", + InsecureSkipNonce: true, }, ApprovalPrompt: "force", }, @@ -228,7 +230,7 @@ redirect_url="http://localhost:4180/oauth2/callback" configContent: testCoreConfig, alphaConfigContent: testAlphaConfig + ":", expectedOptions: func() *options.Options { return nil }, - expectedErr: errors.New("failed to load alpha options: error unmarshalling config: error converting YAML to JSON: yaml: line 48: did not find expected key"), + expectedErr: errors.New("failed to load alpha options: error unmarshalling config: error converting YAML to JSON: yaml: line 49: did not find expected key"), }), Entry("with alpha configuration and bad core configuration", loadConfigurationTableInput{ configContent: testCoreConfig + "unknown_field=\"something\"", diff --git a/oauthproxy.go b/oauthproxy.go index 7081f6e213..581aad9898 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -23,8 +23,8 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/app/pagewriter" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/basic" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" proxyhttp "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/http" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/middleware" @@ -60,14 +60,8 @@ type allowedRoute struct { // OAuthProxy is the main authentication proxy type OAuthProxy struct { - CSRFCookieName string - CookieDomains []string - CookiePath string - CookieSecure bool - CookieHTTPOnly bool - CookieExpire time.Duration - CookieSameSite string - Validator func(string) bool + CookieOptions *options.Cookie + Validator func(string) bool RobotsPath string SignInPath string @@ -179,14 +173,8 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr } p := &OAuthProxy{ - CSRFCookieName: fmt.Sprintf("%v_%v", opts.Cookie.Name, "csrf"), - CookieDomains: opts.Cookie.Domains, - CookiePath: opts.Cookie.Path, - CookieSecure: opts.Cookie.Secure, - CookieHTTPOnly: opts.Cookie.HTTPOnly, - CookieExpire: opts.Cookie.Expire, - CookieSameSite: opts.Cookie.SameSite, - Validator: validator, + CookieOptions: &opts.Cookie, + Validator: validator, RobotsPath: "/robots.txt", SignInPath: fmt.Sprintf("%s/sign_in", opts.ProxyPrefix), @@ -427,47 +415,6 @@ func buildRoutesAllowlist(opts *options.Options) ([]allowedRoute, error) { return routes, nil } -// MakeCSRFCookie creates a cookie for CSRF -func (p *OAuthProxy) MakeCSRFCookie(req *http.Request, value string, expiration time.Duration, now time.Time) *http.Cookie { - return p.makeCookie(req, p.CSRFCookieName, value, expiration, now) -} - -func (p *OAuthProxy) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie { - cookieDomain := cookies.GetCookieDomain(req, p.CookieDomains) - - if cookieDomain != "" { - domain := requestutil.GetRequestHost(req) - if h, _, err := net.SplitHostPort(domain); err == nil { - domain = h - } - if !strings.HasSuffix(domain, cookieDomain) { - logger.Errorf("Warning: request host is %q but using configured cookie domain of %q", domain, cookieDomain) - } - } - - return &http.Cookie{ - Name: name, - Value: value, - Path: p.CookiePath, - Domain: cookieDomain, - HttpOnly: p.CookieHTTPOnly, - Secure: p.CookieSecure, - Expires: now.Add(expiration), - SameSite: cookies.ParseSameSite(p.CookieSameSite), - } -} - -// ClearCSRFCookie creates a cookie to unset the CSRF cookie stored in the user's -// session -func (p *OAuthProxy) ClearCSRFCookie(rw http.ResponseWriter, req *http.Request) { - http.SetCookie(rw, p.MakeCSRFCookie(req, "", time.Hour*-1, time.Now())) -} - -// SetCSRFCookie adds a CSRF cookie to the response -func (p *OAuthProxy) SetCSRFCookie(rw http.ResponseWriter, req *http.Request, val string) { - http.SetCookie(rw, p.MakeCSRFCookie(req, val, p.CookieExpire, time.Now())) -} - // ClearSessionCookie creates a cookie to unset the user's authentication cookie // stored in the user's session func (p *OAuthProxy) ClearSessionCookie(rw http.ResponseWriter, req *http.Request) error { @@ -744,21 +691,35 @@ func (p *OAuthProxy) SignOut(rw http.ResponseWriter, req *http.Request) { // OAuthStart starts the OAuth2 authentication flow func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) { prepareNoCache(rw) - nonce, err := encryption.Nonce() + + csrf, err := cookies.NewCSRF(p.CookieOptions) if err != nil { - logger.Errorf("Error obtaining nonce: %v", err) + logger.Errorf("Error creating CSRF nonce: %v", err) p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } - p.SetCSRFCookie(rw, req, nonce) - redirect, err := p.getAppRedirect(req) + + appRedirect, err := p.getAppRedirect(req) if err != nil { - logger.Errorf("Error obtaining redirect: %v", err) + logger.Errorf("Error obtaining application redirect: %v", err) p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } - redirectURI := p.getOAuthRedirectURI(req) - http.Redirect(rw, req, p.provider.GetLoginURL(redirectURI, fmt.Sprintf("%v:%v", nonce, redirect)), http.StatusFound) + + callbackRedirect := p.getOAuthRedirectURI(req) + loginURL := p.provider.GetLoginURL( + callbackRedirect, + encodeState(csrf.HashOAuthState(), appRedirect), + csrf.HashOIDCNonce(), + ) + + if _, err := csrf.SetCookie(rw, req); err != nil { + logger.Errorf("Error setting CSRF cookie: %v", err) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) + return + } + + http.Redirect(rw, req, loginURL, http.StatusFound) } // OAuthCallback is the OAuth2 authentication flow callback that finishes the @@ -796,29 +757,33 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { return } - state := strings.SplitN(req.Form.Get("state"), ":", 2) - if len(state) != 2 { - logger.Error("Error while parsing OAuth2 state: invalid length") - p.ErrorPage(rw, req, http.StatusInternalServerError, "State paremeter did not have expected length", "Login Failed: Invalid State after login.") - return - } - nonce := state[0] - redirect := state[1] - c, err := req.Cookie(p.CSRFCookieName) + csrf, err := cookies.LoadCSRFCookie(req, p.CookieOptions) if err != nil { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie") p.ErrorPage(rw, req, http.StatusForbidden, err.Error(), "Login Failed: Unable to find a valid CSRF token. Please try again.") return } - p.ClearCSRFCookie(rw, req) - if c.Value != nonce { + + csrf.ClearCookie(rw, req) + + nonce, appRedirect, err := decodeState(req) + if err != nil { + logger.Errorf("Error while parsing OAuth2 state: %v", err) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) + return + } + + if !csrf.CheckOAuthState(nonce) { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: CSRF token mismatch, potential attack") p.ErrorPage(rw, req, http.StatusForbidden, "CSRF token mismatch, potential attack", "Login Failed: Unable to find a valid CSRF token. Please try again.") return } - if !p.IsValidRedirect(redirect) { - redirect = "/" + csrf.SetSessionNonce(session) + p.provider.ValidateSession(req.Context(), session) + + if !p.IsValidRedirect(appRedirect) { + appRedirect = "/" } // set cookie, or deny @@ -834,7 +799,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } - http.Redirect(rw, req, redirect, http.StatusFound) + http.Redirect(rw, req, appRedirect, http.StatusFound) } else { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unauthorized") p.ErrorPage(rw, req, http.StatusForbidden, "Invalid session: unauthorized") @@ -966,7 +931,7 @@ func (p *OAuthProxy) getOAuthRedirectURI(req *http.Request) string { // If CookieSecure is true, return `https` no matter what // Not all reverse proxies set X-Forwarded-Proto - if p.CookieSecure { + if p.CookieOptions.Secure { rd.Scheme = schemeHTTPS } return rd.String() @@ -1207,6 +1172,22 @@ func extractAllowedGroups(req *http.Request) map[string]struct{} { return groups } +// encodedState builds the OAuth state param out of our nonce and +// original application redirect +func encodeState(nonce string, redirect string) string { + return fmt.Sprintf("%v:%v", nonce, redirect) +} + +// decodeState splits the reflected OAuth state response back into +// the nonce and original application redirect +func decodeState(req *http.Request) (string, string, error) { + state := strings.SplitN(req.Form.Get("state"), ":", 2) + if len(state) != 2 { + return "", "", errors.New("invalid length") + } + return state[0], state[1], nil +} + // addHeadersForProxying adds the appropriate headers the request / response for proxying func (p *OAuthProxy) addHeadersForProxying(rw http.ResponseWriter, session *sessionsapi.SessionState) { if session.Email == "" { diff --git a/oauthproxy_test.go b/oauthproxy_test.go index e8d705ed83..d867839695 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -22,6 +22,7 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" sessionscookie "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/cookie" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/upstream" @@ -698,23 +699,42 @@ func (patTest *PassAccessTokenTest) Close() { patTest.providerServer.Close() } -func (patTest *PassAccessTokenTest) getCallbackEndpoint() (httpCode int, - cookie string) { +func (patTest *PassAccessTokenTest) getCallbackEndpoint() (httpCode int, cookie string) { rw := httptest.NewRecorder() - req, err := http.NewRequest("GET", "/oauth2/callback?code=callback_code&state=nonce:", - strings.NewReader("")) + + csrf, err := cookies.NewCSRF(patTest.proxy.CookieOptions) + if err != nil { + panic(err) + } + + req, err := http.NewRequest( + http.MethodGet, + fmt.Sprintf( + "/oauth2/callback?code=callback_code&state=%s", + encodeState(csrf.HashOAuthState(), "%2F"), + ), + strings.NewReader(""), + ) if err != nil { return 0, "" } - req.AddCookie(patTest.proxy.MakeCSRFCookie(req, "nonce", time.Hour, time.Now())) + + // rw is a dummy here, we just want the csrfCookie to add to our req + csrfCookie, err := csrf.SetCookie(httptest.NewRecorder(), req) + if err != nil { + panic(err) + } + req.AddCookie(csrfCookie) + patTest.proxy.ServeHTTP(rw, req) + return rw.Code, rw.Header().Values("Set-Cookie")[1] } // getEndpointWithCookie makes a requests againt the oauthproxy with passed requestPath // and cookie and returns body and status code. func (patTest *PassAccessTokenTest) getEndpointWithCookie(cookie string, endpoint string) (httpCode int, accessToken string) { - cookieName := patTest.opts.Cookie.Name + cookieName := patTest.proxy.CookieOptions.Name var value string keyPrefix := cookieName + "=" @@ -983,6 +1003,9 @@ func NewProcessCookieTest(opts ProcessCookieTestOpts, modifiers ...OptionsModifi } pcTest.proxy.provider.(*TestProvider).SetAllowedGroups(pcTest.opts.Providers[0].AllowedGroups) + // Now, zero-out proxy.CookieRefresh for the cases that don't involve + // access_token validation. + pcTest.proxy.CookieOptions.Refresh = time.Duration(0) pcTest.rw = httptest.NewRecorder() pcTest.req, _ = http.NewRequest("GET", "/", strings.NewReader("")) pcTest.validateUser = true @@ -1104,6 +1127,7 @@ func TestProcessCookieFailIfRefreshSetAndCookieExpired(t *testing.T) { err = pcTest.SaveSession(startSession) assert.NoError(t, err) + pcTest.proxy.CookieOptions.Refresh = time.Hour session, err := pcTest.LoadCookiedSession() assert.NotEqual(t, nil, err) if session != nil { @@ -1999,7 +2023,7 @@ func TestClearSplitCookie(t *testing.T) { t.Fatal(err) } - p := OAuthProxy{sessionStore: store} + p := OAuthProxy{CookieOptions: &opts.Cookie, sessionStore: store} var rw = httptest.NewRecorder() req := httptest.NewRequest("get", "/", nil) @@ -2032,7 +2056,7 @@ func TestClearSingleCookie(t *testing.T) { t.Fatal(err) } - p := OAuthProxy{sessionStore: store} + p := OAuthProxy{CookieOptions: &opts.Cookie, sessionStore: store} var rw = httptest.NewRecorder() req := httptest.NewRequest("get", "/", nil) diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 5519963acb..76367b8f3a 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -48,12 +48,13 @@ func NewLegacyOptions() *LegacyOptions { }, LegacyProvider: LegacyProvider{ - ProviderType: "google", - AzureTenant: "common", - ApprovalPrompt: "force", - UserIDClaim: "email", - OIDCEmailClaim: "email", - OIDCGroupsClaim: "groups", + ProviderType: "google", + AzureTenant: "common", + ApprovalPrompt: "force", + UserIDClaim: "email", + OIDCEmailClaim: "email", + OIDCGroupsClaim: "groups", + InsecureOIDCSkipNonce: true, }, Options: *NewOptions(), @@ -492,6 +493,7 @@ type LegacyProvider struct { OIDCIssuerURL string `flag:"oidc-issuer-url" cfg:"oidc_issuer_url"` InsecureOIDCAllowUnverifiedEmail bool `flag:"insecure-oidc-allow-unverified-email" cfg:"insecure_oidc_allow_unverified_email"` InsecureOIDCSkipIssuerVerification bool `flag:"insecure-oidc-skip-issuer-verification" cfg:"insecure_oidc_skip_issuer_verification"` + InsecureOIDCSkipNonce bool `flag:"insecure-oidc-skip-nonce" cfg:"insecure_oidc_skip_nonce"` SkipOIDCDiscovery bool `flag:"skip-oidc-discovery" cfg:"skip_oidc_discovery"` OIDCJwksURL string `flag:"oidc-jwks-url" cfg:"oidc_jwks_url"` OIDCEmailClaim string `flag:"oidc-email-claim" cfg:"oidc_email_claim"` @@ -540,6 +542,7 @@ func legacyProviderFlagSet() *pflag.FlagSet { flagSet.String("oidc-issuer-url", "", "OpenID Connect issuer URL (ie: https://accounts.google.com)") flagSet.Bool("insecure-oidc-allow-unverified-email", false, "Don't fail if an email address in an id_token is not verified") flagSet.Bool("insecure-oidc-skip-issuer-verification", false, "Do not verify if issuer matches OIDC discovery URL") + flagSet.Bool("insecure-oidc-skip-nonce", true, "skip verifying the OIDC ID Token's nonce claim") flagSet.Bool("skip-oidc-discovery", false, "Skip OIDC discovery and use manually supplied Endpoints") flagSet.String("oidc-jwks-url", "", "OpenID Connect JWKS URL (ie: https://www.googleapis.com/oauth2/v3/certs)") flagSet.String("oidc-groups-claim", providers.OIDCGroupsClaim, "which OIDC claim contains the user groups") @@ -630,6 +633,7 @@ func (l *LegacyProvider) convert() (Providers, error) { IssuerURL: l.OIDCIssuerURL, InsecureAllowUnverifiedEmail: l.InsecureOIDCAllowUnverifiedEmail, InsecureSkipIssuerVerification: l.InsecureOIDCSkipIssuerVerification, + InsecureSkipNonce: l.InsecureOIDCSkipNonce, SkipDiscovery: l.SkipOIDCDiscovery, JwksURL: l.OIDCJwksURL, UserIDClaim: l.UserIDClaim, diff --git a/pkg/apis/options/legacy_options_test.go b/pkg/apis/options/legacy_options_test.go index ee593142a2..fb5b816ac1 100644 --- a/pkg/apis/options/legacy_options_test.go +++ b/pkg/apis/options/legacy_options_test.go @@ -113,6 +113,7 @@ var _ = Describe("Legacy Options", func() { opts.Providers[0].ClientID = "oauth-proxy" opts.Providers[0].ID = "google=oauth-proxy" + opts.Providers[0].OIDCConfig.InsecureSkipNonce = true converted, err := legacyOpts.ToOptions() Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/apis/options/load_test.go b/pkg/apis/options/load_test.go index 145ef3bb7c..dce5d2d90c 100644 --- a/pkg/apis/options/load_test.go +++ b/pkg/apis/options/load_test.go @@ -36,12 +36,13 @@ var _ = Describe("Load", func() { }, LegacyProvider: LegacyProvider{ - ProviderType: "google", - AzureTenant: "common", - ApprovalPrompt: "force", - UserIDClaim: "email", - OIDCEmailClaim: "email", - OIDCGroupsClaim: "groups", + ProviderType: "google", + AzureTenant: "common", + ApprovalPrompt: "force", + UserIDClaim: "email", + OIDCEmailClaim: "email", + OIDCGroupsClaim: "groups", + InsecureOIDCSkipNonce: true, }, Options: Options{ diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index 94a8e4c7d3..3b8d0da4e4 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -132,6 +132,12 @@ type OIDCOptions struct { // InsecureSkipIssuerVerification skips verification of ID token issuers. When false, ID Token Issuers must match the OIDC discovery URL // default set to 'false' InsecureSkipIssuerVerification bool `json:"insecureSkipIssuerVerification,omitempty"` + // InsecureSkipNonce skips verifying the ID Token's nonce claim that must match + // the random nonce sent in the initial OAuth flow. Otherwise, the nonce is checked + // after the initial OAuth redeem & subsequent token refreshes. + // default set to 'true' + // Warning: In a future release, this will change to 'false' by default for enhanced security. + InsecureSkipNonce bool `json:"insecureSkipNonce,omitempty"` // SkipDiscovery allows to skip OIDC discovery and use manually supplied Endpoints // default set to 'false' SkipDiscovery bool `json:"skipDiscovery,omitempty"` @@ -169,6 +175,7 @@ func providerDefaults() Providers { }, OIDCConfig: OIDCOptions{ InsecureAllowUnverifiedEmail: false, + InsecureSkipNonce: true, SkipDiscovery: false, UserIDClaim: providers.OIDCEmailClaim, // Deprecated: Use OIDCEmailClaim EmailClaim: providers.OIDCEmailClaim, diff --git a/pkg/apis/sessions/session_state.go b/pkg/apis/sessions/session_state.go index 8e9d006b07..c3441fe316 100644 --- a/pkg/apis/sessions/session_state.go +++ b/pkg/apis/sessions/session_state.go @@ -24,6 +24,8 @@ type SessionState struct { IDToken string `msgpack:"it,omitempty"` RefreshToken string `msgpack:"rt,omitempty"` + Nonce []byte `msgpack:"n,omitempty"` + Email string `msgpack:"e,omitempty"` User string `msgpack:"u,omitempty"` Groups []string `msgpack:"g,omitempty"` @@ -100,6 +102,11 @@ func (s *SessionState) GetClaim(claim string) []string { } } +// CheckNonce compares the Nonce against a potential hash of it +func (s *SessionState) CheckNonce(hashed string) bool { + return encryption.CheckNonce(s.Nonce, hashed) +} + // EncodeSessionState returns an encrypted, lz4 compressed, MessagePack encoded session func (s *SessionState) EncodeSessionState(c encryption.Cipher, compress bool) ([]byte, error) { packed, err := msgpack.Marshal(s) diff --git a/pkg/apis/sessions/session_state_test.go b/pkg/apis/sessions/session_state_test.go index c28420a94f..0121b4b67f 100644 --- a/pkg/apis/sessions/session_state_test.go +++ b/pkg/apis/sessions/session_state_test.go @@ -153,6 +153,7 @@ func TestEncodeAndDecodeSessionState(t *testing.T) { CreatedAt: &created, ExpiresOn: &expires, RefreshToken: "RefreshToken.12349871293847fdsaihf9238h4f91h8fr.1349f831y98fd7", + Nonce: []byte("abcdef1234567890abcdef1234567890"), }, "No ExpiresOn": { Email: "username@example.com", @@ -162,6 +163,7 @@ func TestEncodeAndDecodeSessionState(t *testing.T) { IDToken: "IDToken.12349871293847fdsaihf9238h4f91h8fr.1349f831y98fd7", CreatedAt: &created, RefreshToken: "RefreshToken.12349871293847fdsaihf9238h4f91h8fr.1349f831y98fd7", + Nonce: []byte("abcdef1234567890abcdef1234567890"), }, "No PreferredUsername": { Email: "username@example.com", @@ -171,6 +173,7 @@ func TestEncodeAndDecodeSessionState(t *testing.T) { CreatedAt: &created, ExpiresOn: &expires, RefreshToken: "RefreshToken.12349871293847fdsaihf9238h4f91h8fr.1349f831y98fd7", + Nonce: []byte("abcdef1234567890abcdef1234567890"), }, "Minimal session": { User: "username", @@ -194,6 +197,7 @@ func TestEncodeAndDecodeSessionState(t *testing.T) { CreatedAt: &created, ExpiresOn: &expires, RefreshToken: "RefreshToken.12349871293847fdsaihf9238h4f91h8fr.1349f831y98fd7", + Nonce: []byte("abcdef1234567890abcdef1234567890"), Groups: []string{"group-a", "group-b"}, }, } diff --git a/pkg/cookies/cookies.go b/pkg/cookies/cookies.go index 80453ab2dc..dc1bff762e 100644 --- a/pkg/cookies/cookies.go +++ b/pkg/cookies/cookies.go @@ -12,46 +12,33 @@ import ( requestutil "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests/util" ) -// MakeCookie constructs a cookie from the given parameters, -// discovering the domain from the request if not specified. -func MakeCookie(req *http.Request, name string, value string, path string, domain string, httpOnly bool, secure bool, expiration time.Duration, now time.Time, sameSite http.SameSite) *http.Cookie { - if domain != "" { - host := requestutil.GetRequestHost(req) - if h, _, err := net.SplitHostPort(host); err == nil { - host = h - } - if !strings.HasSuffix(host, domain) { - logger.Errorf("Warning: request host is %q but using configured cookie domain of %q", host, domain) - } +// MakeCookieFromOptions constructs a cookie based on the given *options.CookieOptions, +// value and creation time +func MakeCookieFromOptions(req *http.Request, name string, value string, opts *options.Cookie, expiration time.Duration, now time.Time) *http.Cookie { + domain := GetCookieDomain(req, opts.Domains) + // If nothing matches, create the cookie with the shortest domain + if domain == "" && len(opts.Domains) > 0 { + logger.Errorf("Warning: request host %q did not match any of the specific cookie domains of %q", + requestutil.GetRequestHost(req), + strings.Join(opts.Domains, ","), + ) + domain = opts.Domains[len(opts.Domains)-1] } - return &http.Cookie{ + c := &http.Cookie{ Name: name, Value: value, - Path: path, + Path: opts.Path, Domain: domain, - HttpOnly: httpOnly, - Secure: secure, Expires: now.Add(expiration), - SameSite: sameSite, + HttpOnly: opts.HTTPOnly, + Secure: opts.Secure, + SameSite: ParseSameSite(opts.SameSite), } -} -// MakeCookieFromOptions constructs a cookie based on the given *options.CookieOptions, -// value and creation time -func MakeCookieFromOptions(req *http.Request, name string, value string, cookieOpts *options.Cookie, expiration time.Duration, now time.Time) *http.Cookie { - domain := GetCookieDomain(req, cookieOpts.Domains) + warnInvalidDomain(c, req) - if domain != "" { - return MakeCookie(req, name, value, cookieOpts.Path, domain, cookieOpts.HTTPOnly, cookieOpts.Secure, expiration, now, ParseSameSite(cookieOpts.SameSite)) - } - // If nothing matches, create the cookie with the shortest domain - defaultDomain := "" - if len(cookieOpts.Domains) > 0 { - logger.Errorf("Warning: request host %q did not match any of the specific cookie domains of %q", requestutil.GetRequestHost(req), strings.Join(cookieOpts.Domains, ",")) - defaultDomain = cookieOpts.Domains[len(cookieOpts.Domains)-1] - } - return MakeCookie(req, name, value, cookieOpts.Path, defaultDomain, cookieOpts.HTTPOnly, cookieOpts.Secure, expiration, now, ParseSameSite(cookieOpts.SameSite)) + return c } // GetCookieDomain returns the correct cookie domain given a list of domains @@ -81,3 +68,19 @@ func ParseSameSite(v string) http.SameSite { panic(fmt.Sprintf("Invalid value for SameSite: %s", v)) } } + +// warnInvalidDomain logs a warning if the request host and cookie domain are +// mismatched. +func warnInvalidDomain(c *http.Cookie, req *http.Request) { + if c.Domain == "" { + return + } + + host := requestutil.GetRequestHost(req) + if h, _, err := net.SplitHostPort(host); err == nil { + host = h + } + if !strings.HasSuffix(host, c.Domain) { + logger.Errorf("Warning: request host is %q but using configured cookie domain of %q", host, c.Domain) + } +} diff --git a/pkg/cookies/cookies_suite_test.go b/pkg/cookies/cookies_suite_test.go new file mode 100644 index 0000000000..208cae32b6 --- /dev/null +++ b/pkg/cookies/cookies_suite_test.go @@ -0,0 +1,35 @@ +package cookies + +import ( + "net/http" + "testing" + "time" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +const ( + csrfState = "1234asdf1234asdf1234asdf" + csrfNonce = "0987lkjh0987lkjh0987lkjh" + + cookieName = "cookie_test_12345" + cookieSecret = "3q48hmFH30FJ2HfJF0239UFJCVcl3kj3" + cookieDomain = "o2p.cookies.test" + cookiePath = "/cookie-tests" + + nowEpoch = 1609366421 +) + +func TestProviderSuite(t *testing.T) { + logger.SetOutput(GinkgoWriter) + + RegisterFailHandler(Fail) + RunSpecs(t, "Cookies") +} + +func testCookieExpires(exp time.Time) string { + var buf [len(http.TimeFormat)]byte + return string(exp.UTC().AppendFormat(buf[:0], http.TimeFormat)) +} diff --git a/pkg/cookies/cookies_test.go b/pkg/cookies/cookies_test.go new file mode 100644 index 0000000000..7f6c03193a --- /dev/null +++ b/pkg/cookies/cookies_test.go @@ -0,0 +1,79 @@ +package cookies + +import ( + "fmt" + "net/http" + + middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" + . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/extensions/table" + . "github.com/onsi/gomega" +) + +var _ = Describe("Cookie Tests", func() { + Context("GetCookieDomain", func() { + type getCookieDomainTableInput struct { + host string + xForwardedHost string + cookieDomains []string + expectedOutput string + } + + DescribeTable("should return expected results", + func(in getCookieDomainTableInput) { + req, err := http.NewRequest( + http.MethodGet, + fmt.Sprintf("https://%s/%s", in.host, cookiePath), + nil, + ) + Expect(err).ToNot(HaveOccurred()) + + if in.xForwardedHost != "" { + req.Header.Add("X-Forwarded-Host", in.xForwardedHost) + req = middlewareapi.AddRequestScope(req, &middlewareapi.RequestScope{ + ReverseProxy: true, + }) + } + + Expect(GetCookieDomain(req, in.cookieDomains)).To(Equal(in.expectedOutput)) + }, + Entry("a single exact match for the Host header", getCookieDomainTableInput{ + host: "www.cookies.test", + cookieDomains: []string{"www.cookies.test"}, + expectedOutput: "www.cookies.test", + }), + Entry("a single exact match for the X-Forwarded-Host header", getCookieDomainTableInput{ + host: "backend.cookies.internal", + xForwardedHost: "www.cookies.test", + cookieDomains: []string{"www.cookies.test"}, + expectedOutput: "www.cookies.test", + }), + Entry("a single suffix match for the Host header", getCookieDomainTableInput{ + host: "www.cookies.test", + cookieDomains: []string{".cookies.test"}, + expectedOutput: ".cookies.test", + }), + Entry("a single suffix match for the X-Forwarded-Host header", getCookieDomainTableInput{ + host: "backend.cookies.internal", + xForwardedHost: "www.cookies.test", + cookieDomains: []string{".cookies.test"}, + expectedOutput: ".cookies.test", + }), + Entry("the first match is used", getCookieDomainTableInput{ + host: "www.cookies.test", + cookieDomains: []string{"www.cookies.test", ".cookies.test"}, + expectedOutput: "www.cookies.test", + }), + Entry("the only match is used", getCookieDomainTableInput{ + host: "www.cookies.test", + cookieDomains: []string{".cookies.wrong", ".cookies.test"}, + expectedOutput: ".cookies.test", + }), + Entry("blank is returned for no matches", getCookieDomainTableInput{ + host: "www.cookies.test", + cookieDomains: []string{".cookies.wrong", ".cookies.false"}, + expectedOutput: "", + }), + ) + }) +}) diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go new file mode 100644 index 0000000000..0af74173e7 --- /dev/null +++ b/pkg/cookies/csrf.go @@ -0,0 +1,199 @@ +package cookies + +import ( + "errors" + "fmt" + "net/http" + "time" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/clock" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" + "github.com/vmihailenco/msgpack/v4" +) + +// CSRF manages various nonces stored in the CSRF cookie during the initial +// authentication flows. +type CSRF interface { + HashOAuthState() string + HashOIDCNonce() string + CheckOAuthState(string) bool + CheckOIDCNonce(string) bool + + SetSessionNonce(s *sessions.SessionState) + + SetCookie(http.ResponseWriter, *http.Request) (*http.Cookie, error) + ClearCookie(http.ResponseWriter, *http.Request) +} + +type csrf struct { + // OAuthState holds the OAuth2 state parameter's nonce component set in the + // initial authentication request and mirrored back in the callback + // redirect from the IdP for CSRF protection. + OAuthState []byte `msgpack:"s,omitempty"` + + // OIDCNonce holds the OIDC nonce parameter used in the initial authentication + // and then set in all subsequent OIDC ID Tokens as the nonce claim. This + // is used to mitigate replay attacks. + OIDCNonce []byte `msgpack:"n,omitempty"` + + cookieOpts *options.Cookie + time clock.Clock +} + +// NewCSRF creates a CSRF with random nonces +func NewCSRF(opts *options.Cookie) (CSRF, error) { + state, err := encryption.Nonce() + if err != nil { + return nil, err + } + nonce, err := encryption.Nonce() + if err != nil { + return nil, err + } + + return &csrf{ + OAuthState: state, + OIDCNonce: nonce, + + cookieOpts: opts, + }, nil +} + +// LoadCSRFCookie loads a CSRF object from a request's CSRF cookie +func LoadCSRFCookie(req *http.Request, opts *options.Cookie) (CSRF, error) { + cookie, err := req.Cookie(csrfCookieName(opts)) + if err != nil { + return nil, err + } + + return decodeCSRFCookie(cookie, opts) +} + +// HashOAuthState returns the hash of the OAuth state nonce +func (c *csrf) HashOAuthState() string { + return encryption.HashNonce(c.OAuthState) +} + +// HashOIDCNonce returns the hash of the OIDC nonce +func (c *csrf) HashOIDCNonce() string { + return encryption.HashNonce(c.OIDCNonce) +} + +// CheckOAuthState compares the OAuth state nonce against a potential +// hash of it +func (c *csrf) CheckOAuthState(hashed string) bool { + return encryption.CheckNonce(c.OAuthState, hashed) +} + +// CheckOIDCNonce compares the OIDC nonce against a potential hash of it +func (c *csrf) CheckOIDCNonce(hashed string) bool { + return encryption.CheckNonce(c.OIDCNonce, hashed) +} + +// SetSessionNonce sets the OIDCNonce on a SessionState +func (c *csrf) SetSessionNonce(s *sessions.SessionState) { + s.Nonce = c.OIDCNonce +} + +// SetCookie encodes the CSRF to a signed cookie and sets it on the ResponseWriter +func (c *csrf) SetCookie(rw http.ResponseWriter, req *http.Request) (*http.Cookie, error) { + encoded, err := c.encodeCookie() + if err != nil { + return nil, err + } + + cookie := MakeCookieFromOptions( + req, + c.cookieName(), + encoded, + c.cookieOpts, + c.cookieOpts.Expire, + c.time.Now(), + ) + http.SetCookie(rw, cookie) + + return cookie, nil +} + +// ClearCookie removes the CSRF cookie +func (c *csrf) ClearCookie(rw http.ResponseWriter, req *http.Request) { + http.SetCookie(rw, MakeCookieFromOptions( + req, + c.cookieName(), + "", + c.cookieOpts, + time.Hour*-1, + c.time.Now(), + )) +} + +// encodeCookie MessagePack encodes and encrypts the CSRF and then creates a +// signed cookie value +func (c *csrf) encodeCookie() (string, error) { + packed, err := msgpack.Marshal(c) + if err != nil { + return "", fmt.Errorf("error marshalling CSRF to msgpack: %v", err) + } + + encrypted, err := encrypt(packed, c.cookieOpts) + if err != nil { + return "", err + } + + return encryption.SignedValue(c.cookieOpts.Secret, c.cookieName(), encrypted, c.time.Now()) +} + +// decodeCSRFCookie validates the signature then decrypts and decodes a CSRF +// cookie into a CSRF struct +func decodeCSRFCookie(cookie *http.Cookie, opts *options.Cookie) (*csrf, error) { + val, _, ok := encryption.Validate(cookie, opts.Secret, opts.Expire) + if !ok { + return nil, errors.New("CSRF cookie failed validation") + } + + decrypted, err := decrypt(val, opts) + if err != nil { + return nil, err + } + + // Valid cookie, Unmarshal the CSRF + csrf := &csrf{cookieOpts: opts} + err = msgpack.Unmarshal(decrypted, csrf) + if err != nil { + return nil, fmt.Errorf("error unmarshalling data to CSRF: %v", err) + } + + return csrf, nil +} + +// cookieName returns the CSRF cookie's name derived from the base +// session cookie name +func (c *csrf) cookieName() string { + return csrfCookieName(c.cookieOpts) +} + +func csrfCookieName(opts *options.Cookie) string { + return fmt.Sprintf("%v_csrf", opts.Name) +} + +func encrypt(data []byte, opts *options.Cookie) ([]byte, error) { + cipher, err := makeCipher(opts) + if err != nil { + return nil, err + } + return cipher.Encrypt(data) +} + +func decrypt(data []byte, opts *options.Cookie) ([]byte, error) { + cipher, err := makeCipher(opts) + if err != nil { + return nil, err + } + return cipher.Decrypt(data) +} + +func makeCipher(opts *options.Cookie) (encryption.Cipher, error) { + return encryption.NewCFBCipher(encryption.SecretBytes(opts.Secret)) +} diff --git a/pkg/cookies/csrf_test.go b/pkg/cookies/csrf_test.go new file mode 100644 index 0000000000..85f3e750ec --- /dev/null +++ b/pkg/cookies/csrf_test.go @@ -0,0 +1,190 @@ +package cookies + +import ( + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "time" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("CSRF Cookie Tests", func() { + var ( + cookieOpts *options.Cookie + publicCSRF CSRF + privateCSRF *csrf + ) + + BeforeEach(func() { + cookieOpts = &options.Cookie{ + Name: cookieName, + Secret: cookieSecret, + Domains: []string{cookieDomain}, + Path: cookiePath, + Expire: time.Hour, + Secure: true, + HTTPOnly: true, + } + + var err error + publicCSRF, err = NewCSRF(cookieOpts) + Expect(err).ToNot(HaveOccurred()) + + privateCSRF = publicCSRF.(*csrf) + }) + + Context("NewCSRF", func() { + It("makes unique nonces for OAuth and OIDC", func() { + Expect(privateCSRF.OAuthState).ToNot(BeEmpty()) + Expect(privateCSRF.OIDCNonce).ToNot(BeEmpty()) + Expect(privateCSRF.OAuthState).ToNot(Equal(privateCSRF.OIDCNonce)) + }) + + It("makes unique nonces between multiple CSRFs", func() { + other, err := NewCSRF(cookieOpts) + Expect(err).ToNot(HaveOccurred()) + + Expect(privateCSRF.OAuthState).ToNot(Equal(other.(*csrf).OAuthState)) + Expect(privateCSRF.OIDCNonce).ToNot(Equal(other.(*csrf).OIDCNonce)) + }) + }) + + Context("CheckOAuthState and CheckOIDCNonce", func() { + It("checks that hashed versions match", func() { + privateCSRF.OAuthState = []byte(csrfState) + privateCSRF.OIDCNonce = []byte(csrfNonce) + + stateHashed := encryption.HashNonce([]byte(csrfState)) + nonceHashed := encryption.HashNonce([]byte(csrfNonce)) + + Expect(publicCSRF.CheckOAuthState(stateHashed)).To(BeTrue()) + Expect(publicCSRF.CheckOIDCNonce(nonceHashed)).To(BeTrue()) + + Expect(publicCSRF.CheckOAuthState(csrfNonce)).To(BeFalse()) + Expect(publicCSRF.CheckOIDCNonce(csrfState)).To(BeFalse()) + Expect(publicCSRF.CheckOAuthState(csrfState + csrfNonce)).To(BeFalse()) + Expect(publicCSRF.CheckOIDCNonce(csrfNonce + csrfState)).To(BeFalse()) + Expect(publicCSRF.CheckOAuthState("")).To(BeFalse()) + Expect(publicCSRF.CheckOIDCNonce("")).To(BeFalse()) + }) + }) + + Context("SetSessionNonce", func() { + It("sets the session.Nonce", func() { + session := &sessions.SessionState{} + publicCSRF.SetSessionNonce(session) + Expect(session.Nonce).To(Equal(privateCSRF.OIDCNonce)) + }) + }) + + Context("encodeCookie and decodeCSRFCookie", func() { + It("encodes and decodes to the same nonces", func() { + privateCSRF.OAuthState = []byte(csrfState) + privateCSRF.OIDCNonce = []byte(csrfNonce) + + encoded, err := privateCSRF.encodeCookie() + Expect(err).ToNot(HaveOccurred()) + + cookie := &http.Cookie{ + Name: privateCSRF.cookieName(), + Value: encoded, + } + decoded, err := decodeCSRFCookie(cookie, cookieOpts) + Expect(err).ToNot(HaveOccurred()) + + Expect(decoded).ToNot(BeNil()) + Expect(decoded.OAuthState).To(Equal([]byte(csrfState))) + Expect(decoded.OIDCNonce).To(Equal([]byte(csrfNonce))) + }) + + It("signs the encoded cookie value", func() { + encoded, err := privateCSRF.encodeCookie() + Expect(err).ToNot(HaveOccurred()) + + cookie := &http.Cookie{ + Name: privateCSRF.cookieName(), + Value: encoded, + } + + _, _, valid := encryption.Validate(cookie, cookieOpts.Secret, cookieOpts.Expire) + Expect(valid).To(BeTrue()) + }) + }) + + Context("Cookie Management", func() { + var req *http.Request + + testNow := time.Unix(nowEpoch, 0) + + BeforeEach(func() { + privateCSRF.time.Set(testNow) + + req = &http.Request{ + Method: http.MethodGet, + Proto: "HTTP/1.1", + Host: cookieDomain, + + URL: &url.URL{ + Scheme: "https", + Host: cookieDomain, + Path: cookiePath, + }, + } + }) + + AfterEach(func() { + privateCSRF.time.Reset() + }) + + Context("SetCookie", func() { + It("adds the encoded CSRF cookie to a ResponseWriter", func() { + rw := httptest.NewRecorder() + + _, err := publicCSRF.SetCookie(rw, req) + Expect(err).ToNot(HaveOccurred()) + + Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring( + fmt.Sprintf("%s=", privateCSRF.cookieName()), + )) + Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring( + fmt.Sprintf( + "; Path=%s; Domain=%s; Expires=%s; HttpOnly; Secure", + cookiePath, + cookieDomain, + testCookieExpires(testNow.Add(cookieOpts.Expire)), + ), + )) + }) + }) + + Context("ClearCookie", func() { + It("sets a cookie with an empty value in the past", func() { + rw := httptest.NewRecorder() + + publicCSRF.ClearCookie(rw, req) + + Expect(rw.Header().Get("Set-Cookie")).To(Equal( + fmt.Sprintf( + "%s=; Path=%s; Domain=%s; Expires=%s; HttpOnly; Secure", + privateCSRF.cookieName(), + cookiePath, + cookieDomain, + testCookieExpires(testNow.Add(time.Hour*-1)), + ), + )) + }) + }) + + Context("cookieName", func() { + It("has the cookie options name as a base", func() { + Expect(privateCSRF.cookieName()).To(ContainSubstring(cookieName)) + }) + }) + }) +}) diff --git a/pkg/encryption/nonce.go b/pkg/encryption/nonce.go index 69850c4e9b..39e3b52026 100644 --- a/pkg/encryption/nonce.go +++ b/pkg/encryption/nonce.go @@ -1,17 +1,37 @@ package encryption import ( + "crypto/hmac" "crypto/rand" - "fmt" + "encoding/base64" + + "golang.org/x/crypto/blake2b" ) -// Nonce generates a random 16 byte string to be used as a nonce -func Nonce() (nonce string, err error) { - b := make([]byte, 16) - _, err = rand.Read(b) +// Nonce generates a random 32-byte slice to be used as a nonce +func Nonce() ([]byte, error) { + b := make([]byte, 32) + _, err := rand.Read(b) if err != nil { - return + return nil, err } - nonce = fmt.Sprintf("%x", b) - return + return b, nil +} + +// HashNonce returns the BLAKE2b 256-bit hash of a nonce +// NOTE: Error checking (G104) is purposefully skipped: +// - `blake2b.New256` has no error path with a nil signing key +// - `hash.Hash` interface's `Write` has an error signature, but +// `blake2b.digest.Write` does not use it. +/* #nosec G104 */ +func HashNonce(nonce []byte) string { + hasher, _ := blake2b.New256(nil) + hasher.Write(nonce) + sum := hasher.Sum(nil) + return base64.RawURLEncoding.EncodeToString(sum) +} + +// CheckNonce tests if a nonce matches the hashed version of it +func CheckNonce(nonce []byte, hashed string) bool { + return hmac.Equal([]byte(HashNonce(nonce)), []byte(hashed)) } diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 5e36f894b7..2e5b884d56 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -264,6 +264,7 @@ func parseProviderInfo(o *options.Options, msgs []string) []string { p.SetTeam(o.Providers[0].BitbucketConfig.Team) p.SetRepository(o.Providers[0].BitbucketConfig.Repository) case *providers.OIDCProvider: + p.SkipNonce = o.Providers[0].OIDCConfig.InsecureSkipNonce if p.Verifier == nil { msgs = append(msgs, "oidc provider requires an oidc issuer URL") } diff --git a/pkg/validation/sessions.go b/pkg/validation/sessions.go index 48d4042a53..5944bf7b3b 100644 --- a/pkg/validation/sessions.go +++ b/pkg/validation/sessions.go @@ -2,6 +2,7 @@ package validation import ( "context" + "encoding/base64" "fmt" "time" @@ -50,10 +51,11 @@ func validateRedisSessionStore(o *options.Options) []string { return []string{fmt.Sprintf("unable to initialize a redis client: %v", err)} } - nonce, err := encryption.Nonce() + n, err := encryption.Nonce() if err != nil { return []string{fmt.Sprintf("unable to generate a redis initialization test key: %v", err)} } + nonce := base64.RawURLEncoding.EncodeToString(n) key := fmt.Sprintf("%s-healthcheck-%s", o.Cookie.Name, nonce) return sendRedisConnectionTest(client, key, nonce) diff --git a/providers/azure.go b/providers/azure.go index 036fefae64..f66d3764cc 100644 --- a/providers/azure.go +++ b/providers/azure.go @@ -107,7 +107,7 @@ func overrideTenantURL(current, defaultURL *url.URL, tenant, path string) { } } -func (p *AzureProvider) GetLoginURL(redirectURI, state string) string { +func (p *AzureProvider) GetLoginURL(redirectURI, state, _ string) string { extraParams := url.Values{} if p.ProtectedResource != nil && p.ProtectedResource.String() != "" { extraParams.Add("resource", p.ProtectedResource.String()) diff --git a/providers/azure_test.go b/providers/azure_test.go index 35509b0d47..bb44f20ad7 100644 --- a/providers/azure_test.go +++ b/providers/azure_test.go @@ -336,7 +336,7 @@ func TestAzureProviderRedeem(t *testing.T) { func TestAzureProviderProtectedResourceConfigured(t *testing.T) { p := testAzureProvider("") p.ProtectedResource, _ = url.Parse("http://my.resource.test") - result := p.GetLoginURL("https://my.test.app/oauth", "") + result := p.GetLoginURL("https://my.test.app/oauth", "", "") assert.Contains(t, result, "resource="+url.QueryEscape("http://my.resource.test")) } diff --git a/providers/logingov.go b/providers/logingov.go index 9e70c857ea..0f62520886 100644 --- a/providers/logingov.go +++ b/providers/logingov.go @@ -228,7 +228,7 @@ func (p *LoginGovProvider) Redeem(ctx context.Context, redirectURL, code string) } // GetLoginURL overrides GetLoginURL to add login.gov parameters -func (p *LoginGovProvider) GetLoginURL(redirectURI, state string) string { +func (p *LoginGovProvider) GetLoginURL(redirectURI, state, _ string) string { extraParams := url.Values{} if p.AcrValues == "" { acr := "http://idmanagement.gov/ns/assurance/loa/1" diff --git a/providers/logingov_test.go b/providers/logingov_test.go index 0b70190b57..2287e643ae 100644 --- a/providers/logingov_test.go +++ b/providers/logingov_test.go @@ -292,7 +292,7 @@ func TestLoginGovProviderBadNonce(t *testing.T) { func TestLoginGovProviderGetLoginURL(t *testing.T) { p, _, _ := newLoginGovProvider() - result := p.GetLoginURL("http://redirect/", "") + result := p.GetLoginURL("http://redirect/", "", "") assert.Contains(t, result, "acr_values="+url.QueryEscape("http://idmanagement.gov/ns/assurance/loa/1")) assert.Contains(t, result, "nonce=fakenonce") } diff --git a/providers/oidc.go b/providers/oidc.go index df133f4d8f..9e7bf56fcf 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net/url" "reflect" "time" @@ -16,16 +17,31 @@ import ( // OIDCProvider represents an OIDC based Identity Provider type OIDCProvider struct { *ProviderData + + SkipNonce bool } // NewOIDCProvider initiates a new OIDCProvider func NewOIDCProvider(p *ProviderData) *OIDCProvider { p.ProviderName = "OpenID Connect" - return &OIDCProvider{ProviderData: p} + return &OIDCProvider{ + ProviderData: p, + SkipNonce: true, + } } var _ Provider = (*OIDCProvider)(nil) +// GetLoginURL makes the LoginURL with optional nonce support +func (p *OIDCProvider) GetLoginURL(redirectURI, state, nonce string) string { + extraParams := url.Values{} + if !p.SkipNonce { + extraParams.Add("nonce", nonce) + } + loginURL := makeLoginURL(p.Data(), redirectURI, state, extraParams) + return loginURL.String() +} + // Redeem exchanges the OAuth2 authentication token for an ID token func (p *OIDCProvider) Redeem(ctx context.Context, redirectURL, code string) (*sessions.SessionState, error) { clientSecret, err := p.GetClientSecret() @@ -109,8 +125,22 @@ func (p *OIDCProvider) enrichFromProfileURL(ctx context.Context, s *sessions.Ses // ValidateSession checks that the session's IDToken is still valid func (p *OIDCProvider) ValidateSession(ctx context.Context, s *sessions.SessionState) bool { - _, err := p.Verifier.Verify(ctx, s.IDToken) - return err == nil + idToken, err := p.Verifier.Verify(ctx, s.IDToken) + if err != nil { + logger.Errorf("id_token verification failed: %v", err) + return false + } + + if p.SkipNonce { + return true + } + err = p.checkNonce(s, idToken) + if err != nil { + logger.Errorf("nonce verification failed: %v", err) + return false + } + + return true } // RefreshSessionIfNeeded checks if the session has expired and uses the diff --git a/providers/oidc_test.go b/providers/oidc_test.go index 7ac9863483..7fae336829 100644 --- a/providers/oidc_test.go +++ b/providers/oidc_test.go @@ -2,8 +2,10 @@ package providers import ( "context" + "encoding/base64" "encoding/json" "errors" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -11,6 +13,7 @@ import ( "github.com/coreos/go-oidc" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" "github.com/stretchr/testify/assert" ) @@ -23,7 +26,6 @@ type redeemTokenResponse struct { } func newOIDCProvider(serverURL *url.URL) *OIDCProvider { - providerData := &ProviderData{ ProviderName: "oidc", ClientID: oidcClientID, @@ -54,7 +56,7 @@ func newOIDCProvider(serverURL *url.URL) *OIDCProvider { ), } - p := &OIDCProvider{ProviderData: providerData} + p := NewOIDCProvider(providerData) return p } @@ -74,8 +76,27 @@ func newTestOIDCSetup(body []byte) (*httptest.Server, *OIDCProvider) { return server, provider } -func TestOIDCProviderRedeem(t *testing.T) { +func TestOIDCProviderGetLoginURL(t *testing.T) { + serverURL := &url.URL{ + Scheme: "https", + Host: "oauth2proxy.oidctest", + } + provider := newOIDCProvider(serverURL) + + n, err := encryption.Nonce() + assert.NoError(t, err) + nonce := base64.RawURLEncoding.EncodeToString(n) + + // SkipNonce defaults to true + skipNonce := provider.GetLoginURL("http://redirect/", "", nonce) + assert.NotContains(t, skipNonce, "nonce") + provider.SkipNonce = false + withNonce := provider.GetLoginURL("http://redirect/", "", nonce) + assert.Contains(t, withNonce, fmt.Sprintf("nonce=%s", nonce)) +} + +func TestOIDCProviderRedeem(t *testing.T) { idToken, _ := newSignedTestIDToken(defaultIDToken) body, _ := json.Marshal(redeemTokenResponse{ AccessToken: accessToken, @@ -98,7 +119,6 @@ func TestOIDCProviderRedeem(t *testing.T) { } func TestOIDCProviderRedeem_custom_userid(t *testing.T) { - idToken, _ := newSignedTestIDToken(defaultIDToken) body, _ := json.Marshal(redeemTokenResponse{ AccessToken: accessToken, diff --git a/providers/provider_data.go b/providers/provider_data.go index a9a41232ca..5b223418b5 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -122,6 +122,7 @@ type OIDCClaims struct { Email string `json:"-"` Groups []string `json:"-"` Verified *bool `json:"email_verified"` + Nonce string `json:"nonce"` raw map[string]interface{} } @@ -192,6 +193,18 @@ func (p *ProviderData) getClaims(idToken *oidc.IDToken) (*OIDCClaims, error) { return claims, nil } +// checkNonce compares the session's nonce with the IDToken's nonce claim +func (p *ProviderData) checkNonce(s *sessions.SessionState, idToken *oidc.IDToken) error { + claims, err := p.getClaims(idToken) + if err != nil { + return fmt.Errorf("id_token claims extraction failed: %v", err) + } + if !s.CheckNonce(claims.Nonce) { + return errors.New("id_token nonce claim does not match the session nonce") + } + return nil +} + // extractGroups extracts groups from a claim to a list in a type safe manner. // If the claim isn't present, `nil` is returned. If the groups claim is // present but empty, `[]string{}` is returned. diff --git a/providers/provider_data_test.go b/providers/provider_data_test.go index 80f6ecab9d..8d9c5fa1dd 100644 --- a/providers/provider_data_test.go +++ b/providers/provider_data_test.go @@ -15,6 +15,7 @@ import ( "github.com/coreos/go-oidc" "github.com/dgrijalva/jwt-go" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" . "github.com/onsi/gomega" "golang.org/x/oauth2" ) @@ -27,6 +28,7 @@ const ( oidcIssuer = "https://issuer.example.com" oidcClientID = "https://test.myapp.com" oidcSecret = "SuperSecret123456789" + oidcNonce = "abcde12345edcba09876abcde12345ff" failureTokenID = "this-id-fails-verification" ) @@ -53,6 +55,7 @@ var ( Groups: []string{"test:a", "test:b"}, Roles: []string{"test:c", "test:d"}, Verified: &verified, + Nonce: encryption.HashNonce([]byte(oidcNonce)), StandardClaims: standardClaims, } @@ -96,6 +99,7 @@ type idTokenClaims struct { Groups interface{} `json:"groups,omitempty"` Roles interface{} `json:"roles,omitempty"` Verified *bool `json:"email_verified,omitempty"` + Nonce string `json:"nonce,omitempty"` jwt.StandardClaims } @@ -348,6 +352,63 @@ func TestProviderData_buildSessionFromClaims(t *testing.T) { } } +func TestProviderData_checkNonce(t *testing.T) { + testCases := map[string]struct { + Session *sessions.SessionState + IDToken idTokenClaims + ExpectedError error + }{ + "Nonces match": { + Session: &sessions.SessionState{ + Nonce: []byte(oidcNonce), + }, + IDToken: defaultIDToken, + ExpectedError: nil, + }, + "Nonces do not match": { + Session: &sessions.SessionState{ + Nonce: []byte("WrongWrongWrong"), + }, + IDToken: defaultIDToken, + ExpectedError: errors.New("id_token nonce claim does not match the session nonce"), + }, + + "Missing nonce claim": { + Session: &sessions.SessionState{ + Nonce: []byte(oidcNonce), + }, + IDToken: minimalIDToken, + ExpectedError: errors.New("id_token nonce claim does not match the session nonce"), + }, + } + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + g := NewWithT(t) + + provider := &ProviderData{ + Verifier: oidc.NewVerifier( + oidcIssuer, + mockJWKS{}, + &oidc.Config{ClientID: oidcClientID}, + ), + } + + rawIDToken, err := newSignedTestIDToken(tc.IDToken) + g.Expect(err).ToNot(HaveOccurred()) + + idToken, err := provider.Verifier.Verify(context.Background(), rawIDToken) + g.Expect(err).ToNot(HaveOccurred()) + + err = provider.checkNonce(tc.Session, idToken) + if err != nil { + g.Expect(err).To(Equal(tc.ExpectedError)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + func TestProviderData_extractGroups(t *testing.T) { testCases := map[string]struct { Claims map[string]interface{} diff --git a/providers/provider_default.go b/providers/provider_default.go index 01b626e88c..1bde54b788 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -33,6 +33,13 @@ var ( _ Provider = (*ProviderData)(nil) ) +// GetLoginURL with typical oauth parameters +func (p *ProviderData) GetLoginURL(redirectURI, state, _ string) string { + extraParams := url.Values{} + loginURL := makeLoginURL(p, redirectURI, state, extraParams) + return loginURL.String() +} + // Redeem provides a default implementation of the OAuth2 token redemption process func (p *ProviderData) Redeem(ctx context.Context, redirectURL, code string) (*sessions.SessionState, error) { if code == "" { @@ -86,13 +93,6 @@ func (p *ProviderData) Redeem(ctx context.Context, redirectURL, code string) (*s return nil, fmt.Errorf("no access token found %s", result.Body()) } -// GetLoginURL with typical oauth parameters -func (p *ProviderData) GetLoginURL(redirectURI, state string) string { - extraParams := url.Values{} - a := makeLoginURL(p, redirectURI, state, extraParams) - return a.String() -} - // GetEmailAddress returns the Account email address // Deprecated: Migrate to EnrichSession func (p *ProviderData) GetEmailAddress(_ context.Context, _ *sessions.SessionState) (string, error) { diff --git a/providers/provider_default_test.go b/providers/provider_default_test.go index df1525cf3b..0bd2f4f083 100644 --- a/providers/provider_default_test.go +++ b/providers/provider_default_test.go @@ -31,7 +31,7 @@ func TestAcrValuesNotConfigured(t *testing.T) { }, } - result := p.GetLoginURL("https://my.test.app/oauth", "") + result := p.GetLoginURL("https://my.test.app/oauth", "", "") assert.NotContains(t, result, "acr_values") } @@ -45,7 +45,7 @@ func TestAcrValuesConfigured(t *testing.T) { AcrValues: "testValue", } - result := p.GetLoginURL("https://my.test.app/oauth", "") + result := p.GetLoginURL("https://my.test.app/oauth", "", "") assert.Contains(t, result, "acr_values=testValue") } diff --git a/providers/providers.go b/providers/providers.go index d4f05e2c15..498d1db074 100644 --- a/providers/providers.go +++ b/providers/providers.go @@ -11,11 +11,11 @@ type Provider interface { Data() *ProviderData // Deprecated: Migrate to EnrichSession GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) + GetLoginURL(redirectURI, state, nonce string) string Redeem(ctx context.Context, redirectURI, code string) (*sessions.SessionState, error) EnrichSession(ctx context.Context, s *sessions.SessionState) error Authorize(ctx context.Context, s *sessions.SessionState) (bool, error) ValidateSession(ctx context.Context, s *sessions.SessionState) bool - GetLoginURL(redirectURI, finalRedirect string) string RefreshSessionIfNeeded(ctx context.Context, s *sessions.SessionState) (bool, error) CreateSessionFromToken(ctx context.Context, token string) (*sessions.SessionState, error) } From 544ba2a21c0e59ad7154eef604c5bfdc9f37dd91 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Fri, 23 Apr 2021 13:22:28 -0700 Subject: [PATCH 003/433] Fix Metrics cfg option naming typo --- CHANGELOG.md | 1 + pkg/apis/options/legacy_options.go | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb257c7e9a..8e21fe7d2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ ## Changes since v7.1.2 +- [#1168](https://github.com/oauth2-proxy/oauth2-proxy/pull/1168) Fix incorrect `cfg` name in Metrics TLS flags (@NickMeves) - [#967](https://github.com/oauth2-proxy/oauth2-proxy/pull/967) Set & verify a nonce with OIDC providers (@NickMeves) - [#1136](https://github.com/oauth2-proxy/oauth2-proxy/pull/1136) Add clock package for better time mocking in tests (@NickMeves) - [#947](https://github.com/oauth2-proxy/oauth2-proxy/pull/947) Multiple provider ingestion and validation in alpha options (first stage: [#926](https://github.com/oauth2-proxy/oauth2-proxy/issues/926)) (@yanasega) diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 76367b8f3a..17cbcbc32f 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -442,8 +442,8 @@ func getXAuthRequestAccessTokenHeader() Header { type LegacyServer struct { MetricsAddress string `flag:"metrics-address" cfg:"metrics_address"` MetricsSecureAddress string `flag:"metrics-secure-address" cfg:"metrics_secure_address"` - MetricsTLSCertFile string `flag:"metrics-tls-cert-file" cfg:"tls_cert_file"` - MetricsTLSKeyFile string `flag:"metrics-tls-key-file" cfg:"tls_key_file"` + MetricsTLSCertFile string `flag:"metrics-tls-cert-file" cfg:"metrics_tls_cert_file"` + MetricsTLSKeyFile string `flag:"metrics-tls-key-file" cfg:"metrics_tls_key_file"` HTTPAddress string `flag:"http-address" cfg:"http_address"` HTTPSAddress string `flag:"https-address" cfg:"https_address"` TLSCertFile string `flag:"tls-cert-file" cfg:"tls_cert_file"` From f9de0e840c777aae252e7d9da1e8c42403c28766 Mon Sep 17 00:00:00 2001 From: Weinong Wang Date: Tue, 27 Apr 2021 10:59:02 -0700 Subject: [PATCH 004/433] add oidc issuer to Azure auth provider doc (#1135) * add oidc issuer to Azure auth provider doc * updated versioned doc --- docs/docs/configuration/auth.md | 1 + docs/versioned_docs/version-7.1.x/configuration/auth.md | 1 + 2 files changed, 2 insertions(+) diff --git a/docs/docs/configuration/auth.md b/docs/docs/configuration/auth.md index 79a735c527..6a667da07d 100644 --- a/docs/docs/configuration/auth.md +++ b/docs/docs/configuration/auth.md @@ -83,6 +83,7 @@ Note: The user is checked against the group members list on initial authenticati --provider=azure --client-id= --client-secret= + --oidc-issuer-url=https://sts.windows.net/{tenant-id}/ ``` Note: When using the Azure Auth provider with nginx and the cookie session store you may find the cookie is too large and doesn't get passed through correctly. Increasing the proxy_buffer_size in nginx or implementing the [redis session storage](sessions.md#redis-storage) should resolve this. diff --git a/docs/versioned_docs/version-7.1.x/configuration/auth.md b/docs/versioned_docs/version-7.1.x/configuration/auth.md index 79a735c527..6a667da07d 100644 --- a/docs/versioned_docs/version-7.1.x/configuration/auth.md +++ b/docs/versioned_docs/version-7.1.x/configuration/auth.md @@ -83,6 +83,7 @@ Note: The user is checked against the group members list on initial authenticati --provider=azure --client-id= --client-secret= + --oidc-issuer-url=https://sts.windows.net/{tenant-id}/ ``` Note: When using the Azure Auth provider with nginx and the cookie session store you may find the cookie is too large and doesn't get passed through correctly. Increasing the proxy_buffer_size in nginx or implementing the [redis session storage](sessions.md#redis-storage) should resolve this. From 2dd4a9647a2549b2671d29b4e978e312dc414a94 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Wed, 28 Apr 2021 09:41:18 -0700 Subject: [PATCH 005/433] Update Changelog for release v7.1.3 --- CHANGELOG.md | 12 ++++++++++++ .../local-environment/docker-compose-keycloak.yaml | 2 +- contrib/local-environment/docker-compose.yaml | 2 +- docs/docs/installation.md | 2 +- docs/versioned_docs/version-7.1.x/installation.md | 2 +- 5 files changed, 16 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e21fe7d2b..bcf4f54edc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,18 @@ ## Important Notes +## Breaking Changes + +## Changes since v7.1.3 + +# V7.1.3 + +## Release Highlights + +- Fixed typos in the metrics server TLS config names + +## Important Notes + - [#967](https://github.com/oauth2-proxy/oauth2-proxy/pull/967) `--insecure-oidc-skip-nonce` is currently `true` by default in case any existing OIDC Identity Providers don't support it. The default will switch to `false` in a future version. diff --git a/contrib/local-environment/docker-compose-keycloak.yaml b/contrib/local-environment/docker-compose-keycloak.yaml index 087050e195..241be3aefb 100644 --- a/contrib/local-environment/docker-compose-keycloak.yaml +++ b/contrib/local-environment/docker-compose-keycloak.yaml @@ -15,7 +15,7 @@ services: oauth2-proxy: container_name: oauth2-proxy - image: quay.io/oauth2-proxy/oauth2-proxy:v7.1.2 + image: quay.io/oauth2-proxy/oauth2-proxy:v7.1.3 command: --config /oauth2-proxy.cfg hostname: oauth2-proxy volumes: diff --git a/contrib/local-environment/docker-compose.yaml b/contrib/local-environment/docker-compose.yaml index 57fe40ee53..dd245730f1 100644 --- a/contrib/local-environment/docker-compose.yaml +++ b/contrib/local-environment/docker-compose.yaml @@ -13,7 +13,7 @@ version: '3.0' services: oauth2-proxy: container_name: oauth2-proxy - image: quay.io/oauth2-proxy/oauth2-proxy:2 + image: quay.io/oauth2-proxy/oauth2-proxy:v7.1.3 command: --config /oauth2-proxy.cfg ports: - 4180:4180/tcp diff --git a/docs/docs/installation.md b/docs/docs/installation.md index aa9ce73fe2..04e8240613 100644 --- a/docs/docs/installation.md +++ b/docs/docs/installation.md @@ -6,7 +6,7 @@ slug: / 1. Choose how to deploy: - a. Download [Prebuilt Binary](https://github.com/oauth2-proxy/oauth2-proxy/releases) (current release is `v7.1.2`) + a. Download [Prebuilt Binary](https://github.com/oauth2-proxy/oauth2-proxy/releases) (current release is `v7.1.3`) b. Build with `$ go get github.com/oauth2-proxy/oauth2-proxy/v7` which will put the binary in `$GOPATH/bin` diff --git a/docs/versioned_docs/version-7.1.x/installation.md b/docs/versioned_docs/version-7.1.x/installation.md index aa9ce73fe2..04e8240613 100644 --- a/docs/versioned_docs/version-7.1.x/installation.md +++ b/docs/versioned_docs/version-7.1.x/installation.md @@ -6,7 +6,7 @@ slug: / 1. Choose how to deploy: - a. Download [Prebuilt Binary](https://github.com/oauth2-proxy/oauth2-proxy/releases) (current release is `v7.1.2`) + a. Download [Prebuilt Binary](https://github.com/oauth2-proxy/oauth2-proxy/releases) (current release is `v7.1.3`) b. Build with `$ go get github.com/oauth2-proxy/oauth2-proxy/v7` which will put the binary in `$GOPATH/bin` From 095e1db8018292528b18dd95878f68cfb792d2d0 Mon Sep 17 00:00:00 2001 From: Itay Brandes Date: Tue, 4 May 2021 19:17:30 +0300 Subject: [PATCH 006/433] fix: SHOW_DEBUG_ON_ERROR environment variable not working (Fixes #1178) --- CHANGELOG.md | 2 ++ pkg/apis/options/app.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bcf4f54edc..0d9e7e6279 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ ## Changes since v7.1.3 +- [#1181](https://github.com/oauth2-proxy/oauth2-proxy/pull/1181) Fix incorrect `cfg` name in show-debug-on-error flag (@iTaybb) + # V7.1.3 ## Release Highlights diff --git a/pkg/apis/options/app.go b/pkg/apis/options/app.go index 4d6353b8f5..fbb2303374 100644 --- a/pkg/apis/options/app.go +++ b/pkg/apis/options/app.go @@ -33,7 +33,7 @@ type Templates struct { // It is not advised to use this in production as errors may contain sensitive // information. // Use only for diagnosing backend errors. - Debug bool `flag:"show-debug-on-error" cfg:"show-debug-on-error"` + Debug bool `flag:"show-debug-on-error" cfg:"show_debug_on_error"` } func templatesFlagSet() *pflag.FlagSet { From befcdd9d042fb745d8c22e6ca67e653b29492daf Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 31 Mar 2021 10:30:42 +0100 Subject: [PATCH 007/433] Add pagewriter to upstream proxy --- CHANGELOG.md | 1 + oauthproxy.go | 2 +- pkg/app/pagewriter/pagewriter.go | 70 +++++++++++++ pkg/app/pagewriter/pagewriter_test.go | 143 ++++++++++++++++++++++++++ pkg/upstream/proxy.go | 9 +- pkg/upstream/proxy_test.go | 11 +- 6 files changed, 227 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d9e7e6279..41bf97d56f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ## Changes since v7.1.3 +- [#1142](https://github.com/oauth2-proxy/oauth2-proxy/pull/1142) Add pagewriter to upstream proxy (@JoelSpeed) - [#1181](https://github.com/oauth2-proxy/oauth2-proxy/pull/1181) Fix incorrect `cfg` name in show-debug-on-error flag (@iTaybb) # V7.1.3 diff --git a/oauthproxy.go b/oauthproxy.go index 581aad9898..5902b9afca 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -124,7 +124,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr return nil, fmt.Errorf("error initialising page writer: %v", err) } - upstreamProxy, err := upstream.NewProxy(opts.UpstreamServers, opts.GetSignatureData(), pageWriter.ProxyErrorHandler) + upstreamProxy, err := upstream.NewProxy(opts.UpstreamServers, opts.GetSignatureData(), pageWriter) if err != nil { return nil, fmt.Errorf("error initialising upstream proxy: %v", err) } diff --git a/pkg/app/pagewriter/pagewriter.go b/pkg/app/pagewriter/pagewriter.go index 5991e625fa..c72400f07d 100644 --- a/pkg/app/pagewriter/pagewriter.go +++ b/pkg/app/pagewriter/pagewriter.go @@ -101,3 +101,73 @@ func NewWriter(opts Opts) (Writer, error) { staticPageWriter: staticPages, }, nil } + +// WriterFuncs is an implementation of the PageWriter interface based +// on override functions. +// If any of the funcs are not provided, a default implementation will be used. +// This is primarily for us in testing. +type WriterFuncs struct { + SignInPageFunc func(rw http.ResponseWriter, req *http.Request, redirectURL string) + ErrorPageFunc func(rw http.ResponseWriter, opts ErrorPageOpts) + ProxyErrorFunc func(rw http.ResponseWriter, req *http.Request, proxyErr error) + RobotsTxtfunc func(rw http.ResponseWriter, req *http.Request) +} + +// WriteSignInPage implements the Writer interface. +// If the SignInPageFunc is provided, this will be used, else a default +// implementation will be used. +func (w *WriterFuncs) WriteSignInPage(rw http.ResponseWriter, req *http.Request, redirectURL string) { + if w.SignInPageFunc != nil { + w.SignInPageFunc(rw, req, redirectURL) + return + } + + if _, err := rw.Write([]byte("Sign In")); err != nil { + rw.WriteHeader(http.StatusInternalServerError) + } +} + +// WriteErrorPage implements the Writer interface. +// If the ErrorPageFunc is provided, this will be used, else a default +// implementation will be used. +func (w *WriterFuncs) WriteErrorPage(rw http.ResponseWriter, opts ErrorPageOpts) { + if w.ErrorPageFunc != nil { + w.ErrorPageFunc(rw, opts) + return + } + + rw.WriteHeader(opts.Status) + errMsg := fmt.Sprintf("%d - %v", opts.Status, opts.AppError) + if _, err := rw.Write([]byte(errMsg)); err != nil { + rw.WriteHeader(http.StatusInternalServerError) + } +} + +// ProxyErrorHandler implements the Writer interface. +// If the ProxyErrorFunc is provided, this will be used, else a default +// implementation will be used. +func (w *WriterFuncs) ProxyErrorHandler(rw http.ResponseWriter, req *http.Request, proxyErr error) { + if w.ProxyErrorFunc != nil { + w.ProxyErrorFunc(rw, req, proxyErr) + return + } + + w.WriteErrorPage(rw, ErrorPageOpts{ + Status: http.StatusBadGateway, + AppError: proxyErr.Error(), + }) +} + +// WriteRobotsTxt implements the Writer interface. +// If the RobotsTxtfunc is provided, this will be used, else a default +// implementation will be used. +func (w *WriterFuncs) WriteRobotsTxt(rw http.ResponseWriter, req *http.Request) { + if w.RobotsTxtfunc != nil { + w.RobotsTxtfunc(rw, req) + return + } + + if _, err := rw.Write([]byte("Allow: *")); err != nil { + rw.WriteHeader(http.StatusInternalServerError) + } +} diff --git a/pkg/app/pagewriter/pagewriter_test.go b/pkg/app/pagewriter/pagewriter_test.go index eefd24378a..2adedd19a7 100644 --- a/pkg/app/pagewriter/pagewriter_test.go +++ b/pkg/app/pagewriter/pagewriter_test.go @@ -1,6 +1,8 @@ package pagewriter import ( + "errors" + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -8,6 +10,7 @@ import ( "path/filepath" . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" ) @@ -135,4 +138,144 @@ var _ = Describe("Writer", func() { }) }) }) + + Context("WriterFuncs", func() { + type writerFuncsTableInput struct { + writer Writer + expectedStatus int + expectedBody string + } + + DescribeTable("WriteSignInPage", + func(in writerFuncsTableInput) { + rw := httptest.NewRecorder() + req := httptest.NewRequest("", "/sign-in", nil) + redirectURL := "" + in.writer.WriteSignInPage(rw, req, redirectURL) + + Expect(rw.Result().StatusCode).To(Equal(in.expectedStatus)) + + body, err := ioutil.ReadAll(rw.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal(in.expectedBody)) + }, + Entry("With no override", writerFuncsTableInput{ + writer: &WriterFuncs{}, + expectedStatus: 200, + expectedBody: "Sign In", + }), + Entry("With an override function", writerFuncsTableInput{ + writer: &WriterFuncs{ + SignInPageFunc: func(rw http.ResponseWriter, req *http.Request, redirectURL string) { + rw.WriteHeader(202) + rw.Write([]byte(fmt.Sprintf("%s %s", req.URL.Path, redirectURL))) + }, + }, + expectedStatus: 202, + expectedBody: "/sign-in ", + }), + ) + + DescribeTable("WriteErrorPage", + func(in writerFuncsTableInput) { + rw := httptest.NewRecorder() + in.writer.WriteErrorPage(rw, ErrorPageOpts{ + Status: http.StatusInternalServerError, + RedirectURL: "", + RequestID: "12345", + AppError: "application error", + }) + + Expect(rw.Result().StatusCode).To(Equal(in.expectedStatus)) + + body, err := ioutil.ReadAll(rw.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal(in.expectedBody)) + }, + Entry("With no override", writerFuncsTableInput{ + writer: &WriterFuncs{}, + expectedStatus: 500, + expectedBody: "500 - application error", + }), + Entry("With an override function", writerFuncsTableInput{ + writer: &WriterFuncs{ + ErrorPageFunc: func(rw http.ResponseWriter, opts ErrorPageOpts) { + rw.WriteHeader(503) + rw.Write([]byte(fmt.Sprintf("%s %s", opts.RequestID, opts.RedirectURL))) + }, + }, + expectedStatus: 503, + expectedBody: "12345 ", + }), + ) + + DescribeTable("ProxyErrorHandler", + func(in writerFuncsTableInput) { + rw := httptest.NewRecorder() + req := httptest.NewRequest("", "/proxy", nil) + err := errors.New("proxy error") + in.writer.ProxyErrorHandler(rw, req, err) + + Expect(rw.Result().StatusCode).To(Equal(in.expectedStatus)) + + body, err := ioutil.ReadAll(rw.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal(in.expectedBody)) + }, + Entry("With no override", writerFuncsTableInput{ + writer: &WriterFuncs{}, + expectedStatus: 502, + expectedBody: "502 - proxy error", + }), + Entry("With an override function for the proxy handler", writerFuncsTableInput{ + writer: &WriterFuncs{ + ProxyErrorFunc: func(rw http.ResponseWriter, req *http.Request, proxyErr error) { + rw.WriteHeader(503) + rw.Write([]byte(fmt.Sprintf("%s %v", req.URL.Path, proxyErr))) + }, + }, + expectedStatus: 503, + expectedBody: "/proxy proxy error", + }), + Entry("With an override function for the error page", writerFuncsTableInput{ + writer: &WriterFuncs{ + ErrorPageFunc: func(rw http.ResponseWriter, opts ErrorPageOpts) { + rw.WriteHeader(500) + rw.Write([]byte("Internal Server Error")) + }, + }, + expectedStatus: 500, + expectedBody: "Internal Server Error", + }), + ) + + DescribeTable("WriteRobotsTxt", + func(in writerFuncsTableInput) { + rw := httptest.NewRecorder() + req := httptest.NewRequest("", "/robots.txt", nil) + in.writer.WriteRobotsTxt(rw, req) + + Expect(rw.Result().StatusCode).To(Equal(in.expectedStatus)) + + body, err := ioutil.ReadAll(rw.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal(in.expectedBody)) + }, + Entry("With no override", writerFuncsTableInput{ + writer: &WriterFuncs{}, + expectedStatus: 200, + expectedBody: "Allow: *", + }), + Entry("With an override function", writerFuncsTableInput{ + writer: &WriterFuncs{ + RobotsTxtfunc: func(rw http.ResponseWriter, req *http.Request) { + rw.WriteHeader(202) + rw.Write([]byte("Disallow: *")) + }, + }, + expectedStatus: 202, + expectedBody: "Disallow: *", + }), + ) + }) }) diff --git a/pkg/upstream/proxy.go b/pkg/upstream/proxy.go index f345158b14..2b0ab70eee 100644 --- a/pkg/upstream/proxy.go +++ b/pkg/upstream/proxy.go @@ -6,6 +6,7 @@ import ( "net/url" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/app/pagewriter" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" ) @@ -15,7 +16,7 @@ type ProxyErrorHandler func(http.ResponseWriter, *http.Request, error) // NewProxy creates a new multiUpstreamProxy that can serve requests directed to // multiple upstreams. -func NewProxy(upstreams options.Upstreams, sigData *options.SignatureData, errorHandler ProxyErrorHandler) (http.Handler, error) { +func NewProxy(upstreams options.Upstreams, sigData *options.SignatureData, writer pagewriter.Writer) (http.Handler, error) { m := &multiUpstreamProxy{ serveMux: http.NewServeMux(), } @@ -34,7 +35,7 @@ func NewProxy(upstreams options.Upstreams, sigData *options.SignatureData, error case fileScheme: m.registerFileServer(upstream, u) case httpScheme, httpsScheme: - m.registerHTTPUpstreamProxy(upstream, u, sigData, errorHandler) + m.registerHTTPUpstreamProxy(upstream, u, sigData, writer) default: return nil, fmt.Errorf("unknown scheme for upstream %q: %q", upstream.ID, u.Scheme) } @@ -66,7 +67,7 @@ func (m *multiUpstreamProxy) registerFileServer(upstream options.Upstream, u *ur } // registerHTTPUpstreamProxy registers a new httpUpstreamProxy based on the configuration given. -func (m *multiUpstreamProxy) registerHTTPUpstreamProxy(upstream options.Upstream, u *url.URL, sigData *options.SignatureData, errorHandler ProxyErrorHandler) { +func (m *multiUpstreamProxy) registerHTTPUpstreamProxy(upstream options.Upstream, u *url.URL, sigData *options.SignatureData, writer pagewriter.Writer) { logger.Printf("mapping path %q => upstream %q", upstream.Path, upstream.URI) - m.serveMux.Handle(upstream.Path, newHTTPUpstreamProxy(upstream, u, sigData, errorHandler)) + m.serveMux.Handle(upstream.Path, newHTTPUpstreamProxy(upstream, u, sigData, writer.ProxyErrorHandler)) } diff --git a/pkg/upstream/proxy_test.go b/pkg/upstream/proxy_test.go index 44483585a4..96b7a0ddb2 100644 --- a/pkg/upstream/proxy_test.go +++ b/pkg/upstream/proxy_test.go @@ -9,6 +9,7 @@ import ( middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/app/pagewriter" . "github.com/onsi/ginkgo" . "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" @@ -20,9 +21,11 @@ var _ = Describe("Proxy Suite", func() { BeforeEach(func() { sigData := &options.SignatureData{Hash: crypto.SHA256, Key: "secret"} - errorHandler := func(rw http.ResponseWriter, _ *http.Request, _ error) { - rw.WriteHeader(502) - rw.Write([]byte("Proxy Error")) + writer := &pagewriter.WriterFuncs{ + ProxyErrorFunc: func(rw http.ResponseWriter, _ *http.Request, _ error) { + rw.WriteHeader(502) + rw.Write([]byte("Proxy Error")) + }, } ok := http.StatusOK @@ -58,7 +61,7 @@ var _ = Describe("Proxy Suite", func() { } var err error - upstreamServer, err = NewProxy(upstreams, sigData, errorHandler) + upstreamServer, err = NewProxy(upstreams, sigData, writer) Expect(err).ToNot(HaveOccurred()) }) From a0e2f785f3564589f01eb53cac40178488e2ea6c Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Sun, 14 Mar 2021 19:48:26 +0000 Subject: [PATCH 008/433] Add alternative ways to generate cookie secrets to docs --- CHANGELOG.md | 1 + docs/docs/configuration/overview.md | 57 ++++++++++++++++++- .../version-6.1.x/configuration/overview.md | 57 ++++++++++++++++++- .../version-7.0.x/configuration/overview.md | 57 ++++++++++++++++++- 4 files changed, 169 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 41bf97d56f..e6517f73b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ## Changes since v7.1.3 +- [#1108](https://github.com/oauth2-proxy/oauth2-proxy/pull/1108) Add alternative ways to generate cookie secrets to docs (@JoelSpeed) - [#1142](https://github.com/oauth2-proxy/oauth2-proxy/pull/1142) Add pagewriter to upstream proxy (@JoelSpeed) - [#1181](https://github.com/oauth2-proxy/oauth2-proxy/pull/1181) Fix incorrect `cfg` name in show-debug-on-error flag (@iTaybb) diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 4702f3217c..ebf7c05e2e 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -7,7 +7,62 @@ title: Overview ### Generating a Cookie Secret -To generate a strong cookie secret use `python -c 'import os,base64; print(base64.urlsafe_b64encode(os.urandom(16)).decode())'` +To generate a strong cookie secret use one of the below commands: + +import Tabs from '@theme/Tabs'; +import TabItem from '@theme/TabItem'; + + + + + ```shell + python -c 'import os,base64; print(base64.urlsafe_b64encode(os.urandom(32)).decode())' + ``` + + + + + ```shell + cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1 | base64 + ``` + + + + + ```shell + openssl rand -base64 32 | tr -- '+/' '-_' + ``` + + + + + ```shell + # Add System.Web assembly to session, just in case + Add-Type -AssemblyName System.Web + [Convert]::ToBase64String([System.Text.Encoding]::UTF8.GetBytes([System.Web.Security.Membership]::GeneratePassword(32,4))).Replace("+","-").Replace("/","_") + ``` + + + + + ```shell + # Valid 32 Byte Base64 URL encoding set that will decode to 24 []byte AES-192 secret + resource "random_password" "cookie_secret" { + length = 32 + override_special = "-_" + } + ``` + + + ### Config File diff --git a/docs/versioned_docs/version-6.1.x/configuration/overview.md b/docs/versioned_docs/version-6.1.x/configuration/overview.md index 575db4dd71..a8e721142b 100644 --- a/docs/versioned_docs/version-6.1.x/configuration/overview.md +++ b/docs/versioned_docs/version-6.1.x/configuration/overview.md @@ -7,7 +7,62 @@ title: Overview ### Generating a Cookie Secret -To generate a strong cookie secret use `python -c 'import os,base64; print(base64.urlsafe_b64encode(os.urandom(16)).decode())'` +To generate a strong cookie secret use one of the below commands: + +import Tabs from '@theme/Tabs'; +import TabItem from '@theme/TabItem'; + + + + + ```shell + python -c 'import os,base64; print(base64.urlsafe_b64encode(os.urandom(32)).decode())' + ``` + + + + + ```shell + cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1 | base64 + ``` + + + + + ```shell + openssl rand -base64 32 | tr -- '+/' '-_' + ``` + + + + + ```shell + # Add System.Web assembly to session, just in case + Add-Type -AssemblyName System.Web + [Convert]::ToBase64String([System.Text.Encoding]::UTF8.GetBytes([System.Web.Security.Membership]::GeneratePassword(32,4))).Replace("+","-").Replace("/","_") + ``` + + + + + ```shell + # Valid 32 Byte Base64 URL encoding set that will decode to 24 []byte AES-192 secret + resource "random_password" "cookie_secret" { + length = 32 + override_special = "-_" + } + ``` + + + ### Config File diff --git a/docs/versioned_docs/version-7.0.x/configuration/overview.md b/docs/versioned_docs/version-7.0.x/configuration/overview.md index 98adc04d00..3b17339d3d 100644 --- a/docs/versioned_docs/version-7.0.x/configuration/overview.md +++ b/docs/versioned_docs/version-7.0.x/configuration/overview.md @@ -7,7 +7,62 @@ title: Overview ### Generating a Cookie Secret -To generate a strong cookie secret use `python -c 'import os,base64; print(base64.urlsafe_b64encode(os.urandom(16)).decode())'` +To generate a strong cookie secret use one of the below commands: + +import Tabs from '@theme/Tabs'; +import TabItem from '@theme/TabItem'; + + + + + ```shell + python -c 'import os,base64; print(base64.urlsafe_b64encode(os.urandom(32)).decode())' + ``` + + + + + ```shell + cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1 | base64 + ``` + + + + + ```shell + openssl rand -base64 32 | tr -- '+/' '-_' + ``` + + + + + ```shell + # Add System.Web assembly to session, just in case + Add-Type -AssemblyName System.Web + [Convert]::ToBase64String([System.Text.Encoding]::UTF8.GetBytes([System.Web.Security.Membership]::GeneratePassword(32,4))).Replace("+","-").Replace("/","_") + ``` + + + + + ```shell + # Valid 32 Byte Base64 URL encoding set that will decode to 24 []byte AES-192 secret + resource "random_password" "cookie_secret" { + length = 32 + override_special = "-_" + } + ``` + + + ### Config File From 818938add29fb50086dba4b9b5a77f27e2059f71 Mon Sep 17 00:00:00 2001 From: Tarvi Pillessaar Date: Wed, 19 May 2021 11:50:53 +0300 Subject: [PATCH 009/433] Fix URI fragment handling Fixes #1205 --- CHANGELOG.md | 1 + pkg/app/pagewriter/sign_in.html | 34 ++++++++++++++++----------------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6517f73b0..46ec8c5a2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - [#1108](https://github.com/oauth2-proxy/oauth2-proxy/pull/1108) Add alternative ways to generate cookie secrets to docs (@JoelSpeed) - [#1142](https://github.com/oauth2-proxy/oauth2-proxy/pull/1142) Add pagewriter to upstream proxy (@JoelSpeed) - [#1181](https://github.com/oauth2-proxy/oauth2-proxy/pull/1181) Fix incorrect `cfg` name in show-debug-on-error flag (@iTaybb) +- [#1207](https://github.com/oauth2-proxy/oauth2-proxy/pull/1207) Fix URI fragment handling on sign-in page, regression introduced in 7.1.0 (@tarvip) # V7.1.3 diff --git a/pkg/app/pagewriter/sign_in.html b/pkg/app/pagewriter/sign_in.html index 652b674e79..fc7ab3ffa5 100644 --- a/pkg/app/pagewriter/sign_in.html +++ b/pkg/app/pagewriter/sign_in.html @@ -22,23 +22,6 @@ text-decoration: underline; } - -
@@ -82,6 +65,23 @@
+ +