From 086cd68dacbbc018d8f1440e51cc5dd5d4288343 Mon Sep 17 00:00:00 2001 From: Matthew McPherrin Date: Mon, 13 Jan 2025 14:22:33 -0500 Subject: [PATCH 1/6] Check via zlint if a domain's TLD is valid 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. --- iana/iana.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/iana/iana.go b/iana/iana.go index 8e138e1db09..86e1e69ba8f 100644 --- a/iana/iana.go +++ b/iana/iana.go @@ -4,12 +4,14 @@ import ( "fmt" "github.com/weppos/publicsuffix-go/publicsuffix" + zlintutil "github.com/zmap/zlint/v3/util" ) // ExtractSuffix returns the public suffix of the domain using only the "ICANN" // section of the Public Suffix List database. // If the domain does not end in a suffix that belongs to an IANA-assigned // domain, ExtractSuffix returns an error. +// It confirms with zlint's TLD list. func ExtractSuffix(name string) (string, error) { if name == "" { return "", fmt.Errorf("Blank name argument passed to ExtractSuffix") @@ -22,6 +24,10 @@ func ExtractSuffix(name string) (string, error) { suffix := rule.Decompose(name)[1] + if !zlintutil.IsInTLDMap(suffix) { + return "", fmt.Errorf("Domain %s has an invalid TLD %s", name, suffix) + } + // If the TLD is empty, it means name is actually a suffix. // In fact, decompose returns an array of empty strings in this case. if suffix == "" { From 9ac5f5ca4cdd6ecfe90d4a55e6bf2cee6fd949fd Mon Sep 17 00:00:00 2001 From: Matthew McPherrin Date: Mon, 13 Jan 2025 14:24:38 -0500 Subject: [PATCH 2/6] Unknown instead of invalid --- iana/iana.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iana/iana.go b/iana/iana.go index 86e1e69ba8f..302d73af840 100644 --- a/iana/iana.go +++ b/iana/iana.go @@ -25,7 +25,7 @@ func ExtractSuffix(name string) (string, error) { suffix := rule.Decompose(name)[1] if !zlintutil.IsInTLDMap(suffix) { - return "", fmt.Errorf("Domain %s has an invalid TLD %s", name, suffix) + return "", fmt.Errorf("Domain %s has an unknown TLD %s", name, suffix) } // If the TLD is empty, it means name is actually a suffix. From 2c3ca813a961ed5f0ceec0cef63001fe0625c19e Mon Sep 17 00:00:00 2001 From: Matthew McPherrin Date: Mon, 13 Jan 2025 14:25:46 -0500 Subject: [PATCH 3/6] move check after the empty-suffix handling --- iana/iana.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/iana/iana.go b/iana/iana.go index 302d73af840..10a5f29ebe2 100644 --- a/iana/iana.go +++ b/iana/iana.go @@ -24,15 +24,15 @@ func ExtractSuffix(name string) (string, error) { suffix := rule.Decompose(name)[1] - if !zlintutil.IsInTLDMap(suffix) { - return "", fmt.Errorf("Domain %s has an unknown TLD %s", name, suffix) - } - // If the TLD is empty, it means name is actually a suffix. // In fact, decompose returns an array of empty strings in this case. if suffix == "" { suffix = name } + if !zlintutil.IsInTLDMap(suffix) { + return "", fmt.Errorf("Domain %s has an unknown TLD %s", name, suffix) + } + return suffix, nil } From f5f7613c3b30353d6688baab3aa55f2f882cbeec Mon Sep 17 00:00:00 2001 From: Matthew McPherrin Date: Mon, 13 Jan 2025 18:25:33 -0500 Subject: [PATCH 4/6] Change domain uses in High-risk domain test This domain is now rejected earlier by not treating .arpa as a valid TLD, so use a different one to get the expected error. --- test/v2_integration.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/v2_integration.py b/test/v2_integration.py index 00107386aea..e5d987df947 100644 --- a/test/v2_integration.py +++ b/test/v2_integration.py @@ -1166,9 +1166,9 @@ def test_new_order_policy_errs(): """ client = chisel2.make_client(None) - # 'in-addr.arpa' is present in `test/hostname-policy.yaml`'s + # 'example.org' is present in `test/hostname-policy.yaml`'s # HighRiskBlockedNames list. - csr_pem = chisel2.make_csr(["out-addr.in-addr.arpa", "between-addr.in-addr.arpa"]) + csr_pem = chisel2.make_csr(["salamander.example.org", "axolotl.example.org"]) # With two policy blocked names in the order we expect to get back a top # level rejectedIdentifier with a detail message that references @@ -1184,7 +1184,7 @@ def test_new_order_policy_errs(): ok = True if e.typ != "urn:ietf:params:acme:error:rejectedIdentifier": raise(Exception("Expected rejectedIdentifier type problem, got {0}".format(e.typ))) - if e.detail != 'Error creating new order :: Cannot issue for "between-addr.in-addr.arpa": The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy (and 1 more problems. Refer to sub-problems for more information.)': + if e.detail != 'Error creating new order :: Cannot issue for "axolotl.example.org": The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy (and 1 more problems. Refer to sub-problems for more information.)': raise(Exception("Order problem detail did not match expected")) if not ok: raise(Exception("Expected problem, got no error")) From 5fdd429f226a8ed97094512d1843e2ce257ff20e Mon Sep 17 00:00:00 2001 From: Matthew McPherrin Date: Mon, 13 Jan 2025 20:36:25 -0500 Subject: [PATCH 5/6] Use HasValidTLD on the full name instead 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. --- iana/iana.go | 9 +++++---- iana/iana_test.go | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/iana/iana.go b/iana/iana.go index 10a5f29ebe2..19fa5526c23 100644 --- a/iana/iana.go +++ b/iana/iana.go @@ -2,6 +2,7 @@ package iana import ( "fmt" + "time" "github.com/weppos/publicsuffix-go/publicsuffix" zlintutil "github.com/zmap/zlint/v3/util" @@ -17,6 +18,10 @@ func ExtractSuffix(name string) (string, error) { return "", fmt.Errorf("Blank name argument passed to ExtractSuffix") } + if !zlintutil.HasValidTLD(name, time.Now()) { + return "", fmt.Errorf("%s has an unknown TLD", name) + } + rule := publicsuffix.DefaultList.Find(name, &publicsuffix.FindOptions{IgnorePrivate: true, DefaultRule: nil}) if rule == nil { return "", fmt.Errorf("Domain %s has no IANA TLD", name) @@ -30,9 +35,5 @@ func ExtractSuffix(name string) (string, error) { suffix = name } - if !zlintutil.IsInTLDMap(suffix) { - return "", fmt.Errorf("Domain %s has an unknown TLD %s", name, suffix) - } - return suffix, nil } diff --git a/iana/iana_test.go b/iana/iana_test.go index 214952abc5b..7715babfd6a 100644 --- a/iana/iana_test.go +++ b/iana/iana_test.go @@ -40,7 +40,7 @@ func TestExtractSuffix_Valid(t *testing.T) { for _, tc := range testCases { got, err := ExtractSuffix(tc.domain) if err != nil { - t.Errorf("%q: returned error", tc.domain) + t.Errorf("%q: returned error: %v", tc.domain, err) continue } if got != tc.want { From b949278d929de0315bf3ec0a3b9bcc6729da344d Mon Sep 17 00:00:00 2001 From: Matthew McPherrin Date: Mon, 13 Jan 2025 20:47:51 -0500 Subject: [PATCH 6/6] Revert "Change domain uses in High-risk domain test" This reverts commit f5f7613c3b30353d6688baab3aa55f2f882cbeec. --- test/v2_integration.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/v2_integration.py b/test/v2_integration.py index e5d987df947..00107386aea 100644 --- a/test/v2_integration.py +++ b/test/v2_integration.py @@ -1166,9 +1166,9 @@ def test_new_order_policy_errs(): """ client = chisel2.make_client(None) - # 'example.org' is present in `test/hostname-policy.yaml`'s + # 'in-addr.arpa' is present in `test/hostname-policy.yaml`'s # HighRiskBlockedNames list. - csr_pem = chisel2.make_csr(["salamander.example.org", "axolotl.example.org"]) + csr_pem = chisel2.make_csr(["out-addr.in-addr.arpa", "between-addr.in-addr.arpa"]) # With two policy blocked names in the order we expect to get back a top # level rejectedIdentifier with a detail message that references @@ -1184,7 +1184,7 @@ def test_new_order_policy_errs(): ok = True if e.typ != "urn:ietf:params:acme:error:rejectedIdentifier": raise(Exception("Expected rejectedIdentifier type problem, got {0}".format(e.typ))) - if e.detail != 'Error creating new order :: Cannot issue for "axolotl.example.org": The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy (and 1 more problems. Refer to sub-problems for more information.)': + if e.detail != 'Error creating new order :: Cannot issue for "between-addr.in-addr.arpa": The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy (and 1 more problems. Refer to sub-problems for more information.)': raise(Exception("Order problem detail did not match expected")) if not ok: raise(Exception("Expected problem, got no error"))