From 715b7113cc8106fdb453b25a3b77aa0b79857f52 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Tue, 17 Dec 2024 16:06:54 -0600 Subject: [PATCH 01/41] Protobuf and configuration for Access Graph Azure Discovery --- api/types/types.pb.go | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/api/types/types.pb.go b/api/types/types.pb.go index a49edaacc6d5b..baccdfd1835f1 100644 --- a/api/types/types.pb.go +++ b/api/types/types.pb.go @@ -50870,6 +50870,47 @@ func (m *AccessGraphAzureSync) MarshalTo(dAtA []byte) (int, error) { return m.MarshalToSizedBuffer(dAtA[:size]) } +func (m *AccessGraphAzureSync) MarshalToSizedBuffer(dAtA []byte) (int, error) { + i := len(dAtA) + _ = i + var l int + _ = l + if m.XXX_unrecognized != nil { + i -= len(m.XXX_unrecognized) + copy(dAtA[i:], m.XXX_unrecognized) + } + if len(m.Integration) > 0 { + i -= len(m.Integration) + copy(dAtA[i:], m.Integration) + i = encodeVarintTypes(dAtA, i, uint64(len(m.Integration))) + i-- + dAtA[i] = 0x1a + } + if len(m.SubscriptionID) > 0 { + i -= len(m.SubscriptionID) + copy(dAtA[i:], m.SubscriptionID) + i = encodeVarintTypes(dAtA, i, uint64(len(m.SubscriptionID))) + i-- + dAtA[i] = 0x12 + } + return len(dAtA) - i, nil +} + +func (m *AccessGraphAzureSync) Marshal() (dAtA []byte, err error) { + size := m.Size() + dAtA = make([]byte, size) + n, err := m.MarshalToSizedBuffer(dAtA[:size]) + if err != nil { + return nil, err + } + return dAtA[:n], nil +} + +func (m *AccessGraphAzureSync) MarshalTo(dAtA []byte) (int, error) { + size := m.Size() + return m.MarshalToSizedBuffer(dAtA[:size]) +} + func (m *AccessGraphAzureSync) MarshalToSizedBuffer(dAtA []byte) (int, error) { i := len(dAtA) _ = i From b377148cd89fa99b530eee789d19760e3a7d5a4c Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Tue, 17 Dec 2024 16:49:05 -0600 Subject: [PATCH 02/41] Adding the Azure sync module functions along with new cloud client functionality --- .../fetchers/azure-sync/msggraphclient.go | 240 ++++++++++++++++++ .../fetchers/azure-sync/reconcile.go | 165 ++++++++++++ .../fetchers/azure-sync/reconcile_test.go | 191 ++++++++++++++ 3 files changed, 596 insertions(+) create mode 100644 lib/srv/discovery/fetchers/azure-sync/msggraphclient.go create mode 100644 lib/srv/discovery/fetchers/azure-sync/reconcile.go create mode 100644 lib/srv/discovery/fetchers/azure-sync/reconcile_test.go diff --git a/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go b/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go new file mode 100644 index 0000000000000..75d2960d7fa55 --- /dev/null +++ b/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go @@ -0,0 +1,240 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package azure_sync + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" +) + +// GraphClient represents generic MS API client +type GraphClient struct { + token azcore.AccessToken +} + +const ( + usersSuffix = "users" + groupsSuffix = "groups" + servicePrincipalsSuffix = "servicePrincipals" + graphBaseURL = "https://graph.microsoft.com/v1.0" + httpTimeout = time.Second * 30 +) + +// graphError represents MS Graph error +type graphError struct { + E struct { + Code string `json:"code"` + Message string `json:"message"` + } `json:"error"` +} + +// genericGraphResponse represents the utility struct for parsing MS Graph API response +type genericGraphResponse struct { + Context string `json:"@odata.context"` + Count int `json:"@odata.count"` + NextLink string `json:"@odata.nextLink"` + Value json.RawMessage `json:"value"` +} + +// User represents user resource +type User struct { + ID string `json:"id"` + Name string `json:"displayName"` + MemberOf []Membership `json:"memberOf"` +} + +type Membership struct { + Type string `json:"@odata.type"` + ID string `json:"id"` +} + +// request represents generic request structure +type request struct { + // Method HTTP method + Method string + // URL which overrides URL construction + URL *string + // Path to a resource + Path string + // Expand $expand value + Expand []string + // Filter $filter value + Filter string + // Body request body + Body string + // Response represents template structure for a response + Response interface{} + // Err represents template structure for an error + Err error + // SuccessCode http code representing success + SuccessCode int +} + +// GetURL builds the request URL +func (r *request) GetURL() (string, error) { + if r.URL != nil { + return *r.URL, nil + } + u, err := url.Parse(graphBaseURL) + if err != nil { + return "", err + } + + data := url.Values{} + if len(r.Expand) > 0 { + data.Set("$expand", strings.Join(r.Expand, ",")) + } + if r.Filter != "" { + data.Set("$filter", r.Filter) + } + + u.Path = u.Path + "/" + r.Path + u.RawQuery = data.Encode() + + return u.String(), nil +} + +// NewGraphClient creates MS Graph API client +func NewGraphClient(token azcore.AccessToken) *GraphClient { + return &GraphClient{ + token: token, + } +} + +// Error returns error string +func (e graphError) Error() string { + return e.E.Code + " " + e.E.Message +} + +func (c *GraphClient) ListUsers(ctx context.Context) ([]User, error) { + return c.listIdentities(ctx, usersSuffix, []string{"memberOf"}) +} + +func (c *GraphClient) ListGroups(ctx context.Context) ([]User, error) { + return c.listIdentities(ctx, groupsSuffix, []string{"memberOf"}) +} + +func (c *GraphClient) ListServicePrincipals(ctx context.Context) ([]User, error) { + return c.listIdentities(ctx, servicePrincipalsSuffix, []string{"memberOf"}) +} + +func (c *GraphClient) listIdentities(ctx context.Context, idType string, expand []string) ([]User, error) { + var users []User + var nextLink *string + for { + g := &genericGraphResponse{} + req := request{ + Method: http.MethodGet, + Path: idType, + Expand: expand, + Response: &g, + Err: &graphError{}, + URL: nextLink, + } + err := c.request(ctx, req) + if err != nil { + return nil, err + } + var newUsers []User + err = json.NewDecoder(bytes.NewReader(g.Value)).Decode(&newUsers) + if err != nil { + return nil, err + } + users = append(users, newUsers...) + if g.NextLink == "" { + break + } + nextLink = &g.NextLink + } + + return users, nil +} + +// request sends the request to the graph/bot service and returns response body as bytes slice +func (c *GraphClient) request(ctx context.Context, req request) error { + reqUrl, err := req.GetURL() + if err != nil { + return err + } + + r, err := http.NewRequestWithContext(ctx, req.Method, reqUrl, strings.NewReader(req.Body)) + if err != nil { + return err + } + + r.Header.Set("Authorization", "Bearer "+c.token.Token) + r.Header.Set("Content-Type", "application/json") + + client := http.Client{Timeout: httpTimeout} + resp, err := client.Do(r) + if err != nil { + return err + } + + defer func(r *http.Response) { + _ = r.Body.Close() + }(resp) + + b, err := io.ReadAll(resp.Body) + if err != nil { + return err + } + + expectedCode := req.SuccessCode + if expectedCode == 0 { + expectedCode = http.StatusOK + } + + if expectedCode == resp.StatusCode { + if req.Response == nil { + return nil + } + + err := json.NewDecoder(bytes.NewReader(b)).Decode(req.Response) + if err != nil { + return err + } + } else { + if req.Err == nil { + return fmt.Errorf("Error requesting MS Graph API: %v", string(b)) + } + + err := json.NewDecoder(bytes.NewReader(b)).Decode(req.Err) + if err != nil { + return err + } + + if req.Err.Error() == "" { + return fmt.Errorf("Error requesting MS Graph API. Expected response code was %v, but is %v", expectedCode, resp.StatusCode) + } + + return req.Err + } + + return nil +} diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile.go b/lib/srv/discovery/fetchers/azure-sync/reconcile.go new file mode 100644 index 0000000000000..2b54c8cfac911 --- /dev/null +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile.go @@ -0,0 +1,165 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package azure_sync + +import ( + "fmt" + + "google.golang.org/protobuf/proto" + + accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" + "github.com/gravitational/teleport/lib/srv/discovery/common" +) + +// MergeResources merges Azure resources fetched from multiple configured Azure fetchers +func MergeResources(results ...*Resources) *Resources { + if len(results) == 0 { + return &Resources{} + } + if len(results) == 1 { + return results[0] + } + result := &Resources{} + for _, r := range results { + result.Principals = append(result.Principals, r.Principals...) + result.RoleAssignments = append(result.RoleAssignments, r.RoleAssignments...) + result.RoleDefinitions = append(result.RoleDefinitions, r.RoleDefinitions...) + result.VirtualMachines = append(result.VirtualMachines, r.VirtualMachines...) + } + result.Principals = common.DeduplicateSlice(result.Principals, azurePrincipalsKey) + result.RoleAssignments = common.DeduplicateSlice(result.RoleAssignments, azureRoleAssignKey) + result.RoleDefinitions = common.DeduplicateSlice(result.RoleDefinitions, azureRoleDefKey) + result.VirtualMachines = common.DeduplicateSlice(result.VirtualMachines, azureVmKey) + return result +} + +// newResourceList creates a new resource list message +func newResourceList() *accessgraphv1alpha.AzureResourceList { + return &accessgraphv1alpha.AzureResourceList{ + Resources: make([]*accessgraphv1alpha.AzureResource, 0), + } +} + +// ReconcileResults compares previously and currently fetched results and determines which resources to upsert and +// which to delete. +func ReconcileResults(old *Resources, new *Resources) (upsert, delete *accessgraphv1alpha.AzureResourceList) { + upsert, delete = newResourceList(), newResourceList() + reconciledResources := []*reconcilePair{ + reconcile(old.Principals, new.Principals, azurePrincipalsKey, azurePrincipalsWrap), + reconcile(old.RoleAssignments, new.RoleAssignments, azureRoleAssignKey, azureRoleAssignWrap), + reconcile(old.RoleDefinitions, new.RoleDefinitions, azureRoleDefKey, azureRoleDefWrap), + reconcile(old.VirtualMachines, new.VirtualMachines, azureVmKey, azureVmWrap), + } + for _, res := range reconciledResources { + upsert.Resources = append(upsert.Resources, res.upsert.Resources...) + delete.Resources = append(delete.Resources, res.delete.Resources...) + } + return upsert, delete +} + +// reconcilePair contains the Azure resources to upsert and delete +type reconcilePair struct { + upsert, delete *accessgraphv1alpha.AzureResourceList +} + +// reconcile compares old and new items to build a list of resources to upsert and delete in the Access Graph +func reconcile[T proto.Message]( + oldItems []T, + newItems []T, + keyFn func(T) string, + wrapFn func(T) *accessgraphv1alpha.AzureResource, +) *reconcilePair { + // Remove duplicates from the new items + newItems = common.DeduplicateSlice(newItems, keyFn) + upsertRes := newResourceList() + deleteRes := newResourceList() + + // Delete all old items if there are no new items + if len(newItems) == 0 { + for _, item := range oldItems { + deleteRes.Resources = append(deleteRes.Resources, wrapFn(item)) + } + return &reconcilePair{upsertRes, deleteRes} + } + + // Create all new items if there are no old items + if len(oldItems) == 0 { + for _, item := range newItems { + upsertRes.Resources = append(upsertRes.Resources, wrapFn(item)) + } + return &reconcilePair{upsertRes, deleteRes} + } + + // Map old and new items by their key + oldMap := make(map[string]T, len(oldItems)) + for _, item := range oldItems { + oldMap[keyFn(item)] = item + } + newMap := make(map[string]T, len(newItems)) + for _, item := range newItems { + newMap[keyFn(item)] = item + } + + // Append new or modified items to the upsert list + for _, item := range newItems { + if oldItem, ok := oldMap[keyFn(item)]; !ok || !proto.Equal(oldItem, item) { + upsertRes.Resources = append(upsertRes.Resources, wrapFn(item)) + } + } + + // Append removed items to the delete list + for _, item := range oldItems { + if _, ok := newMap[keyFn(item)]; !ok { + deleteRes.Resources = append(deleteRes.Resources, wrapFn(item)) + } + } + return &reconcilePair{upsertRes, deleteRes} +} + +func azurePrincipalsKey(user *accessgraphv1alpha.AzurePrincipal) string { + return fmt.Sprintf("%s:%s", user.SubscriptionId, user.Id) +} + +func azurePrincipalsWrap(principal *accessgraphv1alpha.AzurePrincipal) *accessgraphv1alpha.AzureResource { + return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_Principal{Principal: principal}} +} + +func azureRoleAssignKey(roleAssign *accessgraphv1alpha.AzureRoleAssignment) string { + return fmt.Sprintf("%s:%s", roleAssign.SubscriptionId, roleAssign.Id) +} + +func azureRoleAssignWrap(roleAssign *accessgraphv1alpha.AzureRoleAssignment) *accessgraphv1alpha.AzureResource { + return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_RoleAssignment{RoleAssignment: roleAssign}} +} + +func azureRoleDefKey(roleDef *accessgraphv1alpha.AzureRoleDefinition) string { + return fmt.Sprintf("%s:%s", roleDef.SubscriptionId, roleDef.Id) +} + +func azureRoleDefWrap(roleDef *accessgraphv1alpha.AzureRoleDefinition) *accessgraphv1alpha.AzureResource { + return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_RoleDefinition{RoleDefinition: roleDef}} +} + +func azureVmKey(vm *accessgraphv1alpha.AzureVirtualMachine) string { + return fmt.Sprintf("%s:%s", vm.SubscriptionId, vm.Id) +} + +func azureVmWrap(vm *accessgraphv1alpha.AzureVirtualMachine) *accessgraphv1alpha.AzureResource { + return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_VirtualMachine{VirtualMachine: vm}} +} diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go new file mode 100644 index 0000000000000..28b293bcf1f8d --- /dev/null +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go @@ -0,0 +1,191 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package azure_sync + +import ( + "testing" + + "github.com/stretchr/testify/require" + + accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" +) + +func TestReconcileResults(t *testing.T) { + principals := generatePrincipals() + roleDefs := generateRoleDefs() + roleAssigns := generateRoleAssigns() + vms := generateVms() + + tests := []struct { + oldResults *Resources + newResults *Resources + expectedUpserts *accessgraphv1alpha.AzureResourceList + expectedDeletes *accessgraphv1alpha.AzureResourceList + }{ + // Overlapping old and new results + { + oldResults: &Resources{ + Principals: principals[0:2], + RoleDefinitions: roleDefs[0:2], + RoleAssignments: roleAssigns[0:2], + VirtualMachines: vms[0:2], + }, + newResults: &Resources{ + Principals: principals[1:3], + RoleDefinitions: roleDefs[1:3], + RoleAssignments: roleAssigns[1:3], + VirtualMachines: vms[1:3], + }, + expectedUpserts: generateExpected(principals[2:3], roleDefs[2:3], roleAssigns[2:3], vms[2:3]), + expectedDeletes: generateExpected(principals[0:1], roleDefs[0:1], roleAssigns[0:1], vms[0:1]), + }, + // Completely new results + { + oldResults: &Resources{ + Principals: nil, + RoleDefinitions: nil, + RoleAssignments: nil, + VirtualMachines: nil, + }, + newResults: &Resources{ + Principals: principals[1:3], + RoleDefinitions: roleDefs[1:3], + RoleAssignments: roleAssigns[1:3], + VirtualMachines: vms[1:3], + }, + expectedUpserts: generateExpected(principals[1:3], roleDefs[1:3], roleAssigns[1:3], vms[1:3]), + expectedDeletes: generateExpected(nil, nil, nil, nil), + }, + // No new results + { + oldResults: &Resources{ + Principals: principals[1:3], + RoleDefinitions: roleDefs[1:3], + RoleAssignments: roleAssigns[1:3], + VirtualMachines: vms[1:3], + }, + newResults: &Resources{ + Principals: nil, + RoleDefinitions: nil, + RoleAssignments: nil, + VirtualMachines: nil, + }, + expectedUpserts: generateExpected(nil, nil, nil, nil), + expectedDeletes: generateExpected(principals[1:3], roleDefs[1:3], roleAssigns[1:3], vms[1:3]), + }, + } + + for _, tt := range tests { + upserts, deletes := ReconcileResults(tt.oldResults, tt.newResults) + require.ElementsMatch(t, upserts.Resources, tt.expectedUpserts.Resources) + require.ElementsMatch(t, deletes.Resources, tt.expectedDeletes.Resources) + } + +} + +func generateExpected( + principals []*accessgraphv1alpha.AzurePrincipal, + roleDefs []*accessgraphv1alpha.AzureRoleDefinition, + roleAssigns []*accessgraphv1alpha.AzureRoleAssignment, + vms []*accessgraphv1alpha.AzureVirtualMachine, +) *accessgraphv1alpha.AzureResourceList { + resList := &accessgraphv1alpha.AzureResourceList{ + Resources: make([]*accessgraphv1alpha.AzureResource, 0), + } + for _, principal := range principals { + resList.Resources = append(resList.Resources, azurePrincipalsWrap(principal)) + } + for _, roleDef := range roleDefs { + resList.Resources = append(resList.Resources, azureRoleDefWrap(roleDef)) + } + for _, roleAssign := range roleAssigns { + resList.Resources = append(resList.Resources, azureRoleAssignWrap(roleAssign)) + } + for _, vm := range vms { + resList.Resources = append(resList.Resources, azureVmWrap(vm)) + } + return resList +} + +func generatePrincipals() []*accessgraphv1alpha.AzurePrincipal { + return []*accessgraphv1alpha.AzurePrincipal{ + { + Id: "/principals/foo", + DisplayName: "userFoo", + }, + { + Id: "/principals/bar", + DisplayName: "userBar", + }, + { + Id: "/principals/charles", + DisplayName: "userCharles", + }, + } +} + +func generateRoleDefs() []*accessgraphv1alpha.AzureRoleDefinition { + return []*accessgraphv1alpha.AzureRoleDefinition{ + { + Id: "/roledefinitions/foo", + Name: "roleFoo", + }, + { + Id: "/roledefinitions/bar", + Name: "roleBar", + }, + { + Id: "/roledefinitions/charles", + Name: "roleCharles", + }, + } +} + +func generateRoleAssigns() []*accessgraphv1alpha.AzureRoleAssignment { + return []*accessgraphv1alpha.AzureRoleAssignment{ + { + Id: "/roleassignments/foo", + PrincipalId: "userFoo", + }, + { + Id: "/roleassignments/bar", + PrincipalId: "userBar", + }, + { + Id: "/roleassignments/charles", + PrincipalId: "userCharles", + }, + } +} + +func generateVms() []*accessgraphv1alpha.AzureVirtualMachine { + return []*accessgraphv1alpha.AzureVirtualMachine{ + { + Id: "/vms/foo", + Name: "userFoo", + }, + { + Id: "/vms/bar", + Name: "userBar", + }, + { + Id: "/vms/charles", + Name: "userCharles", + }, + } +} From 4399b981f4d4ab173fdd8d11ca612ba8084c0324 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Tue, 17 Dec 2024 17:12:53 -0600 Subject: [PATCH 03/41] Moving reconciliation to the upstream azure sync PR --- .../fetchers/azure-sync/reconcile.go | 165 ------------------ 1 file changed, 165 deletions(-) delete mode 100644 lib/srv/discovery/fetchers/azure-sync/reconcile.go diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile.go b/lib/srv/discovery/fetchers/azure-sync/reconcile.go deleted file mode 100644 index 2b54c8cfac911..0000000000000 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile.go +++ /dev/null @@ -1,165 +0,0 @@ -/* - * Teleport - * Copyright (C) 2024 Gravitational, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -package azure_sync - -import ( - "fmt" - - "google.golang.org/protobuf/proto" - - accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" - "github.com/gravitational/teleport/lib/srv/discovery/common" -) - -// MergeResources merges Azure resources fetched from multiple configured Azure fetchers -func MergeResources(results ...*Resources) *Resources { - if len(results) == 0 { - return &Resources{} - } - if len(results) == 1 { - return results[0] - } - result := &Resources{} - for _, r := range results { - result.Principals = append(result.Principals, r.Principals...) - result.RoleAssignments = append(result.RoleAssignments, r.RoleAssignments...) - result.RoleDefinitions = append(result.RoleDefinitions, r.RoleDefinitions...) - result.VirtualMachines = append(result.VirtualMachines, r.VirtualMachines...) - } - result.Principals = common.DeduplicateSlice(result.Principals, azurePrincipalsKey) - result.RoleAssignments = common.DeduplicateSlice(result.RoleAssignments, azureRoleAssignKey) - result.RoleDefinitions = common.DeduplicateSlice(result.RoleDefinitions, azureRoleDefKey) - result.VirtualMachines = common.DeduplicateSlice(result.VirtualMachines, azureVmKey) - return result -} - -// newResourceList creates a new resource list message -func newResourceList() *accessgraphv1alpha.AzureResourceList { - return &accessgraphv1alpha.AzureResourceList{ - Resources: make([]*accessgraphv1alpha.AzureResource, 0), - } -} - -// ReconcileResults compares previously and currently fetched results and determines which resources to upsert and -// which to delete. -func ReconcileResults(old *Resources, new *Resources) (upsert, delete *accessgraphv1alpha.AzureResourceList) { - upsert, delete = newResourceList(), newResourceList() - reconciledResources := []*reconcilePair{ - reconcile(old.Principals, new.Principals, azurePrincipalsKey, azurePrincipalsWrap), - reconcile(old.RoleAssignments, new.RoleAssignments, azureRoleAssignKey, azureRoleAssignWrap), - reconcile(old.RoleDefinitions, new.RoleDefinitions, azureRoleDefKey, azureRoleDefWrap), - reconcile(old.VirtualMachines, new.VirtualMachines, azureVmKey, azureVmWrap), - } - for _, res := range reconciledResources { - upsert.Resources = append(upsert.Resources, res.upsert.Resources...) - delete.Resources = append(delete.Resources, res.delete.Resources...) - } - return upsert, delete -} - -// reconcilePair contains the Azure resources to upsert and delete -type reconcilePair struct { - upsert, delete *accessgraphv1alpha.AzureResourceList -} - -// reconcile compares old and new items to build a list of resources to upsert and delete in the Access Graph -func reconcile[T proto.Message]( - oldItems []T, - newItems []T, - keyFn func(T) string, - wrapFn func(T) *accessgraphv1alpha.AzureResource, -) *reconcilePair { - // Remove duplicates from the new items - newItems = common.DeduplicateSlice(newItems, keyFn) - upsertRes := newResourceList() - deleteRes := newResourceList() - - // Delete all old items if there are no new items - if len(newItems) == 0 { - for _, item := range oldItems { - deleteRes.Resources = append(deleteRes.Resources, wrapFn(item)) - } - return &reconcilePair{upsertRes, deleteRes} - } - - // Create all new items if there are no old items - if len(oldItems) == 0 { - for _, item := range newItems { - upsertRes.Resources = append(upsertRes.Resources, wrapFn(item)) - } - return &reconcilePair{upsertRes, deleteRes} - } - - // Map old and new items by their key - oldMap := make(map[string]T, len(oldItems)) - for _, item := range oldItems { - oldMap[keyFn(item)] = item - } - newMap := make(map[string]T, len(newItems)) - for _, item := range newItems { - newMap[keyFn(item)] = item - } - - // Append new or modified items to the upsert list - for _, item := range newItems { - if oldItem, ok := oldMap[keyFn(item)]; !ok || !proto.Equal(oldItem, item) { - upsertRes.Resources = append(upsertRes.Resources, wrapFn(item)) - } - } - - // Append removed items to the delete list - for _, item := range oldItems { - if _, ok := newMap[keyFn(item)]; !ok { - deleteRes.Resources = append(deleteRes.Resources, wrapFn(item)) - } - } - return &reconcilePair{upsertRes, deleteRes} -} - -func azurePrincipalsKey(user *accessgraphv1alpha.AzurePrincipal) string { - return fmt.Sprintf("%s:%s", user.SubscriptionId, user.Id) -} - -func azurePrincipalsWrap(principal *accessgraphv1alpha.AzurePrincipal) *accessgraphv1alpha.AzureResource { - return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_Principal{Principal: principal}} -} - -func azureRoleAssignKey(roleAssign *accessgraphv1alpha.AzureRoleAssignment) string { - return fmt.Sprintf("%s:%s", roleAssign.SubscriptionId, roleAssign.Id) -} - -func azureRoleAssignWrap(roleAssign *accessgraphv1alpha.AzureRoleAssignment) *accessgraphv1alpha.AzureResource { - return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_RoleAssignment{RoleAssignment: roleAssign}} -} - -func azureRoleDefKey(roleDef *accessgraphv1alpha.AzureRoleDefinition) string { - return fmt.Sprintf("%s:%s", roleDef.SubscriptionId, roleDef.Id) -} - -func azureRoleDefWrap(roleDef *accessgraphv1alpha.AzureRoleDefinition) *accessgraphv1alpha.AzureResource { - return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_RoleDefinition{RoleDefinition: roleDef}} -} - -func azureVmKey(vm *accessgraphv1alpha.AzureVirtualMachine) string { - return fmt.Sprintf("%s:%s", vm.SubscriptionId, vm.Id) -} - -func azureVmWrap(vm *accessgraphv1alpha.AzureVirtualMachine) *accessgraphv1alpha.AzureResource { - return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_VirtualMachine{VirtualMachine: vm}} -} From d3c66e6d4554f5a94e350d29accad601b46ef00b Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Tue, 17 Dec 2024 17:15:42 -0600 Subject: [PATCH 04/41] Moving reconciliation test to the upstream azure sync PR --- .../fetchers/azure-sync/reconcile_test.go | 191 ------------------ 1 file changed, 191 deletions(-) delete mode 100644 lib/srv/discovery/fetchers/azure-sync/reconcile_test.go diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go deleted file mode 100644 index 28b293bcf1f8d..0000000000000 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go +++ /dev/null @@ -1,191 +0,0 @@ -/* - * Teleport - * Copyright (C) 2024 Gravitational, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ -package azure_sync - -import ( - "testing" - - "github.com/stretchr/testify/require" - - accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" -) - -func TestReconcileResults(t *testing.T) { - principals := generatePrincipals() - roleDefs := generateRoleDefs() - roleAssigns := generateRoleAssigns() - vms := generateVms() - - tests := []struct { - oldResults *Resources - newResults *Resources - expectedUpserts *accessgraphv1alpha.AzureResourceList - expectedDeletes *accessgraphv1alpha.AzureResourceList - }{ - // Overlapping old and new results - { - oldResults: &Resources{ - Principals: principals[0:2], - RoleDefinitions: roleDefs[0:2], - RoleAssignments: roleAssigns[0:2], - VirtualMachines: vms[0:2], - }, - newResults: &Resources{ - Principals: principals[1:3], - RoleDefinitions: roleDefs[1:3], - RoleAssignments: roleAssigns[1:3], - VirtualMachines: vms[1:3], - }, - expectedUpserts: generateExpected(principals[2:3], roleDefs[2:3], roleAssigns[2:3], vms[2:3]), - expectedDeletes: generateExpected(principals[0:1], roleDefs[0:1], roleAssigns[0:1], vms[0:1]), - }, - // Completely new results - { - oldResults: &Resources{ - Principals: nil, - RoleDefinitions: nil, - RoleAssignments: nil, - VirtualMachines: nil, - }, - newResults: &Resources{ - Principals: principals[1:3], - RoleDefinitions: roleDefs[1:3], - RoleAssignments: roleAssigns[1:3], - VirtualMachines: vms[1:3], - }, - expectedUpserts: generateExpected(principals[1:3], roleDefs[1:3], roleAssigns[1:3], vms[1:3]), - expectedDeletes: generateExpected(nil, nil, nil, nil), - }, - // No new results - { - oldResults: &Resources{ - Principals: principals[1:3], - RoleDefinitions: roleDefs[1:3], - RoleAssignments: roleAssigns[1:3], - VirtualMachines: vms[1:3], - }, - newResults: &Resources{ - Principals: nil, - RoleDefinitions: nil, - RoleAssignments: nil, - VirtualMachines: nil, - }, - expectedUpserts: generateExpected(nil, nil, nil, nil), - expectedDeletes: generateExpected(principals[1:3], roleDefs[1:3], roleAssigns[1:3], vms[1:3]), - }, - } - - for _, tt := range tests { - upserts, deletes := ReconcileResults(tt.oldResults, tt.newResults) - require.ElementsMatch(t, upserts.Resources, tt.expectedUpserts.Resources) - require.ElementsMatch(t, deletes.Resources, tt.expectedDeletes.Resources) - } - -} - -func generateExpected( - principals []*accessgraphv1alpha.AzurePrincipal, - roleDefs []*accessgraphv1alpha.AzureRoleDefinition, - roleAssigns []*accessgraphv1alpha.AzureRoleAssignment, - vms []*accessgraphv1alpha.AzureVirtualMachine, -) *accessgraphv1alpha.AzureResourceList { - resList := &accessgraphv1alpha.AzureResourceList{ - Resources: make([]*accessgraphv1alpha.AzureResource, 0), - } - for _, principal := range principals { - resList.Resources = append(resList.Resources, azurePrincipalsWrap(principal)) - } - for _, roleDef := range roleDefs { - resList.Resources = append(resList.Resources, azureRoleDefWrap(roleDef)) - } - for _, roleAssign := range roleAssigns { - resList.Resources = append(resList.Resources, azureRoleAssignWrap(roleAssign)) - } - for _, vm := range vms { - resList.Resources = append(resList.Resources, azureVmWrap(vm)) - } - return resList -} - -func generatePrincipals() []*accessgraphv1alpha.AzurePrincipal { - return []*accessgraphv1alpha.AzurePrincipal{ - { - Id: "/principals/foo", - DisplayName: "userFoo", - }, - { - Id: "/principals/bar", - DisplayName: "userBar", - }, - { - Id: "/principals/charles", - DisplayName: "userCharles", - }, - } -} - -func generateRoleDefs() []*accessgraphv1alpha.AzureRoleDefinition { - return []*accessgraphv1alpha.AzureRoleDefinition{ - { - Id: "/roledefinitions/foo", - Name: "roleFoo", - }, - { - Id: "/roledefinitions/bar", - Name: "roleBar", - }, - { - Id: "/roledefinitions/charles", - Name: "roleCharles", - }, - } -} - -func generateRoleAssigns() []*accessgraphv1alpha.AzureRoleAssignment { - return []*accessgraphv1alpha.AzureRoleAssignment{ - { - Id: "/roleassignments/foo", - PrincipalId: "userFoo", - }, - { - Id: "/roleassignments/bar", - PrincipalId: "userBar", - }, - { - Id: "/roleassignments/charles", - PrincipalId: "userCharles", - }, - } -} - -func generateVms() []*accessgraphv1alpha.AzureVirtualMachine { - return []*accessgraphv1alpha.AzureVirtualMachine{ - { - Id: "/vms/foo", - Name: "userFoo", - }, - { - Id: "/vms/bar", - Name: "userBar", - }, - { - Id: "/vms/charles", - Name: "userCharles", - }, - } -} From 6eee354de970da7de3283049a6c4656ff91ca413 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 18 Dec 2024 15:14:34 -0600 Subject: [PATCH 05/41] Fixing rebase after protobuf gen --- api/types/types.pb.go | 41 ----------------------------------------- 1 file changed, 41 deletions(-) diff --git a/api/types/types.pb.go b/api/types/types.pb.go index baccdfd1835f1..a49edaacc6d5b 100644 --- a/api/types/types.pb.go +++ b/api/types/types.pb.go @@ -50870,47 +50870,6 @@ func (m *AccessGraphAzureSync) MarshalTo(dAtA []byte) (int, error) { return m.MarshalToSizedBuffer(dAtA[:size]) } -func (m *AccessGraphAzureSync) MarshalToSizedBuffer(dAtA []byte) (int, error) { - i := len(dAtA) - _ = i - var l int - _ = l - if m.XXX_unrecognized != nil { - i -= len(m.XXX_unrecognized) - copy(dAtA[i:], m.XXX_unrecognized) - } - if len(m.Integration) > 0 { - i -= len(m.Integration) - copy(dAtA[i:], m.Integration) - i = encodeVarintTypes(dAtA, i, uint64(len(m.Integration))) - i-- - dAtA[i] = 0x1a - } - if len(m.SubscriptionID) > 0 { - i -= len(m.SubscriptionID) - copy(dAtA[i:], m.SubscriptionID) - i = encodeVarintTypes(dAtA, i, uint64(len(m.SubscriptionID))) - i-- - dAtA[i] = 0x12 - } - return len(dAtA) - i, nil -} - -func (m *AccessGraphAzureSync) Marshal() (dAtA []byte, err error) { - size := m.Size() - dAtA = make([]byte, size) - n, err := m.MarshalToSizedBuffer(dAtA[:size]) - if err != nil { - return nil, err - } - return dAtA[:n], nil -} - -func (m *AccessGraphAzureSync) MarshalTo(dAtA []byte) (int, error) { - size := m.Size() - return m.MarshalToSizedBuffer(dAtA[:size]) -} - func (m *AccessGraphAzureSync) MarshalToSizedBuffer(dAtA []byte) (int, error) { i := len(dAtA) _ = i From 5ddb87b2c7396c0059c0082a8eade88b97b8f5ae Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Thu, 19 Dec 2024 17:47:38 -0600 Subject: [PATCH 06/41] Updating to use existing msgraph client --- lib/msgraph/models.go | 10 +- .../fetchers/azure-sync/msggraphclient.go | 240 ------------------ 2 files changed, 8 insertions(+), 242 deletions(-) delete mode 100644 lib/srv/discovery/fetchers/azure-sync/msggraphclient.go diff --git a/lib/msgraph/models.go b/lib/msgraph/models.go index 52c3e97cfec7b..4f2181f81055d 100644 --- a/lib/msgraph/models.go +++ b/lib/msgraph/models.go @@ -28,9 +28,15 @@ type GroupMember interface { isGroupMember() } +type Membership struct { + Type string `json:"@odata.type"` + ID string `json:"id"` +} + type DirectoryObject struct { - ID *string `json:"id,omitempty"` - DisplayName *string `json:"displayName,omitempty"` + ID *string `json:"id,omitempty"` + DisplayName *string `json:"displayName,omitempty"` + MemberOf []Membership `json:"memberOf,omitempty"` } type Group struct { diff --git a/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go b/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go deleted file mode 100644 index 75d2960d7fa55..0000000000000 --- a/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go +++ /dev/null @@ -1,240 +0,0 @@ -/* - * Teleport - * Copyright (C) 2024 Gravitational, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -package azure_sync - -import ( - "bytes" - "context" - "encoding/json" - "fmt" - "io" - "net/http" - "net/url" - "strings" - "time" - - "github.com/Azure/azure-sdk-for-go/sdk/azcore" -) - -// GraphClient represents generic MS API client -type GraphClient struct { - token azcore.AccessToken -} - -const ( - usersSuffix = "users" - groupsSuffix = "groups" - servicePrincipalsSuffix = "servicePrincipals" - graphBaseURL = "https://graph.microsoft.com/v1.0" - httpTimeout = time.Second * 30 -) - -// graphError represents MS Graph error -type graphError struct { - E struct { - Code string `json:"code"` - Message string `json:"message"` - } `json:"error"` -} - -// genericGraphResponse represents the utility struct for parsing MS Graph API response -type genericGraphResponse struct { - Context string `json:"@odata.context"` - Count int `json:"@odata.count"` - NextLink string `json:"@odata.nextLink"` - Value json.RawMessage `json:"value"` -} - -// User represents user resource -type User struct { - ID string `json:"id"` - Name string `json:"displayName"` - MemberOf []Membership `json:"memberOf"` -} - -type Membership struct { - Type string `json:"@odata.type"` - ID string `json:"id"` -} - -// request represents generic request structure -type request struct { - // Method HTTP method - Method string - // URL which overrides URL construction - URL *string - // Path to a resource - Path string - // Expand $expand value - Expand []string - // Filter $filter value - Filter string - // Body request body - Body string - // Response represents template structure for a response - Response interface{} - // Err represents template structure for an error - Err error - // SuccessCode http code representing success - SuccessCode int -} - -// GetURL builds the request URL -func (r *request) GetURL() (string, error) { - if r.URL != nil { - return *r.URL, nil - } - u, err := url.Parse(graphBaseURL) - if err != nil { - return "", err - } - - data := url.Values{} - if len(r.Expand) > 0 { - data.Set("$expand", strings.Join(r.Expand, ",")) - } - if r.Filter != "" { - data.Set("$filter", r.Filter) - } - - u.Path = u.Path + "/" + r.Path - u.RawQuery = data.Encode() - - return u.String(), nil -} - -// NewGraphClient creates MS Graph API client -func NewGraphClient(token azcore.AccessToken) *GraphClient { - return &GraphClient{ - token: token, - } -} - -// Error returns error string -func (e graphError) Error() string { - return e.E.Code + " " + e.E.Message -} - -func (c *GraphClient) ListUsers(ctx context.Context) ([]User, error) { - return c.listIdentities(ctx, usersSuffix, []string{"memberOf"}) -} - -func (c *GraphClient) ListGroups(ctx context.Context) ([]User, error) { - return c.listIdentities(ctx, groupsSuffix, []string{"memberOf"}) -} - -func (c *GraphClient) ListServicePrincipals(ctx context.Context) ([]User, error) { - return c.listIdentities(ctx, servicePrincipalsSuffix, []string{"memberOf"}) -} - -func (c *GraphClient) listIdentities(ctx context.Context, idType string, expand []string) ([]User, error) { - var users []User - var nextLink *string - for { - g := &genericGraphResponse{} - req := request{ - Method: http.MethodGet, - Path: idType, - Expand: expand, - Response: &g, - Err: &graphError{}, - URL: nextLink, - } - err := c.request(ctx, req) - if err != nil { - return nil, err - } - var newUsers []User - err = json.NewDecoder(bytes.NewReader(g.Value)).Decode(&newUsers) - if err != nil { - return nil, err - } - users = append(users, newUsers...) - if g.NextLink == "" { - break - } - nextLink = &g.NextLink - } - - return users, nil -} - -// request sends the request to the graph/bot service and returns response body as bytes slice -func (c *GraphClient) request(ctx context.Context, req request) error { - reqUrl, err := req.GetURL() - if err != nil { - return err - } - - r, err := http.NewRequestWithContext(ctx, req.Method, reqUrl, strings.NewReader(req.Body)) - if err != nil { - return err - } - - r.Header.Set("Authorization", "Bearer "+c.token.Token) - r.Header.Set("Content-Type", "application/json") - - client := http.Client{Timeout: httpTimeout} - resp, err := client.Do(r) - if err != nil { - return err - } - - defer func(r *http.Response) { - _ = r.Body.Close() - }(resp) - - b, err := io.ReadAll(resp.Body) - if err != nil { - return err - } - - expectedCode := req.SuccessCode - if expectedCode == 0 { - expectedCode = http.StatusOK - } - - if expectedCode == resp.StatusCode { - if req.Response == nil { - return nil - } - - err := json.NewDecoder(bytes.NewReader(b)).Decode(req.Response) - if err != nil { - return err - } - } else { - if req.Err == nil { - return fmt.Errorf("Error requesting MS Graph API: %v", string(b)) - } - - err := json.NewDecoder(bytes.NewReader(b)).Decode(req.Err) - if err != nil { - return err - } - - if req.Err.Error() == "" { - return fmt.Errorf("Error requesting MS Graph API. Expected response code was %v, but is %v", expectedCode, resp.StatusCode) - } - - return req.Err - } - - return nil -} From c74277955b2f26549e894d74de4666d23a31125e Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Fri, 20 Dec 2024 11:28:30 -0600 Subject: [PATCH 07/41] PR feedback --- lib/integrations/azureoidc/accessgraph.go | 2 +- lib/msgraph/client_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/integrations/azureoidc/accessgraph.go b/lib/integrations/azureoidc/accessgraph.go index 6cf43a41c3291..d7462f32aca58 100644 --- a/lib/integrations/azureoidc/accessgraph.go +++ b/lib/integrations/azureoidc/accessgraph.go @@ -116,7 +116,7 @@ func CreateTAGCacheFile(ctx context.Context) error { } cache := &TAGInfoCache{} - err = graphClient.IterateApplications(ctx, func(app *msgraph.Application) bool { + err = graphClient.IterateApplications(ctx, nil, func(app *msgraph.Application) bool { appID := app.AppID if appID == nil { slog.WarnContext(ctx, "app ID is nil", "app", app) diff --git a/lib/msgraph/client_test.go b/lib/msgraph/client_test.go index 174b8f924ce14..c302d91cc2f6f 100644 --- a/lib/msgraph/client_test.go +++ b/lib/msgraph/client_test.go @@ -186,7 +186,7 @@ func TestIterateUsers_Empty(t *testing.T) { baseURL: uri, pageSize: defaultPageSize, } - err = client.IterateUsers(context.Background(), func(*User) bool { + err = client.IterateUsers(context.Background(), nil, func(*User) bool { assert.Fail(t, "should never get called") return true }) @@ -215,7 +215,7 @@ func TestIterateUsers(t *testing.T) { } var users []*User - err = client.IterateUsers(context.Background(), func(u *User) bool { + err = client.IterateUsers(context.Background(), nil, func(u *User) bool { users = append(users, u) return true }) From 80318f588c8d001a13fde61fce4583065f9d1d0e Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Mon, 6 Jan 2025 13:25:30 -0600 Subject: [PATCH 08/41] Using variadic options --- lib/integrations/azureoidc/accessgraph.go | 2 +- lib/msgraph/client_test.go | 4 ++-- lib/srv/discovery/fetchers/azure-sync/principals.go | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/integrations/azureoidc/accessgraph.go b/lib/integrations/azureoidc/accessgraph.go index d7462f32aca58..6cf43a41c3291 100644 --- a/lib/integrations/azureoidc/accessgraph.go +++ b/lib/integrations/azureoidc/accessgraph.go @@ -116,7 +116,7 @@ func CreateTAGCacheFile(ctx context.Context) error { } cache := &TAGInfoCache{} - err = graphClient.IterateApplications(ctx, nil, func(app *msgraph.Application) bool { + err = graphClient.IterateApplications(ctx, func(app *msgraph.Application) bool { appID := app.AppID if appID == nil { slog.WarnContext(ctx, "app ID is nil", "app", app) diff --git a/lib/msgraph/client_test.go b/lib/msgraph/client_test.go index c302d91cc2f6f..174b8f924ce14 100644 --- a/lib/msgraph/client_test.go +++ b/lib/msgraph/client_test.go @@ -186,7 +186,7 @@ func TestIterateUsers_Empty(t *testing.T) { baseURL: uri, pageSize: defaultPageSize, } - err = client.IterateUsers(context.Background(), nil, func(*User) bool { + err = client.IterateUsers(context.Background(), func(*User) bool { assert.Fail(t, "should never get called") return true }) @@ -215,7 +215,7 @@ func TestIterateUsers(t *testing.T) { } var users []*User - err = client.IterateUsers(context.Background(), nil, func(u *User) bool { + err = client.IterateUsers(context.Background(), func(u *User) bool { users = append(users, u) return true }) diff --git a/lib/srv/discovery/fetchers/azure-sync/principals.go b/lib/srv/discovery/fetchers/azure-sync/principals.go index 073d6c4713e0c..0f5ad35e88b16 100644 --- a/lib/srv/discovery/fetchers/azure-sync/principals.go +++ b/lib/srv/discovery/fetchers/azure-sync/principals.go @@ -45,7 +45,7 @@ func fetchPrincipals(ctx context.Context, subscriptionID string, cli *msgraph.Cl res := queryResult{metadata: dirObjMetadata{objectType: "user"}, dirObj: user.DirectoryObject} queryResults = append(queryResults, res) return true - }) + }, msgraph.IterateWithExpandMembers()) if err != nil { return nil, trace.Wrap(err) } @@ -53,7 +53,7 @@ func fetchPrincipals(ctx context.Context, subscriptionID string, cli *msgraph.Cl res := queryResult{metadata: dirObjMetadata{objectType: "group"}, dirObj: group.DirectoryObject} queryResults = append(queryResults, res) return true - }) + }, msgraph.IterateWithExpandMembers()) if err != nil { return nil, trace.Wrap(err) } @@ -61,7 +61,7 @@ func fetchPrincipals(ctx context.Context, subscriptionID string, cli *msgraph.Cl res := queryResult{metadata: dirObjMetadata{objectType: "servicePrincipal"}, dirObj: servicePrincipal.DirectoryObject} queryResults = append(queryResults, res) return true - }) + }, msgraph.IterateWithExpandMembers()) if err != nil { return nil, trace.Wrap(err) } From c6281f908ffa898a3ec5ccb14c23702f637fbb0f Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Mon, 6 Jan 2025 13:50:45 -0600 Subject: [PATCH 09/41] Removing memberOf expansion --- lib/msgraph/models.go | 5 ++--- lib/srv/discovery/fetchers/azure-sync/principals.go | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/msgraph/models.go b/lib/msgraph/models.go index 4f2181f81055d..f76d3deaab858 100644 --- a/lib/msgraph/models.go +++ b/lib/msgraph/models.go @@ -34,9 +34,8 @@ type Membership struct { } type DirectoryObject struct { - ID *string `json:"id,omitempty"` - DisplayName *string `json:"displayName,omitempty"` - MemberOf []Membership `json:"memberOf,omitempty"` + ID *string `json:"id,omitempty"` + DisplayName *string `json:"displayName,omitempty"` } type Group struct { diff --git a/lib/srv/discovery/fetchers/azure-sync/principals.go b/lib/srv/discovery/fetchers/azure-sync/principals.go index 0f5ad35e88b16..073d6c4713e0c 100644 --- a/lib/srv/discovery/fetchers/azure-sync/principals.go +++ b/lib/srv/discovery/fetchers/azure-sync/principals.go @@ -45,7 +45,7 @@ func fetchPrincipals(ctx context.Context, subscriptionID string, cli *msgraph.Cl res := queryResult{metadata: dirObjMetadata{objectType: "user"}, dirObj: user.DirectoryObject} queryResults = append(queryResults, res) return true - }, msgraph.IterateWithExpandMembers()) + }) if err != nil { return nil, trace.Wrap(err) } @@ -53,7 +53,7 @@ func fetchPrincipals(ctx context.Context, subscriptionID string, cli *msgraph.Cl res := queryResult{metadata: dirObjMetadata{objectType: "group"}, dirObj: group.DirectoryObject} queryResults = append(queryResults, res) return true - }, msgraph.IterateWithExpandMembers()) + }) if err != nil { return nil, trace.Wrap(err) } @@ -61,7 +61,7 @@ func fetchPrincipals(ctx context.Context, subscriptionID string, cli *msgraph.Cl res := queryResult{metadata: dirObjMetadata{objectType: "servicePrincipal"}, dirObj: servicePrincipal.DirectoryObject} queryResults = append(queryResults, res) return true - }, msgraph.IterateWithExpandMembers()) + }) if err != nil { return nil, trace.Wrap(err) } From c8d95745ec015efb93d1484c89430124273652ed Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Mon, 6 Jan 2025 23:15:04 -0600 Subject: [PATCH 10/41] Expanding memberships by calling memberOf on each user --- lib/msgraph/models.go | 9 +++++++++ lib/msgraph/paginated.go | 23 +++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/lib/msgraph/models.go b/lib/msgraph/models.go index f76d3deaab858..fc2d9a5fee1e1 100644 --- a/lib/msgraph/models.go +++ b/lib/msgraph/models.go @@ -184,3 +184,12 @@ func decodeGroupMember(msg json.RawMessage) (GroupMember, error) { return member, trace.Wrap(err) } + +func decodeDirectoryObject(msg json.RawMessage) (*DirectoryObject, error) { + var d *DirectoryObject + err := json.Unmarshal(msg, &d) + if err != nil { + return nil, trace.Wrap(err) + } + return d, nil +} diff --git a/lib/msgraph/paginated.go b/lib/msgraph/paginated.go index a0b9488af9d70..1e6a915fbe634 100644 --- a/lib/msgraph/paginated.go +++ b/lib/msgraph/paginated.go @@ -109,6 +109,29 @@ func (c *Client) IterateServicePrincipals(ctx context.Context, f func(principal return iterateSimple(c, ctx, "servicePrincipals", f) } +func (c *Client) IterateUserMembership(ctx context.Context, userID string, f func(obj *DirectoryObject) bool) error { + var err error + itErr := c.iterate(ctx, path.Join("users", userID, "memberOf"), func(msg json.RawMessage) bool { + var page []json.RawMessage + if err = json.Unmarshal(msg, &page); err != nil { + return false + } + for _, entry := range page { + var d *DirectoryObject + err := json.Unmarshal(entry, &d) + if err != nil { + return false + } + f(d) + } + return true + }) + if err != nil { + return trace.Wrap(err) + } + return trace.Wrap(itErr) +} + // IterateGroupMembers lists all members for the given Entra ID group using pagination. // `f` will be called for each object in the result set. // if `f` returns `false`, the iteration is stopped (equivalent to `break` in a normal loop). From 0d4d6c393220262a087505ff7df4ea931fc22e1c Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 8 Jan 2025 22:30:19 -0600 Subject: [PATCH 11/41] PR feedback --- lib/msgraph/models.go | 9 --------- lib/msgraph/paginated.go | 27 ++++++--------------------- 2 files changed, 6 insertions(+), 30 deletions(-) diff --git a/lib/msgraph/models.go b/lib/msgraph/models.go index fc2d9a5fee1e1..f76d3deaab858 100644 --- a/lib/msgraph/models.go +++ b/lib/msgraph/models.go @@ -184,12 +184,3 @@ func decodeGroupMember(msg json.RawMessage) (GroupMember, error) { return member, trace.Wrap(err) } - -func decodeDirectoryObject(msg json.RawMessage) (*DirectoryObject, error) { - var d *DirectoryObject - err := json.Unmarshal(msg, &d) - if err != nil { - return nil, trace.Wrap(err) - } - return d, nil -} diff --git a/lib/msgraph/paginated.go b/lib/msgraph/paginated.go index 1e6a915fbe634..da44a4f442a1b 100644 --- a/lib/msgraph/paginated.go +++ b/lib/msgraph/paginated.go @@ -109,27 +109,12 @@ func (c *Client) IterateServicePrincipals(ctx context.Context, f func(principal return iterateSimple(c, ctx, "servicePrincipals", f) } -func (c *Client) IterateUserMembership(ctx context.Context, userID string, f func(obj *DirectoryObject) bool) error { - var err error - itErr := c.iterate(ctx, path.Join("users", userID, "memberOf"), func(msg json.RawMessage) bool { - var page []json.RawMessage - if err = json.Unmarshal(msg, &page); err != nil { - return false - } - for _, entry := range page { - var d *DirectoryObject - err := json.Unmarshal(entry, &d) - if err != nil { - return false - } - f(d) - } - return true - }) - if err != nil { - return trace.Wrap(err) - } - return trace.Wrap(itErr) +// IterateUserMembership lists all group memberships for a given user ID as directory objects. +// `f` will be called for each directory object in the result set. +// if `f` returns `false`, the iteration is stopped (equivalent to `break` in a normal loop). +// Ref: [https://learn.microsoft.com/en-us/graph/api/group-list-memberof]. +func (c *Client) IterateUserMembership(ctx context.Context, userID string, f func(object *DirectoryObject) bool) error { + return iterateSimple(c, ctx, path.Join("users", userID, "memberOf"), f) } // IterateGroupMembers lists all members for the given Entra ID group using pagination. From 1cb9b4280bda27459894c62e28ab80f67a56d17f Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 8 Jan 2025 22:40:43 -0600 Subject: [PATCH 12/41] Rebase go.sum stuff --- go.mod | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index c5594219a47bc..6a63de4bbb19e 100644 --- a/go.mod +++ b/go.mod @@ -242,6 +242,8 @@ require ( software.sslmate.com/src/go-pkcs12 v0.5.0 ) +require github.com/xanzy/go-gitlab v0.115.0 + require ( cel.dev/expr v0.19.1 // indirect cloud.google.com/go v0.117.0 // indirect @@ -520,7 +522,6 @@ require ( github.com/vbatts/tar-split v0.11.5 // indirect github.com/weppos/publicsuffix-go v0.30.3-0.20240510084413-5f1d03393b3d // indirect github.com/x448/float16 v0.8.4 // indirect - github.com/xanzy/go-gitlab v0.115.0 // indirect github.com/xdg-go/pbkdf2 v1.0.0 // indirect github.com/xdg-go/scram v1.1.2 // indirect github.com/xdg-go/stringprep v1.0.4 // indirect From 8524172871211388e94a332c8dc7880c6427a22d Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 8 Jan 2025 22:47:34 -0600 Subject: [PATCH 13/41] Go mod tidy --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 6a63de4bbb19e..8ef88b7103a1b 100644 --- a/go.mod +++ b/go.mod @@ -242,7 +242,7 @@ require ( software.sslmate.com/src/go-pkcs12 v0.5.0 ) -require github.com/xanzy/go-gitlab v0.115.0 +require github.com/xanzy/go-gitlab v0.115.0 // indirect require ( cel.dev/expr v0.19.1 // indirect From 5fa9382b6bba044eda03ce3ef5d9115b5bc1d52f Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Thu, 9 Jan 2025 10:36:27 -0600 Subject: [PATCH 14/41] Fixing go.mod --- go.mod | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 8ef88b7103a1b..c5594219a47bc 100644 --- a/go.mod +++ b/go.mod @@ -242,8 +242,6 @@ require ( software.sslmate.com/src/go-pkcs12 v0.5.0 ) -require github.com/xanzy/go-gitlab v0.115.0 // indirect - require ( cel.dev/expr v0.19.1 // indirect cloud.google.com/go v0.117.0 // indirect @@ -522,6 +520,7 @@ require ( github.com/vbatts/tar-split v0.11.5 // indirect github.com/weppos/publicsuffix-go v0.30.3-0.20240510084413-5f1d03393b3d // indirect github.com/x448/float16 v0.8.4 // indirect + github.com/xanzy/go-gitlab v0.115.0 // indirect github.com/xdg-go/pbkdf2 v1.0.0 // indirect github.com/xdg-go/scram v1.1.2 // indirect github.com/xdg-go/stringprep v1.0.4 // indirect From 44888bbd07b2686ae2fa491440d951a2ce9c2872 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Fri, 10 Jan 2025 10:00:39 -0600 Subject: [PATCH 15/41] Update lib/msgraph/paginated.go Co-authored-by: Tiago Silva --- lib/msgraph/paginated.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/msgraph/paginated.go b/lib/msgraph/paginated.go index da44a4f442a1b..0321b8a326d9c 100644 --- a/lib/msgraph/paginated.go +++ b/lib/msgraph/paginated.go @@ -109,11 +109,11 @@ func (c *Client) IterateServicePrincipals(ctx context.Context, f func(principal return iterateSimple(c, ctx, "servicePrincipals", f) } -// IterateUserMembership lists all group memberships for a given user ID as directory objects. +// IterateUserMemberships lists all group memberships for a given user ID as directory objects. // `f` will be called for each directory object in the result set. // if `f` returns `false`, the iteration is stopped (equivalent to `break` in a normal loop). // Ref: [https://learn.microsoft.com/en-us/graph/api/group-list-memberof]. -func (c *Client) IterateUserMembership(ctx context.Context, userID string, f func(object *DirectoryObject) bool) error { +func (c *Client) IterateUserMemberships(ctx context.Context, userID string, f func(object *DirectoryObject) bool) error { return iterateSimple(c, ctx, path.Join("users", userID, "memberOf"), f) } From 27d7a7378bd95a72e2f0652959eeef1ee2d4117e Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Fri, 10 Jan 2025 10:11:21 -0600 Subject: [PATCH 16/41] PR feedback --- lib/msgraph/models.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/msgraph/models.go b/lib/msgraph/models.go index f76d3deaab858..52c3e97cfec7b 100644 --- a/lib/msgraph/models.go +++ b/lib/msgraph/models.go @@ -28,11 +28,6 @@ type GroupMember interface { isGroupMember() } -type Membership struct { - Type string `json:"@odata.type"` - ID string `json:"id"` -} - type DirectoryObject struct { ID *string `json:"id,omitempty"` DisplayName *string `json:"displayName,omitempty"` From 2604aa8dee4e8d37bfd97fe01fec344ffef12053 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Fri, 10 Jan 2025 10:22:11 -0600 Subject: [PATCH 17/41] e ref update --- e | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e b/e index 65fa473e50c72..498f643ea9033 160000 --- a/e +++ b/e @@ -1 +1 @@ -Subproject commit 65fa473e50c72d8f79261033a1298cc2955ca15c +Subproject commit 498f643ea9033b1235359d83c310caadb18305d2 From adec49e1ae54382c98134ff66cab62499df4408c Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Tue, 17 Dec 2024 16:49:05 -0600 Subject: [PATCH 18/41] Adding the Azure sync module functions along with new cloud client functionality --- .../fetchers/azure-sync/msggraphclient.go | 240 ++++++++++++++++++ .../fetchers/azure-sync/reconcile.go | 165 ++++++++++++ .../fetchers/azure-sync/reconcile_test.go | 191 ++++++++++++++ 3 files changed, 596 insertions(+) create mode 100644 lib/srv/discovery/fetchers/azure-sync/msggraphclient.go create mode 100644 lib/srv/discovery/fetchers/azure-sync/reconcile.go create mode 100644 lib/srv/discovery/fetchers/azure-sync/reconcile_test.go diff --git a/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go b/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go new file mode 100644 index 0000000000000..75d2960d7fa55 --- /dev/null +++ b/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go @@ -0,0 +1,240 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package azure_sync + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" +) + +// GraphClient represents generic MS API client +type GraphClient struct { + token azcore.AccessToken +} + +const ( + usersSuffix = "users" + groupsSuffix = "groups" + servicePrincipalsSuffix = "servicePrincipals" + graphBaseURL = "https://graph.microsoft.com/v1.0" + httpTimeout = time.Second * 30 +) + +// graphError represents MS Graph error +type graphError struct { + E struct { + Code string `json:"code"` + Message string `json:"message"` + } `json:"error"` +} + +// genericGraphResponse represents the utility struct for parsing MS Graph API response +type genericGraphResponse struct { + Context string `json:"@odata.context"` + Count int `json:"@odata.count"` + NextLink string `json:"@odata.nextLink"` + Value json.RawMessage `json:"value"` +} + +// User represents user resource +type User struct { + ID string `json:"id"` + Name string `json:"displayName"` + MemberOf []Membership `json:"memberOf"` +} + +type Membership struct { + Type string `json:"@odata.type"` + ID string `json:"id"` +} + +// request represents generic request structure +type request struct { + // Method HTTP method + Method string + // URL which overrides URL construction + URL *string + // Path to a resource + Path string + // Expand $expand value + Expand []string + // Filter $filter value + Filter string + // Body request body + Body string + // Response represents template structure for a response + Response interface{} + // Err represents template structure for an error + Err error + // SuccessCode http code representing success + SuccessCode int +} + +// GetURL builds the request URL +func (r *request) GetURL() (string, error) { + if r.URL != nil { + return *r.URL, nil + } + u, err := url.Parse(graphBaseURL) + if err != nil { + return "", err + } + + data := url.Values{} + if len(r.Expand) > 0 { + data.Set("$expand", strings.Join(r.Expand, ",")) + } + if r.Filter != "" { + data.Set("$filter", r.Filter) + } + + u.Path = u.Path + "/" + r.Path + u.RawQuery = data.Encode() + + return u.String(), nil +} + +// NewGraphClient creates MS Graph API client +func NewGraphClient(token azcore.AccessToken) *GraphClient { + return &GraphClient{ + token: token, + } +} + +// Error returns error string +func (e graphError) Error() string { + return e.E.Code + " " + e.E.Message +} + +func (c *GraphClient) ListUsers(ctx context.Context) ([]User, error) { + return c.listIdentities(ctx, usersSuffix, []string{"memberOf"}) +} + +func (c *GraphClient) ListGroups(ctx context.Context) ([]User, error) { + return c.listIdentities(ctx, groupsSuffix, []string{"memberOf"}) +} + +func (c *GraphClient) ListServicePrincipals(ctx context.Context) ([]User, error) { + return c.listIdentities(ctx, servicePrincipalsSuffix, []string{"memberOf"}) +} + +func (c *GraphClient) listIdentities(ctx context.Context, idType string, expand []string) ([]User, error) { + var users []User + var nextLink *string + for { + g := &genericGraphResponse{} + req := request{ + Method: http.MethodGet, + Path: idType, + Expand: expand, + Response: &g, + Err: &graphError{}, + URL: nextLink, + } + err := c.request(ctx, req) + if err != nil { + return nil, err + } + var newUsers []User + err = json.NewDecoder(bytes.NewReader(g.Value)).Decode(&newUsers) + if err != nil { + return nil, err + } + users = append(users, newUsers...) + if g.NextLink == "" { + break + } + nextLink = &g.NextLink + } + + return users, nil +} + +// request sends the request to the graph/bot service and returns response body as bytes slice +func (c *GraphClient) request(ctx context.Context, req request) error { + reqUrl, err := req.GetURL() + if err != nil { + return err + } + + r, err := http.NewRequestWithContext(ctx, req.Method, reqUrl, strings.NewReader(req.Body)) + if err != nil { + return err + } + + r.Header.Set("Authorization", "Bearer "+c.token.Token) + r.Header.Set("Content-Type", "application/json") + + client := http.Client{Timeout: httpTimeout} + resp, err := client.Do(r) + if err != nil { + return err + } + + defer func(r *http.Response) { + _ = r.Body.Close() + }(resp) + + b, err := io.ReadAll(resp.Body) + if err != nil { + return err + } + + expectedCode := req.SuccessCode + if expectedCode == 0 { + expectedCode = http.StatusOK + } + + if expectedCode == resp.StatusCode { + if req.Response == nil { + return nil + } + + err := json.NewDecoder(bytes.NewReader(b)).Decode(req.Response) + if err != nil { + return err + } + } else { + if req.Err == nil { + return fmt.Errorf("Error requesting MS Graph API: %v", string(b)) + } + + err := json.NewDecoder(bytes.NewReader(b)).Decode(req.Err) + if err != nil { + return err + } + + if req.Err.Error() == "" { + return fmt.Errorf("Error requesting MS Graph API. Expected response code was %v, but is %v", expectedCode, resp.StatusCode) + } + + return req.Err + } + + return nil +} diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile.go b/lib/srv/discovery/fetchers/azure-sync/reconcile.go new file mode 100644 index 0000000000000..2b54c8cfac911 --- /dev/null +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile.go @@ -0,0 +1,165 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package azure_sync + +import ( + "fmt" + + "google.golang.org/protobuf/proto" + + accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" + "github.com/gravitational/teleport/lib/srv/discovery/common" +) + +// MergeResources merges Azure resources fetched from multiple configured Azure fetchers +func MergeResources(results ...*Resources) *Resources { + if len(results) == 0 { + return &Resources{} + } + if len(results) == 1 { + return results[0] + } + result := &Resources{} + for _, r := range results { + result.Principals = append(result.Principals, r.Principals...) + result.RoleAssignments = append(result.RoleAssignments, r.RoleAssignments...) + result.RoleDefinitions = append(result.RoleDefinitions, r.RoleDefinitions...) + result.VirtualMachines = append(result.VirtualMachines, r.VirtualMachines...) + } + result.Principals = common.DeduplicateSlice(result.Principals, azurePrincipalsKey) + result.RoleAssignments = common.DeduplicateSlice(result.RoleAssignments, azureRoleAssignKey) + result.RoleDefinitions = common.DeduplicateSlice(result.RoleDefinitions, azureRoleDefKey) + result.VirtualMachines = common.DeduplicateSlice(result.VirtualMachines, azureVmKey) + return result +} + +// newResourceList creates a new resource list message +func newResourceList() *accessgraphv1alpha.AzureResourceList { + return &accessgraphv1alpha.AzureResourceList{ + Resources: make([]*accessgraphv1alpha.AzureResource, 0), + } +} + +// ReconcileResults compares previously and currently fetched results and determines which resources to upsert and +// which to delete. +func ReconcileResults(old *Resources, new *Resources) (upsert, delete *accessgraphv1alpha.AzureResourceList) { + upsert, delete = newResourceList(), newResourceList() + reconciledResources := []*reconcilePair{ + reconcile(old.Principals, new.Principals, azurePrincipalsKey, azurePrincipalsWrap), + reconcile(old.RoleAssignments, new.RoleAssignments, azureRoleAssignKey, azureRoleAssignWrap), + reconcile(old.RoleDefinitions, new.RoleDefinitions, azureRoleDefKey, azureRoleDefWrap), + reconcile(old.VirtualMachines, new.VirtualMachines, azureVmKey, azureVmWrap), + } + for _, res := range reconciledResources { + upsert.Resources = append(upsert.Resources, res.upsert.Resources...) + delete.Resources = append(delete.Resources, res.delete.Resources...) + } + return upsert, delete +} + +// reconcilePair contains the Azure resources to upsert and delete +type reconcilePair struct { + upsert, delete *accessgraphv1alpha.AzureResourceList +} + +// reconcile compares old and new items to build a list of resources to upsert and delete in the Access Graph +func reconcile[T proto.Message]( + oldItems []T, + newItems []T, + keyFn func(T) string, + wrapFn func(T) *accessgraphv1alpha.AzureResource, +) *reconcilePair { + // Remove duplicates from the new items + newItems = common.DeduplicateSlice(newItems, keyFn) + upsertRes := newResourceList() + deleteRes := newResourceList() + + // Delete all old items if there are no new items + if len(newItems) == 0 { + for _, item := range oldItems { + deleteRes.Resources = append(deleteRes.Resources, wrapFn(item)) + } + return &reconcilePair{upsertRes, deleteRes} + } + + // Create all new items if there are no old items + if len(oldItems) == 0 { + for _, item := range newItems { + upsertRes.Resources = append(upsertRes.Resources, wrapFn(item)) + } + return &reconcilePair{upsertRes, deleteRes} + } + + // Map old and new items by their key + oldMap := make(map[string]T, len(oldItems)) + for _, item := range oldItems { + oldMap[keyFn(item)] = item + } + newMap := make(map[string]T, len(newItems)) + for _, item := range newItems { + newMap[keyFn(item)] = item + } + + // Append new or modified items to the upsert list + for _, item := range newItems { + if oldItem, ok := oldMap[keyFn(item)]; !ok || !proto.Equal(oldItem, item) { + upsertRes.Resources = append(upsertRes.Resources, wrapFn(item)) + } + } + + // Append removed items to the delete list + for _, item := range oldItems { + if _, ok := newMap[keyFn(item)]; !ok { + deleteRes.Resources = append(deleteRes.Resources, wrapFn(item)) + } + } + return &reconcilePair{upsertRes, deleteRes} +} + +func azurePrincipalsKey(user *accessgraphv1alpha.AzurePrincipal) string { + return fmt.Sprintf("%s:%s", user.SubscriptionId, user.Id) +} + +func azurePrincipalsWrap(principal *accessgraphv1alpha.AzurePrincipal) *accessgraphv1alpha.AzureResource { + return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_Principal{Principal: principal}} +} + +func azureRoleAssignKey(roleAssign *accessgraphv1alpha.AzureRoleAssignment) string { + return fmt.Sprintf("%s:%s", roleAssign.SubscriptionId, roleAssign.Id) +} + +func azureRoleAssignWrap(roleAssign *accessgraphv1alpha.AzureRoleAssignment) *accessgraphv1alpha.AzureResource { + return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_RoleAssignment{RoleAssignment: roleAssign}} +} + +func azureRoleDefKey(roleDef *accessgraphv1alpha.AzureRoleDefinition) string { + return fmt.Sprintf("%s:%s", roleDef.SubscriptionId, roleDef.Id) +} + +func azureRoleDefWrap(roleDef *accessgraphv1alpha.AzureRoleDefinition) *accessgraphv1alpha.AzureResource { + return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_RoleDefinition{RoleDefinition: roleDef}} +} + +func azureVmKey(vm *accessgraphv1alpha.AzureVirtualMachine) string { + return fmt.Sprintf("%s:%s", vm.SubscriptionId, vm.Id) +} + +func azureVmWrap(vm *accessgraphv1alpha.AzureVirtualMachine) *accessgraphv1alpha.AzureResource { + return &accessgraphv1alpha.AzureResource{Resource: &accessgraphv1alpha.AzureResource_VirtualMachine{VirtualMachine: vm}} +} diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go new file mode 100644 index 0000000000000..28b293bcf1f8d --- /dev/null +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go @@ -0,0 +1,191 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package azure_sync + +import ( + "testing" + + "github.com/stretchr/testify/require" + + accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" +) + +func TestReconcileResults(t *testing.T) { + principals := generatePrincipals() + roleDefs := generateRoleDefs() + roleAssigns := generateRoleAssigns() + vms := generateVms() + + tests := []struct { + oldResults *Resources + newResults *Resources + expectedUpserts *accessgraphv1alpha.AzureResourceList + expectedDeletes *accessgraphv1alpha.AzureResourceList + }{ + // Overlapping old and new results + { + oldResults: &Resources{ + Principals: principals[0:2], + RoleDefinitions: roleDefs[0:2], + RoleAssignments: roleAssigns[0:2], + VirtualMachines: vms[0:2], + }, + newResults: &Resources{ + Principals: principals[1:3], + RoleDefinitions: roleDefs[1:3], + RoleAssignments: roleAssigns[1:3], + VirtualMachines: vms[1:3], + }, + expectedUpserts: generateExpected(principals[2:3], roleDefs[2:3], roleAssigns[2:3], vms[2:3]), + expectedDeletes: generateExpected(principals[0:1], roleDefs[0:1], roleAssigns[0:1], vms[0:1]), + }, + // Completely new results + { + oldResults: &Resources{ + Principals: nil, + RoleDefinitions: nil, + RoleAssignments: nil, + VirtualMachines: nil, + }, + newResults: &Resources{ + Principals: principals[1:3], + RoleDefinitions: roleDefs[1:3], + RoleAssignments: roleAssigns[1:3], + VirtualMachines: vms[1:3], + }, + expectedUpserts: generateExpected(principals[1:3], roleDefs[1:3], roleAssigns[1:3], vms[1:3]), + expectedDeletes: generateExpected(nil, nil, nil, nil), + }, + // No new results + { + oldResults: &Resources{ + Principals: principals[1:3], + RoleDefinitions: roleDefs[1:3], + RoleAssignments: roleAssigns[1:3], + VirtualMachines: vms[1:3], + }, + newResults: &Resources{ + Principals: nil, + RoleDefinitions: nil, + RoleAssignments: nil, + VirtualMachines: nil, + }, + expectedUpserts: generateExpected(nil, nil, nil, nil), + expectedDeletes: generateExpected(principals[1:3], roleDefs[1:3], roleAssigns[1:3], vms[1:3]), + }, + } + + for _, tt := range tests { + upserts, deletes := ReconcileResults(tt.oldResults, tt.newResults) + require.ElementsMatch(t, upserts.Resources, tt.expectedUpserts.Resources) + require.ElementsMatch(t, deletes.Resources, tt.expectedDeletes.Resources) + } + +} + +func generateExpected( + principals []*accessgraphv1alpha.AzurePrincipal, + roleDefs []*accessgraphv1alpha.AzureRoleDefinition, + roleAssigns []*accessgraphv1alpha.AzureRoleAssignment, + vms []*accessgraphv1alpha.AzureVirtualMachine, +) *accessgraphv1alpha.AzureResourceList { + resList := &accessgraphv1alpha.AzureResourceList{ + Resources: make([]*accessgraphv1alpha.AzureResource, 0), + } + for _, principal := range principals { + resList.Resources = append(resList.Resources, azurePrincipalsWrap(principal)) + } + for _, roleDef := range roleDefs { + resList.Resources = append(resList.Resources, azureRoleDefWrap(roleDef)) + } + for _, roleAssign := range roleAssigns { + resList.Resources = append(resList.Resources, azureRoleAssignWrap(roleAssign)) + } + for _, vm := range vms { + resList.Resources = append(resList.Resources, azureVmWrap(vm)) + } + return resList +} + +func generatePrincipals() []*accessgraphv1alpha.AzurePrincipal { + return []*accessgraphv1alpha.AzurePrincipal{ + { + Id: "/principals/foo", + DisplayName: "userFoo", + }, + { + Id: "/principals/bar", + DisplayName: "userBar", + }, + { + Id: "/principals/charles", + DisplayName: "userCharles", + }, + } +} + +func generateRoleDefs() []*accessgraphv1alpha.AzureRoleDefinition { + return []*accessgraphv1alpha.AzureRoleDefinition{ + { + Id: "/roledefinitions/foo", + Name: "roleFoo", + }, + { + Id: "/roledefinitions/bar", + Name: "roleBar", + }, + { + Id: "/roledefinitions/charles", + Name: "roleCharles", + }, + } +} + +func generateRoleAssigns() []*accessgraphv1alpha.AzureRoleAssignment { + return []*accessgraphv1alpha.AzureRoleAssignment{ + { + Id: "/roleassignments/foo", + PrincipalId: "userFoo", + }, + { + Id: "/roleassignments/bar", + PrincipalId: "userBar", + }, + { + Id: "/roleassignments/charles", + PrincipalId: "userCharles", + }, + } +} + +func generateVms() []*accessgraphv1alpha.AzureVirtualMachine { + return []*accessgraphv1alpha.AzureVirtualMachine{ + { + Id: "/vms/foo", + Name: "userFoo", + }, + { + Id: "/vms/bar", + Name: "userBar", + }, + { + Id: "/vms/charles", + Name: "userCharles", + }, + } +} From f6a1fad2a760685b1b8f591126751989a57f6178 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Tue, 17 Dec 2024 16:06:54 -0600 Subject: [PATCH 19/41] Protobuf and configuration for Access Graph Azure Discovery --- api/types/types.pb.go | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/api/types/types.pb.go b/api/types/types.pb.go index a49edaacc6d5b..8883bd2fe0a4a 100644 --- a/api/types/types.pb.go +++ b/api/types/types.pb.go @@ -135137,6 +135137,40 @@ func (m *AccessGraphSync) Unmarshal(dAtA []byte) error { return err } iNdEx = postIndex + case 3: + if wireType != 2 { + return fmt.Errorf("proto: wrong wireType = %d for field Azure", wireType) + } + var msglen int + for shift := uint(0); ; shift += 7 { + if shift >= 64 { + return ErrIntOverflowTypes + } + if iNdEx >= l { + return io.ErrUnexpectedEOF + } + b := dAtA[iNdEx] + iNdEx++ + msglen |= int(b&0x7F) << shift + if b < 0x80 { + break + } + } + if msglen < 0 { + return ErrInvalidLengthTypes + } + postIndex := iNdEx + msglen + if postIndex < 0 { + return ErrInvalidLengthTypes + } + if postIndex > l { + return io.ErrUnexpectedEOF + } + m.Azure = append(m.Azure, &AccessGraphAzureSync{}) + if err := m.Azure[len(m.Azure)-1].Unmarshal(dAtA[iNdEx:postIndex]); err != nil { + return err + } + iNdEx = postIndex default: iNdEx = preIndex skippy, err := skipTypes(dAtA[iNdEx:]) From ba79bf76c0fa209d95546815946ed305deeeebf8 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Tue, 17 Dec 2024 17:18:58 -0600 Subject: [PATCH 20/41] Adding Azure sync functionality which can be called by the Azure fetcher --- lib/srv/discovery/common/reconcile.go | 32 +++ .../fetchers/azure-sync/azure-sync.go | 244 ++++++++++++++++++ .../fetchers/azure-sync/azure-sync_test.go | 227 ++++++++++++++++ 3 files changed, 503 insertions(+) create mode 100644 lib/srv/discovery/common/reconcile.go create mode 100644 lib/srv/discovery/fetchers/azure-sync/azure-sync.go create mode 100644 lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go diff --git a/lib/srv/discovery/common/reconcile.go b/lib/srv/discovery/common/reconcile.go new file mode 100644 index 0000000000000..9161ae69b95b1 --- /dev/null +++ b/lib/srv/discovery/common/reconcile.go @@ -0,0 +1,32 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package common + +func DeduplicateSlice[T any](s []T, key func(T) string) []T { + out := make([]T, 0, len(s)) + seen := make(map[string]struct{}) + for _, v := range s { + if _, ok := seen[key(v)]; ok { + continue + } + seen[key(v)] = struct{}{} + out = append(out, v) + } + return out +} diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go new file mode 100644 index 0000000000000..df9549c71c493 --- /dev/null +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -0,0 +1,244 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package azure_sync + +import ( + "context" + "sync" + + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/gravitational/trace" + "golang.org/x/sync/errgroup" + + accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" + "github.com/gravitational/teleport/lib/cloud" + "github.com/gravitational/teleport/lib/cloud/azure" + "github.com/gravitational/teleport/lib/srv/discovery/common" +) + +const ( + featNamePrincipals = "azure/principals" + featNameRoleDefinitions = "azure/roledefinitions" + featNameRoleAssignments = "azure/roleassignments" + featNameVms = "azure/virtualmachines" +) + +// FetcherConcurrency is an arbitrary per-resource type concurrency to ensure significant throughput. As we increase +// the number of resource types, we may increase this value or use some other approach to fetching concurrency. +const FetcherConcurrency = 4 + +// Config defines parameters required for fetching resources from Azure +type Config struct { + CloudClients cloud.Clients + SubscriptionID string + Integration string + DiscoveryConfigName string +} + +// Resources represents the set of resources fetched from Azure +type Resources struct { + Principals []*accessgraphv1alpha.AzurePrincipal + RoleDefinitions []*accessgraphv1alpha.AzureRoleDefinition + RoleAssignments []*accessgraphv1alpha.AzureRoleAssignment + VirtualMachines []*accessgraphv1alpha.AzureVirtualMachine +} + +// Fetcher provides the functionality for fetching resources from Azure +type Fetcher struct { + Config + lastError error + lastDiscoveredResources uint64 + lastResult *Resources + + roleAssignClient RoleAssignmentsClient + roleDefClient RoleDefinitionsClient + vmClient VirtualMachinesClient +} + +// NewFetcher returns a new fetcher based on configuration parameters +func NewFetcher(cfg Config, ctx context.Context) (*Fetcher, error) { + // Establish the credential from the managed identity + cred, err := azidentity.NewDefaultAzureCredential(nil) + if err != nil { + return nil, trace.Wrap(err) + } + + // Create the clients + roleAssignClient, err := azure.NewRoleAssignmentsClient(cfg.SubscriptionID, cred, nil) + if err != nil { + return nil, trace.Wrap(err) + } + roleDefClient, err := azure.NewRoleDefinitionsClient(cfg.SubscriptionID, cred, nil) + if err != nil { + return nil, trace.Wrap(err) + } + vmClient, err := azure.NewVirtualMachinesClient(cfg.SubscriptionID, cred, nil) + if err != nil { + return nil, trace.Wrap(err) + } + + // Create and return the fetcher + return &Fetcher{ + Config: cfg, + lastResult: &Resources{}, + roleAssignClient: roleAssignClient, + roleDefClient: roleDefClient, + vmClient: vmClient, + }, nil +} + +// Features is a set of booleans that are received from the Access Graph to indicate which resources it can receive +type Features struct { + Principals bool + RoleDefinitions bool + RoleAssignments bool + VirtualMachines bool +} + +// BuildFeatures builds the feature flags based on supported types returned by Access Graph Azure endpoints. +func BuildFeatures(values ...string) Features { + features := Features{} + for _, value := range values { + switch value { + case featNamePrincipals: + features.Principals = true + case featNameRoleAssignments: + features.RoleAssignments = true + case featNameRoleDefinitions: + features.RoleDefinitions = true + case featNameVms: + features.VirtualMachines = true + } + } + return features +} + +// Poll fetches and deduplicates Azure resources specified by the Access Graph +func (a *Fetcher) Poll(ctx context.Context, feats Features) (*Resources, error) { + res, err := a.fetch(ctx, feats) + if res == nil { + return nil, err + } + res.Principals = common.DeduplicateSlice(res.Principals, azurePrincipalsKey) + res.RoleAssignments = common.DeduplicateSlice(res.RoleAssignments, azureRoleAssignKey) + res.RoleDefinitions = common.DeduplicateSlice(res.RoleDefinitions, azureRoleDefKey) + res.VirtualMachines = common.DeduplicateSlice(res.VirtualMachines, azureVmKey) + return res, trace.Wrap(err) +} + +// fetch returns the resources specified by the Access Graph +func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) { + // Accumulate Azure resources + eg, ctx := errgroup.WithContext(ctx) + eg.SetLimit(FetcherConcurrency) + var result = &Resources{} + var errs []error + errsCh := make(chan error) + if feats.Principals { + eg.Go(func() error { + cred, err := a.CloudClients.GetAzureCredential() + if err != nil { + return trace.Wrap(err) + } + principals, err := fetchPrincipals(ctx, a.SubscriptionID, cred) + if err != nil { + errsCh <- err + return err + } + result.Principals = principals + return nil + }) + } + if feats.RoleAssignments { + eg.Go(func() error { + roleAssigns, err := fetchRoleAssignments(ctx, a.SubscriptionID, a.roleAssignClient) + if err != nil { + errsCh <- err + return err + } + result.RoleAssignments = roleAssigns + return nil + }) + } + if feats.RoleDefinitions { + eg.Go(func() error { + roleDefs, err := fetchRoleDefinitions(ctx, a.SubscriptionID, a.roleDefClient) + if err != nil { + errsCh <- err + return err + } + result.RoleDefinitions = roleDefs + return nil + }) + } + if feats.VirtualMachines { + eg.Go(func() error { + vms, err := fetchVirtualMachines(ctx, a.SubscriptionID, a.vmClient) + if err != nil { + errsCh <- err + return err + } + result.VirtualMachines = vms + return nil + }) + } + + // Collect the error messages from the error channel + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + for { + err, ok := <-errsCh + if !ok { + return + } + errs = append(errs, err) + } + }() + _ = eg.Wait() + close(errsCh) + wg.Wait() + if len(errs) > 0 { + return result, trace.NewAggregate(errs...) + } + + // Return the resources + return result, nil +} + +// Status returns the number of resources last fetched and/or the last fetching/reconciling error +func (a *Fetcher) Status() (uint64, error) { + return a.lastDiscoveredResources, a.lastError +} + +// DiscoveryConfigName returns the name of the configured discovery +func (a *Fetcher) DiscoveryConfigName() string { + return a.Config.DiscoveryConfigName +} + +// IsFromDiscoveryConfig returns whether the discovery is from configuration or dynamic +func (a *Fetcher) IsFromDiscoveryConfig() bool { + return a.Config.DiscoveryConfigName != "" +} + +// GetSubscriptionID returns the ID of the Azure subscription +func (a *Fetcher) GetSubscriptionID() string { + return a.Config.SubscriptionID +} diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go new file mode 100644 index 0000000000000..d5c57d06edf1e --- /dev/null +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go @@ -0,0 +1,227 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package azure_sync + +import ( + "context" + "fmt" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v6" + "github.com/stretchr/testify/require" +) + +type testRoleDefCli struct { + returnErr bool + roleDefs []*armauthorization.RoleDefinition +} + +func (t testRoleDefCli) ListRoleDefinitions(ctx context.Context, scope string) ([]*armauthorization.RoleDefinition, error) { + if t.returnErr { + return nil, fmt.Errorf("error") + } + return t.roleDefs, nil +} + +type testRoleAssignCli struct { + returnErr bool + roleAssigns []*armauthorization.RoleAssignment +} + +func (t testRoleAssignCli) ListRoleAssignments(ctx context.Context, scope string) ([]*armauthorization.RoleAssignment, error) { + if t.returnErr { + return nil, fmt.Errorf("error") + } + return t.roleAssigns, nil +} + +type testVmCli struct { + returnErr bool + vms []*armcompute.VirtualMachine +} + +func (t testVmCli) ListVirtualMachines(ctx context.Context, resourceGroup string) ([]*armcompute.VirtualMachine, error) { + if t.returnErr { + return nil, fmt.Errorf("error") + } + return t.vms, nil +} + +func newRoleDef(id string, name string) *armauthorization.RoleDefinition { + role_name := "test_role_name" + action1 := "Microsoft.Compute/virtualMachines/read" + action2 := "Microsoft.Compute/virtualMachines/*" + action3 := "Microsoft.Compute/*" + return &armauthorization.RoleDefinition{ + ID: &id, + Name: &name, + Properties: &armauthorization.RoleDefinitionProperties{ + Permissions: []*armauthorization.Permission{ + { + Actions: []*string{&action1, &action2}, + }, + { + Actions: []*string{&action3}, + }, + }, + RoleName: &role_name, + }, + } +} + +func newRoleAssign(id string, name string) *armauthorization.RoleAssignment { + scope := "test_scope" + principalId := "test_principal_id" + roleDefId := "test_role_def_id" + return &armauthorization.RoleAssignment{ + ID: &id, + Name: &name, + Properties: &armauthorization.RoleAssignmentProperties{ + PrincipalID: &principalId, + RoleDefinitionID: &roleDefId, + Scope: &scope, + }, + } +} + +func newVm(id string, name string) *armcompute.VirtualMachine { + return &armcompute.VirtualMachine{ + ID: &id, + Name: &name, + } +} + +func TestPoll(t *testing.T) { + roleDefs := []*armauthorization.RoleDefinition{ + newRoleDef("id1", "name1"), + } + roleAssigns := []*armauthorization.RoleAssignment{ + newRoleAssign("id1", "name1"), + } + vms := []*armcompute.VirtualMachine{ + newVm("id1", "name2"), + } + roleDefClient := testRoleDefCli{} + roleAssignClient := testRoleAssignCli{} + vmClient := testVmCli{} + fetcher := Fetcher{ + Config: Config{SubscriptionID: "1234567890"}, + lastResult: &Resources{}, + roleDefClient: &roleDefClient, + roleAssignClient: &roleAssignClient, + vmClient: &vmClient, + } + ctx := context.Background() + allFeats := Features{ + RoleDefinitions: true, + RoleAssignments: true, + VirtualMachines: true, + } + noVmsFeats := allFeats + noVmsFeats.VirtualMachines = false + + tests := []struct { + returnErr bool + roleDefs []*armauthorization.RoleDefinition + roleAssigns []*armauthorization.RoleAssignment + vms []*armcompute.VirtualMachine + feats Features + }{ + // Process no results from clients + { + returnErr: false, + roleDefs: []*armauthorization.RoleDefinition{}, + roleAssigns: []*armauthorization.RoleAssignment{}, + vms: []*armcompute.VirtualMachine{}, + feats: allFeats, + }, + // Process test results from clients + { + returnErr: false, + roleDefs: roleDefs, + roleAssigns: roleAssigns, + vms: vms, + feats: allFeats, + }, + // Handle errors from clients + { + returnErr: true, + roleDefs: roleDefs, + roleAssigns: roleAssigns, + vms: vms, + feats: allFeats, + }, + // Handle VM features being disabled + { + returnErr: false, + roleDefs: roleDefs, + roleAssigns: roleAssigns, + vms: vms, + feats: noVmsFeats, + }, + } + + for _, tt := range tests { + // Set the test data + roleDefClient.returnErr = tt.returnErr + roleDefClient.roleDefs = tt.roleDefs + roleAssignClient.returnErr = tt.returnErr + roleAssignClient.roleAssigns = tt.roleAssigns + vmClient.returnErr = tt.returnErr + vmClient.vms = tt.vms + + // Poll for resources + resources, err := fetcher.Poll(ctx, tt.feats) + + // Require no error unless otherwise specified + if tt.returnErr { + require.Error(t, err) + continue + } + require.NoError(t, err) + + // Verify the results, based on the features set + require.NotNil(t, resources) + require.Equal(t, tt.feats.RoleDefinitions == false || len(tt.roleDefs) == 0, len(resources.RoleDefinitions) == 0) + for idx, resource := range resources.RoleDefinitions { + roleDef := tt.roleDefs[idx] + require.Equal(t, *roleDef.ID, resource.Id) + require.Equal(t, fetcher.SubscriptionID, resource.SubscriptionId) + require.Equal(t, *roleDef.Properties.RoleName, resource.Name) + require.Len(t, roleDef.Properties.Permissions, len(resource.Permissions)) + } + require.Equal(t, tt.feats.RoleAssignments == false || len(tt.roleAssigns) == 0, len(resources.RoleAssignments) == 0) + for idx, resource := range resources.RoleAssignments { + roleAssign := tt.roleAssigns[idx] + require.Equal(t, *roleAssign.ID, resource.Id) + require.Equal(t, fetcher.SubscriptionID, resource.SubscriptionId) + require.Equal(t, *roleAssign.Properties.PrincipalID, resource.PrincipalId) + require.Equal(t, *roleAssign.Properties.RoleDefinitionID, resource.RoleDefinitionId) + require.Equal(t, *roleAssign.Properties.Scope, resource.Scope) + } + require.Equal(t, tt.feats.VirtualMachines == false || len(tt.vms) == 0, len(resources.VirtualMachines) == 0) + for idx, resource := range resources.VirtualMachines { + vm := tt.vms[idx] + require.Equal(t, *vm.ID, resource.Id) + require.Equal(t, fetcher.SubscriptionID, resource.SubscriptionId) + require.Equal(t, *vm.Name, resource.Name) + } + } +} From d6e92a42cd27818912e490a155b651a18871b7c0 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 18 Dec 2024 15:25:19 -0600 Subject: [PATCH 21/41] Protobuf update --- api/types/types.pb.go | 34 ---------------------------------- 1 file changed, 34 deletions(-) diff --git a/api/types/types.pb.go b/api/types/types.pb.go index 8883bd2fe0a4a..a49edaacc6d5b 100644 --- a/api/types/types.pb.go +++ b/api/types/types.pb.go @@ -135137,40 +135137,6 @@ func (m *AccessGraphSync) Unmarshal(dAtA []byte) error { return err } iNdEx = postIndex - case 3: - if wireType != 2 { - return fmt.Errorf("proto: wrong wireType = %d for field Azure", wireType) - } - var msglen int - for shift := uint(0); ; shift += 7 { - if shift >= 64 { - return ErrIntOverflowTypes - } - if iNdEx >= l { - return io.ErrUnexpectedEOF - } - b := dAtA[iNdEx] - iNdEx++ - msglen |= int(b&0x7F) << shift - if b < 0x80 { - break - } - } - if msglen < 0 { - return ErrInvalidLengthTypes - } - postIndex := iNdEx + msglen - if postIndex < 0 { - return ErrInvalidLengthTypes - } - if postIndex > l { - return io.ErrUnexpectedEOF - } - m.Azure = append(m.Azure, &AccessGraphAzureSync{}) - if err := m.Azure[len(m.Azure)-1].Unmarshal(dAtA[iNdEx:postIndex]); err != nil { - return err - } - iNdEx = postIndex default: iNdEx = preIndex skippy, err := skipTypes(dAtA[iNdEx:]) From 8c4f1d691f675b6475985ff7d4e389e48d4eb79f Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Thu, 19 Dec 2024 17:54:32 -0600 Subject: [PATCH 22/41] Update sync process to use msgraph client --- lib/srv/discovery/fetchers/azure-sync/azure-sync.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index df9549c71c493..c87a9d6992a27 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -20,6 +20,7 @@ package azure_sync import ( "context" + "github.com/gravitational/teleport/lib/msgraph" "sync" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" @@ -66,6 +67,7 @@ type Fetcher struct { lastDiscoveredResources uint64 lastResult *Resources + graphClient *msgraph.Client roleAssignClient RoleAssignmentsClient roleDefClient RoleDefinitionsClient vmClient VirtualMachinesClient @@ -80,6 +82,9 @@ func NewFetcher(cfg Config, ctx context.Context) (*Fetcher, error) { } // Create the clients + graphClient, err := msgraph.NewClient(msgraph.Config{ + TokenProvider: cred, + }) roleAssignClient, err := azure.NewRoleAssignmentsClient(cfg.SubscriptionID, cred, nil) if err != nil { return nil, trace.Wrap(err) @@ -97,6 +102,7 @@ func NewFetcher(cfg Config, ctx context.Context) (*Fetcher, error) { return &Fetcher{ Config: cfg, lastResult: &Resources{}, + graphClient: graphClient, roleAssignClient: roleAssignClient, roleDefClient: roleDefClient, vmClient: vmClient, @@ -152,11 +158,7 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) errsCh := make(chan error) if feats.Principals { eg.Go(func() error { - cred, err := a.CloudClients.GetAzureCredential() - if err != nil { - return trace.Wrap(err) - } - principals, err := fetchPrincipals(ctx, a.SubscriptionID, cred) + principals, err := fetchPrincipals(ctx, a.SubscriptionID, a.graphClient) if err != nil { errsCh <- err return err From 224d89c9f67237e0c8267bd0e9726a5ff3feb478 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Tue, 31 Dec 2024 11:37:57 -0600 Subject: [PATCH 23/41] Conformant package name --- lib/srv/discovery/fetchers/azure-sync/azure-sync.go | 2 +- lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go | 2 +- lib/srv/discovery/fetchers/azure-sync/reconcile.go | 2 +- lib/srv/discovery/fetchers/azure-sync/reconcile_test.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index c87a9d6992a27..66b8fe04a9dd2 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -16,7 +16,7 @@ * along with this program. If not, see . */ -package azure_sync +package azuresync import ( "context" diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go index d5c57d06edf1e..a644177af8f87 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go @@ -16,7 +16,7 @@ * along with this program. If not, see . */ -package azure_sync +package azuresync import ( "context" diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile.go b/lib/srv/discovery/fetchers/azure-sync/reconcile.go index 2b54c8cfac911..e8dc5a61c7219 100644 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile.go +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile.go @@ -16,7 +16,7 @@ * along with this program. If not, see . */ -package azure_sync +package azuresync import ( "fmt" diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go index 28b293bcf1f8d..f63460476eaa5 100644 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go @@ -15,7 +15,7 @@ * You should have received a copy of the GNU Affero General Public License * along with this program. If not, see . */ -package azure_sync +package azuresync import ( "testing" From 998673777efb4bce9c14b582594fcf556135aac7 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Mon, 6 Jan 2025 23:16:53 -0600 Subject: [PATCH 24/41] Invoking membership expansion --- lib/srv/discovery/fetchers/azure-sync/azure-sync.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index 66b8fe04a9dd2..efca495cf7f18 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -163,6 +163,11 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) errsCh <- err return err } + err = expandMemberships(ctx, a.graphClient, principals) + if err != nil { + errsCh <- err + return err + } result.Principals = principals return nil }) From e385f8414fc49413e8fdf913ccef48e040a4b482 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Tue, 7 Jan 2025 10:43:48 -0600 Subject: [PATCH 25/41] Setting principals before expansion --- lib/srv/discovery/fetchers/azure-sync/azure-sync.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index efca495cf7f18..17be6423feb74 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -163,12 +163,11 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) errsCh <- err return err } - err = expandMemberships(ctx, a.graphClient, principals) + result.Principals, err = expandMemberships(ctx, a.graphClient, principals) if err != nil { errsCh <- err return err } - result.Principals = principals return nil }) } From 58eccd71791e0bfaf1f33af9f6243748c74f0971 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Thu, 9 Jan 2025 00:35:27 -0600 Subject: [PATCH 26/41] Removing msgraphclient --- .../fetchers/azure-sync/msggraphclient.go | 240 ------------------ 1 file changed, 240 deletions(-) delete mode 100644 lib/srv/discovery/fetchers/azure-sync/msggraphclient.go diff --git a/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go b/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go deleted file mode 100644 index 75d2960d7fa55..0000000000000 --- a/lib/srv/discovery/fetchers/azure-sync/msggraphclient.go +++ /dev/null @@ -1,240 +0,0 @@ -/* - * Teleport - * Copyright (C) 2024 Gravitational, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -package azure_sync - -import ( - "bytes" - "context" - "encoding/json" - "fmt" - "io" - "net/http" - "net/url" - "strings" - "time" - - "github.com/Azure/azure-sdk-for-go/sdk/azcore" -) - -// GraphClient represents generic MS API client -type GraphClient struct { - token azcore.AccessToken -} - -const ( - usersSuffix = "users" - groupsSuffix = "groups" - servicePrincipalsSuffix = "servicePrincipals" - graphBaseURL = "https://graph.microsoft.com/v1.0" - httpTimeout = time.Second * 30 -) - -// graphError represents MS Graph error -type graphError struct { - E struct { - Code string `json:"code"` - Message string `json:"message"` - } `json:"error"` -} - -// genericGraphResponse represents the utility struct for parsing MS Graph API response -type genericGraphResponse struct { - Context string `json:"@odata.context"` - Count int `json:"@odata.count"` - NextLink string `json:"@odata.nextLink"` - Value json.RawMessage `json:"value"` -} - -// User represents user resource -type User struct { - ID string `json:"id"` - Name string `json:"displayName"` - MemberOf []Membership `json:"memberOf"` -} - -type Membership struct { - Type string `json:"@odata.type"` - ID string `json:"id"` -} - -// request represents generic request structure -type request struct { - // Method HTTP method - Method string - // URL which overrides URL construction - URL *string - // Path to a resource - Path string - // Expand $expand value - Expand []string - // Filter $filter value - Filter string - // Body request body - Body string - // Response represents template structure for a response - Response interface{} - // Err represents template structure for an error - Err error - // SuccessCode http code representing success - SuccessCode int -} - -// GetURL builds the request URL -func (r *request) GetURL() (string, error) { - if r.URL != nil { - return *r.URL, nil - } - u, err := url.Parse(graphBaseURL) - if err != nil { - return "", err - } - - data := url.Values{} - if len(r.Expand) > 0 { - data.Set("$expand", strings.Join(r.Expand, ",")) - } - if r.Filter != "" { - data.Set("$filter", r.Filter) - } - - u.Path = u.Path + "/" + r.Path - u.RawQuery = data.Encode() - - return u.String(), nil -} - -// NewGraphClient creates MS Graph API client -func NewGraphClient(token azcore.AccessToken) *GraphClient { - return &GraphClient{ - token: token, - } -} - -// Error returns error string -func (e graphError) Error() string { - return e.E.Code + " " + e.E.Message -} - -func (c *GraphClient) ListUsers(ctx context.Context) ([]User, error) { - return c.listIdentities(ctx, usersSuffix, []string{"memberOf"}) -} - -func (c *GraphClient) ListGroups(ctx context.Context) ([]User, error) { - return c.listIdentities(ctx, groupsSuffix, []string{"memberOf"}) -} - -func (c *GraphClient) ListServicePrincipals(ctx context.Context) ([]User, error) { - return c.listIdentities(ctx, servicePrincipalsSuffix, []string{"memberOf"}) -} - -func (c *GraphClient) listIdentities(ctx context.Context, idType string, expand []string) ([]User, error) { - var users []User - var nextLink *string - for { - g := &genericGraphResponse{} - req := request{ - Method: http.MethodGet, - Path: idType, - Expand: expand, - Response: &g, - Err: &graphError{}, - URL: nextLink, - } - err := c.request(ctx, req) - if err != nil { - return nil, err - } - var newUsers []User - err = json.NewDecoder(bytes.NewReader(g.Value)).Decode(&newUsers) - if err != nil { - return nil, err - } - users = append(users, newUsers...) - if g.NextLink == "" { - break - } - nextLink = &g.NextLink - } - - return users, nil -} - -// request sends the request to the graph/bot service and returns response body as bytes slice -func (c *GraphClient) request(ctx context.Context, req request) error { - reqUrl, err := req.GetURL() - if err != nil { - return err - } - - r, err := http.NewRequestWithContext(ctx, req.Method, reqUrl, strings.NewReader(req.Body)) - if err != nil { - return err - } - - r.Header.Set("Authorization", "Bearer "+c.token.Token) - r.Header.Set("Content-Type", "application/json") - - client := http.Client{Timeout: httpTimeout} - resp, err := client.Do(r) - if err != nil { - return err - } - - defer func(r *http.Response) { - _ = r.Body.Close() - }(resp) - - b, err := io.ReadAll(resp.Body) - if err != nil { - return err - } - - expectedCode := req.SuccessCode - if expectedCode == 0 { - expectedCode = http.StatusOK - } - - if expectedCode == resp.StatusCode { - if req.Response == nil { - return nil - } - - err := json.NewDecoder(bytes.NewReader(b)).Decode(req.Response) - if err != nil { - return err - } - } else { - if req.Err == nil { - return fmt.Errorf("Error requesting MS Graph API: %v", string(b)) - } - - err := json.NewDecoder(bytes.NewReader(b)).Decode(req.Err) - if err != nil { - return err - } - - if req.Err.Error() == "" { - return fmt.Errorf("Error requesting MS Graph API. Expected response code was %v, but is %v", expectedCode, resp.StatusCode) - } - - return req.Err - } - - return nil -} From 4ded09c6d186bd8f4084ac65bd55b795fc1c052b Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Mon, 13 Jan 2025 13:57:43 -0600 Subject: [PATCH 27/41] Update e ref --- e | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e b/e index 498f643ea9033..65fa473e50c72 160000 --- a/e +++ b/e @@ -1 +1 @@ -Subproject commit 498f643ea9033b1235359d83c310caadb18305d2 +Subproject commit 65fa473e50c72d8f79261033a1298cc2955ca15c From f31b34253101e9b9301288d07f5c84f53bacaaca Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Mon, 13 Jan 2025 14:44:41 -0600 Subject: [PATCH 28/41] Linting --- lib/msgraph/paginated.go | 8 -------- lib/srv/discovery/fetchers/azure-sync/azure-sync.go | 5 ++++- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/msgraph/paginated.go b/lib/msgraph/paginated.go index 0321b8a326d9c..a0b9488af9d70 100644 --- a/lib/msgraph/paginated.go +++ b/lib/msgraph/paginated.go @@ -109,14 +109,6 @@ func (c *Client) IterateServicePrincipals(ctx context.Context, f func(principal return iterateSimple(c, ctx, "servicePrincipals", f) } -// IterateUserMemberships lists all group memberships for a given user ID as directory objects. -// `f` will be called for each directory object in the result set. -// if `f` returns `false`, the iteration is stopped (equivalent to `break` in a normal loop). -// Ref: [https://learn.microsoft.com/en-us/graph/api/group-list-memberof]. -func (c *Client) IterateUserMemberships(ctx context.Context, userID string, f func(object *DirectoryObject) bool) error { - return iterateSimple(c, ctx, path.Join("users", userID, "memberOf"), f) -} - // IterateGroupMembers lists all members for the given Entra ID group using pagination. // `f` will be called for each object in the result set. // if `f` returns `false`, the iteration is stopped (equivalent to `break` in a normal loop). diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index 17be6423feb74..59b9abbf2427d 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -20,7 +20,6 @@ package azuresync import ( "context" - "github.com/gravitational/teleport/lib/msgraph" "sync" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" @@ -30,6 +29,7 @@ import ( accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" "github.com/gravitational/teleport/lib/cloud" "github.com/gravitational/teleport/lib/cloud/azure" + "github.com/gravitational/teleport/lib/msgraph" "github.com/gravitational/teleport/lib/srv/discovery/common" ) @@ -85,6 +85,9 @@ func NewFetcher(cfg Config, ctx context.Context) (*Fetcher, error) { graphClient, err := msgraph.NewClient(msgraph.Config{ TokenProvider: cred, }) + if err != nil { + return nil, trace.Wrap(err) + } roleAssignClient, err := azure.NewRoleAssignmentsClient(cfg.SubscriptionID, cred, nil) if err != nil { return nil, trace.Wrap(err) From 9d139eda7b76a8884ec845d5b27192ce98e44cf9 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 15 Jan 2025 14:47:07 -0600 Subject: [PATCH 29/41] PR feedback --- lib/srv/discovery/common/reconcile.go | 32 ------ .../fetchers/azure-sync/azure-sync.go | 42 +++----- .../fetchers/azure-sync/azure-sync_test.go | 98 ++++++++++--------- .../fetchers/azure-sync/reconcile.go | 14 +-- .../fetchers/azure-sync/reconcile_test.go | 2 +- lib/utils/slice.go | 13 +++ 6 files changed, 85 insertions(+), 116 deletions(-) delete mode 100644 lib/srv/discovery/common/reconcile.go diff --git a/lib/srv/discovery/common/reconcile.go b/lib/srv/discovery/common/reconcile.go deleted file mode 100644 index 9161ae69b95b1..0000000000000 --- a/lib/srv/discovery/common/reconcile.go +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Teleport - * Copyright (C) 2024 Gravitational, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -package common - -func DeduplicateSlice[T any](s []T, key func(T) string) []T { - out := make([]T, 0, len(s)) - seen := make(map[string]struct{}) - for _, v := range s { - if _, ok := seen[key(v)]; ok { - continue - } - seen[key(v)] = struct{}{} - out = append(out, v) - } - return out -} diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index 59b9abbf2427d..3979e154dbf18 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -1,6 +1,6 @@ /* * Teleport - * Copyright (C) 2024 Gravitational, Inc. + * Copyright (C) 2025 Gravitational, Inc. * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as published by @@ -20,7 +20,6 @@ package azuresync import ( "context" - "sync" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/gravitational/trace" @@ -30,7 +29,7 @@ import ( "github.com/gravitational/teleport/lib/cloud" "github.com/gravitational/teleport/lib/cloud/azure" "github.com/gravitational/teleport/lib/msgraph" - "github.com/gravitational/teleport/lib/srv/discovery/common" + "github.com/gravitational/teleport/lib/utils" ) const ( @@ -81,7 +80,7 @@ func NewFetcher(cfg Config, ctx context.Context) (*Fetcher, error) { return nil, trace.Wrap(err) } - // Create the clients + // Create the clients for the fetcher graphClient, err := msgraph.NewClient(msgraph.Config{ TokenProvider: cred, }) @@ -101,7 +100,6 @@ func NewFetcher(cfg Config, ctx context.Context) (*Fetcher, error) { return nil, trace.Wrap(err) } - // Create and return the fetcher return &Fetcher{ Config: cfg, lastResult: &Resources{}, @@ -144,10 +142,10 @@ func (a *Fetcher) Poll(ctx context.Context, feats Features) (*Resources, error) if res == nil { return nil, err } - res.Principals = common.DeduplicateSlice(res.Principals, azurePrincipalsKey) - res.RoleAssignments = common.DeduplicateSlice(res.RoleAssignments, azureRoleAssignKey) - res.RoleDefinitions = common.DeduplicateSlice(res.RoleDefinitions, azureRoleDefKey) - res.VirtualMachines = common.DeduplicateSlice(res.VirtualMachines, azureVmKey) + res.Principals = utils.DeduplicateSlice(res.Principals, azurePrincipalsKey) + res.RoleAssignments = utils.DeduplicateSlice(res.RoleAssignments, azureRoleAssignKey) + res.RoleDefinitions = utils.DeduplicateSlice(res.RoleDefinitions, azureRoleDefKey) + res.VirtualMachines = utils.DeduplicateSlice(res.VirtualMachines, azureVmKey) return res, trace.Wrap(err) } @@ -157,7 +155,6 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) eg, ctx := errgroup.WithContext(ctx) eg.SetLimit(FetcherConcurrency) var result = &Resources{} - var errs []error errsCh := make(chan error) if feats.Principals { eg.Go(func() error { @@ -166,11 +163,12 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) errsCh <- err return err } - result.Principals, err = expandMemberships(ctx, a.graphClient, principals) + principals, err = expandMemberships(ctx, a.graphClient, principals) if err != nil { errsCh <- err return err } + result.Principals = principals return nil }) } @@ -208,28 +206,10 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) }) } - // Collect the error messages from the error channel - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - for { - err, ok := <-errsCh - if !ok { - return - } - errs = append(errs, err) - } - }() + // Return the result along with any errors collected _ = eg.Wait() close(errsCh) - wg.Wait() - if len(errs) > 0 { - return result, trace.NewAggregate(errs...) - } - - // Return the resources - return result, nil + return result, trace.NewAggregateFromChannel(errsCh, ctx) } // Status returns the number of resources last fetched and/or the last fetching/reconciling error diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go index a644177af8f87..9fa723e66d3e7 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go @@ -1,6 +1,6 @@ /* * Teleport - * Copyright (C) 2024 Gravitational, Inc. + * Copyright (C) 2025 Gravitational, Inc. * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as published by @@ -138,6 +138,7 @@ func TestPoll(t *testing.T) { noVmsFeats.VirtualMachines = false tests := []struct { + name string returnErr bool roleDefs []*armauthorization.RoleDefinition roleAssigns []*armauthorization.RoleAssignment @@ -146,6 +147,7 @@ func TestPoll(t *testing.T) { }{ // Process no results from clients { + name: "WithoutResults", returnErr: false, roleDefs: []*armauthorization.RoleDefinition{}, roleAssigns: []*armauthorization.RoleAssignment{}, @@ -154,6 +156,7 @@ func TestPoll(t *testing.T) { }, // Process test results from clients { + name: "WithResults", returnErr: false, roleDefs: roleDefs, roleAssigns: roleAssigns, @@ -162,6 +165,7 @@ func TestPoll(t *testing.T) { }, // Handle errors from clients { + name: "PollErrors", returnErr: true, roleDefs: roleDefs, roleAssigns: roleAssigns, @@ -170,6 +174,7 @@ func TestPoll(t *testing.T) { }, // Handle VM features being disabled { + name: "NoVmsFeats", returnErr: false, roleDefs: roleDefs, roleAssigns: roleAssigns, @@ -179,49 +184,52 @@ func TestPoll(t *testing.T) { } for _, tt := range tests { - // Set the test data - roleDefClient.returnErr = tt.returnErr - roleDefClient.roleDefs = tt.roleDefs - roleAssignClient.returnErr = tt.returnErr - roleAssignClient.roleAssigns = tt.roleAssigns - vmClient.returnErr = tt.returnErr - vmClient.vms = tt.vms - - // Poll for resources - resources, err := fetcher.Poll(ctx, tt.feats) - - // Require no error unless otherwise specified - if tt.returnErr { - require.Error(t, err) - continue - } - require.NoError(t, err) - - // Verify the results, based on the features set - require.NotNil(t, resources) - require.Equal(t, tt.feats.RoleDefinitions == false || len(tt.roleDefs) == 0, len(resources.RoleDefinitions) == 0) - for idx, resource := range resources.RoleDefinitions { - roleDef := tt.roleDefs[idx] - require.Equal(t, *roleDef.ID, resource.Id) - require.Equal(t, fetcher.SubscriptionID, resource.SubscriptionId) - require.Equal(t, *roleDef.Properties.RoleName, resource.Name) - require.Len(t, roleDef.Properties.Permissions, len(resource.Permissions)) - } - require.Equal(t, tt.feats.RoleAssignments == false || len(tt.roleAssigns) == 0, len(resources.RoleAssignments) == 0) - for idx, resource := range resources.RoleAssignments { - roleAssign := tt.roleAssigns[idx] - require.Equal(t, *roleAssign.ID, resource.Id) - require.Equal(t, fetcher.SubscriptionID, resource.SubscriptionId) - require.Equal(t, *roleAssign.Properties.PrincipalID, resource.PrincipalId) - require.Equal(t, *roleAssign.Properties.RoleDefinitionID, resource.RoleDefinitionId) - require.Equal(t, *roleAssign.Properties.Scope, resource.Scope) - } - require.Equal(t, tt.feats.VirtualMachines == false || len(tt.vms) == 0, len(resources.VirtualMachines) == 0) - for idx, resource := range resources.VirtualMachines { - vm := tt.vms[idx] - require.Equal(t, *vm.ID, resource.Id) - require.Equal(t, fetcher.SubscriptionID, resource.SubscriptionId) - require.Equal(t, *vm.Name, resource.Name) - } + t.Run(tt.name, func(t *testing.T) { + // Set the test data + roleDefClient.returnErr = tt.returnErr + roleDefClient.roleDefs = tt.roleDefs + roleAssignClient.returnErr = tt.returnErr + roleAssignClient.roleAssigns = tt.roleAssigns + vmClient.returnErr = tt.returnErr + vmClient.vms = tt.vms + + // Poll for resources + resources, err := fetcher.Poll(ctx, tt.feats) + + // Require no error unless otherwise specified + if tt.returnErr { + require.Error(t, err) + return + } + require.NoError(t, err) + + // Verify the results, based on the features set + require.NotNil(t, resources) + require.Equal(t, tt.feats.RoleDefinitions == false || len(tt.roleDefs) == 0, len(resources.RoleDefinitions) == 0) + for idx, resource := range resources.RoleDefinitions { + roleDef := tt.roleDefs[idx] + require.Equal(t, *roleDef.ID, resource.Id) + require.Equal(t, fetcher.SubscriptionID, resource.SubscriptionId) + require.Equal(t, *roleDef.Properties.RoleName, resource.Name) + require.Len(t, roleDef.Properties.Permissions, len(resource.Permissions)) + } + require.Equal(t, tt.feats.RoleAssignments == false || len(tt.roleAssigns) == 0, len(resources.RoleAssignments) == 0) + for idx, resource := range resources.RoleAssignments { + roleAssign := tt.roleAssigns[idx] + require.Equal(t, *roleAssign.ID, resource.Id) + require.Equal(t, fetcher.SubscriptionID, resource.SubscriptionId) + require.Equal(t, *roleAssign.Properties.PrincipalID, resource.PrincipalId) + require.Equal(t, *roleAssign.Properties.RoleDefinitionID, resource.RoleDefinitionId) + require.Equal(t, *roleAssign.Properties.Scope, resource.Scope) + } + require.Equal(t, tt.feats.VirtualMachines == false || len(tt.vms) == 0, len(resources.VirtualMachines) == 0) + for idx, resource := range resources.VirtualMachines { + vm := tt.vms[idx] + require.Equal(t, *vm.ID, resource.Id) + require.Equal(t, fetcher.SubscriptionID, resource.SubscriptionId) + require.Equal(t, *vm.Name, resource.Name) + } + + }) } } diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile.go b/lib/srv/discovery/fetchers/azure-sync/reconcile.go index e8dc5a61c7219..ee31d6a06a149 100644 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile.go +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile.go @@ -1,6 +1,6 @@ /* * Teleport - * Copyright (C) 2024 Gravitational, Inc. + * Copyright (C) 2025 Gravitational, Inc. * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as published by @@ -24,7 +24,7 @@ import ( "google.golang.org/protobuf/proto" accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" - "github.com/gravitational/teleport/lib/srv/discovery/common" + "github.com/gravitational/teleport/lib/utils" ) // MergeResources merges Azure resources fetched from multiple configured Azure fetchers @@ -42,10 +42,10 @@ func MergeResources(results ...*Resources) *Resources { result.RoleDefinitions = append(result.RoleDefinitions, r.RoleDefinitions...) result.VirtualMachines = append(result.VirtualMachines, r.VirtualMachines...) } - result.Principals = common.DeduplicateSlice(result.Principals, azurePrincipalsKey) - result.RoleAssignments = common.DeduplicateSlice(result.RoleAssignments, azureRoleAssignKey) - result.RoleDefinitions = common.DeduplicateSlice(result.RoleDefinitions, azureRoleDefKey) - result.VirtualMachines = common.DeduplicateSlice(result.VirtualMachines, azureVmKey) + result.Principals = utils.DeduplicateSlice(result.Principals, azurePrincipalsKey) + result.RoleAssignments = utils.DeduplicateSlice(result.RoleAssignments, azureRoleAssignKey) + result.RoleDefinitions = utils.DeduplicateSlice(result.RoleDefinitions, azureRoleDefKey) + result.VirtualMachines = utils.DeduplicateSlice(result.VirtualMachines, azureVmKey) return result } @@ -86,7 +86,7 @@ func reconcile[T proto.Message]( wrapFn func(T) *accessgraphv1alpha.AzureResource, ) *reconcilePair { // Remove duplicates from the new items - newItems = common.DeduplicateSlice(newItems, keyFn) + newItems = utils.DeduplicateSlice(newItems, keyFn) upsertRes := newResourceList() deleteRes := newResourceList() diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go index f63460476eaa5..ddf8524bc6a18 100644 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go @@ -1,6 +1,6 @@ /* * Teleport - * Copyright (C) 2024 Gravitational, Inc. + * Copyright (C) 2025 Gravitational, Inc. * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as published by diff --git a/lib/utils/slice.go b/lib/utils/slice.go index 4009277ceef50..8cabb489d037c 100644 --- a/lib/utils/slice.go +++ b/lib/utils/slice.go @@ -142,3 +142,16 @@ func FromSlice[T any](r []T, key func(T) string) map[string]T { return out } + +func DeduplicateSlice[T any](s []T, key func(T) string) []T { + out := make([]T, 0, len(s)) + seen := make(map[string]struct{}) + for _, v := range s { + if _, ok := seen[key(v)]; ok { + continue + } + seen[key(v)] = struct{}{} + out = append(out, v) + } + return out +} From 29121a21be85d255219cb224038545b719820de4 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 15 Jan 2025 14:50:52 -0600 Subject: [PATCH 30/41] Adding test names to reconciliation tests --- .../discovery/fetchers/azure-sync/reconcile_test.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go index ddf8524bc6a18..3652c4963218c 100644 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile_test.go @@ -32,6 +32,7 @@ func TestReconcileResults(t *testing.T) { vms := generateVms() tests := []struct { + name string oldResults *Resources newResults *Resources expectedUpserts *accessgraphv1alpha.AzureResourceList @@ -39,6 +40,7 @@ func TestReconcileResults(t *testing.T) { }{ // Overlapping old and new results { + name: "OverlapOldAndNewResults", oldResults: &Resources{ Principals: principals[0:2], RoleDefinitions: roleDefs[0:2], @@ -56,6 +58,7 @@ func TestReconcileResults(t *testing.T) { }, // Completely new results { + name: "CompletelyNewResults", oldResults: &Resources{ Principals: nil, RoleDefinitions: nil, @@ -73,6 +76,7 @@ func TestReconcileResults(t *testing.T) { }, // No new results { + name: "NoNewResults", oldResults: &Resources{ Principals: principals[1:3], RoleDefinitions: roleDefs[1:3], @@ -91,9 +95,11 @@ func TestReconcileResults(t *testing.T) { } for _, tt := range tests { - upserts, deletes := ReconcileResults(tt.oldResults, tt.newResults) - require.ElementsMatch(t, upserts.Resources, tt.expectedUpserts.Resources) - require.ElementsMatch(t, deletes.Resources, tt.expectedDeletes.Resources) + t.Run(tt.name, func(t *testing.T) { + upserts, deletes := ReconcileResults(tt.oldResults, tt.newResults) + require.ElementsMatch(t, upserts.Resources, tt.expectedUpserts.Resources) + require.ElementsMatch(t, deletes.Resources, tt.expectedDeletes.Resources) + }) } } From 10c5042c90a2ca554cc31a788a0d7b48de9937de Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 15 Jan 2025 15:25:14 -0600 Subject: [PATCH 31/41] Adding channel buffer --- lib/srv/discovery/fetchers/azure-sync/azure-sync.go | 3 ++- lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index 3979e154dbf18..55ef0916d6003 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -38,6 +38,7 @@ const ( featNameRoleAssignments = "azure/roleassignments" featNameVms = "azure/virtualmachines" ) +const numFeatures = 4 // FetcherConcurrency is an arbitrary per-resource type concurrency to ensure significant throughput. As we increase // the number of resource types, we may increase this value or use some other approach to fetching concurrency. @@ -155,7 +156,7 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) eg, ctx := errgroup.WithContext(ctx) eg.SetLimit(FetcherConcurrency) var result = &Resources{} - errsCh := make(chan error) + errsCh := make(chan error, 4) if feats.Principals { eg.Go(func() error { principals, err := fetchPrincipals(ctx, a.SubscriptionID, a.graphClient) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go index 9fa723e66d3e7..05b73f64da95f 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go @@ -229,7 +229,6 @@ func TestPoll(t *testing.T) { require.Equal(t, fetcher.SubscriptionID, resource.SubscriptionId) require.Equal(t, *vm.Name, resource.Name) } - }) } } From 863ec3dfde09128181886d81c7934cfc34ffb3e3 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 15 Jan 2025 21:31:35 -0600 Subject: [PATCH 32/41] Going back to just reading from channel --- .../fetchers/azure-sync/azure-sync.go | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index 55ef0916d6003..7aab57e5d6b61 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -20,7 +20,6 @@ package azuresync import ( "context" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/gravitational/trace" "golang.org/x/sync/errgroup" @@ -38,7 +37,6 @@ const ( featNameRoleAssignments = "azure/roleassignments" featNameVms = "azure/virtualmachines" ) -const numFeatures = 4 // FetcherConcurrency is an arbitrary per-resource type concurrency to ensure significant throughput. As we increase // the number of resource types, we may increase this value or use some other approach to fetching concurrency. @@ -156,18 +154,18 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) eg, ctx := errgroup.WithContext(ctx) eg.SetLimit(FetcherConcurrency) var result = &Resources{} - errsCh := make(chan error, 4) + errsCh := make(chan error, FetcherConcurrency) if feats.Principals { eg.Go(func() error { principals, err := fetchPrincipals(ctx, a.SubscriptionID, a.graphClient) if err != nil { errsCh <- err - return err + return nil } principals, err = expandMemberships(ctx, a.graphClient, principals) if err != nil { errsCh <- err - return err + return nil } result.Principals = principals return nil @@ -178,7 +176,7 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) roleAssigns, err := fetchRoleAssignments(ctx, a.SubscriptionID, a.roleAssignClient) if err != nil { errsCh <- err - return err + return nil } result.RoleAssignments = roleAssigns return nil @@ -189,7 +187,7 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) roleDefs, err := fetchRoleDefinitions(ctx, a.SubscriptionID, a.roleDefClient) if err != nil { errsCh <- err - return err + return nil } result.RoleDefinitions = roleDefs return nil @@ -200,7 +198,7 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) vms, err := fetchVirtualMachines(ctx, a.SubscriptionID, a.vmClient) if err != nil { errsCh <- err - return err + return nil } result.VirtualMachines = vms return nil @@ -210,7 +208,11 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) // Return the result along with any errors collected _ = eg.Wait() close(errsCh) - return result, trace.NewAggregateFromChannel(errsCh, ctx) + var errs []error + for err := range errsCh { + errs = append(errs, err) + } + return result, trace.NewAggregate(errs...) } // Status returns the number of resources last fetched and/or the last fetching/reconciling error From 096fe320be9fa8c0bbac8446cd65c5e5999d7273 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 15 Jan 2025 21:43:55 -0600 Subject: [PATCH 33/41] Linting --- lib/srv/discovery/fetchers/azure-sync/azure-sync.go | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index 7aab57e5d6b61..a7c568b338cda 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -20,6 +20,7 @@ package azuresync import ( "context" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/gravitational/trace" "golang.org/x/sync/errgroup" From a5a45027ae4c17b29f4ba6341bcb23cf58406aab Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Thu, 16 Jan 2025 17:05:25 -0600 Subject: [PATCH 34/41] PR feedback --- .../fetchers/azure-sync/azure-sync.go | 18 ++-- .../fetchers/azure-sync/reconcile.go | 10 +-- lib/utils/slice.go | 3 +- lib/utils/slice_test.go | 90 +++++++++++++++++++ 4 files changed, 106 insertions(+), 15 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index a7c568b338cda..534d977a0b1eb 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -39,9 +39,9 @@ const ( featNameVms = "azure/virtualmachines" ) -// FetcherConcurrency is an arbitrary per-resource type concurrency to ensure significant throughput. As we increase +// fetcherConcurrency is an arbitrary per-resource type concurrency to ensure significant throughput. As we increase // the number of resource types, we may increase this value or use some other approach to fetching concurrency. -const FetcherConcurrency = 4 +const fetcherConcurrency = 4 // Config defines parameters required for fetching resources from Azure type Config struct { @@ -140,12 +140,12 @@ func BuildFeatures(values ...string) Features { func (a *Fetcher) Poll(ctx context.Context, feats Features) (*Resources, error) { res, err := a.fetch(ctx, feats) if res == nil { - return nil, err + return nil, trace.Wrap(err) } - res.Principals = utils.DeduplicateSlice(res.Principals, azurePrincipalsKey) - res.RoleAssignments = utils.DeduplicateSlice(res.RoleAssignments, azureRoleAssignKey) - res.RoleDefinitions = utils.DeduplicateSlice(res.RoleDefinitions, azureRoleDefKey) - res.VirtualMachines = utils.DeduplicateSlice(res.VirtualMachines, azureVmKey) + res.Principals = utils.DeduplicateKey(res.Principals, azurePrincipalsKey) + res.RoleAssignments = utils.DeduplicateKey(res.RoleAssignments, azureRoleAssignKey) + res.RoleDefinitions = utils.DeduplicateKey(res.RoleDefinitions, azureRoleDefKey) + res.VirtualMachines = utils.DeduplicateKey(res.VirtualMachines, azureVmKey) return res, trace.Wrap(err) } @@ -153,9 +153,9 @@ func (a *Fetcher) Poll(ctx context.Context, feats Features) (*Resources, error) func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) { // Accumulate Azure resources eg, ctx := errgroup.WithContext(ctx) - eg.SetLimit(FetcherConcurrency) + eg.SetLimit(fetcherConcurrency) var result = &Resources{} - errsCh := make(chan error, FetcherConcurrency) + errsCh := make(chan error, fetcherConcurrency) if feats.Principals { eg.Go(func() error { principals, err := fetchPrincipals(ctx, a.SubscriptionID, a.graphClient) diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile.go b/lib/srv/discovery/fetchers/azure-sync/reconcile.go index ee31d6a06a149..0f6d78da6d600 100644 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile.go +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile.go @@ -42,10 +42,10 @@ func MergeResources(results ...*Resources) *Resources { result.RoleDefinitions = append(result.RoleDefinitions, r.RoleDefinitions...) result.VirtualMachines = append(result.VirtualMachines, r.VirtualMachines...) } - result.Principals = utils.DeduplicateSlice(result.Principals, azurePrincipalsKey) - result.RoleAssignments = utils.DeduplicateSlice(result.RoleAssignments, azureRoleAssignKey) - result.RoleDefinitions = utils.DeduplicateSlice(result.RoleDefinitions, azureRoleDefKey) - result.VirtualMachines = utils.DeduplicateSlice(result.VirtualMachines, azureVmKey) + result.Principals = utils.DeduplicateKey(result.Principals, azurePrincipalsKey) + result.RoleAssignments = utils.DeduplicateKey(result.RoleAssignments, azureRoleAssignKey) + result.RoleDefinitions = utils.DeduplicateKey(result.RoleDefinitions, azureRoleDefKey) + result.VirtualMachines = utils.DeduplicateKey(result.VirtualMachines, azureVmKey) return result } @@ -86,7 +86,7 @@ func reconcile[T proto.Message]( wrapFn func(T) *accessgraphv1alpha.AzureResource, ) *reconcilePair { // Remove duplicates from the new items - newItems = utils.DeduplicateSlice(newItems, keyFn) + newItems = utils.DeduplicateKey(newItems, keyFn) upsertRes := newResourceList() deleteRes := newResourceList() diff --git a/lib/utils/slice.go b/lib/utils/slice.go index 8cabb489d037c..8b4f73b9543d5 100644 --- a/lib/utils/slice.go +++ b/lib/utils/slice.go @@ -143,7 +143,8 @@ func FromSlice[T any](r []T, key func(T) string) map[string]T { return out } -func DeduplicateSlice[T any](s []T, key func(T) string) []T { +// DeduplicateKey returns a deduplicated slice by comparing key values from the key function +func DeduplicateKey[T any](s []T, key func(T) string) []T { out := make([]T, 0, len(s)) seen := make(map[string]struct{}) for _, v := range s { diff --git a/lib/utils/slice_test.go b/lib/utils/slice_test.go index 76f4a1dd1a29d..fce1f113025d1 100644 --- a/lib/utils/slice_test.go +++ b/lib/utils/slice_test.go @@ -19,6 +19,7 @@ package utils import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -41,3 +42,92 @@ func TestSlice(t *testing.T) { pool.Put(slice) } } + +// TestDuplicateKey tests slice deduplication via key function +func TestDeduplicateKey(t *testing.T) { + t.Parallel() + + stringTests := []struct { + name string + slice []string + keyFn func(string) string + expected []string + }{ + { + name: "EmptyStringSlice", + slice: []string{}, + keyFn: func(s string) string { return s }, + expected: []string{}, + }, + { + name: "NoStringDuplicates", + slice: []string{"foo", "bar", "baz"}, + keyFn: func(s string) string { return s }, + expected: []string{"foo", "bar", "baz"}, + }, + { + name: "StringDuplicates", + slice: []string{"foo", "bar", "bar", "bar", "baz", "baz"}, + keyFn: func(s string) string { return s }, + expected: []string{"foo", "bar", "baz"}, + }, + { + name: "StringDuplicatesWeirdKeyFn", + slice: []string{"foo", "bar", "bar", "bar", "baz", "baz"}, + keyFn: func(s string) string { return "huh" }, + expected: []string{"foo"}, + }, + } + for _, tt := range stringTests { + t.Run(tt.name, func(t *testing.T) { + res := DeduplicateKey(tt.slice, tt.keyFn) + require.Equal(t, tt.expected, res) + }) + } + + type dedupeStruct struct { + a string + b int + c bool + } + dedupeStructKeyFn := func(d dedupeStruct) string { return fmt.Sprintf("%s:%d:%v", d.a, d.b, d.c) } + structTests := []struct { + name string + slice []dedupeStruct + keyFn func(d dedupeStruct) string + expected []dedupeStruct + }{ + { + name: "EmptySlice", + slice: []dedupeStruct{}, + keyFn: dedupeStructKeyFn, + expected: []dedupeStruct{}, + }, + { + name: "NoStructDuplicates", + slice: []dedupeStruct{ + {a: "foo", b: 1, c: true}, + {a: "foo", b: 1, c: false}, + {a: "foo", b: 2, c: true}, + {a: "bar", b: 1, c: true}, + {a: "bar", b: 1, c: false}, + {a: "bar", b: 2, c: true}, + }, + keyFn: dedupeStructKeyFn, + expected: []dedupeStruct{ + {a: "foo", b: 1, c: true}, + {a: "foo", b: 1, c: false}, + {a: "foo", b: 2, c: true}, + {a: "bar", b: 1, c: true}, + {a: "bar", b: 1, c: false}, + {a: "bar", b: 2, c: true}, + }, + }, + } + for _, tt := range structTests { + t.Run(tt.name, func(t *testing.T) { + res := DeduplicateKey(tt.slice, tt.keyFn) + require.Equal(t, tt.expected, res) + }) + } +} From 6174724257cfdf2a38d098d7203a5e2a3de25173 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Thu, 16 Jan 2025 17:26:04 -0600 Subject: [PATCH 35/41] PR feedback --- .../fetchers/azure-sync/azure-sync.go | 65 +++++++++++-------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index 534d977a0b1eb..e948faee1499e 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -26,7 +26,6 @@ import ( "golang.org/x/sync/errgroup" accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" - "github.com/gravitational/teleport/lib/cloud" "github.com/gravitational/teleport/lib/cloud/azure" "github.com/gravitational/teleport/lib/msgraph" "github.com/gravitational/teleport/lib/utils" @@ -45,31 +44,45 @@ const fetcherConcurrency = 4 // Config defines parameters required for fetching resources from Azure type Config struct { - CloudClients cloud.Clients - SubscriptionID string - Integration string + // SubscriptionID is the Azure subscriptipn ID + SubscriptionID string + // Integration is the name of the associated Teleport integration + Integration string + // DiscoveryConfigName is the name of this Discovery configuration DiscoveryConfigName string } // Resources represents the set of resources fetched from Azure type Resources struct { - Principals []*accessgraphv1alpha.AzurePrincipal + // Principals are Azure users, groups, and service principals + Principals []*accessgraphv1alpha.AzurePrincipal + // RoleDefinitions are Azure role definitions RoleDefinitions []*accessgraphv1alpha.AzureRoleDefinition + // RoleAssignments are Azure role assignments RoleAssignments []*accessgraphv1alpha.AzureRoleAssignment + // VirtualMachines are Azure virtual machines VirtualMachines []*accessgraphv1alpha.AzureVirtualMachine } // Fetcher provides the functionality for fetching resources from Azure type Fetcher struct { + // Config is the configuration values for this fetcher Config - lastError error + // lastError is the last error returned from polling + lastError error + // lastDiscoveredResources is the number of resources last returned from polling lastDiscoveredResources uint64 - lastResult *Resources + // lastResult is the last set of resources returned from polling + lastResult *Resources - graphClient *msgraph.Client + // graphClient is the MS graph client for fetching principals + graphClient *msgraph.Client + // roleAssignClient is the Azure client for fetching role assignments roleAssignClient RoleAssignmentsClient - roleDefClient RoleDefinitionsClient - vmClient VirtualMachinesClient + // roleDefClient is the Azure client for fetching role definitions + roleDefClient RoleDefinitionsClient + // vmClient is the Azure client for fetching virtual machines + vmClient VirtualMachinesClient } // NewFetcher returns a new fetcher based on configuration parameters @@ -137,8 +150,8 @@ func BuildFeatures(values ...string) Features { } // Poll fetches and deduplicates Azure resources specified by the Access Graph -func (a *Fetcher) Poll(ctx context.Context, feats Features) (*Resources, error) { - res, err := a.fetch(ctx, feats) +func (f *Fetcher) Poll(ctx context.Context, feats Features) (*Resources, error) { + res, err := f.fetch(ctx, feats) if res == nil { return nil, trace.Wrap(err) } @@ -150,7 +163,7 @@ func (a *Fetcher) Poll(ctx context.Context, feats Features) (*Resources, error) } // fetch returns the resources specified by the Access Graph -func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) { +func (f *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) { // Accumulate Azure resources eg, ctx := errgroup.WithContext(ctx) eg.SetLimit(fetcherConcurrency) @@ -158,12 +171,12 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) errsCh := make(chan error, fetcherConcurrency) if feats.Principals { eg.Go(func() error { - principals, err := fetchPrincipals(ctx, a.SubscriptionID, a.graphClient) + principals, err := fetchPrincipals(ctx, f.SubscriptionID, f.graphClient) if err != nil { errsCh <- err return nil } - principals, err = expandMemberships(ctx, a.graphClient, principals) + principals, err = expandMemberships(ctx, f.graphClient, principals) if err != nil { errsCh <- err return nil @@ -174,7 +187,7 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) } if feats.RoleAssignments { eg.Go(func() error { - roleAssigns, err := fetchRoleAssignments(ctx, a.SubscriptionID, a.roleAssignClient) + roleAssigns, err := fetchRoleAssignments(ctx, f.SubscriptionID, f.roleAssignClient) if err != nil { errsCh <- err return nil @@ -185,7 +198,7 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) } if feats.RoleDefinitions { eg.Go(func() error { - roleDefs, err := fetchRoleDefinitions(ctx, a.SubscriptionID, a.roleDefClient) + roleDefs, err := fetchRoleDefinitions(ctx, f.SubscriptionID, f.roleDefClient) if err != nil { errsCh <- err return nil @@ -196,7 +209,7 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) } if feats.VirtualMachines { eg.Go(func() error { - vms, err := fetchVirtualMachines(ctx, a.SubscriptionID, a.vmClient) + vms, err := fetchVirtualMachines(ctx, f.SubscriptionID, f.vmClient) if err != nil { errsCh <- err return nil @@ -217,21 +230,21 @@ func (a *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) } // Status returns the number of resources last fetched and/or the last fetching/reconciling error -func (a *Fetcher) Status() (uint64, error) { - return a.lastDiscoveredResources, a.lastError +func (f *Fetcher) Status() (uint64, error) { + return f.lastDiscoveredResources, f.lastError } // DiscoveryConfigName returns the name of the configured discovery -func (a *Fetcher) DiscoveryConfigName() string { - return a.Config.DiscoveryConfigName +func (f *Fetcher) DiscoveryConfigName() string { + return f.Config.DiscoveryConfigName } // IsFromDiscoveryConfig returns whether the discovery is from configuration or dynamic -func (a *Fetcher) IsFromDiscoveryConfig() bool { - return a.Config.DiscoveryConfigName != "" +func (f *Fetcher) IsFromDiscoveryConfig() bool { + return f.Config.DiscoveryConfigName != "" } // GetSubscriptionID returns the ID of the Azure subscription -func (a *Fetcher) GetSubscriptionID() string { - return a.Config.SubscriptionID +func (f *Fetcher) GetSubscriptionID() string { + return f.Config.SubscriptionID } From 7ae825e282ebb9589d72b97527a6877a285c654f Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Tue, 21 Jan 2025 10:53:48 -0600 Subject: [PATCH 36/41] PR feedback --- .../fetchers/azure-sync/azure-sync.go | 43 +++++++++++++------ 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index e948faee1499e..1121732753230 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -20,6 +20,7 @@ package azuresync import ( "context" + "sync" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/gravitational/trace" @@ -31,13 +32,6 @@ import ( "github.com/gravitational/teleport/lib/utils" ) -const ( - featNamePrincipals = "azure/principals" - featNameRoleDefinitions = "azure/roledefinitions" - featNameRoleAssignments = "azure/roleassignments" - featNameVms = "azure/virtualmachines" -) - // fetcherConcurrency is an arbitrary per-resource type concurrency to ensure significant throughput. As we increase // the number of resource types, we may increase this value or use some other approach to fetching concurrency. const fetcherConcurrency = 4 @@ -123,11 +117,22 @@ func NewFetcher(cfg Config, ctx context.Context) (*Fetcher, error) { }, nil } +const ( + featNamePrincipals = "azure/principals" + featNameRoleDefinitions = "azure/roledefinitions" + featNameRoleAssignments = "azure/roleassignments" + featNameVms = "azure/virtualmachines" +) + // Features is a set of booleans that are received from the Access Graph to indicate which resources it can receive type Features struct { - Principals bool + // Principals indicates Azure principals can be be fetched + Principals bool + // RoleDefinitions indicates Azure role definitions can be fetched RoleDefinitions bool + // RoleAssignments indicates Azure role assignments can be fetched RoleAssignments bool + // VirtualMachines indicates Azure virtual machines can be fetched VirtualMachines bool } @@ -168,7 +173,7 @@ func (f *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) eg, ctx := errgroup.WithContext(ctx) eg.SetLimit(fetcherConcurrency) var result = &Resources{} - errsCh := make(chan error, fetcherConcurrency) + errsCh := make(chan error) if feats.Principals { eg.Go(func() error { principals, err := fetchPrincipals(ctx, f.SubscriptionID, f.graphClient) @@ -219,13 +224,25 @@ func (f *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) }) } + // Receive errors from the error channel until it's closed + var errs []error + wg := sync.WaitGroup{} + wg.Add(1) + go func() { + defer wg.Done() + for { + err, ok := <-errsCh + if !ok { + return + } + errs = append(errs, err) + } + }() + // Return the result along with any errors collected _ = eg.Wait() close(errsCh) - var errs []error - for err := range errsCh { - errs = append(errs, err) - } + wg.Wait() return result, trace.NewAggregate(errs...) } From 83206331626752ae7d6b480646a74439c087f1e3 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 22 Jan 2025 11:24:20 -0600 Subject: [PATCH 37/41] Apply suggestions from code review Co-authored-by: Tiago Silva --- .../fetchers/azure-sync/azure-sync.go | 20 +++---------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index 1121732753230..d5df6a00a30e5 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -173,7 +173,8 @@ func (f *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) eg, ctx := errgroup.WithContext(ctx) eg.SetLimit(fetcherConcurrency) var result = &Resources{} - errsCh := make(chan error) + // we use a larger value (50) here so there is always room for any returned error to be sent to errsCh without blocking. + errsCh := make(chan error,50) if feats.Principals { eg.Go(func() error { principals, err := fetchPrincipals(ctx, f.SubscriptionID, f.graphClient) @@ -224,26 +225,11 @@ func (f *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) }) } - // Receive errors from the error channel until it's closed - var errs []error - wg := sync.WaitGroup{} - wg.Add(1) - go func() { - defer wg.Done() - for { - err, ok := <-errsCh - if !ok { - return - } - errs = append(errs, err) - } - }() // Return the result along with any errors collected _ = eg.Wait() close(errsCh) - wg.Wait() - return result, trace.NewAggregate(errs...) + return result, trace.NewAggregateFromChannel(errsCh) } // Status returns the number of resources last fetched and/or the last fetching/reconciling error From 1a78cbf3900d61ad1f6024af30041775241674ed Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 22 Jan 2025 11:40:22 -0600 Subject: [PATCH 38/41] PR feedback --- .../fetchers/azure-sync/azure-sync.go | 18 ++-- .../fetchers/azure-sync/reconcile.go | 12 +-- lib/utils/slice.go | 14 --- lib/utils/slice_test.go | 90 ------------------- lib/utils/slices/slices.go | 14 +++ lib/utils/slices/slices_test.go | 90 +++++++++++++++++++ 6 files changed, 118 insertions(+), 120 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index d5df6a00a30e5..c351289e5246d 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -20,7 +20,7 @@ package azuresync import ( "context" - "sync" + "github.com/gravitational/teleport/lib/utils/slices" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/gravitational/trace" @@ -29,7 +29,6 @@ import ( accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" "github.com/gravitational/teleport/lib/cloud/azure" "github.com/gravitational/teleport/lib/msgraph" - "github.com/gravitational/teleport/lib/utils" ) // fetcherConcurrency is an arbitrary per-resource type concurrency to ensure significant throughput. As we increase @@ -160,10 +159,10 @@ func (f *Fetcher) Poll(ctx context.Context, feats Features) (*Resources, error) if res == nil { return nil, trace.Wrap(err) } - res.Principals = utils.DeduplicateKey(res.Principals, azurePrincipalsKey) - res.RoleAssignments = utils.DeduplicateKey(res.RoleAssignments, azureRoleAssignKey) - res.RoleDefinitions = utils.DeduplicateKey(res.RoleDefinitions, azureRoleDefKey) - res.VirtualMachines = utils.DeduplicateKey(res.VirtualMachines, azureVmKey) + res.Principals = slices.DeduplicateKey(res.Principals, azurePrincipalsKey) + res.RoleAssignments = slices.DeduplicateKey(res.RoleAssignments, azureRoleAssignKey) + res.RoleDefinitions = slices.DeduplicateKey(res.RoleDefinitions, azureRoleDefKey) + res.VirtualMachines = slices.DeduplicateKey(res.VirtualMachines, azureVmKey) return res, trace.Wrap(err) } @@ -173,8 +172,8 @@ func (f *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) eg, ctx := errgroup.WithContext(ctx) eg.SetLimit(fetcherConcurrency) var result = &Resources{} - // we use a larger value (50) here so there is always room for any returned error to be sent to errsCh without blocking. - errsCh := make(chan error,50) + // we use a larger value (50) here so there is always room for any returned error to be sent to errsCh without blocking. + errsCh := make(chan error, 50) if feats.Principals { eg.Go(func() error { principals, err := fetchPrincipals(ctx, f.SubscriptionID, f.graphClient) @@ -225,11 +224,10 @@ func (f *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) }) } - // Return the result along with any errors collected _ = eg.Wait() close(errsCh) - return result, trace.NewAggregateFromChannel(errsCh) + return result, trace.NewAggregateFromChannel(errsCh, ctx) } // Status returns the number of resources last fetched and/or the last fetching/reconciling error diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile.go b/lib/srv/discovery/fetchers/azure-sync/reconcile.go index 0f6d78da6d600..3363a6850f3fb 100644 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile.go +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile.go @@ -20,11 +20,11 @@ package azuresync import ( "fmt" + "github.com/gravitational/teleport/lib/utils/slices" "google.golang.org/protobuf/proto" accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" - "github.com/gravitational/teleport/lib/utils" ) // MergeResources merges Azure resources fetched from multiple configured Azure fetchers @@ -42,10 +42,10 @@ func MergeResources(results ...*Resources) *Resources { result.RoleDefinitions = append(result.RoleDefinitions, r.RoleDefinitions...) result.VirtualMachines = append(result.VirtualMachines, r.VirtualMachines...) } - result.Principals = utils.DeduplicateKey(result.Principals, azurePrincipalsKey) - result.RoleAssignments = utils.DeduplicateKey(result.RoleAssignments, azureRoleAssignKey) - result.RoleDefinitions = utils.DeduplicateKey(result.RoleDefinitions, azureRoleDefKey) - result.VirtualMachines = utils.DeduplicateKey(result.VirtualMachines, azureVmKey) + result.Principals = slices.DeduplicateKey(result.Principals, azurePrincipalsKey) + result.RoleAssignments = slices.DeduplicateKey(result.RoleAssignments, azureRoleAssignKey) + result.RoleDefinitions = slices.DeduplicateKey(result.RoleDefinitions, azureRoleDefKey) + result.VirtualMachines = slices.DeduplicateKey(result.VirtualMachines, azureVmKey) return result } @@ -86,7 +86,7 @@ func reconcile[T proto.Message]( wrapFn func(T) *accessgraphv1alpha.AzureResource, ) *reconcilePair { // Remove duplicates from the new items - newItems = utils.DeduplicateKey(newItems, keyFn) + newItems = slices.DeduplicateKey(newItems, keyFn) upsertRes := newResourceList() deleteRes := newResourceList() diff --git a/lib/utils/slice.go b/lib/utils/slice.go index 8b4f73b9543d5..4009277ceef50 100644 --- a/lib/utils/slice.go +++ b/lib/utils/slice.go @@ -142,17 +142,3 @@ func FromSlice[T any](r []T, key func(T) string) map[string]T { return out } - -// DeduplicateKey returns a deduplicated slice by comparing key values from the key function -func DeduplicateKey[T any](s []T, key func(T) string) []T { - out := make([]T, 0, len(s)) - seen := make(map[string]struct{}) - for _, v := range s { - if _, ok := seen[key(v)]; ok { - continue - } - seen[key(v)] = struct{}{} - out = append(out, v) - } - return out -} diff --git a/lib/utils/slice_test.go b/lib/utils/slice_test.go index fce1f113025d1..76f4a1dd1a29d 100644 --- a/lib/utils/slice_test.go +++ b/lib/utils/slice_test.go @@ -19,7 +19,6 @@ package utils import ( - "fmt" "testing" "github.com/stretchr/testify/require" @@ -42,92 +41,3 @@ func TestSlice(t *testing.T) { pool.Put(slice) } } - -// TestDuplicateKey tests slice deduplication via key function -func TestDeduplicateKey(t *testing.T) { - t.Parallel() - - stringTests := []struct { - name string - slice []string - keyFn func(string) string - expected []string - }{ - { - name: "EmptyStringSlice", - slice: []string{}, - keyFn: func(s string) string { return s }, - expected: []string{}, - }, - { - name: "NoStringDuplicates", - slice: []string{"foo", "bar", "baz"}, - keyFn: func(s string) string { return s }, - expected: []string{"foo", "bar", "baz"}, - }, - { - name: "StringDuplicates", - slice: []string{"foo", "bar", "bar", "bar", "baz", "baz"}, - keyFn: func(s string) string { return s }, - expected: []string{"foo", "bar", "baz"}, - }, - { - name: "StringDuplicatesWeirdKeyFn", - slice: []string{"foo", "bar", "bar", "bar", "baz", "baz"}, - keyFn: func(s string) string { return "huh" }, - expected: []string{"foo"}, - }, - } - for _, tt := range stringTests { - t.Run(tt.name, func(t *testing.T) { - res := DeduplicateKey(tt.slice, tt.keyFn) - require.Equal(t, tt.expected, res) - }) - } - - type dedupeStruct struct { - a string - b int - c bool - } - dedupeStructKeyFn := func(d dedupeStruct) string { return fmt.Sprintf("%s:%d:%v", d.a, d.b, d.c) } - structTests := []struct { - name string - slice []dedupeStruct - keyFn func(d dedupeStruct) string - expected []dedupeStruct - }{ - { - name: "EmptySlice", - slice: []dedupeStruct{}, - keyFn: dedupeStructKeyFn, - expected: []dedupeStruct{}, - }, - { - name: "NoStructDuplicates", - slice: []dedupeStruct{ - {a: "foo", b: 1, c: true}, - {a: "foo", b: 1, c: false}, - {a: "foo", b: 2, c: true}, - {a: "bar", b: 1, c: true}, - {a: "bar", b: 1, c: false}, - {a: "bar", b: 2, c: true}, - }, - keyFn: dedupeStructKeyFn, - expected: []dedupeStruct{ - {a: "foo", b: 1, c: true}, - {a: "foo", b: 1, c: false}, - {a: "foo", b: 2, c: true}, - {a: "bar", b: 1, c: true}, - {a: "bar", b: 1, c: false}, - {a: "bar", b: 2, c: true}, - }, - }, - } - for _, tt := range structTests { - t.Run(tt.name, func(t *testing.T) { - res := DeduplicateKey(tt.slice, tt.keyFn) - require.Equal(t, tt.expected, res) - }) - } -} diff --git a/lib/utils/slices/slices.go b/lib/utils/slices/slices.go index 3c33c0baf8710..077911c8738cf 100644 --- a/lib/utils/slices/slices.go +++ b/lib/utils/slices/slices.go @@ -57,3 +57,17 @@ func FromPointers[T any](in []*T) []T { } return out } + +// DeduplicateKey returns a deduplicated slice by comparing key values from the key function +func DeduplicateKey[T any](s []T, key func(T) string) []T { + out := make([]T, 0, len(s)) + seen := make(map[string]struct{}) + for _, v := range s { + if _, ok := seen[key(v)]; ok { + continue + } + seen[key(v)] = struct{}{} + out = append(out, v) + } + return out +} diff --git a/lib/utils/slices/slices_test.go b/lib/utils/slices/slices_test.go index 1f031d21eca49..b0fe86a1bfd2e 100644 --- a/lib/utils/slices/slices_test.go +++ b/lib/utils/slices/slices_test.go @@ -19,6 +19,7 @@ package slices import ( + "fmt" "strings" "testing" @@ -92,3 +93,92 @@ func TestFilterMapUnique(t *testing.T) { require.Equal(t, expected, got) }) } + +// TestDuplicateKey tests slice deduplication via key function +func TestDeduplicateKey(t *testing.T) { + t.Parallel() + + stringTests := []struct { + name string + slice []string + keyFn func(string) string + expected []string + }{ + { + name: "EmptyStringSlice", + slice: []string{}, + keyFn: func(s string) string { return s }, + expected: []string{}, + }, + { + name: "NoStringDuplicates", + slice: []string{"foo", "bar", "baz"}, + keyFn: func(s string) string { return s }, + expected: []string{"foo", "bar", "baz"}, + }, + { + name: "StringDuplicates", + slice: []string{"foo", "bar", "bar", "bar", "baz", "baz"}, + keyFn: func(s string) string { return s }, + expected: []string{"foo", "bar", "baz"}, + }, + { + name: "StringDuplicatesWeirdKeyFn", + slice: []string{"foo", "bar", "bar", "bar", "baz", "baz"}, + keyFn: func(s string) string { return "huh" }, + expected: []string{"foo"}, + }, + } + for _, tt := range stringTests { + t.Run(tt.name, func(t *testing.T) { + res := DeduplicateKey(tt.slice, tt.keyFn) + require.Equal(t, tt.expected, res) + }) + } + + type dedupeStruct struct { + a string + b int + c bool + } + dedupeStructKeyFn := func(d dedupeStruct) string { return fmt.Sprintf("%s:%d:%v", d.a, d.b, d.c) } + structTests := []struct { + name string + slice []dedupeStruct + keyFn func(d dedupeStruct) string + expected []dedupeStruct + }{ + { + name: "EmptySlice", + slice: []dedupeStruct{}, + keyFn: dedupeStructKeyFn, + expected: []dedupeStruct{}, + }, + { + name: "NoStructDuplicates", + slice: []dedupeStruct{ + {a: "foo", b: 1, c: true}, + {a: "foo", b: 1, c: false}, + {a: "foo", b: 2, c: true}, + {a: "bar", b: 1, c: true}, + {a: "bar", b: 1, c: false}, + {a: "bar", b: 2, c: true}, + }, + keyFn: dedupeStructKeyFn, + expected: []dedupeStruct{ + {a: "foo", b: 1, c: true}, + {a: "foo", b: 1, c: false}, + {a: "foo", b: 2, c: true}, + {a: "bar", b: 1, c: true}, + {a: "bar", b: 1, c: false}, + {a: "bar", b: 2, c: true}, + }, + }, + } + for _, tt := range structTests { + t.Run(tt.name, func(t *testing.T) { + res := DeduplicateKey(tt.slice, tt.keyFn) + require.Equal(t, tt.expected, res) + }) + } +} From 2fcbfc3d93e038228eddb58888033ea23e4db499 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 22 Jan 2025 13:39:13 -0600 Subject: [PATCH 39/41] Fixing flaky test --- lib/srv/discovery/fetchers/azure-sync/azure-sync.go | 2 +- lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index c351289e5246d..478b272dc4567 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -227,7 +227,7 @@ func (f *Fetcher) fetch(ctx context.Context, feats Features) (*Resources, error) // Return the result along with any errors collected _ = eg.Wait() close(errsCh) - return result, trace.NewAggregateFromChannel(errsCh, ctx) + return result, trace.NewAggregateFromChannel(errsCh, context.WithoutCancel(ctx)) } // Status returns the number of resources last fetched and/or the last fetching/reconciling error diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go index 05b73f64da95f..261555b21578e 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go @@ -65,7 +65,7 @@ func (t testVmCli) ListVirtualMachines(ctx context.Context, resourceGroup string } func newRoleDef(id string, name string) *armauthorization.RoleDefinition { - role_name := "test_role_name" + roleName := "test_role_name" action1 := "Microsoft.Compute/virtualMachines/read" action2 := "Microsoft.Compute/virtualMachines/*" action3 := "Microsoft.Compute/*" @@ -81,7 +81,7 @@ func newRoleDef(id string, name string) *armauthorization.RoleDefinition { Actions: []*string{&action3}, }, }, - RoleName: &role_name, + RoleName: &roleName, }, } } From 83932f61e7a067b77fe630fc9cbf3645656a3057 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 22 Jan 2025 13:52:29 -0600 Subject: [PATCH 40/41] Lint --- lib/srv/discovery/fetchers/azure-sync/azure-sync.go | 3 ++- lib/srv/discovery/fetchers/azure-sync/reconcile.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index 478b272dc4567..1ee1415424a05 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -20,7 +20,7 @@ package azuresync import ( "context" - "github.com/gravitational/teleport/lib/utils/slices" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/gravitational/trace" @@ -29,6 +29,7 @@ import ( accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" "github.com/gravitational/teleport/lib/cloud/azure" "github.com/gravitational/teleport/lib/msgraph" + "github.com/gravitational/teleport/lib/utils/slices" ) // fetcherConcurrency is an arbitrary per-resource type concurrency to ensure significant throughput. As we increase diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile.go b/lib/srv/discovery/fetchers/azure-sync/reconcile.go index 3363a6850f3fb..6fe6f8801a73f 100644 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile.go +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile.go @@ -20,11 +20,12 @@ package azuresync import ( "fmt" - "github.com/gravitational/teleport/lib/utils/slices" + "google.golang.org/protobuf/proto" accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha" + "github.com/gravitational/teleport/lib/utils/slices" ) // MergeResources merges Azure resources fetched from multiple configured Azure fetchers From 45e21f9d35cbdd44f412a3d18b317c1f8a40e710 Mon Sep 17 00:00:00 2001 From: Matt Brock Date: Wed, 22 Jan 2025 14:06:19 -0600 Subject: [PATCH 41/41] Fix imports --- lib/srv/discovery/fetchers/azure-sync/azure-sync.go | 1 - lib/srv/discovery/fetchers/azure-sync/reconcile.go | 1 - 2 files changed, 2 deletions(-) diff --git a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go index 1ee1415424a05..84dac1d26ec3c 100644 --- a/lib/srv/discovery/fetchers/azure-sync/azure-sync.go +++ b/lib/srv/discovery/fetchers/azure-sync/azure-sync.go @@ -21,7 +21,6 @@ package azuresync import ( "context" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/gravitational/trace" "golang.org/x/sync/errgroup" diff --git a/lib/srv/discovery/fetchers/azure-sync/reconcile.go b/lib/srv/discovery/fetchers/azure-sync/reconcile.go index 6fe6f8801a73f..a874f48215811 100644 --- a/lib/srv/discovery/fetchers/azure-sync/reconcile.go +++ b/lib/srv/discovery/fetchers/azure-sync/reconcile.go @@ -21,7 +21,6 @@ package azuresync import ( "fmt" - "google.golang.org/protobuf/proto" accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha"