fix signature verification for oci hosted bundles

[gitu]

Why the changes in this PR are needed?

Verification of bundles downloaded via OCI fail.

What are the changes in this PR?

Removes double usage of base dir.

[ashutosh-narkar]

@gitu it would be helpful if you could explain why this change is needed. It's not clear from the comment. Thanks.

[gitu]

@ashutosh-narkar sure, when downloading a signed image from oci, the path, where the persisted bundle is stored, gets added to verify the signature.

Here an example with a signed bundle containing a data.json file it it's root, and persistence_directory: /tmp/this-is-a-test:

> {"level":"error","msg":"Bundle load failed: unexpected error file tmp/this-is-a-test/oci/data.json not included in bundle signature","name":"authz","plugin":"bundle","time":"2023-08-08T20:51:39+02:00"}

when downloading the same bundle via oras manually and loading it directly the signature comes out as valid.

the base path gets added here to the verification:

opa/bundle/bundle.go

Lines 544 to 548 in cca8197

	// verify the file content
	if bundle.Type() == SnapshotBundleType && !bundle.Signatures.isEmpty() {
	path := f.Path()
	if r.baseDir != "" {
	path = f.URL()

[ashutosh-narkar]

Thanks for the context. It would good to have some tests to ensure we don't break anything. The base dir would be set on modules iirc so a test or some would be useful.

[gitu]

The only other usage of basedir I could find is when loading the bundle from a folder, even there it only seems to be added for debugging purposes.

opa/loader/loader.go

Lines 253 to 257 in cca8197

	// For bundle directories add the full path in front of module file names
	// to simplify debugging.
	if isDir {
	br.WithBaseDir(path)
	}

The question I know have maybe the verifier is wrong and it might be sensible to remove the adding of the baseDir for getting the right hash from the signature:

opa/bundle/bundle.go

Lines 543 to 549 in cca8197

	// verify the file content
	if bundle.Type() == SnapshotBundleType && !bundle.Signatures.isEmpty() {
	path := f.Path()
	if r.baseDir != "" {
	path = f.URL()
	}

[gitu]

after a bit playing that further, can a signature as such ever exist:

opa/bundle/bundle_test.go

Lines 462 to 470 in cca8197

	func TestReadWithSignaturesWithBaseDir(t *testing.T) {
	signedTokenHS256 := `eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImZvbyJ9.eyJmaWxlcyI6W3sibmFtZSI6ImZvby9iYXIvLm1hbmlmZXN0IiwiaGFzaCI6IjUwN2EyYzM4YTE0NDFkYjU4ZDJjYjg3OTgyYzQyYWE5MWE0MzQyZWY0MjJhNmI1NDJlZGRlYmVlZjZmMDQxMmYiLCJhbGdvcml0aG0iOiJTSEEtMjU2In0seyJuYW1lIjoiZm9vL2Jhci9hL2IvYy9kYXRhLmpzb24iLCJoYXNoIjoiYTYxNWVlYWVlMjFkZTUxNzlkZTA4MGRlOGMzMDUyYzhkYTkwMTEzODQwNmJhNzFjMzhjMDMyODQ1ZjdkNTRmNCIsImFsZ29yaXRobSI6IlNIQS0yNTYifSx7Im5hbWUiOiJmb28vYmFyL2h0dHAvcG9saWN5L3BvbGljeS5yZWdvIiwiaGFzaCI6ImY2NjQ0NjFlMzAzYjM3YzIwYzVlMGJlMjkwMDg4MTY3OGNkZjhlODYwYWE0MzNhNWExNGQ0OTRiYTNjNjY2NDkiLCJhbGdvcml0aG0iOiJTSEEtMjU2In1dLCJpYXQiOjE1OTIyNDgwMjcsImlzcyI6IkpXVFNlcnZpY2UiLCJzY29wZSI6IndyaXRlIn0.qTHkuBDVuT-Zl5pbJdZ6LoJ9eooFOhhpRdCheauDrlA`
	files := [][2]string{
	{"/.manifest", `{"revision": "quickbrownfaux"}`},
	{"/.signatures.json", fmt.Sprintf(`{"signatures": ["%v"]}`, signedTokenHS256)},
	{"/a/b/c/data.json", "[1,2,3]"},
	{"/http/policy/policy.rego", `package example`},
	}

decoded signature:

  {
        "name": "foo/bar/.manifest",
        "hash": "507a2c38a1441db58d2cb87982c42aa91a4342ef422a6b542eddebeef6f0412f",
        "algorithm": "SHA-256"
      },
      {
        "name": "foo/bar/a/b/c/data.json",
        "hash": "a615eeaee21de5179de080de8c3052c8da901138406ba71c38c032845f7d54f4",
        "algorithm": "SHA-256"
      },
      {
        "name": "foo/bar/http/policy/policy.rego",
        "hash": "f664461e303b37c20c5e0be2900881678cdf8e860aa433a5a14d494ba3c66649",
        "algorithm": "SHA-256"
      }
    ],
    "iat": 1592248027,
    "iss": "JWTService",
    "scope": "write"
  }

when reading: https://www.openpolicyagent.org/docs/latest/management-bundles/#signature-format my understanding would be that the filename should be relative to the .signatures.json file see also #6147 that would implement that.

[ashutosh-narkar]

when reading: https://www.openpolicyagent.org/docs/latest/management-bundles/#signature-format my understanding would be that the filename should be relative to the .signatures.json

That is not necessary. They could be absolute paths as well. So #6147 would possibly break existing bundle checks and loading as well. Hence for backwards compatibility let's not change that. If this is a concern for OCI bundles, we should address that in this PR.

[netlify]

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	0506d01
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/64edfc896212840008beb39f
😎 Deploy Preview	https://deploy-preview-6145--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[gitu]

@ashutosh-narkar took me a while to get to write a proper test for only changing the behaviour for the oci downloader please have another look

[ashutosh-narkar]

Wouldn't this break verification for some existing OCI bundles?

[gitu]

As there was no test for it I assume that it didn't work before.

If you want to try that just add revert the line you mentioned and rerun the test.

The test uses a bundle that is signed directly via opa:

opa/download/oci_download_test.go

Line 21 in c66511f

//go:generate go run github.com/open-policy-agent/opa build -b --signing-alg HS256 --signing-key secret testdata/signed_bundle_data --output testdata/signed.tar.gz

I still consider the #6147 the more correct thing to do, although I see that might lead to not backward compatible changes. As my assumption is that I could sign a bundle in one folder for example: /home/userx/bundles/myBundle then copy that to another system into /data/bundles/myBundle and verify the signature without having to move to bundle to a specific folder. Besides that point the other usages of the bundle reader do not seem to us the WithBaseDir option.

opa/download/download.go

Lines 331 to 338 in de896d9

	reader := bundle.NewCustomReader(loader).
	WithMetrics(m).
	WithBundleVerificationConfig(d.bvc).
	WithBundleEtag(etag).
	WithLazyLoadingMode(d.lazyLoadingMode).
	WithBundleName(d.bundleName).
	WithBundlePersistence(d.persist)
	if d.sizeLimitBytes != nil {

or

opa/internal/bundle/utils.go

Lines 101 to 110 in de896d9

	r := bundle.NewCustomReader(bundle.NewTarballLoaderWithBaseURL(f, ""))
	if bvc != nil {
	r = r.WithBundleVerificationConfig(bvc)
	}
	b, err := r.Read()
	if err != nil {
	return nil, err
	}

[ashutosh-narkar]

@DerGut if you have sometime to review these changes especially anything you think that affects current behavior that would be great!

[ashutosh-narkar]

Few nits and comments inline. Thanks for working on this!

[ashutosh-narkar]

What is this file?

[gitu]

my bad, that went lost while rebasing now it should be clear that is just a rename of the existing tar.layer file

[gitu]

Few nits and comments inline. Thanks for working on this!

@ashutosh-narkar thanks for the review, now this small stuff is also still easy to change :D

[ashutosh-narkar]

Thanks @gitu! Can you please squash your commits and we can get this in.

[gitu]

@ashutosh-narkar done, thanks for you review