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

Add pattern based from/to validity for keys #193

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion attestation/sign_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func TestSignVerifyAttestation(t *testing.T) {
ID: tc.keyID,
PEM: string(tc.pem),
Distrust: tc.distrust,
From: tc.from,
From: &tc.from,
To: tc.to,
Status: tc.status,
}
Expand Down
22 changes: 15 additions & 7 deletions attestation/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,22 @@ type VerifyOptions struct {
}

type KeyMetadata struct {
ID string `json:"id"`
PEM string `json:"key"`
From time.Time `json:"from"`
To *time.Time `json:"to"`
Status string `json:"status"`
SigningFormat string `json:"signing-format"`
Distrust bool `json:"distrust,omitempty"`
ID string `json:"id" yaml:"id"`
PEM string `json:"key" yaml:"key"`
From *time.Time `json:"from" yaml:"from"`
To *time.Time `json:"to" yaml:"to"`
Status string `json:"status" yaml:"status"`
SigningFormat string `json:"signing-format" yaml:"signing-format"`
Distrust bool `json:"distrust,omitempty" yaml:"distrust,omitempty"`
publicKey crypto.PublicKey
Expiries []*KeyExpiry `json:"expiries,omitempty" yaml:"expiries,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

expiries is a strange word, at first I thought it was a spelling mistake!

So this becomes an additional layer of from/to values on top of the key's main from/to value. To me this seems quite complex for key management but I am guessing that it is the only way to handle key usage for platforms and repos?

It's really less of an expiry object or list of expiry objects as much as it is an applicability object that determines which artifacts this key should be used for. I think that naming would also make it more clear what the purpose of it is and separate it from the key's main from/to expiry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm up for some new words here for sure, but I don't understand your comment about complexity. The problem we're solving here is complex, so the solution needs to account for that, and I think it does that in a clear and concise way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering if we should deprecate the to/from at the key level and only use the expiries (or validity_ranges?) because with empty platforms and a wildcard for the pattern it covers the current behaviour....

}

type KeyExpiry struct {
Patterns []string `json:"patterns"`
Platforms []string `json:"platforms"`
To *time.Time `json:"to"`
From *time.Time `json:"from"`
}

type (
Expand Down
34 changes: 29 additions & 5 deletions attestation/verifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,35 @@ func (v *verifier) VerifyLog(ctx context.Context, keyMeta *KeyMetadata, encPaylo
if err != nil {
return fmt.Errorf("TL entry failed verification: %w", err)
}
if integratedTime.Before(keyMeta.From) {
return fmt.Errorf("key %s was not yet valid at TL log time %s (key valid from %s)", keyMeta.ID, integratedTime, keyMeta.From)
}
if keyMeta.To != nil && !integratedTime.Before(*keyMeta.To) {
return fmt.Errorf("key %s was already %s at TL log time %s (key %s at %s)", keyMeta.ID, keyMeta.Status, integratedTime, keyMeta.Status, *keyMeta.To)

// important to allow an empty array here so that we don't fail open
// search for test 'no match should fail closed'
if keyMeta.Expiries != nil {
// any repo expirey still on the keys must match the times
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// any repo expirey still on the keys must match the times
// any repo expiry still on the keys must match the times

(there are a few of this same typo in the PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

toMatch := false
fromMatch := false
// must match at least one - nil is open ended
for _, filter := range keyMeta.Expiries {
if filter.To == nil || (filter.To != nil && integratedTime.Before(*filter.To)) {
toMatch = true
}
if filter.From == nil || (filter.From != nil && integratedTime.After(*filter.From)) {
fromMatch = true
}
if toMatch && fromMatch {
break
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is right - this will match if the To of one expiry matches and the From of another matches, even if the windows don't overlap. Does it even make sense to have more than one expiry that matches the repo/platform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we return the first match.

if !toMatch || !fromMatch {
return fmt.Errorf("Log entry is not within the expiry range of the key: %s", keyMeta.ID)
}
} else {
if keyMeta.To != nil && !integratedTime.Before(*keyMeta.To) {
return fmt.Errorf("key %s was already %s at TL log time %s (key %s at %s)", keyMeta.ID, keyMeta.Status, integratedTime, keyMeta.Status, *keyMeta.To)
}
if keyMeta.From != nil && integratedTime.Before(*keyMeta.From) {
return fmt.Errorf("key %s was not yet valid at TL log time %s (key valid from %s)", keyMeta.ID, integratedTime, keyMeta.From)
}
}
return nil
}
76 changes: 75 additions & 1 deletion policy/rego.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@ import (
"fmt"
"os"
"path/filepath"
"regexp"

"github.com/distribution/reference"
"github.com/docker-library/bashbrew/manifest"
"github.com/docker/attest/attestation"
v1 "github.com/google/go-containerregistry/pkg/v1"
intoto "github.com/in-toto/in-toto-golang/in_toto"
"github.com/open-policy-agent/opa/ast"
"github.com/open-policy-agent/opa/rego"
Expand Down Expand Up @@ -259,7 +262,7 @@ func (regoOpts *RegoFnOpts) fetchInTotoAttestations(rCtx rego.BuiltinContext, pr
return set, nil
}

// because we don't control the signature here (blame rego)
// because we don't control the function signature here (blame rego)
// nolint:gocritic
func (regoOpts *RegoFnOpts) verifyInTotoEnvelope(rCtx rego.BuiltinContext, envTerm, optsTerm *ast.Term) (*ast.Term, error) {
env := new(attestation.Envelope)
Expand All @@ -272,6 +275,11 @@ func (regoOpts *RegoFnOpts) verifyInTotoEnvelope(rCtx rego.BuiltinContext, envTe
if err != nil {
return nil, fmt.Errorf("failed to cast verifier options: %w", err)
}

err = regoOpts.filterRepoExpiries(rCtx.Context, opts)
if err != nil {
return nil, fmt.Errorf("failed to filter repo expiries: %w", err)
}
payload, err := attestation.VerifyDSSE(rCtx.Context, regoOpts.attestationVerifier, env, opts)
if err != nil {
return nil, fmt.Errorf("failed to verify envelope: %w", err)
Expand Down Expand Up @@ -302,6 +310,72 @@ func (regoOpts *RegoFnOpts) verifyInTotoEnvelope(rCtx rego.BuiltinContext, envTe
return ast.NewTerm(value), nil
}

func (regoOpts *RegoFnOpts) filterRepoExpiries(ctx context.Context, opts *attestation.VerifyOptions) error {
// remove any keys that match the pattern but not the platform
imageName, err := regoOpts.attestationResolver.ImageName(ctx)
if err != nil {
return fmt.Errorf("failed to resolve image name: %w", err)
}
parsed, err := reference.ParseNormalizedNamed(imageName)
if err != nil {
return fmt.Errorf("failed to parse image name: %w", err)
}
imageName = parsed.Name()
platform, err := regoOpts.attestationResolver.ImagePlatform(ctx)
if err != nil {
return fmt.Errorf("failed to get image platform: %w", err)
}
for i := range opts.Keys {
key := opts.Keys[i]
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to do

Suggested change
for i := range opts.Keys {
key := opts.Keys[i]
for _, key := range opts.Keys {

Is this a linting rule?

// if there are NO custom expiries, assume key can be checked as normal
if len(key.Expiries) == 0 {
continue
}
if key.From != nil || key.To != nil {
return fmt.Errorf("error key has 'from' or 'to' time set which is not supported when `expiries` is set")
}
// when there are custom expiries, we only keep those that match the image name and platform
// so that the log verifier can check the from/to times
expiries := make([]*attestation.KeyExpiry, 0)
for j := range key.Expiries {
expiry := key.Expiries[j]
if len(expiry.Patterns) == 0 {
return fmt.Errorf("error need at least one expiry pattern")
}
for _, pattern := range expiry.Patterns {
if pattern == "" {
return fmt.Errorf("error empty expiry pattern")
}
patternRegex, err := regexp.Compile(pattern)
if err != nil {
return fmt.Errorf("error failed to compile expiry repo pattern: %w", err)
}
// if there's an image match, then platforms must match too
if patternRegex.MatchString(imageName) {
// either there are no platforms, or at least one must match
if len(expiry.Platforms) == 0 {
expiries = append(expiries, expiry)
break
}
for _, expiryPlatform := range expiry.Platforms {
parsedPlatform, err := v1.ParsePlatform(expiryPlatform)
if err != nil {
return fmt.Errorf("failed to parse platform %s: %w", expiryPlatform, err)
}
if parsedPlatform.Equals(*platform) {
expiries = append(expiries, expiry)
break
}
Copy link
Member

Choose a reason for hiding this comment

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

Is the intent here to break the outer loop, like on line 358 above? You can name the loop with a label and break label.

Assuming I've got that right, I think this answers my previous question - if every time we append to expiries we also break the loop over key.Expiries, the new key.Expiries will only ever have 0 or 1 entries. Maybe we can instead store the only match in another field so that the usage is clearer?

Along the same lines, it might be better not to break, and instead error on multiple matches. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've refactored this whole abstract because it stinks :)

}
}
}
}
// if this is empty, then the time check later will fail on the key
key.Expiries = expiries
}
return nil
}

// because we don't control the signature here (blame rego)
// nolint:gocritic
func (regoOpts *RegoFnOpts) internalParseLibraryDefinition(_ rego.BuiltinContext, definitionTerm *ast.Term) (*ast.Term, error) {
Expand Down
97 changes: 95 additions & 2 deletions policy/rego_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package policy
import (
"context"
"testing"
"time"

"github.com/docker/attest/attestation"
v1 "github.com/google/go-containerregistry/pkg/v1"
Expand Down Expand Up @@ -68,14 +69,19 @@ func buffer[T any](ch chan T) []T {
}

type NullAttestationResolver struct {
called bool
called bool
imageName string
platform *v1.Platform
}

func (r *NullAttestationResolver) ImageName(_ context.Context) (string, error) {
return "", nil
return r.imageName, nil
}

func (r *NullAttestationResolver) ImagePlatform(_ context.Context) (*v1.Platform, error) {
if r.platform != nil {
return r.platform, nil
}
return v1.ParsePlatform("")
}

Expand All @@ -87,3 +93,90 @@ func (r *NullAttestationResolver) Attestations(_ context.Context, _ string) ([]*
r.called = true
return nil, nil
}

func TestRegoFnOpts_filterRepoExpiries(t *testing.T) {
now := time.Now()
tests := []struct {
name string
imageName string
key *attestation.KeyMetadata
wantErr bool
expiryRemoved bool
keyKept bool
}{
{name: "no custom expirey", key: &attestation.KeyMetadata{}, keyKept: true},
{name: "no custom expirey", key: &attestation.KeyMetadata{
Expiries: []*attestation.KeyExpiry{},
}, keyKept: true},
{name: "malformed pattern", key: &attestation.KeyMetadata{
Expiries: []*attestation.KeyExpiry{
{Patterns: []string{"[]"}, To: &now},
},
}, wantErr: true},
{name: "missing pattern", key: &attestation.KeyMetadata{
Expiries: []*attestation.KeyExpiry{
{To: &now},
},
}, wantErr: true},
{name: "no matching image", key: &attestation.KeyMetadata{
Expiries: []*attestation.KeyExpiry{
{Patterns: []string{"bar"}, To: &now},
},
}, expiryRemoved: true},
{name: "matching image, no platforms", key: &attestation.KeyMetadata{
Expiries: []*attestation.KeyExpiry{
{Patterns: []string{"foo"}, To: &now},
},
}},
{name: "matching image, wrong platform", key: &attestation.KeyMetadata{
Expiries: []*attestation.KeyExpiry{
{Patterns: []string{"foo"}, Platforms: []string{"linux/arm64"}, To: &now},
},
}, expiryRemoved: true},
{name: "matching image, matching platform", key: &attestation.KeyMetadata{
Expiries: []*attestation.KeyExpiry{
{Patterns: []string{"foo"}, Platforms: []string{"linux/amd64"}, To: &now},
},
}},
{name: "matching canonical image, matching platform", key: &attestation.KeyMetadata{
Expiries: []*attestation.KeyExpiry{
{Patterns: []string{"^docker.io/library/foo$"}, Platforms: []string{"linux/amd64"}, To: &now},
},
}},
{name: "matching image, matching platform (on of many)", key: &attestation.KeyMetadata{
Expiries: []*attestation.KeyExpiry{
{Patterns: []string{"foo"}, Platforms: []string{"linux/amd64", "linux/arm64"}, To: &now},
},
}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
imageName := tt.imageName
if imageName == "" {
imageName = "foo"
}
regoOpts := &RegoFnOpts{
attestationResolver: &NullAttestationResolver{
imageName: imageName,
platform: &v1.Platform{OS: "linux", Architecture: "amd64"},
},
}

opts := &attestation.VerifyOptions{
Keys: attestation.Keys{tt.key},
}
if err := regoOpts.filterRepoExpiries(context.Background(), opts); (err != nil) != tt.wantErr {
t.Fatalf("RegoFnOpts.filterRepoExpiries() error = %v, wantErr %v", err, tt.wantErr)
} else {
switch {
case tt.expiryRemoved:
assert.Empty(t, opts.Keys[0].Expiries)
case tt.keyKept:
assert.NotEmpty(t, opts.Keys)
default:
assert.NotEmpty(t, opts.Keys[0].Expiries)
}
}
})
}
}
19 changes: 12 additions & 7 deletions policy/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,12 @@ type Options struct {
AttestationStyle mapping.AttestationStyle
Debug bool
AttestationVerifier attestation.Verifier
// extra parameters to pass through to rego as policy inputs
Parameters *Parameters
}

type Parameters map[string]string

type Policy struct {
InputFiles []*File
Query string
Expand All @@ -50,13 +54,14 @@ type Policy struct {
}

type Input struct {
Digest string `json:"digest"`
PURL string `json:"purl"`
Tag string `json:"tag,omitempty"`
Domain string `json:"domain"`
NormalizedName string `json:"normalized_name"`
FamiliarName string `json:"familiar_name"`
Platform string `json:"platform"`
Digest string `json:"digest"`
PURL string `json:"purl"`
Tag string `json:"tag,omitempty"`
Domain string `json:"domain"`
NormalizedName string `json:"normalized_name"`
FamiliarName string `json:"familiar_name"`
Platform string `json:"platform"`
Parameters *Parameters `json:"parameters"`
jonnystoten marked this conversation as resolved.
Show resolved Hide resolved
}

type File struct {
Expand Down
4 changes: 2 additions & 2 deletions signerverifier/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ const pemType = "PUBLIC KEY"
func ParsePublicKey(pubkeyBytes []byte) (crypto.PublicKey, error) {
p, _ := pem.Decode(pubkeyBytes)
if p == nil {
return nil, fmt.Errorf("pubkey file does not contain any PEM data")
return nil, fmt.Errorf("pubkeyBytes does not contain any PEM data")
}
if p.Type != pemType {
return nil, fmt.Errorf("pubkey file does not contain a public key")
return nil, fmt.Errorf("pubkeyBytes does not contain a public key")
}
return x509.ParsePKIXPublicKey(p.Bytes)
}
Expand Down
1 change: 1 addition & 0 deletions test/testdata/expires/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
config.yaml
14 changes: 14 additions & 0 deletions test/testdata/expires/mapping.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@

version: v1
kind: policy-mapping
policies:
- id: test
description: "Example of dual policy with per repo key expirey"
files:
- path: policy.rego
- path: config.yaml #auto generated
attestations:
style: attached
rules:
- pattern: "^docker[.]io/library/.*$"
policy-id: test
Loading
Loading