Domain resolution too clever

[agjohnson]

One of the tactics that we use for domain magic can result in some unexpected problems. If the project does not have a domain associated with it yet, our middleware will attempt a DNS lookup. This block will attempt to get a slug from a CNAME:
https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/core/middleware.py#L67-L76

We aren't doing anything to check that the domain is our though. In this case, the domain virtualenv.pypa.io resolves as such:

% dig +short virtualenv.pypa.io
c.global-ssl.fastly.net.
199.27.79.175

The above block will split the CNAME returned, assume the slug is the c, and apply the domain to this project if it exists.

Instead, we should probably ensure the CNAME domain is something we control, and this is indeed a slug, not a hostname.

[berkerpeksag]

This is also happening in API v2: http://readthedocs.org/api/v2/cname/?host=packaging.python.org

{"host":"packaging.python.org","slug":"python"}

dig output for packaging.python.org:

$ dig +short packaging.python.org
python.map.fastly.net.
prod.python.map.fastlylb.net.
151.101.60.223

I've opened #2317 to use the same helper both in middleware and API.

[stsewd]

So, I was reading this, I don't fully understand the issue. In what case this bug is triggered? Why some of this domains aren't resolving to *.readthedocs.io (but they are hosted by rtd).

As a solution, we should check that the domain matches PUBLIC_DOMAIN, is that correct? But I still have the same doubts as above p:

Also, do we really need the /cname endpoint?

[agjohnson]

So, to trigger this, we have middleware that tries to guess the project based on DNS. I'm not convinced this should be a feature of RTD. The lookup logic is supposed to be:

• See incoming request for domain that isn't our PUBLIC_DOMAIN and isn't already a Domain
• Do a DNS lookup on the domain
• Assume the domain is pointed to <project_slug>.readthedocs.io and parse it accordingly
• Bring up the project for <project_slug>

When it fails is when the domain we're seeing a request for either isn't actually pointing to us, or when there's some other record, like the DNS request above:

% dig packaging.python.org +short
dualstack.python.map.fastly.net.
151.101.52.223

Our code assumes the project slug is dualstack, ie:
http://readthedocs.org/api/v2/cname/?host=packaging.python.org

So!

• We need to determine what might be removed if we just gutted this feature
• We need to know if there domains that would be affected by removing this feature? We might be logging this currently. Historically I think we made Domain objects that were machine=True when we relied on this feature
• I'm not sure what else is using the /cname endpoint, that also needs some discovery

Based on what you find, we are going to be weighing:

• Removing the feature altogether and forcing users to establish domains with us (with cloudflare, I believe this might be an absolute requirement) -- @davidfischer can confirm
• Or do we repair the logic and fix parsing. We are currently asking users to point to readthedocs.io I believe in our docs, so this middleware will just be working less and less.

Looks like things could be stacking up to just remove the feature 🔥

[stsewd]

So, this happens if users cname us without creating a domain in the admin panel, right?

[davidfischer]

Historically I think we made Domain objects that were machine=True when we relied on this feature

There are ~1k of these.

Removing the feature altogether and forcing users to establish domains with us (with cloudflare, I believe this might be an absolute requirement) -- @davidfischer can confirm

It is required to generate a certificate but *.readthedocs.io goes right to our load balancer and not to Cloudflare. Edit: at least until #4521.

With that said, I'm +1 on removing this feature. If people want to use RTD with a custom domain, I don't think requiring them to put that domain in the dashboard is burdensome.

[agjohnson]

So, this happens if users cname us without creating a domain in the admin panel, right?

Correct

I don't think requiring them to put that domain in the dashboard is burdensome

I'd say it is probably expected even

[ericholscher]

+1 to removing our old magic logic. We just need to make sure it won't break a bunch of users when we do it. I believe we backported domains that were doing this into Domain objects when we first released the explicit Domain's, but we might need to do something similar when we remove the auto-slugify based on DNS, to make sure old domains don't break.