diff --git a/auth/providers/azure/azure.go b/auth/providers/azure/azure.go index 92338fe9..8b7db894 100644 --- a/auth/providers/azure/azure.go +++ b/auth/providers/azure/azure.go @@ -267,6 +267,8 @@ func (s Authenticator) Check(ctx context.Context, token string) (*authv1.UserInf return nil, err } + isServicePrincipal := isAppToken(claims) + if s.Options.ResolveGroupMembershipOnlyOnOverageClaim { groups, skipGraphAPI, err := getGroupsAndCheckOverage(claims) if err != nil { @@ -276,16 +278,16 @@ func (s Authenticator) Check(ctx context.Context, token string) (*authv1.UserInf resp.Groups = groups return resp, nil } - // Service principal token with >200 groups. OBO flow does not - // support SPNs, so short-circuit instead of letting AAD fail with - // a cryptic AADSTS7000113. + + // Service principal token with an overage claim (>200 groups) while SP + // group resolution is switched off: short-circuit instead of letting the + // user flow fail with a cryptic AADSTS7000113. // - // StatusOK: K8s webhook authenticator only reads and logs - // TokenReview Status.Error on HTTP 200. Non-200 is treated as a - // transport error and the message is discarded. We need this error - // to surface in API server logs so operators can diagnose why the - // SPN authentication was rejected. - if isAppToken(claims) { + // StatusOK: K8s webhook authenticator only reads and logs TokenReview + // Status.Error on HTTP 200. Non-200 is treated as a transport error and + // the message is discarded. We need this error to surface in API server + // logs so operators can diagnose why the SPN was rejected. + if !s.Options.EnableSPGroupResolution && isServicePrincipal { return nil, errutils.WithCode( fmt.Errorf( "service principal with group membership exceeding 200 is not supported. "+ @@ -299,11 +301,20 @@ func (s Authenticator) Check(ctx context.Context, token string) (*authv1.UserInf if err := s.graphClient.RefreshToken(ctx, token); err != nil { return nil, err } - resp.Groups, err = s.graphClient.GetGroups(ctx, resp.Username, token) + principal := resp.Username + if isServicePrincipal { + // Service principals are resolved by object ID against the + // /servicePrincipals resource. + spOID, oidErr := claims.string(azureObjectIDClaim) + if oidErr != nil { + return nil, errors.Wrap(oidErr, "unable to get oid claim for service principal") + } + principal = spOID + } + resp.Groups, err = s.graphClient.GetGroups(ctx, principal, token, isServicePrincipal) if err != nil { return nil, errors.Wrap(err, "failed to get groups") } - } return resp, nil } diff --git a/auth/providers/azure/azure_test.go b/auth/providers/azure/azure_test.go index 963b5c75..6ee7dac9 100644 --- a/auth/providers/azure/azure_test.go +++ b/auth/providers/azure/azure_test.go @@ -32,6 +32,7 @@ import ( "time" "go.kubeguard.dev/guard/auth/providers/azure/graph" + errutils "go.kubeguard.dev/guard/util/error" "github.com/coreos/go-oidc" "github.com/go-chi/chi/v5" @@ -216,6 +217,15 @@ func serverSetup(loginResp string, loginStatus int, jwkResp, groupIds, groupList _, _ = w.Write(groupIds) })) + m.Post("/api/servicePrincipals/{oid}/getMemberGroups", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if len(groupStatus) > 0 { + w.WriteHeader(groupStatus[0]) + } else { + w.WriteHeader(http.StatusOK) + } + _, _ = w.Write(groupIds) + })) + m.Post("/api/directoryObjects/getByIds", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) _, _ = w.Write(groupList) @@ -456,7 +466,26 @@ func TestCheckAzureAuthenticationSPNWithOverage(t *testing.T) { t.Fatalf("Error when creating signing key. reason : %v", err) } - t.Run("SPN token with overage claim should return clear error", func(t *testing.T) { + t.Run("SPN token with overage claim resolves groups via servicePrincipals getMemberGroups", func(t *testing.T) { + srv, client := getServerAndClient(t, signKey, loginResp, 3, true, false, ClientCredentialAuthMode) + client.Options.ResolveGroupMembershipOnlyOnOverageClaim = true + client.Options.UseGroupUID = true + client.Options.EnableSPGroupResolution = true + defer srv.Close() + + token, err := signKey.sign([]byte(fmt.Sprintf(accessTokenSPNWithOverage, srv.URL))) + if err != nil { + t.Fatalf("Error when signing token. reason: %v", err) + } + + resp, err := client.Check(ctx, token) + assert.Nil(t, err) + assert.NotNil(t, resp) + assert.Equal(t, 3, len(resp.Groups)) + assert.ElementsMatch(t, []string{"1", "2", "3"}, resp.Groups) + }) + + t.Run("SPN token with overage claim is rejected when SP group resolution is disabled", func(t *testing.T) { srv, client := getServerAndClient(t, signKey, loginResp, 3, true, false, ClientCredentialAuthMode) client.Options.ResolveGroupMembershipOnlyOnOverageClaim = true client.Options.UseGroupUID = true @@ -471,11 +500,29 @@ func TestCheckAzureAuthenticationSPNWithOverage(t *testing.T) { assert.NotNil(t, err) assert.Nil(t, resp) assert.Contains(t, err.Error(), "service principal with group membership exceeding 200 is not supported") - assert.Contains(t, err.Error(), "kubelogin-authentication-in-aks-limitations") - codeErr, ok := err.(interface{ Code() int }) - assert.True(t, ok, "error should implement Code()") - assert.Equal(t, http.StatusOK, codeErr.Code(), "error should HTTP 200 so the API server logs the message.") + codeErr, ok := err.(errutils.HttpStatusCode) + assert.True(t, ok, "error should carry an http status code") + assert.Equal(t, http.StatusOK, codeErr.Code()) + }) + + t.Run("SPN token with overage claim surfaces graph failure", func(t *testing.T) { + srv, client := getServerAndClient(t, signKey, loginResp, 3, true, false, ClientCredentialAuthMode, http.StatusInternalServerError) + client.Options.ResolveGroupMembershipOnlyOnOverageClaim = true + client.Options.UseGroupUID = true + client.Options.EnableSPGroupResolution = true + defer srv.Close() + + token, err := signKey.sign([]byte(fmt.Sprintf(accessTokenSPNWithOverage, srv.URL))) + if err != nil { + t.Fatalf("Error when signing token. reason: %v", err) + } + + resp, err := client.Check(ctx, token) + assert.NotNil(t, err) + assert.Nil(t, resp) + assert.Contains(t, err.Error(), "failed to get groups") + assert.NotContains(t, err.Error(), "service principal with group membership exceeding 200 is not supported") }) t.Run("SPN token with groups in token should succeed (no Graph call needed)", func(t *testing.T) { diff --git a/auth/providers/azure/graph/graph.go b/auth/providers/azure/graph/graph.go index a9e430e1..b8040d02 100644 --- a/auth/providers/azure/graph/graph.go +++ b/auth/providers/azure/graph/graph.go @@ -48,11 +48,18 @@ var ( expandedGroupsPerCall = 500 idtypClaim = "idtyp" + // For getMemberGroups (user flow) getMemberGroupsFailed = promauto.NewCounter(prometheus.CounterOpts{ Name: "guard_azure_graph_failure_total", Help: "Azure graph getMemberGroups call failed.", }) + // For getMemberGroups (service principal flow) + getMemberGroupsForSPFailed = promauto.NewCounter(prometheus.CounterOpts{ + Name: "guard_azure_graph_sp_failure_total", + Help: "Azure graph getMemberGroups call for service principal failed.", + }) + getMemberGroupsUsingARCOboServiceCounter = prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "guard_azure_arc_obo_request_total", Help: "Total no of arc obo getMemberGroups calls by code.", @@ -100,15 +107,23 @@ type UserInfo struct { lock sync.RWMutex } -func (u *UserInfo) getGroupIDs(ctx context.Context, userPrincipal string) ([]string, error) { - // Create a new request for finding the user. +// getGroupIDs returns the group IDs the given principal is a member of. Users and +// service principals are distinct Graph resources, so the principal type selects +// the resource to query: users are looked up by user principal name, service +// principals by object ID. +func (u *UserInfo) getGroupIDs(ctx context.Context, principal string, isServicePrincipal bool) ([]string, error) { + // Create a new request for finding the principal. // Shallow copy of the base API URL - userSearchURL := *u.apiURL + searchURL := *u.apiURL + resource := "users" + if isServicePrincipal { + resource = "servicePrincipals" + } // Append the path for the member list - userSearchURL.Path = path.Join(userSearchURL.Path, fmt.Sprintf("/users/%s/getMemberGroups", userPrincipal)) + searchURL.Path = path.Join(searchURL.Path, fmt.Sprintf("/%s/%s/getMemberGroups", resource, principal)) // The body being sent makes sure that all groups are returned, not just security groups - req, err := http.NewRequest(http.MethodPost, userSearchURL.String(), strings.NewReader(`{"securityEnabledOnly": false}`)) + req, err := http.NewRequest(http.MethodPost, searchURL.String(), strings.NewReader(`{"securityEnabledOnly": false}`)) if err != nil { return nil, errors.Wrap(err, "error creating group IDs request") } @@ -117,8 +132,12 @@ func (u *UserInfo) getGroupIDs(ctx context.Context, userPrincipal string) ([]str resp, err := u.client.Do(req.WithContext(ctx)) if err != nil { - getMemberGroupsFailed.Inc() - return nil, errors.Wrap(err, "error listing users") + if isServicePrincipal { + getMemberGroupsForSPFailed.Inc() + } else { + getMemberGroupsFailed.Inc() + } + return nil, errors.Wrapf(err, "error listing group memberships for %s", resource) } defer func() { _ = resp.Body.Close() @@ -363,10 +382,12 @@ func (u *UserInfo) isTokenExpired() bool { return u.expires.Before(time.Now()) } -// GetGroups gets a list of all groups that the given user principal is part of -// Generally in federated directories the email address is the userPrincipalName -func (u *UserInfo) GetGroups(ctx context.Context, userPrincipal string, token string) ([]string, error) { +// GetGroups gets a list of all groups that the given principal is part of. +// For users the principal is generally the email address in federated directories +// (the userPrincipalName), for service principals it is the object ID. +func (u *UserInfo) GetGroups(ctx context.Context, principal string, token string, isServicePrincipal bool) ([]string, error) { // use arc obo service to get groups if authn mode is arc + // the arc obo service rejects service principal tokens with a clear error if u.authMode == arcAuthMode { groupIds, err := u.getMemberGroupsUsingARCOboService(ctx, token) if err != nil { @@ -375,8 +396,8 @@ func (u *UserInfo) GetGroups(ctx context.Context, userPrincipal string, token st return groupIds, nil } - // Get the group IDs for the user - groupIDs, err := u.getGroupIDs(ctx, userPrincipal) + // Get the group IDs for the principal + groupIDs, err := u.getGroupIDs(ctx, principal, isServicePrincipal) if err != nil { return nil, err } diff --git a/auth/providers/azure/graph/graph_test.go b/auth/providers/azure/graph/graph_test.go index a82c68e0..bc308eed 100644 --- a/auth/providers/azure/graph/graph_test.go +++ b/auth/providers/azure/graph/graph_test.go @@ -194,7 +194,7 @@ func TestGetGroupIDs(t *testing.T) { ts, u := getAPIServerAndUserInfo(http.StatusOK, validBody) defer ts.Close() - groups, err := u.getGroupIDs(ctx, "john.michael.kane@yacht.io") + groups, err := u.getGroupIDs(ctx, "john.michael.kane@yacht.io", false) if err != nil { t.Errorf("Should not have gotten error: %s", err) } @@ -206,7 +206,7 @@ func TestGetGroupIDs(t *testing.T) { ts, u := getAPIServerAndUserInfo(http.StatusInternalServerError, "shutdown") defer ts.Close() - groups, err := u.getGroupIDs(ctx, "alexander.conklin@cia.gov") + groups, err := u.getGroupIDs(ctx, "alexander.conklin@cia.gov", false) if err == nil { t.Error("Should have gotten error") } @@ -224,7 +224,7 @@ func TestGetGroupIDs(t *testing.T) { groupsPerCall: expandedGroupsPerCall, } - groups, err := u.getGroupIDs(ctx, "richard.webb@cia.gov") + groups, err := u.getGroupIDs(ctx, "richard.webb@cia.gov", false) if err == nil { t.Error("Should have gotten error") } @@ -236,7 +236,7 @@ func TestGetGroupIDs(t *testing.T) { ts, u := getAPIServerAndUserInfo(http.StatusOK, "{bad_json") defer ts.Close() - groups, err := u.getGroupIDs(ctx, "nicky.parsons@cia.gov") + groups, err := u.getGroupIDs(ctx, "nicky.parsons@cia.gov", false) if err == nil { t.Error("Should have gotten error") } @@ -489,6 +489,136 @@ func TestGetMemberGroupsUsingARCOboService(t *testing.T) { }) } +func TestGetGroupsForServicePrincipal(t *testing.T) { + ctx := context.Background() + + t.Run("arc auth mode routes to the arc obo service", func(t *testing.T) { + key, err := NewSwkKey() + if err != nil { + t.Fatalf("Failed to generate SF key. Error:%+v", err) + } + + ts, u := getAPIServerAndUserInfo(http.StatusOK, `{"value": []}`) + u.region = location + u.authMode = arcAuthMode + u.resourceID = ts.URL + u.tenantID = tenant_id + defer ts.Close() + + getOBORegionalEndpoint = func(location string, resourceID string) (string, error) { + return ts.URL, nil + } + + u.headers.Set("Authorization", "Bearer msitoken") + + tokenstring, err := key.GenerateToken([]byte(fmt.Sprintf(accessTokenWithOverageClaimForApp, ts.URL, time.Now().Add(time.Minute*5).Unix()))) + if err != nil { + t.Fatalf("Error when generating token. Error:%+v", err) + } + + groups, err := u.GetGroups(ctx, "spn-oid-123", tokenstring, true) + if err == nil { + t.Fatal("Should have gotten error") + } + if groups != nil { + t.Error("Group list should be nil") + } + if !strings.Contains(err.Error(), "Overage claim (users with more than 200 group membership) for SPN is currently not supported") { + t.Errorf("Expected: Overage claim for SPN error, Got: %s", err.Error()) + } + }) + + t.Run("non-arc auth mode calls graph servicePrincipals", func(t *testing.T) { + var gotPath, gotMethod string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath, gotMethod = r.URL.Path, r.Method + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"value": ["g1", "g2", "g3"]}`)) + })) + defer ts.Close() + + apiURL, _ := url.Parse(ts.URL + "/v1.0") + u := &UserInfo{ + client: httpclient.DefaultHTTPClient, + apiURL: apiURL, + headers: http.Header{}, + expires: time.Now().Add(time.Hour), + groupsPerCall: expandedGroupsPerCall, + useGroupUID: true, + } + + groups, err := u.GetGroups(ctx, "spn-oid-123", "", true) + if err != nil { + t.Fatalf("Should not have gotten error: %s", err) + } + if gotMethod != http.MethodPost { + t.Errorf("Expected POST, got %s", gotMethod) + } + if gotPath != "/v1.0/servicePrincipals/spn-oid-123/getMemberGroups" { + t.Errorf("Unexpected request path: %s", gotPath) + } + if len(groups) != 3 { + t.Fatalf("Should have gotten a list of groups with 3 entries. Got: %d", len(groups)) + } + for i, want := range []string{"g1", "g2", "g3"} { + if groups[i] != want { + t.Errorf("Expected group %s at index %d, got %s", want, i, groups[i]) + } + } + }) + + t.Run("bad server response", func(t *testing.T) { + ts, u := getAPIServerAndUserInfo(http.StatusInternalServerError, "shutdown") + defer ts.Close() + + groups, err := u.getGroupIDs(ctx, "spn-oid-123", true) + if err == nil { + t.Fatal("Should have gotten error") + } + if groups != nil { + t.Error("Group list should be nil") + } + if !strings.Contains(err.Error(), "failed with status code: 500") { + t.Errorf("Expected status code in error, got: %s", err.Error()) + } + }) + + t.Run("request error", func(t *testing.T) { + badURL, _ := url.Parse("https://127.0.0.1:34567") + u := &UserInfo{ + client: httpclient.DefaultHTTPClient, + apiURL: badURL, + headers: http.Header{}, + expires: time.Now().Add(time.Hour), + groupsPerCall: expandedGroupsPerCall, + } + + groups, err := u.getGroupIDs(ctx, "spn-oid-123", true) + if err == nil { + t.Fatal("Should have gotten error") + } + if groups != nil { + t.Error("Group list should be nil") + } + }) + + t.Run("bad response body", func(t *testing.T) { + ts, u := getAPIServerAndUserInfo(http.StatusOK, "{bad_json") + defer ts.Close() + + groups, err := u.getGroupIDs(ctx, "spn-oid-123", true) + if err == nil { + t.Fatal("Should have gotten error") + } + if groups != nil { + t.Error("Group list should be nil") + } + if !strings.Contains(err.Error(), "failed to decode response for request") { + t.Errorf("Expected decode error, got: %s", err.Error()) + } + }) +} + // This is only testing the full function run, error cases are handled in the tests above func TestGetGroups(t *testing.T) { ctx := context.Background() @@ -531,7 +661,7 @@ func TestGetGroups(t *testing.T) { } defer ts.Close() - groups, err := u.GetGroups(ctx, "blackbriar@cia.gov", "") + groups, err := u.GetGroups(ctx, "blackbriar@cia.gov", "", false) if err != nil { t.Errorf("Should not have gotten error: %s", err) } @@ -549,7 +679,7 @@ func TestGetGroups(t *testing.T) { } defer ts.Close() - groups, err = uWithGroupID.GetGroups(ctx, "blackbriar@cia.gov", "") + groups, err = uWithGroupID.GetGroups(ctx, "blackbriar@cia.gov", "", false) if err != nil { t.Errorf("Should not have gotten error: %s", err) } @@ -625,7 +755,7 @@ func TestGetGroupsPaging(t *testing.T) { defer ts.Close() ctx := context.Background() - groups, err := u.GetGroups(ctx, "blackbriar@cia.gov", "") + groups, err := u.GetGroups(ctx, "blackbriar@cia.gov", "", false) if err != nil { t.Errorf("Should not have gotten error: %s", err) } diff --git a/auth/providers/azure/options.go b/auth/providers/azure/options.go index 07c05fbe..2f46ecb0 100644 --- a/auth/providers/azure/options.go +++ b/auth/providers/azure/options.go @@ -52,6 +52,7 @@ type Options struct { PoPTokenValidityDuration time.Duration ResolveGroupMembershipOnlyOnOverageClaim bool SkipGroupMembershipResolution bool + EnableSPGroupResolution bool VerifyClientID bool ResourceId string AzureRegion string @@ -80,6 +81,7 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) { fs.BoolVar(&o.ResolveGroupMembershipOnlyOnOverageClaim, "azure.graph-call-on-overage-claim", o.ResolveGroupMembershipOnlyOnOverageClaim, "set to true to resolve group membership only when overage claim is present. setting to false will always call graph api to resolve group membership") fs.BoolVar(&o.VerifyClientID, "azure.verify-clientID", o.VerifyClientID, "set to true to validate token's audience claim matches clientID") fs.BoolVar(&o.SkipGroupMembershipResolution, "azure.skip-group-membership-resolution", false, "when set to true, this will bypass getting group membership from graph api") + fs.BoolVar(&o.EnableSPGroupResolution, "azure.enable-sp-group-resolution", false, "when set to true, group membership of service principal (application) tokens is resolved via the servicePrincipals graph endpoint. when false, service principal tokens carrying an overage claim are rejected") // resource id and region are needed to retrieve user's security group info via Arc obo service fs.StringVar(&o.ResourceId, "azure.auth-resource-id", "", "azure cluster resource id (//subscription//resourcegroups//providers/Microsoft.Kubernetes/connectedClusters/ for connectedk8s) used for making getMemberGroups to ARC OBO service") fs.StringVar(&o.AzureRegion, "azure.region", "", "region where cluster is deployed")