Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutated manifest file prevents bundle loading #4233

Closed
CristianJena opened this issue Jan 14, 2022 · 1 comment · Fixed by #4242
Closed

Mutated manifest file prevents bundle loading #4233

CristianJena opened this issue Jan 14, 2022 · 1 comment · Fixed by #4242
Labels
bug investigating Issues being actively investigated

Comments

@CristianJena
Copy link

Short description

I'm working on a project that will use OPA (v0.35.0) for controlling access upon certain resources. For this we have policies, data and we sign our bundle (this is a strong requirement). Policies and data are coming from another source to our service that is in change with building the bundle. We write the policies and data to a folder and use OPA cli, like this:

opa build -b . --signing-key=opa-bundle-key.pem

We needed some information related to bundle itself (revision) because we build the bundle based on certain events (new policy coming in). This information was added in the .manifest file, file that is created in the same folder as policies and data before using the build command. In the same file, under metadata tag we have some additional informations useful for us.

The initial .manifest file looks like this:

{
    "metadata":
    {
        "buildContainerName": "service-container-id",
        "buildJobId": "job-id",
        "buildJobVersion": "my-version"
    }
}

and another version

{
    "metadata":
    {
        "buildContainerName": "service-container-id",
        "buildJobId": "job-id",
        "buildJobVersion": "my-version"
    },
    "revision": "1642085539338",
    "roots":[""]
}

but after bundle comand, the .manifest file is changed like this (metadata property is moved at the end).

{
    "revision": "1642085539338",
    "roots":[""],
    "metadata":
    {
        "buildContainerName":"service-container-id",
        "buildJobId":"job-id",
        "buildJobVersion":"my-version"
    }
}

The problem occurs when when OPA wants to load the bundle, we get a signature mismatch error for .manifest file.
We know that the properties of structured data (as JSON) have to be sorted alphabetically in order to have a corect hash calculated (as OPA will do the same when it verifies the bundle) but in this case the file is mutated after the signature is calculated I presume.

To bypass this "behaviour" we used --exclude-files-verifyflag for the .manifest file but this is more like an unexpected workaround.

Steps To Reproduce

  1. Create a .manifest with a metadata property (example above) .
  2. Create a bundle that contains policy, data etc. and sign the bundle.
  3. Load the bundle with OPA server.

Expected behavior

OPA should load a signed bundle created with OPA cli and a provided .manifest file.

Additional context

@anderseknert
Copy link
Member

Thanks for filing this @CristianJena! Looks like a bug indeed.

@ashutosh-narkar ashutosh-narkar added the investigating Issues being actively investigated label Jan 18, 2022
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Jan 18, 2022
When OPA verifies the content of the manifest file,
it first parses it into a JSON structure and then recursively orders
the fields of all objects alphabetically and then applies the
hash function. The same process was not followed while generating
the hash for the manifest content which would result in a digest
mismatch during verification. This can be observed with a manifest
that contains metadata.

Fixes: open-policy-agent#4233

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar added a commit that referenced this issue Jan 19, 2022
When OPA verifies the content of the manifest file,
it first parses it into a JSON structure and then recursively orders
the fields of all objects alphabetically and then applies the
hash function. The same process was not followed while generating
the hash for the manifest content which would result in a digest
mismatch during verification. This can be observed with a manifest
that contains metadata.

Fixes: #4233

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
damienjburks pushed a commit to damienjburks/opa that referenced this issue Jan 19, 2022
Signed-off-by: Damien Burks <damien@hackintosh.attlocal.net>

website/load-docs.sh: add versioned docs for latest version (open-policy-agent#4222)

With this change, we should get both a /docs/v0.36.1 and a /docs/latest
in our deployed docs.

Before, the symlink would have made hugo only build a /docs/latest,
but not the other link.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

removing -l flag from the test subcommand

Adding additional space to sign off on commit after removing -l from test.go

Signed-off-by: Damien Burks <damien@hackintosh.attlocal.net>

removing space

Signed-off-by: Damien Burks <damien@hackintosh.attlocal.net>

Add Dapr integration (open-policy-agent#4229)

Also included some improvements to the Rego checks:

* Pass GITHUB_TOKEN to policy to not exceed API quota
* Ensure required attributes included in integration
* Ensure commited .json files are valid JSON

Signed-off-by: Anders Eknert <anders@eknert.com>

Follow redirects in http.send Rego checks (open-policy-agent#4232)

Signed-off-by: Anders Eknert <anders@eknert.com>

fixing broken links in the online documentation (open-policy-agent#4224)

Signed-off-by: Peter Helewski <phelewski@gmail.com>

plugins/bundle: Update persisted bundle activation mechanism

Earlier errors encountered during loading and activating persisted
bundles would cause the OPA runtime to exit. This behavior is different
from when OPA downloads a bundle and activation errors if any would possibly
get resolved in successive download attempts. This fix adds a retry mechanism
to activate persisted bundles in an attempt to mimic the behavior seen during
bundle downloads. Errors if any encountered during the process will be
surfaced in the bundle's status update and not result in an abrupt exit.

Fixes: open-policy-agent#3840

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

built-ins: add graph.reachable_paths (open-policy-agent#4205)

This new built-in functionality allows callers to find all reachable
paths in a graph based on an array or set of root nodes. See the
updates to policy-reference.md for more details and usage information.

Signed-off-by: Justin Lindh <justin.lindh@webfilings.com>

ast/parser: don't error when future kw import is redundant (open-policy-agent#4237)

When passing `ParserOptions{AllFutureKeywords: true}` or
`ParserOptions{FutureKeywords: []string{"in"}}` to the `ast` package's
parse methods, and when parsing a module that contains

    import future.keywords.in

then we would previously have raised an error: when the parser
encountered the `in`, it complains about not expecting an "in" token
there.

Now, parsing imports is done with a scanner copy that knows nothing
about the future keywords.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

ast: Adding duplicate imports check to compiler strict mode (open-policy-agent#4228)

When strict mode is enabled, an import shadowing another import is an error.

Fixes: open-policy-agent#2698
Signed-off-by: Johan Fylling <johan.dev@fylling.se>

ast: add `every` future keyword (parser, internal representation) (open-policy-agent#4179)

* ast: add 'every' future keyword, parser support, scaffolding

With this commit, "every x in xs { ... }" and "every k, v in { ... }"
will be parsed into a new struct, which is basically

    Every {
      Key, Value *Term
      Domain *Term
      Body Body
    }

This includes the required to changes to visitors, comparisons, copy, ...
all the ceremony required to introduce a new keyword.

* format: format every statements

* ast: hide 'every' from capabilities for now

With this change,

- capabilities.json will NOT mention "every"
- "import future.keywords" will NOT get you "every"
- "import future.keywords.every" will complain about "every" being unknown

In tests, we're passing an unexported field in `ast.ParserOptions`,
which makes the parser treat "every" like it would eventually be
treated when unveiled.

The formatting tests are bluntly SKIPPED for now, to be re-enabled later.

---------------
NB: The work on "every" is ongoing.
Rewriting and evaluation follows. When it's documented, we'll unhide it.
---------------

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

bundle: Roundtrip manifest before hashing

When OPA verifies the content of the manifest file,
it first parses it into a JSON structure and then recursively orders
the fields of all objects alphabetically and then applies the
hash function. The same process was not followed while generating
the hash for the manifest content which would result in a digest
mismatch during verification. This can be observed with a manifest
that contains metadata.

Fixes: open-policy-agent#4233

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

Removing more deprecated code related to failureLine

Signed-off-by: Damien Burks <damien@hackintosh.attlocal.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug investigating Issues being actively investigated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants