Skip to content

Commit 31e9d13

Browse files
javorszkyjavorszky
authored
Add additional validation to otel endpoint (#7909)
* Add additional validation to otel endpoint * Add new ValidateURI function and tests * Further refine URI validation and add more tests
1 parent a477f42 commit 31e9d13

File tree

4 files changed

+340
-0
lines changed

4 files changed

+340
-0
lines changed

‎internal/configs/configmaps.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -754,6 +754,11 @@ func parseConfigMapOpenTelemetry(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *
754754
if otelExporterEndpoint, exists := cfgm.Data["otel-exporter-endpoint"]; exists {
755755
otelExporterEndpoint = strings.TrimSpace(otelExporterEndpoint)
756756
if otelExporterEndpoint != "" {
757+
if err := validation.ValidateURI(otelExporterEndpoint); err != nil {
758+
nl.Warn(l, err)
759+
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, err.Error())
760+
return nil, err
761+
}
757762
cfgParams.MainOtelExporterEndpoint = otelExporterEndpoint
758763
}
759764
}

‎internal/configs/configmaps_test.go‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1403,6 +1403,34 @@ func TestOpenTelemetryConfigurationSuccess(t *testing.T) {
14031403
expectedServiceName: "nginx-ingress-controller:nginx",
14041404
msg: "endpoint set, minimal config",
14051405
},
1406+
{
1407+
configMap: &v1.ConfigMap{
1408+
Data: map[string]string{
1409+
"otel-exporter-endpoint": "subdomain.goes.on.and.on.and.on.and.on.example.com",
1410+
},
1411+
},
1412+
expectedLoadModule: true,
1413+
expectedExporterEndpoint: "subdomain.goes.on.and.on.and.on.and.on.example.com",
1414+
expectedExporterHeaderName: "",
1415+
expectedExporterHeaderValue: "",
1416+
expectedServiceName: "",
1417+
expectedTraceInHTTP: false,
1418+
msg: "endpoint set, complicated long subdomain",
1419+
},
1420+
{
1421+
configMap: &v1.ConfigMap{
1422+
Data: map[string]string{
1423+
"otel-exporter-endpoint": "localhost:9933",
1424+
},
1425+
},
1426+
expectedLoadModule: true,
1427+
expectedExporterEndpoint: "localhost:9933",
1428+
expectedExporterHeaderName: "",
1429+
expectedExporterHeaderValue: "",
1430+
expectedServiceName: "",
1431+
expectedTraceInHTTP: false,
1432+
msg: "endpoint set, hostname and port no scheme",
1433+
},
14061434
{
14071435
configMap: &v1.ConfigMap{
14081436
Data: map[string]string{
@@ -1595,6 +1623,76 @@ func TestOpenTelemetryConfigurationInvalid(t *testing.T) {
15951623
expectedTraceInHTTP: true,
15961624
msg: "partially invalid, header name missing, trace in http set",
15971625
},
1626+
{
1627+
configMap: &v1.ConfigMap{
1628+
Data: map[string]string{
1629+
"otel-exporter-endpoint": "something%invalid*30here",
1630+
},
1631+
},
1632+
expectedLoadModule: false,
1633+
expectedExporterEndpoint: "",
1634+
expectedExporterHeaderName: "",
1635+
expectedExporterHeaderValue: "",
1636+
expectedServiceName: "",
1637+
expectedTraceInHTTP: false,
1638+
msg: "invalid, endpoint does not look like a host",
1639+
},
1640+
{
1641+
configMap: &v1.ConfigMap{
1642+
Data: map[string]string{
1643+
"otel-exporter-endpoint": "localhost:0",
1644+
},
1645+
},
1646+
expectedLoadModule: false,
1647+
expectedExporterEndpoint: "",
1648+
expectedExporterHeaderName: "",
1649+
expectedExporterHeaderValue: "",
1650+
expectedServiceName: "",
1651+
expectedTraceInHTTP: false,
1652+
msg: "invalid, port is outside of range down",
1653+
},
1654+
{
1655+
configMap: &v1.ConfigMap{
1656+
Data: map[string]string{
1657+
"otel-exporter-endpoint": "localhost:99999",
1658+
},
1659+
},
1660+
expectedLoadModule: false,
1661+
expectedExporterEndpoint: "",
1662+
expectedExporterHeaderName: "",
1663+
expectedExporterHeaderValue: "",
1664+
expectedServiceName: "",
1665+
expectedTraceInHTTP: false,
1666+
msg: "invalid, port is outside of range up",
1667+
},
1668+
{
1669+
configMap: &v1.ConfigMap{
1670+
Data: map[string]string{
1671+
"otel-exporter-endpoint": "fe80::1",
1672+
},
1673+
},
1674+
expectedLoadModule: false,
1675+
expectedExporterEndpoint: "",
1676+
expectedExporterHeaderName: "",
1677+
expectedExporterHeaderValue: "",
1678+
expectedServiceName: "",
1679+
expectedTraceInHTTP: false,
1680+
msg: "invalid, endpoint is an ipv6 address",
1681+
},
1682+
{
1683+
configMap: &v1.ConfigMap{
1684+
Data: map[string]string{
1685+
"otel-exporter-endpoint": "thisisaverylongsubdomainthatexceedsatotalofsixtythreecharactersz.example.com",
1686+
},
1687+
},
1688+
expectedLoadModule: false,
1689+
expectedExporterEndpoint: "",
1690+
expectedExporterHeaderName: "",
1691+
expectedExporterHeaderValue: "",
1692+
expectedServiceName: "",
1693+
expectedTraceInHTTP: false,
1694+
msg: "invalid, subdomain is more than 63 characters long",
1695+
},
15981696
}
15991697

16001698
isPlus := false

‎internal/validation/validation.go‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,17 @@
11
package validation
22

33
import (
4+
"errors"
45
"fmt"
6+
"net/netip"
7+
"net/url"
58
"regexp"
69
"strconv"
710
"strings"
811
)
912

13+
const schemeSeparator = "://"
14+
1015
var (
1116
validDNSRegex = regexp.MustCompile(`^(?:[A-Za-z0-9][A-Za-z0-9-]{1,62}\.)([A-Za-z0-9-]{1,63}\.)*[A-Za-z]{2,63}(?::\d{1,5})?$`)
1217
validIPRegex = regexp.MustCompile(`^(?:(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])(?::\d{1,5})?$`)
@@ -51,3 +56,125 @@ func ValidateHost(host string) error {
5156
}
5257
return fmt.Errorf("error parsing host: %s not a valid host", host)
5358
}
59+
60+
// URIValidationOption defines a functional option pattern for configuring the
61+
// unexported uriValidator that gets used in ValidateURI.
62+
type URIValidationOption func(u *uriValidator)
63+
64+
type uriValidator struct {
65+
allowedSchemes map[string]struct{}
66+
userAllowed bool
67+
defaultScheme string
68+
}
69+
70+
// WithAllowedSchemes configures a URIValidator with allowed URI schemes. By
71+
// default, http and https are the only schemes considered valid. This option
72+
// allows changing the allowed schemes.
73+
func WithAllowedSchemes(allowedSchemes ...string) URIValidationOption {
74+
return func(cfg *uriValidator) {
75+
schemes := make(map[string]struct{})
76+
for _, scheme := range allowedSchemes {
77+
schemes[scheme] = struct{}{}
78+
}
79+
80+
cfg.allowedSchemes = schemes
81+
}
82+
}
83+
84+
// WithUserAllowed configures a URIValidator with a flag for whether user
85+
// information is allowed in the URI. Defaults to false. It is not recommended
86+
// to pass user information in a URL as it's generally considered to be unsafe.
87+
func WithUserAllowed(userAllowed bool) URIValidationOption {
88+
return func(cfg *uriValidator) {
89+
cfg.userAllowed = userAllowed
90+
}
91+
}
92+
93+
// WithDefaultScheme configures a URIValidator with a default scheme that
94+
// gets used if no scheme is present in the incoming URI. Defaults to "https".
95+
func WithDefaultScheme(defaultScheme string) URIValidationOption {
96+
return func(cfg *uriValidator) {
97+
cfg.defaultScheme = defaultScheme
98+
}
99+
}
100+
101+
// ValidateURI is a more robust extensible function to validate URIs. It
102+
// improved on ValidateHost as that one wasn't able to handle addresses that
103+
// start with a scheme.
104+
func ValidateURI(uri string, options ...URIValidationOption) error {
105+
cfg := &uriValidator{
106+
allowedSchemes: map[string]struct{}{
107+
"http": {},
108+
"https": {},
109+
},
110+
userAllowed: false,
111+
defaultScheme: "https",
112+
}
113+
114+
// Apply options to the configuration
115+
for _, option := range options {
116+
option(cfg)
117+
}
118+
119+
// Check if the incoming uri is coming in with a scheme. At this point we'll
120+
// assume that any uri that does have a scheme will also have the :// in it.
121+
// If the uri does not have a scheme, let's add the default one so that
122+
// url.Parse can deal with situations like "localhost:80".
123+
if !strings.Contains(uri, schemeSeparator) {
124+
uri = cfg.defaultScheme + schemeSeparator + uri
125+
}
126+
127+
parsed, err := url.Parse(uri)
128+
if err != nil {
129+
return fmt.Errorf("error parsing uri: %w", err)
130+
}
131+
132+
// Check whether the posted scheme is valid for the allowed list.
133+
if _, ok := cfg.allowedSchemes[parsed.Scheme]; !ok {
134+
return fmt.Errorf("scheme %s is not allowed", parsed.Scheme)
135+
}
136+
137+
// Check whether the user:pass pattern is not allowed, but we have one.
138+
if !cfg.userAllowed && parsed.User != nil {
139+
return errors.New("user is not allowed")
140+
}
141+
142+
// Check whether we're dealing with an IPV6 address.
143+
checkIPv6 := parsed.Host
144+
if strings.Contains(checkIPv6, "[") {
145+
checkIPv6 = parsed.Hostname()
146+
}
147+
148+
if ip, err := netip.ParseAddr(checkIPv6); err == nil && !ip.Is4() {
149+
return fmt.Errorf("ipv6 addresses are not allowed")
150+
}
151+
152+
// Check whether the ports posted are valid.
153+
if parsed.Port() != "" {
154+
// Turn the string port into an integer and check if it's in the correct
155+
// range. The net.url.Parse does not check whether the port is the allowed
156+
// value, only that it's syntactically correct. Similarly, the
157+
// net.SplitHostPort function also doesn't check whether the value is
158+
// correct.
159+
numericPort, err := strconv.Atoi(parsed.Port())
160+
if err != nil {
161+
return fmt.Errorf("invalid port %s: %w", parsed.Port(), err)
162+
}
163+
164+
if err = ValidatePort(numericPort); err != nil {
165+
return fmt.Errorf("invalid port %s: %w", parsed.Port(), err)
166+
}
167+
}
168+
169+
// Check whether each part of the domain is not too long.
170+
// This should really be octets
171+
for _, part := range strings.Split(parsed.Hostname(), ".") {
172+
// turn each part into a byte array to get a length of octets.
173+
// max length of a subdomain is 63 octets per RFC 1035.
174+
if len([]byte(part)) > 63 {
175+
return fmt.Errorf("invalid hostname part %s, value must be between 1 and 63 octets", part)
176+
}
177+
}
178+
179+
return nil
180+
}

‎internal/validation/validation_test.go‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,3 +95,113 @@ func TestValidateHost(t *testing.T) {
9595
}
9696
}
9797
}
98+
99+
func TestValidateURI(t *testing.T) {
100+
tests := []struct {
101+
name string
102+
uri string
103+
options []URIValidationOption
104+
wantErr bool
105+
}{
106+
{
107+
name: "simple uri with scheme",
108+
uri: "https://localhost:8080",
109+
options: []URIValidationOption{},
110+
wantErr: false,
111+
},
112+
{
113+
name: "simple uri without scheme",
114+
uri: "localhost:8080",
115+
options: []URIValidationOption{},
116+
wantErr: false,
117+
},
118+
{
119+
name: "uri with out of bounds port down",
120+
uri: "http://localhost:0",
121+
options: []URIValidationOption{},
122+
wantErr: true,
123+
},
124+
{
125+
name: "uri with out of bounds port up",
126+
uri: "http://localhost:65536",
127+
options: []URIValidationOption{},
128+
wantErr: true,
129+
},
130+
{
131+
name: "uri with bad port",
132+
uri: "http://localhost:abc",
133+
options: []URIValidationOption{},
134+
wantErr: true,
135+
},
136+
{
137+
name: "uri with username and password and allowed",
138+
uri: "http://user:password@localhost",
139+
options: []URIValidationOption{
140+
WithUserAllowed(true),
141+
},
142+
wantErr: false,
143+
},
144+
{
145+
name: "uri with username and password and not allowed",
146+
uri: "http://user:password@localhost",
147+
options: []URIValidationOption{},
148+
wantErr: true,
149+
},
150+
{
151+
name: "uri with http scheme but that's not allowed",
152+
uri: "http://localhost",
153+
options: []URIValidationOption{
154+
WithAllowedSchemes("https"),
155+
},
156+
wantErr: true,
157+
},
158+
{
159+
name: "uri with https scheme but that's not allowed",
160+
uri: "https://localhost",
161+
options: []URIValidationOption{
162+
WithAllowedSchemes("http"),
163+
},
164+
wantErr: true,
165+
},
166+
{
167+
name: "uri with no scheme, default set to https, not allowed",
168+
uri: "localhost",
169+
options: []URIValidationOption{
170+
WithDefaultScheme("https"),
171+
WithAllowedSchemes("http"),
172+
},
173+
wantErr: true,
174+
},
175+
{
176+
name: "uri that is an ipv6 address with a port",
177+
uri: "https://[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:17000",
178+
options: []URIValidationOption{},
179+
wantErr: true,
180+
},
181+
{
182+
name: "uri that is an ipv6 address without a port",
183+
uri: "https://2001:0db8:85a3:0000:0000:8a2e:0370:7334",
184+
options: []URIValidationOption{},
185+
wantErr: true,
186+
},
187+
{
188+
name: "uri that is a short ipv6 without port without scheme",
189+
uri: "fe80::1",
190+
options: []URIValidationOption{},
191+
wantErr: true,
192+
},
193+
{
194+
name: "uri that is a short ipv6 with a port without scheme",
195+
uri: "[fe80::1]:80",
196+
options: []URIValidationOption{},
197+
wantErr: true,
198+
},
199+
}
200+
for _, tt := range tests {
201+
t.Run(tt.name, func(t *testing.T) {
202+
if err := ValidateURI(tt.uri, tt.options...); (err != nil) != tt.wantErr {
203+
t.Errorf("ValidateURI() error = %v, wantErr %v", err, tt.wantErr)
204+
}
205+
})
206+
}
207+
}

0 commit comments

Comments
 (0)