From 1b804fc87c5586989392116a81bc0d07047dbb87 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Thu, 15 Dec 2022 13:04:24 +0000 Subject: [PATCH] Services API: warnings The Services API should warn users about some IP addresses representations, mainly because some of them are not allowed by the golang std parsers since go 1.17 Specifically: - IPv4 addresses with leading zeros, that may cause security risks - IPv6 addresses in non canonical format, that may cause problems with controllers hotlooping or cause security issues Change-Id: Ife50a651d1b22dc4c318e42bd3e5f2e5f88ecbcd --- pkg/api/service/warnings.go | 95 ++++++++++ pkg/api/service/warnings_test.go | 244 ++++++++++++++++++++++++++ pkg/registry/core/service/strategy.go | 8 +- 3 files changed, 345 insertions(+), 2 deletions(-) create mode 100644 pkg/api/service/warnings.go create mode 100644 pkg/api/service/warnings_test.go diff --git a/pkg/api/service/warnings.go b/pkg/api/service/warnings.go new file mode 100644 index 00000000000..6d0ff8e6651 --- /dev/null +++ b/pkg/api/service/warnings.go @@ -0,0 +1,95 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + "fmt" + "net/netip" + + "k8s.io/apimachinery/pkg/util/validation/field" + api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/apis/core/helper" +) + +func GetWarningsForService(service, oldService *api.Service) []string { + if service == nil { + return nil + } + var warnings []string + + if helper.IsServiceIPSet(service) { + for i, clusterIP := range service.Spec.ClusterIPs { + warnings = append(warnings, getWarningsForIP(field.NewPath("spec").Child("clusterIPs").Index(i), clusterIP)...) + } + } + + for i, externalIP := range service.Spec.ExternalIPs { + warnings = append(warnings, getWarningsForIP(field.NewPath("spec").Child("externalIPs").Index(i), externalIP)...) + } + + if len(service.Spec.LoadBalancerIP) > 0 { + warnings = append(warnings, getWarningsForIP(field.NewPath("spec").Child("loadBalancerIP"), service.Spec.LoadBalancerIP)...) + } + + for i, cidr := range service.Spec.LoadBalancerSourceRanges { + warnings = append(warnings, getWarningsForCIDR(field.NewPath("spec").Child("loadBalancerSourceRanges").Index(i), cidr)...) + } + + return warnings +} + +func getWarningsForIP(fieldPath *field.Path, address string) []string { + // IPv4 addresses with leading zeros CVE-2021-29923 are not valid in golang since 1.17 + // This will also warn about possible future changes on the golang std library + // xref: https://issues.k8s.io/108074 + ip, err := netip.ParseAddr(address) + if err != nil { + return []string{fmt.Sprintf("%s: IP address was accepted, but will be invalid in a future Kubernetes release: %v", fieldPath, err)} + } + // A Recommendation for IPv6 Address Text Representation + // + // "All of the above examples represent the same IPv6 address. This + // flexibility has caused many problems for operators, systems + // engineers, and customers. + // ..." + // https://datatracker.ietf.org/doc/rfc5952/ + if ip.Is6() && ip.String() != address { + return []string{fmt.Sprintf("%s: IPv6 address %q is not in RFC 5952 canonical format (%q), which may cause controller apply-loops", fieldPath, address, ip.String())} + } + return []string{} +} + +func getWarningsForCIDR(fieldPath *field.Path, cidr string) []string { + // IPv4 addresses with leading zeros CVE-2021-29923 are not valid in golang since 1.17 + // This will also warn about possible future changes on the golang std library + // xref: https://issues.k8s.io/108074 + prefix, err := netip.ParsePrefix(cidr) + if err != nil { + return []string{fmt.Sprintf("%s: IP prefix was accepted, but will be invalid in a future Kubernetes release: %v", fieldPath, err)} + } + // A Recommendation for IPv6 Address Text Representation + // + // "All of the above examples represent the same IPv6 address. This + // flexibility has caused many problems for operators, systems + // engineers, and customers. + // ..." + // https://datatracker.ietf.org/doc/rfc5952/ + if prefix.Addr().Is6() && prefix.String() != cidr { + return []string{fmt.Sprintf("%s: IPv6 prefix %q is not in RFC 5952 canonical format (%q), which may cause controller apply-loops", fieldPath, cidr, prefix.String())} + } + return []string{} +} diff --git a/pkg/api/service/warnings_test.go b/pkg/api/service/warnings_test.go new file mode 100644 index 00000000000..b3ca4b09cb4 --- /dev/null +++ b/pkg/api/service/warnings_test.go @@ -0,0 +1,244 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + "reflect" + "testing" + + "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" + api "k8s.io/kubernetes/pkg/apis/core" +) + +func TestGetWarningsForServiceClusterIPs(t *testing.T) { + service := func(clusterIPs []string) *api.Service { + svc := api.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-test", + Namespace: "ns", + }, + Spec: api.ServiceSpec{ + Type: api.ServiceTypeClusterIP, + }, + } + + if len(clusterIPs) > 0 { + svc.Spec.ClusterIP = clusterIPs[0] + svc.Spec.ClusterIPs = clusterIPs + } + return &svc + } + + tests := []struct { + name string + service *api.Service + oldService *api.Service + want []string + }{ + { + name: "IPv4 No failures", + service: service([]string{"192.12.2.2"}), + }, + { + name: "IPv6 No failures", + service: service([]string{"2001:db8::2"}), + }, + { + name: "IPv4 with leading zeros", + service: service([]string{"192.012.2.2"}), + want: []string{ + `spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + }, + }, + { + name: "Dual Stack IPv4-IPv6 and IPv4 with leading zeros", + service: service([]string{"192.012.2.2", "2001:db8::2"}), + want: []string{ + `spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + }, + }, + { + name: "Dual Stack IPv6-IPv4 and IPv4 with leading zeros", + service: service([]string{"2001:db8::2", "192.012.2.2"}), + want: []string{ + `spec.clusterIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + }, + }, + { + name: "IPv6 non canonical format", + service: service([]string{"2001:db8:0:0::2"}), + want: []string{ + `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + }, + }, + { + name: "Dual Stack IPv4-IPv6 and IPv6 non-canonical format", + service: service([]string{"192.12.2.2", "2001:db8:0:0::2"}), + want: []string{ + `spec.clusterIPs[1]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + }, + }, + { + name: "Dual Stack IPv6-IPv4 and IPv6 non-canonical formats", + service: service([]string{"2001:db8:0:0::2", "192.12.2.2"}), + want: []string{ + `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + }, + }, + { + name: "Dual Stack IPv4-IPv6 and IPv4 with leading zeros and IPv6 non-canonical format", + service: service([]string{"192.012.2.2", "2001:db8:0:0::2"}), + want: []string{ + `spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + `spec.clusterIPs[1]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + }, + }, + { + name: "Dual Stack IPv6-IPv4 and IPv4 with leading zeros and IPv6 non-canonical format", + service: service([]string{"2001:db8:0:0::2", "192.012.2.2"}), + want: []string{ + `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + `spec.clusterIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + }, + }, + { + name: "Service with all IPs fields with errors", + service: &api.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-test", + Namespace: "ns", + }, + Spec: api.ServiceSpec{ + ClusterIP: "2001:db8:0:0::2", + ClusterIPs: []string{"2001:db8:0:0::2", "192.012.2.2"}, + ExternalIPs: []string{"2001:db8:1:0::2", "10.012.2.2"}, + LoadBalancerIP: "10.001.1.1", + LoadBalancerSourceRanges: []string{"2001:db8:1:0::/64", "10.012.2.0/24"}, + Type: api.ServiceTypeClusterIP, + }, + }, + want: []string{ + `spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + `spec.clusterIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + `spec.externalIPs[0]: IPv6 address "2001:db8:1:0::2" is not in RFC 5952 canonical format ("2001:db8:1::2"), which may cause controller apply-loops`, + `spec.externalIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("10.012.2.2"): IPv4 field has octet with leading zero`, + `spec.loadBalancerIP: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("10.001.1.1"): IPv4 field has octet with leading zero`, + `spec.loadBalancerSourceRanges[0]: IPv6 prefix "2001:db8:1:0::/64" is not in RFC 5952 canonical format ("2001:db8:1::/64"), which may cause controller apply-loops`, + `spec.loadBalancerSourceRanges[1]: IP prefix was accepted, but will be invalid in a future Kubernetes release: netip.ParsePrefix("10.012.2.0/24"): ParseAddr("10.012.2.0"): IPv4 field has octet with leading zero`, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := GetWarningsForService(tt.service, tt.oldService); !reflect.DeepEqual(got, tt.want) { + t.Errorf("GetWarningsForService() = %v", cmp.Diff(got, tt.want)) + } + }) + } +} + +func Test_getWarningsForIP(t *testing.T) { + tests := []struct { + name string + fieldPath *field.Path + address string + want []string + }{ + { + name: "IPv4 No failures", + address: "192.12.2.2", + fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0), + want: []string{}, + }, + { + name: "IPv6 No failures", + address: "2001:db8::2", + fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0), + want: []string{}, + }, + { + name: "IPv4 with leading zeros", + address: "192.012.2.2", + fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0), + want: []string{ + `spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`, + }, + }, + { + name: "IPv6 non-canonical format", + address: "2001:db8:0:0::2", + fieldPath: field.NewPath("spec").Child("loadBalancerIP"), + want: []string{ + `spec.loadBalancerIP: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := getWarningsForIP(tt.fieldPath, tt.address); !reflect.DeepEqual(got, tt.want) { + t.Errorf("getWarningsForIP() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_getWarningsForCIDR(t *testing.T) { + tests := []struct { + name string + fieldPath *field.Path + cidr string + want []string + }{ + { + name: "IPv4 No failures", + cidr: "192.12.2.0/24", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: []string{}, + }, + { + name: "IPv6 No failures", + cidr: "2001:db8::/64", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: []string{}, + }, + { + name: "IPv4 with leading zeros", + cidr: "192.012.2.0/24", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: []string{ + `spec.loadBalancerSourceRanges[0]: IP prefix was accepted, but will be invalid in a future Kubernetes release: netip.ParsePrefix("192.012.2.0/24"): ParseAddr("192.012.2.0"): IPv4 field has octet with leading zero`, + }, + }, + { + name: "IPv6 non-canonical format", + cidr: "2001:db8:0:0::/64", + fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0), + want: []string{ + `spec.loadBalancerSourceRanges[0]: IPv6 prefix "2001:db8:0:0::/64" is not in RFC 5952 canonical format ("2001:db8::/64"), which may cause controller apply-loops`, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := getWarningsForCIDR(tt.fieldPath, tt.cidr); !reflect.DeepEqual(got, tt.want) { + t.Errorf("getWarningsForCIDR() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index e7e4e21e36a..acb3f3d66a2 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -25,8 +25,10 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/storage/names" "k8s.io/kubernetes/pkg/api/legacyscheme" + serviceapi "k8s.io/kubernetes/pkg/api/service" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/validation" + "sigs.k8s.io/structured-merge-diff/v4/fieldpath" ) @@ -83,7 +85,9 @@ func (svcStrategy) Validate(ctx context.Context, obj runtime.Object) field.Error } // WarningsOnCreate returns warnings for the creation of the given object. -func (svcStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { return nil } +func (svcStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { + return serviceapi.GetWarningsForService(obj.(*api.Service), nil) +} // Canonicalize normalizes the object after validation. func (svcStrategy) Canonicalize(obj runtime.Object) { @@ -100,7 +104,7 @@ func (strategy svcStrategy) ValidateUpdate(ctx context.Context, obj, old runtime // WarningsOnUpdate returns warnings for the given update. func (svcStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return nil + return serviceapi.GetWarningsForService(obj.(*api.Service), old.(*api.Service)) } func (svcStrategy) AllowUnconditionalUpdate() bool {