Skip to content

Commit

Permalink
auto-config: relax node name validation for JWT authorization
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
kyhavlov committed Nov 14, 2022
1 parent 225ae55 commit 49080ea
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 1 deletion.
4 changes: 4 additions & 0 deletions agent/config/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 "+
Expand Down
3 changes: 2 additions & 1 deletion agent/consul/auto_config_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand Down
2 changes: 2 additions & 0 deletions website/content/docs/agent/config/cli-flags.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,8 @@ 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.
This cannot contain whitespace or quotes, and in order for the node to be
discoverable via DNS it must only contain alphanumerics and dashes (`-`).

- `-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
Expand Down

0 comments on commit 49080ea

Please sign in to comment.