diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index f925a37129..d75ac83680 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -277,7 +277,7 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool cfgParams.JWTRealm = jwtRealm } if jwtKey, exists := ingEx.Ingress.Annotations[JWTKeyAnnotation]; exists { - cfgParams.JWTKey = fmt.Sprintf("%v/%v", ingEx.Ingress.Namespace, jwtKey) + cfgParams.JWTKey = jwtKey } if jwtToken, exists := ingEx.Ingress.Annotations["nginx.com/jwt-token"]; exists { cfgParams.JWTToken = jwtToken diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index 22da4ba709..5685e7321a 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -243,11 +243,17 @@ func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) error { func (cnf *Configurator) addOrUpdateIngress(ingEx *IngressEx) error { apResources := cnf.updateApResources(ingEx) - pems := cnf.updateTLSSecrets(ingEx) - jwtKeyFileName := cnf.updateJWKSecret(ingEx) + + if jwtKey, exists := ingEx.Ingress.Annotations[JWTKeyAnnotation]; exists { + // LocalSecretStore will not set Path if the secret is not on the filesystem. + // However, NGINX configuration for an Ingress resource, to handle the case of a missing secret, + // relies on the path to be always configured. + ingEx.Secrets[jwtKey].Path = cnf.nginxManager.GetFilenameForSecret(ingEx.Ingress.Namespace + "-" + jwtKey) + } isMinion := false - nginxCfg := generateNginxCfg(ingEx, pems, apResources, isMinion, cnf.cfgParams, cnf.isPlus, cnf.IsResolverConfigured(), jwtKeyFileName, cnf.staticCfgParams) + nginxCfg := generateNginxCfg(ingEx, apResources, isMinion, cnf.cfgParams, cnf.isPlus, cnf.IsResolverConfigured(), + cnf.staticCfgParams, cnf.isWildcardEnabled) name := objectMetaToFileName(&ingEx.Ingress.ObjectMeta) content, err := cnf.templateExecutor.ExecuteIngressConfigTemplate(&nginxCfg) if err != nil { @@ -277,15 +283,18 @@ func (cnf *Configurator) AddOrUpdateMergeableIngress(mergeableIngs *MergeableIng func (cnf *Configurator) addOrUpdateMergeableIngress(mergeableIngs *MergeableIngresses) error { masterApResources := cnf.updateApResources(mergeableIngs.Master) - masterPems := cnf.updateTLSSecrets(mergeableIngs.Master) - masterJwtKeyFileName := cnf.updateJWKSecret(mergeableIngs.Master) - minionJwtKeyFileNames := make(map[string]string) + for _, minion := range mergeableIngs.Minions { - minionName := objectMetaToFileName(&minion.Ingress.ObjectMeta) - minionJwtKeyFileNames[minionName] = cnf.updateJWKSecret(minion) + if jwtKey, exists := minion.Ingress.Annotations[JWTKeyAnnotation]; exists { + // LocalSecretStore will not set Path if the secret is not on the filesystem. + // However, NGINX configuration for an Ingress resource, to handle the case of a missing secret, + // relies on the path to be always configured. + minion.Secrets[jwtKey].Path = cnf.nginxManager.GetFilenameForSecret(minion.Ingress.Namespace + "-" + jwtKey) + } } - nginxCfg := generateNginxCfgForMergeableIngresses(mergeableIngs, masterPems, masterApResources, masterJwtKeyFileName, minionJwtKeyFileNames, cnf.cfgParams, cnf.isPlus, cnf.IsResolverConfigured(), cnf.staticCfgParams) + nginxCfg := generateNginxCfgForMergeableIngresses(mergeableIngs, masterApResources, cnf.cfgParams, cnf.isPlus, + cnf.IsResolverConfigured(), cnf.staticCfgParams, cnf.isWildcardEnabled) name := objectMetaToFileName(&mergeableIngs.Master.Ingress.ObjectMeta) content, err := cnf.templateExecutor.ExecuteIngressConfigTemplate(&nginxCfg) @@ -405,22 +414,10 @@ func (cnf *Configurator) addOrUpdateOpenTracingTracerConfig(content string) erro } func (cnf *Configurator) addOrUpdateVirtualServer(virtualServerEx *VirtualServerEx) (Warnings, error) { - var tlsPemFileName string - var ingressMTLSFileName string name := getFileNameForVirtualServer(virtualServerEx.VirtualServer) - if virtualServerEx.TLSSecret != nil { - tlsPemFileName = cnf.addOrUpdateTLSSecret(virtualServerEx.TLSSecret) - } - if virtualServerEx.IngressMTLSCert != nil { - ingressMTLSFileName = cnf.addOrUpdateCASecret(virtualServerEx.IngressMTLSCert) - } - - jwtKeys := cnf.addOrUpdateJWKSecretsForVirtualServer(virtualServerEx.JWTKeys) - egressMTLSSecrets := cnf.addOrUpdateEgressMTLSecretsForVirtualServer(virtualServerEx.EgressTLSSecrets) - vsc := newVirtualServerConfigurator(cnf.cfgParams, cnf.isPlus, cnf.IsResolverConfigured(), cnf.staticCfgParams) - vsCfg, warnings := vsc.GenerateVirtualServerConfig(virtualServerEx, tlsPemFileName, jwtKeys, ingressMTLSFileName, egressMTLSSecrets) + vsCfg, warnings := vsc.GenerateVirtualServerConfig(virtualServerEx) content, err := cnf.templateExecutorV2.ExecuteVirtualServerTemplate(&vsCfg) if err != nil { return warnings, fmt.Errorf("Error generating VirtualServer config: %v: %v", name, err) @@ -550,42 +547,6 @@ func generateTLSPassthroughHostsConfig(tlsPassthroughPairs map[string]tlsPassthr return &cfg, duplicatedHosts } -func (cnf *Configurator) updateTLSSecrets(ingEx *IngressEx) map[string]string { - pems := make(map[string]string) - - for _, tls := range ingEx.Ingress.Spec.TLS { - secretName := tls.SecretName - - pemFileName := pemFileNameForMissingTLSSecret - if secretName == "" && cnf.isWildcardEnabled { - pemFileName = pemFileNameForWildcardTLSSecret - } else if secret, exists := ingEx.TLSSecrets[secretName]; exists { - pemFileName = cnf.addOrUpdateTLSSecret(secret) - } - - for _, host := range tls.Hosts { - pems[host] = pemFileName - } - if len(tls.Hosts) == 0 { - pems[emptyHost] = pemFileName - } - } - - return pems -} - -func (cnf *Configurator) updateJWKSecret(ingEx *IngressEx) string { - if !cnf.isPlus || ingEx.JWTKey.Name == "" { - return "" - } - - if ingEx.JWTKey.Secret != nil { - cnf.addOrUpdateJWKSecret(ingEx.JWTKey.Secret) - } - - return cnf.nginxManager.GetFilenameForSecret(ingEx.Ingress.Namespace + "-" + ingEx.JWTKey.Name) -} - func (cnf *Configurator) addOrUpdateCASecret(secret *api_v1.Secret) string { name := objectMetaToFileName(&secret.ObjectMeta) data := GenerateCAFileContent(secret) @@ -598,88 +559,6 @@ func (cnf *Configurator) addOrUpdateJWKSecret(secret *api_v1.Secret) string { return cnf.nginxManager.CreateSecret(name, data, nginx.JWKSecretFileMode) } -// AddOrUpdateJWKSecret adds a JWK secret to the filesystem or updates it if it already exists. -func (cnf *Configurator) AddOrUpdateJWKSecret(secret *api_v1.Secret, virtualServerExes []*VirtualServerEx) (Warnings, error) { - cnf.addOrUpdateJWKSecret(secret) - - allWarnings := newWarnings() - - if len(virtualServerExes) > 0 { - for _, vsEx := range virtualServerExes { - warnings, err := cnf.addOrUpdateVirtualServer(vsEx) - if err != nil { - return allWarnings, fmt.Errorf("Error adding or updating VirtualServer %v/%v: %v", vsEx.VirtualServer.Namespace, vsEx.VirtualServer.Name, err) - } - allWarnings.Add(warnings) - } - - if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil { - return allWarnings, fmt.Errorf("Error when reloading NGINX when updating Secret: %v", err) - } - } - - return allWarnings, nil -} - -// AddOrUpdateCASecret adds a CA secret to the filesystem or updates it if it already exists. -func (cnf *Configurator) AddOrUpdateCASecret(secret *api_v1.Secret, virtualServerExes []*VirtualServerEx) (Warnings, error) { - cnf.addOrUpdateCASecret(secret) - - allWarnings := newWarnings() - - if len(virtualServerExes) > 0 { - for _, vsEx := range virtualServerExes { - warnings, err := cnf.addOrUpdateVirtualServer(vsEx) - if err != nil { - return allWarnings, fmt.Errorf("Error adding or updating VirtualServer %v/%v: %v", vsEx.VirtualServer.Namespace, vsEx.VirtualServer.Name, err) - } - allWarnings.Add(warnings) - } - - if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil { - return allWarnings, fmt.Errorf("Error when reloading NGINX when updating Secret: %v", err) - } - } - - return allWarnings, nil -} - -// addOrUpdateJWKSecretsForVirtualServer adds JWK secrets to the filesystem or updates them if they already exist. -// Returns map[jwkKeyName]jwtKeyFilename -func (cnf *Configurator) addOrUpdateJWKSecretsForVirtualServer(jwtKeys map[string]*api_v1.Secret) map[string]string { - if !cnf.isPlus { - return nil - } - - jwkSecrets := make(map[string]string) - - for jwkKeyName, jwkKey := range jwtKeys { - filename := cnf.addOrUpdateJWKSecret(jwkKey) - jwkSecrets[jwkKeyName] = filename - } - - return jwkSecrets -} - -func (cnf *Configurator) addOrUpdateEgressMTLSecretsForVirtualServer(egressMTLSsecrets map[string]*api_v1.Secret) map[string]string { - - secrets := make(map[string]string) - var filename string - - for v, k := range egressMTLSsecrets { - if _, exists := k.Data[api_v1.TLSCertKey]; exists { - filename = cnf.addOrUpdateTLSSecret(k) - } - if _, exists := k.Data[CAKey]; exists { - filename = cnf.addOrUpdateCASecret(k) - - } - secrets[v] = filename - } - - return secrets -} - // AddOrUpdateResources adds or updates configuration for resources. func (cnf *Configurator) AddOrUpdateResources(ingExes []*IngressEx, mergeableIngresses []*MergeableIngresses, virtualServerExes []*VirtualServerEx) (Warnings, error) { allWarnings := newWarnings() @@ -713,12 +592,6 @@ func (cnf *Configurator) AddOrUpdateResources(ingExes []*IngressEx, mergeableIng return allWarnings, nil } -// AddOrUpdateTLSSecret adds or updates a file with the content of the TLS secret. -func (cnf *Configurator) AddOrUpdateTLSSecret(secret *api_v1.Secret, ingExes []*IngressEx, mergeableIngresses []*MergeableIngresses, virtualServerExes []*VirtualServerEx) (Warnings, error) { - cnf.addOrUpdateTLSSecret(secret) - return cnf.AddOrUpdateResources(ingExes, mergeableIngresses, virtualServerExes) -} - func (cnf *Configurator) addOrUpdateTLSSecret(secret *api_v1.Secret) string { name := objectMetaToFileName(&secret.ObjectMeta) data := GenerateCertAndKeyFileContent(secret) @@ -760,44 +633,6 @@ func GenerateCAFileContent(secret *api_v1.Secret) []byte { return res.Bytes() } -// DeleteSecret deletes the file associated with the secret and the configuration files for Ingress and VirtualServer resources. -// NGINX is reloaded only when the total number of the resources > 0. -func (cnf *Configurator) DeleteSecret(key string, ingExes []*IngressEx, mergeableIngresses []*MergeableIngresses, virtualServerExes []*VirtualServerEx) (Warnings, error) { - cnf.nginxManager.DeleteSecret(keyToFileName(key)) - - allWarnings := newWarnings() - - for _, ingEx := range ingExes { - err := cnf.addOrUpdateIngress(ingEx) - if err != nil { - return allWarnings, fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err) - } - } - - for _, m := range mergeableIngresses { - err := cnf.addOrUpdateMergeableIngress(m) - if err != nil { - return allWarnings, fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", m.Master.Ingress.Namespace, m.Master.Ingress.Name, err) - } - } - - for _, vsEx := range virtualServerExes { - warnings, err := cnf.addOrUpdateVirtualServer(vsEx) - if err != nil { - return allWarnings, fmt.Errorf("Error adding or updating VirtualServer %v/%v: %v", vsEx.VirtualServer.Namespace, vsEx.VirtualServer.Name, err) - } - allWarnings.Add(warnings) - } - - if len(ingExes)+len(mergeableIngresses)+len(virtualServerExes) > 0 { - if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil { - return allWarnings, fmt.Errorf("Error when reloading NGINX when deleting Secret %v: %v", key, err) - } - } - - return allWarnings, nil -} - // DeleteIngress deletes NGINX configuration for the Ingress resource. func (cnf *Configurator) DeleteIngress(key string) error { name := keyToFileName(key) @@ -1444,3 +1279,20 @@ func (cnf *Configurator) AddInternalRouteConfig() error { } return nil } + +// AddOrUpdateSecret adds or updates a secret. +func (cnf *Configurator) AddOrUpdateSecret(secret *api_v1.Secret) string { + switch secret.Type { + case SecretTypeCA: + return cnf.addOrUpdateCASecret(secret) + case SecretTypeJWK: + return cnf.addOrUpdateJWKSecret(secret) + default: + return cnf.addOrUpdateTLSSecret(secret) + } +} + +// DeleteSecret deletes a secret. +func (cnf *Configurator) DeleteSecret(key string) { + cnf.nginxManager.DeleteSecret(keyToFileName(key)) +} diff --git a/internal/configs/ingress.go b/internal/configs/ingress.go index e6a5ab6006..3ca8cc3506 100644 --- a/internal/configs/ingress.go +++ b/internal/configs/ingress.go @@ -21,8 +21,6 @@ const appProtectLogConfKey = "logconf" // IngressEx holds an Ingress along with the resources that are referenced in this Ingress. type IngressEx struct { Ingress *networking.Ingress - TLSSecrets map[string]*api_v1.Secret - JWTKey JWTKey Endpoints map[string][]string HealthChecks map[string]*api_v1.Probe ExternalNameSvcs map[string]bool @@ -32,6 +30,7 @@ type IngressEx struct { AppProtectPolicy *unstructured.Unstructured AppProtectLogConf *unstructured.Unstructured AppProtectLogDst string + Secrets map[string]*SecretReference } // JWTKey represents a secret that holds JSON Web Key. @@ -54,7 +53,8 @@ type MergeableIngresses struct { Minions []*IngressEx } -func generateNginxCfg(ingEx *IngressEx, pems map[string]string, apResources map[string]string, isMinion bool, baseCfgParams *ConfigParams, isPlus bool, isResolverConfigured bool, jwtKeyFileName string, staticParams *StaticConfigParams) version1.IngressNginxConfig { +func generateNginxCfg(ingEx *IngressEx, apResources map[string]string, isMinion bool, baseCfgParams *ConfigParams, isPlus bool, + isResolverConfigured bool, staticParams *StaticConfigParams, isWildcardEnabled bool) version1.IngressNginxConfig { hasAppProtect := staticParams.MainAppProtectLoadModule cfgParams := parseAnnotations(ingEx, baseCfgParams, isPlus, hasAppProtect, staticParams.EnableInternalRoutes) @@ -131,33 +131,18 @@ func generateNginxCfg(ingEx *IngressEx, pems map[string]string, apResources map[ SpiffeCerts: cfgParams.SpiffeServerCerts, } - if pemFile, ok := pems[serverName]; ok { - server.SSL = true - server.SSLCertificate = pemFile - server.SSLCertificateKey = pemFile - if pemFile == pemFileNameForMissingTLSSecret { - server.SSLCiphers = "NULL" - } - } + addSSLConfig(&server, rule.Host, ingEx.Ingress.Spec.TLS, ingEx.Secrets, isWildcardEnabled) if hasAppProtect { server.AppProtectPolicy = apResources[appProtectPolicyKey] server.AppProtectLogConf = apResources[appProtectLogConfKey] } - if !isMinion && ingEx.JWTKey.Name != "" { - server.JWTAuth = &version1.JWTAuth{ - Key: jwtKeyFileName, - Realm: cfgParams.JWTRealm, - Token: cfgParams.JWTToken, - } - - if cfgParams.JWTLoginURL != "" { - server.JWTAuth.RedirectLocationName = getNameForRedirectLocation(ingEx.Ingress) - server.JWTRedirectLocations = append(server.JWTRedirectLocations, version1.JWTRedirectLocation{ - Name: server.JWTAuth.RedirectLocationName, - LoginURL: cfgParams.JWTLoginURL, - }) + if !isMinion && cfgParams.JWTKey != "" { + jwtAuth, redirectLoc := generateJWTConfig(ingEx.Secrets, &cfgParams, getNameForRedirectLocation(ingEx.Ingress)) + server.JWTAuth = jwtAuth + if redirectLoc != nil { + server.JWTRedirectLocations = append(server.JWTRedirectLocations, *redirectLoc) } } @@ -201,21 +186,15 @@ func generateNginxCfg(ingEx *IngressEx, pems map[string]string, apResources map[ proxySSLName := generateProxySSLName(path.Backend.ServiceName, ingEx.Ingress.Namespace) loc := createLocation(pathOrDefault(path.Path), upstreams[upsName], &cfgParams, wsServices[path.Backend.ServiceName], rewrites[path.Backend.ServiceName], ssl, grpcServices[path.Backend.ServiceName], proxySSLName, path.PathType, path.Backend.ServiceName) - if isMinion && ingEx.JWTKey.Name != "" { - loc.JWTAuth = &version1.JWTAuth{ - Key: jwtKeyFileName, - Realm: cfgParams.JWTRealm, - Token: cfgParams.JWTToken, - } - if cfgParams.JWTLoginURL != "" { - loc.JWTAuth.RedirectLocationName = getNameForRedirectLocation(ingEx.Ingress) - server.JWTRedirectLocations = append(server.JWTRedirectLocations, version1.JWTRedirectLocation{ - Name: loc.JWTAuth.RedirectLocationName, - LoginURL: cfgParams.JWTLoginURL, - }) + if isMinion && cfgParams.JWTKey != "" { + jwtAuth, redirectLoc := generateJWTConfig(ingEx.Secrets, &cfgParams, getNameForRedirectLocation(ingEx.Ingress)) + loc.JWTAuth = jwtAuth + if redirectLoc != nil { + server.JWTRedirectLocations = append(server.JWTRedirectLocations, *redirectLoc) } } + locations = append(locations, loc) if loc.Path == "/" { @@ -269,6 +248,82 @@ func generateNginxCfg(ingEx *IngressEx, pems map[string]string, apResources map[ } } +func generateJWTConfig(secrets map[string]*SecretReference, cfgParams *ConfigParams, redirectLocationName string) (*version1.JWTAuth, *version1.JWTRedirectLocation) { + secret := secrets[cfgParams.JWTKey] + + if secret.Error != nil { + // TO-DO: add a warning + } else if secret.Type != SecretTypeJWK { + // TO-DO: add a warning + } + + // Key is configured for all cases, including when the secret is (1) invalid or (2) of a wrong type. + // For (1) and (2), NGINX Plus will reject such a key at runtime and return 500 to clients. + jwtAuth := &version1.JWTAuth{ + Key: secret.Path, + Realm: cfgParams.JWTRealm, + Token: cfgParams.JWTToken, + } + + var redirectLocation *version1.JWTRedirectLocation + + if cfgParams.JWTLoginURL != "" { + jwtAuth.RedirectLocationName = redirectLocationName + redirectLocation = &version1.JWTRedirectLocation{ + Name: redirectLocationName, + LoginURL: cfgParams.JWTLoginURL, + } + } + + return jwtAuth, redirectLocation +} + +func addSSLConfig(server *version1.Server, host string, ingressTLS []networking.IngressTLS, secrets map[string]*SecretReference, isWildcardEnabled bool) { + var tlsEnabled bool + var tlsSecret string + + for _, tls := range ingressTLS { + for _, h := range tls.Hosts { + if h == host { + tlsEnabled = true + tlsSecret = tls.SecretName + break + } + } + } + + if !tlsEnabled { + return + } + + var pemFile string + + if tlsSecret != "" { + secret := secrets[tlsSecret] + if secret.Error != nil { + pemFile = pemFileNameForMissingTLSSecret + // TO-DO: add a warning + } else if secret.Type != api_v1.SecretTypeTLS { + pemFile = pemFileNameForMissingTLSSecret + // TO-DO: add a warning + } else { + pemFile = secret.Path + } + } else if isWildcardEnabled { + pemFile = pemFileNameForWildcardTLSSecret + } else { + pemFile = pemFileNameForMissingTLSSecret + // TO-DO: add a warning + } + + server.SSL = true + server.SSLCertificate = pemFile + server.SSLCertificateKey = pemFile + if pemFile == pemFileNameForMissingTLSSecret { + server.SSLCiphers = "NULL" + } +} + func generateIngressPath(path string, pathType *networking.PathType) string { if pathType == nil { return path @@ -424,8 +479,9 @@ func upstreamMapToSlice(upstreams map[string]version1.Upstream) []version1.Upstr return result } -func generateNginxCfgForMergeableIngresses(mergeableIngs *MergeableIngresses, masterPems map[string]string, masterApResources map[string]string, masterJwtKeyFileName string, - minionJwtKeyFileNames map[string]string, baseCfgParams *ConfigParams, isPlus bool, isResolverConfigured bool, staticParams *StaticConfigParams) version1.IngressNginxConfig { +func generateNginxCfgForMergeableIngresses(mergeableIngs *MergeableIngresses, masterApResources map[string]string, + baseCfgParams *ConfigParams, isPlus bool, isResolverConfigured bool, staticParams *StaticConfigParams, + isWildcardEnabled bool) version1.IngressNginxConfig { var masterServer version1.Server var locations []version1.Location @@ -443,7 +499,7 @@ func generateNginxCfgForMergeableIngresses(mergeableIngs *MergeableIngresses, ma } isMinion := false - masterNginxCfg := generateNginxCfg(mergeableIngs.Master, masterPems, masterApResources, isMinion, baseCfgParams, isPlus, isResolverConfigured, masterJwtKeyFileName, staticParams) + masterNginxCfg := generateNginxCfg(mergeableIngs.Master, masterApResources, isMinion, baseCfgParams, isPlus, isResolverConfigured, staticParams, isWildcardEnabled) masterServer = masterNginxCfg.Servers[0] masterServer.Locations = []version1.Location{} @@ -471,12 +527,10 @@ func generateNginxCfgForMergeableIngresses(mergeableIngs *MergeableIngresses, ma minion.Ingress.Namespace, minion.Ingress.Name, strings.Join(removedAnnotations, ",")) } - pems := make(map[string]string) - jwtKeyFileName := minionJwtKeyFileNames[objectMetaToFileName(&minion.Ingress.ObjectMeta)] isMinion := true // App Protect Resources not allowed in minions - pass empty map dummyApResources := make(map[string]string) - nginxCfg := generateNginxCfg(minion, pems, dummyApResources, isMinion, baseCfgParams, isPlus, isResolverConfigured, jwtKeyFileName, staticParams) + nginxCfg := generateNginxCfg(minion, dummyApResources, isMinion, baseCfgParams, isPlus, isResolverConfigured, staticParams, isWildcardEnabled) for _, server := range nginxCfg.Servers { for _, loc := range server.Locations { diff --git a/internal/configs/ingress_test.go b/internal/configs/ingress_test.go index 81d76b11c5..9e68ef17f6 100644 --- a/internal/configs/ingress_test.go +++ b/internal/configs/ingress_test.go @@ -1,10 +1,12 @@ package configs import ( + "errors" "reflect" "strings" "testing" + "github.com/google/go-cmp/cmp" v1 "k8s.io/api/core/v1" networking "k8s.io/api/networking/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -19,17 +21,12 @@ func TestGenerateNginxCfg(t *testing.T) { expected := createExpectedConfigForCafeIngressEx() - pems := map[string]string{ - "cafe.example.com": "/etc/nginx/secrets/default-cafe-secret", - } - apRes := make(map[string]string) - result := generateNginxCfg(&cafeIngressEx, pems, apRes, false, configParams, false, false, "", &StaticConfigParams{}) + result := generateNginxCfg(&cafeIngressEx, apRes, false, configParams, false, false, &StaticConfigParams{}, false) - if !reflect.DeepEqual(result, expected) { - t.Errorf("generateNginxCfg returned \n%v, but expected \n%v", result, expected) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("generateNginxCfg() returned unexpected result (-want +got):\n%s", diff) } - } func TestGenerateNginxCfgForJWT(t *testing.T) { @@ -38,12 +35,10 @@ func TestGenerateNginxCfgForJWT(t *testing.T) { cafeIngressEx.Ingress.Annotations["nginx.com/jwt-realm"] = "Cafe App" cafeIngressEx.Ingress.Annotations["nginx.com/jwt-token"] = "$cookie_auth_token" cafeIngressEx.Ingress.Annotations["nginx.com/jwt-login-url"] = "https://login.example.com" - cafeIngressEx.JWTKey = JWTKey{"cafe-jwk", &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "cafe-jwk", - Namespace: "default", - }, - }} + cafeIngressEx.Secrets["cafe-jwk"] = &SecretReference{ + Type: "nginx.org/jwk", + Path: "/etc/nginx/secrets/default-cafe-jwk", + } configParams := NewDefaultConfigParams() @@ -61,12 +56,8 @@ func TestGenerateNginxCfgForJWT(t *testing.T) { }, } - pems := map[string]string{ - "cafe.example.com": "/etc/nginx/secrets/default-cafe-secret", - } - apRes := make(map[string]string) - result := generateNginxCfg(&cafeIngressEx, pems, apRes, false, configParams, true, false, "/etc/nginx/secrets/default-cafe-jwk", &StaticConfigParams{}) + result := generateNginxCfg(&cafeIngressEx, apRes, false, configParams, true, false, &StaticConfigParams{}, false) if !reflect.DeepEqual(result.Servers[0].JWTAuth, expected.Servers[0].JWTAuth) { t.Errorf("generateNginxCfg returned \n%v, but expected \n%v", result.Servers[0].JWTAuth, expected.Servers[0].JWTAuth) @@ -78,13 +69,11 @@ func TestGenerateNginxCfgForJWT(t *testing.T) { func TestGenerateNginxCfgWithMissingTLSSecret(t *testing.T) { cafeIngressEx := createCafeIngressEx() + cafeIngressEx.Secrets["cafe-secret"].Error = errors.New("secret doesn't exist") configParams := NewDefaultConfigParams() - pems := map[string]string{ - "cafe.example.com": pemFileNameForMissingTLSSecret, - } apRes := make(map[string]string) - result := generateNginxCfg(&cafeIngressEx, pems, apRes, false, configParams, false, false, "", &StaticConfigParams{}) + result := generateNginxCfg(&cafeIngressEx, apRes, false, configParams, false, false, &StaticConfigParams{}, false) expectedCiphers := "NULL" resultCiphers := result.Servers[0].SSLCiphers @@ -95,13 +84,11 @@ func TestGenerateNginxCfgWithMissingTLSSecret(t *testing.T) { func TestGenerateNginxCfgWithWildcardTLSSecret(t *testing.T) { cafeIngressEx := createCafeIngressEx() + cafeIngressEx.Ingress.Spec.TLS[0].SecretName = "" configParams := NewDefaultConfigParams() - pems := map[string]string{ - "cafe.example.com": pemFileNameForWildcardTLSSecret, - } apRes := make(map[string]string) - result := generateNginxCfg(&cafeIngressEx, pems, apRes, false, configParams, false, false, "", &StaticConfigParams{}) + result := generateNginxCfg(&cafeIngressEx, apRes, false, configParams, false, false, &StaticConfigParams{}, true) resultServer := result.Servers[0] if !reflect.DeepEqual(resultServer.SSLCertificate, pemFileNameForWildcardTLSSecret) { @@ -294,9 +281,6 @@ func createCafeIngressEx() IngressEx { } cafeIngressEx := IngressEx{ Ingress: &cafeIngress, - TLSSecrets: map[string]*v1.Secret{ - "cafe-secret": {}, - }, Endpoints: map[string][]string{ "coffee-svc80": {"10.0.0.1:80"}, "tea-svc80": {"10.0.0.2:80"}, @@ -305,6 +289,12 @@ func createCafeIngressEx() IngressEx { ValidHosts: map[string]bool{ "cafe.example.com": true, }, + Secrets: map[string]*SecretReference{ + "cafe-secret": { + Type: v1.SecretTypeTLS, + Path: "/etc/nginx/secrets/default-cafe-secret", + }, + }, } return cafeIngressEx } @@ -313,17 +303,13 @@ func TestGenerateNginxCfgForMergeableIngresses(t *testing.T) { mergeableIngresses := createMergeableCafeIngress() expected := createExpectedConfigForMergeableCafeIngress() - masterPems := map[string]string{ - "cafe.example.com": "/etc/nginx/secrets/default-cafe-secret", - } - minionJwtKeyFileNames := make(map[string]string) configParams := NewDefaultConfigParams() masterApRes := make(map[string]string) - result := generateNginxCfgForMergeableIngresses(mergeableIngresses, masterPems, masterApRes, "", minionJwtKeyFileNames, configParams, false, false, &StaticConfigParams{}) + result := generateNginxCfgForMergeableIngresses(mergeableIngresses, masterApRes, configParams, false, false, &StaticConfigParams{}, false) - if !reflect.DeepEqual(result, expected) { - t.Errorf("generateNginxCfgForMergeableIngresses returned \n%v, but expected \n%v", result, expected) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("generateNginxCfgForMergeableIngresses() returned unexpected result (-want +got):\n%s", diff) } } @@ -339,19 +325,14 @@ func TestGenerateNginxConfigForCrossNamespaceMergeableIngresses(t *testing.T) { } expected := createExpectedConfigForCrossNamespaceMergeableCafeIngress() - masterPems := map[string]string{ - "cafe.example.com": "/etc/nginx/secrets/default-cafe-secret", - } - minionJwtKeyFileNames := make(map[string]string) configParams := NewDefaultConfigParams() emptyApResources := make(map[string]string) - result := generateNginxCfgForMergeableIngresses(mergeableIngresses, masterPems, emptyApResources, "", minionJwtKeyFileNames, configParams, false, false, &StaticConfigParams{}) + result := generateNginxCfgForMergeableIngresses(mergeableIngresses, emptyApResources, configParams, false, false, &StaticConfigParams{}, false) - if !reflect.DeepEqual(result, expected) { - t.Errorf("generateNginxCfgForMergeableIngresses returned \n%v, but expected \n%v", result, expected) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("generateNginxCfgForMergeableIngresses() returned unexpected result (-want +got):\n%s", diff) } - } func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) { @@ -360,28 +341,18 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) { mergeableIngresses.Master.Ingress.Annotations["nginx.com/jwt-realm"] = "Cafe" mergeableIngresses.Master.Ingress.Annotations["nginx.com/jwt-token"] = "$cookie_auth_token" mergeableIngresses.Master.Ingress.Annotations["nginx.com/jwt-login-url"] = "https://login.example.com" - mergeableIngresses.Master.JWTKey = JWTKey{ - "cafe-jwk", - &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "cafe-jwk", - Namespace: "default", - }, - }, + mergeableIngresses.Master.Secrets["cafe-jwk"] = &SecretReference{ + Type: SecretTypeJWK, + Path: "/etc/nginx/secrets/default-cafe-jwk", } mergeableIngresses.Minions[0].Ingress.Annotations["nginx.com/jwt-key"] = "coffee-jwk" mergeableIngresses.Minions[0].Ingress.Annotations["nginx.com/jwt-realm"] = "Coffee" mergeableIngresses.Minions[0].Ingress.Annotations["nginx.com/jwt-token"] = "$cookie_auth_token_coffee" mergeableIngresses.Minions[0].Ingress.Annotations["nginx.com/jwt-login-url"] = "https://login.cofee.example.com" - mergeableIngresses.Minions[0].JWTKey = JWTKey{ - "coffee-jwk", - &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "coffee-jwk", - Namespace: "default", - }, - }, + mergeableIngresses.Minions[0].Secrets["coffee-jwk"] = &SecretReference{ + Type: SecretTypeJWK, + Path: "/etc/nginx/secrets/default-coffee-jwk", } expected := createExpectedConfigForMergeableCafeIngress() @@ -408,16 +379,13 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) { }, } - masterPems := map[string]string{ - "cafe.example.com": "/etc/nginx/secrets/default-cafe-secret", - } minionJwtKeyFileNames := make(map[string]string) minionJwtKeyFileNames[objectMetaToFileName(&mergeableIngresses.Minions[0].Ingress.ObjectMeta)] = "/etc/nginx/secrets/default-coffee-jwk" configParams := NewDefaultConfigParams() isPlus := true masterApRes := make(map[string]string) - result := generateNginxCfgForMergeableIngresses(mergeableIngresses, masterPems, masterApRes, "/etc/nginx/secrets/default-cafe-jwk", minionJwtKeyFileNames, configParams, isPlus, false, &StaticConfigParams{}) + result := generateNginxCfgForMergeableIngresses(mergeableIngresses, masterApRes, configParams, isPlus, false, &StaticConfigParams{}, false) if !reflect.DeepEqual(result.Servers[0].JWTAuth, expected.Servers[0].JWTAuth) { t.Errorf("generateNginxCfgForMergeableIngresses returned \n%v, but expected \n%v", result.Servers[0].JWTAuth, expected.Servers[0].JWTAuth) @@ -525,14 +493,6 @@ func createMergeableCafeIngress() *MergeableIngresses { mergeableIngresses := &MergeableIngresses{ Master: &IngressEx{ Ingress: &master, - TLSSecrets: map[string]*v1.Secret{ - "cafe-secret": { - ObjectMeta: meta_v1.ObjectMeta{ - Name: "cafe-secret", - Namespace: "default", - }, - }, - }, Endpoints: map[string][]string{ "coffee-svc80": {"10.0.0.1:80"}, "tea-svc80": {"10.0.0.2:80"}, @@ -540,6 +500,13 @@ func createMergeableCafeIngress() *MergeableIngresses { ValidHosts: map[string]bool{ "cafe.example.com": true, }, + Secrets: map[string]*SecretReference{ + "cafe-secret": { + Type: v1.SecretTypeTLS, + Path: "/etc/nginx/secrets/default-cafe-secret", + Error: nil, + }, + }, }, Minions: []*IngressEx{ { @@ -553,6 +520,7 @@ func createMergeableCafeIngress() *MergeableIngresses { ValidMinionPaths: map[string]bool{ "/coffee": true, }, + Secrets: map[string]*SecretReference{}, }, { Ingress: &teaMinion, @@ -565,6 +533,7 @@ func createMergeableCafeIngress() *MergeableIngresses { ValidMinionPaths: map[string]bool{ "/tea": true, }, + Secrets: map[string]*SecretReference{}, }}, } @@ -785,15 +754,12 @@ func TestGenerateNginxCfgForSpiffe(t *testing.T) { expected.Servers[0].Locations[i].SSL = true } - pems := map[string]string{ - "cafe.example.com": "/etc/nginx/secrets/default-cafe-secret", - } - apResources := make(map[string]string) - result := generateNginxCfg(&cafeIngressEx, pems, apResources, false, configParams, false, false, "", &StaticConfigParams{NginxServiceMesh: true}) + result := generateNginxCfg(&cafeIngressEx, apResources, false, configParams, false, false, + &StaticConfigParams{NginxServiceMesh: true}, false) - if !reflect.DeepEqual(result, expected) { - t.Errorf("generateNginxCfg returned \n%v, but expected \n%v", result, expected) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("generateNginxCfg() returned unexpected result (-want +got):\n%s", diff) } } @@ -807,15 +773,12 @@ func TestGenerateNginxCfgForInternalRoute(t *testing.T) { expected.Servers[0].SpiffeCerts = true expected.Ingress.Annotations[internalRouteAnnotation] = "true" - pems := map[string]string{ - "cafe.example.com": "/etc/nginx/secrets/default-cafe-secret", - } - apResources := make(map[string]string) - result := generateNginxCfg(&cafeIngressEx, pems, apResources, false, configParams, false, false, "", &StaticConfigParams{NginxServiceMesh: true, EnableInternalRoutes: true}) + result := generateNginxCfg(&cafeIngressEx, apResources, false, configParams, false, false, + &StaticConfigParams{NginxServiceMesh: true, EnableInternalRoutes: true}, false) - if !reflect.DeepEqual(result, expected) { - t.Errorf("generateNginxCfg returned \n%+v, but expected \n%+v", result, expected) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("generateNginxCfg() returned unexpected result (-want +got):\n%s", diff) } } @@ -883,3 +846,255 @@ func TestIsSSLEnabled(t *testing.T) { } } } + +func TestAddSSLConfig(t *testing.T) { + tests := []struct { + host string + tls []networking.IngressTLS + secrets map[string]*SecretReference + isWildcardEnabled bool + expected version1.Server + msg string + }{ + { + host: "some.example.com", + tls: []networking.IngressTLS{ + { + Hosts: []string{"cafe.example.com"}, + SecretName: "cafe-secret", + }, + }, + secrets: map[string]*SecretReference{ + "cafe-secret": { + Type: v1.SecretTypeTLS, + Path: "/etc/nginx/secrets/default-cafe-secret", + }, + }, + isWildcardEnabled: false, + expected: version1.Server{}, + msg: "TLS termination for different host", + }, + { + host: "cafe.example.com", + tls: []networking.IngressTLS{ + { + Hosts: []string{"cafe.example.com"}, + SecretName: "cafe-secret", + }, + }, + secrets: map[string]*SecretReference{ + "cafe-secret": { + Type: v1.SecretTypeTLS, + Path: "/etc/nginx/secrets/default-cafe-secret", + }, + }, + isWildcardEnabled: false, + expected: version1.Server{ + SSL: true, + SSLCertificate: "/etc/nginx/secrets/default-cafe-secret", + SSLCertificateKey: "/etc/nginx/secrets/default-cafe-secret", + }, + msg: "TLS termination", + }, + { + host: "cafe.example.com", + tls: []networking.IngressTLS{ + { + Hosts: []string{"cafe.example.com"}, + SecretName: "cafe-secret", + }, + }, + secrets: map[string]*SecretReference{ + "cafe-secret": { + Error: errors.New("invalid secret"), + }, + }, + isWildcardEnabled: false, + expected: version1.Server{ + SSL: true, + SSLCertificate: pemFileNameForMissingTLSSecret, + SSLCertificateKey: pemFileNameForMissingTLSSecret, + SSLCiphers: "NULL", + }, + msg: "invalid secret", + }, + { + host: "cafe.example.com", + tls: []networking.IngressTLS{ + { + Hosts: []string{"cafe.example.com"}, + SecretName: "cafe-secret", + }, + }, + secrets: map[string]*SecretReference{ + "cafe-secret": { + Type: SecretTypeCA, + Path: "/etc/nginx/secrets/default-cafe-secret", + }, + }, + isWildcardEnabled: false, + expected: version1.Server{ + SSL: true, + SSLCertificate: pemFileNameForMissingTLSSecret, + SSLCertificateKey: pemFileNameForMissingTLSSecret, + SSLCiphers: "NULL", + }, + msg: "secret of wrong type", + }, + { + host: "cafe.example.com", + tls: []networking.IngressTLS{ + { + Hosts: []string{"cafe.example.com"}, + SecretName: "", + }, + }, + isWildcardEnabled: true, + expected: version1.Server{ + SSL: true, + SSLCertificate: pemFileNameForWildcardTLSSecret, + SSLCertificateKey: pemFileNameForWildcardTLSSecret, + }, + msg: "no secret name with wildcard enabled", + }, + { + host: "cafe.example.com", + tls: []networking.IngressTLS{ + { + Hosts: []string{"cafe.example.com"}, + SecretName: "", + }, + }, + isWildcardEnabled: false, + expected: version1.Server{ + SSL: true, + SSLCertificate: pemFileNameForMissingTLSSecret, + SSLCertificateKey: pemFileNameForMissingTLSSecret, + SSLCiphers: "NULL", + }, + msg: "no secret name with wildcard disabled", + }, + } + + for _, test := range tests { + var result version1.Server + + addSSLConfig(&result, test.host, test.tls, test.secrets, test.isWildcardEnabled) + + if diff := cmp.Diff(test.expected, result); diff != "" { + t.Errorf("addSSLConfig() '%s' mismatch (-want +got):\n%s", test.msg, diff) + } + } +} + +func TestGenerateJWTConfig(t *testing.T) { + tests := []struct { + secrets map[string]*SecretReference + cfgParams *ConfigParams + redirectLocationName string + expectedJWTAuth *version1.JWTAuth + expectedRedirectLocation *version1.JWTRedirectLocation + msg string + }{ + { + secrets: map[string]*SecretReference{ + "cafe-jwk": { + Type: SecretTypeJWK, + Path: "/etc/nginx/secrets/default-cafe-jwk", + }, + }, + cfgParams: &ConfigParams{ + JWTKey: "cafe-jwk", + JWTRealm: "cafe", + JWTToken: "$http_token", + }, + redirectLocationName: "@loc", + expectedJWTAuth: &version1.JWTAuth{ + Key: "/etc/nginx/secrets/default-cafe-jwk", + Realm: "cafe", + Token: "$http_token", + }, + expectedRedirectLocation: nil, + msg: "normal case", + }, + { + secrets: map[string]*SecretReference{ + "cafe-jwk": { + Type: SecretTypeJWK, + Path: "/etc/nginx/secrets/default-cafe-jwk", + }, + }, + cfgParams: &ConfigParams{ + JWTKey: "cafe-jwk", + JWTRealm: "cafe", + JWTToken: "$http_token", + JWTLoginURL: "http://cafe.example.com/login", + }, + redirectLocationName: "@loc", + expectedJWTAuth: &version1.JWTAuth{ + Key: "/etc/nginx/secrets/default-cafe-jwk", + Realm: "cafe", + Token: "$http_token", + RedirectLocationName: "@loc", + }, + expectedRedirectLocation: &version1.JWTRedirectLocation{ + Name: "@loc", + LoginURL: "http://cafe.example.com/login", + }, + msg: "normal case with login url", + }, + { + secrets: map[string]*SecretReference{ + "cafe-jwk": { + Path: "/etc/nginx/secrets/default-cafe-jwk", + Error: errors.New("invalid secret"), + }, + }, + cfgParams: &ConfigParams{ + JWTKey: "cafe-jwk", + JWTRealm: "cafe", + JWTToken: "$http_token", + }, + redirectLocationName: "@loc", + expectedJWTAuth: &version1.JWTAuth{ + Key: "/etc/nginx/secrets/default-cafe-jwk", + Realm: "cafe", + Token: "$http_token", + }, + expectedRedirectLocation: nil, + msg: "invalid secret", + }, + { + secrets: map[string]*SecretReference{ + "cafe-jwk": { + Type: SecretTypeCA, + Path: "/etc/nginx/secrets/default-cafe-jwk", + }, + }, + cfgParams: &ConfigParams{ + JWTKey: "cafe-jwk", + JWTRealm: "cafe", + JWTToken: "$http_token", + }, + redirectLocationName: "@loc", + expectedJWTAuth: &version1.JWTAuth{ + Key: "/etc/nginx/secrets/default-cafe-jwk", + Realm: "cafe", + Token: "$http_token", + }, + expectedRedirectLocation: nil, + msg: "secret of wrong type", + }, + } + + for _, test := range tests { + jwtAuth, redirectLocation := generateJWTConfig(test.secrets, test.cfgParams, test.redirectLocationName) + + if diff := cmp.Diff(test.expectedJWTAuth, jwtAuth); diff != "" { + t.Errorf("generateJWTConfig() '%s' mismatch for jwtAuth (-want +got):\n%s", test.msg, diff) + } + if diff := cmp.Diff(test.expectedRedirectLocation, redirectLocation); diff != "" { + t.Errorf("generateJWTConfig() '%s' mismatch for redirectLocation (-want +got):\n%s", test.msg, diff) + } + } +} diff --git a/internal/configs/secret.go b/internal/configs/secret.go new file mode 100644 index 0000000000..9c332791fb --- /dev/null +++ b/internal/configs/secret.go @@ -0,0 +1,20 @@ +package configs + +import api_v1 "k8s.io/api/core/v1" + +// The constants below are defined because we can't use the same constants from the internal/k8s package +// because that will lead to import cycles +// TO-DO: Consider refactoring the internal/k8s package, so that we only define those constants once + +// SecretTypeCA contains a certificate authority for TLS certificate verification. +const SecretTypeCA api_v1.SecretType = "nginx.org/ca" + +// SecretTypeJWK contains a JWK (JSON Web Key) for validating JWTs (JSON Web Tokens). +const SecretTypeJWK api_v1.SecretType = "nginx.org/jwk" + +// SecretReference holds a reference to a secret stored on the file system. +type SecretReference struct { + Type api_v1.SecretType + Path string + Error error +} diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index e5e1572ee0..0fb5f605d6 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -55,13 +55,11 @@ type VirtualServerEx struct { VirtualServer *conf_v1.VirtualServer Endpoints map[string][]string TLSSecret *api_v1.Secret - JWTKeys map[string]*api_v1.Secret - IngressMTLSCert *api_v1.Secret - EgressTLSSecrets map[string]*api_v1.Secret VirtualServerRoutes []*conf_v1.VirtualServerRoute ExternalNameSvcs map[string]bool Policies map[string]*conf_v1alpha1.Policy PodsByIP map[string]PodInfo + Secrets map[string]*SecretReference } func (vsx *VirtualServerEx) String() string { @@ -257,34 +255,24 @@ func (vsc *virtualServerConfigurator) generateEndpointsForUpstream( } // GenerateVirtualServerConfig generates a full configuration for a VirtualServer -func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( - vsEx *VirtualServerEx, - tlsPemFileName string, - jwtKeys map[string]string, - ingressMTLSPemFileName string, - egressMTLSSecrets map[string]string, -) (version2.VirtualServerConfig, Warnings) { +func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig(vsEx *VirtualServerEx) (version2.VirtualServerConfig, Warnings) { vsc.clearWarnings() + sslConfig := generateSSLConfig(vsEx.VirtualServer.Spec.TLS, vsEx.VirtualServer.Namespace, vsEx.Secrets, vsc.cfgParams) + tlsRedirectConfig := generateTLSRedirectConfig(vsEx.VirtualServer.Spec.TLS) + + policyOpts := policyOptions{ + tls: sslConfig != nil, + secrets: vsEx.Secrets, + } + ownerDetails := policyOwnerDetails{ owner: vsEx.VirtualServer, ownerNamespace: vsEx.VirtualServer.Namespace, vsNamespace: vsEx.VirtualServer.Namespace, vsName: vsEx.VirtualServer.Name, } - policyOpts := policyOptions{ - jwtKeys: jwtKeys, - ingressMTLSPemFileName: ingressMTLSPemFileName, - tlsPemFileName: tlsPemFileName, - egressMTLSSecrets: egressMTLSSecrets, - } - policiesCfg := vsc.generatePolicies( - ownerDetails, - vsEx.VirtualServer.Spec.Policies, - vsEx.Policies, - specContext, - policyOpts, - ) + policiesCfg := vsc.generatePolicies(ownerDetails, vsEx.VirtualServer.Spec.Policies, vsEx.Policies, specContext, policyOpts) // crUpstreams maps an UpstreamName to its conf_v1.Upstream as they are generated // necessary for generateLocation to know what Upstream each Location references @@ -396,19 +384,12 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( } vsLocSnippets := r.LocationSnippets - // ingressMTLSPemFileName argument is always empty for route policies ownerDetails := policyOwnerDetails{ owner: vsEx.VirtualServer, ownerNamespace: vsEx.VirtualServer.Namespace, vsNamespace: vsEx.VirtualServer.Namespace, vsName: vsEx.VirtualServer.Name, } - policyOpts := policyOptions{ - jwtKeys: jwtKeys, - ingressMTLSPemFileName: "", - tlsPemFileName: tlsPemFileName, - egressMTLSSecrets: egressMTLSSecrets, - } routePoliciesCfg := vsc.generatePolicies(ownerDetails, r.Policies, vsEx.Policies, routeContext, policyOpts) limitReqZones = append(limitReqZones, routePoliciesCfg.LimitReqZones...) @@ -507,13 +488,6 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( policyRefs = r.Policies context = subRouteContext } - // ingressMTLSPemFileName argument is always empty for route policies - policyOpts := policyOptions{ - jwtKeys: jwtKeys, - ingressMTLSPemFileName: "", - tlsPemFileName: tlsPemFileName, - egressMTLSSecrets: egressMTLSSecrets, - } routePoliciesCfg := vsc.generatePolicies(ownerDetails, policyRefs, vsEx.Policies, context, policyOpts) limitReqZones = append(limitReqZones, routePoliciesCfg.LimitReqZones...) @@ -566,8 +540,6 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( } } - ssl := generateSSLConfig(vsEx.VirtualServer.Spec.TLS, tlsPemFileName, vsc.cfgParams) - tlsRedirectConfig := generateTLSRedirectConfig(vsEx.VirtualServer.Spec.TLS) httpSnippets := generateSnippets(vsc.enableSnippets, vsEx.VirtualServer.Spec.HTTPSnippets, []string{""}) serverSnippets := generateSnippets( vsc.enableSnippets, @@ -586,7 +558,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( ServerName: vsEx.VirtualServer.Spec.Host, StatusZone: vsEx.VirtualServer.Spec.Host, ProxyProtocol: vsc.cfgParams.ProxyProtocol, - SSL: ssl, + SSL: sslConfig, ServerTokens: vsc.cfgParams.ServerTokens, SetRealIPFrom: vsc.cfgParams.SetRealIPFrom, RealIPHeader: vsc.cfgParams.RealIPHeader, @@ -639,10 +611,8 @@ type policyOwnerDetails struct { } type policyOptions struct { - jwtKeys map[string]string - ingressMTLSPemFileName string - tlsPemFileName string - egressMTLSSecrets map[string]string + tls bool + secrets map[string]*SecretReference } type validationResults struct { @@ -703,7 +673,7 @@ func (p *policiesCfg) addJWTAuthConfig( jwtAuth *conf_v1alpha1.JWTAuth, polKey string, polNamespace string, - jwtKeys map[string]string, + secrets map[string]*SecretReference, ) *validationResults { res := newValidationResults() if p.JWTAuth != nil { @@ -712,14 +682,19 @@ func (p *policiesCfg) addJWTAuthConfig( } jwtSecretKey := fmt.Sprintf("%v/%v", polNamespace, jwtAuth.Secret) - if _, existsOnFilesystem := jwtKeys[jwtSecretKey]; !existsOnFilesystem { - res.addWarningf("JWT policy %q references a JWKSecret %q which does not exist", polKey, jwtSecretKey) + secret := secrets[jwtSecretKey] + if secret.Error != nil { + res.addWarningf("JWT policy %q references an invalid Secret: %v", polKey, secret.Error) + res.isError = true + return res + } else if secret.Type != SecretTypeJWK { + res.addWarningf("JWT policy %q references a Secret of an incorrect type %q", polKey, secret.Type) res.isError = true return res } p.JWTAuth = &version2.JWTAuth{ - Secret: jwtKeys[jwtSecretKey], + Secret: secret.Path, Realm: jwtAuth.Realm, Token: jwtAuth.Token, } @@ -729,12 +704,13 @@ func (p *policiesCfg) addJWTAuthConfig( func (p *policiesCfg) addIngressMTLSConfig( ingressMTLS *conf_v1alpha1.IngressMTLS, polKey string, + polNamespace string, context string, - tlsPemFileName string, - ingressMTLSPemFileName string, + tls bool, + secrets map[string]*SecretReference, ) *validationResults { res := newValidationResults() - if tlsPemFileName == "" { + if !tls { res.addWarningf("TLS configuration needed for IngressMTLS policy") res.isError = true return res @@ -749,8 +725,14 @@ func (p *policiesCfg) addIngressMTLSConfig( return res } - if ingressMTLSPemFileName == "" { - res.addWarningf("IngressMTLS policy %q references a Secret which does not exist", polKey) + secretKey := fmt.Sprintf("%v/%v", polNamespace, ingressMTLS.ClientCertSecret) + secret := secrets[secretKey] + if secret.Error != nil { + res.addWarningf("IngressMTLS policy %q references an invalid Secret: %v", polKey, secret.Error) + res.isError = true + return res + } else if secret.Type != SecretTypeCA { + res.addWarningf("IngressMTLS policy %q references a Secret of an incorrect type %q", polKey, secret.Type) res.isError = true return res } @@ -765,7 +747,7 @@ func (p *policiesCfg) addIngressMTLSConfig( } p.IngressMTLS = &version2.IngressMTLS{ - ClientCert: ingressMTLSPemFileName, + ClientCert: secret.Path, VerifyClient: verifyClient, VerifyDepth: verifyDepth, } @@ -776,7 +758,7 @@ func (p *policiesCfg) addEgressMTLSConfig( egressMTLS *conf_v1alpha1.EgressMTLS, polKey string, polNamespace string, - egressMTLSSecrets map[string]string, + secrets map[string]*SecretReference, ) *validationResults { res := newValidationResults() if p.EgressMTLS != nil { @@ -787,33 +769,54 @@ func (p *policiesCfg) addEgressMTLSConfig( return res } - egressTLSSecret := fmt.Sprintf("%v/%v", polNamespace, egressMTLS.TLSSecret) - TrustedCertSecret := fmt.Sprintf("%v/%v", polNamespace, egressMTLS.TrustedCertSecret) + var tlsSecretPath string - trustedCAFileName, trustedExists := egressMTLSSecrets[TrustedCertSecret] - if egressMTLS.TrustedCertSecret != "" && !trustedExists { - res.addWarningf("EgressMTLS policy %q references a Secret which does not exist", polKey) - res.isError = true - return res + if egressMTLS.TLSSecret != "" { + egressTLSSecret := fmt.Sprintf("%v/%v", polNamespace, egressMTLS.TLSSecret) + + tlsSecret := secrets[egressTLSSecret] + if tlsSecret.Error != nil { + res.addWarningf("EgressMTLS policy %q references an invalid Secret: %v", polKey, tlsSecret.Error) + res.isError = true + return res + } else if tlsSecret.Type != api_v1.SecretTypeTLS { + res.addWarningf("EgressMTLS policy %q references a Secret of an incorrect type %q", polKey, tlsSecret.Type) + res.isError = true + return res + } + + tlsSecretPath = tlsSecret.Path } - egressMTLSPemFileName, tlsExists := egressMTLSSecrets[egressTLSSecret] - if egressMTLS.TLSSecret != "" && !tlsExists { - res.addWarningf("EgressMTLS policy %q references a Secret which does not exist", polKey) - res.isError = true - return res + var trustedSecretPath string + + if egressMTLS.TrustedCertSecret != "" { + trustedCertSecret := fmt.Sprintf("%v/%v", polNamespace, egressMTLS.TrustedCertSecret) + + trustedSecret := secrets[trustedCertSecret] + if trustedSecret.Error != nil { + res.addWarningf("EgressMTLS policy %q references an invalid Secret: %v", polKey, trustedSecret.Error) + res.isError = true + return res + } else if trustedSecret.Type != SecretTypeCA { + res.addWarningf("EgressMTLS policy %q references a Secret of an incorrect type %q", polKey, trustedSecret.Type) + res.isError = true + return res + } + + trustedSecretPath = trustedSecret.Path } p.EgressMTLS = &version2.EgressMTLS{ - Certificate: egressMTLSPemFileName, - CertificateKey: egressMTLSPemFileName, + Certificate: tlsSecretPath, + CertificateKey: tlsSecretPath, Ciphers: generateString(egressMTLS.Ciphers, "DEFAULT"), Protocols: generateString(egressMTLS.Protocols, "TLSv1 TLSv1.1 TLSv1.2"), VerifyServer: egressMTLS.VerifyServer, VerifyDepth: generateIntFromPointer(egressMTLS.VerifyDepth, 1), SessionReuse: generateBool(egressMTLS.SessionReuse, true), ServerName: egressMTLS.ServerName, - TrustedCert: trustedCAFileName, + TrustedCert: trustedSecretPath, SSLName: generateString(egressMTLS.SSLName, "$proxy_host"), } return res @@ -851,17 +854,18 @@ func (vsc *virtualServerConfigurator) generatePolicies( ownerDetails.vsName, ) case pol.Spec.JWTAuth != nil: - res = config.addJWTAuthConfig(pol.Spec.JWTAuth, key, polNamespace, policyOpts.jwtKeys) + res = config.addJWTAuthConfig(pol.Spec.JWTAuth, key, polNamespace, policyOpts.secrets) case pol.Spec.IngressMTLS != nil: res = config.addIngressMTLSConfig( pol.Spec.IngressMTLS, key, + polNamespace, context, - policyOpts.tlsPemFileName, - policyOpts.ingressMTLSPemFileName, + policyOpts.tls, + policyOpts.secrets, ) case pol.Spec.EgressMTLS != nil: - res = config.addEgressMTLSConfig(pol.Spec.EgressMTLS, key, polNamespace, policyOpts.egressMTLSSecrets) + res = config.addEgressMTLSConfig(pol.Spec.EgressMTLS, key, polNamespace, policyOpts.secrets) default: res = newValidationResults() } @@ -1776,7 +1780,7 @@ func getNameForSourceForMatchesRouteMapFromCondition(condition conf_v1.Condition return condition.Variable } -func generateSSLConfig(tls *conf_v1.TLS, tlsPemFileName string, cfgParams *ConfigParams) *version2.SSL { +func generateSSLConfig(tls *conf_v1.TLS, namespace string, secrets map[string]*SecretReference, cfgParams *ConfigParams) *version2.SSL { if tls == nil { return nil } @@ -1785,14 +1789,21 @@ func generateSSLConfig(tls *conf_v1.TLS, tlsPemFileName string, cfgParams *Confi return nil } + secret := secrets[fmt.Sprintf("%s/%s", namespace, tls.Secret)] + var name string var ciphers string - if tlsPemFileName != "" { - name = tlsPemFileName - } else { + if secret.Error != nil { + name = pemFileNameForMissingTLSSecret + ciphers = "NULL" + // TO-DO: add a warning + } else if secret.Type != api_v1.SecretTypeTLS { name = pemFileNameForMissingTLSSecret ciphers = "NULL" + // TO-DO: add a warning + } else { + name = secret.Path } ssl := version2.SSL{ diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index f12718f22c..8fedeb8221 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -1,6 +1,7 @@ package configs import ( + "errors" "fmt" "reflect" "testing" @@ -10,6 +11,7 @@ import ( "github.com/nginxinc/kubernetes-ingress/internal/nginx" conf_v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1" conf_v1alpha1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1alpha1" + api_v1 "k8s.io/api/core/v1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -624,23 +626,14 @@ func TestGenerateVirtualServerConfig(t *testing.T) { isPlus := false isResolverConfigured := false - tlsPemFileName := "" - ingressMTLSFileName := "" vsc := newVirtualServerConfigurator( &baseCfgParams, isPlus, isResolverConfigured, &StaticConfigParams{TLSPassthrough: true}, ) - jwtKeys := make(map[string]string) - egressMTLSSecrets := make(map[string]string) - result, warnings := vsc.GenerateVirtualServerConfig( - &virtualServerEx, - tlsPemFileName, - jwtKeys, - ingressMTLSFileName, - egressMTLSSecrets, - ) + + result, warnings := vsc.GenerateVirtualServerConfig(&virtualServerEx) if !reflect.DeepEqual(result, expected) { t.Errorf("GenerateVirtualServerConfig returned \n%+v but expected \n%+v", result, expected) } @@ -743,19 +736,10 @@ func TestGenerateVirtualServerConfigWithSpiffeCerts(t *testing.T) { isPlus := false isResolverConfigured := false - tlsPemFileName := "" staticConfigParams := &StaticConfigParams{TLSPassthrough: true, NginxServiceMesh: true} vsc := newVirtualServerConfigurator(&baseCfgParams, isPlus, isResolverConfigured, staticConfigParams) - jwtKeys := make(map[string]string) - egressMTLSSecrets := make(map[string]string) - ingressMTLSFileName := "" - result, warnings := vsc.GenerateVirtualServerConfig( - &virtualServerEx, - tlsPemFileName, - jwtKeys, - ingressMTLSFileName, - egressMTLSSecrets, - ) + + result, warnings := vsc.GenerateVirtualServerConfig(&virtualServerEx) if !reflect.DeepEqual(result, expected) { t.Errorf("GenerateVirtualServerConfig returned \n%+v but expected \n%+v", result, expected) } @@ -1026,18 +1010,9 @@ func TestGenerateVirtualServerConfigForVirtualServerWithSplits(t *testing.T) { isPlus := false isResolverConfigured := false - tlsPemFileName := "" vsc := newVirtualServerConfigurator(&baseCfgParams, isPlus, isResolverConfigured, &StaticConfigParams{}) - jwtKeys := make(map[string]string) - egressMTLSSecrets := make(map[string]string) - ingressMTLSFileName := "" - result, warnings := vsc.GenerateVirtualServerConfig( - &virtualServerEx, - tlsPemFileName, - jwtKeys, - ingressMTLSFileName, - egressMTLSSecrets, - ) + + result, warnings := vsc.GenerateVirtualServerConfig(&virtualServerEx) if !reflect.DeepEqual(result, expected) { t.Errorf("GenerateVirtualServerConfig returned \n%+v but expected \n%+v", result, expected) } @@ -1341,18 +1316,9 @@ func TestGenerateVirtualServerConfigForVirtualServerWithMatches(t *testing.T) { isPlus := false isResolverConfigured := false - tlsPemFileName := "" vsc := newVirtualServerConfigurator(&baseCfgParams, isPlus, isResolverConfigured, &StaticConfigParams{}) - jwtKeys := make(map[string]string) - egressMTLSSecrets := make(map[string]string) - ingressMTLSFileName := "" - result, warnings := vsc.GenerateVirtualServerConfig( - &virtualServerEx, - tlsPemFileName, - jwtKeys, - ingressMTLSFileName, - egressMTLSSecrets, - ) + + result, warnings := vsc.GenerateVirtualServerConfig(&virtualServerEx) if !reflect.DeepEqual(result, expected) { t.Errorf("GenerateVirtualServerConfig returned \n%+v but expected \n%+v", result, expected) } @@ -1822,18 +1788,9 @@ func TestGenerateVirtualServerConfigForVirtualServerWithReturns(t *testing.T) { isPlus := false isResolverConfigured := false - tlsPemFileName := "" vsc := newVirtualServerConfigurator(&baseCfgParams, isPlus, isResolverConfigured, &StaticConfigParams{}) - jwtKeys := make(map[string]string) - egressMTLSSecrets := make(map[string]string) - ingressMTLSFileName := "" - result, warnings := vsc.GenerateVirtualServerConfig( - &virtualServerEx, - tlsPemFileName, - jwtKeys, - ingressMTLSFileName, - egressMTLSSecrets, - ) + + result, warnings := vsc.GenerateVirtualServerConfig(&virtualServerEx) if !reflect.DeepEqual(result, expected) { t.Errorf("GenerateVirtualServerConfig returned \n%+v but expected \n%+v", result, expected) } @@ -1851,7 +1808,27 @@ func TestGeneratePolicies(t *testing.T) { vsName: "test", } ingressMTLSCertPath := "/etc/nginx/secrets/default-ingress-mtls-secret" - tlsPemFileName := "/etc/nginx/secrets/default-tls-secret" + policyOpts := policyOptions{ + tls: true, + secrets: map[string]*SecretReference{ + "default/ingress-mtls-secret": { + Type: "nginx.org/ca", + Path: ingressMTLSCertPath, + }, + "default/egress-mtls-secret": { + Type: api_v1.SecretTypeTLS, + Path: "/etc/nginx/secrets/default-egress-mtls-secret", + }, + "default/egress-trusted-ca-secret": { + Type: "nginx.org/ca", + Path: "/etc/nginx/secrets/default-egress-trusted-ca-secret", + }, + "default/jwt-secret": { + Type: SecretTypeJWK, + Path: "/etc/nginx/secrets/default-jwt-secret", + }, + }, + } tests := []struct { policyRefs []conf_v1.PolicyReference @@ -1877,10 +1854,6 @@ func TestGeneratePolicies(t *testing.T) { }, }, }, - policyOpts: policyOptions{ - ingressMTLSPemFileName: ingressMTLSCertPath, - tlsPemFileName: tlsPemFileName, - }, expected: policiesCfg{ Allow: []string{"127.0.0.1"}, }, @@ -1931,10 +1904,6 @@ func TestGeneratePolicies(t *testing.T) { }, }, }, - policyOpts: policyOptions{ - ingressMTLSPemFileName: ingressMTLSCertPath, - tlsPemFileName: tlsPemFileName, - }, expected: policiesCfg{ Allow: []string{"127.0.0.1", "127.0.0.2"}, }, @@ -1959,10 +1928,6 @@ func TestGeneratePolicies(t *testing.T) { }, }, }, - policyOpts: policyOptions{ - ingressMTLSPemFileName: ingressMTLSCertPath, - tlsPemFileName: tlsPemFileName, - }, expected: policiesCfg{ LimitReqZones: []version2.LimitReqZone{ { @@ -2015,10 +1980,6 @@ func TestGeneratePolicies(t *testing.T) { }, }, }, - policyOpts: policyOptions{ - ingressMTLSPemFileName: ingressMTLSCertPath, - tlsPemFileName: tlsPemFileName, - }, expected: policiesCfg{ LimitReqZones: []version2.LimitReqZone{ { @@ -2058,6 +2019,10 @@ func TestGeneratePolicies(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/jwt-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "jwt-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ JWTAuth: &conf_v1alpha1.JWTAuth{ Realm: "My Test API", @@ -2066,13 +2031,6 @@ func TestGeneratePolicies(t *testing.T) { }, }, }, - policyOpts: policyOptions{ - ingressMTLSPemFileName: ingressMTLSCertPath, - tlsPemFileName: tlsPemFileName, - jwtKeys: map[string]string{ - "default/jwt-secret": "/etc/nginx/secrets/default-jwt-secret", - }, - }, expected: policiesCfg{ JWTAuth: &version2.JWTAuth{ Secret: "/etc/nginx/secrets/default-jwt-secret", @@ -2090,6 +2048,10 @@ func TestGeneratePolicies(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/ingress-mtls-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "ingress-mtls-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ IngressMTLS: &conf_v1alpha1.IngressMTLS{ ClientCertSecret: "ingress-mtls-secret", @@ -2098,10 +2060,6 @@ func TestGeneratePolicies(t *testing.T) { }, }, }, - policyOpts: policyOptions{ - ingressMTLSPemFileName: ingressMTLSCertPath, - tlsPemFileName: tlsPemFileName, - }, context: "spec", expected: policiesCfg{ IngressMTLS: &version2.IngressMTLS{ @@ -2131,14 +2089,6 @@ func TestGeneratePolicies(t *testing.T) { }, }, }, - policyOpts: policyOptions{ - ingressMTLSPemFileName: ingressMTLSCertPath, - tlsPemFileName: tlsPemFileName, - egressMTLSSecrets: map[string]string{ - "default/egress-mtls-secret": "/etc/nginx/secrets/default-egress-mtls-secret", - "default/egress-trusted-ca-secret": "/etc/nginx/secrets/default-egress-trusted-ca-secret", - }, - }, context: "route", expected: policiesCfg{ EgressMTLS: &version2.EgressMTLS{ @@ -2161,7 +2111,7 @@ func TestGeneratePolicies(t *testing.T) { vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}) for _, test := range tests { - result := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, test.policyOpts) + result := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, policyOpts) if diff := cmp.Diff(test.expected, result); diff != "" { t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) } @@ -2331,6 +2281,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/jwt-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "jwt-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ JWTAuth: &conf_v1alpha1.JWTAuth{ Realm: "test", @@ -2339,7 +2293,14 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, }, - policyOpts: policyOptions{}, + policyOpts: policyOptions{ + secrets: map[string]*SecretReference{ + "default/jwt-secret": { + Type: SecretTypeJWK, + Error: errors.New("secret is invalid"), + }, + }, + }, expected: policiesCfg{ ErrorReturn: &version2.Return{ Code: 500, @@ -2347,7 +2308,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: map[runtime.Object][]string{ nil: { - `JWT policy "default/jwt-policy" references a JWKSecret "default/jwt-secret" which does not exist`, + `JWT policy "default/jwt-policy" references an invalid Secret: secret is invalid`, }, }, msg: "jwt reference missing secret", @@ -2365,6 +2326,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/jwt-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "jwt-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ JWTAuth: &conf_v1alpha1.JWTAuth{ Realm: "test", @@ -2373,6 +2338,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, "default/jwt-policy2": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "jwt-policy2", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ JWTAuth: &conf_v1alpha1.JWTAuth{ Realm: "test", @@ -2382,9 +2351,15 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, policyOpts: policyOptions{ - jwtKeys: map[string]string{ - "default/jwt-secret": "/etc/nginx/secrets/default-jwt-secret", - "default/jwt-secret2": "", + secrets: map[string]*SecretReference{ + "default/jwt-secret": { + Type: SecretTypeJWK, + Path: "/etc/nginx/secrets/default-jwt-secret", + }, + "default/jwt-secret2": { + Type: SecretTypeJWK, + Path: "/etc/nginx/secrets/default-jwt-secret2", + }, }, }, expected: policiesCfg{ @@ -2409,6 +2384,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/ingress-mtls-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "ingress-mtls-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ IngressMTLS: &conf_v1alpha1.IngressMTLS{ ClientCertSecret: "ingress-mtls-secret", @@ -2417,7 +2396,12 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, policyOpts: policyOptions{ - tlsPemFileName: "/etc/nginx/secrets/default-tls-secret", + tls: true, + secrets: map[string]*SecretReference{ + "default/ingress-mtls-secret": { + Error: errors.New("secret is invalid"), + }, + }, }, context: "spec", expected: policiesCfg{ @@ -2427,10 +2411,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: map[runtime.Object][]string{ nil: { - `IngressMTLS policy "default/ingress-mtls-policy" references a Secret which does not exist`, + `IngressMTLS policy "default/ingress-mtls-policy" references an invalid Secret: secret is invalid`, }, }, - msg: "ingress mtls reference missing secret", + msg: "ingress mtls reference an invalid secret", }, { policyRefs: []conf_v1.PolicyReference{ @@ -2445,6 +2429,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/ingress-mtls-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "ingress-mtls-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ IngressMTLS: &conf_v1alpha1.IngressMTLS{ ClientCertSecret: "ingress-mtls-secret", @@ -2460,8 +2448,13 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, policyOpts: policyOptions{ - ingressMTLSPemFileName: "/etc/nginx/secrets/default-ingress-mtls-secret", - tlsPemFileName: "/etc/nginx/secrets/default-tls-secret", + tls: true, + secrets: map[string]*SecretReference{ + "default/ingress-mtls-secret": { + Type: SecretTypeCA, + Path: "/etc/nginx/secrets/default-ingress-mtls-secret", + }, + }, }, context: "spec", expected: policiesCfg{ @@ -2487,6 +2480,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/ingress-mtls-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "ingress-mtls-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ IngressMTLS: &conf_v1alpha1.IngressMTLS{ ClientCertSecret: "ingress-mtls-secret", @@ -2495,8 +2492,13 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, policyOpts: policyOptions{ - ingressMTLSPemFileName: "/etc/nginx/secrets/default-ingress-mtls-secret", - tlsPemFileName: "/etc/nginx/secrets/default-tls-secret", + tls: true, + secrets: map[string]*SecretReference{ + "default/ingress-mtls-secret": { + Type: SecretTypeCA, + Path: "/etc/nginx/secrets/default-ingress-mtls-secret", + }, + }, }, context: "route", expected: policiesCfg{ @@ -2520,6 +2522,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/ingress-mtls-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "ingress-mtls-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ IngressMTLS: &conf_v1alpha1.IngressMTLS{ ClientCertSecret: "ingress-mtls-secret", @@ -2528,7 +2534,13 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, policyOpts: policyOptions{ - ingressMTLSPemFileName: "/etc/nginx/secrets/default-ingress-mtls-secret", + tls: false, + secrets: map[string]*SecretReference{ + "default/ingress-mtls-secret": { + Type: SecretTypeCA, + Path: "/etc/nginx/secrets/default-ingress-mtls-secret", + }, + }, }, context: "route", expected: policiesCfg{ @@ -2556,6 +2568,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/egress-mtls-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "egress-mtls-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ EgressMTLS: &conf_v1alpha1.EgressMTLS{ TLSSecret: "egress-mtls-secret", @@ -2563,6 +2579,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, "default/egress-mtls-policy2": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "egress-mtls-policy2", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ EgressMTLS: &conf_v1alpha1.EgressMTLS{ TLSSecret: "egress-mtls-secret2", @@ -2571,8 +2591,11 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, policyOpts: policyOptions{ - egressMTLSSecrets: map[string]string{ - "default/egress-mtls-secret": "/etc/nginx/secrets/default-egress-mtls-secret", + secrets: map[string]*SecretReference{ + "default/egress-mtls-secret": { + Type: api_v1.SecretTypeTLS, + Path: "/etc/nginx/secrets/default-egress-mtls-secret", + }, }, }, context: "route", @@ -2604,17 +2627,24 @@ func TestGeneratePoliciesFails(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/egress-mtls-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "egress-mtls-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ EgressMTLS: &conf_v1alpha1.EgressMTLS{ - TrustedCertSecret: "egress-tusted-secret", + TrustedCertSecret: "egress-trusted-secret", SSLName: "foo.com", }, }, }, }, policyOpts: policyOptions{ - egressMTLSSecrets: map[string]string{ - "default/egress-mtls-secret": "/etc/nginx/secrets/default-egress-mtls-secret", + secrets: map[string]*SecretReference{ + "default/egress-trusted-secret": { + Type: SecretTypeCA, + Error: errors.New("secret is invalid"), + }, }, }, context: "route", @@ -2625,10 +2655,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: map[runtime.Object][]string{ nil: { - `EgressMTLS policy "default/egress-mtls-policy" references a Secret which does not exist`, + `EgressMTLS policy "default/egress-mtls-policy" references an invalid Secret: secret is invalid`, }, }, - msg: "egress mtls referencing missing CA secret", + msg: "egress mtls referencing an invalid CA secret", }, { policyRefs: []conf_v1.PolicyReference{ @@ -2639,6 +2669,10 @@ func TestGeneratePoliciesFails(t *testing.T) { }, policies: map[string]*conf_v1alpha1.Policy{ "default/egress-mtls-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "egress-mtls-policy", + Namespace: "default", + }, Spec: conf_v1alpha1.PolicySpec{ EgressMTLS: &conf_v1alpha1.EgressMTLS{ TLSSecret: "egress-mtls-secret", @@ -2648,8 +2682,11 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, policyOpts: policyOptions{ - egressMTLSSecrets: map[string]string{ - "default/egress-trusted-secret": "/etc/nginx/secrets/default-egress-trusted-secret", + secrets: map[string]*SecretReference{ + "default/egress-mtls-secret": { + Type: api_v1.SecretTypeTLS, + Error: errors.New("secret is invalid"), + }, }, }, context: "route", @@ -2660,7 +2697,7 @@ func TestGeneratePoliciesFails(t *testing.T) { }, expectedWarnings: map[runtime.Object][]string{ nil: { - `EgressMTLS policy "default/egress-mtls-policy" references a Secret which does not exist`, + `EgressMTLS policy "default/egress-mtls-policy" references an invalid Secret: secret is invalid`, }, }, msg: "egress mtls referencing missing tls secret", @@ -3288,60 +3325,71 @@ func TestGenerateLocationForRedirect(t *testing.T) { func TestGenerateSSLConfig(t *testing.T) { tests := []struct { - inputTLS *conf_v1.TLS - inputTLSPemFileName string - inputCfgParams *ConfigParams - expected *version2.SSL - msg string + inputTLS *conf_v1.TLS + inputSecrets map[string]*SecretReference + inputCfgParams *ConfigParams + expected *version2.SSL + msg string }{ { - inputTLS: nil, - inputTLSPemFileName: "", - inputCfgParams: &ConfigParams{}, - expected: nil, - msg: "no TLS field", + inputTLS: nil, + inputSecrets: map[string]*SecretReference{}, + inputCfgParams: &ConfigParams{}, + expected: nil, + msg: "no TLS field", }, { inputTLS: &conf_v1.TLS{ Secret: "", }, - inputTLSPemFileName: "", - inputCfgParams: &ConfigParams{}, - expected: nil, - msg: "TLS field with empty secret", + inputSecrets: map[string]*SecretReference{}, + inputCfgParams: &ConfigParams{}, + expected: nil, + msg: "TLS field with empty secret", }, { inputTLS: &conf_v1.TLS{ Secret: "secret", }, - inputTLSPemFileName: "", - inputCfgParams: &ConfigParams{}, + inputCfgParams: &ConfigParams{}, + inputSecrets: map[string]*SecretReference{ + "default/secret": { + Error: errors.New("secret doesn't exist"), + }, + }, expected: &version2.SSL{ HTTP2: false, Certificate: pemFileNameForMissingTLSSecret, CertificateKey: pemFileNameForMissingTLSSecret, Ciphers: "NULL", }, - msg: "secret doesn't exist in the cluster with HTTP2", + msg: "secret doesn't exist in the cluster with HTTPS", }, { inputTLS: &conf_v1.TLS{ Secret: "secret", }, - inputTLSPemFileName: "secret.pem", - inputCfgParams: &ConfigParams{}, + inputSecrets: map[string]*SecretReference{ + "default/secret": { + Type: api_v1.SecretTypeTLS, + Path: "secret.pem", + }, + }, + inputCfgParams: &ConfigParams{}, expected: &version2.SSL{ HTTP2: false, Certificate: "secret.pem", CertificateKey: "secret.pem", Ciphers: "", }, - msg: "normal case with HTTP2", + msg: "normal case with HTTPS", }, } + namespace := "default" + for _, test := range tests { - result := generateSSLConfig(test.inputTLS, test.inputTLSPemFileName, test.inputCfgParams) + result := generateSSLConfig(test.inputTLS, namespace, test.inputSecrets, test.inputCfgParams) if !reflect.DeepEqual(result, test.expected) { t.Errorf("generateSSLConfig() returned %v but expected %v for the case of %s", result, test.expected, test.msg) } diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 217430bc27..0b0578076c 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -154,6 +154,7 @@ type LoadBalancerController struct { isNginxReady bool isLatencyMetricsEnabled bool configuration *Configuration + secretStore SecretStore } var keyFunc = cache.DeletionHandlingMetaNamespaceKeyFunc @@ -289,6 +290,8 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc lbc.configuration = NewConfiguration(lbc.HasCorrectIngressClass, input.IsNginxPlus, input.VirtualServerValidator) + lbc.secretStore = NewLocalSecretStore(lbc.configurator) + lbc.updateIngressMetrics() return lbc } @@ -1418,9 +1421,13 @@ func (lbc *LoadBalancerController) syncSecret(task task) { glog.V(2).Infof("Found %v Resources with Secret %v", len(resources), key) if !secrExists { + lbc.secretStore.DeleteSecret(key) + glog.V(2).Infof("Deleting Secret: %v\n", key) - lbc.handleRegularSecretDeletion(key, resources) + if len(resources) > 0 { + lbc.handleRegularSecretDeletion(resources) + } if lbc.isSpecialSecret(key) { glog.Warningf("A special TLS Secret %v was removed. Retaining the Secret.", key) } @@ -1431,6 +1438,8 @@ func (lbc *LoadBalancerController) syncSecret(task task) { secret := obj.(*api_v1.Secret) + lbc.secretStore.AddOrUpdateSecret(secret) + if lbc.isSpecialSecret(key) { lbc.handleSpecialSecretUpdate(secret) // we don't return here in case the special secret is also used in resources. @@ -1459,46 +1468,22 @@ func (lbc *LoadBalancerController) isSpecialSecret(secretName string) bool { return secretName == lbc.defaultServerSecret || secretName == lbc.wildcardTLSSecret } -func (lbc *LoadBalancerController) handleRegularSecretDeletion(key string, resources []Resource) { +func (lbc *LoadBalancerController) handleRegularSecretDeletion(resources []Resource) { ingressExes, mergeableIngresses, virtualServerExes := lbc.createExtendedResources(resources) - warnings, deleteErr := lbc.configurator.DeleteSecret(key, ingressExes, mergeableIngresses, virtualServerExes) - if deleteErr != nil { - glog.Errorf("Error when deleting Secret: %v: %v", key, deleteErr) - } + warnings, addOrUpdateErr := lbc.configurator.AddOrUpdateResources(ingressExes, mergeableIngresses, virtualServerExes) - lbc.updateResourcesStatusAndEvents(resources, warnings, deleteErr) + lbc.updateResourcesStatusAndEvents(resources, warnings, addOrUpdateErr) } func (lbc *LoadBalancerController) handleSecretUpdate(secret *api_v1.Secret, resources []Resource) { secretNsName := secret.Namespace + "/" + secret.Name - err := ValidateSecret(secret) - if err != nil { - // Secret becomes Invalid - glog.Errorf("Couldn't validate secret %v: %v", secretNsName, err) - glog.Errorf("Removing invalid secret %v", secretNsName) - - lbc.handleRegularSecretDeletion(secretNsName, resources) - - lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "Rejected", "%v was rejected: %v", secretNsName, err) - return - } - var warnings configs.Warnings var addOrUpdateErr error - switch secret.Type { - case SecretTypeJWK: - _, _, virtualServerExes := lbc.createExtendedResources(resources) - warnings, addOrUpdateErr = lbc.configurator.AddOrUpdateJWKSecret(secret, virtualServerExes) - case SecretTypeCA: - _, _, virtualServerExes := lbc.createExtendedResources(resources) - warnings, addOrUpdateErr = lbc.configurator.AddOrUpdateCASecret(secret, virtualServerExes) - default: - ingressExes, mergeableIngresses, virtualServerExes := lbc.createExtendedResources(resources) - warnings, addOrUpdateErr = lbc.configurator.AddOrUpdateTLSSecret(secret, ingressExes, mergeableIngresses, virtualServerExes) - } + ingressExes, mergeableIngresses, virtualServerExes := lbc.createExtendedResources(resources) + warnings, addOrUpdateErr = lbc.configurator.AddOrUpdateResources(ingressExes, mergeableIngresses, virtualServerExes) if addOrUpdateErr != nil { glog.Errorf("Error when updating Secret %v: %v", secretNsName, addOrUpdateErr) @@ -1720,57 +1705,6 @@ func getIPAddressesFromEndpoints(endpoints []podEndpoint) []string { return endps } -func (lbc *LoadBalancerController) getSecret(secretKey string) (*api_v1.Secret, error) { - secretObject, secretExists, err := lbc.secretLister.GetByKey(secretKey) - if err != nil { - return nil, fmt.Errorf("error retrieving secret %v", secretKey) - } - if !secretExists { - return nil, fmt.Errorf("secret %v not found", secretKey) - } - secret := secretObject.(*api_v1.Secret) - return secret, nil -} - -func (lbc *LoadBalancerController) getAndValidateSecret(secretKey string) (*api_v1.Secret, error) { - secret, err := lbc.getSecret(secretKey) - if err != nil { - return nil, err - } - - err = ValidateTLSSecret(secret) - if err != nil { - return nil, fmt.Errorf("error validating secret %v", secretKey) - } - return secret, nil -} - -func (lbc *LoadBalancerController) getAndValidateJWTSecret(secretKey string) (*api_v1.Secret, error) { - secret, err := lbc.getSecret(secretKey) - if err != nil { - return nil, err - } - - err = ValidateJWKSecret(secret) - if err != nil { - return nil, fmt.Errorf("error validating secret %v", secretKey) - } - return secret, nil -} - -func (lbc *LoadBalancerController) getAndValidateCASecret(secretKey string) (*api_v1.Secret, error) { - secret, err := lbc.getSecret(secretKey) - if err != nil { - return nil, err - } - - err = ValidateCASecret(secret) - if err != nil { - return nil, fmt.Errorf("error validating secret %v", secretKey) - } - return secret, nil -} - func (lbc *LoadBalancerController) createMergeableIngresses(fullIng *FullIngress) *configs.MergeableIngresses { // for master Ingress, validMinionPaths are nil masterIngressEx := lbc.createIngressEx(fullIng.Ingress, fullIng.ValidHosts, nil) @@ -1794,37 +1728,38 @@ func (lbc *LoadBalancerController) createIngressEx(ing *networking.Ingress, vali ValidMinionPaths: validMinionPaths, } - ingEx.TLSSecrets = make(map[string]*api_v1.Secret) + ingEx.Secrets = make(map[string]*configs.SecretReference) + for _, tls := range ing.Spec.TLS { secretName := tls.SecretName secretKey := ing.Namespace + "/" + secretName - secret, err := lbc.getAndValidateSecret(secretKey) + + secretType, secretPath, err := lbc.secretStore.GetSecret(secretKey) if err != nil { glog.Warningf("Error trying to get the secret %v for Ingress %v: %v", secretName, ing.Name, err) - continue } - ingEx.TLSSecrets[secretName] = secret + + ingEx.Secrets[secretName] = &configs.SecretReference{ + Type: secretType, + Path: secretPath, + Error: err, + } } if lbc.isNginxPlus { if jwtKey, exists := ingEx.Ingress.Annotations[configs.JWTKeyAnnotation]; exists { secretName := jwtKey + secretKey := ing.Namespace + "/" + secretName - secret, err := lbc.client.CoreV1().Secrets(ing.Namespace).Get(context.TODO(), secretName, meta_v1.GetOptions{}) + secretType, secretPath, err := lbc.secretStore.GetSecret(secretKey) if err != nil { - glog.Warningf("Error retrieving secret %v for Ingress %v: %v", secretName, ing.Name, err) - secret = nil - } else { - err = ValidateJWKSecret(secret) - if err != nil { - glog.Warningf("Error validating secret %v for Ingress %v: %v", secretName, ing.Name, err) - secret = nil - } + glog.Warningf("Error trying to get the secret %v for Ingress %v: %v", secretName, ing.Name, err) } - ingEx.JWTKey = configs.JWTKey{ - Name: jwtKey, - Secret: secret, + ingEx.Secrets[secretName] = &configs.SecretReference{ + Type: secretType, + Path: secretPath, + Error: err, } } if lbc.appProtectEnabled { @@ -2008,18 +1943,22 @@ func (lbc *LoadBalancerController) getAppProtectPolicy(ing *networking.Ingress) func (lbc *LoadBalancerController) createVirtualServerEx(virtualServer *conf_v1.VirtualServer, virtualServerRoutes []*conf_v1.VirtualServerRoute) *configs.VirtualServerEx { virtualServerEx := configs.VirtualServerEx{ - VirtualServer: virtualServer, - JWTKeys: make(map[string]*api_v1.Secret), - EgressTLSSecrets: make(map[string]*api_v1.Secret), + VirtualServer: virtualServer, + Secrets: make(map[string]*configs.SecretReference), } if virtualServer.Spec.TLS != nil && virtualServer.Spec.TLS.Secret != "" { secretKey := virtualServer.Namespace + "/" + virtualServer.Spec.TLS.Secret - secret, err := lbc.getAndValidateSecret(secretKey) + + secretType, secretPath, err := lbc.secretStore.GetSecret(secretKey) if err != nil { glog.Warningf("Error trying to get the secret %v for VirtualServer %v: %v", secretKey, virtualServer.Name, err) - } else { - virtualServerEx.TLSSecret = secret + } + + virtualServerEx.Secrets[secretKey] = &configs.SecretReference{ + Type: secretType, + Path: secretPath, + Error: err, } } @@ -2028,19 +1967,17 @@ func (lbc *LoadBalancerController) createVirtualServerEx(virtualServer *conf_v1. glog.Warningf("Error getting policy for VirtualServer %s/%s: %v", virtualServer.Namespace, virtualServer.Name, err) } - err := lbc.addJWTSecrets(policies, virtualServerEx.JWTKeys) + err := lbc.addJWTSecrets(virtualServerEx.Secrets, policies) if err != nil { glog.Warningf("Error getting JWT secrets for VirtualServer %v/%v: %v", virtualServer.Namespace, virtualServer.Name, err) } - ingressSecret, err := lbc.getIngressMTLSSecret(policies) + err = lbc.addIngressMTLSSecret(virtualServerEx.Secrets, policies) if err != nil { glog.Warningf("Error getting IngressMTLS secret for VirtualServer %v/%v: %v", virtualServer.Namespace, virtualServer.Name, err) - } else { - virtualServerEx.IngressMTLSCert = ingressSecret } - err = lbc.addEgressMTLSSecrets(policies, virtualServerEx.EgressTLSSecrets) + err = lbc.addEgressMTLSSecrets(virtualServerEx.Secrets, policies) if err != nil { glog.Warningf("Error getting EgressMTLS secrets for VirtualServer %v/%v: %v", virtualServer.Namespace, virtualServer.Name, err) } @@ -2090,11 +2027,11 @@ func (lbc *LoadBalancerController) createVirtualServerEx(virtualServer *conf_v1. } policies = append(policies, vsRoutePolicies...) - err = lbc.addEgressMTLSSecrets(policies, virtualServerEx.EgressTLSSecrets) + err = lbc.addEgressMTLSSecrets(virtualServerEx.Secrets, policies) if err != nil { glog.Warningf("Error getting EgressMTLS secrets for VirtualServer %v/%v: %v", virtualServer.Namespace, virtualServer.Name, err) } - err = lbc.addJWTSecrets(vsRoutePolicies, virtualServerEx.JWTKeys) + err = lbc.addJWTSecrets(virtualServerEx.Secrets, vsRoutePolicies) if err != nil { glog.Warningf("Error getting JWT secrets for VirtualServer %v/%v: %v", virtualServer.Namespace, virtualServer.Name, err) } @@ -2108,12 +2045,12 @@ func (lbc *LoadBalancerController) createVirtualServerEx(virtualServer *conf_v1. } policies = append(policies, vsrSubroutePolicies...) - err = lbc.addJWTSecrets(vsrSubroutePolicies, virtualServerEx.JWTKeys) + err = lbc.addJWTSecrets(virtualServerEx.Secrets, vsrSubroutePolicies) if err != nil { glog.Warningf("Error getting JWT secrets for VirtualServerRoute %v/%v: %v", vsr.Namespace, vsr.Name, err) } - err = lbc.addEgressMTLSSecrets(policies, virtualServerEx.EgressTLSSecrets) + err = lbc.addEgressMTLSSecrets(virtualServerEx.Secrets, policies) if err != nil { glog.Warningf("Error getting EgressMTLS secrets for VirtualServer %v/%v: %v", virtualServer.Namespace, virtualServer.Name, err) } @@ -2227,58 +2164,84 @@ func (lbc *LoadBalancerController) getPolicies(policies []conf_v1.PolicyReferenc return result, errors } -func (lbc *LoadBalancerController) addJWTSecrets(policies []*conf_v1alpha1.Policy, jwtKeys map[string]*api_v1.Secret) error { +func (lbc *LoadBalancerController) addJWTSecrets(secrets map[string]*configs.SecretReference, policies []*conf_v1alpha1.Policy) error { for _, pol := range policies { if pol.Spec.JWTAuth == nil { continue } + secretKey := fmt.Sprintf("%v/%v", pol.Namespace, pol.Spec.JWTAuth.Secret) - secret, err := lbc.getAndValidateJWTSecret(secretKey) + secretType, secretPath, err := lbc.secretStore.GetSecret(secretKey) + + secrets[secretKey] = &configs.SecretReference{ + Type: secretType, + Path: secretPath, + Error: err, + } + if err != nil { - return fmt.Errorf("Error getting or validating the JWT secret %v for the policy %v/%v: %v", secretKey, pol.Namespace, pol.Name, err) + return err } - jwtKeys[secretKey] = secret } return nil } -func (lbc *LoadBalancerController) getIngressMTLSSecret(policies []*conf_v1alpha1.Policy) (*api_v1.Secret, error) { +func (lbc *LoadBalancerController) addIngressMTLSSecret(secrets map[string]*configs.SecretReference, policies []*conf_v1alpha1.Policy) error { for _, pol := range policies { if pol.Spec.IngressMTLS == nil { continue } + secretKey := fmt.Sprintf("%v/%v", pol.Namespace, pol.Spec.IngressMTLS.ClientCertSecret) - secret, err := lbc.getAndValidateCASecret(secretKey) - if err != nil { - return nil, fmt.Errorf("Error getting or validating the IngressMTLS secret %v for the policy %v/%v: %v", secretKey, pol.Namespace, pol.Name, err) + secretType, secretPath, err := lbc.secretStore.GetSecret(secretKey) + + secrets[secretKey] = &configs.SecretReference{ + Type: secretType, + Path: secretPath, + Error: err, } - return secret, nil + + return err } - return nil, nil + return nil } -func (lbc *LoadBalancerController) addEgressMTLSSecrets(policies []*conf_v1alpha1.Policy, egressSecrets map[string]*api_v1.Secret) error { +func (lbc *LoadBalancerController) addEgressMTLSSecrets(secrets map[string]*configs.SecretReference, policies []*conf_v1alpha1.Policy) error { for _, pol := range policies { if pol.Spec.EgressMTLS == nil { continue } if pol.Spec.EgressMTLS.TLSSecret != "" { secretKey := fmt.Sprintf("%v/%v", pol.Namespace, pol.Spec.EgressMTLS.TLSSecret) - secret, err := lbc.getAndValidateSecret(secretKey) + + secretType, secretPath, err := lbc.secretStore.GetSecret(secretKey) + + secrets[secretKey] = &configs.SecretReference{ + Type: secretType, + Path: secretPath, + Error: err, + } + if err != nil { - return fmt.Errorf("Error getting or validating the EgressMTLS secret %v for the policy %v/%v: %v", secretKey, pol.Namespace, pol.Name, err) + return err } - egressSecrets[secretKey] = secret } if pol.Spec.EgressMTLS.TrustedCertSecret != "" { secretKey := fmt.Sprintf("%v/%v", pol.Namespace, pol.Spec.EgressMTLS.TrustedCertSecret) - secret, err := lbc.getAndValidateCASecret(secretKey) + + secretType, secretPath, err := lbc.secretStore.GetSecret(secretKey) + + secrets[secretKey] = &configs.SecretReference{ + Type: secretType, + Path: secretPath, + Error: err, + } + if err != nil { - return fmt.Errorf("Error getting or validating the TrustedCert secret %v for the policy %v/%v: %v", secretKey, pol.Namespace, pol.Name, err) + return err } - egressSecrets[secretKey] = secret } } diff --git a/internal/k8s/controller_test.go b/internal/k8s/controller_test.go index 19f5b74e8f..c630bfb1f3 100644 --- a/internal/k8s/controller_test.go +++ b/internal/k8s/controller_test.go @@ -1216,27 +1216,20 @@ func TestFindPoliciesForSecret(t *testing.T) { } } -func TestAddJWTSecrets(t *testing.T) { - validSecret := &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "valid-jwk-secret", - Namespace: "default", - }, - Type: SecretTypeJWK, - Data: map[string][]byte{"jwk": nil}, +func errorComparer(e1, e2 error) bool { + if e1 == nil || e2 == nil { + return e1 == e2 } - invalidSecret := &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "invalid-jwk-secret", - Namespace: "default", - }, - Data: nil, - } + return e1.Error() == e2.Error() +} + +func TestAddJWTSecrets(t *testing.T) { + invalidErr := errors.New("invalid") tests := []struct { policies []*conf_v1alpha1.Policy - expectedJWTKeys map[string]*v1.Secret + expectedSecrets map[string]*configs.SecretReference wantErr bool msg string }{ @@ -1255,15 +1248,18 @@ func TestAddJWTSecrets(t *testing.T) { }, }, }, - expectedJWTKeys: map[string]*v1.Secret{ - "default/valid-jwk-secret": validSecret, + expectedSecrets: map[string]*configs.SecretReference{ + "default/valid-jwk-secret": { + Type: SecretTypeJWK, + Path: "/etc/nginx/secrets/default-valid-jwk-secret", + }, }, wantErr: false, msg: "test getting valid secret", }, { policies: []*conf_v1alpha1.Policy{}, - expectedJWTKeys: map[string]*v1.Secret{}, + expectedSecrets: map[string]*configs.SecretReference{}, wantErr: false, msg: "test getting valid secret with no policy", }, @@ -1281,9 +1277,9 @@ func TestAddJWTSecrets(t *testing.T) { }, }, }, - expectedJWTKeys: map[string]*v1.Secret{}, + expectedSecrets: map[string]*configs.SecretReference{}, wantErr: false, - msg: "test getting valid secret with wrong policy", + msg: "test getting invalid secret with wrong policy", }, { policies: []*conf_v1alpha1.Policy{ @@ -1294,92 +1290,70 @@ func TestAddJWTSecrets(t *testing.T) { }, Spec: conf_v1alpha1.PolicySpec{ JWTAuth: &conf_v1alpha1.JWTAuth{ - Secret: "non-existing-jwk-secret", + Secret: "invalid-jwk-secret", Realm: "My API", }, }, }, }, - expectedJWTKeys: map[string]*v1.Secret{}, - wantErr: true, - msg: "test getting secret that does not exist", + expectedSecrets: map[string]*configs.SecretReference{ + "default/invalid-jwk-secret": { + Type: SecretTypeJWK, + Error: invalidErr, + }, + }, + wantErr: true, + msg: "test getting invalid secret", }, - { - policies: []*conf_v1alpha1.Policy{ - { + } + + lbc := LoadBalancerController{ + secretStore: newFakeSecretsStore(map[string]*StoredSecret{ + "default/valid-jwk-secret": { + Secret: &v1.Secret{ ObjectMeta: meta_v1.ObjectMeta{ - Name: "jwt-policy", + Name: "valid-jwk-secret", Namespace: "default", }, - Spec: conf_v1alpha1.PolicySpec{ - JWTAuth: &conf_v1alpha1.JWTAuth{ - Secret: "invalid-jwk-secret", - Realm: "My API", - }, - }, + Type: SecretTypeJWK, }, + Path: "/etc/nginx/secrets/default-valid-jwk-secret", }, - expectedJWTKeys: map[string]*v1.Secret{}, - wantErr: true, - msg: "test getting invalid secret", - }, - } - - for _, test := range tests { - lbc := LoadBalancerController{ - secretLister: storeToSecretLister{ - &cache.FakeCustomStore{ - GetByKeyFunc: func(key string) (item interface{}, exists bool, err error) { - switch key { - case "default/valid-jwk-secret": - return validSecret, true, nil - case "default/invalid-jwk-secret": - return invalidSecret, true, errors.New("secret is missing jwk key in data") - default: - return nil, false, errors.New("GetByKey error") - } + "default/invalid-jwk-secret": { + Secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "invalid-jwk-secret", + Namespace: "default", }, + Type: SecretTypeJWK, }, + ValidationErr: invalidErr, }, - } + }), + } - jwtKeys := make(map[string]*v1.Secret) + for _, test := range tests { + result := make(map[string]*configs.SecretReference) - err := lbc.addJWTSecrets(test.policies, jwtKeys) + err := lbc.addJWTSecrets(result, test.policies) if (err != nil) != test.wantErr { t.Errorf("addJWTSecrets() returned %v, for the case of %v", err, test.msg) } - if !reflect.DeepEqual(jwtKeys, test.expectedJWTKeys) { - t.Errorf("addJWTSecrets() returned \n%+v but expected \n%+v", jwtKeys, test.expectedJWTKeys) + if diff := cmp.Diff(test.expectedSecrets, result, cmp.Comparer(errorComparer)); diff != "" { + t.Errorf("addJWTSecrets() '%v' mismatch (-want +got):\n%s", test.msg, diff) } - } } -func TestGetIngressMTLSSecret(t *testing.T) { - validSecret := &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "valid-ingress-mtls-secret", - Namespace: "default", - }, - Type: SecretTypeCA, - Data: map[string][]byte{"ca.crt": validCACert}, - } - - invalidSecret := &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "invalid-ingress-mtls-secret", - Namespace: "default", - }, - Data: nil, - } +func TestAddIngressMTLSSecret(t *testing.T) { + invalidErr := errors.New("invalid") tests := []struct { - policies []*conf_v1alpha1.Policy - expectedIngressMTLSSecret *v1.Secret - wantErr bool - msg string + policies []*conf_v1alpha1.Policy + expectedSecrets map[string]*configs.SecretReference + wantErr bool + msg string }{ { policies: []*conf_v1alpha1.Policy{ @@ -1395,15 +1369,20 @@ func TestGetIngressMTLSSecret(t *testing.T) { }, }, }, - expectedIngressMTLSSecret: validSecret, - wantErr: false, - msg: "test getting valid secret", + expectedSecrets: map[string]*configs.SecretReference{ + "default/valid-ingress-mtls-secret": { + Type: SecretTypeCA, + Path: "/etc/nginx/secrets/default-valid-ingress-mtls-secret", + }, + }, + wantErr: false, + msg: "test getting valid secret", }, { - policies: []*conf_v1alpha1.Policy{}, - expectedIngressMTLSSecret: nil, - wantErr: false, - msg: "test getting valid secret with no policy", + policies: []*conf_v1alpha1.Policy{}, + expectedSecrets: map[string]*configs.SecretReference{}, + wantErr: false, + msg: "test getting valid secret with no policy", }, { policies: []*conf_v1alpha1.Policy{ @@ -1419,9 +1398,9 @@ func TestGetIngressMTLSSecret(t *testing.T) { }, }, }, - expectedIngressMTLSSecret: nil, - wantErr: false, - msg: "test getting valid secret with wrong policy", + expectedSecrets: map[string]*configs.SecretReference{}, + wantErr: false, + msg: "test getting valid secret with wrong policy", }, { policies: []*conf_v1alpha1.Policy{ @@ -1432,95 +1411,69 @@ func TestGetIngressMTLSSecret(t *testing.T) { }, Spec: conf_v1alpha1.PolicySpec{ IngressMTLS: &conf_v1alpha1.IngressMTLS{ - ClientCertSecret: "non-existing-ingress-mtls-secret", + ClientCertSecret: "invalid-ingress-mtls-secret", }, }, }, }, - expectedIngressMTLSSecret: nil, - wantErr: true, - msg: "test getting secret that does not exist", + expectedSecrets: map[string]*configs.SecretReference{ + "default/invalid-ingress-mtls-secret": { + Type: SecretTypeCA, + Error: invalidErr, + }, + }, + wantErr: true, + msg: "test getting invalid secret", }, - { - policies: []*conf_v1alpha1.Policy{ - { + } + + lbc := LoadBalancerController{ + secretStore: newFakeSecretsStore(map[string]*StoredSecret{ + "default/valid-ingress-mtls-secret": { + Secret: &v1.Secret{ ObjectMeta: meta_v1.ObjectMeta{ - Name: "ingress-mtls-policy", + Name: "valid-ingress-mtls-secret", Namespace: "default", }, - Spec: conf_v1alpha1.PolicySpec{ - IngressMTLS: &conf_v1alpha1.IngressMTLS{ - ClientCertSecret: "invalid-ingress-mtls-secret", - }, + Type: SecretTypeCA, + }, + Path: "/etc/nginx/secrets/default-valid-ingress-mtls-secret", + }, + "default/invalid-ingress-mtls-secret": { + Secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "invalid-ingress-mtls-secret", + Namespace: "default", }, + Type: SecretTypeCA, }, + ValidationErr: invalidErr, }, - expectedIngressMTLSSecret: nil, - wantErr: true, - msg: "test getting invalid secret", - }, + }), } for _, test := range tests { - lbc := LoadBalancerController{ - secretLister: storeToSecretLister{ - &cache.FakeCustomStore{ - GetByKeyFunc: func(key string) (item interface{}, exists bool, err error) { - switch key { - case "default/valid-ingress-mtls-secret": - return validSecret, true, nil - case "default/invalid-ingress-mtls-secret": - return invalidSecret, true, errors.New("secret is missing ingress-mtls key in data") - default: - return nil, false, errors.New("GetByKey error") - } - }, - }, - }, - } + result := make(map[string]*configs.SecretReference) - secret, err := lbc.getIngressMTLSSecret(test.policies) + err := lbc.addIngressMTLSSecret(result, test.policies) if (err != nil) != test.wantErr { - t.Errorf("getIngressMTLSSecret() returned %v, for the case of %v", err, test.msg) - } - if !reflect.DeepEqual(secret, test.expectedIngressMTLSSecret) { - t.Errorf("getIngressMTLSSecret() returned \n%+v but expected \n%+v", secret, test.expectedIngressMTLSSecret) + t.Errorf("addIngressMTLSSecret() returned %v, for the case of %v", err, test.msg) } + if diff := cmp.Diff(test.expectedSecrets, result, cmp.Comparer(errorComparer)); diff != "" { + t.Errorf("addIngressMTLSSecret() '%v' mismatch (-want +got):\n%s", test.msg, diff) + } } } func TestAddEgressMTLSSecrets(t *testing.T) { - validSecret := &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "valid-egress-mtls-secret", - Namespace: "default", - }, - Type: api_v1.SecretTypeTLS, - Data: map[string][]byte{"tls.crt": validCert, "tls.key": validKey}, - } - validCASecret := &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "valid-egress-trusted-secret", - Namespace: "default", - }, - Type: SecretTypeCA, - Data: map[string][]byte{"ca.crt": validCACert}, - } - - invalidSecret := &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "invalid-egress-mtls-secret", - Namespace: "default", - }, - Data: nil, - } + invalidErr := errors.New("invalid") tests := []struct { - policies []*conf_v1alpha1.Policy - expectedEgressMTLSSecrets map[string]*v1.Secret - wantErr bool - msg string + policies []*conf_v1alpha1.Policy + expectedSecrets map[string]*configs.SecretReference + wantErr bool + msg string }{ { policies: []*conf_v1alpha1.Policy{ @@ -1536,8 +1489,11 @@ func TestAddEgressMTLSSecrets(t *testing.T) { }, }, }, - expectedEgressMTLSSecrets: map[string]*v1.Secret{ - "default/valid-egress-mtls-secret": validSecret, + expectedSecrets: map[string]*configs.SecretReference{ + "default/valid-egress-mtls-secret": { + Type: api_v1.SecretTypeTLS, + Path: "/etc/nginx/secrets/default-valid-egress-mtls-secret", + }, }, wantErr: false, msg: "test getting valid TLS secret", @@ -1556,8 +1512,11 @@ func TestAddEgressMTLSSecrets(t *testing.T) { }, }, }, - expectedEgressMTLSSecrets: map[string]*v1.Secret{ - "default/valid-egress-trusted-secret": validCASecret, + expectedSecrets: map[string]*configs.SecretReference{ + "default/valid-egress-trusted-secret": { + Type: SecretTypeCA, + Path: "/etc/nginx/secrets/default-valid-egress-trusted-secret", + }, }, wantErr: false, msg: "test getting valid TrustedCA secret", @@ -1577,18 +1536,24 @@ func TestAddEgressMTLSSecrets(t *testing.T) { }, }, }, - expectedEgressMTLSSecrets: map[string]*v1.Secret{ - "default/valid-egress-mtls-secret": validSecret, - "default/valid-egress-trusted-secret": validCASecret, + expectedSecrets: map[string]*configs.SecretReference{ + "default/valid-egress-mtls-secret": { + Type: api_v1.SecretTypeTLS, + Path: "/etc/nginx/secrets/default-valid-egress-mtls-secret", + }, + "default/valid-egress-trusted-secret": { + Type: SecretTypeCA, + Path: "/etc/nginx/secrets/default-valid-egress-trusted-secret", + }, }, wantErr: false, msg: "test getting valid secrets", }, { - policies: []*conf_v1alpha1.Policy{}, - expectedEgressMTLSSecrets: map[string]*v1.Secret{}, - wantErr: false, - msg: "test getting valid secret with no policy", + policies: []*conf_v1alpha1.Policy{}, + expectedSecrets: map[string]*configs.SecretReference{}, + wantErr: false, + msg: "test getting valid secret with no policy", }, { policies: []*conf_v1alpha1.Policy{ @@ -1604,9 +1569,9 @@ func TestAddEgressMTLSSecrets(t *testing.T) { }, }, }, - expectedEgressMTLSSecrets: map[string]*v1.Secret{}, - wantErr: false, - msg: "test getting valid secret with wrong policy", + expectedSecrets: map[string]*configs.SecretReference{}, + wantErr: false, + msg: "test getting valid secret with wrong policy", }, { policies: []*conf_v1alpha1.Policy{ @@ -1617,14 +1582,19 @@ func TestAddEgressMTLSSecrets(t *testing.T) { }, Spec: conf_v1alpha1.PolicySpec{ EgressMTLS: &conf_v1alpha1.EgressMTLS{ - TLSSecret: "non-existing-ingress-mtls-secret", + TLSSecret: "invalid-egress-mtls-secret", }, }, }, }, - expectedEgressMTLSSecrets: map[string]*v1.Secret{}, - wantErr: true, - msg: "test getting secret that does not exist", + expectedSecrets: map[string]*configs.SecretReference{ + "default/invalid-egress-mtls-secret": { + Type: api_v1.SecretTypeTLS, + Error: invalidErr, + }, + }, + wantErr: true, + msg: "test getting invalid TLS secret", }, { policies: []*conf_v1alpha1.Policy{ @@ -1635,44 +1605,75 @@ func TestAddEgressMTLSSecrets(t *testing.T) { }, Spec: conf_v1alpha1.PolicySpec{ EgressMTLS: &conf_v1alpha1.EgressMTLS{ - TLSSecret: "invalid-egress-mtls-secret", + TrustedCertSecret: "invalid-egress-trusted-secret", }, }, }, }, - expectedEgressMTLSSecrets: map[string]*v1.Secret{}, - wantErr: true, - msg: "test getting invalid secret", + expectedSecrets: map[string]*configs.SecretReference{ + "default/invalid-egress-trusted-secret": { + Type: SecretTypeCA, + Error: invalidErr, + }, + }, + wantErr: true, + msg: "test getting invalid TrustedCA secret", }, } - for _, test := range tests { - lbc := LoadBalancerController{ - secretLister: storeToSecretLister{ - &cache.FakeCustomStore{ - GetByKeyFunc: func(key string) (item interface{}, exists bool, err error) { - switch key { - case "default/valid-egress-mtls-secret": - return validSecret, true, nil - case "default/valid-egress-trusted-secret": - return validCASecret, true, nil - case "default/invalid-egress-mtls-secret": - return invalidSecret, true, errors.New("secret is missing egress-mtls key in data") - default: - return nil, false, errors.New("GetByKey error") - } + lbc := LoadBalancerController{ + secretStore: newFakeSecretsStore(map[string]*StoredSecret{ + "default/valid-egress-mtls-secret": { + Secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "valid-egress-mtls-secret", + Namespace: "default", }, + Type: api_v1.SecretTypeTLS, }, + Path: "/etc/nginx/secrets/default-valid-egress-mtls-secret", }, - } + "default/valid-egress-trusted-secret": { + Secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "valid-egress-trusted-secret", + Namespace: "default", + }, + Type: SecretTypeCA, + }, + Path: "/etc/nginx/secrets/default-valid-egress-trusted-secret", + }, + "default/invalid-egress-mtls-secret": { + Secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "invalid-egress-mtls-secret", + Namespace: "default", + }, + Type: api_v1.SecretTypeTLS, + }, + ValidationErr: invalidErr, + }, + "default/invalid-egress-trusted-secret": { + Secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "invalid-egress-trusted-secret", + Namespace: "default", + }, + Type: SecretTypeCA, + }, + ValidationErr: invalidErr, + }, + }), + } - egressTLSSecrets := make(map[string]*v1.Secret) + for _, test := range tests { + result := make(map[string]*configs.SecretReference) - err := lbc.addEgressMTLSSecrets(test.policies, egressTLSSecrets) + err := lbc.addEgressMTLSSecrets(result, test.policies) if (err != nil) != test.wantErr { t.Errorf("addEgressMTLSSecrets() returned %v, for the case of %v", err, test.msg) } - if diff := cmp.Diff(test.expectedEgressMTLSSecrets, egressTLSSecrets); diff != "" { + if diff := cmp.Diff(test.expectedSecrets, result, cmp.Comparer(errorComparer)); diff != "" { t.Errorf("addEgressMTLSSecrets() '%v' mismatch (-want +got):\n%s", test.msg, diff) } }