Repository: oauth2-proxy/oauth2-proxy
Pull request #753 (2020-09-03 12:32:34)

Pass resource parameter in login url
## Description

This passes the value given by `--resource=<my-resource>` to the login url.

## Motivation and Context

Without this, Azure will always return access tokens for graph.microsoft.com, which is not what one would expect when `--resource` is used. The modified method is not Azure specific, but I would assume that the resource parameter should also be present in other providers.

## How Has This Been Tested?

We have this code (based on a [master](cfa1160) version shortly after 6.0.0 was released) running in production since a few weeks.

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [ ] My change requires a change to the documentation or CHANGELOG.
- [ ] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.


codablock: @NickMeves I've added a unit test for the `resource=xxx` parameter.

Unfortunately it is a huge mess to navigate through Azure AD documentation and keep track of all the details and how stuff is related to other stuff...so I have no idea anymore where I got this info from. Googling for it I found this [Stackoverflow](https://stackoverflow.com/questions/23921884/what-is-the-resource-parameter-in-windows-azure-ad-tenant-application-oauth-2-0) entry which describes the resource parameter in the response, but also fails to mention the documentation that actually describes it.
codablock: @NickMeves I've refactored the default_provider.go GetLoginURL in a way so that it can be re-used by other providers. Azure now uses the new `DefaultGetLoginURL` and then adds azure specific stuff to the `params`. And just for fun, I used the same method to de-duplicate the logingov provider.
NickMeves: @codablock Just an FYI - I need to think about the repercussions more, but I think this and your other PR might barely hover on the `breaking` change threshold between requiring a minor vs major release to get this functionality out. (for instance `--cookie-refresh` & the session validation never did anything in the past for Azure - but it wasn't documented that it wasn't Azure supported, so users might've set flags and not realized they were no-ops and then starting to care about them might break setups).

But the point might be moot - I'm chatting with @JoelSpeed about our next release potentially being `v7` since we are starting to get to a critical mass of PRs that need a major release.

We'll keep you posted.
codablock: @NickMeves I'm fine with this being a major release and thus needing more time. We're already running oauth2-proxy from a forked branch with the necessary changes merged.
codablock: @NickMeves please see https://github.com/oauth2-proxy/oauth2-proxy/pull/787 for CHANGELOG.
NickMeves: I'm ready to merge this one -- can you get the CHANGELOG entry over in this PR and not split out and rebase?

@JoelSpeed wanted to take a peek at #754 still
JoelSpeed: I agree, let's get this PR rebased, add the changelog into this PR and then we can merge, it's much easier to look back at changes if the changelog entry is in the same PR

Do we need to update the docs at all?

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5e4ec5bf..3e1ac927 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
+- [#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)
 
 ## 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)
diff --git a/providers/azure.go b/providers/azure.go
index 0ae0cba6..934f4511 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 {
+	extraParams := url.Values{}
+	if p.ProtectedResource != nil && p.ProtectedResource.String() != "" {
+		extraParams.Add("resource", p.ProtectedResource.String())
+	}
+	a := makeLoginURL(p.ProviderData, redirectURI, state, extraParams)
+	return a.String()
+}
diff --git a/providers/logingov.go b/providers/logingov.go
index c524741f..a822108f 100644
--- a/providers/logingov.go
+++ b/providers/logingov.go
@@ -225,20 +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 := *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"
-	}
-	params.Add("acr_values", acr)
-	params.Add("nonce", p.Nonce)
-	a.RawQuery = params.Encode()
+	extraParams := url.Values{}
+	if p.AcrValues == "" {
+		acr := "http://idmanagement.gov/ns/assurance/loa/1"
+		extraParams.Add("acr_values", acr)
+	}
+	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 ba05a96c..337b284c 100644
--- a/providers/provider_default.go
+++ b/providers/provider_default.go
@@ -75,22 +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 := *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)
-	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 374f637e..b4b65ac6 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,28 @@ func makeOIDCHeader(accessToken string) http.Header {
 	}
 	return makeAuthorizationHeader(tokenTypeBearer, accessToken, extraHeaders)
 }
+
+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)
+	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)
+	for n, p := range extraParams {
+		for _, v := range p {
+			params.Add(n, v)
+		}
+	}
+	a.RawQuery = params.Encode()
+	return a
+}


Test patch:
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/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")
+}