Skip to content

[WIP] verify signature for untagged references #129

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.

Sign up for GitHub

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

Closed
runcom wants to merge 1 commit into from
Closed

[WIP] verify signature for untagged references #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 13 additions & 25 deletions docs/policy.json.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,13 +146,25 @@ This requirement requires an image to be signed with an expected identity, or ac
"keyType": "GPGKeys", /* The only currently supported value */
"keyPath": "/path/to/local/keyring/file",
"keyData": "base64-encoded-keyring-data",
"signedIdentity": identity_requirement
"signedIdentity": identity_requirement,
"referencesByDigest": refs_by_digest_requirement
}
```
<!-- Later: other keyType values -->

Exactly one of `keyPath` and `keyData` must be present, containing a GPG keyring of one or more public keys. Only signatures made by these keys are accepted.

The `referencesByDigest` field controls whether an reference with a digest should be taken into consideration when checking signatures or be rejected altogheter.
Copy link
Collaborator

@mtrmac mtrmac Oct 24, 2016

Choose a reason for hiding this comment

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

This seems unclear to me, the user does not have the context of our PR. What is “reference” and what is “taken into consideration”?

Perhaps something like

The referencesByDigest field controls treatment of images which are referred to (pulled, named, etc.) using a digest reference (e.g. busybox@sha256:… instead of a tag (busybox:latest).

One of the following alternatives are supported:

- References by digest are allowed (default). In this case, the digest should match the one from the signature for the verification to be successful.
```json
{"referencesByDigest: allow"}
Copy link
Collaborator

@mtrmac mtrmac Oct 24, 2016

Choose a reason for hiding this comment

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

The {…} form for signedIdentity is used because a signedIdentity value is such an object; but the above is not a valid "type":"signedBy" object. Perhaps, just use a bulleted list like - "allow": Reference by digest are …

```
- References by digest are rejected and no signature verification is performed.
Copy link
Collaborator

@mtrmac mtrmac Oct 24, 2016

Choose a reason for hiding this comment

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

s,and no signature verification is preformed,regardless of the existence and/or contents of signatures, if any; users must refer to images using a tag, ? “no signature verification is performed” could be misinterpreted to mean that something is let through.

Copy link
Collaborator

@mtrmac mtrmac Oct 24, 2016

Choose a reason for hiding this comment

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

Nothing actually seems to implement it that way???! This ends up being used in the various if byDigest conditionals, at the end of both branches there is always an extra comparison. (BTW I’d expect a single enforcement point in prSignedBy.isSignatureAuthorAccepted, perhaps even before doing any cryptography.)

```json
{"referencesByDigest: reject"}
```
The `signedIdentity` field, a JSON object, specifies what image identity the signature claims about the image.
One of the following alternatives are supported:

Expand Down Expand Up @@ -257,27 +269,3 @@ selectively allow individual transports and scopes as desired.
"default": [{"type": "insecureAcceptAnything"}]
}
```

### A _temporary_ work-around to allow accessing any image by digest

Usually, identities in signatures use the _repository_`:`_tag_ format,
which is not matched when pulling a specific image using a digest.
To allow such operations, a policy may set `signedIdentity` to `matchRepository`, similar
to the following fragment:

```json
"hostname:5000/allow/pull/by/tag": [
{
"type": "signedBy",
"keyType": "GPGKeys",
"keyPath": "/path/to/some.gpg"
"signedIdentity": {
"type": "matchRepository"
}
}
]
```

*Warning*: This completely turns off tag matching for the signature check in question,
allowing also pulls by tag to accept signatures for any other tag.
A more granular solution for this situation will be provided in the future ( https://github.com/containers/image/issues/99 ).
2 changes: 1 addition & 1 deletion signature/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func VerifyDockerManifestSignature(unverifiedSignature, unverifiedManifest []byt
}
return nil
},
validateSignedDockerReference: func(signedDockerReference string) error {
validateSignedDockerReference: func(signedDockerReference, digest string) error {
if signedDockerReference != expectedDockerReference {
return InvalidSignatureError{msg: fmt.Sprintf("Docker reference %s does not match %s",
signedDockerReference, expectedDockerReference)}
Expand Down
44 changes: 27 additions & 17 deletions signature/policy_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ func (pr *prReject) UnmarshalJSON(data []byte) error {
}

// newPRSignedBy returns a new prSignedBy if parameters are valid.
func newPRSignedBy(keyType sbKeyType, keyPath string, keyData []byte, signedIdentity PolicyReferenceMatch) (*prSignedBy, error) {
func newPRSignedBy(keyType sbKeyType, keyPath string, keyData []byte, signedIdentity PolicyReferenceMatch, referencesByDigest sbUntaggedReferenceMatch) (*prSignedBy, error) {
if !keyType.IsValid() {
return nil, InvalidPolicyFormatError(fmt.Sprintf("invalid keyType \"%s\"", keyType))
}
Expand All @@ -323,33 +323,37 @@ func newPRSignedBy(keyType sbKeyType, keyPath string, keyData []byte, signedIden
if signedIdentity == nil {
return nil, InvalidPolicyFormatError("signedIdentity not specified")
}
if referencesByDigest != SBKeyTypeUntaggedReferenceAllow && referencesByDigest != SBKeyTypeUntaggedReferenceReject {
return nil, InvalidPolicyFormatError("referencesByDigest accepts either allow or reject")
}
Copy link
Collaborator

@mtrmac mtrmac Oct 24, 2016

Choose a reason for hiding this comment

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

A test, please.

return &prSignedBy{
prCommon: prCommon{Type: prTypeSignedBy},
KeyType: keyType,
KeyPath: keyPath,
KeyData: keyData,
SignedIdentity: signedIdentity,
prCommon: prCommon{Type: prTypeSignedBy},
KeyType: keyType,
KeyPath: keyPath,
KeyData: keyData,
SignedIdentity: signedIdentity,
ReferencesByDigest: referencesByDigest,
Copy link
Collaborator

@mtrmac mtrmac Oct 24, 2016

Choose a reason for hiding this comment

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

A test, please. (I would expect TestNewPRSignedBy to fail right now.)

}, nil
}

// newPRSignedByKeyPath is NewPRSignedByKeyPath, except it returns the private type.
func newPRSignedByKeyPath(keyType sbKeyType, keyPath string, signedIdentity PolicyReferenceMatch) (*prSignedBy, error) {
return newPRSignedBy(keyType, keyPath, nil, signedIdentity)
func newPRSignedByKeyPath(keyType sbKeyType, keyPath string, signedIdentity PolicyReferenceMatch, referencesByDigest sbUntaggedReferenceMatch) (*prSignedBy, error) {
return newPRSignedBy(keyType, keyPath, nil, signedIdentity, referencesByDigest)
}

// NewPRSignedByKeyPath returns a new "signedBy" PolicyRequirement using a KeyPath
func NewPRSignedByKeyPath(keyType sbKeyType, keyPath string, signedIdentity PolicyReferenceMatch) (PolicyRequirement, error) {
return newPRSignedByKeyPath(keyType, keyPath, signedIdentity)
func NewPRSignedByKeyPath(keyType sbKeyType, keyPath string, signedIdentity PolicyReferenceMatch, referencesByDigest sbUntaggedReferenceMatch) (PolicyRequirement, error) {
return newPRSignedByKeyPath(keyType, keyPath, signedIdentity, referencesByDigest)
}

// newPRSignedByKeyData is NewPRSignedByKeyData, except it returns the private type.
func newPRSignedByKeyData(keyType sbKeyType, keyData []byte, signedIdentity PolicyReferenceMatch) (*prSignedBy, error) {
return newPRSignedBy(keyType, "", keyData, signedIdentity)
func newPRSignedByKeyData(keyType sbKeyType, keyData []byte, signedIdentity PolicyReferenceMatch, referencesByDigest sbUntaggedReferenceMatch) (*prSignedBy, error) {
return newPRSignedBy(keyType, "", keyData, signedIdentity, referencesByDigest)
}

// NewPRSignedByKeyData returns a new "signedBy" PolicyRequirement using a KeyData
func NewPRSignedByKeyData(keyType sbKeyType, keyData []byte, signedIdentity PolicyReferenceMatch) (PolicyRequirement, error) {
return newPRSignedByKeyData(keyType, keyData, signedIdentity)
func NewPRSignedByKeyData(keyType sbKeyType, keyData []byte, signedIdentity PolicyReferenceMatch, referencesByDigest sbUntaggedReferenceMatch) (PolicyRequirement, error) {
return newPRSignedByKeyData(keyType, keyData, signedIdentity, referencesByDigest)
}

// Compile-time check that prSignedBy implements json.Unmarshaler.
Expand All @@ -375,6 +379,8 @@ func (pr *prSignedBy) UnmarshalJSON(data []byte) error {
return &tmp.KeyData
case "signedIdentity":
return &signedIdentity
case "referencesByDigest":
return &tmp.ReferencesByDigest
Copy link
Collaborator

@mtrmac mtrmac Oct 24, 2016

Choose a reason for hiding this comment

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

A test, please. (Invalid value, duplicated value)

default:
return nil
}
Expand All @@ -395,15 +401,19 @@ func (pr *prSignedBy) UnmarshalJSON(data []byte) error {
tmp.SignedIdentity = si
}

if tmp.ReferencesByDigest == "" {
tmp.ReferencesByDigest = SBKeyTypeUntaggedReferenceAllow
}
Copy link
Collaborator

@mtrmac mtrmac Oct 24, 2016

Choose a reason for hiding this comment

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

A test, please.


var res *prSignedBy
var err error
switch {
case gotKeyPath && gotKeyData:
return InvalidPolicyFormatError("keyPath and keyData cannot be used simultaneously")
case gotKeyPath && !gotKeyData:
res, err = newPRSignedByKeyPath(tmp.KeyType, tmp.KeyPath, tmp.SignedIdentity)
res, err = newPRSignedByKeyPath(tmp.KeyType, tmp.KeyPath, tmp.SignedIdentity, tmp.ReferencesByDigest)
case !gotKeyPath && gotKeyData:
res, err = newPRSignedByKeyData(tmp.KeyType, tmp.KeyData, tmp.SignedIdentity)
res, err = newPRSignedByKeyData(tmp.KeyType, tmp.KeyData, tmp.SignedIdentity, tmp.ReferencesByDigest)
case !gotKeyPath && !gotKeyData:
return InvalidPolicyFormatError("At least one of keyPath and keyData mus be specified")
default: // Coverage: This should never happen
Expand Down Expand Up @@ -501,7 +511,7 @@ func (pr *prSignedBaseLayer) UnmarshalJSON(data []byte) error {
return nil
}

// newPolicyRequirementFromJSON parses JSON data into a PolicyReferenceMatch implementation.
// newPolicyReferenceFromJSON parses JSON data into a PolicyReferenceMatch implementation.
Copy link
Collaborator

@mtrmac mtrmac Oct 24, 2016

Choose a reason for hiding this comment

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

Better (thanks!), but still misses a Match

func newPolicyReferenceMatchFromJSON(data []byte) (PolicyReferenceMatch, error) {
var typeField prmCommon
if err := json.Unmarshal(data, &typeField); err != nil {
Expand Down
26 changes: 13 additions & 13 deletions signature/policy_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func tryUnmarshalModifiedPolicy(t *testing.T, p *Policy, validJSON []byte, modif

// xNewPRSignedByKeyPath is like NewPRSignedByKeyPath, except it must not fail.
func xNewPRSignedByKeyPath(keyType sbKeyType, keyPath string, signedIdentity PolicyReferenceMatch) PolicyRequirement {
pr, err := NewPRSignedByKeyPath(keyType, keyPath, signedIdentity)
pr, err := NewPRSignedByKeyPath(keyType, keyPath, signedIdentity, SBKeyTypeUntaggedReferenceAllow)
if err != nil {
panic("xNewPRSignedByKeyPath failed")
}
Expand All @@ -213,7 +213,7 @@ func xNewPRSignedByKeyPath(keyType sbKeyType, keyPath string, signedIdentity Pol

// xNewPRSignedByKeyData is like NewPRSignedByKeyData, except it must not fail.
func xNewPRSignedByKeyData(keyType sbKeyType, keyData []byte, signedIdentity PolicyReferenceMatch) PolicyRequirement {
pr, err := NewPRSignedByKeyData(keyType, keyData, signedIdentity)
pr, err := NewPRSignedByKeyData(keyType, keyData, signedIdentity, SBKeyTypeUntaggedReferenceAllow)
if err != nil {
panic("xNewPRSignedByKeyData failed")
}
Expand Down Expand Up @@ -635,7 +635,7 @@ func TestNewPRSignedBy(t *testing.T) {
testIdentity := NewPRMMatchExact()

// Success
pr, err := newPRSignedBy(SBKeyTypeGPGKeys, testPath, nil, testIdentity)
pr, err := newPRSignedBy(SBKeyTypeGPGKeys, testPath, nil, testIdentity, SBKeyTypeUntaggedReferenceAllow)
require.NoError(t, err)
assert.Equal(t, &prSignedBy{
prCommon: prCommon{prTypeSignedBy},
Expand All @@ -644,7 +644,7 @@ func TestNewPRSignedBy(t *testing.T) {
KeyData: nil,
SignedIdentity: testIdentity,
}, pr)
pr, err = newPRSignedBy(SBKeyTypeGPGKeys, "", testData, testIdentity)
pr, err = newPRSignedBy(SBKeyTypeGPGKeys, "", testData, testIdentity, SBKeyTypeUntaggedReferenceAllow)
require.NoError(t, err)
assert.Equal(t, &prSignedBy{
prCommon: prCommon{prTypeSignedBy},
Expand All @@ -655,23 +655,23 @@ func TestNewPRSignedBy(t *testing.T) {
}, pr)

// Invalid keyType
pr, err = newPRSignedBy(sbKeyType(""), testPath, nil, testIdentity)
pr, err = newPRSignedBy(sbKeyType(""), testPath, nil, testIdentity, SBKeyTypeUntaggedReferenceAllow)
assert.Error(t, err)
pr, err = newPRSignedBy(sbKeyType("this is invalid"), testPath, nil, testIdentity)
pr, err = newPRSignedBy(sbKeyType("this is invalid"), testPath, nil, testIdentity, SBKeyTypeUntaggedReferenceAllow)
assert.Error(t, err)

// Both keyPath and keyData specified
pr, err = newPRSignedBy(SBKeyTypeGPGKeys, testPath, testData, testIdentity)
pr, err = newPRSignedBy(SBKeyTypeGPGKeys, testPath, testData, testIdentity, SBKeyTypeUntaggedReferenceAllow)
assert.Error(t, err)

// Invalid signedIdentity
pr, err = newPRSignedBy(SBKeyTypeGPGKeys, testPath, nil, nil)
pr, err = newPRSignedBy(SBKeyTypeGPGKeys, testPath, nil, nil, SBKeyTypeUntaggedReferenceAllow)
assert.Error(t, err)
}

func TestNewPRSignedByKeyPath(t *testing.T) {
const testPath = "/foo/bar"
_pr, err := NewPRSignedByKeyPath(SBKeyTypeGPGKeys, testPath, NewPRMMatchExact())
_pr, err := NewPRSignedByKeyPath(SBKeyTypeGPGKeys, testPath, NewPRMMatchExact(), SBKeyTypeUntaggedReferenceAllow)
require.NoError(t, err)
pr, ok := _pr.(*prSignedBy)
require.True(t, ok)
Expand All @@ -681,7 +681,7 @@ func TestNewPRSignedByKeyPath(t *testing.T) {

func TestNewPRSignedByKeyData(t *testing.T) {
testData := []byte("abc")
_pr, err := NewPRSignedByKeyData(SBKeyTypeGPGKeys, testData, NewPRMMatchExact())
_pr, err := NewPRSignedByKeyData(SBKeyTypeGPGKeys, testData, NewPRMMatchExact(), SBKeyTypeUntaggedReferenceAllow)
require.NoError(t, err)
pr, ok := _pr.(*prSignedBy)
require.True(t, ok)
Expand Down Expand Up @@ -710,7 +710,7 @@ func TestPRSignedByUnmarshalJSON(t *testing.T) {
testInvalidJSONInput(t, &pr)

// Start with a valid JSON.
validPR, err := NewPRSignedByKeyData(SBKeyTypeGPGKeys, []byte("abc"), NewPRMMatchExact())
validPR, err := NewPRSignedByKeyData(SBKeyTypeGPGKeys, []byte("abc"), NewPRMMatchExact(), SBKeyTypeUntaggedReferenceAllow)
require.NoError(t, err)
validJSON, err := json.Marshal(validPR)
require.NoError(t, err)
Expand All @@ -722,7 +722,7 @@ func TestPRSignedByUnmarshalJSON(t *testing.T) {
assert.Equal(t, validPR, &pr)

// Success with KeyPath
kpPR, err := NewPRSignedByKeyPath(SBKeyTypeGPGKeys, "/foo/bar", NewPRMMatchExact())
kpPR, err := NewPRSignedByKeyPath(SBKeyTypeGPGKeys, "/foo/bar", NewPRMMatchExact(), SBKeyTypeUntaggedReferenceAllow)
require.NoError(t, err)
testJSON, err := json.Marshal(kpPR)
require.NoError(t, err)
Expand Down Expand Up @@ -782,7 +782,7 @@ func TestPRSignedByUnmarshalJSON(t *testing.T) {
assert.Error(t, err)
}
// Handle "keyPath", which is not in validJSON, specially
pathPR, err := NewPRSignedByKeyPath(SBKeyTypeGPGKeys, "/foo/bar", NewPRMMatchExact())
pathPR, err := NewPRSignedByKeyPath(SBKeyTypeGPGKeys, "/foo/bar", NewPRMMatchExact(), SBKeyTypeUntaggedReferenceAllow)
require.NoError(t, err)
testJSON, err = json.Marshal(pathPR)
require.NoError(t, err)
Expand Down
5 changes: 4 additions & 1 deletion signature/policy_eval.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ type PolicyReferenceMatch interface {
// matchesDockerReference decides whether a specific image identity is accepted for an image
// (or, usually, for the image's Reference().DockerReference()). Note that
// image.Reference().DockerReference() may be nil.
matchesDockerReference(image types.UnparsedImage, signatureDockerReference string) bool
//
// TODO(runcom): document byDigest
// TODO(runcom): document digest
matchesDockerReference(image types.UnparsedImage, signatureDockerReference, signatureManifstDigest string, byDigest bool) bool
Copy link
Collaborator

@mtrmac mtrmac Oct 24, 2016

Choose a reason for hiding this comment

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

Yes, document. I’ll continue reading the code to figure this out, but the intent needs to be clearly stated in words as well.

}

// PolicyContext encapsulates a policy and possible cached state
Expand Down
4 changes: 2 additions & 2 deletions signature/policy_eval_signedby.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ func (pr *prSignedBy) isSignatureAuthorAccepted(image types.UnparsedImage, sig [
// not be reachable.
return PolicyRequirementError(fmt.Sprintf("Signature by key %s is not accepted", keyIdentity))
},
validateSignedDockerReference: func(ref string) error {
if !pr.SignedIdentity.matchesDockerReference(image, ref) {
validateSignedDockerReference: func(ref, digest string) error {
if !pr.SignedIdentity.matchesDockerReference(image, ref, digest, pr.ReferencesByDigest == SBKeyTypeUntaggedReferenceAllow) {
return PolicyRequirementError(fmt.Sprintf("Signature for identity %s is not accepted", ref))
}
return nil
Expand Down
Loading