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

Consul's DNS parser allows junk between the DC and the consul subdomain #3200

Closed
slackpad opened this issue Jun 27, 2017 · 5 comments
Closed

Consul's DNS parser allows junk between the DC and the consul subdomain #3200

slackpad opened this issue Jun 27, 2017 · 5 comments
Assignees
@magiconair
Labels
theme/api Relating to the HTTP API interface type/bug Feature does not function as expected
Milestone
1.0.1

Comments

@slackpad
Copy link
Contributor

This captures an issue that came in from Google groups. It looks like Consul quietly drops anything after the DC and before the Consul subdomain:

$: dig @127.0.0.1 -p 8600 consul-server0.node.eu-west-2.consul
;; ANSWER SECTION:
consul-server0.node.eu-west-2.consul. 5 IN A    172.26.2.14

$: dig @127.0.0.1 -p 8600 consul-server0.node.eu-west-2.randomstring.consul
;; ANSWER SECTION:
consul-server0.node.eu-west-2.randomstring.consul. 5 IN A 172.26.2.14

$: dig @127.0.0.1 -p 8600 consul-server0.node.eu-west-2.moregarbage.randomstring.consul
;; ANSWER SECTION:
consul-server0.node.eu-west-2.moregarbage.randomstring.consul. 5 IN A 172.26.2.14

Consul should parse this strictly and return an NXDOMAIN error for the cases with junk. I couldn't reproduce this anywhere other than between the DC and the Consul subdomain.
@slackpad slackpad added type/bug Feature does not function as expected theme/api Relating to the HTTP API interface labels Jun 27, 2017
@corford
Copy link

corford commented Jun 27, 2017 •
edited
Loading

Hi James

For the fix, it's important that it still allows subdomains if they're set intentionally via the -domain cli flag. E.g. in our case, we use <nodename>.node[.dc].prod.consul for our production cluster and <nodename>.node[.dc].test.consul for our test cluster.

@corford
Copy link

corford commented Oct 17, 2017

Just circling back to see if there has been any progress on this? Worried that the longer it's left, the more pain it risks causing people when the bug's squashed (e.g. misconfigured service discovery that currently works thanks to the bug).

@magiconair
Copy link
Contributor

I can have a look at this.

@magiconair magiconair self-assigned this Oct 17, 2017
magiconair added a commit that referenced this issue Oct 20, 2017
@magiconair
dns: return NXDOMAIN if datacenter is invalid (#3200)
ad63fd8 
Queries to the DNS server can contain an optional datacenter
name in the query name. You can query for 'foo.service.consul'
or 'foo.service.dc.consul' to get a response for either the
default or a specific datacenter.

Datacenter names cannot have dots, therefore the datacenter
name can refer to only one element in the DNS query name.

The DNS server allowed extra labels between the optional
datacenter name and the domain and returned a valid response
instead of returning NXDOMAIN. For example, if the domain
is set to '.consul' then 'foo.service.dc1.extra.consul'
should return NXDOMAIN because of 'extra' being between
the datacenter name 'dc1' and the domain '.consul'.

Fixes #3200
@magiconair
Copy link
Contributor

The dispatch method first strips off the domain before continuing with the parsing, AFAICT.

https://github.com/hashicorp/consul/blob/v1.0.0/agent/dns.go#L360-L488

However, it seems the implementation is a bit too flexible and considers the last element before the node, service, ... element the datacenter and ignores the rest.

@magiconair magiconair mentioned this issue Oct 20, 2017
Merged
@magiconair magiconair added this to the 1.0.1 milestone Oct 20, 2017
@corford
Copy link

corford commented Oct 20, 2017

Thanks very much @magiconair Fix seems good and looking forward to this landing in 1.0.1

slackpad pushed a commit that referenced this issue Oct 20, 2017
@magiconair @slackpad
dns: return NXDOMAIN if datacenter is invalid (#3200) (#3596)
3509321 
Queries to the DNS server can contain an optional datacenter
name in the query name. You can query for 'foo.service.consul'
or 'foo.service.dc.consul' to get a response for either the
default or a specific datacenter.

Datacenter names cannot have dots, therefore the datacenter
name can refer to only one element in the DNS query name.

The DNS server allowed extra labels between the optional
datacenter name and the domain and returned a valid response
instead of returning NXDOMAIN. For example, if the domain
is set to '.consul' then 'foo.service.dc1.extra.consul'
should return NXDOMAIN because of 'extra' being between
the datacenter name 'dc1' and the domain '.consul'.

Fixes #3200
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@magiconair magiconair

Labels
theme/api Relating to the HTTP API interface type/bug Feature does not function as expected
Projects
None yet
Milestone
1.0.1
Development

No branches or pull requests
3 participants
@magiconair @slackpad @corford