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.

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

Custom domain: check CNAME when adding domains #11747

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 5, 2024

If the user owns the domain, they can delete the CNAME before adding the domain to their project.

Hashed domains won't be needed anymore to prevent domain hijacking (but still be something we should do), since users trying to add a domain with an old CNAME will need to delete that CNAME first, and only domain owners can do that. It may be a little of a pain for some users that first add the CNAME before creating the domain on .org, but that's only on .org, since on .com the users doesn't know the CNAME before creating the domain.

Ref https://github.com/readthedocs/meta/discussions/157

dockerfiles/Dockerfile Outdated Show resolved Hide resolved
readthedocs/projects/forms.py Outdated Show resolved Hide resolved
@stsewd stsewd marked this pull request as ready for review November 6, 2024 14:46
@stsewd stsewd requested a review from a team as a code owner November 6, 2024 14:46
@stsewd stsewd requested a review from humitos November 6, 2024 14:46
humitos
humitos previously approved these changes Nov 7, 2024
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It called my attention that we are asking users to delete the CNAME first. I think we didn't talk about that in our meeting, did we?

However, it seems this approach solves our concern at the expenses of having to follow the documented steps in order, which is not a big deal, I guess.

Pinging @ericholscher in case he has more thoughts on this.

readthedocs/projects/forms.py Outdated Show resolved Hide resolved
@stsewd
Copy link
Member Author

stsewd commented Nov 7, 2024

It called my attention that we are asking users to delete the CNAME first. I think we didn't talk about that in our meeting, did we?

We talked about detecting chained CNAMEs, but www usually points to the root domain, there is no CNAME in there (A record). We could try detecting that case as well, but just checking if there is a CNAME seemed easier. There is still the case when a root domain is left pointing to us, but that case is less usual I'd say (since your domain will serve nothing until you re-point it to something valid).

@humitos humitos dismissed their stale review November 11, 2024 12:17

Dismissing because Eric pointed out really good arguments against this approach.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach works. I don't love that we're discarding the additional CNAME answers, but that seems fine given the overall simplicity of what we're doing here.

We should document these limitations in the custom domains page, and then I think we're OK to merge.

readthedocs/projects/forms.py Show resolved Hide resolved
Comment on lines +1040 to +1050
- Has a CNAME pointing to another CNAME.
This prevents the subdomain from being hijacked if the last subdomain is already on RTD,
but the user didn't register the other subdomain.
Example: doc.example.com -> docs.example.com -> readthedocs.io,
We don't want to allow doc.example.com to be added.
- Has a CNAME pointing to the APEX domain.
This prevents a subdomain from being hijacked if the APEX domain is already on RTD.
A common case is `www` pointing to the APEX domain, but users didn't register the
`www` subdomain, only the APEX domain.
Example: www.example.com -> example.com,
we don't want to allow www.example.com to be added.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can mostly just copy this to the limitations in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants