Skip to content

Commit fde145a

Browse files
author
Daniel McCarney
authored
RA: implement stricter email validation. (letsencrypt#4574)
Prev. we weren't checking the domain portion of an email contact address very strictly in the RA. This updates the PA to export a function that can be used to validate the domain the same way we validate domain portions of DNS type identifiers for issuance. This also changes the RA to use the `invalidEmail` error type in more places. A new Go integration test is added that checks these errors end-to-end for both account creation and account update.
1 parent a86ed0f commit fde145a

File tree

12 files changed

+209
-44
lines changed

12 files changed

+209
-44
lines changed

core/interfaces.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ type PolicyAuthority interface {
108108
WillingToIssueWildcards(identifiers []identifier.ACMEIdentifier) error
109109
ChallengesFor(domain identifier.ACMEIdentifier) ([]Challenge, error)
110110
ChallengeTypeEnabled(t string) bool
111+
ValidDomain(domain string) error
111112
}
112113

113114
// StorageGetter are the Boulder SA's read-only methods

csr/csr_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,10 @@ func (pa *mockPA) ChallengeTypeEnabled(t string) bool {
4646
return true
4747
}
4848

49+
func (pa *mockPA) ValidDomain(_ string) error {
50+
return nil
51+
}
52+
4953
func TestVerifyCSR(t *testing.T) {
5054
private, err := rsa.GenerateKey(rand.Reader, 2048)
5155
test.AssertNotError(t, err, "error generating test key")

policy/pa.go

Lines changed: 46 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -199,32 +199,21 @@ var (
199199
errWildcardNotSupported = berrors.MalformedError("Wildcard domain names are not supported")
200200
)
201201

202-
// WillingToIssue determines whether the CA is willing to issue for the provided
203-
// identifier. It expects domains in id to be lowercase to prevent mismatched
204-
// cases breaking queries.
202+
// ValidDomain checks that a domain isn't:
205203
//
206-
// We place several criteria on identifiers we are willing to issue for:
207-
//
208-
// * MUST self-identify as DNS identifiers
209-
// * MUST contain only bytes in the DNS hostname character set
210-
// * MUST NOT have more than maxLabels labels
211-
// * MUST follow the DNS hostname syntax rules in RFC 1035 and RFC 2181
212-
// In particular:
213-
// * MUST NOT contain underscores
214-
// * MUST NOT match the syntax of an IP address
215-
// * MUST end in a public suffix
216-
// * MUST have at least one label in addition to the public suffix
217-
// * MUST NOT be a label-wise suffix match for a name on the block list,
218-
// where comparison is case-independent (normalized to lower case)
204+
// * empty
205+
// * prefixed with the wildcard label `*.`
206+
// * made of invalid DNS characters
207+
// * longer than the maxDNSIdentifierLength
208+
// * an IPv4 or IPv6 address
209+
// * suffixed with just "."
210+
// * made of too many DNS labels
211+
// * made of any invalid DNS labels
212+
// * suffixed with something other than an IANA registered TLD
213+
// * exactly equal to an IANA registered TLD
219214
//
220-
// If WillingToIssue returns an error, it will be of type MalformedRequestError
221-
// or RejectedIdentifierError
222-
func (pa *AuthorityImpl) WillingToIssue(id identifier.ACMEIdentifier) error {
223-
if id.Type != identifier.DNS {
224-
return errInvalidIdentifier
225-
}
226-
domain := id.Value
227-
215+
// It does _not_ check that the domain isn't on any PA blocked lists.
216+
func (pa *AuthorityImpl) ValidDomain(domain string) error {
228217
if domain == "" {
229218
return errEmptyName
230219
}
@@ -300,6 +289,39 @@ func (pa *AuthorityImpl) WillingToIssue(id identifier.ACMEIdentifier) error {
300289
return errICANNTLD
301290
}
302291

292+
return nil
293+
}
294+
295+
// WillingToIssue determines whether the CA is willing to issue for the provided
296+
// identifier. It expects domains in id to be lowercase to prevent mismatched
297+
// cases breaking queries.
298+
//
299+
// We place several criteria on identifiers we are willing to issue for:
300+
//
301+
// * MUST self-identify as DNS identifiers
302+
// * MUST contain only bytes in the DNS hostname character set
303+
// * MUST NOT have more than maxLabels labels
304+
// * MUST follow the DNS hostname syntax rules in RFC 1035 and RFC 2181
305+
// In particular:
306+
// * MUST NOT contain underscores
307+
// * MUST NOT match the syntax of an IP address
308+
// * MUST end in a public suffix
309+
// * MUST have at least one label in addition to the public suffix
310+
// * MUST NOT be a label-wise suffix match for a name on the block list,
311+
// where comparison is case-independent (normalized to lower case)
312+
//
313+
// If WillingToIssue returns an error, it will be of type MalformedRequestError
314+
// or RejectedIdentifierError
315+
func (pa *AuthorityImpl) WillingToIssue(id identifier.ACMEIdentifier) error {
316+
if id.Type != identifier.DNS {
317+
return errInvalidIdentifier
318+
}
319+
domain := id.Value
320+
321+
if err := pa.ValidDomain(domain); err != nil {
322+
return err
323+
}
324+
303325
// Require no match against hostname block lists
304326
if err := pa.checkHostLists(domain); err != nil {
305327
return err

ra/ra.go

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
"github.com/letsencrypt/boulder/features"
2626
"github.com/letsencrypt/boulder/goodkey"
2727
bgrpc "github.com/letsencrypt/boulder/grpc"
28-
"github.com/letsencrypt/boulder/iana"
2928
"github.com/letsencrypt/boulder/identifier"
3029
blog "github.com/letsencrypt/boulder/log"
3130
"github.com/letsencrypt/boulder/metrics"
@@ -349,22 +348,22 @@ func (ra *RegistrationAuthorityImpl) validateContacts(ctx context.Context, conta
349348

350349
for _, contact := range *contacts {
351350
if contact == "" {
352-
return berrors.MalformedError("empty contact")
351+
return berrors.InvalidEmailError("empty contact")
353352
}
354353
parsed, err := url.Parse(contact)
355354
if err != nil {
356-
return berrors.MalformedError("invalid contact")
355+
return berrors.InvalidEmailError("invalid contact")
357356
}
358357
if parsed.Scheme != "mailto" {
359-
return berrors.MalformedError("contact method %s is not supported", parsed.Scheme)
358+
return berrors.InvalidEmailError("contact method %q is not supported", parsed.Scheme)
360359
}
361360
if !core.IsASCII(contact) {
362-
return berrors.MalformedError(
363-
"contact email [%s] contains non-ASCII characters",
361+
return berrors.InvalidEmailError(
362+
"contact email [%q] contains non-ASCII characters",
364363
contact,
365364
)
366365
}
367-
if err := validateEmail(parsed.Opaque); err != nil {
366+
if err := ra.validateEmail(parsed.Opaque); err != nil {
368367
return err
369368
}
370369
}
@@ -385,9 +384,9 @@ var forbiddenMailDomains = map[string]bool{
385384
}
386385

387386
// validateEmail returns an error if the given address is not parseable as an
388-
// email address or if the domain portion of the email address is a member of
389-
// the forbiddenMailDomains map.
390-
func validateEmail(address string) error {
387+
// email address or if the domain portion of the email address is invalid or
388+
// a member of the forbiddenMailDomains map.
389+
func (ra *RegistrationAuthorityImpl) validateEmail(address string) error {
391390
email, err := mail.ParseAddress(address)
392391
if err != nil {
393392
if len(address) > 254 {
@@ -397,14 +396,16 @@ func validateEmail(address string) error {
397396
}
398397
splitEmail := strings.SplitN(email.Address, "@", -1)
399398
domain := strings.ToLower(splitEmail[len(splitEmail)-1])
399+
if err := ra.PA.ValidDomain(domain); err != nil {
400+
return berrors.InvalidEmailError(
401+
"contact email %q has invalid domain : %s",
402+
email.Address, err)
403+
}
400404
if forbiddenMailDomains[domain] {
401405
return berrors.InvalidEmailError(
402406
"invalid contact domain. Contact emails @%s are forbidden",
403407
domain)
404408
}
405-
if _, err := iana.ExtractSuffix(domain); err != nil {
406-
return berrors.InvalidEmailError("email domain name does not end in a IANA suffix")
407-
}
408409
return nil
409410
}
410411

ra/ra_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3708,7 +3708,9 @@ func TestIssueCertificateInnerErrs(t *testing.T) {
37083708
}
37093709

37103710
func TestValidateEmailError(t *testing.T) {
3711-
err := validateEmail("(๑•́ ω •̀๑)")
3711+
_, _, ra, _, cleanUp := initAuthorities(t)
3712+
defer cleanUp()
3713+
err := ra.validateEmail("(๑•́ ω •̀๑)")
37123714
test.AssertEquals(t, err.Error(), "\"(๑•́ ω •̀๑)\" is not a valid e-mail address")
37133715
}
37143716

test/config-next/ra.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
"ra": {
33
"rateLimitPoliciesFilename": "test/rate-limit-policies.yml",
44
"maxConcurrentRPCServerRequests": 100000,
5-
"maxContactsPerRegistration": 100,
5+
"maxContactsPerRegistration": 3,
66
"debugAddr": ":8002",
77
"hostnamePolicyFile": "test/hostname-policy.yaml",
88
"maxNames": 100,

test/config/ra.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
"ra": {
33
"rateLimitPoliciesFilename": "test/rate-limit-policies.yml",
44
"maxConcurrentRPCServerRequests": 100000,
5-
"maxContactsPerRegistration": 100,
5+
"maxContactsPerRegistration": 3,
66
"debugAddr": ":8002",
77
"hostnamePolicyFile": "test/hostname-policy.yaml",
88
"maxNames": 100,

test/integration/common_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ type client struct {
4141
acme.Client
4242
}
4343

44-
func makeClient() (*client, error) {
44+
func makeClient(contacts ...string) (*client, error) {
4545
c, err := acme.NewClient(os.Getenv("DIRECTORY"))
4646
if err != nil {
4747
return nil, fmt.Errorf("Error connecting to acme directory: %v", err)
@@ -50,9 +50,9 @@ func makeClient() (*client, error) {
5050
if err != nil {
5151
return nil, fmt.Errorf("error creating private key: %v", err)
5252
}
53-
account, err := c.NewAccount(privKey, false, true, "mailto:example@letsencrypt.org")
53+
account, err := c.NewAccount(privKey, false, true, contacts...)
5454
if err != nil {
55-
return nil, fmt.Errorf("error creating new account: %v", err)
55+
return nil, err
5656
}
5757
return &client{account, c}, nil
5858
}

test/integration/errors_test.go

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package integration
55
import (
66
"fmt"
77
"os"
8+
"strings"
89
"testing"
910

1011
"github.com/eggsampler/acme/v3"
@@ -33,3 +34,129 @@ func TestTooBigOrderError(t *testing.T) {
3334
test.AssertEquals(t, prob.Detail, "Error creating new order :: Order cannot contain more than 100 DNS names")
3435
}
3536
}
37+
38+
// TestAccountEmailError tests that registering a new account, or updating an
39+
// account, with invalid contact information produces the expected problem
40+
// result to ACME clients.
41+
func TestAccountEmailError(t *testing.T) {
42+
t.Parallel()
43+
os.Setenv("DIRECTORY", "http://boulder:4001/directory")
44+
45+
/*
46+
// TODO(@cpu): Uncomment this when the too-long test case is re-added.
47+
var longStringBuf strings.Builder
48+
for i := 0; i < 254; i++ {
49+
longStringBuf.WriteRune('a')
50+
}
51+
*/
52+
53+
createErrorPrefix := "Error creating new account :: "
54+
updateErrorPrefix := "Unable to update account :: "
55+
56+
testCases := []struct {
57+
name string
58+
contacts []string
59+
expectedProbType string
60+
expectedProbDetail string
61+
}{
62+
{
63+
name: "empty contact",
64+
contacts: []string{"mailto:valid@valid.com", ""},
65+
expectedProbType: "urn:ietf:params:acme:error:invalidEmail",
66+
expectedProbDetail: `empty contact`,
67+
},
68+
{
69+
name: "empty proto",
70+
contacts: []string{"mailto:valid@valid.com", " "},
71+
expectedProbType: "urn:ietf:params:acme:error:invalidEmail",
72+
expectedProbDetail: `contact method "" is not supported`,
73+
},
74+
{
75+
name: "empty mailto",
76+
contacts: []string{"mailto:valid@valid.com", "mailto:"},
77+
expectedProbType: "urn:ietf:params:acme:error:invalidEmail",
78+
expectedProbDetail: `"" is not a valid e-mail address`,
79+
},
80+
{
81+
name: "non-ascii mailto",
82+
contacts: []string{"mailto:valid@valid.com", "mailto:cpu@l̴etsencrypt.org"},
83+
expectedProbType: "urn:ietf:params:acme:error:invalidEmail",
84+
expectedProbDetail: `contact email ["mailto:cpu@l̴etsencrypt.org"] contains non-ASCII characters`,
85+
},
86+
{
87+
name: "too many contacts",
88+
contacts: []string{"a", "b", "c", "d"},
89+
expectedProbType: "urn:ietf:params:acme:error:malformed",
90+
expectedProbDetail: `too many contacts provided: 4 > 3`,
91+
},
92+
{
93+
name: "invalid contact",
94+
contacts: []string{"mailto:valid@valid.com", "mailto:a@"},
95+
expectedProbType: "urn:ietf:params:acme:error:invalidEmail",
96+
expectedProbDetail: `"a@" is not a valid e-mail address`,
97+
},
98+
{
99+
name: "forbidden contact domain",
100+
contacts: []string{"mailto:valid@valid.com", "mailto:a@example.com"},
101+
expectedProbType: "urn:ietf:params:acme:error:invalidEmail",
102+
expectedProbDetail: "invalid contact domain. Contact emails @example.com are forbidden",
103+
},
104+
{
105+
name: "contact domain invalid TLD",
106+
contacts: []string{"mailto:valid@valid.com", "mailto:a@example.cpu"},
107+
expectedProbType: "urn:ietf:params:acme:error:invalidEmail",
108+
expectedProbDetail: `contact email "a@example.cpu" has invalid domain : Domain name does not end with a valid public suffix (TLD)`,
109+
},
110+
{
111+
name: "contact domain invalid",
112+
contacts: []string{"mailto:valid@valid.com", "mailto:a@example./.com"},
113+
expectedProbType: "urn:ietf:params:acme:error:invalidEmail",
114+
expectedProbDetail: "contact email \"a@example./.com\" has invalid domain : Domain name contains an invalid character",
115+
},
116+
/*
117+
// NOTE(@cpu): Disabled for now - causes serverInternal err when SA saves
118+
// contacts.
119+
{
120+
name: "too long contact",
121+
contacts: []string{
122+
fmt.Sprintf("mailto:%s@a.com", longStringBuf.String()),
123+
},
124+
expectedProbType: "urn:ietf:params:acme:error:invalidEmail",
125+
expectedProbDetail: "??????",
126+
},
127+
*/
128+
}
129+
130+
for _, tc := range testCases {
131+
t.Run(tc.name, func(t *testing.T) {
132+
// First try registering a new account and ensuring the expected problem occurs
133+
if _, err := makeClient(tc.contacts...); err != nil {
134+
if prob, ok := err.(acme.Problem); !ok {
135+
t.Fatalf("expected acme.Problem error got %#v", err)
136+
} else {
137+
test.AssertEquals(t, prob.Type, tc.expectedProbType)
138+
test.AssertEquals(t, prob.Detail, createErrorPrefix+tc.expectedProbDetail)
139+
}
140+
} else if err == nil {
141+
t.Errorf("expected %s type problem for %q, got nil",
142+
tc.expectedProbType, strings.Join(tc.contacts, ","))
143+
}
144+
145+
// Next try making a client with a good contact and updating with the test
146+
// case contact info. The same problem should occur.
147+
c, err := makeClient("mailto:valid@valid.com")
148+
test.AssertNotError(t, err, "failed to create account with valid contact")
149+
if _, err := c.UpdateAccount(c.Account, tc.contacts...); err != nil {
150+
if prob, ok := err.(acme.Problem); !ok {
151+
t.Fatalf("expected acme.Problem error after updating account got %#v", err)
152+
} else {
153+
test.AssertEquals(t, prob.Type, tc.expectedProbType)
154+
test.AssertEquals(t, prob.Detail, updateErrorPrefix+tc.expectedProbDetail)
155+
}
156+
} else if err == nil {
157+
t.Errorf("expected %s type problem after updating account to %q, got nil",
158+
tc.expectedProbType, strings.Join(tc.contacts, ","))
159+
}
160+
})
161+
}
162+
}

test/integration/revocation_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ func TestPrecertificateRevocation(t *testing.T) {
4040

4141
// Create a base account to use for revocation tests.
4242
os.Setenv("DIRECTORY", "http://boulder:4001/directory")
43-
c, err := makeClient()
43+
c, err := makeClient("mailto:example@letsencrypt.org")
4444
test.AssertNotError(t, err, "creating acme client")
4545

4646
// Create a specific key for CSRs so that it is possible to test revocation

0 commit comments

Comments
 (0)