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

Check via zlint if a domain's TLD is valid #7943

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

Conversation

mcpherrinm
Copy link
Contributor

If there's discrepencies between the PSL and Zlint TLD list, this will take the
most conservative option of rejecting if either list doesn't have a TLD.

If there's discrepencies between the PSL and Zlint TLD list, this will take the
most conservative option of rejecting if either list doesn't have a TLD.
@mcpherrinm mcpherrinm requested a review from a team as a code owner January 13, 2025 19:23
@mcpherrinm mcpherrinm requested a review from jprenken January 13, 2025 19:23
jprenken
jprenken previously approved these changes Jan 13, 2025
@mcpherrinm
Copy link
Contributor Author

Tests fail because a domain was blocked for the wrong reason, I'll look at an update there in a bit.

@mcpherrinm mcpherrinm marked this pull request as draft January 13, 2025 19:37
This domain is now rejected earlier by not treating .arpa as a valid TLD, so
use a different one to get the expected error.
If there's any disagreement over what "The TLD" is, IsInTLDMap is not the
correct thing to use.

zlint has add/remove dates for TLD, which we should use time.Now() for issuance.
I don't think we need to plumb in a fake clock here.
@mcpherrinm
Copy link
Contributor Author

mcpherrinm commented Jan 14, 2025

I was incorrect about the reason for the failure. The "TLD" returned from PSL is a string like "co.uk" or "in-addr.arpa", but IsInTLDMap only takes the final component "uk". So I have switched to use HasValidTLD on the full name instead.

zlint has add/remove dates for TLD, which we should use time.Now() for issuance. cert-checker could inadvertently alert here if a TLD is removed between issuance and cert-checker running. I'm not sure if that's worth fixing, or if it is, I don't think I want to take on plumbing fake clocks around.

@mcpherrinm mcpherrinm marked this pull request as ready for review January 14, 2025 01:59
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

The use of bare time.Now() does make me slightly concerned. I'm going to take some more time to think about this and whether there are viable alternatives.

@mcpherrinm
Copy link
Contributor Author

I think the best path forward is passing the certificate's issuance time (or time.NotBefore) in, but that's a deeper slice across the codebase.

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