diff --git a/signature/docker.go b/signature/docker.go index eb575c06..223385c5 100644 --- a/signature/docker.go +++ b/signature/docker.go @@ -28,16 +28,33 @@ func SignDockerManifest(manifest []byte, dockerReference string, mech SigningMec // using mech. func VerifyDockerManifestSignature(unverifiedSignature, unverifiedManifest []byte, expectedDockerReference string, mech SigningMechanism, expectedKeyIdentity string) (*Signature, error) { - expectedManifestDigest, err := utils.ManifestDigest(unverifiedManifest) + sig, err := verifyAndExtractSignature(mech, unverifiedSignature, signatureAcceptanceRules{ + validateKeyIdentity: func(keyIdentity string) error { + if keyIdentity != expectedKeyIdentity { + return InvalidSignatureError{msg: fmt.Sprintf("Signature by %s does not match expected fingerprint %s", keyIdentity, expectedKeyIdentity)} + } + return nil + }, + validateSignedDockerReference: func(signedDockerReference string) error { + if signedDockerReference != expectedDockerReference { + return InvalidSignatureError{msg: fmt.Sprintf("Docker reference %s does not match %s", + signedDockerReference, expectedDockerReference)} + } + return nil + }, + validateSignedDockerManifestDigest: func(signedDockerManifestDigest string) error { + matches, err := utils.ManifestMatchesDigest(unverifiedManifest, signedDockerManifestDigest) + if err != nil { + return err + } + if !matches { + return InvalidSignatureError{msg: fmt.Sprintf("Signature for docker digest %s does not match", signedDockerManifestDigest, signedDockerManifestDigest)} + } + return nil + }, + }) if err != nil { return nil, err } - sig, err := verifyAndExtractSignature(mech, unverifiedSignature, expectedKeyIdentity, expectedDockerReference) - if err != nil { - return nil, err - } - if sig.DockerManifestDigest != expectedManifestDigest { - return nil, InvalidSignatureError{msg: fmt.Sprintf("Docker manifest digest %s does not match %s", sig.DockerManifestDigest, expectedManifestDigest)} - } return sig, nil } diff --git a/signature/docker_test.go b/signature/docker_test.go index ec4c858d..766fb036 100644 --- a/signature/docker_test.go +++ b/signature/docker_test.go @@ -72,6 +72,11 @@ func TestVerifyDockerManifestSignature(t *testing.T) { assert.Error(t, err) assert.Nil(t, sig) + // Docker reference mismatch + sig, err = VerifyDockerManifestSignature(signature, manifest, "example.com/doesnt/match", mech, TestKeyFingerprint) + assert.Error(t, err) + assert.Nil(t, sig) + // Docker manifest digest mismatch sig, err = VerifyDockerManifestSignature(signature, []byte("unexpected manifest"), TestImageSignatureReference, mech, TestKeyFingerprint) assert.Error(t, err) diff --git a/signature/fixtures/dir-img-manifest-digest-error/manifest.json b/signature/fixtures/dir-img-manifest-digest-error/manifest.json new file mode 120000 index 00000000..3dee14b4 --- /dev/null +++ b/signature/fixtures/dir-img-manifest-digest-error/manifest.json @@ -0,0 +1 @@ +../v2s1-invalid-signatures.manifest.json \ No newline at end of file diff --git a/signature/fixtures/dir-img-manifest-digest-error/signature-1 b/signature/fixtures/dir-img-manifest-digest-error/signature-1 new file mode 120000 index 00000000..f010fd4c --- /dev/null +++ b/signature/fixtures/dir-img-manifest-digest-error/signature-1 @@ -0,0 +1 @@ +../dir-img-valid/signature-1 \ No newline at end of file diff --git a/signature/fixtures/dir-img-mixed/manifest.json b/signature/fixtures/dir-img-mixed/manifest.json new file mode 120000 index 00000000..ff7d2ffa --- /dev/null +++ b/signature/fixtures/dir-img-mixed/manifest.json @@ -0,0 +1 @@ +../dir-img-valid/manifest.json \ No newline at end of file diff --git a/signature/fixtures/dir-img-mixed/signature-1 b/signature/fixtures/dir-img-mixed/signature-1 new file mode 120000 index 00000000..b27cdc45 --- /dev/null +++ b/signature/fixtures/dir-img-mixed/signature-1 @@ -0,0 +1 @@ +../invalid-blob.signature \ No newline at end of file diff --git a/signature/fixtures/dir-img-mixed/signature-2 b/signature/fixtures/dir-img-mixed/signature-2 new file mode 120000 index 00000000..f010fd4c --- /dev/null +++ b/signature/fixtures/dir-img-mixed/signature-2 @@ -0,0 +1 @@ +../dir-img-valid/signature-1 \ No newline at end of file diff --git a/signature/fixtures/dir-img-modified-manifest/manifest.json b/signature/fixtures/dir-img-modified-manifest/manifest.json new file mode 100644 index 00000000..82fde381 --- /dev/null +++ b/signature/fixtures/dir-img-modified-manifest/manifest.json @@ -0,0 +1,27 @@ +{ + "schemaVersion": 2, + "mediaType": "application/vnd.docker.distribution.manifest.v2+json", + "config": { + "mediaType": "application/vnd.docker.container.image.v1+json", + "size": 7023, + "digest": "sha256:b5b2b2c507a0944348e0303114d8d93aaaa081732b86451d9bce1f432a537bc7" + }, + "layers": [ + { + "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip", + "size": 32654, + "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f" + }, + { + "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip", + "size": 16724, + "digest": "sha256:3c3a4604a545cdc127456d94e421cd355bca5b528f4a9c1905b15da2eb4a4c6b" + }, + { + "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip", + "size": 73109, + "digest": "sha256:ec4b8955958665577945c89419d1af06b5f7636b4ac3da7f12184802ad867736" + } + ], + "extra": "this manifest has been modified" +} diff --git a/signature/fixtures/dir-img-modified-manifest/signature-1 b/signature/fixtures/dir-img-modified-manifest/signature-1 new file mode 120000 index 00000000..f010fd4c --- /dev/null +++ b/signature/fixtures/dir-img-modified-manifest/signature-1 @@ -0,0 +1 @@ +../dir-img-valid/signature-1 \ No newline at end of file diff --git a/signature/fixtures/dir-img-no-manifest/signature-1 b/signature/fixtures/dir-img-no-manifest/signature-1 new file mode 120000 index 00000000..f010fd4c --- /dev/null +++ b/signature/fixtures/dir-img-no-manifest/signature-1 @@ -0,0 +1 @@ +../dir-img-valid/signature-1 \ No newline at end of file diff --git a/signature/fixtures/dir-img-unsigned/manifest.json b/signature/fixtures/dir-img-unsigned/manifest.json new file mode 120000 index 00000000..ff7d2ffa --- /dev/null +++ b/signature/fixtures/dir-img-unsigned/manifest.json @@ -0,0 +1 @@ +../dir-img-valid/manifest.json \ No newline at end of file diff --git a/signature/fixtures/dir-img-valid-2/manifest.json b/signature/fixtures/dir-img-valid-2/manifest.json new file mode 120000 index 00000000..ff7d2ffa --- /dev/null +++ b/signature/fixtures/dir-img-valid-2/manifest.json @@ -0,0 +1 @@ +../dir-img-valid/manifest.json \ No newline at end of file diff --git a/signature/fixtures/dir-img-valid-2/signature-1 b/signature/fixtures/dir-img-valid-2/signature-1 new file mode 120000 index 00000000..f010fd4c --- /dev/null +++ b/signature/fixtures/dir-img-valid-2/signature-1 @@ -0,0 +1 @@ +../dir-img-valid/signature-1 \ No newline at end of file diff --git a/signature/fixtures/dir-img-valid-2/signature-2 b/signature/fixtures/dir-img-valid-2/signature-2 new file mode 100644 index 00000000..dbba8f42 Binary files /dev/null and b/signature/fixtures/dir-img-valid-2/signature-2 differ diff --git a/signature/fixtures/dir-img-valid/manifest.json b/signature/fixtures/dir-img-valid/manifest.json new file mode 120000 index 00000000..c5bd2543 --- /dev/null +++ b/signature/fixtures/dir-img-valid/manifest.json @@ -0,0 +1 @@ +../image.manifest.json \ No newline at end of file diff --git a/signature/fixtures/dir-img-valid/signature-1 b/signature/fixtures/dir-img-valid/signature-1 new file mode 100644 index 00000000..d0e18720 Binary files /dev/null and b/signature/fixtures/dir-img-valid/signature-1 differ diff --git a/signature/fixtures/unknown-key.signature b/signature/fixtures/unknown-key.signature new file mode 100644 index 00000000..2277b130 Binary files /dev/null and b/signature/fixtures/unknown-key.signature differ diff --git a/signature/mechanism_test.go b/signature/mechanism_test.go index 8617b6d2..e6cbc0a7 100644 --- a/signature/mechanism_test.go +++ b/signature/mechanism_test.go @@ -138,5 +138,12 @@ func TestGPGSigningMechanismVerify(t *testing.T) { require.NoError(t, err) content, signingFingerprint, err = mech.Verify(signature) assertSigningError(t, content, signingFingerprint, err) + + // Valid signature with an unknown key + signature, err = ioutil.ReadFile("./fixtures/unknown-key.signature") + require.NoError(t, err) + content, signingFingerprint, err = mech.Verify(signature) + assertSigningError(t, content, signingFingerprint, err) + // The various GPG/GPGME failures cases are not obviously easy to reach. } diff --git a/signature/policy_config.go b/signature/policy_config.go index fd634783..7fc5a382 100644 --- a/signature/policy_config.go +++ b/signature/policy_config.go @@ -88,10 +88,9 @@ func (m *policySpecificMap) UnmarshalJSON(data []byte) error { // So, use a temporary map of pointers-to-slices and convert. tmpMap := map[string]*PolicyRequirements{} if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - // Check that the scope format is at least plausible. - if _, err := reference.ParseNamed(key); err != nil { - return nil // FIXME? This returns an "Unknown key" error instead of saying that the format is invalid. - } + // FIXME? We might want to validate the scope format. + // Note that reference.ParseNamed is unsuitable; it would understand "example.com" as + // "docker.io/library/example.com" // paranoidUnmarshalJSONObject detects key duplication for us, check just to be safe. if _, ok := tmpMap[key]; ok { return nil diff --git a/signature/policy_config_test.go b/signature/policy_config_test.go index 63fd8c74..23c2f1a6 100644 --- a/signature/policy_config_test.go +++ b/signature/policy_config_test.go @@ -160,7 +160,7 @@ func TestPolicyUnmarshalJSON(t *testing.T) { xNewPRSignedByKeyData(SBKeyTypeGPGKeys, []byte("abc"), NewPRMMatchExact()), }, Specific: map[string]PolicyRequirements{ - "library/busybox": []PolicyRequirement{ + "docker.io/library/busybox": []PolicyRequirement{ xNewPRSignedByKeyData(SBKeyTypeGPGKeys, []byte("def"), NewPRMMatchExact()), }, "registry.access.redhat.com": []PolicyRequirement{ @@ -191,11 +191,8 @@ func TestPolicyUnmarshalJSON(t *testing.T) { func(v mSI) { v["specific"] = []string{} }, // "default" is an invalid PolicyRequirements func(v mSI) { v["default"] = PolicyRequirements{} }, - // Invalid scope name in "specific". Uppercase is invalid in Docker reference components. - // Get valid PolicyRequirements by copying them from "library/buxybox". - func(v mSI) { x(v, "specific")["INVALIDUPPERCASE"] = x(v, "specific")["library/busybox"] }, // A field in "specific" is an invalid PolicyRequirements - func(v mSI) { x(v, "specific")["library/busybox"] = PolicyRequirements{} }, + func(v mSI) { x(v, "specific")["docker.io/library/busybox"] = PolicyRequirements{} }, } for _, fn := range breakFns { err = tryUnmarshalModifiedPolicy(t, &p, validJSON, fn) diff --git a/signature/policy_eval.go b/signature/policy_eval.go new file mode 100644 index 00000000..c62328ed --- /dev/null +++ b/signature/policy_eval.go @@ -0,0 +1,334 @@ +// This defines the top-level policy evaluation API. +// To the extent possible, the interface of the fuctions provided +// here is intended to be completely unambiguous, and stable for users +// to rely on. + +package signature + +import ( + "fmt" + "strings" + + "github.com/Sirupsen/logrus" + distreference "github.com/docker/distribution/reference" + "github.com/projectatomic/skopeo/reference" + "github.com/projectatomic/skopeo/types" +) + +// PolicyRequirementError is an explanatory text for rejecting a signature or an image. +type PolicyRequirementError string + +func (err PolicyRequirementError) Error() string { + return string(err) +} + +// signatureAcceptanceResult is the principal value returned by isSignatureAuthorAccepted. +type signatureAcceptanceResult string + +const ( + sarAccepted signatureAcceptanceResult = "sarAccepted" + sarRejected signatureAcceptanceResult = "sarRejected" + sarUnknown signatureAcceptanceResult = "sarUnknown" +) + +// PolicyRequirement is a rule which must be satisfied by at least one of the signatures of an image. +// The type is public, but its definition is private. +type PolicyRequirement interface { + // FIXME: For speed, we should support creating per-context state (not stored in the PolicyRequirement), to cache + // costly initialization like creating temporary GPG home directories and reading files. + // Setup() (someState, error) + // Then, the operations below would be done on the someState object, not directly on a PolicyRequirement. + + // isSignatureAuthorAccepted, given an image and a signature blob, returns: + // - sarAccepted if the signature has been verified against the appropriate public key + // (where "appropriate public key" may depend on the contents of the signature); + // in that case a parsed Signature should be returned. + // - sarRejected if the signature has not been verified; + // in that case error must be non-nil, and should be an PolicyRequirementError if evaluation + // succeeded but the result was rejection. + // - sarUnknown if if this PolicyRequirement does not deal with signatures. + // NOTE: sarUnknown should not be returned if this PolicyRequirement should make a decision but something failed. + // Returning sarUnknown and a non-nil error value is invalid. + // WARNING: This makes the signature contents acceptable for futher processing, + // but it does not necessarily mean that the contents of the signature are + // consistent with local policy. + // For example: + // - Do not use a true value to determine whether to run + // a container based on this image; use IsRunningImageAllowed instead. + // - Just because a signature is accepted does not automatically mean the contents of the + // signature are authorized to run code as root, or to affect system or cluster configuration. + isSignatureAuthorAccepted(image types.Image, sig []byte) (signatureAcceptanceResult, *Signature, error) + + // isRunningImageAllowed returns true if the requirement allows running an image. + // If it returns false, err must be non-nil, and should be an PolicyRequirementError if evaluation + // succeeded but the result was rejection. + // WARNING: This validates signatures and the manifest, but does not download or validate the + // layers. Users must validate that the layers match their expected digests. + isRunningImageAllowed(image types.Image) (bool, error) +} + +// PolicyReferenceMatch specifies a set of image identities accepted in PolicyRequirement. +// The type is public, but its implementation is private. +type PolicyReferenceMatch interface { + // matchesDockerReference decides whether a specific image identity is accepted for an image + // (or, usually, for the image's IntendedDockerReference()), + matchesDockerReference(image types.Image, signatureDockerReference string) bool +} + +// PolicyContext encapsulates a policy and possible cached state +// for speeding up its evaluation. +type PolicyContext struct { + Policy *Policy + state policyContextState // Internal consistency checking +} + +// policyContextState is used internally to verify the users are not misusing a PolicyContext. +type policyContextState string + +const ( + pcInvalid policyContextState = "" + pcInitializing policyContextState = "Initializing" + pcReady policyContextState = "Ready" + pcInUse policyContextState = "InUse" + pcDestroying policyContextState = "Destroying" + pcDestroyed policyContextState = "Destroyed" +) + +// changeContextState changes pc.state, or fails if the state is unexpected +func (pc *PolicyContext) changeState(expected, new policyContextState) error { + if pc.state != expected { + return fmt.Errorf(`"Invalid PolicyContext state, expected "%s", found "%s"`, expected, pc.state) + } + pc.state = new + return nil +} + +// NewPolicyContext sets up and initializes a context for the specified policy. +// The policy must not be modified while the context exists. FIXME: make a deep copy? +// If this function succeeds, the caller should call PolicyContext.Destroy() when done. +func NewPolicyContext(policy *Policy) (*PolicyContext, error) { + pc := &PolicyContext{Policy: policy, state: pcInitializing} + // FIXME: initialize + if err := pc.changeState(pcInitializing, pcReady); err != nil { + // Huh?! This should never fail, we didn't give the pointer to anybody. + // Just give up and leave unclean state around. + return nil, err + } + return pc, nil +} + +// Destroy should be called when the user of the context is done with it. +func (pc *PolicyContext) Destroy() error { + if err := pc.changeState(pcReady, pcDestroying); err != nil { + return err + } + // FIXME: destroy + return pc.changeState(pcDestroying, pcDestroyed) +} + +// fullyExpandedDockerReference converts a reference.Named into a fully expanded format; +// i.e. soft of an opposite to ref.String(), which is a fully canonicalized/minimized format. +// This is guaranteed to be the same as reference.FullName(), with a tag or digest appended, if available. +// FIXME? This feels like it should be provided by skopeo/reference. +func fullyExpandedDockerReference(ref reference.Named) (string, error) { + res := ref.FullName() + tagged, isTagged := ref.(distreference.Tagged) + digested, isDigested := ref.(distreference.Digested) + // A github.com/distribution/reference value can have a tag and a digest at the same time! + // skopeo/reference does not handle that, so fail. + // FIXME? Should we support that? + switch { + case isTagged && isDigested: + // Coverage: This should currently not happen, the way skopeo/reference sets up types, + // isTagged and isDigested is mutually exclusive. + return "", fmt.Errorf("Names with both a tag and digest are not currently supported") + case isTagged: + res = res + ":" + tagged.Tag() + case isDigested: + res = res + "@" + digested.Digest().String() + default: + // res is already OK. + } + return res, nil +} + +// requirementsForImage selects the appropriate requirements for image. +func (pc *PolicyContext) requirementsForImage(image types.Image) (PolicyRequirements, error) { + imageIdentity := image.IntendedDockerReference() + // We don't technically need to parse it first in order to match the full name:tag, + // but do so anyway to ensure that the intended identity really does follow that + // format, or at least that it is not demonstrably wrong. + ref, err := reference.ParseNamed(imageIdentity) + if err != nil { + return nil, err + } + ref = reference.WithDefaultTag(ref) + + // Look for a full match. + fullyExpanded, err := fullyExpandedDockerReference(ref) + if err != nil { // Coverage: This cannot currently happen. + return nil, err + } + if req, ok := pc.Policy.Specific[fullyExpanded]; ok { + logrus.Debugf(" Using specific policy section %s", fullyExpanded) + return req, nil + } + + // Look for a match of the repository, and then of the possible parent + // namespaces. Note that this only happens on the expanded host names + // and repository names, i.e. "busybox" is looked up as "docker.io/library/busybox", + // then in its parent "docker.io/library"; in none of "busybox", + // un-namespaced "library" nor in "" implicitly representing "library/". + // + // ref.FullName() == ref.Hostname() + "/" + ref.RemoteName(), so the last + // iteration matches the host name (for any namespace). + name := ref.FullName() + for { + if req, ok := pc.Policy.Specific[name]; ok { + logrus.Debugf(" Using specific policy section %s", name) + return req, nil + } + + lastSlash := strings.LastIndex(name, "/") + if lastSlash == -1 { + break + } + name = name[:lastSlash] + } + + logrus.Debugf(" Using default policy section") + return pc.Policy.Default, nil +} + +// GetSignaturesWithAcceptedAuthor returns those signatures from an image +// for which the policy accepts the author (and which have been successfully +// verified). +// NOTE: This may legitimately return an empty list and no error, if the image +// has no signatures or only invalid signatures. +// WARNING: This makes the signature contents acceptable for futher processing, +// but it does not necessarily mean that the contents of the signature are +// consistent with local policy. +// For example: +// - Do not use a an existence of an accepted signature to determine whether to run +// a container based on this image; use IsRunningImageAllowed instead. +// - Just because a signature is accepted does not automatically mean the contents of the +// signature are authorized to run code as root, or to affect system or cluster configuration. +func (pc *PolicyContext) GetSignaturesWithAcceptedAuthor(image types.Image) (sigs []*Signature, finalErr error) { + if err := pc.changeState(pcReady, pcInUse); err != nil { + return nil, err + } + defer func() { + if err := pc.changeState(pcInUse, pcReady); err != nil { + sigs = nil + finalErr = err + } + }() + + logrus.Debugf("GetSignaturesWithAcceptedAuthor for image %s", image.IntendedDockerReference()) + + reqs, err := pc.requirementsForImage(image) + if err != nil { + return nil, err + } + + // FIXME: rename Signatures to UnverifiedSignatures + unverifiedSignatures, err := image.Signatures() + if err != nil { + return nil, err + } + + res := make([]*Signature, 0, len(unverifiedSignatures)) + for sigNumber, sig := range unverifiedSignatures { + var acceptedSig *Signature // non-nil if accepted + rejected := false + // FIXME? Say more about the contents of the signature, i.e. parse it even before verification?! + logrus.Debugf("Evaluating signature %d:", sigNumber) + interpretingReqs: + for reqNumber, req := range reqs { + // FIXME: Log the requirement itself? For now, we use just the number. + // FIXME: supply state + switch res, as, err := req.isSignatureAuthorAccepted(image, sig); res { + case sarAccepted: + if as == nil { // Coverage: this should never happen + logrus.Debugf(" Requirement %d: internal inconsistency: sarAccepted but no parsed contents", reqNumber) + rejected = true + break interpretingReqs + } + logrus.Debugf(" Requirement %d: signature accepted", reqNumber) + if acceptedSig == nil { + acceptedSig = as + } else if *as != *acceptedSig { // Coverage: this should never happen + // Huh?! Two ways of verifying the same signature blob resulted in two different parses of its already accepted contents? + logrus.Debugf(" Requirement %d: internal inconsistency: sarAccepted but different parsed contents", reqNumber) + rejected = true + acceptedSig = nil + break interpretingReqs + } + case sarRejected: + logrus.Debugf(" Requirement %d: signature rejected: %s", reqNumber, err.Error()) + rejected = true + break interpretingReqs + case sarUnknown: + if err != nil { // Coverage: this should never happen + logrus.Debugf(" Requirement %d: internal inconsistency: sarUnknown but an error message %s", reqNumber, err.Error()) + rejected = true + break interpretingReqs + } + logrus.Debugf(" Requirement %d: signature state unknown, continuing", reqNumber) + default: // Coverage: this should never happen + logrus.Debugf(" Requirement %d: internal inconsistency: unknown result %#v", reqNumber, string(res)) + rejected = true + break interpretingReqs + } + } + // This also handles the (invalid) case of empty reqs, by rejecting the signature. + if acceptedSig != nil && !rejected { + logrus.Debugf(" Overall: OK, signature accepted") + res = append(res, acceptedSig) + } else { + logrus.Debugf(" Overall: Signature not accepted") + } + } + return res, nil +} + +// IsRunningImageAllowed returns true iff the policy allows running the image. +// If it returns false, err must be non-nil, and should be an PolicyRequirementError if evaluation +// succeeded but the result was rejection. +// WARNING: This validates signatures and the manifest, but does not download or validate the +// layers. Users must validate that the layers match their expected digests. +func (pc *PolicyContext) IsRunningImageAllowed(image types.Image) (res bool, finalErr error) { + if err := pc.changeState(pcReady, pcInUse); err != nil { + return false, err + } + defer func() { + if err := pc.changeState(pcInUse, pcReady); err != nil { + res = false + finalErr = err + } + }() + + logrus.Debugf("IsRunningImageAllowed for image %s", image.IntendedDockerReference()) + + reqs, err := pc.requirementsForImage(image) + if err != nil { + return false, err + } + + if len(reqs) == 0 { + return false, PolicyRequirementError("List of verification policy requirements must not be empty") + } + + for reqNumber, req := range reqs { + // FIXME: supply state + allowed, err := req.isRunningImageAllowed(image) + if !allowed { + logrus.Debugf("Requirement %d: denied, done", reqNumber) + return false, err + } + logrus.Debugf(" Requirement %d: allowed", reqNumber) + } + // We have tested that len(reqs) != 0, so at least one req must have explicitly allowed this image. + logrus.Debugf("Overall: allowed") + return true, nil +} diff --git a/signature/policy_eval_baselayer.go b/signature/policy_eval_baselayer.go new file mode 100644 index 00000000..61b62e08 --- /dev/null +++ b/signature/policy_eval_baselayer.go @@ -0,0 +1,18 @@ +// Policy evaluation for prSignedBaseLayer. + +package signature + +import ( + "github.com/Sirupsen/logrus" + "github.com/projectatomic/skopeo/types" +) + +func (pr *prSignedBaseLayer) isSignatureAuthorAccepted(image types.Image, sig []byte) (signatureAcceptanceResult, *Signature, error) { + return sarUnknown, nil, nil +} + +func (pr *prSignedBaseLayer) isRunningImageAllowed(image types.Image) (bool, error) { + // FIXME? Reject this at policy parsing time already? + logrus.Errorf("signedBaseLayer not implemented yet!") + return false, PolicyRequirementError("signedBaseLayer not implemented yet!") +} diff --git a/signature/policy_eval_baselayer_test.go b/signature/policy_eval_baselayer_test.go new file mode 100644 index 00000000..937cb928 --- /dev/null +++ b/signature/policy_eval_baselayer_test.go @@ -0,0 +1,24 @@ +package signature + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestPRSignedBaseLayerIsSignatureAuthorAccepted(t *testing.T) { + pr, err := NewPRSignedBaseLayer(NewPRMMatchRepository()) + require.NoError(t, err) + // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. + sar, parsedSig, err := pr.isSignatureAuthorAccepted(nil, nil) + assertSARUnknown(t, sar, parsedSig, err) +} + +func TestPRSignedBaseLayerIsRunningImageAllowed(t *testing.T) { + // This will obviously need to change after signedBaseLayer is implemented. + pr, err := NewPRSignedBaseLayer(NewPRMMatchRepository()) + require.NoError(t, err) + // Pass a nil pointer to, kind of, test that the return value does not depend on the image. + res, err := pr.isRunningImageAllowed(nil) + assertRunningRejectedPolicyRequirement(t, res, err) +} diff --git a/signature/policy_eval_signedby.go b/signature/policy_eval_signedby.go new file mode 100644 index 00000000..c8018e8a --- /dev/null +++ b/signature/policy_eval_signedby.go @@ -0,0 +1,137 @@ +// Policy evaluation for prSignedBy. + +package signature + +import ( + "errors" + "fmt" + "io/ioutil" + "os" + "strings" + + "github.com/projectatomic/skopeo/docker/utils" + "github.com/projectatomic/skopeo/types" +) + +func (pr *prSignedBy) isSignatureAuthorAccepted(image types.Image, sig []byte) (signatureAcceptanceResult, *Signature, error) { + switch pr.KeyType { + case SBKeyTypeGPGKeys: + case SBKeyTypeSignedByGPGKeys, SBKeyTypeX509Certificates, SBKeyTypeSignedByX509CAs: + // FIXME? Reject this at policy parsing time already? + return sarRejected, nil, fmt.Errorf(`"Unimplemented "keyType" value "%s"`, string(pr.KeyType)) + default: + // This should never happen, newPRSignedBy ensures KeyType.IsValid() + return sarRejected, nil, fmt.Errorf(`"Unknown "keyType" value "%s"`, string(pr.KeyType)) + } + + if pr.KeyPath != "" && pr.KeyData != nil { + return sarRejected, nil, errors.New(`Internal inconsistency: both "keyPath" and "keyData" specified`) + } + // FIXME: move this to per-context initialization + var data []byte + if pr.KeyData != nil { + data = pr.KeyData + } else { + d, err := ioutil.ReadFile(pr.KeyPath) + if err != nil { + return sarRejected, nil, err + } + data = d + } + + // FIXME: move this to per-context initialization + dir, err := ioutil.TempDir("", "skopeo-signedBy-") + if err != nil { + return sarRejected, nil, err + } + defer os.RemoveAll(dir) + mech, err := newGPGSigningMechanismInDirectory(dir) + if err != nil { + return sarRejected, nil, err + } + + trustedIdentities, err := mech.ImportKeysFromBytes(data) + if err != nil { + return sarRejected, nil, err + } + if len(trustedIdentities) == 0 { + return sarRejected, nil, PolicyRequirementError("No public keys imported") + } + + signature, err := verifyAndExtractSignature(mech, sig, signatureAcceptanceRules{ + validateKeyIdentity: func(keyIdentity string) error { + for _, trustedIdentity := range trustedIdentities { + if keyIdentity == trustedIdentity { + return nil + } + } + // Coverage: We use a private GPG home directory and only import trusted keys, so this should + // 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) { + return PolicyRequirementError(fmt.Sprintf("Signature for identity %s is not accepted", ref)) + } + return nil + }, + validateSignedDockerManifestDigest: func(digest string) error { + manifest, err := image.Manifest() + if err != nil { + return err + } + digestMatches, err := utils.ManifestMatchesDigest(manifest, digest) + if err != nil { + return err + } + if !digestMatches { + return PolicyRequirementError(fmt.Sprintf("Signature for digest %s does not match", digest)) + } + return nil + }, + }) + if err != nil { + return sarRejected, nil, err + } + + return sarAccepted, signature, nil +} + +func (pr *prSignedBy) isRunningImageAllowed(image types.Image) (bool, error) { + sigs, err := image.Signatures() + if err != nil { + return false, err + } + var rejections []error + for _, s := range sigs { + var reason error + switch res, _, err := pr.isSignatureAuthorAccepted(image, s); res { + case sarAccepted: + // One accepted signature is enough. + return true, nil + case sarRejected: + reason = err + case sarUnknown: + // Huh?! This should not happen at all; treat it as any other invalid value. + fallthrough + default: + reason = fmt.Errorf(`Internal error: Unexpected signature verification result "%s"`, string(res)) + } + rejections = append(rejections, reason) + } + var summary error + switch len(rejections) { + case 0: + summary = PolicyRequirementError("A signature was required, but no signature exists") + case 1: + summary = rejections[0] + default: + var msgs []string + for _, e := range rejections { + msgs = append(msgs, e.Error()) + } + summary = PolicyRequirementError(fmt.Sprintf("None of the signatures were accepted, reasons: %s", + strings.Join(msgs, "; "))) + } + return false, summary +} diff --git a/signature/policy_eval_signedby_test.go b/signature/policy_eval_signedby_test.go new file mode 100644 index 00000000..b3cd026f --- /dev/null +++ b/signature/policy_eval_signedby_test.go @@ -0,0 +1,239 @@ +package signature + +import ( + "io/ioutil" + "os" + "path" + "testing" + + "github.com/projectatomic/skopeo/directory" + "github.com/projectatomic/skopeo/docker" + "github.com/projectatomic/skopeo/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// dirImageMock returns a types.Image for a directory, claiming a specified intendedDockerReference. +func dirImageMock(dir, intendedDockerReference string) types.Image { + return docker.GenericImageFromSource(&dirImageSourceMock{ + ImageSource: directory.NewDirImageSource(dir), + intendedDockerReference: intendedDockerReference, + }) +} + +// dirImageSourceMock inherits dirImageSource, but overrides its IntendedDockerReference method. +type dirImageSourceMock struct { + types.ImageSource + intendedDockerReference string +} + +func (d *dirImageSourceMock) IntendedDockerReference() string { + return d.intendedDockerReference +} + +func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) { + ktGPG := SBKeyTypeGPGKeys + prm := NewPRMMatchExact() + testImage := dirImageMock("fixtures/dir-img-valid", "testing/manifest:latest") + testImageSig, err := ioutil.ReadFile("fixtures/dir-img-valid/signature-1") + require.NoError(t, err) + + // Successful validation, with KeyData and KeyPath + pr, err := NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + sar, parsedSig, err := pr.isSignatureAuthorAccepted(testImage, testImageSig) + assertSARAccepted(t, sar, parsedSig, err, Signature{ + DockerManifestDigest: TestImageManifestDigest, + DockerReference: "testing/manifest:latest", + }) + + keyData, err := ioutil.ReadFile("fixtures/public-key.gpg") + require.NoError(t, err) + pr, err = NewPRSignedByKeyData(ktGPG, keyData, prm) + require.NoError(t, err) + sar, parsedSig, err = pr.isSignatureAuthorAccepted(testImage, testImageSig) + assertSARAccepted(t, sar, parsedSig, err, Signature{ + DockerManifestDigest: TestImageManifestDigest, + DockerReference: "testing/manifest:latest", + }) + + // Unimplemented and invalid KeyType values + for _, keyType := range []sbKeyType{SBKeyTypeSignedByGPGKeys, + SBKeyTypeX509Certificates, + SBKeyTypeSignedByX509CAs, + sbKeyType("This is invalid"), + } { + // Do not use NewPRSignedByKeyData, because it would reject invalid values. + pr := &prSignedBy{ + KeyType: keyType, + KeyData: []byte("abc"), + SignedIdentity: prm, + } + // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. + sar, parsedSig, err := pr.isSignatureAuthorAccepted(nil, nil) + assertSARRejected(t, sar, parsedSig, err) + } + + // Both KeyPath and KeyData set. Do not use NewPRSignedBy*, because it would reject this. + prSB := &prSignedBy{ + KeyType: ktGPG, + KeyPath: "/foo/bar", + KeyData: []byte("abc"), + SignedIdentity: prm, + } + // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. + sar, parsedSig, err = prSB.isSignatureAuthorAccepted(nil, nil) + assertSARRejected(t, sar, parsedSig, err) + + // Invalid KeyPath + pr, err = NewPRSignedByKeyPath(ktGPG, "/this/does/not/exist", prm) + require.NoError(t, err) + // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. + sar, parsedSig, err = pr.isSignatureAuthorAccepted(nil, nil) + assertSARRejected(t, sar, parsedSig, err) + + // Errors initializing the temporary GPG directory and mechanism are not obviously easy to reach. + + // KeyData has no public keys. + pr, err = NewPRSignedByKeyData(ktGPG, []byte{}, prm) + require.NoError(t, err) + // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. + sar, parsedSig, err = pr.isSignatureAuthorAccepted(nil, nil) + assertSARRejectedPolicyRequirement(t, sar, parsedSig, err) + + // A signature which does not GPG verify + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + // Pass a nil pointer to, kind of, test that the return value does not depend on the image parmater.. + sar, parsedSig, err = pr.isSignatureAuthorAccepted(nil, []byte("invalid signature")) + assertSARRejected(t, sar, parsedSig, err) + + // A valid signature using an unknown key. + // (This is (currently?) rejected through the "mech.Verify fails" path, not the "!identityFound" path, + // because we use a temporary directory and only import the trusted keys.) + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + sig, err := ioutil.ReadFile("fixtures/unknown-key.signature") + require.NoError(t, err) + // Pass a nil pointer to, kind of, test that the return value does not depend on the image parmater.. + sar, parsedSig, err = pr.isSignatureAuthorAccepted(nil, sig) + assertSARRejected(t, sar, parsedSig, err) + + // A valid signature of an invalid JSON. + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + sig, err = ioutil.ReadFile("fixtures/invalid-blob.signature") + require.NoError(t, err) + // Pass a nil pointer to, kind of, test that the return value does not depend on the image parmater.. + sar, parsedSig, err = pr.isSignatureAuthorAccepted(nil, sig) + assertSARRejected(t, sar, parsedSig, err) + assert.IsType(t, InvalidSignatureError{}, err) + + // A valid signature with a rejected identity. + nonmatchingPRM, err := NewPRMExactReference("this/doesnt:match") + require.NoError(t, err) + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", nonmatchingPRM) + require.NoError(t, err) + sar, parsedSig, err = pr.isSignatureAuthorAccepted(testImage, testImageSig) + assertSARRejectedPolicyRequirement(t, sar, parsedSig, err) + + // Error reading image manifest + image := dirImageMock("fixtures/dir-img-no-manifest", "testing/manifest:latest") + sig, err = ioutil.ReadFile("fixtures/dir-img-no-manifest/signature-1") + require.NoError(t, err) + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + sar, parsedSig, err = pr.isSignatureAuthorAccepted(image, sig) + assertSARRejected(t, sar, parsedSig, err) + + // Error computing manifest digest + image = dirImageMock("fixtures/dir-img-manifest-digest-error", "testing/manifest:latest") + sig, err = ioutil.ReadFile("fixtures/dir-img-manifest-digest-error/signature-1") + require.NoError(t, err) + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + sar, parsedSig, err = pr.isSignatureAuthorAccepted(image, sig) + assertSARRejected(t, sar, parsedSig, err) + + // A valid signature with a non-matching manifest + image = dirImageMock("fixtures/dir-img-modified-manifest", "testing/manifest:latest") + sig, err = ioutil.ReadFile("fixtures/dir-img-modified-manifest/signature-1") + require.NoError(t, err) + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + sar, parsedSig, err = pr.isSignatureAuthorAccepted(image, sig) + assertSARRejectedPolicyRequirement(t, sar, parsedSig, err) +} + +// createInvalidSigDir creates a directory suitable for dirImageMock, in which image.Signatures() +// fails. +// The caller should eventually call os.RemoveAll on the returned path. +func createInvalidSigDir(t *testing.T) string { + dir, err := ioutil.TempDir("", "skopeo-test-unreadable-signature") + require.NoError(t, err) + err = ioutil.WriteFile(path.Join(dir, "manifest.json"), []byte("{}"), 0644) + require.NoError(t, err) + // Creating a 000-permissions file would work for unprivileged accounts, but root (in particular, + // in the Docker container we use for testing) would still have access. So, create a symlink + // pointing to itself, to cause an ELOOP. (Note that a symlink pointing to a nonexistent file would be treated + // just like a nonexistent signature file, and not an error.) + err = os.Symlink("signature-1", path.Join(dir, "signature-1")) + require.NoError(t, err) + return dir +} + +func TestPRSignedByIsRunningImageAllowed(t *testing.T) { + ktGPG := SBKeyTypeGPGKeys + prm := NewPRMMatchExact() + + // A simple success case: single valid signature. + image := dirImageMock("fixtures/dir-img-valid", "testing/manifest:latest") + pr, err := NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + allowed, err := pr.isRunningImageAllowed(image) + assertRunningAllowed(t, allowed, err) + + // Error reading signatures + invalidSigDir := createInvalidSigDir(t) + defer os.RemoveAll(invalidSigDir) + image = dirImageMock(invalidSigDir, "testing/manifest:latest") + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + allowed, err = pr.isRunningImageAllowed(image) + assertRunningRejected(t, allowed, err) + + // No signatures + image = dirImageMock("fixtures/dir-img-unsigned", "testing/manifest:latest") + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + allowed, err = pr.isRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, allowed, err) + + // 1 invalid signature: use dir-img-valid, but a non-matching Docker reference + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:notlatest") + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + allowed, err = pr.isRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, allowed, err) + + // 2 valid signatures + image = dirImageMock("fixtures/dir-img-valid-2", "testing/manifest:latest") + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + allowed, err = pr.isRunningImageAllowed(image) + assertRunningAllowed(t, allowed, err) + + // One invalid, one valid signature (in this order) + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:latest") + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + allowed, err = pr.isRunningImageAllowed(image) + assertRunningAllowed(t, allowed, err) + + // 2 invalid signatures: use dir-img-valid-2, but a non-matching Docker reference + image = dirImageMock("fixtures/dir-img-valid-2", "testing/manifest:notlatest") + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + require.NoError(t, err) + allowed, err = pr.isRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, allowed, err) +} diff --git a/signature/policy_eval_simple.go b/signature/policy_eval_simple.go new file mode 100644 index 00000000..53530353 --- /dev/null +++ b/signature/policy_eval_simple.go @@ -0,0 +1,25 @@ +// Policy evaluation for the various simple PolicyRequirement types. + +package signature + +import "github.com/projectatomic/skopeo/types" + +func (pr *prInsecureAcceptAnything) isSignatureAuthorAccepted(image types.Image, sig []byte) (signatureAcceptanceResult, *Signature, error) { + // prInsecureAcceptAnything semantics: Every image is allowed to run, + // but this does not consider the signature as verified. + return sarUnknown, nil, nil +} + +func (pr *prInsecureAcceptAnything) isRunningImageAllowed(image types.Image) (bool, error) { + return true, nil +} + +func (pr *prReject) isSignatureAuthorAccepted(image types.Image, sig []byte) (signatureAcceptanceResult, *Signature, error) { + // FIXME? Name the image, or better the matched scope in Policy.Specific. + return sarRejected, nil, PolicyRequirementError("Any signatures for these images are rejected by policy.") +} + +func (pr *prReject) isRunningImageAllowed(image types.Image) (bool, error) { + // FIXME? Name the image, or better the matched scope in Policy.Specific. + return false, PolicyRequirementError("Running these images is rejected by policy.") +} diff --git a/signature/policy_eval_simple_test.go b/signature/policy_eval_simple_test.go new file mode 100644 index 00000000..c4434b10 --- /dev/null +++ b/signature/policy_eval_simple_test.go @@ -0,0 +1,32 @@ +package signature + +import "testing" + +func TestPRInsecureAcceptAnythingIsSignatureAuthorAccepted(t *testing.T) { + pr := NewPRInsecureAcceptAnything() + // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. + sar, parsedSig, err := pr.isSignatureAuthorAccepted(nil, nil) + assertSARUnknown(t, sar, parsedSig, err) +} + +func TestPRInsecureAcceptAnythingIsRunningImageAllowed(t *testing.T) { + pr := NewPRInsecureAcceptAnything() + // Pass a nil pointer to, kind of, test that the return value does not depend on the image. + res, err := pr.isRunningImageAllowed(nil) + assertRunningAllowed(t, res, err) +} + +func TestPRRejectIsSignatureAuthorAccepted(t *testing.T) { + pr := NewPRReject() + // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. + sar, parsedSig, err := pr.isSignatureAuthorAccepted(nil, nil) + assertSARRejectedPolicyRequirement(t, sar, parsedSig, err) +} + +func TestPRRejectIsRunningImageAllowed(t *testing.T) { + // This will obviously need to change after this is implemented. + pr := NewPRReject() + // Pass a nil pointer to, kind of, test that the return value does not depend on the image. + res, err := pr.isRunningImageAllowed(nil) + assertRunningRejectedPolicyRequirement(t, res, err) +} diff --git a/signature/policy_eval_test.go b/signature/policy_eval_test.go new file mode 100644 index 00000000..254b285d --- /dev/null +++ b/signature/policy_eval_test.go @@ -0,0 +1,504 @@ +package signature + +import ( + "fmt" + "os" + "testing" + + "github.com/projectatomic/skopeo/reference" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPolicyRequirementError(t *testing.T) { + // A stupid test just to keep code coverage + s := "test" + err := PolicyRequirementError(s) + assert.Equal(t, s, err.Error()) +} + +func TestPolicyContextChangeState(t *testing.T) { + pc, err := NewPolicyContext(&Policy{Default: PolicyRequirements{NewPRReject()}}) + require.NoError(t, err) + defer pc.Destroy() + + require.Equal(t, pcReady, pc.state) + err = pc.changeState(pcReady, pcInUse) + require.NoError(t, err) + + err = pc.changeState(pcReady, pcInUse) + require.Error(t, err) + + // Return state to pcReady to allow pc.Destroy to clean up. + err = pc.changeState(pcInUse, pcReady) + require.NoError(t, err) +} + +func TestPolicyContextNewDestroy(t *testing.T) { + pc, err := NewPolicyContext(&Policy{Default: PolicyRequirements{NewPRReject()}}) + require.NoError(t, err) + assert.Equal(t, pcReady, pc.state) + + err = pc.Destroy() + require.NoError(t, err) + assert.Equal(t, pcDestroyed, pc.state) + + // Trying to destroy when not pcReady + pc, err = NewPolicyContext(&Policy{Default: PolicyRequirements{NewPRReject()}}) + require.NoError(t, err) + err = pc.changeState(pcReady, pcInUse) + require.NoError(t, err) + err = pc.Destroy() + require.Error(t, err) + assert.Equal(t, pcInUse, pc.state) // The state, and hopefully nothing else, has changed. + + err = pc.changeState(pcInUse, pcReady) + require.NoError(t, err) + err = pc.Destroy() + assert.NoError(t, err) +} + +func TestFullyExpandedDockerReference(t *testing.T) { + sha256Digest := "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + // Test both that fullyExpandedDockerReference returns the expected value (fullName+suffix), + // and that .FullName returns the expected value (fullName), i.e. that the two functions are + // consistent. + for inputName, fullName := range map[string]string{ + "example.com/ns/repo": "example.com/ns/repo", + "example.com/repo": "example.com/repo", + "localhost/ns/repo": "localhost/ns/repo", + // Note that "localhost" is special here: notlocalhost/repo is be parsed as docker.io/notlocalhost.repo: + "localhost/repo": "localhost/repo", + "notlocalhost/repo": "docker.io/notlocalhost/repo", + "docker.io/ns/repo": "docker.io/ns/repo", + "docker.io/library/repo": "docker.io/library/repo", + "docker.io/repo": "docker.io/library/repo", + "ns/repo": "docker.io/ns/repo", + "library/repo": "docker.io/library/repo", + "repo": "docker.io/library/repo", + } { + for inputSuffix, mappedSuffix := range map[string]string{ + ":tag": ":tag", + sha256Digest: sha256Digest, + "": "", + // A github.com/distribution/reference value can have a tag and a digest at the same time! + // github.com/skopeo/reference handles that by dropping the tag. That is not obviously the + // right thing to do, but it is at least reasonable, so test that we keep behaving reasonably. + // This test case should not be construed to make this an API promise. + ":tag" + sha256Digest: sha256Digest, + } { + fullInput := inputName + inputSuffix + ref, err := reference.ParseNamed(fullInput) + require.NoError(t, err) + assert.Equal(t, fullName, ref.FullName(), fullInput) + expanded, err := fullyExpandedDockerReference(ref) + require.NoError(t, err) + assert.Equal(t, fullName+mappedSuffix, expanded, fullInput) + } + } +} + +func TestPolicyContextRequirementsForImage(t *testing.T) { + ktGPG := SBKeyTypeGPGKeys + prm := NewPRMMatchExact() + + policy := &Policy{ + Default: PolicyRequirements{NewPRReject()}, + Specific: map[string]PolicyRequirements{}, + } + // Just put _something_ into the Specific map for the keys we care about, and make it pairwise + // distinct so that we can compare the values and show them when debugging the tests. + for _, scope := range []string{ + "unmatched", + "hostname.com", + "hostname.com/namespace", + "hostname.com/namespace/repo", + "hostname.com/namespace/repo:latest", + "hostname.com/namespace/repo:tag2", + "localhost", + "localhost/namespace", + "localhost/namespace/repo", + "localhost/namespace/repo:latest", + "localhost/namespace/repo:tag2", + "deep.com", + "deep.com/n1", + "deep.com/n1/n2", + "deep.com/n1/n2/n3", + "deep.com/n1/n2/n3/repo", + "deep.com/n1/n2/n3/repo:tag2", + "docker.io", + "docker.io/library", + "docker.io/library/busybox", + "docker.io/namespaceindocker", + "docker.io/namespaceindocker/repo", + "docker.io/namespaceindocker/repo:tag2", + // Note: these non-fully-expanded repository names are not matched against canonical (shortened) + // Docker names; they are instead parsed as starting with hostnames. + "busybox", + "library/busybox", + "namespaceindocker", + "namespaceindocker/repo", + "namespaceindocker/repo:tag2", + } { + policy.Specific[scope] = PolicyRequirements{xNewPRSignedByKeyData(ktGPG, []byte(scope), prm)} + } + + pc, err := NewPolicyContext(policy) + require.NoError(t, err) + + for input, matched := range map[string]string{ + // Full match + "hostname.com/namespace/repo:latest": "hostname.com/namespace/repo:latest", + "hostname.com/namespace/repo:tag2": "hostname.com/namespace/repo:tag2", + "hostname.com/namespace/repo": "hostname.com/namespace/repo:latest", + "localhost/namespace/repo:latest": "localhost/namespace/repo:latest", + "localhost/namespace/repo:tag2": "localhost/namespace/repo:tag2", + "localhost/namespace/repo": "localhost/namespace/repo:latest", + "deep.com/n1/n2/n3/repo:tag2": "deep.com/n1/n2/n3/repo:tag2", + // Repository match + "hostname.com/namespace/repo:notlatest": "hostname.com/namespace/repo", + "localhost/namespace/repo:notlatest": "localhost/namespace/repo", + "deep.com/n1/n2/n3/repo:nottag2": "deep.com/n1/n2/n3/repo", + // Namespace match + "hostname.com/namespace/notrepo:latest": "hostname.com/namespace", + "localhost/namespace/notrepo:latest": "localhost/namespace", + "deep.com/n1/n2/n3/notrepo:tag2": "deep.com/n1/n2/n3", + "deep.com/n1/n2/notn3/repo:tag2": "deep.com/n1/n2", + "deep.com/n1/notn2/n3/repo:tag2": "deep.com/n1", + // Host name match + "hostname.com/notnamespace/repo:latest": "hostname.com", + "localhost/notnamespace/repo:latest": "localhost", + "deep.com/notn1/n2/n3/repo:tag2": "deep.com", + // Default + "this.doesnt/match:anything": "", + "this.doesnt/match-anything/defaulttag": "", + + // docker.io canonizalication effects + "docker.io/library/busybox": "docker.io/library/busybox", + "library/busybox": "docker.io/library/busybox", + "busybox": "docker.io/library/busybox", + "docker.io/library/somethinginlibrary": "docker.io/library", + "library/somethinginlibrary": "docker.io/library", + "somethinginlibrary": "docker.io/library", + "docker.io/namespaceindocker/repo:tag2": "docker.io/namespaceindocker/repo:tag2", + "namespaceindocker/repo:tag2": "docker.io/namespaceindocker/repo:tag2", + "docker.io/namespaceindocker/repo:nottag2": "docker.io/namespaceindocker/repo", + "namespaceindocker/repo:nottag2": "docker.io/namespaceindocker/repo", + "docker.io/namespaceindocker/notrepo:tag2": "docker.io/namespaceindocker", + "namespaceindocker/notrepo:tag2": "docker.io/namespaceindocker", + "docker.io/notnamespaceindocker/repo:tag2": "docker.io", + "notnamespaceindocker/repo:tag2": "docker.io", + } { + var expected PolicyRequirements + if matched != "" { + e, ok := policy.Specific[matched] + require.True(t, ok, fmt.Sprintf("case %s: expected reqs not found", input)) + expected = e + } else { + expected = policy.Default + } + + reqs, err := pc.requirementsForImage(refImageMock(input)) + require.NoError(t, err) + comment := fmt.Sprintf("case %s: %#v", input, reqs[0]) + // Do not sue assert.Equal, which would do a deep contents comparison; we want to compare + // the pointers. Also, == does not work on slices; so test that the slices start at the + // same element and have the same length. + assert.True(t, &(reqs[0]) == &(expected[0]), comment) + assert.True(t, len(reqs) == len(expected), comment) + } + + // Invalid reference format + _, err = pc.requirementsForImage(refImageMock("UPPERCASEISINVALID")) + assert.Error(t, err) +} + +func TestPolicyContextGetSignaturesWithAcceptedAuthor(t *testing.T) { + expectedSig := &Signature{ + DockerManifestDigest: TestImageManifestDigest, + DockerReference: "testing/manifest:latest", + } + + pc, err := NewPolicyContext(&Policy{ + Default: PolicyRequirements{NewPRReject()}, + Specific: map[string]PolicyRequirements{ + "docker.io/testing/manifest:latest": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchExact()), + }, + "docker.io/testing/manifest:twoAccepts": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + }, + "docker.io/testing/manifest:acceptReject": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + NewPRReject(), + }, + "docker.io/testing/manifest:acceptUnknown": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + xNewPRSignedBaseLayer(NewPRMMatchRepository()), + }, + "docker.io/testing/manifest:rejectUnknown": { + NewPRReject(), + xNewPRSignedBaseLayer(NewPRMMatchRepository()), + }, + "docker.io/testing/manifest:unknown": { + xNewPRSignedBaseLayer(NewPRMMatchRepository()), + }, + "docker.io/testing/manifest:unknown2": { + NewPRInsecureAcceptAnything(), + }, + "docker.io/testing/manifest:invalidEmptyRequirements": {}, + }, + }) + require.NoError(t, err) + defer pc.Destroy() + + // Success + image := dirImageMock("fixtures/dir-img-valid", "testing/manifest:latest") + sigs, err := pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Equal(t, []*Signature{expectedSig}, sigs) + + // Two signatures + // FIXME? Use really different signatures for this? + image = dirImageMock("fixtures/dir-img-valid-2", "testing/manifest:latest") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Equal(t, []*Signature{expectedSig, expectedSig}, sigs) + + // No signatures + image = dirImageMock("fixtures/dir-img-unsigned", "testing/manifest:latest") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // Only invalid signatures + image = dirImageMock("fixtures/dir-img-modified-manifest", "testing/manifest:latest") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // 1 invalid, 1 valid signature (in this order) + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:latest") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Equal(t, []*Signature{expectedSig}, sigs) + + // Two sarAccepted results for one signature + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:twoAccepts") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Equal(t, []*Signature{expectedSig}, sigs) + + // sarAccepted+sarRejected for a signature + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:acceptReject") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // sarAccepted+sarUnknown for a signature + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:acceptUnknown") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Equal(t, []*Signature{expectedSig}, sigs) + + // sarRejected+sarUnknown for a signature + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:rejectUnknown") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // sarUnknown only + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:unknown") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:unknown2") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // Empty list of requirements (invalid) + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:invalidEmptyRequirements") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // Failures: Make sure we return nil sigs. + + // Unexpected state (context already destroyed) + destroyedPC, err := NewPolicyContext(pc.Policy) + require.NoError(t, err) + err = destroyedPC.Destroy() + require.NoError(t, err) + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:latest") + sigs, err = destroyedPC.GetSignaturesWithAcceptedAuthor(image) + assert.Error(t, err) + assert.Nil(t, sigs) + // Not testing the pcInUse->pcReady transition, that would require custom PolicyRequirement + // implementations meddling with the state, or threads. This is for catching trivial programmer + // mistakes only, anyway. + + // Invalid IntendedDockerReference value + image = dirImageMock("fixtures/dir-img-valid", "UPPERCASEISINVALID") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + assert.Error(t, err) + assert.Nil(t, sigs) + + // Error reading signatures. + invalidSigDir := createInvalidSigDir(t) + defer os.RemoveAll(invalidSigDir) + image = dirImageMock(invalidSigDir, "testing/manifest:latest") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + assert.Error(t, err) + assert.Nil(t, sigs) +} + +func TestPolicyContextIsRunningImageAllowed(t *testing.T) { + pc, err := NewPolicyContext(&Policy{ + Default: PolicyRequirements{NewPRReject()}, + Specific: map[string]PolicyRequirements{ + "docker.io/testing/manifest:latest": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchExact()), + }, + "docker.io/testing/manifest:twoAllows": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + }, + "docker.io/testing/manifest:allowDeny": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + NewPRReject(), + }, + "docker.io/testing/manifest:reject": { + NewPRReject(), + }, + "docker.io/testing/manifest:acceptAnything": { + NewPRInsecureAcceptAnything(), + }, + "docker.io/testing/manifest:invalidEmptyRequirements": {}, + }, + }) + require.NoError(t, err) + defer pc.Destroy() + + // Success + image := dirImageMock("fixtures/dir-img-valid", "testing/manifest:latest") + res, err := pc.IsRunningImageAllowed(image) + assertRunningAllowed(t, res, err) + + // Two signatures + // FIXME? Use really different signatures for this? + image = dirImageMock("fixtures/dir-img-valid-2", "testing/manifest:latest") + res, err = pc.IsRunningImageAllowed(image) + assertRunningAllowed(t, res, err) + + // No signatures + image = dirImageMock("fixtures/dir-img-unsigned", "testing/manifest:latest") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, res, err) + + // Only invalid signatures + image = dirImageMock("fixtures/dir-img-modified-manifest", "testing/manifest:latest") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, res, err) + + // 1 invalid, 1 valid signature (in this order) + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:latest") + res, err = pc.IsRunningImageAllowed(image) + assertRunningAllowed(t, res, err) + + // Two allowed results + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:twoAllows") + res, err = pc.IsRunningImageAllowed(image) + assertRunningAllowed(t, res, err) + + // Allow + deny results + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:allowDeny") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, res, err) + + // prReject works + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:reject") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, res, err) + + // prInsecureAcceptAnything works + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:acceptAnything") + res, err = pc.IsRunningImageAllowed(image) + assertRunningAllowed(t, res, err) + + // Empty list of requirements (invalid) + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:invalidEmptyRequirements") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, res, err) + + // Unexpected state (context already destroyed) + destroyedPC, err := NewPolicyContext(pc.Policy) + require.NoError(t, err) + err = destroyedPC.Destroy() + require.NoError(t, err) + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:latest") + res, err = destroyedPC.IsRunningImageAllowed(image) + assertRunningRejected(t, res, err) + // Not testing the pcInUse->pcReady transition, that would require custom PolicyRequirement + // implementations meddling with the state, or threads. This is for catching trivial programmer + // mistakes only, anyway. + + // Invalid IntendedDockerReference value + image = dirImageMock("fixtures/dir-img-valid", "UPPERCASEISINVALID") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejected(t, res, err) +} + +// Helpers for validating PolicyRequirement.isSignatureAuthorAccepted results: + +// assertSARRejected verifies that isSignatureAuthorAccepted returns a consistent sarRejected result +// with the expected signature. +func assertSARAccepted(t *testing.T, sar signatureAcceptanceResult, parsedSig *Signature, err error, expectedSig Signature) { + assert.Equal(t, sarAccepted, sar) + assert.Equal(t, &expectedSig, parsedSig) + assert.NoError(t, err) +} + +// assertSARRejected verifies that isSignatureAuthorAccepted returns a consistent sarRejected result. +func assertSARRejected(t *testing.T, sar signatureAcceptanceResult, parsedSig *Signature, err error) { + assert.Equal(t, sarRejected, sar) + assert.Nil(t, parsedSig) + assert.Error(t, err) +} + +// assertSARRejectedPolicyRequiremnt verifies that isSignatureAuthorAccepted returns a consistent sarRejected resul, +// and that the returned error is a PolicyRequirementError.. +func assertSARRejectedPolicyRequirement(t *testing.T, sar signatureAcceptanceResult, parsedSig *Signature, err error) { + assertSARRejected(t, sar, parsedSig, err) + assert.IsType(t, PolicyRequirementError(""), err) +} + +// assertSARRejected verifies that isSignatureAuthorAccepted returns a consistent sarUnknown result. +func assertSARUnknown(t *testing.T, sar signatureAcceptanceResult, parsedSig *Signature, err error) { + assert.Equal(t, sarUnknown, sar) + assert.Nil(t, parsedSig) + assert.NoError(t, err) +} + +// Helpers for validating PolicyRequirement.isRunningImageAllowed results: + +// assertRunningAllowed verifies that isRunningImageAllowed returns a consistent true result +func assertRunningAllowed(t *testing.T, allowed bool, err error) { + assert.Equal(t, true, allowed) + assert.NoError(t, err) +} + +// assertRunningRejected verifies that isRunningImageAllowed returns a consistent false result +func assertRunningRejected(t *testing.T, allowed bool, err error) { + assert.Equal(t, false, allowed) + assert.Error(t, err) +} + +// assertRunningRejectedPolicyRequirement verifies that isRunningImageAllowed returns a consistent false result +// and that the returned error is a PolicyRequirementError. +func assertRunningRejectedPolicyRequirement(t *testing.T, allowed bool, err error) { + assertRunningRejected(t, allowed, err) + assert.IsType(t, PolicyRequirementError(""), err) +} diff --git a/signature/policy_reference_match.go b/signature/policy_reference_match.go new file mode 100644 index 00000000..b1d2caea --- /dev/null +++ b/signature/policy_reference_match.go @@ -0,0 +1,61 @@ +// PolicyReferenceMatch implementations. + +package signature + +import ( + "github.com/projectatomic/skopeo/reference" + "github.com/projectatomic/skopeo/types" +) + +// parseDockerReferences converts two reference strings into parsed entities, failing on any error +func parseDockerReferences(s1, s2 string) (reference.Named, reference.Named, error) { + r1, err := reference.ParseNamed(s1) + if err != nil { + return nil, nil, err + } + r2, err := reference.ParseNamed(s2) + if err != nil { + return nil, nil, err + } + return r1, r2, nil +} + +func (prm *prmMatchExact) matchesDockerReference(image types.Image, signatureDockerReference string) bool { + intended, signature, err := parseDockerReferences(image.IntendedDockerReference(), signatureDockerReference) + if err != nil { + return false + } + // Do not add default tags: image.IntendedDockerReference() has it added already per its construction, and signatureDockerReference should be exact; so, verify that now. + if reference.IsNameOnly(intended) || reference.IsNameOnly(signature) { + return false + } + return signature.String() == intended.String() +} + +func (prm *prmMatchRepository) matchesDockerReference(image types.Image, signatureDockerReference string) bool { + intended, signature, err := parseDockerReferences(image.IntendedDockerReference(), signatureDockerReference) + if err != nil { + return false + } + return signature.Name() == intended.Name() +} + +func (prm *prmExactReference) matchesDockerReference(image types.Image, signatureDockerReference string) bool { + intended, signature, err := parseDockerReferences(prm.DockerReference, signatureDockerReference) + if err != nil { + return false + } + // prm.DockerReference and signatureDockerReference should be exact; so, verify that now. + if reference.IsNameOnly(intended) || reference.IsNameOnly(signature) { + return false + } + return signature.String() == intended.String() +} + +func (prm *prmExactRepository) matchesDockerReference(image types.Image, signatureDockerReference string) bool { + intended, signature, err := parseDockerReferences(prm.DockerRepository, signatureDockerReference) + if err != nil { + return false + } + return signature.Name() == intended.Name() +} diff --git a/signature/policy_reference_match_test.go b/signature/policy_reference_match_test.go new file mode 100644 index 00000000..0b00214b --- /dev/null +++ b/signature/policy_reference_match_test.go @@ -0,0 +1,202 @@ +package signature + +import ( + "fmt" + "testing" + + "github.com/projectatomic/skopeo/types" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + fullRHELRef = "registry.access.redhat.com/rhel7/rhel:7.2.3" + untaggedRHELRef = "registry.access.redhat.com/rhel7/rhel" +) + +func TestParseDockerReferences(t *testing.T) { + const ( + ok1 = "busybox" + ok2 = fullRHELRef + bad1 = "UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES" + bad2 = "" + ) + + // Success + r1, r2, err := parseDockerReferences(ok1, ok2) + require.NoError(t, err) + assert.Equal(t, ok1, r1.String()) + assert.Equal(t, ok2, r2.String()) + + // Failures + for _, refs := range [][]string{ + {bad1, ok2}, + {ok1, bad2}, + {bad1, bad2}, + } { + _, _, err := parseDockerReferences(refs[0], refs[1]) + assert.Error(t, err) + } +} + +// refImageMock is a mock of types.Image which returns itself in IntendedDockerReference. +type refImageMock string + +func (ref refImageMock) IntendedDockerReference() string { + return string(ref) +} +func (ref refImageMock) Manifest() ([]byte, error) { + panic("unexpected call to a mock function") +} +func (ref refImageMock) ManifestMatchesDigest(expectedDigest string) (bool, error) { + panic("unexpected call to a mock function") +} +func (ref refImageMock) Signatures() ([][]byte, error) { + panic("unexpected call to a mock function") +} +func (ref refImageMock) Layers(layers ...string) error { + panic("unexpected call to a mock function") +} +func (ref refImageMock) Inspect() (*types.ImageInspectInfo, error) { + panic("unexpected call to a mock function") +} +func (ref refImageMock) DockerTar() ([]byte, error) { + panic("unexpected call to a mock function") +} +func (ref refImageMock) GetRepositoryTags() ([]string, error) { + panic("unexpected call to a mock function") +} + +type prmTableTest struct { + imageRef, sigRef string + result bool +} + +// Test cases for exact reference match +var prmExactMatchTestTable = []prmTableTest{ + // Success, simple matches + {"busybox:latest", "busybox:latest", true}, + {fullRHELRef, fullRHELRef, true}, + // Non-canonical reference format is canonicalized + {"library/busybox:latest", "busybox:latest", true}, + {"busybox:latest", "library/busybox:latest", true}, + {"docker.io/library/busybox:latest", "busybox:latest", true}, + {"busybox:latest", "docker.io/library/busybox:latest", true}, + // Mismatch + {"busybox:latest", "busybox:notlatest", false}, + {"busybox:latest", "notbusybox:latest", false}, + {"busybox:latest", "hostname/library/busybox:notlatest", false}, + {"hostname/library/busybox:latest", "busybox:notlatest", false}, + {"busybox:latest", fullRHELRef, false}, + // Missing tags + {"busybox", "busybox:latest", false}, + {"busybox:latest", "busybox", false}, + {"busybox", "busybox", false}, + // Invalid format + {"UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", "busybox:latest", false}, + {"busybox:latest", "UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", false}, + {"", "UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", false}, + // Even if they are exactly equal, invalid values are rejected. + {"INVALID", "INVALID", false}, +} + +// Test cases for repository-only reference match +var prmRepositoryMatchTestTable = []prmTableTest{ + // Success, simple matches + {"busybox:latest", "busybox:latest", true}, + {fullRHELRef, fullRHELRef, true}, + // Non-canonical reference format is canonicalized + {"library/busybox:latest", "busybox:latest", true}, + {"busybox:latest", "library/busybox:latest", true}, + {"docker.io/library/busybox:latest", "busybox:latest", true}, + {"busybox:latest", "docker.io/library/busybox:latest", true}, + // The same as above, but with mismatching tags + {"busybox:latest", "busybox:notlatest", true}, + {fullRHELRef + "tagsuffix", fullRHELRef, true}, + {"library/busybox:latest", "busybox:notlatest", true}, + {"busybox:latest", "library/busybox:notlatest", true}, + {"docker.io/library/busybox:notlatest", "busybox:latest", true}, + {"busybox:notlatest", "docker.io/library/busybox:latest", true}, + // The same as above, but with defaulted tags (should not actually happen) + {"busybox", "busybox:notlatest", true}, + {fullRHELRef, untaggedRHELRef, true}, + {"library/busybox", "busybox", true}, + {"busybox", "library/busybox", true}, + {"docker.io/library/busybox", "busybox", true}, + {"busybox", "docker.io/library/busybox", true}, + // Mismatch + {"busybox:latest", "notbusybox:latest", false}, + {"hostname/library/busybox:latest", "busybox:notlatest", false}, + {"busybox:latest", fullRHELRef, false}, + // Invalid format + {"UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", "busybox:latest", false}, + {"busybox:latest", "UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", false}, + {"", "UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", false}, + // Even if they are exactly equal, invalid values are rejected. + {"INVALID", "INVALID", false}, +} + +func TestPRMMatchExactMatchesDockerReference(t *testing.T) { + prm := NewPRMMatchExact() + for _, test := range prmExactMatchTestTable { + res := prm.matchesDockerReference(refImageMock(test.imageRef), test.sigRef) + assert.Equal(t, test.result, res, fmt.Sprintf("%s vs. %s", test.imageRef, test.sigRef)) + } +} + +func TestPRMMatchRepositoryMatchesDockerReference(t *testing.T) { + prm := NewPRMMatchRepository() + for _, test := range prmRepositoryMatchTestTable { + res := prm.matchesDockerReference(refImageMock(test.imageRef), test.sigRef) + assert.Equal(t, test.result, res, fmt.Sprintf("%s vs. %s", test.imageRef, test.sigRef)) + } +} + +// forbiddenImageMock is a mock of types.Image which ensures IntendedDockerReference is not called +type forbiddenImageMock string + +func (ref forbiddenImageMock) IntendedDockerReference() string { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) Manifest() ([]byte, error) { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) ManifestMatchesDigest(expectedDigest string) (bool, error) { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) Signatures() ([][]byte, error) { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) Layers(layers ...string) error { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) Inspect() (*types.ImageInspectInfo, error) { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) DockerTar() ([]byte, error) { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) GetRepositoryTags() ([]string, error) { + panic("unexpected call to a mock function") +} + +func TestPRMExactReferenceMatchesDockerReference(t *testing.T) { + for _, test := range prmExactMatchTestTable { + // Do not use NewPRMExactReference, we want to also test the case with an invalid DockerReference, + // even though NewPRMExactReference should never let it happen. + prm := prmExactReference{DockerReference: test.imageRef} + res := prm.matchesDockerReference(forbiddenImageMock(""), test.sigRef) + assert.Equal(t, test.result, res, fmt.Sprintf("%s vs. %s", test.imageRef, test.sigRef)) + } +} + +func TestPRMExactRepositoryMatchesDockerReference(t *testing.T) { + for _, test := range prmRepositoryMatchTestTable { + // Do not use NewPRMExactRepository, we want to also test the case with an invalid DockerReference, + // even though NewPRMExactRepository should never let it happen. + prm := prmExactRepository{DockerRepository: test.imageRef} + res := prm.matchesDockerReference(forbiddenImageMock(""), test.sigRef) + assert.Equal(t, test.result, res, fmt.Sprintf("%s vs. %s", test.imageRef, test.sigRef)) + } +} diff --git a/signature/policy_types.go b/signature/policy_types.go index c38b1e76..8e614cce 100644 --- a/signature/policy_types.go +++ b/signature/policy_types.go @@ -11,7 +11,9 @@ type Policy struct { // Default applies to any image which does not have a matching policy in Specific. Default PolicyRequirements `json:"default"` // Specific applies to images matching scope, the map key. - // Scope is registry/server, namespace in a registry, single repository. + // Scope is hostname[/zero/or/more/namespaces[/repository[:tag|@digest]]]; note that in order to be + // unambiguous, this must use a fully expanded format, e.g. "docker.io/library/busybox" or + // "docker.io/library", not "busybox" or "library". // FIXME: Scope syntax - should it be namespaced docker:something ? Or, in the worst case, a composite object (we couldn't use a JSON map) // Most specific scope wins, duplication is prohibited (hard failure). // Defaults to an empty map if not specified. @@ -24,7 +26,6 @@ type PolicyRequirements []PolicyRequirement // PolicyRequirement is a rule which must be satisfied by at least one of the signatures of an image. // The type is public, but its definition is private. -type PolicyRequirement interface{} // Will be expanded and moved elsewhere later. // prCommon is the common type field in a JSON encoding of PolicyRequirement. type prCommon struct { @@ -99,7 +100,6 @@ type prSignedBaseLayer struct { // PolicyReferenceMatch specifies a set of image identities accepted in PolicyRequirement. // The type is public, but its implementation is private. -type PolicyReferenceMatch interface{} // Will be expanded and moved elsewhere later. // prmCommon is the common type field in a JSON encoding of PolicyReferenceMatch. type prmCommon struct { diff --git a/signature/signature.go b/signature/signature.go index b59209ac..bf1c63e9 100644 --- a/signature/signature.go +++ b/signature/signature.go @@ -155,26 +155,37 @@ func (s privateSignature) sign(mech SigningMechanism, keyIdentity string) ([]byt return mech.Sign(json, keyIdentity) } -// verifyAndExtractSignature verifies that signature has been signed by expectedKeyIdentity -// using mech for expectedDockerReference, and returns it (without matching its contents to an image). -func verifyAndExtractSignature(mech SigningMechanism, unverifiedSignature []byte, - expectedKeyIdentity, expectedDockerReference string) (*Signature, error) { +// signatureAcceptanceRules specifies how to decide whether an untrusted signature is acceptable. +// We centralize the actual parsing and data extraction in verifyAndExtractSignature; this supplies +// the policy. We use an object instead of supplying func parameters to verifyAndExtractSignature +// because all of the functions have the same type, so there is a risk of exchanging the functions; +// named members of this struct are more explicit. +type signatureAcceptanceRules struct { + validateKeyIdentity func(string) error + validateSignedDockerReference func(string) error + validateSignedDockerManifestDigest func(string) error +} + +// verifyAndExtractSignature verifies that unverifiedSignature has been signed, and that its principial components +// match expected values, both as specified by rules, and returns it +func verifyAndExtractSignature(mech SigningMechanism, unverifiedSignature []byte, rules signatureAcceptanceRules) (*Signature, error) { signed, keyIdentity, err := mech.Verify(unverifiedSignature) if err != nil { return nil, err } - if keyIdentity != expectedKeyIdentity { - return nil, InvalidSignatureError{msg: fmt.Sprintf("Signature by %s does not match expected fingerprint %s", keyIdentity, expectedKeyIdentity)} + if err := rules.validateKeyIdentity(keyIdentity); err != nil { + return nil, err } var unmatchedSignature privateSignature if err := json.Unmarshal(signed, &unmatchedSignature); err != nil { return nil, InvalidSignatureError{msg: err.Error()} } - - if unmatchedSignature.DockerReference != expectedDockerReference { - return nil, InvalidSignatureError{msg: fmt.Sprintf("Docker reference %s does not match %s", - unmatchedSignature.DockerReference, expectedDockerReference)} + if err := rules.validateSignedDockerManifestDigest(unmatchedSignature.DockerManifestDigest); err != nil { + return nil, err + } + if err := rules.validateSignedDockerReference(unmatchedSignature.DockerReference); err != nil { + return nil, err } signature := unmatchedSignature.Signature // Policy OK. return &signature, nil diff --git a/signature/signature_test.go b/signature/signature_test.go index 471e8b9f..db63abee 100644 --- a/signature/signature_test.go +++ b/signature/signature_test.go @@ -2,6 +2,7 @@ package signature import ( "encoding/json" + "fmt" "io/ioutil" "testing" @@ -149,7 +150,26 @@ func TestSign(t *testing.T) { signature, err := sig.sign(mech, TestKeyFingerprint) require.NoError(t, err) - verified, err := verifyAndExtractSignature(mech, signature, TestKeyFingerprint, sig.DockerReference) + verified, err := verifyAndExtractSignature(mech, signature, signatureAcceptanceRules{ + validateKeyIdentity: func(keyIdentity string) error { + if keyIdentity != TestKeyFingerprint { + return fmt.Errorf("Unexpected keyIdentity") + } + return nil + }, + validateSignedDockerReference: func(signedDockerReference string) error { + if signedDockerReference != sig.DockerReference { + return fmt.Errorf("Unexpected signedDockerReference") + } + return nil + }, + validateSignedDockerManifestDigest: func(signedDockerManifestDigest string) error { + if signedDockerManifestDigest != sig.DockerManifestDigest { + return fmt.Errorf("Unexpected signedDockerManifestDigest") + } + return nil + }, + }) require.NoError(t, err) assert.Equal(t, sig.Signature, *verified) @@ -167,40 +187,102 @@ func TestVerifyAndExtractSignature(t *testing.T) { mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory) require.NoError(t, err) + type triple struct{ keyIdentity, signedDockerReference, signedDockerManifestDigest string } + var wanted, recorded triple + // recordingRules are a plausible signatureAcceptanceRules implementations, but equally + // importantly record that we are passing the correct values to the rule callbacks. + recordingRules := signatureAcceptanceRules{ + validateKeyIdentity: func(keyIdentity string) error { + recorded.keyIdentity = keyIdentity + if keyIdentity != wanted.keyIdentity { + return fmt.Errorf("keyIdentity mismatch") + } + return nil + }, + validateSignedDockerReference: func(signedDockerReference string) error { + recorded.signedDockerReference = signedDockerReference + if signedDockerReference != wanted.signedDockerReference { + return fmt.Errorf("signedDockerReference mismatch") + } + return nil + }, + validateSignedDockerManifestDigest: func(signedDockerManifestDigest string) error { + recorded.signedDockerManifestDigest = signedDockerManifestDigest + if signedDockerManifestDigest != wanted.signedDockerManifestDigest { + return fmt.Errorf("signedDockerManifestDigest mismatch") + } + return nil + }, + } + signature, err := ioutil.ReadFile("./fixtures/image.signature") require.NoError(t, err) + signatureData := triple{ + keyIdentity: TestKeyFingerprint, + signedDockerReference: TestImageSignatureReference, + signedDockerManifestDigest: TestImageManifestDigest, + } // Successful verification - sig, err := verifyAndExtractSignature(mech, signature, TestKeyFingerprint, TestImageSignatureReference) + wanted = signatureData + recorded = triple{} + sig, err := verifyAndExtractSignature(mech, signature, recordingRules) require.NoError(t, err) assert.Equal(t, TestImageSignatureReference, sig.DockerReference) assert.Equal(t, TestImageManifestDigest, sig.DockerManifestDigest) + assert.Equal(t, signatureData, recorded) // For extra paranoia, test that we return a nil signature object on error. // Completely invalid signature. - sig, err = verifyAndExtractSignature(mech, []byte{}, TestKeyFingerprint, TestImageSignatureReference) + recorded = triple{} + sig, err = verifyAndExtractSignature(mech, []byte{}, recordingRules) assert.Error(t, err) assert.Nil(t, sig) + assert.Equal(t, triple{}, recorded) - sig, err = verifyAndExtractSignature(mech, []byte("invalid signature"), TestKeyFingerprint, TestImageSignatureReference) + recorded = triple{} + sig, err = verifyAndExtractSignature(mech, []byte("invalid signature"), recordingRules) assert.Error(t, err) assert.Nil(t, sig) + assert.Equal(t, triple{}, recorded) - // Valid signature of non-JSON + // Valid signature of non-JSON: asked for keyIdentity, only invalidBlobSignature, err := ioutil.ReadFile("./fixtures/invalid-blob.signature") require.NoError(t, err) - sig, err = verifyAndExtractSignature(mech, invalidBlobSignature, TestKeyFingerprint, TestImageSignatureReference) + recorded = triple{} + sig, err = verifyAndExtractSignature(mech, invalidBlobSignature, recordingRules) assert.Error(t, err) assert.Nil(t, sig) + assert.Equal(t, triple{keyIdentity: signatureData.keyIdentity}, recorded) - // Valid signature with a wrong key - sig, err = verifyAndExtractSignature(mech, signature, "unexpected fingerprint", TestImageSignatureReference) + // Valid signature with a wrong key: asked for keyIdentity, only + wanted = signatureData + wanted.keyIdentity = "unexpected fingerprint" + recorded = triple{} + sig, err = verifyAndExtractSignature(mech, signature, recordingRules) assert.Error(t, err) assert.Nil(t, sig) + assert.Equal(t, triple{keyIdentity: signatureData.keyIdentity}, recorded) + + // Valid signature with a wrong manifest digest: asked for keyIdentity and signedDockerManifestDigest + wanted = signatureData + wanted.signedDockerManifestDigest = "invalid digest" + recorded = triple{} + sig, err = verifyAndExtractSignature(mech, signature, recordingRules) + assert.Error(t, err) + assert.Nil(t, sig) + assert.Equal(t, triple{ + keyIdentity: signatureData.keyIdentity, + signedDockerManifestDigest: signatureData.signedDockerManifestDigest, + }, recorded) // Valid signature with a wrong image reference - sig, err = verifyAndExtractSignature(mech, signature, TestKeyFingerprint, "unexpected docker reference") + wanted = signatureData + wanted.signedDockerReference = "unexpected docker reference" + recorded = triple{} + sig, err = verifyAndExtractSignature(mech, signature, recordingRules) assert.Error(t, err) assert.Nil(t, sig) + assert.Equal(t, signatureData, recorded) }