Support for bundle signature verification #2475

ashutosh-narkar · 2020-06-17T20:09:09Z

These changes add support to verify the signature of a signed bundle.

At a high-level, bundle signature verification involves the following steps:

Verify the JWT signature
Verify the files in the JWT payload exist in the bundle
Verify the file content of the files in bundle match with those in the JWT payload

More details about the changes are included in the commit messages.

patrick-east

Looks pretty good so far! Handful of nits and questions. Looking forward to more of the changes for the CLI tooling so I can play around with it 😄

patrick-east · 2020-06-18T23:37:59Z

bundle/bundle.go

+	var value interface{}
+	if isStructuredDoc(path) {
+		err := util.Unmarshal(data.Bytes(), &value)
+		if err != nil {
+			return err
+		}
+	} else {
+		value = data.Bytes()
+	}


This is interesting, and maybe just a detail I missed in the design doc. Why aren't we using the hash of the source file for structured docs?

Added details in the docs and also a comment in the code. lmk if it clarifies this.

The doc updates look good! That helps a lot 👍

patrick-east · 2020-06-18T23:41:42Z

bundle/bundle.go

+		var buf bytes.Buffer
+		buf.Write(payload)
+
+		var jpl JWTPayload
+		if err := util.NewJSONDecoder(&buf).Decode(&jpl); err != nil {
+			return err
+		}


nit: Can use util.UnmarshalJSON and simplify some of this

Updated to use util.UnmarshalJSON

patrick-east · 2020-06-18T23:45:51Z

bundle/bundle_test.go

+		})
+	}
+
+	// add files to the bundle and reader


nit: Maybe split up this test into the ones using the list of cases for error checking and the case below here?

Separated into 2 tests.

patrick-east · 2020-06-18T23:52:51Z

plugins/bundle/config.go

+	Name    string                  `json:"name"`    // Deprecated: Use `Bundles` map instead
+	Service string                  `json:"service"` // Deprecated: Use `Bundles` map instead
+	Prefix  *string                 `json:"prefix"`  // Deprecated: Use `Bundles` map instead
+	Signing *bdl.VerificationConfig `json:"signing"` // Deprecated: Use `Bundles` map instead


IMO we shouldn't add it here. If someone wants to use the new feature they should update to using the non-deprecated bundles config

I agree too. Removed the signing config from the deprecated config.

patrick-east · 2020-06-18T23:59:03Z

plugins/plugins.go

@@ -122,6 +123,7 @@ type Manager struct {
 	Config *config.Config
 	Info   *ast.Term
 	ID     string
+	Keys   map[string]*bdl.KeyConfig


We probably shouldn't have public exported fields using types that are internal. I think moving the config structs for the keys/signing stuff out of the internal package is OK.

Unexported keys

We still have a handful of places where we have usage of the internal types as parameters to exported methods, for example looking at:

func (r *Reader) WithBundleVerificationConfig(config *bdl.VerificationConfig) *Reader {

If I was writing a golang tool that wanted to load and verify a bundle, how could I use this API? I don't have a way to make a config to pass into this.

Please take a look at either making those types public (just merge it into the public bundles package) or double check all the API's where they are required to ensure they are usable from the outside.

Moved the code in internal/bundle to the public bundle package.

tsandall · 2020-06-23T19:56:46Z

This looks great! A few thoughts/actions that came out of manual testing.

I was expecting opa sign to accept a set of path arguments. Let's remove --data and make those arguments positional and add the --bundle option. This will align with opa build.
It doesn't look like build and sign work together yet. E.g., I ran sign and then build but the latter did not produce a bundle containing the signature file. We should make them work together.
I was expecting opa run bundle.tar.gz to just work on a signed bundle but that's not the case. Let's add a --skip-verify flag so that users can easily opt-out of verification if needed (e.g., suppose you don't have the cert to verify the signatures with…)
Bundle verification error logs stutter a bit:

Bundle download failed: bundle signature verification failed: bundle reader not provided with signature verification config

Maybe this instead?

Bundle download failed: reader missing signature verification config (hint: check bundle configuration includes key(s))

EDIT: re: comment above about making opa sign and opa build work together.

The build command is going to be recommended tool to use for packaging policies for distribution. Given this, we want to make sure that features (like signing) are compatible with it. One of the core features that build provides is optimization. Currently the build command has optimizations disabled by default, however, in the future, hopefully, we'll be be comfortable with having them enabled by default. This means that we have to assume that build may modify the source files (rego or json). Since the build command may modify the source files it's in conflict with the opa sign command which computes hashes based on source file contents. To workaround this we can do the following:

Update the opa build command so that (by default) if .signatures.json is present the user MUST provide verification AND signing config. The opa build command will use the verification config (keys) to check the signatures on the files and it will use the signing config to write out an updated .signatures.json file to the bundle. This way callers can (1) trust that the files passed as input to opa build are intact and (2) ensure that the output from opa build can be trusted. If we don't do (2) then users could accidentally run opa sign and then opa build and think that somehow the signatures are intact.
Update the opa build command so if .signatures.json is NOT present, the user MAY (optionally) provide signing config.

patrick-east · 2020-06-23T20:34:21Z

👍 This is really cool, finished playing around with it and have a few random notes. Lots of it is just docs or tweaks to the help output.

Random notes (in no particular order):

Help note for opa sign could maybe be more succinct?

An open source project to policy-enable your service.

Usage:
  opa [command]

Available Commands:
  bench       Benchmark a Rego query
  build       Build an OPA bundle
  check       Check Rego source files
  deps        Analyze Rego query dependencies
  eval        Evaluate a Rego query
  fmt         Format Rego source files
  help        Help about any command
  parse       Parse Rego source file
  run         Start OPA in interactive or server mode
  sign        Generate ".signatures.json" file for bundle signature verification
  test        Execute Rego test cases
  version     Print the version of OPA

It stands out from the others in verbosity. Maybe just saying like: "Sign Rego bundle files"

For the error cases where I left out required fields we could provide better usage messaging, ex:

{12:58} ~/p/g/s/g/o/opa:ashutosh-narkar-bundle-signing ✓ ❯ go run . sign
error: specify atleast one directory containing policy and data files
exit status 1

but for eval we give more useful information:

{13:01} ~/p/g/s/g/o/opa:ashutosh-narkar-bundle-signing ✓ ❯ go run . eval
Error: specify query argument or --stdin
Usage:
  opa eval <query> [flags]

Flags:
  -b, --bundle string                                 set bundle file(s) or directory path(s)
      --coverage                                      report coverage
  -d, --data string                                   set data file(s) or directory path(s)
      --disable-indexing                              disable indexing optimizations
      --disable-inlining stringArray                  set paths of documents to exclude from inlining
      --explain {off,full,notes,fails}                enable query explanations (default off)
      --fail                                          exits with non-zero exit code on undefined/empty result and errors
      --fail-defined                                  exits with non-zero exit code on defined/non-empty result and errors
  -f, --format {json,values,bindings,pretty,source}   set output format (default json)
  -h, --help                                          help for eval
      --ignore strings                                set file and directory names to ignore during loading (e.g., '.*' excludes hidden files)
      --import string                                 set query import(s)
  -i, --input string                                  set input file path
      --instrument                                    enable query instrumentation metrics (implies --metrics)
      --metrics                                       report query performance metrics
      --package string                                set query package
  -p, --partial                                       perform partial evaluation
      --pretty-limit int                              set limit after which pretty output gets truncated (default 80)
      --profile                                       perform expression profiling
      --profile-limit int                             set number of profiling results to show (default 10)
      --profile-sort string                           set sort order of expression profiler results
      --shallow-inlining                              disable inlining of rules that depend on unknowns
      --stdin                                         read query from stdin
  -I, --stdin-input                                   read input document from stdin
  -u, --unknowns stringArray                          set paths to treat as unknown during partial evaluation (default [input])

specify query argument or --stdin
exit status 1

Since we have such a large help text for opa sign maybe we want to split up the like shorter "usage" section from the longer description? Like one for short "you did it wrong" errors, and a longer version for the --help output.

* Should we support a -b/--bundle flag for sign? Or make the (required) paths as. We have tried to push the "bundle" style options on the other CLI's, seems like it would be nice to do it here too.

* Any reason we don't support a list of paths mirroring the opa build syntax? If there must always be at least one path then it would have a very similar look and feel. I think I'd prefer it to be closer to opa build and opa run than opa eval with regards to passing in those paths.
Edit: Torin beat me to these last two 😄

The help text doesn't mention that you must provide either a key or hmac secret, we should maybe specify that somewhere. I don't think it specifies that a -d path is required either.
The help text doesn't mention what algorithms are available, only which one is the default. We should have a list in the help text (or a pointer to one)
Sign should maybe exclude the signature file (or maybe only if it is going to overwrite it?). As-is running opa sign on the same bundle source is not idempotent:

{13:24} ~/p/g/s/g/o/opa:ashutosh-narkar-bundle-signing ✓ ❯ go run . sign -d /tmp/my-bundle -o /tmp/my-bundle/ -k lol --crypto-alg HS256
{13:25} ~/p/g/s/g/o/opa:ashutosh-narkar-bundle-signing ✓ ❯ cat /tmp/my-bundle/.signatures.json
{
 "signatures": [
  "eyJhbGciOiJIUzI1NiJ9.eyJmaWxlcyI6W3sibmFtZSI6InRtcC9teS1idW5kbGUvYXV0aHovYWxsb3cucmVnbyIsImhhc2giOiJhNDI3MTI5YWU4OTgwZGVhMjNlOWY4NTZjODM1YjAyM2EzMTU2MzFmMTIwZGQyZDY3MjI0NzFlYWMyYzJjZjVjIiwiYWxnb3JpdGhtIjoiU0hBMjU2In0seyJuYW1lIjoidG1wL215LWJ1bmRsZS9hdXRoei9kYXRhLmpzb24iLCJoYXNoIjoiYzRlZTEzOGVjOTI3YTFmYjQ1NjU1ZTU4MTgyNTJjZjdmZjkxMzA2ZjcxMjBlYWI5YTBmM2U5NzJlMzlkNGQ5YiIsImFsZ29yaXRobSI6IlNIQTI1NiJ9LHsibmFtZSI6InRtcC9teS1idW5kbGUvZGF0YS5qc29uIiwiaGFzaCI6ImM0ZWUxMzhlYzkyN2ExZmI0NTY1NWU1ODE4MjUyY2Y3ZmY5MTMwNmY3MTIwZWFiOWEwZjNlOTcyZTM5ZDRkOWIiLCJhbGdvcml0aG0iOiJTSEEyNTYifSx7Im5hbWUiOiJ0bXAvbXktYnVuZGxlL21haW4ucmVnbyIsImhhc2giOiI5ZjNjYjUxZjgzMTJmN2YwZTQ0OWExNTA3NzRiYmM0MTc5NzcwYTFjZmM2YmQ5MTg1YjdiZjc5MDAxMDA1NzZhIiwiYWxnb3JpdGhtIjoiU0hBMjU2In0seyJuYW1lIjoidG1wL215LWJ1bmRsZS94L3kvZGF0YS5qc29uIiwiaGFzaCI6ImRhOGQxNDFlOTEwN2M1MTFkNmJmMTA4NmIyMmUwNTBhNGJlMGMwMzA0YTYwYTExYzU1NmUxNGU1ZTZiMWU1YzkiLCJhbGdvcml0aG0iOiJTSEEyNTYifV19.PfbdO_Ju2w49AGyUjVD9VZCakAnKnEwwCIwhQ8B7xdE"
 ]
}
{13:25} ~/p/g/s/g/o/opa:ashutosh-narkar-bundle-signing ✓ ❯ go run . sign -d /tmp/my-bundle -o /tmp/my-bundle/ -k lol --crypto-alg HS256
{13:25} ~/p/g/s/g/o/opa:ashutosh-narkar-bundle-signing ✓ ❯ cat /tmp/my-bundle/.signatures.json
{
 "signatures": [
  "eyJhbGciOiJIUzI1NiJ9.eyJmaWxlcyI6W3sibmFtZSI6InRtcC9teS1idW5kbGUvLnNpZ25hdHVyZXMuanNvbiIsImhhc2giOiIzMmI0N2FhZWYxM2E4Mzc3MTNhZWFkODI4ODI5NDJiZGE4ZjEzNzZjYmYzNzk0NmEwMDZjMTNhOTM2Mjg4ZWEwIiwiYWxnb3JpdGhtIjoiU0hBMjU2In0seyJuYW1lIjoidG1wL215LWJ1bmRsZS9hdXRoei9hbGxvdy5yZWdvIiwiaGFzaCI6ImE0MjcxMjlhZTg5ODBkZWEyM2U5Zjg1NmM4MzViMDIzYTMxNTYzMWYxMjBkZDJkNjcyMjQ3MWVhYzJjMmNmNWMiLCJhbGdvcml0aG0iOiJTSEEyNTYifSx7Im5hbWUiOiJ0bXAvbXktYnVuZGxlL2F1dGh6L2RhdGEuanNvbiIsImhhc2giOiJjNGVlMTM4ZWM5MjdhMWZiNDU2NTVlNTgxODI1MmNmN2ZmOTEzMDZmNzEyMGVhYjlhMGYzZTk3MmUzOWQ0ZDliIiwiYWxnb3JpdGhtIjoiU0hBMjU2In0seyJuYW1lIjoidG1wL215LWJ1bmRsZS9kYXRhLmpzb24iLCJoYXNoIjoiYzRlZTEzOGVjOTI3YTFmYjQ1NjU1ZTU4MTgyNTJjZjdmZjkxMzA2ZjcxMjBlYWI5YTBmM2U5NzJlMzlkNGQ5YiIsImFsZ29yaXRobSI6IlNIQTI1NiJ9LHsibmFtZSI6InRtcC9teS1idW5kbGUvbWFpbi5yZWdvIiwiaGFzaCI6IjlmM2NiNTFmODMxMmY3ZjBlNDQ5YTE1MDc3NGJiYzQxNzk3NzBhMWNmYzZiZDkxODViN2JmNzkwMDEwMDU3NmEiLCJhbGdvcml0aG0iOiJTSEEyNTYifSx7Im5hbWUiOiJ0bXAvbXktYnVuZGxlL3gveS9kYXRhLmpzb24iLCJoYXNoIjoiZGE4ZDE0MWU5MTA3YzUxMWQ2YmYxMDg2YjIyZTA1MGE0YmUwYzAzMDRhNjBhMTFjNTU2ZTE0ZTVlNmIxZTVjOSIsImFsZ29yaXRobSI6IlNIQTI1NiJ9XX0.VDxMg_XOGrJVYJ3vTCsYorVzbyHQQiozh57TSdGEBF0"
 ]
}
{13:25} ~/p/g/s/g/o/opa:ashutosh-narkar-bundle-signing ✓ ❯ go run . sign -d /tmp/my-bundle -o /tmp/my-bundle/ -k lol --crypto-alg HS256
{13:25} ~/p/g/s/g/o/opa:ashutosh-narkar-bundle-signing ✓ ❯ cat /tmp/my-bundle/.signatures.json
{
 "signatures": [
  "eyJhbGciOiJIUzI1NiJ9.eyJmaWxlcyI6W3sibmFtZSI6InRtcC9teS1idW5kbGUvLnNpZ25hdHVyZXMuanNvbiIsImhhc2giOiI1ZmY3MmUyYTU4MDllYjI1NjZmYTFmMjdkZmRhYjYyNjE5MDc1ZDQ4NTY3ZDdlMTQxMjJiNDQ5YjJkNDM5NDhjIiwiYWxnb3JpdGhtIjoiU0hBMjU2In0seyJuYW1lIjoidG1wL215LWJ1bmRsZS9hdXRoei9hbGxvdy5yZWdvIiwiaGFzaCI6ImE0MjcxMjlhZTg5ODBkZWEyM2U5Zjg1NmM4MzViMDIzYTMxNTYzMWYxMjBkZDJkNjcyMjQ3MWVhYzJjMmNmNWMiLCJhbGdvcml0aG0iOiJTSEEyNTYifSx7Im5hbWUiOiJ0bXAvbXktYnVuZGxlL2F1dGh6L2RhdGEuanNvbiIsImhhc2giOiJjNGVlMTM4ZWM5MjdhMWZiNDU2NTVlNTgxODI1MmNmN2ZmOTEzMDZmNzEyMGVhYjlhMGYzZTk3MmUzOWQ0ZDliIiwiYWxnb3JpdGhtIjoiU0hBMjU2In0seyJuYW1lIjoidG1wL215LWJ1bmRsZS9kYXRhLmpzb24iLCJoYXNoIjoiYzRlZTEzOGVjOTI3YTFmYjQ1NjU1ZTU4MTgyNTJjZjdmZjkxMzA2ZjcxMjBlYWI5YTBmM2U5NzJlMzlkNGQ5YiIsImFsZ29yaXRobSI6IlNIQTI1NiJ9LHsibmFtZSI6InRtcC9teS1idW5kbGUvbWFpbi5yZWdvIiwiaGFzaCI6IjlmM2NiNTFmODMxMmY3ZjBlNDQ5YTE1MDc3NGJiYzQxNzk3NzBhMWNmYzZiZDkxODViN2JmNzkwMDEwMDU3NmEiLCJhbGdvcml0aG0iOiJTSEEyNTYifSx7Im5hbWUiOiJ0bXAvbXktYnVuZGxlL3gveS9kYXRhLmpzb24iLCJoYXNoIjoiZGE4ZDE0MWU5MTA3YzUxMWQ2YmYxMDg2YjIyZTA1MGE0YmUwYzAzMDRhNjBhMTFjNTU2ZTE0ZTVlNmIxZTVjOSIsImFsZ29yaXRobSI6IlNIQTI1NiJ9XX0.5fReW1GDZHkNgbQ8JqYi3MFoLylSxuHUsZEyFoktYsY"
 ]
}

I would have expected the signatures to not be changing without any changes to the bundle source files. Looking at the output I see that the subsequent "sign" operations are including the signature file:

{
  "files": [
    {
      "name": "tmp/my-bundle/.signatures.json",
      "hash": "5ff72e2a5809eb2566fa1f27dfdab62619075d48567d7e14122b449b2d43948c",
      "algorithm": "SHA256"
    },

..

patrick-east · 2020-06-23T20:40:33Z

My last comment there reminded me too that we should document somewhere how the paths are specified, for example in my example for the idempotency issue I was specifying -d /tmp/my-bundle/ and the signatures were using paths like:

{
  "files": [
    {
      "name": "tmp/my-bundle/authz/allow.rego",
      "hash": "a427129ae8980dea23e9f856c835b023a315631f120dd2d6722471eac2c2cf5c",
      "algorithm": "SHA256"
    },
    {
      "name": "tmp/my-bundle/authz/data.json",
      "hash": "c4ee138ec927a1fb45655e5818252cf7ff91306f7120eab9a0f3e972e39d4d9b",
      "algorithm": "SHA256"
    },
    {
      "name": "tmp/my-bundle/data.json",
      "hash": "c4ee138ec927a1fb45655e5818252cf7ff91306f7120eab9a0f3e972e39d4d9b",
      "algorithm": "SHA256"
    },
    {
      "name": "tmp/my-bundle/main.rego",
      "hash": "9f3cb51f8312f7f0e449a150774bbc4179770a1cfc6bd9185b7bf7900100576a",
      "algorithm": "SHA256"
    },
    {
      "name": "tmp/my-bundle/x/y/data.json",
      "hash": "da8d141e9107c511d6bf1086b22e050a4be0c0304a60a11c556e14e5e6b1e5c9",
      "algorithm": "SHA256"
    }
  ]
}

But if I were to do a opa build with a path to /tmp/my-bundle I would expect to see relative paths inside the bundle from that directory, eg I would have wanted:

{
  "files": [
    {
      "name": "authz/allow.rego",
      "hash": "a427129ae8980dea23e9f856c835b023a315631f120dd2d6722471eac2c2cf5c",
      "algorithm": "SHA256"
    },
    {
      "name": "authz/data.json",
      "hash": "c4ee138ec927a1fb45655e5818252cf7ff91306f7120eab9a0f3e972e39d4d9b",
      "algorithm": "SHA256"
    },
    {
      "name": "data.json",
      "hash": "c4ee138ec927a1fb45655e5818252cf7ff91306f7120eab9a0f3e972e39d4d9b",
      "algorithm": "SHA256"
    },
    {
      "name": "main.rego",
      "hash": "9f3cb51f8312f7f0e449a150774bbc4179770a1cfc6bd9185b7bf7900100576a",
      "algorithm": "SHA256"
    },
    {
      "name": "x/y/data.json",
      "hash": "da8d141e9107c511d6bf1086b22e050a4be0c0304a60a11c556e14e5e6b1e5c9",
      "algorithm": "SHA256"
    }
  ]
}

tsandall

Reviewed the implementation. Looks great. Mostly just nits.

bundle/bundle.go

plugins/discovery/config.go

tsandall · 2020-06-24T14:53:42Z

bundle/sign.go

+// Use of this source code is governed by an Apache2
+// license that can be found in the LICENSE file.
+
+// Package bundle defines structures that contain information required during the bundle signature verification process


This seems out-of-sync since thes files are in the bunde package. I could imagine splitting this into it's own top-level package (keys). This would also be nice if we end up re-using keys for something other than bundles in the future. @patrick-east WDYT?

tsandall · 2020-06-24T14:58:31Z

plugins/discovery/config_test.go

+			wantErr:  false,
+		},
+		{
+			input:    `{"name": "a/b/c", "decision": "query", "signing": {"keyid": "bar", "scope": "write"}}}`,


what happens if the key configuration that discovery depends on changes?

We can add a check to see if the key that discovery depends on has changed like we do for services.

The keyid could be included in the bundle signature, which we could read from the bundle itself. Since we only support one signature in the bundle, this would work.

If the keyid was not provided in the signature, we could pick it up from discovery.signing.

WDYT ?

Yep, except per the offline discussion yesterday, we should make the config keyid override the bundle keyid.

Added a check to avoid updates to keys in the boot config.

cmd/sign.go

tsandall · 2020-06-24T15:00:19Z

cmd/sign.go

+	return writeTokenToFile(string(token), params.outputFilePath)
+}
+
+func generatePayload(dataPaths []string, claimsFile string) ([]byte, error) {


Let's figure out how to avoid this. We don't want to duplicate this code. We'll likely need to improve/refactor the loader package.

koponen-styra · 2020-06-24T16:31:07Z

bundle/hash.go

+// object: Hash {, then each key (in alphabetical order) and digest of the value, and finally }.
+//
+// array: Hash [, then digest of the value, and finally ].
+func (h *hasher) file(v interface{}) ([]byte, error) {


Per the offline discussion, we can align this algorithm to match the hashing of raw (ordered) JSON file by making the actual digested content to match the bytes matched in that case. In other words, need to remove a) type strings from hashed, b) add commas to containers, c) consider the primitive types' hashing details.

Updated the algorithm.

koponen-styra · 2020-06-24T17:13:02Z

bundle/hash_test.go

+		"map_unequal": {`{"a": {"foo": [1, 2, 3]}, "b": "bar"}`, `{"b": "bars", "a": {"foo": [1, 2, 3]}}`, SHA384, false},
+		"array":       {`[1, 2, 3]`, `[1, 2, 3]`, SHA512, true},
+		"null":        {`null`, `null`, SHA512224, true},
+		"bool":        {`false`, `false`, SHA512256, true},


I'd think you can transform this into test that checks the hash computed directly from the raw bytes equals to the one the alg computes. Then a few more corner cases to cover in the types of docs hashed: []byte, string with unicode characters, string that could HTML escaped (but won't).

Updated with more tests.

koponen-styra · 2020-06-24T22:42:54Z

bundle/hash.go

+// Supported values for HashingAlgorithm
+const (
+	MD5       HashingAlgorithm = "MD5"
+	SHA1      HashingAlgorithm = "SHA1"


nit: I think the convention is to have dash between "SHA" and the number of bits (per RFCs at least). Once you do that, then perhaps change the underscores to dashes too, for consistency.

ashutosh-narkar · 2020-06-25T18:49:51Z

Some updates to opa sign:

$ opa --help
An open source project to policy-enable your service.

Usage:
  opa [command]

Available Commands:
  bench       Benchmark a Rego query
  build       Build an OPA bundle
  check       Check Rego source files
  deps        Analyze Rego query dependencies
  eval        Evaluate a Rego query
  fmt         Format Rego source files
  help        Help about any command
  parse       Parse Rego source file
  run         Start OPA in interactive or server mode
  sign        Sign OPA bundle files
  test        Execute Rego test cases
  version     Print the version of OPA

Flags:
  -h, --help   help for opa

Use "opa [command] --help" for more information about a command.

$ opa sign
Error: specify atleast one path containing policy and/or data files
Usage:
  opa sign <path> [<path> [...]] [flags]

Flags:
      --claims-file string        set path of JSON file containing optional claims (see: https://openpolicyagent.org/docs/latest/management/#bundle-signature-format)
      --crypto-alg string         name of the signing algorithm (default "RS256")
  -h, --help                      help for sign
  -k, --key string                set the secret (HMAC) or path of the PEM file containing the private key (RSA and ECDSA)
  -o, --output-file-path string   set the location for the .signatures.json file (default ".")

specify atleast one path containing policy and/or data files

$ opa sign .
Error: specify the secret (HMAC) or path of the PEM file containing the private key (RSA and ECDSA)
Usage:
  opa sign <path> [<path> [...]] [flags]

Flags:
      --claims-file string        set path of JSON file containing optional claims (see: https://openpolicyagent.org/docs/latest/management/#bundle-signature-format)
      --crypto-alg string         name of the signing algorithm (default "RS256")
  -h, --help                      help for sign
  -k, --key string                set the secret (HMAC) or path of the PEM file containing the private key (RSA and ECDSA)
  -o, --output-file-path string   set the location for the .signatures.json file (default ".")

specify the secret (HMAC) or path of the PEM file containing the private key (RSA and ECDSA)

koponen-styra · 2020-06-25T20:38:23Z

cmd/flags.go

@@ -85,6 +85,10 @@ func addIgnoreFlag(fs *pflag.FlagSet, ignoreNames *[]string) {
 	fs.StringSliceVarP(ignoreNames, "ignore", "", []string{}, "set file and directory names to ignore during loading (e.g., '.*' excludes hidden files)")
 }

+func addSigningAlgFlag(fs *pflag.FlagSet, alg *string, value string) {
+	fs.StringVarP(alg, "crypto-alg", "", value, "name of the signing algorithm")


crypto is rather meaningless term here. how about --signature-alg?

Renamed to --signing-alg to match the description.

tsandall · 2020-06-26T15:17:35Z

bundle/bundle.go

+func (r *Reader) checkSignaturesAndDescriptors(signatures *SignaturesConfig, descriptors []*Descriptor) error {
+	if signatures == nil && r.verificationConfig != nil {
+		msg := "bundle reader provided with signature verification config but .signatures.json file not included in the bundle"
+		return fmt.Errorf("%v", msg)


nit: call errors.New("...") or fmt.Errorf("..."); not need for the intermediate variable.

tsandall · 2020-06-26T15:17:47Z

bundle/bundle.go

+
+	if signatures != nil {
+		if r.verificationConfig == nil {
+			msg := "reader missing signature verification config (hint: check opa configuration includes key(s))"


nit: same as previous (use errors.New)

tsandall · 2020-06-26T15:22:34Z

bundle/bundle.go

+		}
+
+		// check that number of files in the bundle signatures is equal to number of files in the bundle
+		if len(r.files) != len(descriptors) {


I think it would be better to report (1) missing and (2) extra files in the bundle. Currently we're reporting the # of files but that information is not very useful from the user's perspective (e.g., what are they going to do if they see that 7 != 6? We can just perform a diff and return something much more useful.)

+1, Saying which files were missing or extra would be really nice for the error message.

Updated error message format: file(s) [a/b/c/data.json http/policy/policy.rego] specified in bundle signatures but not found in the target bundle

tsandall · 2020-06-26T15:25:49Z

plugins/bundle/config.go

+// ParseBundlesConfigWithSigning validates the config and injects default values for
+// the defined `bundles`. This expects a map of bundle names to resource
+// configurations and public keys to verify a signed bundle.
+func ParseBundlesConfigWithSigning(config []byte, services []string, keys map[string]*bundle.KeyConfig) (*Config, error) {


One of the nice things about using the builder pattern is that you don't have to add variations every time the API needs to be extended. For example:

type ConfigBuilder struct { raw []byte services []string keys map[string]*bundle.KeyConfig } func NewConfigBuilder() *ConfigBuilder { ... } func (b *ConfigBuilder) WithBytes([]byte) *ConfigBuilder {...} func (b *ConfigBuilder) WithServices([]string) *ConfigBuilder {...} func (b *ConfigBuilder) WithKeyConfigs(map[string]*bundle.KeyConfig) {...} func (b *ConfigBuilder) Parse() (*Config, error)

Example usage:

config, err := bundle.NewConfigBuilder(). WithBytes(bs) WithServices(services). WithKeyConfigs(keys). Parse()

Updated the bundle and discovery config code to use builder pattern.

patrick-east · 2020-07-01T20:20:09Z

Notes (in no particular order):

The help docs are great, I have one super nit-picky nit: can we adjust the line width to be consistent with the older help text? Ex:

Note that the newer signing stuff uses a wider line boundary than the rest of it.
Similar nit for the opa run help text
For OPA run we should maybe rename --pubkey or --pub-key-id to like --bundle-key or something? It is particularly confusing just looking at the short description of the flags to know what they do:

--pubkey string                        set the secret (HMAC) or path of the PEM file containing the public key (RSA and ECDSA)

If its a HMAC secret it shouldn't be public, so it seems like we would want to maybe name the flag something to indicate as much. Calling it --bundle-key or something helps make it more explanatory that its the key for the bundle and that it may not be literally a public key in the RSA sense.

For the error message on:

{12:47} ~/scratch ❯ opa build ./bundle
error: missing configuration to verify bundle signature

We should maybe clarify that it is unable to verify the signed bundle source because it was missing the config, not just that it was missing configuration.

For opa build and opa sign we might want to change --key to like --signing-key or something. Its not really clear from the flag description what key it is or how its used:

--key string           set the secret (HMAC) or path of the PEM file containing the private key (RSA and ECDSA)

It's way more obvious for opa sign but for opa build it gets harder as we have a verification key and a signing key. Its harder to tell them apart. For consistency between the commands however we might want to rename both.

There seems to be an issue with loading the .manifest when signing in the current directory:

{13:00} ~/s/bundle ❯ find .
.
./main.rego
./data.yaml
./authz
./authz/util
./authz/util/helpers.rego
./authz/allow.rego
./external
./external/data.json
./.manifest
{13:00} ~/s/bundle ❯ opa sign --signing-alg HS256 --key k1 -b .
{13:01} ~/s/bundle ❯ find .
.
./.signatures.json
./main.rego
./data.yaml
./authz
./authz/util
./authz/util/helpers.rego
./authz/allow.rego
./external
./external/data.json
./.manifest
{13:01} ~/s/bundle ❯ opa run --signing-alg HS256 --pubkey k1 .
error: load error: 1 error occurred during loading: file .manifest not included in bundle signature

^ I didn't expect to see an error doing that

Whats the plan for eval, bench, check, test, etc? As-is they error out for any signed bundle saying more config is required but that cannot be supplied. If we want to merge without the flags added and plumbed through maybe we should catch this error and give a better "not implemented" sort of error?

{13:05} ✘ ~/scratch ❯ opa eval -b ./bundle 'data'
{
  "errors": [
    {
      "message": "loading error: bundle ./bundle: reader missing signature verification config (hint: check opa configuration includes key(s))"
    }
  ]
}

patrick-east · 2020-07-01T20:22:41Z

bundle/bundle.go

+		}
+
+		// check that number of files in the bundle signatures is equal to number of files in the bundle
+		if len(r.files) != len(descriptors) {


+1, Saying which files were missing or extra would be really nice for the error message.

patrick-east · 2020-07-01T20:34:13Z

bundle/keys.go

+// Use of this source code is governed by an Apache2
+// license that can be found in the LICENSE file.
+
+// Package bundle defines structures that contain information required during the bundle signature verification process


This description seems a little off for bundle, is this remnants from when it was separate?

Updated the description.

What happens if you have multiple package level godoc comments? At this point, we should probably just create a separate doc.go file and put the package level comment in there that explains what the overall bundle package does.

patrick-east · 2020-07-01T20:45:10Z

docs/content/configuration.md

+The following signing algorithms are supported:
+
+	ES256       "ES256" // ECDSA using P-256 and SHA-256
+	ES384       "ES384" // ECDSA using P-384 and SHA-384
+	ES512       "ES512" // ECDSA using P-521 and SHA-512
+	HS256       "HS256" // HMAC using SHA-256
+	HS384       "HS384" // HMAC using SHA-384
+	HS512       "HS512" // HMAC using SHA-512
+	PS256       "PS256" // RSASSA-PSS using SHA256 and MGF1-SHA256
+	PS384       "PS384" // RSASSA-PSS using SHA384 and MGF1-SHA384
+	PS512       "PS512" // RSASSA-PSS using SHA512 and MGF1-SHA512
+	RS256       "RS256" // RSASSA-PKCS-v1.5 using SHA-256
+	RS384       "RS384" // RSASSA-PKCS-v1.5 using SHA-384
+	RS512       "RS512" // RSASSA-PKCS-v1.5 using SHA-512


Can we make this into a table like:

Name Description

ES256 ECDSA using P-256 and SHA-256

... ...

or something? Its a little more work to maintain but I think helps make the docs a bit easier to understand. As-is its unclear what the ES256 "ES256" means, like do I need quotes? no quotes? which one goes into the --signing-alg parameter?

Updated to a table.

patrick-east · 2020-07-01T21:05:26Z

loader/loader.go

+	if signatures != nil {
+		root.Signatures = signatures
+
+		if fl.bvc == nil {
+			return nil, fmt.Errorf("missing configuration to verify bundle signature")
+		}


What happens if someone specifies a directory with >1 bundle in it? Say I had:

./bundles/b1 ./bundles/b1/.signatures.json ./bundles/b1/policy.rego ./bundles/b2 ./bundles/b2/.signatures.json ./bundles/b1/other-policy.rego

And I do something like opa run ./bundles IIRC both bundles get handled by a single call to Filtered(). If I'm reading this correctly we would only verify the last bundle signature file we found, right?

We could potentially say if someone wants bundle verification to happen they need to use the -b option and specify them as bundles. Not sure how others feel about that.. but we did want to push more people to using that. I'd almost prefer having this error out saying "found a signed bundle, use --bundle instead" rather than potentially giving wrong results or having a ton of complexity in the --data loading path.

Just kind of brainstorming here.. if we do go the "unsupported" path we could iterate later on to add support as needed.

What happens if someone specifies a directory with >1 bundle in it? Say I had:

With the current implementation, I'd expect an error since we do not support multiple JWTs. OTOH, if there was only a single signature file then it would probably error because the signature file would not reference the files in othe other bundle (if it did, things may just work but that seems like a very strange edge case.)

I'd expect an error since we do not support multiple JWTs

Not sure I follow, if we load those files as if they are bundles, eg:

./bundles/b1 ./bundles/b1/.signatures.json ./bundles/b1/policy.rego ./bundles/b2 ./bundles/b2/.signatures.json ./bundles/b1/other-policy.rego

should be loaded the same as

./bundles/b1.tar.gz ./bundles/b2.tar.gz

if I did opa run ./bundles. Each bundle still has a single JWT that should verify its content. I guess the question is whether or not we want the non-bundle-mode loading option to even try to handle these or if we say a user has to specify opa run -b ./bundles/b1 ./bundles/b2 or opa run -b ./bundles/b1.tar.gz ./bundles/b2.tar.gz instead if they want validation.

Not sure I follow, if we load those files as if they are bundles, eg:

If we're passing multiple paths and interpreting them as bundles (e.g., -b was given) then yes, but I thought the point was that one of those was not happening.

With the rest of this PR it looks like we're erring on the side of security; if the user does opa run . without -b and the filesystem contains a signatures.json file, it will be honored. This makes sense to some extent though it does depart from the rest of the file loading conventions.

Thinking about this a bit more, I'm a bit surprised we pickup .signatures.json files under b1 and b2 when the load path is . or bundles/. I was under the impression the signatures.json file could only exist at the root of the bundle. This doesn't matter too much though since users could also do opa run ./bundles/b1 ./bundles/b2 (without -b) and in that case the signatures files would get honored.

Closing the loop on the offline discussion. The plan is to leave non-bundle-mode loading alone for now. OPA will not verify signatures unless bundle mode is enabled. This simplifies the implementation, removes room for error, and avoids the backwards incompatibility issue of making certain files have meaning when non-bundle-mode loading is in use.

Done. Bundle signing and verification is supported only in the bundle mode.

ashutosh-narkar · 2020-07-06T07:10:25Z

Few Updates:

--key --> --signing-key
--pubkey --> --verification-key
Help text for sign, build and run updated to be of consistent width with existing text.
Updated error message in loader: error: missing configuration to verify bundle signature --> unable to verify signature of signed bundle (hint: check --verification-key flag to provide signature verification config)
Resolved issue with loading the .manifest when signing in the current directory. opa sign --signing-alg HS256 --key k1 -b . with opa run --signing-alg HS256 --pubkey k1 . should work now.

Notes (in no particular order):

The help docs are great, I have one super nit-picky nit: can we adjust the line width to be consistent with the older help text? Ex:

Note that the newer signing stuff uses a wider line boundary than the rest of it.

Similar nit for the opa run help text

For OPA run we should maybe rename --pubkey or --pub-key-id to like --bundle-key or something? It is particularly confusing just looking at the short description of the flags to know what they do:
--pubkey string                        set the secret (HMAC) or path of the PEM file containing the public key (RSA and ECDSA)
If its a HMAC secret it shouldn't be public, so it seems like we would want to maybe name the flag something to indicate as much. Calling it --bundle-key or something helps make it more explanatory that its the key for the bundle and that it may not be literally a public key in the RSA sense.

For the error message on:
{12:47} ~/scratch ❯ opa build ./bundle
error: missing configuration to verify bundle signature
We should maybe clarify that it is unable to verify the signed bundle source because it was missing the config, not just that it was missing configuration.

For opa build and opa sign we might want to change --key to like --signing-key or something. Its not really clear from the flag description what key it is or how its used:
--key string           set the secret (HMAC) or path of the PEM file containing the private key (RSA and ECDSA)
It's way more obvious for opa sign but for opa build it gets harder as we have a verification key and a signing key. Its harder to tell them apart. For consistency between the commands however we might want to rename both.

There seems to be an issue with loading the .manifest when signing in the current directory:
{13:00} ~/s/bundle ❯ find .
.
./main.rego
./data.yaml
./authz
./authz/util
./authz/util/helpers.rego
./authz/allow.rego
./external
./external/data.json
./.manifest
{13:00} ~/s/bundle ❯ opa sign --signing-alg HS256 --key k1 -b .
{13:01} ~/s/bundle ❯ find .
.
./.signatures.json
./main.rego
./data.yaml
./authz
./authz/util
./authz/util/helpers.rego
./authz/allow.rego
./external
./external/data.json
./.manifest
{13:01} ~/s/bundle ❯ opa run --signing-alg HS256 --pubkey k1 .
error: load error: 1 error occurred during loading: file .manifest not included in bundle signature
^ I didn't expect to see an error doing that

Whats the plan for eval, bench, check, test, etc? As-is they error out for any signed bundle saying more config is required but that cannot be supplied. If we want to merge without the flags added and plumbed through maybe we should catch this error and give a better "not implemented" sort of error?
{13:05} ✘ ~/scratch ❯ opa eval -b ./bundle 'data'
{
  "errors": [
    {
      "message": "loading error: bundle ./bundle: reader missing signature verification config (hint: check opa configuration includes key(s))"
    }
  ]
}

ashutosh-narkar · 2020-07-06T19:47:38Z

Added changes that skip bundle verification in bench, check, deps, eval, oracle and test. In these commands, verification cannot be turned on (ie. no configuration flag to turn on verification). We could document that these commands do not support bundle verification in their respective help text. WDYT ?

Whats the plan for eval, bench, check, test, etc? As-is they error out for any signed bundle saying more config is required but that cannot be supplied. If we want to merge without the flags added and plumbed through maybe we should catch this error and give a better "not implemented" sort of error?

tsandall

I did another round of manual testing on the latest changes. I think this is quite close. I've left a few comments on error messages and help text but that's about it.

I did notice one behaviour that seems like a bug. This produces an error:

torin:~/temp$ touch .foo
torin:~/temp$ ls -al
total 8
drwxr-xr-x  2 torin torin 4096 Jul  6 15:50 .
drwxr-xr-x 16 torin torin 4096 Jul  6 15:50 ..
-rw-r--r--  1 torin torin    0 Jul  6 15:50 .foo
torin:~/temp$ opa sign . --signing-key secret --signing-alg HS256
error: open foo: no such file or directory

This does not:

torin:~/temp$ mkdir x
torin:~/temp$ touch x/.foo
torin:~/temp$ rm .foo
torin:~/temp$ opa sign . --signing-key secret --signing-alg HS256
torin:~/temp$ ls -alR
.:
total 16
drwxr-xr-x  3 torin torin 4096 Jul  6 15:51 .
drwxr-xr-x 16 torin torin 4096 Jul  6 15:51 ..
-rw-r--r--  1 torin torin  260 Jul  6 15:51 .signatures.json
drwxr-xr-x  2 torin torin 4096 Jul  6 15:51 x

./x:
total 8
drwxr-xr-x 2 torin torin 4096 Jul  6 15:51 .
drwxr-xr-x 3 torin torin 4096 Jul  6 15:51 ..
-rw-r--r-- 1 torin torin    0 Jul  6 15:51 .foo
$ opa eval -i .signatures.json 'io.jwt.decode(input.signatures[0])'
{
  "result": [
    {
      "expressions": [
        {
          "value": [
            {
              "alg": "HS256"
            },
            {
              "files": [
                {
                  "algorithm": "SHA-256",
                  "hash": "12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126",
                  "name": "x/.foo"
                }
              ]
            },
            "a2ebc02cff48ab0f0ae551bc24ef4a84d324a8567ab101e9b160ea9f81641538"
          ],
          "text": "io.jwt.decode(input.signatures[0])",
          "location": {
            "row": 1,
            "col": 1
          }
        }
      ]
    }
  ]
}

[EDIT: hit submit accidentally]

This made me realize that opa sign is not idempotent; if you run opa sign ... without -b then it produces a .signatures.json file. The second time you run the same command, you get an error. However, if we were to fix this and run the command a second time then opa sign need to ignore .signatures.json in order to be idempotent. This begs the question of how the CLI treats .signatures.json files...today, without signing, if you run commands without -b then we don't apply any meaning to the files; OPA just loads files into memory and tries to treat them as policy or data (with directories controlling the location of raw JSON). With -b that behaviour is modified slightly; OPA only looks for data.json files and manifests can control roots. In the future we may choose to apply more meaning to bundles.

With these changes, it seems like we've moved away from that model. If you run opa run . and there is a signatures.json file then we try to check them. From a security POV this makes sense; we don't want users running commands on file systems and assuming they're secure. OTOH, from a consistency POV this marks a change; we're now applying special meaning to certain files.

I'm not immediately sure what to takeaway from this but figured it was worth putting into text.

EDIT2:

It looks like we're treating both .signatures.json and signatures.json as the same?

torin:~$ opa run signatures.json
error: load error: unable to verify signature of signed bundle (hint: check --verification-key flag to provide signature verification config)
torin:~$ cat signatures.json
{"foo":1}
torin:~$ mv signatures.json .signatures.json
torin:~$ opa run .signatures.json
error: load error: unable to verify signature of signed bundle (hint: check --verification-key flag to provide signature verification config)

This seems like a bug.

cmd/sign.go

loader/loader.go

bundle/bundle.go

tsandall · 2020-07-06T21:10:43Z

cmd/build.go

 	"github.com/spf13/cobra"

 	"github.com/open-policy-agent/opa/compile"
 	"github.com/open-policy-agent/opa/util"
 )

+const defaultPublicKeyID = "opa_default_key"


For some reason this key ID bothers me--I think it's because opa and key seem redundant. Could we just use "default"?

Digging into this a bit more...what am I doing wrong?

# build a bunlde $ opa build . --signing-key=secret --signing-alg=HS256 # serve over http $ python3 -m http.server

Inside another terminal:

$ opa run -s --set services.default.url=http://localhost:8000 --set bundles.default.resource=bundle.tar.gz --set keys.opa_default_key.key=secret --set keys.opa_default_key.algorithm=HS256 {"addrs":[":8181"],"diagnostic-addrs":[],"insecure_addr":"","level":"info","msg":"Initializing server.","time":"2020-07-06T17:51:53-04:00"} {"level":"info","msg":"Starting bundle downloader.","name":"default","plugin":"bundle","time":"2020-07-06T17:51:53-04:00"} {"level":"error","msg":"Bundle download failed: verification key ID is empty","name":"default","plugin":"bundle","time":"2020-07-06T17:51:53-04:00"}

It seems the JWTs produced by opa build do not have the key ID claim contained in them. This seems like a bug? I can specify the keyid inside the bundle config section but that does not seem correct.

Updated the default key ID to default.

The key ID can be optionally included in the JWT by providing a path to the JSON file that contains optional claims (--claims-file). OPA will look for the key ID on the command-line first, then in the OPA config and finally in the JWT. We've documented this here.

# build a bundle $ opa build . --signing-key=secret --signing-alg=HS256 # serve over http $ python3 -m http.server # specify the key ID on the command-line $ opa run -s --set services.default.url=http://localhost:8000 --set bundles.default.resource=bundle.tar.gz --set bundles.default.signing.keyid=default --set keys.default.key=secret --set keys.default.algorithm=HS256 {"addrs":[":8181"],"diagnostic-addrs":[],"insecure_addr":"","level":"info","msg":"Initializing server.","time":"2020-07-06T16:12:50-07:00"} {"level":"info","msg":"Starting bundle downloader.","name":"default","plugin":"bundle","time":"2020-07-06T16:12:50-07:00"} {"level":"info","msg":"Bundle downloaded and activated successfully.","name":"default","plugin":"bundle","time":"2020-07-06T16:12:50-07:00"}

claims.json

{ "keyid": "default" }

# build a bundle $ opa build . --signing-key=secret --signing-alg=HS256 --claims-file claims.json # serve over http $ python3 -m http.server # use key ID specified in the JWT $ opa run -s --set services.default.url=http://localhost:8000 --set bundles.default.resource=bundle.tar.gz --set keys.default.key=secret --set keys.default.algorithm=HS256 {"addrs":[":8181"],"diagnostic-addrs":[],"insecure_addr":"","level":"info","msg":"Initializing server.","time":"2020-07-06T16:22:44-07:00"} {"level":"info","msg":"Starting bundle downloader.","name":"default","plugin":"bundle","time":"2020-07-06T16:22:44-07:00"} {"level":"info","msg":"Bundle downloaded and activated successfully.","name":"default","plugin":"bundle","time":"2020-07-06T16:22:44-07:00"}

@ashutosh-narkar WDYT about making opa build set the keyid claim automatically (the claim file could override it if needed).

Just to close the loop on the offline conversation. We reached a decision to have opa build inject the keyid claim using the default value, by default. Users can override the keyid by passing their own claims.

Done.

# build a bundle opa build -b . --signing-key=secret --signing-alg=HS256 # serve over http $ python3 -m http.server # run OPA opa run -s --set services.default.url=http://localhost:8000 --set bundles.default.resource=bundle.tar.gz --set keys.default.key=secret --set keys.default.algorithm=HS256 {"addrs":[":8181"],"diagnostic-addrs":[],"insecure_addr":"","level":"info","msg":"Initializing server.","time":"2020-07-09T01:33:30-07:00"} {"level":"info","msg":"Starting bundle downloader.","name":"default","plugin":"bundle","time":"2020-07-09T01:33:30-07:00"} {"level":"info","msg":"Bundle downloaded and activated successfully.","name":"default","plugin":"bundle","time":"2020-07-09T01:33:30-07:00"}

tsandall · 2020-07-06T21:45:11Z

compile/compile.go

@@ -243,6 +258,7 @@ func (c *Compiler) initBundle() error {
 	result := &bundle.Bundle{}
 	result.Manifest.Init()
 	result.Data = load.Files.Documents
+	result.Signatures = load.Files.Signatures


I was just playing around with the build command a bit more and noticed this line; I don't think it has any effect and we don't have any corresponding test coverage for the changes to this file to verify it. Let's at least add some tests that verify the expected behaviour (e.g., you could have two tests, one that compiles w/o optimization disables, so that it's effectively a no-op that just tar/gzips the input files and another that enables optimization so the file contents change and the resulting signatures are rewritten.)

Added more tests to cover this.

Two thoughts...

I was a bit surprised to see that we're modifying the bundle passed as a parameter to the Write call. The Write call accepts a bundle by-value not by-reference. This means that writes to the bundle structure will not be seen by the caller. However, since the signature config field is a pointer, the caller will see writes to that. From a maintainability POV, I could see this being an issue down the road.

If the signatures are being updated by the Write call, why do we need to initialize here?

Takeaways:

Is the Signatures field a pointer to SignaturesConfig by design? If so, why? If not, I'd be tempted to remove the pointer so that we can continue passing the Bundle around by value and safely assume it's not being mutated by the receiver.

If we compute a bundle signature and then write out the bundle, when we re-read the bundle into memory, can we re-compute the signature?

Couple of changes:

Removed the pointer for the Signatures field in the SignaturesConfig.

Removed signing logic from the writer. So now it simply writes the bundle object to the underlying data stream.

If we compute a bundle signature and then write out the bundle, when we re-read the bundle into memory, can we re-compute the signature?

Kind of, you can re-compute the payload. For example:

# in directory "foo" foo/data.json foo/policy.rego # generate a signature opa build --signing-alg HS256 --signing-key secret -b foo # this creates a bundle "bundle.tar.gz" # bundle contents tar -tzf bundle.tar.gz /data.json /foo/policy.rego /.manifest /.signatures.json -----> A # compute the bundle signature opa sign --signing-key secret --signing-alg HS256 -b bundle.tar.gz # this generates a ".signatures.json" in the current directory -----> B

The JWT payload in A and B will match. Although the payloads match, the token itself won't match as the files list in the payload has the files in different orders in A and B. Hope this makes sense.

koponen-styra · 2020-07-13T17:58:34Z

bundle/hash.go

+
+		h.Write([]byte("]"))
+	default:
+		h.Write(encodePrimitive(x))


@tsandall asked about hashing []byte offline, whether it needs to be serialized to JSON first before hashing. That's unnecessary, you should hash the []byte directly here.

tsandall

@ashutosh-narkar LGTM. Can you add that documentation note I just mentioned and squash your commits into a smaller set that makes sense (1 commit is fine or multiple, up to you!)

tsandall · 2020-07-13T19:42:29Z

docs/content/management.md

+When OPA receives a new bundle, it checks that it has been properly signed using a (public) key that OPA has been
+configured with out-of-band.  Only if that verification succeeds does OPA activate the new bundle; otherwise, OPA
+continues using its existing bundle and reports an activation failure via the status API and error logging.
+


Can we add a note here to warn folks that bundle signing is only supported in some modes? Something like this:

> ⚠️ Bundle signature verification is only performed by the `opa run` when the `-b`/`--bundle` flag is given or when Bundle downloading is enabled. Sub-commands primarily used in development and debug environments (such as `opa eval`, `opa test`, etc.) DO NOT verify bundle signatures at this point in time.

I just want to make sure we have something we can point to for the time being. We can work on introducing CLI support for verification in the future.

Added the comment and squashed the commits.

These changes add support for digital signatures for policy bundles which can be used to verify their authenticity. Bundle signature verification involves the following steps: * Verify the JWT signature * Verify the files in the JWT payload exist in the bundle * Verify the file content of the files in bundle match with those in the payload This commit adds a new `sign` command to generate a digital signature for policy bundles. For more details, run "opa sign --help" The signatures generated by the 'sign' command can be verified by the 'build' command. The 'build' command can also sign the bundle it generates. The 'run' command can verify a signed bundle or skip verification altogether. OPA 'sign', 'build' and 'run' can be used to sign/verify bundles in bundle mode (--bundle) mode only. Verification can be also be performed when bundle downloading is enabled. Fixes: open-policy-agent#1757 Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

tsandall

LGTM 🔥

ashutosh-narkar marked this pull request as draft June 17, 2020 20:09

tsandall requested a review from patrick-east June 18, 2020 16:01

patrick-east reviewed Jun 19, 2020

View reviewed changes

ashutosh-narkar force-pushed the bundle-signing branch from be87ff4 to 30c76ea Compare June 22, 2020 22:27

ashutosh-narkar marked this pull request as ready for review June 22, 2020 23:07

ashutosh-narkar changed the title ~~[WIP] Bundle signature verification~~ Support for bundle signature verification Jun 22, 2020

tsandall reviewed Jun 24, 2020

View reviewed changes

koponen-styra reviewed Jun 24, 2020

View reviewed changes

ashutosh-narkar force-pushed the bundle-signing branch from 248b7e5 to cbdcf6d Compare June 25, 2020 08:26

koponen-styra reviewed Jun 25, 2020

View reviewed changes

tsandall reviewed Jun 26, 2020

View reviewed changes

ashutosh-narkar force-pushed the bundle-signing branch 3 times, most recently from ddd3994 to 4e3039c Compare June 30, 2020 21:56

patrick-east reviewed Jul 1, 2020

View reviewed changes

ashutosh-narkar force-pushed the bundle-signing branch from d8b02ad to f203c3b Compare July 2, 2020 04:34

ashutosh-narkar force-pushed the bundle-signing branch from eefd530 to f5e5389 Compare July 6, 2020 19:41

tsandall reviewed Jul 6, 2020

View reviewed changes

ashutosh-narkar force-pushed the bundle-signing branch 6 times, most recently from 443eea3 to 41d28ff Compare July 13, 2020 16:54

koponen-styra reviewed Jul 13, 2020

View reviewed changes

tsandall previously approved these changes Jul 13, 2020

View reviewed changes

ashutosh-narkar dismissed tsandall’s stale review via 1c9b933 July 13, 2020 23:29

ashutosh-narkar force-pushed the bundle-signing branch from 28d89d9 to 1c9b933 Compare July 13, 2020 23:29

tsandall approved these changes Jul 14, 2020

View reviewed changes

tsandall merged commit 338583c into open-policy-agent:master Jul 14, 2020

Support for bundle signature verification #2475

Support for bundle signature verification #2475

Conversation

ashutosh-narkar commented Jun 17, 2020 • edited Loading

patrick-east left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsandall commented Jun 23, 2020 • edited Loading

patrick-east commented Jun 23, 2020 • edited Loading

patrick-east commented Jun 23, 2020

tsandall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsandall Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick-east commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsandall Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar commented Jul 6, 2020

ashutosh-narkar commented Jul 6, 2020

tsandall left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsandall Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsandall Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar commented Jun 17, 2020 •

edited

Loading

tsandall commented Jun 23, 2020 •

edited

Loading

patrick-east commented Jun 23, 2020 •

edited

Loading

tsandall Jun 26, 2020 •

edited

Loading

tsandall Jul 6, 2020 •

edited

Loading

tsandall left a comment •

edited

Loading

tsandall Jul 6, 2020 •

edited

Loading

tsandall Jul 7, 2020 •

edited

Loading