Skip to content

Update tests to allow chains in any order. Fixes #68. - #69

Merged
lgarron merged 1 commit into
masterfrom
cert-set-test-fix
Aug 12, 2020
Merged

Update tests to allow chains in any order. Fixes #68.#69
lgarron merged 1 commit into
masterfrom
cert-set-test-fix

Conversation

@lgarron

@lgarronlgarron commented Jul 30, 2020

Copy link
Copy Markdown
Contributor

The chain in question is defined in RFC5652 as a CertificateSet, which is a SET OF certs:

Go 1.15 introduced a change that sorts such sets: https://go.googlesource.com/go/+/f0cea848679b8f8cdc5f76e1b1e36ebb924a68f8

Our tests were implicitly relying on the sets to stay in the order we order we create them (leaf then intermediate then root, or a truncated subset of those), but the right thing to do at this point is update our tests.

Comment threadcommand_sign_test.go
@@ -37,8 +46,8 @@ func TestSignIncludeCertsAIA(t *testing.T) {
require.NoError(t, err)

require.Equal(t, 2, len(certs))

@lgarronlgarronJul 30, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we already check the chain size, so we'll still be testing that the chains are "equal" to what is expected.

@cryptoquecryptoque left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me 👍

The chain in question is defined in RFC5652 as a CertificateSet, which is a `SET OF` certs:
- https://tools.ietf.org/html/rfc5652#section-5.1
- https://tools.ietf.org/html/rfc5652#section-10.2.3
Go 1.15 introduced a change that sorts such sets: https://go.googlesource.com/go/+/f0cea848679b8f8cdc5f76e1b1e36ebb924a68f8
Our tests were implicitly relying on the sets to stay sorted, but the right thing to do at this point is update our tests.
@lgarron
lgarron merged commit d7c50b9 into masterAug 12, 2020
@lgarron
lgarron deleted the cert-set-test-fix branch August 12, 2020 00:57
@ptoomey3

Copy link
Copy Markdown
Member

🎊

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@lgarron@ptoomey3@fletchto99@cryptoque