Repository: oauth2-proxy/oauth2-proxy
Pull request #754 (2020-09-03 12:36:27)

Azure token refresh
## Description

This is a re-submit of https://github.com/oauth2-proxy/oauth2-proxy/pull/278 after rebasing on the current master and applying necessary fixes needed due to changes in master since the original PR was created.

2 Additional changes were needed to fix issues we encountered with refresh tokens:
1. ~091727b09c49c19744e4ef46c21ca6a1c35d1a28~: This ensures that a refresh is performed early enough and that there is no chance of not refreshing and then running into an expired token a second later.
2. ~7505b020684d2f46bcc2afd01b926dca7880b10b: Tokens redeemed through a refresh token do not contain email addresses anymore. This behavior was quite strange and I can't really say what is going on. I figured out the easiest and still safe solution would be to allow empty emails when `--email-domain=*` is used.~ EDIT: This is obsolete due to https://github.com/oauth2-proxy/oauth2-proxy/pull/770

## Motivation and Context

This PR implements Azure token refreshs, as requested in https://github.com/oauth2-proxy/oauth2-proxy/issues/260

## 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. The original [PR](https://github.com/oauth2-proxy/oauth2-proxy/pull/278) also contained test cases for the new functionality, these tests are also present in this PR.

## 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.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.


JoeSainsburys: Did you ever get this working? I've tried using this branch and I've tried implementing it in the OIDC provider and I can't get an identity_token. [Here ](https://github.com/MicrosoftDocs/azure-docs/issues/34799#issuecomment-510983463) MS are pretty explicit in saying that you can't refresh identity tokens like this.
codablock: @JoeSainsburys You have to use access tokens. At the same time, the default access tokens returned by Azure AD are for graph.microsoft.com, which probably not what you actually want (these tokens are also hard to verify due to some internal use of nonces). You must add a custom scope to the app registration in Azure and then use the scope id (without the `api://` part, so really only the ID) in `--resource` and the scope name as `--scope`. After that you'll get refreshable access tokens for your own scope.
codablock: @JoeSainsburys it's also important to have https://github.com/oauth2-proxy/oauth2-proxy/pull/753 merged as well when you want to try it out, as otherwise you'll still get the graph.microsoft.com access tokens even if you specify --resource.
JoeSainsburys: Ahhh, thank you! I was thinking it had to be something to do with it either being set up as a B2C app or actually using an access token. I'll give it a go, thanks.
JoeSainsburys: I think I've got it working to the point where it gets a refreshed access token, but it falls down when attempting to query the /me endpoint. It says that the audience is wrong (which makes sense because we have changed it from graph.microsoft.com to our app). I think that this is probably an issue with my permissions setup. I've not got the Directory.AccessAsUser.All permission, realistically I'm probably not going to get this (our tenant has ~200,000 users and that sounds like delegated god mode.) Would you expect to see this error if I was missing that permission?
codablock: @JoeSainsburys Unfortunately I can't help with that issue, as we're not using graph.microsoft.com in our applications. We solely use the access tokens for authentication and then use internal apis/resources to gather information about the logged in user.

If you only need access to graph.microsoft.com and do not plan on using the token to do your own authentication/authorization, it's probably ok to just stick with graph.microsoft.com and but use access instead of id tokens instead. This then means you can't really verify the access tokens by yourself, but you can use the token to access graph.microsoft.com.

Btw, I'm not an oauth/oidc/AD specialist and my knowledge is very limited in this regard, so please take my works with a grain of salt, especially as this is security related.
codablock: https://github.com/oauth2-proxy/oauth2-proxy/commit/7505b020684d2f46bcc2afd01b926dca7880b10b will become obsolete after #770 is merged, so I'm going to remote that commit from here.
codablock: Rebased on master and handled the requested changes (except the NIT as I'm for sticking with surrounding style and doing style fixes in separate PRs).
codablock: @NickMeves please see https://github.com/oauth2-proxy/oauth2-proxy/pull/787 for CHANGELOG.
codablock: @NickMeves I've rebased on develop, fixed merge conflicts and squashed everything into the first commit. I've also added CHANGELOG entries to this PR.
knightian: Wen merge 😅
NickMeves: @codablock I think once you resolve the couple of notes @JoelSpeed left this is good to merge.

FYI - There is a draft in progress of the refresh timing logic centralization we discussed: https://github.com/oauth2-proxy/oauth2-proxy/pull/886
codablock: Sorry for the delay, I didn't notice the comments from JoelSpeed. I've applied suggestions/fixes and rebased on master. I hope we're ready for the merge now :)
knightian: ping ping ping
NickMeves: > ping ping ping 😅😃

You can always Fork if we are too slow for you 😛 
knightian: > > ping ping ping 😅😃
> 
> You can always Fork if we are too slow for you 😛

Haha it is true what you say, and I thought about it, but because it's a security thing I decided I'll stay on official-master

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f9f3f1f1..67eb73ba 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -23,9 +23,16 @@
   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)
+- [#754](https://github.com/oauth2-proxy/oauth2-proxy/pull/754) The Azure provider now has token refresh functionality implemented. This means that there won't
+  be any redirects in the browser anymore when tokens expire, but instead a token refresh is initiated
+  in the background, which leads to new tokens being returned in the cookies.
+  - Please note that `--cookie-refresh` must be 0 (the default) or equal to the token lifespan configured in Azure AD to make
+  Azure token refresh reliable. Setting this value to 0 means that it relies on the provider implementation
+  to decide if a refresh is required.
 
 ## Changes since v6.1.1
 
+- [#754](https://github.com/oauth2-proxy/oauth2-proxy/pull/754) Azure token refresh (@codablock)
 - [#825](https://github.com/oauth2-proxy/oauth2-proxy/pull/825) Fix code coverage reporting on GitHub actions(@JoelSpeed)
 - [#796](https://github.com/oauth2-proxy/oauth2-proxy/pull/796) Deprecate GetUserName & GetEmailAdress for EnrichSessionState (@NickMeves)
 - [#705](https://github.com/oauth2-proxy/oauth2-proxy/pull/705) Add generic Header injectors for upstream request and response headers (@JoelSpeed)
diff --git a/providers/azure.go b/providers/azure.go
index 234aaff2..d65b11f4 100644
--- a/providers/azure.go
+++ b/providers/azure.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"errors"
+	"fmt"
 	"net/http"
 	"net/url"
 	"time"
@@ -74,6 +75,9 @@ func NewAzureProvider(p *ProviderData) *AzureProvider {
 	if p.ProtectedResource == nil || p.ProtectedResource.String() == "" {
 		p.ProtectedResource = azureDefaultProtectResourceURL
 	}
+	if p.ValidateURL == nil || p.ValidateURL.String() == "" {
+		p.ValidateURL = p.ProfileURL
+	}
 
 	return &AzureProvider{
 		ProviderData: p,
@@ -103,6 +107,7 @@ func overrideTenantURL(current, defaultURL *url.URL, tenant, path string) {
 	}
 }
 
+// Redeem exchanges the OAuth2 authentication token for an ID token
 func (p *AzureProvider) Redeem(ctx context.Context, redirectURL, code string) (s *sessions.SessionState, err error) {
 	if code == "" {
 		err = errors.New("missing code")
@@ -123,6 +128,7 @@ func (p *AzureProvider) Redeem(ctx context.Context, redirectURL, code string) (s
 		params.Add("resource", p.ProtectedResource.String())
 	}
 
+	// blindly try json and x-www-form-urlencoded
 	var jsonResponse struct {
 		AccessToken  string `json:"access_token"`
 		RefreshToken string `json:"refresh_token"`
@@ -151,6 +157,61 @@ func (p *AzureProvider) Redeem(ctx context.Context, redirectURL, code string) (s
 		RefreshToken: jsonResponse.RefreshToken,
 	}
 	return
+
+}
+
+// RefreshSessionIfNeeded checks if the session has expired and uses the
+// RefreshToken to fetch a new ID token if required
+func (p *AzureProvider) RefreshSessionIfNeeded(ctx context.Context, s *sessions.SessionState) (bool, error) {
+	if s == nil || s.ExpiresOn.After(time.Now()) || s.RefreshToken == "" {
+		return false, nil
+	}
+
+	origExpiration := s.ExpiresOn
+
+	err := p.redeemRefreshToken(ctx, s)
+	if err != nil {
+		return false, fmt.Errorf("unable to redeem refresh token: %v", err)
+	}
+
+	fmt.Printf("refreshed id token %s (expired on %s)\n", s, origExpiration)
+	return true, nil
+}
+
+func (p *AzureProvider) redeemRefreshToken(ctx context.Context, s *sessions.SessionState) (err error) {
+	params := url.Values{}
+	params.Add("client_id", p.ClientID)
+	params.Add("client_secret", p.ClientSecret)
+	params.Add("refresh_token", s.RefreshToken)
+	params.Add("grant_type", "refresh_token")
+
+	var jsonResponse struct {
+		AccessToken  string `json:"access_token"`
+		RefreshToken string `json:"refresh_token"`
+		ExpiresOn    int64  `json:"expires_on,string"`
+		IDToken      string `json:"id_token"`
+	}
+
+	err = requests.New(p.RedeemURL.String()).
+		WithContext(ctx).
+		WithMethod("POST").
+		WithBody(bytes.NewBufferString(params.Encode())).
+		SetHeader("Content-Type", "application/x-www-form-urlencoded").
+		Do().
+		UnmarshalInto(&jsonResponse)
+
+	if err != nil {
+		return
+	}
+
+	now := time.Now()
+	expires := time.Unix(jsonResponse.ExpiresOn, 0)
+	s.AccessToken = jsonResponse.AccessToken
+	s.IDToken = jsonResponse.IDToken
+	s.RefreshToken = jsonResponse.RefreshToken
+	s.CreatedAt = &now
+	s.ExpiresOn = &expires
+	return
 }
 
 func makeAzureHeader(accessToken string) http.Header {
@@ -219,3 +280,8 @@ func (p *AzureProvider) GetLoginURL(redirectURI, state string) string {
 	a := makeLoginURL(p.ProviderData, redirectURI, state, extraParams)
 	return a.String()
 }
+
+// ValidateSessionState validates the AccessToken
+func (p *AzureProvider) ValidateSessionState(ctx context.Context, s *sessions.SessionState) bool {
+	return validateToken(ctx, p, s.AccessToken, makeAzureHeader(s.AccessToken))
+}


Test patch:
diff --git a/providers/azure_test.go b/providers/azure_test.go
index 6e2e4e97..9e3cabf7 100644
--- a/providers/azure_test.go
+++ b/providers/azure_test.go
@@ -8,6 +8,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"
+
 	. "github.com/onsi/gomega"
 	"github.com/stretchr/testify/assert"
 )
@@ -42,7 +44,7 @@ func TestNewAzureProvider(t *testing.T) {
 	g.Expect(providerData.LoginURL.String()).To(Equal("https://login.microsoftonline.com/common/oauth2/authorize"))
 	g.Expect(providerData.RedeemURL.String()).To(Equal("https://login.microsoftonline.com/common/oauth2/token"))
 	g.Expect(providerData.ProfileURL.String()).To(Equal("https://graph.microsoft.com/v1.0/me"))
-	g.Expect(providerData.ValidateURL.String()).To(Equal(""))
+	g.Expect(providerData.ValidateURL.String()).To(Equal("https://graph.microsoft.com/v1.0/me"))
 	g.Expect(providerData.Scope).To(Equal("openid"))
 }
 
@@ -97,7 +99,7 @@ func TestAzureSetTenant(t *testing.T) {
 		p.Data().ProfileURL.String())
 	assert.Equal(t, "https://graph.microsoft.com",
 		p.Data().ProtectedResource.String())
-	assert.Equal(t, "", p.Data().ValidateURL.String())
+	assert.Equal(t, "https://graph.microsoft.com/v1.0/me", p.Data().ValidateURL.String())
 	assert.Equal(t, "openid", p.Data().Scope)
 }
 
@@ -220,3 +222,47 @@ func TestAzureProviderProtectedResourceConfigured(t *testing.T) {
 	result := p.GetLoginURL("https://my.test.app/oauth", "")
 	assert.Contains(t, result, "resource="+url.QueryEscape("http://my.resource.test"))
 }
+
+func TestAzureProviderGetsTokensInRedeem(t *testing.T) {
+	b := testAzureBackend(`{ "access_token": "some_access_token", "refresh_token": "some_refresh_token", "expires_on": "1136239445", "id_token": "some_id_token" }`)
+	defer b.Close()
+	timestamp, _ := time.Parse(time.RFC3339, "2006-01-02T22:04:05Z")
+	bURL, _ := url.Parse(b.URL)
+	p := testAzureProvider(bURL.Host)
+
+	session, err := p.Redeem(context.Background(), "http://redirect/", "code1234")
+	assert.Equal(t, nil, err)
+	assert.NotEqual(t, session, nil)
+	assert.Equal(t, "some_access_token", session.AccessToken)
+	assert.Equal(t, "some_refresh_token", session.RefreshToken)
+	assert.Equal(t, "some_id_token", session.IDToken)
+	assert.Equal(t, timestamp, session.ExpiresOn.UTC())
+}
+
+func TestAzureProviderNotRefreshWhenNotExpired(t *testing.T) {
+	p := testAzureProvider("")
+
+	expires := time.Now().Add(time.Duration(1) * time.Hour)
+	session := &sessions.SessionState{AccessToken: "some_access_token", RefreshToken: "some_refresh_token", IDToken: "some_id_token", ExpiresOn: &expires}
+	refreshNeeded, err := p.RefreshSessionIfNeeded(context.Background(), session)
+	assert.Equal(t, nil, err)
+	assert.False(t, refreshNeeded)
+}
+
+func TestAzureProviderRefreshWhenExpired(t *testing.T) {
+	b := testAzureBackend(`{ "access_token": "new_some_access_token", "refresh_token": "new_some_refresh_token", "expires_on": "32693148245", "id_token": "new_some_id_token" }`)
+	defer b.Close()
+	timestamp, _ := time.Parse(time.RFC3339, "3006-01-02T22:04:05Z")
+	bURL, _ := url.Parse(b.URL)
+	p := testAzureProvider(bURL.Host)
+
+	expires := time.Now().Add(time.Duration(-1) * time.Hour)
+	session := &sessions.SessionState{AccessToken: "some_access_token", RefreshToken: "some_refresh_token", IDToken: "some_id_token", ExpiresOn: &expires}
+	_, err := p.RefreshSessionIfNeeded(context.Background(), session)
+	assert.Equal(t, nil, err)
+	assert.NotEqual(t, session, nil)
+	assert.Equal(t, "new_some_access_token", session.AccessToken)
+	assert.Equal(t, "new_some_refresh_token", session.RefreshToken)
+	assert.Equal(t, "new_some_id_token", session.IDToken)
+	assert.Equal(t, timestamp, session.ExpiresOn.UTC())
+}