Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Network config sanitization #7395

Merged
merged 1 commit into from
Mar 27, 2020
Merged

Network config sanitization #7395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/apis/config/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,11 @@ func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) {
field: &nc.ContainerConcurrencyMaxLimit,
}} {
if raw, ok := data[i64.key]; ok {
if val, err := strconv.ParseInt(raw, 10, 64); err != nil {
val, err := strconv.ParseInt(raw, 10, 64)
if err != nil {
return nil, err
} else {
*i64.field = val
}
*i64.field = val
}
}

Expand Down
37 changes: 19 additions & 18 deletions pkg/network/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,66 +193,67 @@ const (
HTTPRedirected HTTPProtocol = "redirected"
)

func defaultConfig() *Config {
return &Config{
DefaultIngressClass: IstioIngressClassName,
DefaultCertificateClass: CertManagerCertificateClassName,
DomainTemplate: DefaultDomainTemplate,
TagTemplate: DefaultTagTemplate,
AutoTLS: false,
HTTPProtocol: HTTPEnabled,
}
}

// NewConfigFromConfigMap creates a Config from the supplied ConfigMap
func NewConfigFromConfigMap(configMap *corev1.ConfigMap) (*Config, error) {
nc := &Config{}
nc := defaultConfig()
if _, ok := configMap.Data[IstioOutboundIPRangesKey]; ok {
// Until the next version is released, the validation check is enabled to notify users who configure some value.
// TODO(0.15): Until the next version is released, the validation check is
// enabled to notify users who configure this value.
logger := logging.FromContext(context.Background()).Named(configMap.Name)
logger.Warnf("%q is deprecated as outbound network access is enabled by default now. Remove it from config-network", IstioOutboundIPRangesKey)
}

nc.DefaultIngressClass = IstioIngressClassName
if ingressClass, ok := configMap.Data[DefaultIngressClassKey]; ok {
nc.DefaultIngressClass = ingressClass
} else if ingressClass, ok := configMap.Data[DeprecatedDefaultIngressClassKey]; ok {
nc.DefaultIngressClass = ingressClass
}

nc.DefaultCertificateClass = CertManagerCertificateClassName
if certClass, ok := configMap.Data[DefaultCertificateClassKey]; ok {
nc.DefaultCertificateClass = certClass
}

// Blank DomainTemplate makes no sense so use our default
if dt, ok := configMap.Data[DomainTemplateKey]; !ok {
nc.DomainTemplate = DefaultDomainTemplate
} else {
if dt, ok := configMap.Data[DomainTemplateKey]; ok {
t, err := template.New("domain-template").Parse(dt)
if err != nil {
return nil, err
}
if err := checkDomainTemplate(t); err != nil {
return nil, err
}

nc.DomainTemplate = dt
}

// Blank TagTemplate makes no sense so use our default
if tt, ok := configMap.Data[TagTemplateKey]; !ok {
nc.TagTemplate = DefaultTagTemplate
} else {
if tt, ok := configMap.Data[TagTemplateKey]; ok {
t, err := template.New("tag-template").Parse(tt)
if err != nil {
return nil, err
}
if err := checkTagTemplate(t); err != nil {
return nil, err
}

nc.TagTemplate = tt
}

nc.AutoTLS = strings.EqualFold(configMap.Data[AutoTLSKey], "enabled")

switch strings.ToLower(configMap.Data[HTTPProtocolKey]) {
case string(HTTPEnabled):
nc.HTTPProtocol = HTTPEnabled
case "":
// If HTTPProtocol is not set in the config-network, we set the default value
// to HTTPEnabled.
nc.HTTPProtocol = HTTPEnabled
case "", string(HTTPEnabled):
// If HTTPProtocol is not set in the config-network, default is already
// set to HTTPEnabled.
case string(HTTPDisabled):
nc.HTTPProtocol = HTTPDisabled
case string(HTTPRedirected):
Expand Down
200 changes: 66 additions & 134 deletions pkg/network/network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,10 @@ func TestOurConfig(t *testing.T) {
if _, err := NewConfigFromConfigMap(cm); err != nil {
t.Errorf("NewConfigFromConfigMap(actual) = %v", err)
}
if _, err := NewConfigFromConfigMap(example); err != nil {
if got, err := NewConfigFromConfigMap(example); err != nil {
t.Errorf("NewConfigFromConfigMap(example) = %v", err)
} else if want := defaultConfig(); !cmp.Equal(got, want) {
t.Errorf("ExampleConfig does not match default confif: (-want,+got):\n%s", cmp.Diff(want, got))
}
}

Expand All @@ -54,25 +56,13 @@ func TestConfiguration(t *testing.T) {
name string
wantErr bool
wantConfig *Config
config *corev1.ConfigMap
data map[string]string
}{{
name: "network configuration with no network input",
wantErr: false,
wantConfig: &Config{
DefaultIngressClass: "istio.ingress.networking.knative.dev",
DefaultCertificateClass: CertManagerCertificateClassName,
DomainTemplate: DefaultDomainTemplate,
TagTemplate: DefaultTagTemplate,
HTTPProtocol: HTTPEnabled,
},
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
},
name: "network configuration with no network input",
wantErr: false,
wantConfig: defaultConfig(),
}, {
name: "network configuration with non-Istio ingress type",
name: "network configuration with non-default ingress type",
wantErr: false,
wantConfig: &Config{
DefaultIngressClass: "foo-ingress",
Expand All @@ -81,14 +71,8 @@ func TestConfiguration(t *testing.T) {
TagTemplate: DefaultTagTemplate,
HTTPProtocol: HTTPEnabled,
},
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
},
data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
},
}, {
name: "network configuration with non-Cert-Manager Certificate type",
Expand All @@ -100,14 +84,8 @@ func TestConfiguration(t *testing.T) {
TagTemplate: DefaultTagTemplate,
HTTPProtocol: HTTPEnabled,
},
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
DefaultCertificateClassKey: "foo-cert",
},
data: map[string]string{
DefaultCertificateClassKey: "foo-cert",
},
}, {
name: "network configuration with diff domain template",
Expand All @@ -119,87 +97,54 @@ func TestConfiguration(t *testing.T) {
TagTemplate: DefaultTagTemplate,
HTTPProtocol: HTTPEnabled,
},
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
DomainTemplateKey: nonDefaultDomainTemplate,
},
data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
DomainTemplateKey: nonDefaultDomainTemplate,
},
}, {
name: "network configuration with blank domain template",
wantErr: true,
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
DomainTemplateKey: "",
},
data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
DomainTemplateKey: "",
},
}, {
name: "network configuration with bad domain template",
wantErr: true,
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
// This is missing a closing brace.
DomainTemplateKey: "{{.Namespace}.{{.Name}}.{{.Domain}}",
},
data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
// This is missing a closing brace.
DomainTemplateKey: "{{.Namespace}.{{.Name}}.{{.Domain}}",
},
}, {
name: "network configuration with bad domain template",
wantErr: true,
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
// This is missing a closing brace.
DomainTemplateKey: "{{.Namespace}.{{.Name}}.{{.Domain}}",
},
data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
// This is missing a closing brace.
DomainTemplateKey: "{{.Namespace}.{{.Name}}.{{.Domain}}",
},
}, {
name: "network configuration with bad url",
wantErr: true,
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
// Paths are disallowed
DomainTemplateKey: "{{.Domain}}/{{.Namespace}}/{{.Name}}.",
},
data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
// Paths are disallowed
DomainTemplateKey: "{{.Domain}}/{{.Namespace}}/{{.Name}}.",
},
}, {
name: "network configuration with bad variable",
wantErr: true,
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
// Bad variable
DomainTemplateKey: "{{.Name}}.{{.NAmespace}}.{{.Domain}}",
},
data: map[string]string{
DefaultIngressClassKey: "foo-ingress",
// Bad variable
DomainTemplateKey: "{{.Name}}.{{.NAmespace}}.{{.Domain}}",
},
}, {
name: "network configuration with Auto TLS enabled",
name: "network configuration with Auto TLS enabled",
data: map[string]string{
AutoTLSKey: "enabled",
},
wantErr: false,
wantConfig: &Config{
DefaultIngressClass: "istio.ingress.networking.knative.dev",
Expand All @@ -209,17 +154,11 @@ func TestConfiguration(t *testing.T) {
AutoTLS: true,
HTTPProtocol: HTTPEnabled,
},
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
AutoTLSKey: "enabled",
},
},
}, {
name: "network configuration with Auto TLS disabled",
name: "network configuration with Auto TLS disabled",
data: map[string]string{
AutoTLSKey: "disabled",
},
wantErr: false,
wantConfig: &Config{
DefaultIngressClass: "istio.ingress.networking.knative.dev",
Expand All @@ -229,17 +168,12 @@ func TestConfiguration(t *testing.T) {
AutoTLS: false,
HTTPProtocol: HTTPEnabled,
},
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
AutoTLSKey: "disabled",
},
},
}, {
name: "network configuration with HTTPProtocol disabled",
name: "network configuration with HTTPProtocol disabled",
data: map[string]string{
AutoTLSKey: "enabled",
HTTPProtocolKey: "Disabled",
},
wantErr: false,
wantConfig: &Config{
DefaultIngressClass: "istio.ingress.networking.knative.dev",
Expand All @@ -249,18 +183,12 @@ func TestConfiguration(t *testing.T) {
AutoTLS: true,
HTTPProtocol: HTTPDisabled,
},
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
AutoTLSKey: "enabled",
HTTPProtocolKey: "Disabled",
},
},
}, {
name: "network configuration with HTTPProtocol redirected",
name: "network configuration with HTTPProtocol redirected",
data: map[string]string{
AutoTLSKey: "enabled",
HTTPProtocolKey: "Redirected",
},
wantErr: false,
wantConfig: &Config{
DefaultIngressClass: "istio.ingress.networking.knative.dev",
Expand All @@ -270,21 +198,25 @@ func TestConfiguration(t *testing.T) {
AutoTLS: true,
HTTPProtocol: HTTPRedirected,
},
config: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: map[string]string{
AutoTLSKey: "enabled",
HTTPProtocolKey: "Redirected",
},
}, {
name: "network configuration with HTTPProtocol bad",
data: map[string]string{
AutoTLSKey: "enabled",
HTTPProtocolKey: "under-the-bridge",
},
wantErr: true,
}}

for _, tt := range networkConfigTests {
t.Run(tt.name, func(t *testing.T) {
actualConfig, err := NewConfigFromConfigMap(tt.config)
config := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: system.Namespace(),
Name: ConfigName,
},
Data: tt.data,
}
actualConfig, err := NewConfigFromConfigMap(config)
if (err != nil) != tt.wantErr {
t.Fatalf("Test: %q; NewConfigFromConfigMap() error = %v, WantErr %v",
tt.name, err, tt.wantErr)
Expand Down