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

Proposal 64: Allowlist signing and verification improvements #65

Closed
wants to merge 1 commit into from

Conversation

mbestavros
Copy link
Contributor

Accompanies #64

Signed-off-by: Mark Bestavros mbestavr@redhat.com

@mbestavros
Copy link
Contributor Author

@lukehinds @mpeters likely of interest to both of you 😄

@mbestavros mbestavros force-pushed the sigstore-signing branch 2 times, most recently from c967b5c to e11289f Compare March 1, 2022 21:11
@mbestavros mbestavros changed the title Proposal 64: allowlist signing and verification using Sigstore Allowlist signing and verification improvements Mar 4, 2022
@mbestavros mbestavros force-pushed the sigstore-signing branch 2 times, most recently from 7de2931 to f22417b Compare March 4, 2022 21:23
@mpeters
Copy link
Member

mpeters commented Mar 9, 2022

Can this be expanded to things beyond just allowlists? Can we also include measured boot refstates? Payloads? Just spitballing a bit here.

@THS-on
Copy link
Member

THS-on commented Mar 9, 2022

It would be nice if we could combine this proposal with the completion of https://github.com/keylime/enhancements/blob/master/38-allowlist-management.md and put the entire verification code on the server side not the tenant.
This has the advantage, that if you integrate Keylime into other products directly with the Keylime APIs (this is what we are doing) this feature is still useful.

@mbestavros mbestavros changed the title Allowlist signing and verification improvements Proposal 64: Allowlist signing and verification improvements Mar 14, 2022
@lukehinds
Copy link
Member

lukehinds commented Mar 18, 2022

Can this be expanded to things beyond just allowlists? Can we also include measured boot refstates? Payloads? Just spitballing a bit here.

other things is a good idea, but I think payloads are already protected as part of the agent k,v key schema?


- A Keylime deployment with access available to a transparency log (can be deployed privately or a public instance is available and run by the Linux Foundation).
- User provides the tenant with paths to an allowlist, a signature, and a public key using a modern algorithm (e.g. ECDSA)
- Keylime verifies the inputs are consistent, then checks Rekor for an inclusion proof of those artifacts, before loading them into Keylime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also look at signed bundles here, for air gapped machines. They do something similar in cosign, and we can replicate that into the keylime tenant CLI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can include the signed bundle work flow as another user story.

Comment on lines +201 to +225
Currently, all signature validation happens within `read_allowlist()` in [ima.py](https://github.com/keylime/keylime/blob/master/keylime/ima.py#L408). This proposal suggests an additional, optional verification step that would check provided artifacts for inclusion proof in a transparency log. This assumes the user has already uploaded those artifacts to that transparency log using a separate tool (for example, `rekor-cli`).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you plan to refactor the key verification into the verifier (good move), would this enhancement follow that work? If so I expect the inclusion proofs will occur in the verifier (or maybe both)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I think this is a good time to move this verification into the verifier and it should be part of this propsoal.


To support this new functionality, the tenant command should have two new flags:

- `--allowlist-verify-transparency-log`: to indicate the user would like to verify against a transparency log. Since both a signature and public key are necessary, existing flags`--allowlist-sig` and `--allowlist-sig-key` should also be set when using this new flag.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a nit, but i would go for allowlist-verify-tlog or rekor-verify to save a few keystrokes for users :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. We're already using some pretty long options to the tenant, no need to make it worse.

@lukehinds
Copy link
Member

It would be nice if we could combine this proposal with the completion of https://github.com/keylime/enhancements/blob/master/38-allowlist-management.md and put the entire verification code on the server side not the tenant. This has the advantage, that if you integrate Keylime into other products directly with the Keylime APIs (this is what we are doing) this feature is still useful.

Yep, agree. We could have the tenant generate keys and sign things, but all verification should happen on the verifier side. That way any client side implementation can do the signing however it wants to. I spoke with @mbestavros and he has the same view and onboard with that.

@lukehinds
Copy link
Member

p.s whats the status of https://github.com/keylime/enhancements/blob/master/38-allowlist-management.md, did that get underway?

@THS-on
Copy link
Member

THS-on commented Mar 18, 2022

p.s whats the status of https://github.com/keylime/enhancements/blob/master/38-allowlist-management.md, did that get underway?

Basic CRUD functionality is there, but the lists are not shared between agents. In the current scenario the tenant downloads the list and then adds it to the agent when it is added to the verifier which is not ideal.
A better way would be that the only allowlist name is sent and the verifier gets that information when needed from the database.


- A Keylime deployment with access available to a transparency log (can be deployed privately or a public instance is available and run by the Linux Foundation).
- User provides the tenant with paths to an allowlist, a signature, and a public key using a modern algorithm (e.g. ECDSA)
- Keylime verifies the inputs are consistent, then checks Rekor for an inclusion proof of those artifacts, before loading them into Keylime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can include the signed bundle work flow as another user story.


How will security be reviewed and by whom?
-->

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a feature for enhanced security, we should add some notes about how the security review will take place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would that look like? Are there any existing enhancement proposals that have suitable language I could draw on?

64_allowlist_signing.md Show resolved Hide resolved
Comment on lines +201 to +225
Currently, all signature validation happens within `read_allowlist()` in [ima.py](https://github.com/keylime/keylime/blob/master/keylime/ima.py#L408). This proposal suggests an additional, optional verification step that would check provided artifacts for inclusion proof in a transparency log. This assumes the user has already uploaded those artifacts to that transparency log using a separate tool (for example, `rekor-cli`).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I think this is a good time to move this verification into the verifier and it should be part of this propsoal.


To support this new functionality, the tenant command should have two new flags:

- `--allowlist-verify-transparency-log`: to indicate the user would like to verify against a transparency log. Since both a signature and public key are necessary, existing flags`--allowlist-sig` and `--allowlist-sig-key` should also be set when using this new flag.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. We're already using some pretty long options to the tenant, no need to make it worse.

To support this new functionality, the tenant command should have two new flags:

- `--allowlist-verify-transparency-log`: to indicate the user would like to verify against a transparency log. Since both a signature and public key are necessary, existing flags`--allowlist-sig` and `--allowlist-sig-key` should also be set when using this new flag.
- `--transparency-log-url`: to provide the server address of a compatible transparency log. Defaults to `rekor.sigstore.dev`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful defaults are good.

expectations).
-->

- A new test should be written to ensure that the allowlist loading process checks for inclusion proof in a transparency log.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be done carefully. We can't always assume tests will have access to the outside internet. CI should be fine as that needs to download packages, etc, but local runs might not be connected. So we should have a way to skip the test if the public tlog can't be reached, or at least fail gracefully so devs can know if they need to be concerned.

64_allowlist_signing.md Outdated Show resolved Hide resolved
64_allowlist_signing.md Outdated Show resolved Hide resolved
information to express the idea and why it was not acceptable.
-->

There are more or less no alternatives (at present) for Sigstore as a transparency log.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "more or less" seems wishy washy. Maybe something like "Sigstore is the best known suitable transparency log for this kind of application. While the public-good version is a great option, users can always run their own internal versions for enhanced control and security. And our design does not preclude the use of other compatible transparency logs in the future". Or something like that.

Signed-off-by: Mark Bestavros <mbestavr@redhat.com>
@mbestavros
Copy link
Contributor Author

I've written a second draft of the proposal that incorporates verifier-side signing as part of the design, and have addressed various bits of feedback. Would love another round of review!

cc @mpeters @lukehinds @THS-on

@THS-on
Copy link
Member

THS-on commented Mar 31, 2022

@mbestavros @mpeters what do you thing about using JSON Web Signature as a standard format for the IMA policy signatures?

@mpeters
Copy link
Member

mpeters commented Mar 31, 2022

I could be wrong, but my reading of the JWS spec is that it's more akin to JWT in that it's for small structures that are then hashed/signed and passed around with HMACs, suitable for cookies and HTTP headers. Nothing as large and sprawling as a host's allowlist.

@lukehinds
Copy link
Member

Not a big fan of JWS myself, its prone to multiple attacks and users can easily shoot themselves in the foot with how loosely opinionated it is on setting algorithm types etc. A lot of the flaws are covered in here: https://nathantypanski.com/blog/2021-12-24-jws-nightmare.html

It's a lot simpler to just have a key pair coupled with X509, it works well and is not as error prone.

@THS-on
Copy link
Member

THS-on commented Apr 1, 2022

@lukehinds thanks for the link. I was just looking if there is a standard for putting signatures in json objects, but I agree that for our use case JWS is not a great fit.

We can do just something like this:

{
  "payload": "BASE64(ima_policy)",
  "signature": "BASE64(signature)",
  "type": "(GPG|X509)"
}

@lukehinds
Copy link
Member

lukehinds commented Apr 1, 2022

@THS-on , ahh, I get you now. Interesting!

So what you outlined there, is quite similar to TUF (theupdateframework.org), the use a very similair manifest layout, where you sign over the body (or payload as you have here). https://theupdateframework.github.io/specification/latest/#rootjson

I think something like this is a good idea as we could even parse out different styles of manifest, so to take your example:

{
  "payload": "BASE64(ima_policy)",
  "signature": "BASE64(signature)",
  "type": "SPDX",
  "alg": "(GPG|X509)"
}

My thinking here is @THS-on that we could have keylime attest SBOMs. Someone for example compiles a go binary (or any compiled lang). They then sign the digest (or list of digests) using a keypair. This SBOM can then be fed into Keylime who will in turn measure the binary at execution time using IMA.

This way a developer or a CI system could build some source code and then at the other end, when it runs in production, we ensure it has not been tampered with.

I think I will try and sketch some ideas out in a google doc and share it back with you.

@mbestavros apologies about noodling on this matter, mid flight of your PR, but I think this is an exciting topic.

@THS-on
Copy link
Member

THS-on commented Apr 1, 2022

@lukehinds this kind of format makes sense. SBOMs sound interesting, but I have no experience with that. Can we encode your current IMA configuration (keys, allowlist, exclude list, several configuration settings) as an SBOM?

The question is where we want to implement support for this?
If it is part of the tenant, it is not useful to all Keylime users because for example we directly use the verifier API.
If it is part of the verifier, we need to make sure that the API that we expose is rather easy to integrate and that feature is long term maintainable because it is now part of the Keylime API.

@THS-on
Copy link
Member

THS-on commented Feb 17, 2023

I think this is now superseded by #89, @mbestavros correct?

@mbestavros
Copy link
Contributor Author

mbestavros commented Feb 17, 2023

@THS-on More or less. Some details of this PR are already in Keylime as natural consequences of other work, like storing unmodified policies on the verifier and verifier-side signature checks. But this didn't account for attached signatures, which supersedes most of the ideas here.

@mbestavros mbestavros closed this Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants