Skip to content

Commit

Permalink
Fix flaky CT submission bug (#1085)
Browse files Browse the repository at this point in the history 
* Create additional conditions to determine log results and if a log should be submitted when minInclusions > 0.

* modify chromeLike unit test to better fit the actual use case and refine how safeSubmissionState decides which SCTs to insert in the results

* update changelog

* Remove MaxSubmissions and set maxSubmissionsPerGroup from the minGroups value

* Set max submissions per operator in ctpolicy package

* Removed  to reduce complexity and confusion.

* Resolving comments

- move base into switch expression

- change maxSubmissionsPerGroup to maxSubmissionsPerOperator

* Use MinDistinctOperators instead of MaxSubmissionsPerOperator to reduce confusion

* Add zOS build support (#1088)

* Add support for WASI port (#1089)

Fix building when the new `wasip1` port is being used.
This is a new target that will be introduced by go 1.21.

For more details golang/go#58141

Signed-off-by: Flavio Castelli <fcastelli@suse.com>

* update changelog

* fix changelog merge issues

* My merge conflict mishap reverted the change to have groups be a map[string]int so I am reverting it back to the updated state

* replace groupNeeds with minSubmissions and change the name of groups to groupsSubmitted

groupNeeds was used for the old chrome policy when we required SCTs from specific groups. It's not necessary anymore with the new policies so a single integer (minSubmissions) should be suffice.

groups is changed to groupsSubmitted to make it easier to understand upon a glance.

* change minSubmissions since it gets changed after initialization

* Change dayDuration to use time.Hour for easier understanding

* Resolve comments

* add comments to clarify reservedSubmissions

---------

Co-authored-by: Freddy Zhang <zhangfreddy@google.com>
Co-authored-by: onlywork1984 <102848417+onlywork1984@users.noreply.github.com>
Co-authored-by: Flavio Castelli <flavio@castelli.me>
  • Loading branch information
@breadyzhang @onlywork1984 @flavio
4 people authored Jun 23, 2023
1 parent b379604 commit 1bf39e3
Show file tree
Hide file tree
Showing 6 changed files with 66 additions and 84 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## 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 (
minOperators = 2 // minimum number of distinct CT log operators that issue an SCT.
dayDuration = 86400 * time.Second // time.Duration of one day
dayDuration = 24 * time.Hour // time.Duration of one day
minDistinctOperators = 2 // Number of distinct CT log operators that submit an SCT
)

// 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.MinOperators = minOperators
baseGroup.MinDistinctOperators = minDistinctOperators
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,
MinOperators: minOperators,
IsBase: true,
MinInclusions: base,
MinDistinctOperators: minDistinctOperators,
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.
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
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
}

func (group *LogGroupInfo) setMinInclusions(i int) error {
Expand Down
87 changes: 33 additions & 54 deletions submission/races.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,27 +52,24 @@ 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
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.
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.

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.minGroups = baseGroup.MinOperators
s.remainingSubmissions = baseGroup.MinInclusions
s.minDistinctGroups = baseGroup.MinDistinctOperators
}
s.groups = make(map[string]bool)
s.groupsSubmitted = make(map[string]bool)
s.results = make(map[string]*submissionResult)
s.cancels = make(map[string]context.CancelFunc)
return &s
Expand All @@ -88,15 +85,7 @@ func (sub *safeSubmissionState) request(logURL string, cancel context.CancelFunc
return false
}
sub.results[logURL] = &submissionResult{}
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.
if sub.remainingSubmissions <= 0 {
return false
}
sub.cancels[logURL] = cancel
Expand All @@ -113,69 +102,59 @@ 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
}
if sub.groupNeeds[groupName] > 0 {
// Set the result if the group does not have a submission.
if !sub.groupsSubmitted[groupName] {
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 {
// 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
}
}
// 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)
// Set the result only if the base group still needs SCTs more than total counts
// of minimum inclusions for other groups.
if sub.groupNeeds[ctpolicy.BaseName] > minInclusionsForOtherGroup {
if sub.remainingSubmissions > reservedSubmissions {
sub.results[logURL] = &submissionResult{sct: sct, err: err}
sub.groupNeeds[ctpolicy.BaseName]--
sub.remainingSubmissions--
}
}
}

// Cancel any pending Log-requests for which there're no more awaiting
// Log-groups.
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 {
for logURL := range sub.logToGroups {
if sub.remainingSubmissions <= 0 && 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(groupName string) bool {
func (sub *safeSubmissionState) groupComplete() bool {
sub.mu.Lock()
defer sub.mu.Unlock()
needs, ok := sub.groupNeeds[groupName]
if !ok {
return true
}
if len(sub.groups) < sub.minGroups {
if len(sub.groupsSubmitted) < sub.minDistinctGroups {
return false
}
return needs <= 0
return sub.remainingSubmissions <= 0
}

func (sub *safeSubmissionState) collectSCTs() []*AssignedSCT {
Expand Down Expand Up @@ -226,7 +205,7 @@ func groupRace(ctx context.Context, chain []ct.ASN1Cert, asPreChain bool,
return
case <-timeoutchan:
}
if state.groupComplete(group.Name) {
if state.groupComplete() {
cancel()
return
}
Expand All @@ -243,14 +222,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(group.Name)}
return groupState{Name: group.Name, Success: state.groupComplete()}
case <-counter:
if state.groupComplete(group.Name) {
if state.groupComplete() {
return groupState{Name: group.Name, Success: true}
}
}
}
return groupState{Name: group.Name, Success: state.groupComplete(group.Name)}
return groupState{Name: group.Name, Success: state.groupComplete()}
}

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{
"a": {
ctpolicy.BaseName: {
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{
"a": {
ctpolicy.BaseName: {
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: 1,
MinInclusions: 0,
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: 1,
MinInclusions: 0,
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: 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},
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},
},
},
resultTrail: map[string]int{"a": 1, "b": 1, ctpolicy.BaseName: 5},
resultTrail: map[string]int{"a": 1, "b": 1, ctpolicy.BaseName: 2},
},
{
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,
MinOperators: 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,
MinDistinctOperators: 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 1bf39e3

Please sign in to comment.