manager: improve handling of custom domains#45
manager: improve handling of custom domains#45etnguyen03 wants to merge 2 commits intotjcsl:masterfrom
Conversation
etnguyen03
commented
Dec 22, 2020
etnguyen03
commented
- Validate custom domains with the same regexes as the orchestrator
- Check if a domain is CNAMEd properly before it is added for non-superusers
- We probably want to update the documentation to clarify that if a CDN is being used, the CDN must be disabled for this record
- For instance, domains on Cloudflare need to have the CNAME record "grey clouded"
- We probably want to update the documentation to clarify that if a CDN is being used, the CDN must be disabled for this record
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
A few suggestions based on a quick review.
manager/director/apps/sites/forms.py
Outdated
| try: | ||
| # Attempt to resolve this domain (check for CNAME records) | ||
| result = dns.resolver.resolve( | ||
| cleaned_data["domain"], "CNAME", raise_on_no_answer=False |
There was a problem hiding this comment.
From glancing through dnspython's documentation, it looks like you don't need raise_on_no_answer=False because you handle dns.resolver.NoAnswer later. Am I missing something, or is this just left over from previous versions of the code?
There was a problem hiding this comment.
That was left over, I forgot about that. Removed.
manager/director/apps/sites/forms.py
Outdated
| self.add_error("domain", "Only administrators can add tjhsst.edu domains") | ||
|
|
||
| # Check if this domain CNAMEs to the proper value | ||
| if "domain" in cleaned_data and cleaned_data["domain"] and not self.user_is_superuser: |
There was a problem hiding this comment.
| if "domain" in cleaned_data and cleaned_data["domain"] and not self.user_is_superuser: | |
| if cleaned_data.get("domain") and not self.user_is_superuser: |
This is what I would probably do. I think it's a little more readable because it helps cut down the line length, but that's just my opinion. Feel free to ignore this.
manager/director/apps/sites/forms.py
Outdated
| # point to the CNAME configured in settings, raise a ValueError (which is caught) | ||
| if result.rrset is None or not any( | ||
| [ | ||
| settings.DIRECTOR_CUSTOM_DOMAIN_FQDN_CNAME in response.to_text() |
There was a problem hiding this comment.
This validation seems a little loose (for example, somebody could set director.csl.tjhsst.eduXXX and it would be accepted). I'm guessing you did this to handle the trailing dot in CNAME records; couldn't that be done with .rstrip(".")?
etnguyen03
force-pushed
the
manager-check-cnames
branch
2 times, most recently
from
f164c12 to
dcfa36b
Compare
manager/director/apps/sites/forms.py
Outdated
| validators=[ | ||
| validators.RegexValidator( | ||
| regex=r"^(?!(.*\.)?sites\.tjhsst\.edu$)[0-9a-zA-Z_\- .]+$", | ||
| regex=r"^.*sites\.tjhsst\.edu$", |
There was a problem hiding this comment.
The previous regex specifically did (.*\.)?sites\.tjhsst\.edu$ to allow things like test-sites.tjhsst.edu while blocking sites.tjhsst.edu and a.sites.tjhsst.edu. I'd recommend keeping that.
Nice work with inverse_match, though. I didn't know about that.
etnguyen03
force-pushed
the
manager-check-cnames
branch
from
dcfa36b to
7f15618
Compare
2 participants
