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

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())

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"))
 }
 

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"

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")
 }

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

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,

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.

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{}

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) {

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")
 }
 

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)
 }