From 8cbf9219bc0e93e9d0e62705d2195e802b60b23f Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Mon, 13 Jul 2020 14:24:04 +0200 Subject: [PATCH 1/9] Pass resource parameter in login url --- providers/provider_default.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/providers/provider_default.go b/providers/provider_default.go index ba05a96c..f20fe687 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -90,6 +90,9 @@ func (p *ProviderData) GetLoginURL(redirectURI, state string) string { params.Set("client_id", p.ClientID) params.Set("response_type", "code") params.Add("state", state) + if p.ProtectedResource != nil && p.ProtectedResource.String() != "" { + params.Add("resource", p.ProtectedResource.String()) + } a.RawQuery = params.Encode() return a.String() } From 017b9bcfb7f387c44e158be02f6d405e66e07228 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Thu, 10 Sep 2020 10:30:05 +0200 Subject: [PATCH 2/9] Add unit test for protected resources --- providers/provider_default_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/providers/provider_default_test.go b/providers/provider_default_test.go index 74d7096f..e699a68e 100644 --- a/providers/provider_default_test.go +++ b/providers/provider_default_test.go @@ -47,3 +47,21 @@ func TestAcrValuesConfigured(t *testing.T) { result := p.GetLoginURL("https://my.test.app/oauth", "") assert.Contains(t, result, "acr_values=testValue") } + +func TestProtectedResourceConfigured(t *testing.T) { + p := &ProviderData{ + LoginURL: &url.URL{ + Scheme: "http", + Host: "my.test.idp", + Path: "/oauth/authorize", + }, + AcrValues: "testValue", + ProtectedResource: &url.URL{ + Scheme: "http", + Host: "my.resource.test", + }, + } + + result := p.GetLoginURL("https://my.test.app/oauth", "") + assert.Contains(t, result, "resource="+url.QueryEscape("http://my.resource.test")) +} From 97e95fd4ffe4887fe2a334d78c0279d2f30d46a5 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Mon, 14 Sep 2020 13:22:53 +0200 Subject: [PATCH 3/9] Move actual implementation of default provider GetLoginURL into DefaultGetLoginURL This allows us to reuse code from different providers in case slight modifications to the URL are needed. --- providers/provider_default.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/providers/provider_default.go b/providers/provider_default.go index f20fe687..6e898a84 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -73,8 +73,7 @@ func (p *ProviderData) Redeem(ctx context.Context, redirectURL, code string) (s return } -// GetLoginURL with typical oauth parameters -func (p *ProviderData) GetLoginURL(redirectURI, state string) string { +func DefaultGetLoginURL(p *ProviderData, redirectURI, state string) (url.URL, url.Values) { a := *p.LoginURL params, _ := url.ParseQuery(a.RawQuery) params.Set("redirect_uri", redirectURI) @@ -93,6 +92,12 @@ func (p *ProviderData) GetLoginURL(redirectURI, state string) string { if p.ProtectedResource != nil && p.ProtectedResource.String() != "" { params.Add("resource", p.ProtectedResource.String()) } + return a, params +} + +// GetLoginURL with typical oauth parameters +func (p *ProviderData) GetLoginURL(redirectURI, state string) string { + a, params := DefaultGetLoginURL(p, redirectURI, state) a.RawQuery = params.Encode() return a.String() } From fde09bea4ec847d4611836d657215cfc60dbc2ef Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Mon, 14 Sep 2020 13:48:17 +0200 Subject: [PATCH 4/9] Move azure specific resource parameter handling into azure provider --- providers/azure.go | 9 +++++++++ providers/azure_test.go | 7 +++++++ providers/provider_default.go | 3 --- providers/provider_default_test.go | 18 ------------------ 4 files changed, 16 insertions(+), 21 deletions(-) diff --git a/providers/azure.go b/providers/azure.go index 0ae0cba6..c9940d61 100644 --- a/providers/azure.go +++ b/providers/azure.go @@ -210,3 +210,12 @@ func (p *AzureProvider) GetEmailAddress(ctx context.Context, s *sessions.Session return email, err } + +func (p *AzureProvider) GetLoginURL(redirectURI, state string) string { + a, params := DefaultGetLoginURL(p.ProviderData, redirectURI, state) + if p.ProtectedResource != nil && p.ProtectedResource.String() != "" { + params.Add("resource", p.ProtectedResource.String()) + } + a.RawQuery = params.Encode() + return a.String() +} diff --git a/providers/azure_test.go b/providers/azure_test.go index fe9bbb42..6e2e4e97 100644 --- a/providers/azure_test.go +++ b/providers/azure_test.go @@ -213,3 +213,10 @@ func TestAzureProviderRedeemReturnsIdToken(t *testing.T) { assert.Equal(t, timestamp, s.ExpiresOn.UTC()) assert.Equal(t, "refresh1234", s.RefreshToken) } + +func TestAzureProviderProtectedResourceConfigured(t *testing.T) { + p := testAzureProvider("") + p.ProtectedResource, _ = url.Parse("http://my.resource.test") + result := p.GetLoginURL("https://my.test.app/oauth", "") + assert.Contains(t, result, "resource="+url.QueryEscape("http://my.resource.test")) +} diff --git a/providers/provider_default.go b/providers/provider_default.go index 6e898a84..65c7f729 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -89,9 +89,6 @@ func DefaultGetLoginURL(p *ProviderData, redirectURI, state string) (url.URL, ur params.Set("client_id", p.ClientID) params.Set("response_type", "code") params.Add("state", state) - if p.ProtectedResource != nil && p.ProtectedResource.String() != "" { - params.Add("resource", p.ProtectedResource.String()) - } return a, params } diff --git a/providers/provider_default_test.go b/providers/provider_default_test.go index e699a68e..74d7096f 100644 --- a/providers/provider_default_test.go +++ b/providers/provider_default_test.go @@ -47,21 +47,3 @@ func TestAcrValuesConfigured(t *testing.T) { result := p.GetLoginURL("https://my.test.app/oauth", "") assert.Contains(t, result, "acr_values=testValue") } - -func TestProtectedResourceConfigured(t *testing.T) { - p := &ProviderData{ - LoginURL: &url.URL{ - Scheme: "http", - Host: "my.test.idp", - Path: "/oauth/authorize", - }, - AcrValues: "testValue", - ProtectedResource: &url.URL{ - Scheme: "http", - Host: "my.resource.test", - }, - } - - result := p.GetLoginURL("https://my.test.app/oauth", "") - assert.Contains(t, result, "resource="+url.QueryEscape("http://my.resource.test")) -} From 9a64e67d5bacf69ee8ec363842aa652260ff3fa4 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Mon, 14 Sep 2020 13:55:47 +0200 Subject: [PATCH 5/9] De-duplicate code in GetLoginURL of in logingov provider Also add unit test to ensure logingov specific logic is applied. --- providers/logingov.go | 16 ++++------------ providers/logingov_test.go | 7 +++++++ 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/providers/logingov.go b/providers/logingov.go index c524741f..e631237c 100644 --- a/providers/logingov.go +++ b/providers/logingov.go @@ -225,19 +225,11 @@ 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 { - a := *p.LoginURL - params, _ := url.ParseQuery(a.RawQuery) - params.Set("redirect_uri", redirectURI) - params.Set("approval_prompt", p.ApprovalPrompt) - params.Add("scope", p.Scope) - params.Set("client_id", p.ClientID) - params.Set("response_type", "code") - params.Add("state", state) - acr := p.AcrValues - if acr == "" { - acr = "http://idmanagement.gov/ns/assurance/loa/1" + a, params := DefaultGetLoginURL(p.ProviderData, redirectURI, state) + if p.AcrValues == "" { + acr := "http://idmanagement.gov/ns/assurance/loa/1" + params.Add("acr_values", acr) } - params.Add("acr_values", acr) params.Add("nonce", p.Nonce) a.RawQuery = params.Encode() return a.String() diff --git a/providers/logingov_test.go b/providers/logingov_test.go index 2c0f8357..0b70190b 100644 --- a/providers/logingov_test.go +++ b/providers/logingov_test.go @@ -289,3 +289,10 @@ func TestLoginGovProviderBadNonce(t *testing.T) { // The "badfakenonce" in the idtoken above should cause this to error out assert.Error(t, err) } + +func TestLoginGovProviderGetLoginURL(t *testing.T) { + p, _, _ := newLoginGovProvider() + 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") +} From 4eb96126799c8dc7288873db4d814fb91de9f7e9 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Tue, 15 Sep 2020 10:12:25 +0200 Subject: [PATCH 6/9] Move DefaultGetLoginURL into util.go --- providers/azure.go | 2 +- providers/logingov.go | 2 +- providers/provider_default.go | 21 +-------------------- providers/util.go | 20 ++++++++++++++++++++ 4 files changed, 23 insertions(+), 22 deletions(-) diff --git a/providers/azure.go b/providers/azure.go index c9940d61..9103d178 100644 --- a/providers/azure.go +++ b/providers/azure.go @@ -212,7 +212,7 @@ func (p *AzureProvider) GetEmailAddress(ctx context.Context, s *sessions.Session } func (p *AzureProvider) GetLoginURL(redirectURI, state string) string { - a, params := DefaultGetLoginURL(p.ProviderData, redirectURI, state) + a, params := makeLoginURL(p.ProviderData, redirectURI, state) if p.ProtectedResource != nil && p.ProtectedResource.String() != "" { params.Add("resource", p.ProtectedResource.String()) } diff --git a/providers/logingov.go b/providers/logingov.go index e631237c..32fe1c78 100644 --- a/providers/logingov.go +++ b/providers/logingov.go @@ -225,7 +225,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 { - a, params := DefaultGetLoginURL(p.ProviderData, redirectURI, state) + a, params := makeLoginURL(p.ProviderData, redirectURI, state) if p.AcrValues == "" { acr := "http://idmanagement.gov/ns/assurance/loa/1" params.Add("acr_values", acr) diff --git a/providers/provider_default.go b/providers/provider_default.go index 65c7f729..5fc53219 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -73,28 +73,9 @@ func (p *ProviderData) Redeem(ctx context.Context, redirectURL, code string) (s return } -func DefaultGetLoginURL(p *ProviderData, redirectURI, state string) (url.URL, url.Values) { - a := *p.LoginURL - params, _ := url.ParseQuery(a.RawQuery) - params.Set("redirect_uri", redirectURI) - if p.AcrValues != "" { - params.Add("acr_values", p.AcrValues) - } - if p.Prompt != "" { - params.Set("prompt", p.Prompt) - } else { // Legacy variant of the prompt param: - params.Set("approval_prompt", p.ApprovalPrompt) - } - params.Add("scope", p.Scope) - params.Set("client_id", p.ClientID) - params.Set("response_type", "code") - params.Add("state", state) - return a, params -} - // GetLoginURL with typical oauth parameters func (p *ProviderData) GetLoginURL(redirectURI, state string) string { - a, params := DefaultGetLoginURL(p, redirectURI, state) + a, params := makeLoginURL(p, redirectURI, state) a.RawQuery = params.Encode() return a.String() } diff --git a/providers/util.go b/providers/util.go index 374f637e..5cbc7fb9 100644 --- a/providers/util.go +++ b/providers/util.go @@ -3,6 +3,7 @@ package providers import ( "fmt" "net/http" + "net/url" ) const ( @@ -29,3 +30,22 @@ func makeOIDCHeader(accessToken string) http.Header { } return makeAuthorizationHeader(tokenTypeBearer, accessToken, extraHeaders) } + +func makeLoginURL(p *ProviderData, redirectURI, state string) (url.URL, url.Values) { + a := *p.LoginURL + params, _ := url.ParseQuery(a.RawQuery) + params.Set("redirect_uri", redirectURI) + if p.AcrValues != "" { + params.Add("acr_values", p.AcrValues) + } + if p.Prompt != "" { + params.Set("prompt", p.Prompt) + } else { // Legacy variant of the prompt param: + params.Set("approval_prompt", p.ApprovalPrompt) + } + params.Add("scope", p.Scope) + params.Set("client_id", p.ClientID) + params.Set("response_type", "code") + params.Add("state", state) + return a, params +} From 74918c40d8ba0975f84e101f5d39dc7378acdc51 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Tue, 15 Sep 2020 10:20:10 +0200 Subject: [PATCH 7/9] Refactor makeLoginURL to accept extraParams And don't require the caller to know how to use the returned params. --- providers/azure.go | 6 +++--- providers/logingov.go | 8 ++++---- providers/provider_default.go | 4 ++-- providers/util.go | 10 ++++++++-- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/providers/azure.go b/providers/azure.go index 9103d178..934f4511 100644 --- a/providers/azure.go +++ b/providers/azure.go @@ -212,10 +212,10 @@ func (p *AzureProvider) GetEmailAddress(ctx context.Context, s *sessions.Session } func (p *AzureProvider) GetLoginURL(redirectURI, state string) string { - a, params := makeLoginURL(p.ProviderData, redirectURI, state) + extraParams := url.Values{} if p.ProtectedResource != nil && p.ProtectedResource.String() != "" { - params.Add("resource", p.ProtectedResource.String()) + extraParams.Add("resource", p.ProtectedResource.String()) } - a.RawQuery = params.Encode() + a := makeLoginURL(p.ProviderData, redirectURI, state, extraParams) return a.String() } diff --git a/providers/logingov.go b/providers/logingov.go index 32fe1c78..a822108f 100644 --- a/providers/logingov.go +++ b/providers/logingov.go @@ -225,12 +225,12 @@ 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 { - a, params := makeLoginURL(p.ProviderData, redirectURI, state) + extraParams := url.Values{} if p.AcrValues == "" { acr := "http://idmanagement.gov/ns/assurance/loa/1" - params.Add("acr_values", acr) + extraParams.Add("acr_values", acr) } - params.Add("nonce", p.Nonce) - a.RawQuery = params.Encode() + extraParams.Add("nonce", p.Nonce) + a := makeLoginURL(p.ProviderData, redirectURI, state, extraParams) return a.String() } diff --git a/providers/provider_default.go b/providers/provider_default.go index 5fc53219..337b284c 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -75,8 +75,8 @@ func (p *ProviderData) Redeem(ctx context.Context, redirectURL, code string) (s // GetLoginURL with typical oauth parameters func (p *ProviderData) GetLoginURL(redirectURI, state string) string { - a, params := makeLoginURL(p, redirectURI, state) - a.RawQuery = params.Encode() + extraParams := url.Values{} + a := makeLoginURL(p, redirectURI, state, extraParams) return a.String() } diff --git a/providers/util.go b/providers/util.go index 5cbc7fb9..b4b65ac6 100644 --- a/providers/util.go +++ b/providers/util.go @@ -31,7 +31,7 @@ func makeOIDCHeader(accessToken string) http.Header { return makeAuthorizationHeader(tokenTypeBearer, accessToken, extraHeaders) } -func makeLoginURL(p *ProviderData, redirectURI, state string) (url.URL, url.Values) { +func makeLoginURL(p *ProviderData, redirectURI, state string, extraParams url.Values) url.URL { a := *p.LoginURL params, _ := url.ParseQuery(a.RawQuery) params.Set("redirect_uri", redirectURI) @@ -47,5 +47,11 @@ func makeLoginURL(p *ProviderData, redirectURI, state string) (url.URL, url.Valu params.Set("client_id", p.ClientID) params.Set("response_type", "code") params.Add("state", state) - return a, params + for n, p := range extraParams { + for _, v := range p { + params.Add(n, v) + } + } + a.RawQuery = params.Encode() + return a } From 090eff0197b7c347c95ad9a7461dc771b3432ed7 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Mon, 21 Sep 2020 09:45:16 +0200 Subject: [PATCH 8/9] Add CHANGELOG.md entries for #753 --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e4ec5bf..4bf63159 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,14 @@ ## Breaking Changes - [#722](https://github.com/oauth2-proxy/oauth2-proxy/pull/722) When a Redis session store is configured, OAuth2-Proxy will fail to start up unless connection and health checks to Redis pass +- A bug in the Azure provider prevented it from properly passing the configured protected `--resource` + via the login url. If this option was used in the past, behavior will change with this release as it will + affect the tokens returned by Azure. In the past, the tokens were always for `https://graph.microsoft.com` (the default) + and will now be for the configured resource (if it exists, otherwise it will run into errors) ## Changes since v6.1.1 +- [#753](https://github.com/oauth2-proxy/oauth2-proxy/pull/753) Pass resource parameter in login url (@codablock) - [#575](https://github.com/oauth2-proxy/oauth2-proxy/pull/575) Stop accepting legacy SHA1 signed cookies (@NickMeves) - [#722](https://github.com/oauth2-proxy/oauth2-proxy/pull/722) Validate Redis configuration options at startup (@NickMeves) - [#791](https://github.com/oauth2-proxy/oauth2-proxy/pull/791) Remove GetPreferredUsername method from provider interface (@NickMeves) From d046782f6171dad9ff100e21c8ae60d096d84395 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Tue, 29 Sep 2020 13:35:40 +0200 Subject: [PATCH 9/9] Add link to #753 in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bf63159..3e1ac927 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ ## Breaking Changes - [#722](https://github.com/oauth2-proxy/oauth2-proxy/pull/722) When a Redis session store is configured, OAuth2-Proxy will fail to start up unless connection and health checks to Redis pass -- A bug in the Azure provider prevented it from properly passing the configured protected `--resource` +- [#753](https://github.com/oauth2-proxy/oauth2-proxy/pull/753) A bug in the Azure provider prevented it from properly passing the configured protected `--resource` via the login url. If this option was used in the past, behavior will change with this release as it will affect the tokens returned by Azure. In the past, the tokens were always for `https://graph.microsoft.com` (the default) and will now be for the configured resource (if it exists, otherwise it will run into errors)