🚑️ OIDC: use Azure-AD unique identifier (#2510)

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 <[email protected]> Co-authored-by: slang <[email protected]>
zalando · Aug 14, 2023 · 0ee30aa · 0ee30aa
1 parent 5b429fe
commit 0ee30aa
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 13 deletions.
diff --git 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
@@ -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)