search

grafeas / kritis

Deploy-time Policy Enforcer for Kubernetes applications
https://github.com/grafeas/kritis/blob/master/docs/binary-authorization.md
Apache License 2.0
699 stars 135 forks source link

Add support for the separation of noteReference and occurrenceReference #401

Closed marco-lancini closed 5 years ago

marco-lancini commented 5 years ago

Feature Request

Building on Issue #396, it would be good to extend Kritis so to add support for the separation of noteReference and occurrenceReference. This will also help lining up with the multi project setup supported by BinAuth

Proposed Solution

aysylu commented 5 years ago

Hey @marco-lancini, my understanding from our conversation today was that we'll put the new occurrenceReference field to ImageSecurityPolicy and GenericAttestationPolicy. Did something change on your end to end up with this proposal?

marco-lancini commented 5 years ago

Hey @aysylu, you are correct! I edited the description of this issue to reflect this. Thanks for pointing this out

marco-lancini commented 5 years ago

Hi @aysylu, after considering this further today while planning the required changes, we were wondering if it could be more convenient to attach the new occurrenceReference field directly to an AttestationAuthority, rather than to ISPs/GAPs.

As the AttestationAuthority is used by both ISPs and GAPs, adding the new field to AttestationAuthority would enable separation of note and occurrences for both ISPs and GAPs.

In this way we would also prevent the de-duplication of the code related to the verification logic that we will have to introduce in the other case. For example, findUnsatisfiedAuths [1] would have to be extended to support both the v1beta1.ImageSecurityPolicy and v1beta1.GenericAttestationPolicy types distinctly.

[1] findUnsatisfiedAuths: https://github.com/grafeas/kritis/blob/22bf0899396a6b164253193421e6cbacc2fc3dcf/pkg/kritis/review/review.go#L146

aysylu commented 5 years ago

Hi @marco-lancini, thank you for filing this issue and the discussion! That's a great question. I could see this go either way, so let me think about this. Let's also get a perspective from @nenaddedic who's working on BinAuthz.

nenaddedic commented 5 years ago

@marco-lancini This seems to be a bug in Kritis implementation.

Grafeas has the ListNoteOccurrences method [1] which returns all Occurrences attached to a Note (subject to access restrictions). Returned Occurrences may be in multiple projects. Kritis should use this method instead of ListOccurrences [2] which is scoped to a single project. I think this would address your concern, because noteReference would be fully respected when fetching the Occurrences during attestor evaluation.

So specifically: I propose to change the code in [2] to use ListNoteOccurrences, where nID (the ID of the Note occurrences to fetch) is set to noteReference found in the attestor.

Does this sound right to you?

[1] https://github.com/grafeas/grafeas/blob/master/go/v1/api/grafeas.go#L96 [2] https://github.com/grafeas/kritis/blob/22bf0899396a6b164253193421e6cbacc2fc3dcf/pkg/kritis/metadata/containeranalysis/containeranalysis.go#L104

marco-lancini commented 5 years ago

Hi @nenaddedic, thanks for your input. I'm trying to modify the fetchOccurrence as below:

req := &grafeas.ListNoteOccurrencesRequest{
    Name: "projects/project-attestor/notes/build-attestor-note",
    Filter:    fmt.Sprintf("resource_url=%q AND kind=%q", util.GetResourceURL(containerImage), kind),
    PageSize:  constants.PageSize,
    PageToken: "0",
}

it := c.client.ListNoteOccurrences(c.ctx, req)

But I'm facing the following error:

E1007 10:35:05.673599       1 validating_transport.go:57] rpc error: code = InvalidArgument desc = Request contains an invalid argument.
E1007 10:35:05.673625       1 review.go:151] Error fetching validated attestations for eu.gcr.io/project-gcr/<path>/alpine@sha256:<hash>: rpc error: code = InvalidArgument desc = Request contains an invalid argument.

After some trial and error, this seems to be related to the Filter field. In particular, we discovered that the offending part of the Filter is the kind: 

Filter:    fmt.Sprintf("resource_url=%q AND kind=%q", util.GetResourceURL(containerImage), kind),

It does work fine when we omit it from the Filter, but it is probably not the best solution.

Any suggestions?

nenaddedic commented 5 years ago

It is fine to drop the kind clause from the filter. In fact filtering by kind is somewhat incorrect when using ListNoteOccurrences, because Occurrences attached to a Note must be of the same kind as the Note.

This said, verifying that the Note is of the Attestation kind would be good.

nenaddedic commented 5 years ago

If you already have a PR, feel free to send it for review. I opened https://github.com/grafeas/kritis/issues/405 to track.

marco-lancini commented 5 years ago

Hi @nenaddedic and @aysylu, I've updated PR #399 to reflect this. I'd be grateful if you could review and approve it!

aysylu commented 5 years ago

This has now been released as part of v0.2.0. Thanks for your valuable contribution and patience!

Please feel free to re-open this issue if it's not fully addressed in the new release.