From d77048f1ea7136af9f627182bc79126f9472a060 Mon Sep 17 00:00:00 2001 From: Ronald Ekambi Date: Thu, 15 Jun 2023 09:53:33 -0400 Subject: [PATCH 1/3] Stop referenced jwt providers from being deleted --- .changelog/17755.txt | 3 + agent/consul/state/config_entry.go | 63 ++++++++++++ agent/consul/state/config_entry_test.go | 129 ++++++++++++++++++++++++ 3 files changed, 195 insertions(+) create mode 100644 .changelog/17755.txt diff --git a/.changelog/17755.txt b/.changelog/17755.txt new file mode 100644 index 000000000000..7edf7b26e159 --- /dev/null +++ b/.changelog/17755.txt @@ -0,0 +1,3 @@ +```release-note:improvement +mesh: Stop jwt providers referenced by intentions from being deleted. +``` \ No newline at end of file diff --git a/agent/consul/state/config_entry.go b/agent/consul/state/config_entry.go index 340a53f1192c..d73a75dcdb6d 100644 --- a/agent/consul/state/config_entry.go +++ b/agent/consul/state/config_entry.go @@ -634,6 +634,12 @@ func validateProposedConfigEntryInGraph( case structs.TCPRoute: case structs.RateLimitIPConfig: case structs.JWTProvider: + if newEntry == nil && existingEntry != nil { + err := validateJWTProviderIsReferenced(tx, kindName, existingEntry) + if err != nil { + return err + } + } default: return fmt.Errorf("unhandled kind %q during validation of %q", kindName.Kind, kindName.Name) } @@ -704,6 +710,63 @@ func getReferencedProviderNames(j *structs.IntentionJWTRequirement, s []*structs return providerNames } +func validateJWTProviderIsReferenced(tx ReadTxn, kn configentry.KindName, ce structs.ConfigEntry) error { + meta := acl.NewEnterpriseMetaWithPartition( + kn.EnterpriseMeta.PartitionOrDefault(), + acl.DefaultNamespaceName, + ) + entry, ok := ce.(*structs.JWTProviderConfigEntry) + if !ok { + return fmt.Errorf("invalid jwt provider config entry: %T", entry) + } + + _, ixnEntries, err := configEntriesByKindTxn(tx, nil, structs.ServiceIntentions, &meta) + if err != nil { + return err + } + + providerNames, err := collectJWTProviderNames(ixnEntries) + if err != nil { + return err + } + + _, exist := providerNames[entry.Name] + if exist { + return fmt.Errorf("cannot delete jwt provider config entry referenced by an intention. Provider name:%s", entry.Name) + } + + return nil +} + +func collectJWTProviderNames(e []structs.ConfigEntry) (map[string]struct{}, error) { + names := make(map[string]struct{}) + + for _, entry := range e { + ixn, ok := entry.(*structs.ServiceIntentionsConfigEntry) + if !ok { + return names, fmt.Errorf("type %T is not a service intentions config entry", entry) + } + + if ixn.JWT != nil { + for _, prov := range ixn.JWT.Providers { + names[prov.Name] = struct{}{} + } + } + + for _, s := range ixn.Sources { + for _, perm := range s.Permissions { + if perm.JWT == nil { + continue + } + for _, prov := range perm.JWT.Providers { + names[prov.Name] = struct{}{} + } + } + } + } + return names, nil +} + // This fetches all the jwt-providers config entries and iterates over them // to validate that any provider referenced exists. // This is okay because we assume there are very few jwt-providers per partition diff --git a/agent/consul/state/config_entry_test.go b/agent/consul/state/config_entry_test.go index 572719dc4b1f..bc1439926301 100644 --- a/agent/consul/state/config_entry_test.go +++ b/agent/consul/state/config_entry_test.go @@ -3714,3 +3714,132 @@ func TestStateStore_DiscoveryChain_AttachVirtualIPs(t *testing.T) { require.Equal(t, []string{"2.2.2.2", "3.3.3.3"}, chain.ManualVirtualIPs) } + +func TestCollectJWTProviderNames(t *testing.T) { + oktaProvider := structs.IntentionJWTProvider{Name: "okta"} + auth0Provider := structs.IntentionJWTProvider{Name: "auth0"} + cases := map[string]struct { + entries []structs.ConfigEntry + expected map[string]struct{} + }{ + "no jwt at any level": { + entries: []structs.ConfigEntry{}, + expected: map[string]struct{}{}, + }, + "only top level jwt with no permissions": { + entries: []structs.ConfigEntry{ + &structs.ServiceIntentionsConfigEntry{ + Kind: "service-intentions", + Name: "api-intention", + JWT: &structs.IntentionJWTRequirement{ + Providers: []*structs.IntentionJWTProvider{&oktaProvider, &auth0Provider}, + }, + }, + }, + expected: map[string]struct{}{ + "okta": {}, "auth0": {}, + }, + }, + "top level jwt with permissions": { + entries: []structs.ConfigEntry{ + &structs.ServiceIntentionsConfigEntry{ + Kind: "service-intentions", + Name: "api-intention", + JWT: &structs.IntentionJWTRequirement{ + Providers: []*structs.IntentionJWTProvider{&oktaProvider}, + }, + Sources: []*structs.SourceIntention{ + { + Name: "api", + Action: "allow", + Permissions: []*structs.IntentionPermission{ + { + Action: "allow", + JWT: &structs.IntentionJWTRequirement{ + Providers: []*structs.IntentionJWTProvider{&oktaProvider}, + }, + }, + }, + }, + { + Name: "serv", + Action: "allow", + Permissions: []*structs.IntentionPermission{ + { + Action: "allow", + JWT: &structs.IntentionJWTRequirement{ + Providers: []*structs.IntentionJWTProvider{&auth0Provider}, + }, + }, + }, + }, + { + Name: "web", + Action: "allow", + Permissions: []*structs.IntentionPermission{ + {Action: "allow"}, + }, + }, + }, + }, + }, + expected: map[string]struct{}{ + "okta": {}, "auth0": {}, + }, + }, + "no top level jwt and existing permissions": { + entries: []structs.ConfigEntry{ + &structs.ServiceIntentionsConfigEntry{ + Kind: "service-intentions", + Name: "api-intention", + Sources: []*structs.SourceIntention{ + { + Name: "api", + Action: "allow", + Permissions: []*structs.IntentionPermission{ + { + Action: "allow", + JWT: &structs.IntentionJWTRequirement{ + Providers: []*structs.IntentionJWTProvider{&oktaProvider}, + }, + }, + }, + }, + { + Name: "serv", + Action: "allow", + Permissions: []*structs.IntentionPermission{ + { + Action: "allow", + JWT: &structs.IntentionJWTRequirement{ + Providers: []*structs.IntentionJWTProvider{&auth0Provider}, + }, + }, + }, + }, + { + Name: "web", + Action: "allow", + Permissions: []*structs.IntentionPermission{ + {Action: "allow"}, + }, + }, + }, + }, + }, + expected: map[string]struct{}{ + "okta": {}, "auth0": {}, + }, + }, + } + + for name, tt := range cases { + tt := tt + t.Run(name, func(t *testing.T) { + names, err := collectJWTProviderNames(tt.entries) + + require.NoError(t, err) + require.Equal(t, tt.expected, names) + }) + } +} From 301de5980e7fcc8d68d16e6cdcbd5b2afea73711 Mon Sep 17 00:00:00 2001 From: Ronald Ekambi Date: Fri, 16 Jun 2023 10:12:07 -0400 Subject: [PATCH 2/3] return early in case of matches, state store tests --- agent/consul/state/config_entry.go | 31 +++++----- agent/consul/state/config_entry_test.go | 81 +++++++++++++++++++------ 2 files changed, 81 insertions(+), 31 deletions(-) diff --git a/agent/consul/state/config_entry.go b/agent/consul/state/config_entry.go index d73a75dcdb6d..9abaafc390d3 100644 --- a/agent/consul/state/config_entry.go +++ b/agent/consul/state/config_entry.go @@ -710,6 +710,11 @@ func getReferencedProviderNames(j *structs.IntentionJWTRequirement, s []*structs return providerNames } +// validateJWTProviderIsReferenced iterates over intentions to determine if the provider being +// deleted is referenced by any intention. +// +// This could be an expensive operation based on the number of intentions. We purposely set this to only +// run on delete and don't expect this to be called often. func validateJWTProviderIsReferenced(tx ReadTxn, kn configentry.KindName, ce structs.ConfigEntry) error { meta := acl.NewEnterpriseMetaWithPartition( kn.EnterpriseMeta.PartitionOrDefault(), @@ -725,31 +730,27 @@ func validateJWTProviderIsReferenced(tx ReadTxn, kn configentry.KindName, ce str return err } - providerNames, err := collectJWTProviderNames(ixnEntries) + err = findJWTProviderNameReferences(ixnEntries, entry.Name) if err != nil { return err } - _, exist := providerNames[entry.Name] - if exist { - return fmt.Errorf("cannot delete jwt provider config entry referenced by an intention. Provider name:%s", entry.Name) - } - return nil } -func collectJWTProviderNames(e []structs.ConfigEntry) (map[string]struct{}, error) { - names := make(map[string]struct{}) - - for _, entry := range e { +func findJWTProviderNameReferences(entries []structs.ConfigEntry, pName string) error { + errMsg := "cannot delete jwt provider config entry referenced by an intention. Provider name: %s, intention name: %s" + for _, entry := range entries { ixn, ok := entry.(*structs.ServiceIntentionsConfigEntry) if !ok { - return names, fmt.Errorf("type %T is not a service intentions config entry", entry) + return fmt.Errorf("type %T is not a service intentions config entry", entry) } if ixn.JWT != nil { for _, prov := range ixn.JWT.Providers { - names[prov.Name] = struct{}{} + if prov.Name == pName { + return fmt.Errorf(errMsg, pName, ixn.Name) + } } } @@ -759,12 +760,14 @@ func collectJWTProviderNames(e []structs.ConfigEntry) (map[string]struct{}, erro continue } for _, prov := range perm.JWT.Providers { - names[prov.Name] = struct{}{} + if prov.Name == pName { + return fmt.Errorf(errMsg, pName, ixn.Name) + } } } } } - return names, nil + return nil } // This fetches all the jwt-providers config entries and iterates over them diff --git a/agent/consul/state/config_entry_test.go b/agent/consul/state/config_entry_test.go index bc1439926301..8840ccfcaeb7 100644 --- a/agent/consul/state/config_entry_test.go +++ b/agent/consul/state/config_entry_test.go @@ -3715,18 +3715,19 @@ func TestStateStore_DiscoveryChain_AttachVirtualIPs(t *testing.T) { } -func TestCollectJWTProviderNames(t *testing.T) { +func TestFindJWTProviderNameReferences(t *testing.T) { oktaProvider := structs.IntentionJWTProvider{Name: "okta"} auth0Provider := structs.IntentionJWTProvider{Name: "auth0"} cases := map[string]struct { - entries []structs.ConfigEntry - expected map[string]struct{} + entries []structs.ConfigEntry + providerName string + expectedError string }{ "no jwt at any level": { - entries: []structs.ConfigEntry{}, - expected: map[string]struct{}{}, + entries: []structs.ConfigEntry{}, + providerName: "okta", }, - "only top level jwt with no permissions": { + "provider not referenced": { entries: []structs.ConfigEntry{ &structs.ServiceIntentionsConfigEntry{ Kind: "service-intentions", @@ -3736,9 +3737,20 @@ func TestCollectJWTProviderNames(t *testing.T) { }, }, }, - expected: map[string]struct{}{ - "okta": {}, "auth0": {}, + providerName: "fake-provider", + }, + "only top level jwt with no permissions": { + entries: []structs.ConfigEntry{ + &structs.ServiceIntentionsConfigEntry{ + Kind: "service-intentions", + Name: "api-intention", + JWT: &structs.IntentionJWTRequirement{ + Providers: []*structs.IntentionJWTProvider{&oktaProvider, &auth0Provider}, + }, + }, }, + providerName: "okta", + expectedError: "cannot delete jwt provider config entry referenced by an intention. Provider name: okta, intention name: api-intention", }, "top level jwt with permissions": { entries: []structs.ConfigEntry{ @@ -3783,9 +3795,8 @@ func TestCollectJWTProviderNames(t *testing.T) { }, }, }, - expected: map[string]struct{}{ - "okta": {}, "auth0": {}, - }, + providerName: "auth0", + expectedError: "cannot delete jwt provider config entry referenced by an intention. Provider name: auth0, intention name: api-intention", }, "no top level jwt and existing permissions": { entries: []structs.ConfigEntry{ @@ -3827,19 +3838,55 @@ func TestCollectJWTProviderNames(t *testing.T) { }, }, }, - expected: map[string]struct{}{ - "okta": {}, "auth0": {}, - }, + providerName: "okta", + expectedError: "cannot delete jwt provider config entry referenced by an intention. Provider name: okta, intention name: api-intention", }, } for name, tt := range cases { tt := tt t.Run(name, func(t *testing.T) { - names, err := collectJWTProviderNames(tt.entries) + err := findJWTProviderNameReferences(tt.entries, tt.providerName) - require.NoError(t, err) - require.Equal(t, tt.expected, names) + if tt.expectedError != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tt.expectedError) + } else { + require.NoError(t, err) + } }) } } + +func TestStore_ValidateJWTProviderIsReferenced(t *testing.T) { + s := testStateStore(t) + + // First create a config entry + provider := &structs.JWTProviderConfigEntry{ + Kind: structs.JWTProvider, + Name: "okta", + } + require.NoError(t, s.EnsureConfigEntry(0, provider)) + + // create a service intention referencing the config entry + ixn := &structs.ServiceIntentionsConfigEntry{ + Name: "api", + JWT: &structs.IntentionJWTRequirement{ + Providers: []*structs.IntentionJWTProvider{ + {Name: provider.Name}, + }, + }, + } + + require.NoError(t, s.EnsureConfigEntry(1, ixn)) + + // attempt deleting a referenced provider + err := s.DeleteConfigEntry(0, structs.JWTProvider, provider.Name, nil) + require.Error(t, err) + require.Contains(t, err.Error(), `cannot delete jwt provider config entry referenced by an intention. Provider name: okta, intention name: api`) + + // delete the intention + require.NoError(t, s.DeleteConfigEntry(1, structs.ServiceIntentions, ixn.Name, nil)) + // successfully delete the provider after deleting the intention + require.NoError(t, s.DeleteConfigEntry(0, structs.JWTProvider, provider.Name, nil)) +} From 436bcda5ec58abbc5a65f6d244a1e1ec26beb46c Mon Sep 17 00:00:00 2001 From: Ronald Ekambi Date: Fri, 16 Jun 2023 10:13:41 -0400 Subject: [PATCH 3/3] remove extra space --- agent/consul/state/config_entry_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/agent/consul/state/config_entry_test.go b/agent/consul/state/config_entry_test.go index 8840ccfcaeb7..d72f12c87689 100644 --- a/agent/consul/state/config_entry_test.go +++ b/agent/consul/state/config_entry_test.go @@ -3877,7 +3877,6 @@ func TestStore_ValidateJWTProviderIsReferenced(t *testing.T) { }, }, } - require.NoError(t, s.EnsureConfigEntry(1, ixn)) // attempt deleting a referenced provider