CNAME flattening #3403

gamalan · 2018-05-28T20:26:10Z

What does this PR do?

Allow handling host rule with different names than the HOST header that comes in.

Motivation

I am working in a project that allow user masking their domain/subdomain to our domain. Previously, only able to serve request that have same host rule and host header that came.

Fixes #141

More

Added dependency(https://github.com/patrickmn/go-cache)
Add new set of config
Added/updated unit test
Add new integration test
Add new documentation

Additional Notes

To allow generate/serving the request SSL, must use httpChallenge with active onDemand rule, dnsChallenge is only possible if the request host dns is also managed by ours own.
ACME onDemand config, should not be deprecated, if it being deprecated and removed, then this feature will lost the ability to generate the requested SSL cert. Resulting to a breaking response because SSL protocol error, Invalid CA/CN, etc.
Current additional config example

[hostResolver] #currently using resolver label
cnameFlattening = true #default is false, if this not set true, following config is useless
resolvConfig = "/etc/resolv.conf" # default is /etc/resolv.conf, if changed, the following file must follow resolv.conf standard
resolvDepth = 5 # maximum CNAME lookup depth before finishing

- Resolve CNAME recursively - Get the first(request) and last(potential) - Compare with the available host rule

- add cache mechanism for cname flattening

nmengin · 2018-05-29T14:53:16Z

Hello @gamalan .

Many thanks for this PR. 👍

We talked about it with the team and, even if we understand the need, we are not sure that it's a best practice to check the CNAME flattening by default.
Indeed, only Cloudflare implements this (useful) feature and, I guess, only a part of Clouflare users use it.

That's why, I guess it should be great if the CNAME flattening was enable thanks to a flag. Thus only users who really need it will activate it.

WDYT?

gregorybrzeski · 2018-05-29T15:03:11Z

AFAIK Openstack Swift is using CNAME flattening, as a precaution they have a limit (lookup_depth) of dns resolves which are allowed for each request.

https://docs.openstack.org/newton/config-reference/object-storage/features.html

"CNAME lookup"

https://github.com/openstack/swift/blob/master/swift/common/middleware/cname_lookup.py

gamalan · 2018-05-29T19:34:39Z

@nmengin agreed, not all people need this. My take is set up an additional set of global config, inside entrypoint, called entryPoints.resolver or a new set of config, simply called resolver. With a set of value for :

CNAMEFlattening = true/false if false, any other option is useless, the default is false
resolverDepth = 0-XX, how deep it should lookup a DNS record, 0 means unlimited. default is 0
cache = 0-XX, cache time, in a minutes format, the default is 30 minutes.

An example :

[entryPoints.resolver]
CNAMEFlattening = true
resolverDepth = 10
cache = 30

NB : to able generating the SSL for the incoming request, this feature need onDemand rule to be available (currently is being deprecated)

Happy to accept some opinion.

gregorybrzeski · 2018-05-29T22:53:21Z

@gamalan if you set resolverDepth by default to 0 (unlimited), then what would happen if the CNAME resolve chain has a circular reference ? Maybe setting it by default to 3 or 5 would be a better option ?

Would you also consider a case where dns servers for CNAMEFlattening resolving can be specified in config file ? The /etc/resolv.conf can be pointing to internal dns servers which might not be (in some cases) best choice to use as a resolving dns servers for CNAMEFlattening process.

gamalan · 2018-05-30T02:04:59Z

@gregorybrzeski ah, forgot about that case, you are right, I think 5 should suffice for default. Also for the resolver config, good idea, but it should follow the resolv.conf standard(not sure gonna work in windows though).

gamalan · 2018-05-30T08:52:02Z

Updated, already make a new set of config, haven't been pushed yet. Currently, the CNAME flattening process is separated as a new package, the unit test has been added, the integration test and documentation is WIP. Not sure where to put for the documentation.

The additional config is set to global, as example

[resolver] #currently using resolver label
cnameFlattening = true #default is false, if this not set true, following config is useless
resolvConfig = "/etc/resolv.conf" # default is /etc/resolv.conf, if changed, the following file must follow resolv.conf standard
resolvDepth = 5 # maximum CNAME lookup depth before finishing
cacheDuration = "30m" # default cache expiration 30 minute

# Conflicts: # server/server.go

nmengin · 2018-05-30T15:28:07Z

Hello @gamalan ,

Many thanks for the modifications you made.

We think the resolver block is a good idea but we prefer keep the resolvdepth and the cacheduration hardcoded.

The flag should look like this :

[resolver] #currently using resolver label
cnameFlattening = true #default is false, if this not set true, following config is useless
resolvConfig = "/etc/resolv.conf" # default is /etc/resolv.conf, if changed, the following file must follow resolv.conf standard

Moreover, we would like to give the possibility to the users to enable resolver and customizeresolver.resolvconfig even if the resolver.cnameflattening is set to false.
It can be useful for others Use Cases.

WDYT?

nmengin

Hello @gamalan .

Few new remarks 😉

nmengin · 2018-06-28T15:40:51Z

docs/configuration/commons.md

+
+```
+
+- To allow serving secure https request and generate the SSL using ACME while `cnameFlattening` is set to `true`. The `acme` configuration 


Please only write one sentence by line.

nmengin · 2018-06-28T15:40:57Z

docs/configuration/commons.md

+```
+
+- To allow serving secure https request and generate the SSL using ACME while `cnameFlattening` is set to `true`. The `acme` configuration 
+for `HTTP-01` challenge and `onDemand` is mandatory. Refer to [ACME configuration](/configuration/acme) for more information.


Please only write one sentence by line.

nmengin · 2018-06-28T15:41:40Z

hostresolver/hostresolver.go

+	Record string
+}
+
+// CNAMEFlatten check if CNAME records is exists, flatten if possible


CNAMEFlatten check if CNAME records is exists,

Please modify in CNAMEFlatten check if CNAME records exists.

nmengin · 2018-06-28T15:45:36Z

hostresolver/hostresolver.go

+	return result[0], result[len(result)-1]
+}
+
+// CNAMEResolve resolve CNAME if exists, and return with the highest TTL


Can you correct the comment please: CNAMEResolve resolves CNAME if exists, and returns with the highest TTL ?

nmengin · 2018-06-28T15:45:51Z

hostresolver/hostresolver.go

+
+// CNAMEResolve resolve CNAME if exists, and return with the highest TTL
+func (hr *HostResolver) CNAMEResolve(host string) *CNAMEResolv {
+	config, _ := dns.ClientConfigFromFile(hr.ResolvConfig)


Can you manage the error please?

nmengin · 2018-06-28T15:47:19Z

hostresolver/hostresolver.go

+		r, _, err := c.Exchange(m, net.JoinHostPort(config.Servers[i], config.Port))
+		if err != nil {
+			log.Errorf("Failed to resolve host %s with server %s", host, config.Servers[i])
+		}


Do you want to contin ue to precces the statement in the loop if there is an error?
WDYT to add a instructioncontinue if err != nil?

nmengin · 2018-06-28T15:49:53Z

hostresolver/hostresolver.go

+}
+
+// CNAMEResolve resolve CNAME if exists, and return with the highest TTL
+func (hr *HostResolver) CNAMEResolve(host string) *CNAMEResolv {


The method seems to be used only on this file.
WDYT to make it private?

refactor function visibility, add error logging fix loop control process

nmengin

Hello @gamalan .

New comments 😉

nmengin · 2018-07-02T06:40:58Z

hostresolver/hostresolver.go

+	ResolvDepth  int
+}
+
+// CNAMEResolv used for storing CNAME result


Please replace for storing by to store

nmengin · 2018-07-02T06:41:52Z

hostresolver/hostresolver.go

+	Record string
+}
+
+// CNAMEFlatten check if CNAME records exists, flatten if possible


Do you mean records exist or record exists?

nmengin · 2018-07-02T06:50:12Z

integration/hostresolver_test.go

+	c.Assert(err, checker.IsNil)
+	req.Host = "frontend1.docker.local"
+
+	err = try.Request(req, 500*time.Millisecond, try.StatusCodeIs(http.StatusOK), try.HasBody())


Not sure, but this test only checks if the usual behavior is the same if the cnameflattening is enabled but it does not test if this one really works, it isn't?

Is it possible to add a test to check if CNAME flattening works or is it hard to check?

nmengin · 2018-07-02T06:55:06Z

rules/rules.go

+				if types.CanonicalDomain(reqH) == types.CanonicalDomain(host) || types.CanonicalDomain(flatH) == types.CanonicalDomain(host) {
+					return true
+				}
+			}


WDYT to add a DEBUG log to indicate to user that the CNAMEFlattening did not allow resolving his domain?

# Conflicts: # Gopkg.toml # server/server.go

resolve conflict with master branch

ldez

LGTM

mmatur

LGTM

nmengin

LGTM

gamalan added 2 commits May 27, 2018 20:14

- Check if request is resolving to CNAME

a26686f

- Resolve CNAME recursively - Get the first(request) and last(potential) - Compare with the available host rule

- add new dependency

a1314de

- add cache mechanism for cname flattening

traefiker added size/S status/0-needs-triage labels May 28, 2018

reformat rules.go

97b75b6

ldez changed the title ~~CNAME flattening #141~~ CNAME flattening May 28, 2018

ldez added kind/enhancement a new or improved feature. status/2-needs-review contributor/waiting-for-corrections and removed status/0-needs-triage labels May 28, 2018

fix failed test

9224ec2

ldez removed the contributor/waiting-for-corrections label May 29, 2018

add simple test,CNAMEFlatten

1b914f0

ldez added this to the 1.7 milestone May 30, 2018

gamalan added 2 commits May 30, 2018 16:28

add new config, separate the package

2ea89ce

Merge remote-tracking branch 'origin/master' into cname-flattening

b0db833

# Conflicts: # server/server.go

ldez removed this from the 1.7 milestone May 30, 2018

nmengin added status/1-needs-design-review and removed status/2-needs-review labels May 30, 2018

gamalan added 2 commits May 30, 2018 16:50

reformat with gofmt

81c7aa9

fix linting

3bfd0e6

nmengin added the contributor/waiting-for-feedback label May 30, 2018

add integration test, change config to hostResolver block

bace07f

nmengin requested changes Jun 28, 2018

View reviewed changes

update from feedback,

89ff204

refactor function visibility, add error logging fix loop control process

ldez removed the contributor/waiting-for-corrections label Jun 29, 2018

nmengin requested changes Jul 2, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 2, 2018

gamalan added 5 commits July 2, 2018 16:27

Merge remote-tracking branch 'origin/master' into cname-flattening

25e283f

# Conflicts: # Gopkg.toml # server/server.go

update from feedback,

60ffc16

resolve conflict with master branch

Merge remote-tracking branch 'origin/master'

927095e

Merge branch 'master' into cname-flattening

2a66649

fix formatting

6fafc3c

ldez removed the contributor/waiting-for-corrections label Jul 3, 2018

ldez self-requested a review July 3, 2018 09:30

review: simplify.

f143cfb

ldez approved these changes Jul 3, 2018

View reviewed changes

mmatur approved these changes Jul 3, 2018

View reviewed changes

nmengin approved these changes Jul 3, 2018

View reviewed changes

nmengin added status/3-needs-merge and removed status/2-needs-review labels Jul 3, 2018

traefiker added the status/4-merge-in-progress label Jul 3, 2018

Merge remote-tracking branch 'upstream/master' into cname-flattening

6d0922a

traefiker removed the status/4-merge-in-progress label Jul 3, 2018

traefiker merged commit 31a8e3e into traefik:master Jul 3, 2018

traefiker removed the status/3-needs-merge label Jul 3, 2018

gamalan mentioned this pull request Jul 7, 2018

API for generating SSL cert. #3570

Closed

gamalan deleted the cname-flattening branch July 7, 2018 11:03

ldez added the area/rules label Jul 7, 2018

gamalan mentioned this pull request Aug 31, 2018

Add provider specific cnameFlattening Triger #3843

Closed

2 tasks

gamalan mentioned this pull request Oct 12, 2018

Add provider specific cnameFlattening Trigger #4032

Open

gamalan mentioned this pull request Oct 30, 2018

Add provider specific cnameFlattening Trigger #4134

Closed

2 tasks

elreydetoda mentioned this pull request Aug 31, 2022

subdomain www.jupiterbroadcasting.com certificate error JupiterBroadcasting/jupiterbroadcasting.com#358

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNAME flattening #3403

CNAME flattening #3403

gamalan commented May 28, 2018 •

edited

Loading

nmengin commented May 29, 2018

gregorybrzeski commented May 29, 2018

gamalan commented May 29, 2018 •

edited

Loading

gregorybrzeski commented May 29, 2018

gamalan commented May 30, 2018

gamalan commented May 30, 2018 •

edited

Loading

nmengin commented May 30, 2018

nmengin left a comment

nmengin Jun 28, 2018

nmengin Jun 28, 2018

nmengin Jun 28, 2018

nmengin Jun 28, 2018

nmengin Jun 28, 2018

nmengin Jun 28, 2018

nmengin Jun 28, 2018

nmengin left a comment

nmengin Jul 2, 2018

nmengin Jul 2, 2018

nmengin Jul 2, 2018

nmengin Jul 2, 2018

ldez left a comment

mmatur left a comment

nmengin left a comment


		```

		- To allow serving secure https request and generate the SSL using ACME while `cnameFlattening` is set to `true`. The `acme` configuration

CNAME flattening #3403

CNAME flattening #3403

Conversation

gamalan commented May 28, 2018 • edited Loading

What does this PR do?

Motivation

More

Additional Notes

nmengin commented May 29, 2018

gregorybrzeski commented May 29, 2018

gamalan commented May 29, 2018 • edited Loading

gregorybrzeski commented May 29, 2018

gamalan commented May 30, 2018

gamalan commented May 30, 2018 • edited Loading

nmengin commented May 30, 2018

nmengin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

gamalan commented May 28, 2018 •

edited

Loading

gamalan commented May 29, 2018 •

edited

Loading

gamalan commented May 30, 2018 •

edited

Loading