Skip to content

Commit

Permalink
Revert "Fix flaky CT submission bug (#1085)" (#1153)
Browse files Browse the repository at this point in the history 
This reverts commit 1bf39e3.
  • Loading branch information
@breadyzhang
breadyzhang authored Sep 20, 2023
1 parent a69b877 commit 25e0c3f
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 66 deletions.
3 changes: 0 additions & 3 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 3 additions & 3 deletions ctpolicy/chromepolicy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
6 changes: 3 additions & 3 deletions ctpolicy/chromepolicy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 7 additions & 7 deletions ctpolicy/ctpolicy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
87 changes: 54 additions & 33 deletions submission/races.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,24 +52,27 @@ 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
}

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
Expand All @@ -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
Expand All @@ -102,59 +113,69 @@ 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
}
}
}

// 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 {
Expand Down Expand Up @@ -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
}
Expand All @@ -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 {
Expand Down
34 changes: 17 additions & 17 deletions submission/races_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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",
Expand All @@ -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"),
Expand Down

0 comments on commit 25e0c3f

Please sign in to comment.