Failing Travis unit tests using "go: master" configuration

[Kurlee]

Since commit 7e96b8b, Travis unit tests have been failing. The tests fail when there is an equality test between leaf certificates. I believe that this bug is not due to some bug in smimesign after some troubleshooting.

Expected Behavior

Unit tests should pass on all versions of golang

Current Behavior

Several unit tests fail when testing against the development branch of golang. This occurs when comparing two leaf certificates. After troubleshooting I found that the first handful of bytes match between the two certs (so I assume that they are the same cert). However, the length does not match, and the length does not match the results when testing against the other versions of golang. In all local tests, and in the golang 1.12 and 1.14 tests in Travis, both certs are 842 bytes in length. In the Travis unit tests using the devel branch of golang, the certs are 866 bytes in length for the cert parsed from signed data, and 875 bytes for the test cert used to generate the signed data.

There doesn't seem to be any issues in creating signed data, parsing the content or reading certs from signed data.

Possible Solution

Because both the cert used to sign (Unmodified since generation), and the cert recovered from the signed data both have different lengths, I believe this issue likely did not originate from a bug in this project. I cannot offer any possible solutions currently but will continue to troubleshoot this issue.

Steps to Reproduce

Link to latest failed Travis run

1. Trigger a Travis test using the go: master configuration

Context (Environment)

This issue has not affected me in my use of smimesign on Windows using go version 1.14.6 (64 bit), and it does not seem to affect other platforms using the latest version of go either. Unit tests are run on OSX only and I have not had a chance to swap platforms.

[Kurlee]

I think I have narrowed down the issue to a change in the crypto/x509 library, specifically the change introduced in the commit 6f3a9515b6bb879472f3b3443a052b07ed11ee2f.

This change adds the SubjectKeyId extension for CAs and is generated by default when creating a CA using the x509.CreateCertificate method. The fakeca package used for CA generation for unit testing calls this method in the fakeca.generate method.

This change causes the leaf certs generated during testing by the fakeca package to be a tad longer, due to the 2.5.29.35 authorityKeyIdentifier field which is added because certs use this field to refer to their parent certificate. You can easily see the difference in certificates generated between the latest release of golang and the master branch of golang. Certs generated using the master branch are about 31 bytes longer.

The root cause of the issue is that the cms package likely does not expect to parse this field when reading certificates from the signed data and fails to pass the option to the certificate it returns. I still need to confirm that the cms package is the misbehaving component, but that is my strong suspicion. If the cms package is the issue, then I will close this issue.

[ptoomey3]

Thanks for the super helpful info. @lgarron is going to take a look when he has some spare cycles.

[Kurlee]

Thanks @ptoomey3,

@lgarron After some additional troubleshooting yesterday, I found the issue. Seems way more simple than my original theory.
The unit tests assume the order of the certificates that the cms signed data object returns when calling the X509Certificates function. For some unknown reason, the latest unstable build of golang causes the certificates to be parsed and returned in reverse order than previous versions. Modifying the TestSignIncludeCertsAIA unit test in command_sign_test

From

certs, err := sd.X509Certificates()
require.NoError(t, err)

require.Equal(t, 2, len(certs))
require.True(t, certs[0].Equal(aiaLeaf.Certificate))
require.True(t, certs[1].Equal(intermediate.Certificate))

To

certs, err := sd.X509Certificates()
require.NoError(t, err)

require.Equal(t, 2, len(certs))
// Note the certs index swap
require.True(t, certs[1].Equal(aiaLeaf.Certificate))
require.True(t, certs[0].Equal(intermediate.Certificate))

Causes the unit test to pass.

If the order of the certificates returned from the X509Certificates function cannot be assumed, then I suggest using another method of certificate identification, such as

• Compare the leaf/intermediate cert to a cert in the array with a matching signature
• Compare the leaf/intermediate cert to a cert in the array with a matching length

Let me know what you suggest as the best path forward and I will take care of implementing the changes if this is desired.

Thanks guys.

[lgarron]

@Kurlee Thanks! I had just gotten debugging working locally, and you have it spot-on!

What throws me off here is that the order is not just reversed. In one place it becomes:

require.True(t, certs[1].Equal(leaf.Certificate))
require.True(t, certs[2].Equal(intermediate.Certificate))
require.True(t, certs[0].Equal(ca.Certificate))

So what it's actually doing is putting the highest-level CA first, and producing an unsorted order. I assume that matching the web certificate order is better.

From talking to @FiloSottile I should see if this is possibly a Go 1.15 bug, and maybe file an issue. I'm currently trying to double-check if this is somehow the fault of smimesign or GPG, though.

[lgarron]

Okay, I've narrowed it down to this line: https://github.com/github/cms/blob/master/protocol/protocol.go#L790

If we write out some info surrounding that line:

_ = ioutil.WriteFile("/tmp/smimesign_cms_cid_pre_marshal_0.raw", sd.Certificates[0].FullBytes, 0644)
_ = ioutil.WriteFile("/tmp/smimesign_cms_cid_pre_marshal_1.raw", sd.Certificates[1].FullBytes, 0644)
_ = ioutil.WriteFile("/tmp/smimesign_cms_cid_pre_marshal_2.raw", sd.Certificates[2].FullBytes, 0644)

der, err := asn1.Marshal(ci)

_ = ioutil.WriteFile("/tmp/smimesign_cms_cid_post_marshal.raw", der, 0644)

return der, err

... then the first three files are in the expected order (leaf, intermediate, root), but the der is in the wrong order (root, leaf, intermediate).

I suspect https://go.googlesource.com/go/+/f0cea848679b8f8cdc5f76e1b1e36ebb924a68f8 as the cause of this.

[lgarron]

It looks like this was an intentional design decision for Go 1.15 based on a DER ordering spec: golang/go#24254 (comment)

I'll wait to hear back from @FiloSottile about whether we should expect the cert order to (possibly) change.

Updating the tests is easy in that case (just write a helper function that checks for each cert in any position, and double-check that the chain length matches). I think @mastahyeti also implemented robust path building in GitHub itself, although I don't know what the implications of reordered certs are for other clients.

[lgarron]

So, it looks like we are tagging our certificates as a set when it should maybe be a sequence:

https://github.com/github/cms/blob/c91ec665515d079488bcbc69bd9022d7f4ed7909/protocol/protocol.go#L580

Removing set, from that line fixes the order, but I don't know if it could cause other issues.

From the looks of it, a lot of fields are tagged with set when the surrounding documentation suggests they shouldn't. I can't tell if this was intentional, or e.g. a copy-paste typo.

[lgarron]

From the looks of it, a lot of fields are tagged with set when the surrounding documentation suggests they shouldn't. I can't tell if this was intentional, or e.g. a copy-paste typo.

Just to follow up, this actually not the case, because it's actually not a list of CertificateSets, but a list that represents a CertificateSet. So it seems that the set order was intentional. We should try updating the tests and see if GitHub accepts the chain in a different order.

@BenToews @mastahyeti If we're making a grave mistake, feel free to jump in here and tell us. :-P

[lgarron]

@BenToews @mastahyeti If we're making a grave mistake, feel free to jump in here and tell us. :-P

Uh, follow-up: A little earlier I discovered that the spec (RFC5652) itself actually specifies a set, not a sequence.

https://tools.ietf.org/html/rfc5652#section-5.1
https://tools.ietf.org/html/rfc5652#section-10.2.3

This means we shouldn't have compatibility issues in consumers of this module, if they are following standards.

Go 1.14 had a bug that we happened to rely on, so I'll fix up the tests.

[Kurlee]

Thank you @lgarron for that speedy turnaround and in depth resolution! Glad to see this get sorted.