Skip to content

Commit 7469948

Browse files
jshaDaniel McCarney
jsha
authored and
Daniel McCarney
committed
Fix FasterGetOrderForNames and add tests. (letsencrypt#4331)
This rolls forward letsencrypt#4326 after it was reverted in letsencrypt#4328. Resolves letsencrypt#4329 The older query didn't have a `LIMIT 1` so it was returning multiple results, but gorp's `SelectOne` was okay with multiple results when the selection was going into an `int64`. When I changed this to a `struct` in letsencrypt#4326, gorp started producing errors. For this bug to manifest, an account needs to create an order, then fail validation, twice in a row for a given domain name, then create an order once more for the same domain name - that third request will fail because there are multiple orders in the orderFqdnSets table for that domain. Note that the bug condition doesn't happen when an account does three successful issuances in a row, because finalizing an order (that is, issuing a certificate for it) deletes the row in orderFqdnSets. Failing an authorization does not delete the row in orderFqdnSets. I believe this was an intentional design decision because an authorization can participate in many orders, and those orders can have many other authorizations, so computing the updated state of all those orders would be expensive (remember, order state is not persisted in the DB but is calculated dynamically based on the authorizations it contains). This wasn't detected in integration tests because we don't have any tests that fail validation for the same domain multiple times. I filed an issue for an integration test that would have incidentally caught this: letsencrypt#4332. There's also a more specific test case in letsencrypt#4331.
1 parent df2909a commit 7469948

File tree

6 files changed

+114
-15
lines changed

6 files changed

+114
-15
lines changed

‎features/featureflag_string.go‎

Lines changed: 3 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎features/features.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@ const (
5757
// MandatoryPOSTAsGET forbids legacy unauthenticated GET requests for ACME
5858
// resources.
5959
MandatoryPOSTAsGET
60+
// Use an optimized query for GetOrderForNames.
61+
FasterGetOrderForNames
6062
)
6163

6264
// List of features and their default value, protected by fMu
@@ -82,6 +84,7 @@ var features = map[FeatureFlag]bool{
8284
CheckRenewalFirst: false,
8385
MandatoryPOSTAsGET: false,
8486
DisableAuthz2Orders: false,
87+
FasterGetOrderForNames: false,
8588
}
8689

8790
var fMu = new(sync.RWMutex)

‎sa/sa.go‎

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1812,26 +1812,55 @@ func (ssa *SQLStorageAuthority) GetOrderForNames(
18121812
// Hash the names requested for lookup in the orderFqdnSets table
18131813
fqdnHash := hashNames(req.Names)
18141814

1815-
var orderID int64
1816-
err := ssa.dbMap.WithContext(ctx).SelectOne(&orderID, `
1817-
SELECT orderID
1818-
FROM orderFqdnSets
1819-
WHERE setHash = ?
1820-
AND registrationID = ?
1821-
AND expires > ?`,
1822-
fqdnHash, *req.AcctID, ssa.clk.Now())
1823-
1824-
// There isn't an unexpired order for the provided AcctID that has the
1825-
// fqdnHash requested.
1815+
// Find a possibly-suitable order. We don't include the account ID or order
1816+
// status in this query because there's no index that includes those, so
1817+
// including them could require the DB to scan extra rows.
1818+
// Instead, we select one unexpired order that matches the fqdnSet. If
1819+
// that order doesn't match the account ID or status we need, just return
1820+
// nothing. We use `ORDER BY expires ASC` because the index on
1821+
// (setHash, expires) is in ASC order. DESC would be slightly nicer from a
1822+
// user experience perspective but would be slow when there are many entries
1823+
// to sort.
1824+
// This approach works fine because in most cases there's only one account
1825+
// issuing for a given name. If there are other accounts issuing for the same
1826+
// name, it just means order reuse happens less often.
1827+
var result struct {
1828+
OrderID int64
1829+
RegistrationID int64
1830+
}
1831+
var err error
1832+
if features.Enabled(features.FasterGetOrderForNames) {
1833+
err = ssa.dbMap.WithContext(ctx).SelectOne(&result, `
1834+
SELECT orderID, registrationID
1835+
FROM orderFqdnSets
1836+
WHERE setHash = ?
1837+
AND expires > ?
1838+
ORDER BY expires DESC
1839+
LIMIT 1`,
1840+
fqdnHash, ssa.clk.Now())
1841+
} else {
1842+
err = ssa.dbMap.WithContext(ctx).SelectOne(&result, `
1843+
SELECT orderID, registrationID
1844+
FROM orderFqdnSets
1845+
WHERE setHash = ?
1846+
AND registrationID = ?
1847+
AND expires > ?
1848+
LIMIT 1`,
1849+
fqdnHash, *req.AcctID, ssa.clk.Now())
1850+
}
1851+
18261852
if err == sql.ErrNoRows {
18271853
return nil, berrors.NotFoundError("no order matching request found")
18281854
} else if err != nil {
1829-
// An unexpected error occurred
18301855
return nil, err
18311856
}
18321857

1858+
if result.RegistrationID != *req.AcctID {
1859+
return nil, berrors.NotFoundError("no order matching request found")
1860+
}
1861+
18331862
// Get the order
1834-
order, err := ssa.GetOrder(ctx, &sapb.OrderRequest{Id: &orderID, UseV2Authorizations: req.UseV2Authorizations})
1863+
order, err := ssa.GetOrder(ctx, &sapb.OrderRequest{Id: &result.OrderID, UseV2Authorizations: req.UseV2Authorizations})
18351864
if err != nil {
18361865
return nil, err
18371866
}

‎sa/sa_test.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1979,6 +1979,51 @@ func TestCountOrders(t *testing.T) {
19791979
test.AssertEquals(t, count, 0)
19801980
}
19811981

1982+
func TestFasterGetOrderForNames(t *testing.T) {
1983+
sa, fc, cleanUp := initSA(t)
1984+
defer cleanUp()
1985+
1986+
domain := "example.com"
1987+
expires := fc.Now().Add(time.Hour)
1988+
1989+
reg, err := sa.NewRegistration(ctx, core.Registration{
1990+
Key: satest.GoodJWK(),
1991+
InitialIP: net.ParseIP("42.42.42.42"),
1992+
})
1993+
test.AssertNotError(t, err, "Couldn't create test registration")
1994+
1995+
authz, err := sa.NewPendingAuthorization(ctx, core.Authorization{
1996+
Identifier: identifier.DNSIdentifier(domain),
1997+
RegistrationID: reg.ID,
1998+
Status: core.StatusPending,
1999+
Expires: &expires,
2000+
})
2001+
test.AssertNotError(t, err, "creating authorization")
2002+
2003+
expiresNano := expires.UnixNano()
2004+
_, err = sa.NewOrder(ctx, &corepb.Order{
2005+
RegistrationID: &reg.ID,
2006+
Expires: &expiresNano,
2007+
Authorizations: []string{authz.ID},
2008+
Names: []string{domain},
2009+
})
2010+
test.AssertNotError(t, err, "sa.NewOrder failed")
2011+
2012+
_, err = sa.NewOrder(ctx, &corepb.Order{
2013+
RegistrationID: &reg.ID,
2014+
Expires: &expiresNano,
2015+
Authorizations: []string{authz.ID},
2016+
Names: []string{domain},
2017+
})
2018+
test.AssertNotError(t, err, "sa.NewOrder failed")
2019+
2020+
_, err = sa.GetOrderForNames(ctx, &sapb.GetOrderForNamesRequest{
2021+
AcctID: &reg.ID,
2022+
Names: []string{domain},
2023+
})
2024+
test.AssertNotError(t, err, "sa.GetOrderForNames failed")
2025+
}
2026+
19822027
func TestGetOrderForNames(t *testing.T) {
19832028
sa, fc, cleanUp := initSA(t)
19842029
defer cleanUp()

‎test/config-next/sa.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
]
2525
},
2626
"features": {
27+
"FasterGetOrderForNames": true
2728
}
2829
},
2930

‎test/v2_integration.py‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,26 @@ def test_http_challenge_broken_redirect():
104104

105105
challSrv.remove_http_redirect(challengePath)
106106

107+
def test_fail_thrice():
108+
"""
109+
Fail a challenge for the same domain, with the same account, three times in
110+
a row. This tests a fix for
111+
https://github.com/letsencrypt/boulder/issues/4329. We expect to get
112+
ValidationErrors, but no 500s.
113+
"""
114+
domain = "failthrice." + random_domain()
115+
csr_pem = chisel2.make_csr([domain])
116+
client = chisel2.make_client()
117+
for _ in range(3):
118+
order = client.new_order(csr_pem)
119+
chall = order.authorizations[0].body.challenges[0]
120+
client.answer_challenge(chall, chall.response(client.net.key))
121+
try:
122+
client.poll_and_finalize(order)
123+
except errors.ValidationError as e:
124+
pass
125+
126+
107127
def test_http_challenge_loop_redirect():
108128
client = chisel2.make_client()
109129

0 commit comments

Comments
 (0)