From 0ee30aa5dc7f584ac62fb18471575043079ff768 Mon Sep 17 00:00:00 2001
From: Samuel Lang <gh@lang-sam.de>
Date: Mon, 14 Aug 2023 12:50:24 +0200
Subject: [PATCH] =?UTF-8?q?=F0=9F=9A=91=EF=B8=8F=20OIDC:=20use=20Azure-AD?=
 =?UTF-8?q?=20unique=20identifier=20(#2510)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We are facing a security vulnerability by the use of `DisplayName` as the unique identifier in Azure-AD.

Since this field can be duplicated it can be exploited to gain privileges.

https://morgansimonsen.com/2016/06/28/azure-ad-allows-duplicate-group-names/

Switching to the guaranteed unique identifier `onPremisesSamAccountName` available in the Microsoft Graph API

Signed-off-by: Samuel Lang <gh@lang-sam.de>
Co-authored-by: slang <slang@bethel.jw.org>
---
 filters/auth/oidc.go      | 11 +++++++----
 filters/auth/oidc_test.go | 19 ++++++++++---------
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/filters/auth/oidc.go b/filters/auth/oidc.go
index 6a4cb3ab3f..c3b87f174e 100644
--- a/filters/auth/oidc.go
+++ b/filters/auth/oidc.go
@@ -65,8 +65,8 @@ type claimSource struct {
 type azureGraphGroups struct {
 	OdataNextLink string `json:"@odata.nextLink,omitempty"`
 	Value         []struct {
-		DisplayName string `json:"displayName"`
-		ID          string `json:"id"`
+		OnPremisesSamAccountName string `json:"onPremisesSamAccountName"`
+		ID                       string `json:"id"`
 	} `json:"value"`
 }
 
@@ -1017,7 +1017,7 @@ func (f *tokenOidcFilter) handleDistributedClaimsAzure(url *url.URL, oauth2Token
 	}
 	url.Path = fmt.Sprintf("/v1.0/users/%s/transitiveMemberOf", userID)
 	q := url.Query()
-	q.Set("$select", "displayName,id")
+	q.Set("$select", "onPremisesSamAccountName,id")
 	url.RawQuery = q.Encode()
 	return f.resolveDistributedClaimAzure(url, oauth2Token)
 }
@@ -1075,7 +1075,9 @@ func (f *tokenOidcFilter) resolveDistributedClaimAzure(url *url.URL, oauth2Token
 		return nil, fmt.Errorf("unabled to decode response: %w", err)
 	}
 	for _, v := range target.Value {
-		values = append(values, v.DisplayName)
+		if v.OnPremisesSamAccountName != "" {
+			values = append(values, v.OnPremisesSamAccountName)
+		}
 	}
 	// recursive pagination
 	if target.OdataNextLink != "" {
@@ -1089,6 +1091,7 @@ func (f *tokenOidcFilter) resolveDistributedClaimAzure(url *url.URL, oauth2Token
 		}
 		values = append(values, vs...)
 	}
+	log.Debugf("Distributed claim is :%v", values)
 	return
 }
 
diff --git a/filters/auth/oidc_test.go b/filters/auth/oidc_test.go
index 8fad17d596..3ab4569d7b 100644
--- a/filters/auth/oidc_test.go
+++ b/filters/auth/oidc_test.go
@@ -316,15 +316,15 @@ func createOIDCServer(cb, client, clientsecret string, extraClaims jwt.MapClaims
 			}
 		case "/v1.0/users/me/transitiveMemberOf":
 			if r.Header.Get(authHeaderName) == authHeaderPrefix+validAccessToken &&
-				r.URL.Query().Get("$select") == "displayName,id" {
+				r.URL.Query().Get("$select") == "onPremisesSamAccountName,id" {
 				body, err := json.Marshal(azureGraphGroups{
 					OdataNextLink: fmt.Sprintf("http://%s/v1.0/users/paginatedresponse", r.Host),
 					Value: []struct {
-						DisplayName string `json:"displayName"`
-						ID          string `json:"id"`
+						OnPremisesSamAccountName string `json:"onPremisesSamAccountName"`
+						ID                       string `json:"id"`
 					}{
-						{DisplayName: "CD-Administrators", ID: "1"},
-						{DisplayName: "Purchasing-Department", ID: "2"},
+						{OnPremisesSamAccountName: "CD-Administrators", ID: "1"},
+						{OnPremisesSamAccountName: "Purchasing-Department", ID: "2"},
 					}})
 				if err != nil {
 					log.Fatalf("Failed to marshal to json: %v", err)
@@ -338,11 +338,12 @@ func createOIDCServer(cb, client, clientsecret string, extraClaims jwt.MapClaims
 				body, err := json.Marshal(azureGraphGroups{
 					OdataNextLink: "",
 					Value: []struct {
-						DisplayName string `json:"displayName"`
-						ID          string `json:"id"`
+						OnPremisesSamAccountName string `json:"onPremisesSamAccountName"`
+						ID                       string `json:"id"`
 					}{
-						{DisplayName: "AppX-Test-Users", ID: "3"},
-						{DisplayName: "white space", ID: "4"},
+						{OnPremisesSamAccountName: "AppX-Test-Users", ID: "3"},
+						{OnPremisesSamAccountName: "white space", ID: "4"},
+						{ID: "5"}, // null value
 					}})
 				if err != nil {
 					log.Fatalf("Failed to marshal to json: %v", err)