From caf3c32bf7df659d2606d41afa749a1cade08517 Mon Sep 17 00:00:00 2001 From: Freddy Zhang <38476084+breadyzhang@users.noreply.github.com> Date: Wed, 20 Sep 2023 07:43:14 -0700 Subject: [PATCH] Revert "Fix flaky CT submission bug (#1085)" This reverts commit 1bf39e3d1498b7fe231dadda80c8a2761531a14b. --- CHANGELOG.md | 3 -- ctpolicy/chromepolicy.go | 6 +-- ctpolicy/chromepolicy_test.go | 6 +-- ctpolicy/ctpolicy.go | 14 +++--- submission/races.go | 87 ++++++++++++++++++++++------------- submission/races_test.go | 34 +++++++------- 6 files changed, 84 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e1bfff2cb..668f337332 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,6 @@ ## HEAD -### Fix flaky submission failures -* #1084: CT flaky submission failures - ### Add support for WASI port * Add build tags for wasip1 GOOS diff --git a/ctpolicy/chromepolicy.go b/ctpolicy/chromepolicy.go index 09e4e8e1d7..910fb7d62a 100644 --- a/ctpolicy/chromepolicy.go +++ b/ctpolicy/chromepolicy.go @@ -22,8 +22,8 @@ import ( ) const ( - dayDuration = 24 * time.Hour // time.Duration of one day - minDistinctOperators = 2 // Number of distinct CT log operators that submit an SCT + minOperators = 2 // minimum number of distinct CT log operators that issue an SCT. + dayDuration = 86400 * time.Second // time.Duration of one day ) // ChromeCTPolicy implements logic for complying with Chrome's CT log policy @@ -56,7 +56,7 @@ func (chromeP ChromeCTPolicy) LogsByGroup(cert *x509.Certificate, approved *logl if err != nil { return nil, err } - baseGroup.MinDistinctOperators = minDistinctOperators + baseGroup.MinOperators = minOperators groups[baseGroup.Name] = baseGroup return groups, nil } diff --git a/ctpolicy/chromepolicy_test.go b/ctpolicy/chromepolicy_test.go index 573c95b7cb..eba41ba72c 100644 --- a/ctpolicy/chromepolicy_test.go +++ b/ctpolicy/chromepolicy_test.go @@ -61,9 +61,9 @@ func wantedGroups(base int, minusBob bool) LogPolicyData { "https://ct.googleapis.com/racketeer/": true, "https://log.bob.io": true, }, - MinInclusions: base, - MinDistinctOperators: minDistinctOperators, - IsBase: true, + MinInclusions: base, + MinOperators: minOperators, + IsBase: true, LogWeights: map[string]float32{ "https://ct.googleapis.com/logs/argon2020/": 1.0, "https://ct.googleapis.com/aviator/": 1.0, diff --git a/ctpolicy/ctpolicy.go b/ctpolicy/ctpolicy.go index dabde4f93c..d0d44ac545 100644 --- a/ctpolicy/ctpolicy.go +++ b/ctpolicy/ctpolicy.go @@ -31,13 +31,13 @@ const ( // LogGroupInfo holds information on a single group of logs specified by Policy. type LogGroupInfo struct { - Name string - LogURLs map[string]bool // set of members - MinInclusions int // Required number of submissions. - MinDistinctOperators int // Required number of distinct CT log operators that submit an SCT. - IsBase bool // True only for Log-group covering all logs. - LogWeights map[string]float32 // weights used for submission, default weight is 1 - wMu sync.RWMutex // guards weights + Name string + LogURLs map[string]bool // set of members + MinInclusions int // Required number of submissions. + MinOperators int // Required number of distinct CT log operators. + IsBase bool // True only for Log-group covering all logs. + LogWeights map[string]float32 // weights used for submission, default weight is 1 + wMu sync.RWMutex // guards weights } func (group *LogGroupInfo) setMinInclusions(i int) error { diff --git a/submission/races.go b/submission/races.go index e1bbee8962..f9edb1fa5b 100644 --- a/submission/races.go +++ b/submission/races.go @@ -52,12 +52,12 @@ type groupState struct { // When some group is complete cancels all requests that are not needed by any // group. type safeSubmissionState struct { - mu sync.Mutex - logToGroups map[string]ctpolicy.GroupSet - remainingSubmissions int // number of logs that still need to be submitted. - minDistinctGroups int // number of groups that need a submission - groupsSubmitted map[string]bool // number of logs submitted to each group. + mu sync.Mutex + logToGroups map[string]ctpolicy.GroupSet + groupNeeds map[string]int // number of logs that need to be submitted for each group. + minGroups int // minimum number of distinct groups that need a log submitted. + groups map[string]bool // set of groups that have a log submitted. results map[string]*submissionResult cancels map[string]context.CancelFunc } @@ -65,11 +65,14 @@ type safeSubmissionState struct { func newSafeSubmissionState(groups ctpolicy.LogPolicyData) *safeSubmissionState { var s safeSubmissionState s.logToGroups = ctpolicy.GroupByLogs(groups) + s.groupNeeds = make(map[string]int) + for _, g := range groups { + s.groupNeeds[g.Name] = g.MinInclusions + } if baseGroup, ok := groups[ctpolicy.BaseName]; ok { - s.remainingSubmissions = baseGroup.MinInclusions - s.minDistinctGroups = baseGroup.MinDistinctOperators + s.minGroups = baseGroup.MinOperators } - s.groupsSubmitted = make(map[string]bool) + s.groups = make(map[string]bool) s.results = make(map[string]*submissionResult) s.cancels = make(map[string]context.CancelFunc) return &s @@ -85,7 +88,15 @@ func (sub *safeSubmissionState) request(logURL string, cancel context.CancelFunc return false } sub.results[logURL] = &submissionResult{} - if sub.remainingSubmissions <= 0 { + isAwaited := false + for g := range sub.logToGroups[logURL] { + if sub.groupNeeds[g] > 0 { + isAwaited = true + break + } + } + if !isAwaited { + // No groups expecting result from this Log. return false } sub.cancels[logURL] = cancel @@ -102,45 +113,51 @@ func (sub *safeSubmissionState) setResult(logURL string, sct *ct.SignedCertifica sub.results[logURL] = &submissionResult{sct: sct, err: err} return } - // group name associated with logURL outside of BaseName. - // (this assumes the logURL is associated with only one group ignoring BaseName) // If at least one group needs that SCT, result is set. Otherwise dumped. for groupName := range sub.logToGroups[logURL] { // Ignore the base group (All-logs) here to check separately. if groupName == ctpolicy.BaseName { continue } - // Set the result if the group does not have a submission. - if !sub.groupsSubmitted[groupName] { + if sub.groupNeeds[groupName] > 0 { sub.results[logURL] = &submissionResult{sct: sct, err: err} - sub.groupsSubmitted[groupName] = true } + sub.groups[groupName] = true + sub.groupNeeds[groupName]-- } // Check the base group (All-logs) only if sub.logToGroups[logURL][ctpolicy.BaseName] { if sub.results[logURL].sct != nil { - // The cert has been observed in a non-base group, so account for it. - sub.remainingSubmissions-- - } else if sub.remainingSubmissions > 0 { - // Arriving at this portion of the code implies that the result contains an SCT from - // the same log operator. - // reservedSubmissions represents the number of submissions that still need to be - // submitted from different log operators. - reservedSubmissions := sub.minDistinctGroups - len(sub.groupsSubmitted) + // It is already processed in a non-base group, so we can reduce the groupNeeds for the base group as well. + sub.groupNeeds[ctpolicy.BaseName]-- + } else if sub.groupNeeds[ctpolicy.BaseName] > 0 { + minInclusionsForOtherGroup := 0 + for g, cnt := range sub.groupNeeds { + if g != ctpolicy.BaseName && cnt > 0 { + minInclusionsForOtherGroup += cnt + } + } // Set the result only if the base group still needs SCTs more than total counts // of minimum inclusions for other groups. - if sub.remainingSubmissions > reservedSubmissions { + if sub.groupNeeds[ctpolicy.BaseName] > minInclusionsForOtherGroup { sub.results[logURL] = &submissionResult{sct: sct, err: err} - sub.remainingSubmissions-- + sub.groupNeeds[ctpolicy.BaseName]-- } } } // Cancel any pending Log-requests for which there are no more awaiting // Log-groups. - for logURL := range sub.logToGroups { - if sub.remainingSubmissions <= 0 && sub.cancels[logURL] != nil { + for logURL, groupSet := range sub.logToGroups { + isAwaited := false + for g := range groupSet { + if sub.groupNeeds[g] > 0 { + isAwaited = true + break + } + } + if !isAwaited && sub.cancels[logURL] != nil { sub.cancels[logURL]() sub.cancels[logURL] = nil } @@ -148,13 +165,17 @@ func (sub *safeSubmissionState) setResult(logURL string, sct *ct.SignedCertifica } // groupComplete returns true iff the specified group has all the SCTs it needs. -func (sub *safeSubmissionState) groupComplete() bool { +func (sub *safeSubmissionState) groupComplete(groupName string) bool { sub.mu.Lock() defer sub.mu.Unlock() - if len(sub.groupsSubmitted) < sub.minDistinctGroups { + needs, ok := sub.groupNeeds[groupName] + if !ok { + return true + } + if len(sub.groups) < sub.minGroups { return false } - return sub.remainingSubmissions <= 0 + return needs <= 0 } func (sub *safeSubmissionState) collectSCTs() []*AssignedSCT { @@ -205,7 +226,7 @@ func groupRace(ctx context.Context, chain []ct.ASN1Cert, asPreChain bool, return case <-timeoutchan: } - if state.groupComplete() { + if state.groupComplete(group.Name) { cancel() return } @@ -222,14 +243,14 @@ func groupRace(ctx context.Context, chain []ct.ASN1Cert, asPreChain bool, for range session { select { case <-ctx.Done(): - return groupState{Name: group.Name, Success: state.groupComplete()} + return groupState{Name: group.Name, Success: state.groupComplete(group.Name)} case <-counter: - if state.groupComplete() { + if state.groupComplete(group.Name) { return groupState{Name: group.Name, Success: true} } } } - return groupState{Name: group.Name, Success: state.groupComplete()} + return groupState{Name: group.Name, Success: state.groupComplete(group.Name)} } func parallelNums(groups ctpolicy.LogPolicyData) map[string]int { diff --git a/submission/races_test.go b/submission/races_test.go index 11cd82309c..90b8045b91 100644 --- a/submission/races_test.go +++ b/submission/races_test.go @@ -93,7 +93,7 @@ func TestGetSCTs(t *testing.T) { name: "singleGroupOneSCT", sbMock: &mockSubmitter{fixedDelay: map[byte]time.Duration{'a': 0}, firstLetterURLReqNumber: make(map[byte]int)}, groups: ctpolicy.LogPolicyData{ - ctpolicy.BaseName: { + "a": { Name: "a", LogURLs: map[string]bool{"a1.com": true, "a2.com": true}, MinInclusions: 1, @@ -107,7 +107,7 @@ func TestGetSCTs(t *testing.T) { name: "singleGroupMultiSCT", sbMock: &mockSubmitter{fixedDelay: map[byte]time.Duration{'a': 0}, firstLetterURLReqNumber: make(map[byte]int)}, groups: ctpolicy.LogPolicyData{ - ctpolicy.BaseName: { + "a": { Name: "a", LogURLs: map[string]bool{"a1.com": true, "a2.com": true, "a3.com": true, "a4.com": true, "a5.com": true}, MinInclusions: 3, @@ -124,27 +124,27 @@ func TestGetSCTs(t *testing.T) { "a": { Name: "a", LogURLs: map[string]bool{"a1.com": true, "a2.com": true, "a3.com": true, "a4.com": true}, - MinInclusions: 0, + MinInclusions: 1, IsBase: false, LogWeights: map[string]float32{"a1.com": 1.0, "a2.com": 1.0, "a3.com": 1.0, "a4.com": 1.0}, }, "b": { Name: "b", LogURLs: map[string]bool{"b1.com": true, "b2.com": true, "b3.com": true, "b4.com": true}, - MinInclusions: 0, + MinInclusions: 1, IsBase: false, LogWeights: map[string]float32{"b1.com": 1.0, "b2.com": 1.0, "b3.com": 1.0, "b4.com": 1.0}, }, ctpolicy.BaseName: { - Name: ctpolicy.BaseName, - LogURLs: map[string]bool{"a1.com": true, "a2.com": true, "a3.com": true, "a4.com": true, "b1.com": true, "b2.com": true, "b3.com": true, "b4.com": true}, - MinInclusions: 2, - MinDistinctOperators: 2, - IsBase: true, - LogWeights: map[string]float32{"a1.com": 1.0, "a2.com": 1.0, "a3.com": 1.0, "a4.com": 1.0, "b1.com": 1.0, "b2.com": 1.0, "b3.com": 1.0, "b4.com": 1.0}, + Name: ctpolicy.BaseName, + LogURLs: map[string]bool{"a1.com": true, "a2.com": true, "a3.com": true, "a4.com": true, "b1.com": true, "b2.com": true, "b3.com": true, "b4.com": true}, + MinInclusions: 5, + MinOperators: 2, + IsBase: true, + LogWeights: map[string]float32{"a1.com": 1.0, "a2.com": 1.0, "a3.com": 1.0, "a4.com": 1.0, "b1.com": 1.0, "b2.com": 1.0, "b3.com": 1.0, "b4.com": 1.0}, }, }, - resultTrail: map[string]int{"a": 1, "b": 1, ctpolicy.BaseName: 2}, + resultTrail: map[string]int{"a": 1, "b": 1, ctpolicy.BaseName: 5}, }, { name: "notEnoughDistinctOperators", @@ -158,12 +158,12 @@ func TestGetSCTs(t *testing.T) { LogWeights: map[string]float32{"a1.com": 1.0, "a2.com": 1.0, "a3.com": 1.0, "a4.com": 1.0}, }, ctpolicy.BaseName: { - Name: ctpolicy.BaseName, - LogURLs: map[string]bool{"a1.com": true, "a2.com": true, "a3.com": true, "a4.com": true}, - MinInclusions: 2, - MinDistinctOperators: 2, - IsBase: true, - LogWeights: map[string]float32{"a1.com": 1.0, "a2.com": 1.0, "a3.com": 1.0, "a4.com": 1.0}, + Name: ctpolicy.BaseName, + LogURLs: map[string]bool{"a1.com": true, "a2.com": true, "a3.com": true, "a4.com": true}, + MinInclusions: 2, + MinOperators: 2, + IsBase: true, + LogWeights: map[string]float32{"a1.com": 1.0, "a2.com": 1.0, "a3.com": 1.0, "a4.com": 1.0}, }, }, errRegexp: regexp.MustCompile("didn't receive enough SCTs"),