From f5c5d2f5c661e0725a314d62b8b2e9f57083ea75 Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Mon, 14 Nov 2022 16:24:40 -0800 Subject: [PATCH] auto-config: relax node name validation for JWT authorization (#15370) * auto-config: relax node name validation for JWT authorization This changes the JWT authorization logic to allow all non-whitespace, non-quote characters when validating node names. Consul had previously allowed these characters in node names, until this validation was added to fix a security vulnerability with whitespace/quotes being passed to the `bexpr` library. This unintentionally broke node names with characters like `.` which aren't related to this vulnerability. * Update website/content/docs/agent/config/cli-flags.mdx Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com> Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com> --- .changelog/15370.txt | 3 +++ agent/config/builder.go | 4 ++++ agent/consul/auto_config_endpoint.go | 3 ++- website/content/docs/agent/config/cli-flags.mdx | 1 + 4 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 .changelog/15370.txt diff --git a/.changelog/15370.txt b/.changelog/15370.txt new file mode 100644 index 000000000..dca44b37c --- /dev/null +++ b/.changelog/15370.txt @@ -0,0 +1,3 @@ +```release-note:improvement +auto-config: Relax the validation on auto-config JWT authorization to allow non-whitespace, non-quote characters in node names. +``` diff --git a/agent/config/builder.go b/agent/config/builder.go index 6645b3a33..f77054db7 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -1258,6 +1258,10 @@ func (b *builder) validate(rt RuntimeConfig) error { b.warn("Node name %q will not be discoverable "+ "via DNS due to invalid characters. Valid characters include "+ "all alpha-numerics and dashes.", rt.NodeName) + case consul.InvalidNodeName.MatchString(rt.NodeName): + // todo(kyhavlov): Add stronger validation here for node names. + b.warn("Found invalid characters in node name %q - whitespace and quotes "+ + "(', \", `) cannot be used with auto-config.", rt.NodeName) case len(rt.NodeName) > dns.MaxLabelLength: b.warn("Node name %q will not be discoverable "+ "via DNS due to it being too long. Valid lengths are between "+ diff --git a/agent/consul/auto_config_endpoint.go b/agent/consul/auto_config_endpoint.go index 7eda55b67..e33fb19d4 100644 --- a/agent/consul/auto_config_endpoint.go +++ b/agent/consul/auto_config_endpoint.go @@ -57,6 +57,7 @@ type jwtAuthorizer struct { // This includes an extra single-quote character not specified in the grammar for safety in case it is later added. // https://github.com/hashicorp/go-bexpr/blob/v0.1.11/grammar/grammar.peg#L188-L191 var invalidSegmentName = regexp.MustCompile("[`'\"\\s]+") +var InvalidNodeName = invalidSegmentName func (a *jwtAuthorizer) Authorize(req *pbautoconf.AutoConfigRequest) (AutoConfigOptions, error) { // perform basic JWT Authorization @@ -70,7 +71,7 @@ func (a *jwtAuthorizer) Authorize(req *pbautoconf.AutoConfigRequest) (AutoConfig // This is not the cleanest way to prevent this behavior. Ideally, the bexpr would allow us to // inject a variable on the RHS for comparison as well, but it would be a complex change to implement // that would likely break backwards-compatibility in certain circumstances. - if dns.InvalidNameRe.MatchString(req.Node) { + if InvalidNodeName.MatchString(req.Node) { return AutoConfigOptions{}, fmt.Errorf("Invalid request field. %v = `%v`", "node", req.Node) } if invalidSegmentName.MatchString(req.Segment) { diff --git a/website/content/docs/agent/config/cli-flags.mdx b/website/content/docs/agent/config/cli-flags.mdx index 8c480778d..e75d476cc 100644 --- a/website/content/docs/agent/config/cli-flags.mdx +++ b/website/content/docs/agent/config/cli-flags.mdx @@ -482,6 +482,7 @@ information. - `-node` ((#\_node)) - The name of this node in the cluster. This must be unique within the cluster. By default this is the hostname of the machine. + The node name cannot contain whitespace or quotation marks. To query the node from DNS, the name must only contain alphanumeric characters and hyphens (`-`). - `-node-id` ((#\_node_id)) - Available in Consul 0.7.3 and later, this is a unique identifier for this node across all time, even if the name of the node