Make E2E tests hermetic #3499

cmurphy · 2024-01-26T00:41:07Z

Use local services for verify tests

(This is the primary change)

Update TestDockerfileVerify and TestManifestVerify to sign ephemeral
images within the tests so that the signatures can be created with and
verified from the locally running Fulcio and Rekor instances instead of
verifying images with the public Rekor instance, so that the tests no
longer depend on external services.

The images are signed using --identity-token to avoid changing the
nature of the verification tests, which were originally written to be
keyless. A mock OIDC server is provisioned to provide the token and
enable verification.

Refactoring/cleanup/supporting changes:

Set rekor env variable in Go test suite

Move the setting of SIGSTORE_REKOR_PUBLIC_KEY from the e2e shell script
to the Go test suite, so that only the tests that need it have it set
and the shell script is doing less setup. Also remove unnecessary
instances of os.RemoveAll for temporary directories that the Go testing
framework will automatically clean up.

Move verify tests from shell script to Go suite

Move the cosign dockerfile verify and cosign manifest verify tests
out of the shell script and into the e2e Go test suite file with all the
other tests. This makes them consistent to manage.

The initialization of fulcio roots in other tests pollutes the trust
root in the new tests, so a reset is added to the fulcioroots package
for testing only.

Fix cleanup in E2E script

Calling trap multiple times replaces the last signal handler rather than
appending to it. This change ensures that the most recent trap includes
all previous traps so that all cleanups are executed.

Replace os.Setenv with testing.Setenv in e2e tests

Using os.Setenv pollutes the testing environment with environment
variables with other tests. Deferring a cleanup function to unset the
variable is unnecessary because the Go testing library provides a Setenv
function that localizes the environment setting to the scope of the
test.

Also removes unused variables.

Depends on sigstore/fulcio#1560
Relates to sigstore/sigstore-probers#105 - by making the current e2e tests more self-contained, creating new sigstore-prober scheduled tests run against the production service instances will be more valuable.

Summary

Release Note

Documentation

codecov · 2024-01-26T00:43:03Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (f43eb6b) 40.10% compared to head (d4243fd) 40.44%.
Report is 17 commits behind head on main.

Files	Patch %	Lines
...ernal/pkg/cosign/fulcio/fulcioroots/fulcioroots.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3499      +/-   ##
==========================================
+ Coverage   40.10%   40.44%   +0.34%     
==========================================
  Files         155      155              
  Lines       10044    10047       +3     
==========================================
+ Hits         4028     4064      +36     
+ Misses       5530     5494      -36     
- Partials      486      489       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

haydentherapper

This is fantastic! Nice work!

After this is merged, we can file an issue to track moving other e2e shell script tests over to this.

haydentherapper · 2024-01-31T00:17:23Z

test/e2e_test.go

@@ -1000,7 +1011,7 @@ func TestAttachWithRekorBundle(t *testing.T) {

 func TestRekorBundle(t *testing.T) {
 	// turn on the tlog
-	defer setenv(t, env.VariableExperimental.String(), "1")()
+	t.Setenv(env.VariableExperimental.String(), "1")


This and any other place in the tests that VariableExperimental is set can be removed, it no longer changes behavior (or if it did, that would be a bug)

haydentherapper · 2024-01-31T00:28:09Z

test/e2e_test.go

+
+	// Use the workload github token to exchange for an OIDC token.
+	// To run tests locally, set GITHUB_TOKEN=$(gh auth token).
+	identityToken, err := getGithubOIDCToken()


To make this entirely hermetic, would it be possible to create a mock OIDC server (we could use https://github.com/chainguard-dev/justtrustme, or create our own like we did in https://github.com/sigstore/fulcio/blob/main/pkg/server/grpc_server_test.go#L1626) and configure Fulcio to trust that provider? That would make these tests locally executable too without fetching a token out of band.

I'll look more closely at those options. I had the perception that an arbitrary OIDC provider would be tricky to integrate because I thought it might require registering it in the fulcio code https://github.com/sigstore/fulcio/blob/main/docs/oidc.md#integration-guide (edited to fix link)

The issue we ran into with conformance tests is that we were testing against the production instance, which couldn't be configured for ephemeral OIDC providers (hence why we use a GitHub Actions token, since that's supported by the prod instance). Since you're spinning up a local instance of Fulcio, we could modify the config (like https://github.com/sigstore/fulcio/blob/main/config/config.jsn) that gets read in when Fulcio starts up so that it trusts the ephemeral OIDC provider.

I put the wrong link in my comment, I meant to link to this doc https://github.com/sigstore/fulcio/blob/main/docs/oidc.md#integration-guide - is that guide up to date?

Ah, that guide is up to date, but you only need to follow it for adding new providers for the public deployment. You can just modify the config which is passed in via https://github.com/sigstore/fulcio/blob/main/cmd/app/serve.go#L90.

haydentherapper

This looks great! Just a small comment about correcting some outdated comments.

haydentherapper · 2024-02-07T18:29:03Z

test/e2e_test.go

@@ -79,6 +84,7 @@ const (
 	serverEnv = "REKOR_SERVER"


Can this be removed? It doesn't look like this env var is read by Cosign.

If we wanted to confirm the verification is offline, you could pass an invalid URL for Rekor to verify

Updated to remove this and set RekorURL instead

haydentherapper · 2024-02-07T18:35:38Z

test/e2e_test.go

@@ -1029,7 +1032,7 @@ func TestRekorBundle(t *testing.T) {

 	// Make sure offline verification works with bundling
 	// use rekor prod since we have hardcoded the public key


Related to the above comment, it looks like the verify function sets IgnoreTlog to true, so it's not checking if the entry is in the log. This is fine, we can do a pass over the e2e tests later to make sure we're covering a few key use cases. We should be good to remove this comment and line though in each test.

Removed this comment since we're not using rekor prod anymore

haydentherapper · 2024-02-07T18:39:20Z

test/e2e_test.go

+						CertIdentity:   certID,
+					},
+					RekorURL:   rekorURL,
+					IgnoreTlog: true,


Since the tests are now hermetic, can we remove IgnoreTlog so it tests inclusion in the log (and for the other new tests)? I would expect the test to still pass.

Removed IgnoreTlog, but also had to add TlogUpload to the sign command.

Some tests were setting the REKOR_URL environment variable to try to test offline verification. This variable is no longer read so it was not doing anything. This change removes the variable and instead sets RekorURL in the command to either the local rekor instance (so that the public instance is not used) or to a bad url with Offline set to true so that offline verification is truly tested. This change also removes the COSIGN_EXPERIMENTAL variable which is no longer used, and replaces os.Setenv with testing.Setenv which localizes the environment setting to the scope of the test and removes the need for a cleanup function. Signed-off-by: Colleen Murphy <[email protected]>

Calling trap multiple times replaces the last signal handler rather than appending to it. This change ensures that the most recent trap includes all previous traps so that all cleanups are executed. Signed-off-by: Colleen Murphy <[email protected]>

Move the `cosign dockerfile verify` and `cosign manifest verify` tests out of the shell script and into the e2e Go test suite file with all the other tests. This makes them consistent to manage. The initialization of fulcio roots in other tests pollutes the trust root in the new tests, so a reset is added to the fulcioroots package for testing only. Signed-off-by: Colleen Murphy <[email protected]>

Update TestDockerfileVerify and TestManifestVerify to sign ephemeral images within the tests so that the signatures can be created with and verified from the locally running Fulcio and Rekor instances instead of verifying images with the public Rekor instance, so that the tests no longer depend on external services. The images are signed using --identity-token to avoid changing the nature of the verification tests, which were originally written to be keyless. A mock OIDC server is provisioned to provide the token and enable verification. Signed-off-by: Colleen Murphy <[email protected]>

Move the setting of SIGSTORE_REKOR_PUBLIC_KEY from the e2e shell script to the Go test suite, so that only the tests that need it have it set and the shell script is doing less setup. Also remove unnecessary instances of os.RemoveAll for temporary directories that the Go testing framework will automatically clean up. Signed-off-by: Colleen Murphy <[email protected]>

haydentherapper · 2024-02-07T22:04:14Z

test/e2e_test.go

+		Annotations:   sigs.AnnotationsMap{Annotations: annotations},
+		Attachment:    attachment,
+		HashAlgorithm: crypto.SHA256,
+		IgnoreTlog:    true,


Can this be removed and the tests still pass? Otherwise we skip rekor verification

This is supposed to be offline only, do we want online rekor verification?

The Rekor proof should be uploaded to the OCI annotation, and then read in during verification and not require contacting the log. Setting "offline" to "true" and providing the fake rekor URL will force offline verification.

For what it's worth, some of these tests are a bit old so there may be cases where this was turned off incorrectly. I would try to set ignoreTlog to false and see if all tests pass. The only case where it might not is if the test set TlogUpload to false (https://github.com/sigstore/cosign/blob/main/cmd/cosign/cli/options/sign.go#L36).

The Rekor proof should be uploaded to the OCI annotation

It's only uploaded if I set TlogUpload to true in the signing command, so I'll have to update all of those.

I would try to set ignoreTlog to false and see if all tests pass

This is a net-new verify helper function. Should I turn off IgnoreTlog for all the other verify helpers as well?

Ah yes, good point about having to update everything. I was thinking that TlogUpload is true by default, but that's only for the flag, not the CLI struct. Let's get this merged in then as-is and then we can circle back to improving e2e test coverage for Rekor verification later. Does that sound good?

Sounds good 👍

cmurphy requested a review from haydentherapper January 26, 2024 00:41

cmurphy changed the title ~~Make e2e tests hermetic~~ Improve E2E tests Jan 26, 2024

haydentherapper reviewed Jan 31, 2024

View reviewed changes

cmurphy mentioned this pull request Feb 6, 2024

Fix docker-compose file sigstore/fulcio#1560

Merged

cmurphy force-pushed the e2e-tests-2 branch from 3f19f4d to 62c0d1a Compare February 6, 2024 19:19

cmurphy changed the title ~~Improve E2E tests~~ Make E2E tests hermetic Feb 6, 2024

cmurphy force-pushed the e2e-tests-2 branch 2 times, most recently from 1e61062 to a488e5a Compare February 6, 2024 20:22

cmurphy requested a review from haydentherapper February 7, 2024 18:14

haydentherapper reviewed Feb 7, 2024

View reviewed changes

cmurphy added 5 commits February 7, 2024 11:19

Fix cleanup in E2E script

ce26f23

Calling trap multiple times replaces the last signal handler rather than appending to it. This change ensures that the most recent trap includes all previous traps so that all cleanups are executed. Signed-off-by: Colleen Murphy <[email protected]>

cmurphy force-pushed the e2e-tests-2 branch from a488e5a to d4243fd Compare February 7, 2024 20:01

cmurphy requested a review from haydentherapper February 7, 2024 20:23

haydentherapper reviewed Feb 7, 2024

View reviewed changes

haydentherapper approved these changes Feb 7, 2024

View reviewed changes

haydentherapper merged commit a0b02b7 into sigstore:main Feb 7, 2024

github-actions bot added this to the v2.3.0 milestone Feb 7, 2024

cmurphy mentioned this pull request Feb 9, 2024

Don't ignore transparency log in tests if possible #3528

Merged

		@@ -1029,7 +1032,7 @@ func TestRekorBundle(t *testing.T) {

		// Make sure offline verification works with bundling
		// use rekor prod since we have hardcoded the public key

Make E2E tests hermetic #3499

Make E2E tests hermetic #3499

Uh oh!

Conversation

cmurphy commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Use local services for verify tests

Set rekor env variable in Go test suite

Move verify tests from shell script to Go suite

Fix cleanup in E2E script

Replace os.Setenv with testing.Setenv in e2e tests

Summary

Release Note

Documentation

Uh oh!

codecov bot commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

haydentherapper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmurphy Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haydentherapper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cmurphy commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading

cmurphy Feb 2, 2024 •

edited

Loading