Creating a signed bundle implicitly signs a .manifest file even when one doesn't exist

[friedrichsenm]

Short description

When running opa build to create a signed bundle against a directory that doesn't include a .manifest file, OPA create a signature over a phantom .manifest file and you won't be able to load or verify the bundle and will get the error:
error: load error: bundle example.tar.gz: file(s) [.manifest] specified in bundle signatures but not found in the target bundle

Steps To Reproduce

(requires PEM private and public keys for signing and verification, respectively)

1. Create a directory dir with a data file and a policy file (say data.json and policies.rego)
2. Create a signed bundle using that directory as the source opa build -b dir -o example.tar.gz --signing-key signing.key
3. Try to verify the bundle opa build -b example.tar.gz --verification-key signing.pub

Expected behavior

I would expect bundle loading/verification to succeed even if there wasn't a .manifest file in the original directory or for a generic .manifest to be created in the .tar.gz file and used in the signing process.

Based on the Bundle File Format link, I would expect that the .manifest should not be included if it isn't present.

Additional context

It appears the problem comes from here:

opa/bundle/bundle.go

Lines 854 to 870 in 643eacd

	// Parse the manifest into a JSON structure;
	// then recursively order the fields of all objects alphabetically and then apply
	// the hash function to result to compute the hash.
	mbs, err := json.Marshal(b.Manifest)
	if err != nil {
	return files, err
	}
	var result map[string]interface{}
	if err := util.Unmarshal(mbs, &result); err != nil {
	return files, err
	}
	bs, err = hash.HashFile(result)
	if err != nil {
	return files, err
	}

[ashutosh-narkar]

The fix could be to only include the manifest if it's non-empty. Something like below should fix this. If you'd like to contribute we'd be happy to help.

if !b.Manifest.Equal(Manifest{}) {
  mbs, err := json.Marshal(b.Manifest)
  if err != nil {
	  return files, err
  }
  
  var result map[string]interface{}
  if err := util.Unmarshal(mbs, &result); err != nil {
	  return files, err
  }
  
  bs, err = hash.HashFile(result)
  if err != nil {
	  return files, err
  }
  
  files = append(files, NewFile(strings.TrimPrefix(ManifestExt, "/"), hex.EncodeToString(bs), defaultHashingAlg))
}

[friedrichsenm]

I'd be happy to take a crack at it.
Looking at this piece in initBundle(), it seems that if you did have a .manifest file that had roots set to just the empty string then maybe it wouldn't be picked up.

opa/compile/compile.go

Lines 343 to 348 in cb6a4c0

	// TODO(tsandall): add support for controlling roots. Either the caller could
	// supply them or the compiler could infer them based on the packages and data
	// contents. The latter would require changes to the loader to preserve the
	// locations where base documents were mounted under data.
	result := &bundle.Bundle{}
	result.Manifest.Init()

I'll have to test it out to see if it causes problems. If it does, the solution might be to create a .manifest file before creating the tarball if one doesn't exist.

[friedrichsenm]

After looking, the bundle doesn't write the .manifest file when the structure is empty. So, my previous concern is not a problem.