Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions signature/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,7 @@ func SignDockerManifest(m []byte, dockerReference string, mech SigningMechanism,
if err != nil {
return nil, err
}
sig := privateSignature{
Signature{
DockerManifestDigest: manifestDigest,
DockerReference: dockerReference,
},
}
sig := newUntrustedSignature(manifestDigest, dockerReference)
return sig.sign(mech, keyIdentity)
}

Expand Down
Binary file added signature/fixtures/no-optional-fields.signature
Binary file not shown.
2 changes: 2 additions & 0 deletions signature/fixtures_info_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ const (
TestImageSignatureReference = "testing/manifest"
// TestKeyFingerprint is the fingerprint of the private key in this directory.
TestKeyFingerprint = "1D8230F6CDB6A06716E414C1DB72F2188BB46CC8"
// TestKeyShortID is the short ID of the private key in this directory.
TestKeyShortID = "DB72F2188BB46CC8"
)
17 changes: 17 additions & 0 deletions signature/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,23 @@ func validateExactMapKeys(m map[string]interface{}, expectedKeys ...string) erro
return nil
}

// int64Field returns a member fieldName of m, if it is an int64, or an error.
func int64Field(m map[string]interface{}, fieldName string) (int64, error) {
untyped, ok := m[fieldName]
if !ok {
return -1, jsonFormatError(fmt.Sprintf("Field %s missing", fieldName))
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to return -1 here. the 0 is default for int64 when not set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well it is returning -1, not 0. Anyway, there is no “good” undefined value to return here if the caller makes a mistake and ignores err. -1 is perhaps slightly less likely to be valid than a non-negative number… but it really is anybody’s guess whether that makes it more likely to be noticed as invalid or more likely to have completely unpredicted effects. (Go’s error handling sucks, it would be nice to have something like optional checked exceptions, so that a caller can’t ignore an error by mistake without it being very visible.)

It seems to me not to be worth it to bother with named return parameters so that we can skip explicitly naming a value returned in the error case—especially because that also makes a decision, namely to use 0, only that decision is not explicitly visible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mtrmac the called should never ignore "err", doing so is a programmer error and we can't prevent all human errors :-)

I'm fine with -1, just pointing out it is something unusual to me as usually if the return value is not defined, we default to a default value of the type (which in case of int64 is '0').

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mtrmac the called should never ignore "err", doing so is a programmer error and we can't prevent all human errors :-)

True, and yet, we are in the business of engineering tools and interfaces to minimize errors anyway. (Ordinarily I wouldn’t bother, but the signature subpackage brings out my worst pedantic tendencies.)

}
f, ok := untyped.(float64)
if !ok {
return -1, jsonFormatError(fmt.Sprintf("Field %s is not a number", fieldName))
}
v := int64(f)
if float64(v) != f {
return -1, jsonFormatError(fmt.Sprintf("Field %s is not an integer", fieldName))
}
return v, nil
}

// mapField returns a member fieldName of m, if it is a JSON map, or an error.
func mapField(m map[string]interface{}, fieldName string) (map[string]interface{}, error) {
untyped, ok := m[fieldName]
Expand Down
33 changes: 33 additions & 0 deletions signature/json_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package signature

import (
"encoding/json"
"fmt"
"math"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -44,6 +46,37 @@ func TestValidateExactMapKeys(t *testing.T) {
assert.Error(t, err)
}

func TestInt64Field(t *testing.T) {
// Field not found
_, err := int64Field(mSI{"a": "x"}, "b")
assert.Error(t, err)

// Field has a wrong type
_, err = int64Field(mSI{"a": "string"}, "a")
assert.Error(t, err)

for _, value := range []float64{
0.5, // Fractional input
math.Inf(1), // Infinity
math.NaN(), // NaN
} {
_, err = int64Field(mSI{"a": value}, "a")
assert.Error(t, err, fmt.Sprintf("%f", value))
}

// Success
// The float64 type has 53 bits of effective precision, so ±1FFFFFFFFFFFFF is the
// range of integer values which can all be represented exactly (beyond that,
// some are representable if they are divisible by a high enough power of 2,
// but most are not).
for _, value := range []int64{0, 1, -1, 0x1FFFFFFFFFFFFF, -0x1FFFFFFFFFFFFF} {
testName := fmt.Sprintf("%d", value)
v, err := int64Field(mSI{"a": float64(value), "b": nil}, "a")
require.NoError(t, err, testName)
assert.Equal(t, value, v, testName)
}
}

func TestMapField(t *testing.T) {
// Field not found
_, err := mapField(mSI{"a": mSI{}}, "b")
Expand Down
38 changes: 38 additions & 0 deletions signature/mechanism.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@ package signature

import (
"bytes"
"errors"
"fmt"
"io/ioutil"
"strings"

"github.com/mtrmac/gpgme"
"golang.org/x/crypto/openpgp"
Copy link
Contributor

Choose a reason for hiding this comment

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

this wont work for openshift, we will not build with gpgme :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does not add any new uses of gpgme. Getting rid of gpgme happens in #207 not here (though the two do touch the same code, so the latter one will have to be modified. I expect the newly added UntrustedSignatureContents to remain in mechanism.go as a shared helper, called by both GPG backends.)

)

// SigningMechanism abstracts a way to sign binary blobs and verify their signatures.
Expand All @@ -21,6 +25,12 @@ type SigningMechanism interface {
Sign(input []byte, keyIdentity string) ([]byte, error)
// Verify parses unverifiedSignature and returns the content and the signer's identity
Verify(unverifiedSignature []byte) (contents []byte, keyIdentity string, err error)
// UntrustedSignatureContents returns UNTRUSTED contents of the signature WITHOUT ANY VERIFICATION,
// along with a short identifier of the key used for signing.
// WARNING: The short key identifier (which correponds to "Key ID" for OpenPGP keys)
// is NOT the same as a "key identity" used in other calls ot this interface, and
// the values may have no recognizable relationship if the public key is not available.
UntrustedSignatureContents(untrustedSignature []byte) (untrustedContents []byte, shortKeyIdentifier string, err error)
}

// A GPG/OpenPGP signing mechanism.
Expand Down Expand Up @@ -119,3 +129,31 @@ func (m gpgSigningMechanism) Verify(unverifiedSignature []byte) (contents []byte
}
return signedBuffer.Bytes(), sig.Fingerprint, nil
}

// UntrustedSignatureContents returns UNTRUSTED contents of the signature WITHOUT ANY VERIFICATION,
// along with a short identifier of the key used for signing.
// WARNING: The short key identifier (which correponds to "Key ID" for OpenPGP keys)
// is NOT the same as a "key identity" used in other calls ot this interface, and
// the values may have no recognizable relationship if the public key is not available.
func (m gpgSigningMechanism) UntrustedSignatureContents(untrustedSignature []byte) (untrustedContents []byte, shortKeyIdentifier string, err error) {
// This uses the Golang-native OpenPGP implementation instead of gpgme because we are not doing any cryptography.
md, err := openpgp.ReadMessage(bytes.NewReader(untrustedSignature), openpgp.EntityList{}, nil, nil)
if err != nil {
return nil, "", err
}
if !md.IsSigned {
return nil, "", errors.New("The input is not a signature")
}
content, err := ioutil.ReadAll(md.UnverifiedBody)
if err != nil {
// Coverage: An error during reading the body can happen only if
// 1) the message is encrypted, which is not our case (and we don’t give ReadMessage the key
// to decrypt the contents anyway), or
// 2) the message is signed AND we give ReadMessage a correspnding public key, which we don’t.
return nil, "", err
}

// Uppercase the key ID for minimal consistency with the gpgme-returned fingerprints
// (but note that key ID is a suffix of the fingerprint only for V4 keys, not V3)!
return content, strings.ToUpper(fmt.Sprintf("%016X", md.SignedByKeyId)), nil
}
56 changes: 56 additions & 0 deletions signature/mechanism_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,59 @@ func TestGPGSigningMechanismVerify(t *testing.T) {

// The various GPG/GPGME failures cases are not obviously easy to reach.
}

func TestGPGSigningMechanismUntrustedSignatureContents(t *testing.T) {
mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
require.NoError(t, err)

// A valid signature
signature, err := ioutil.ReadFile("./fixtures/invalid-blob.signature")
require.NoError(t, err)
content, shortKeyID, err := mech.UntrustedSignatureContents(signature)
require.NoError(t, err)
assert.Equal(t, []byte("This is not JSON\n"), content)
assert.Equal(t, TestKeyShortID, shortKeyID)

// Completely invalid signature.
_, _, err = mech.UntrustedSignatureContents([]byte{})
assert.Error(t, err)

_, _, err = mech.UntrustedSignatureContents([]byte("invalid signature"))
assert.Error(t, err)

// Literal packet, not a signature
signature, err = ioutil.ReadFile("./fixtures/unsigned-literal.signature")
require.NoError(t, err)
content, shortKeyID, err = mech.UntrustedSignatureContents(signature)
assert.Error(t, err)

// Encrypted data, not a signature.
signature, err = ioutil.ReadFile("./fixtures/unsigned-encrypted.signature")
require.NoError(t, err)
content, shortKeyID, err = mech.UntrustedSignatureContents(signature)
assert.Error(t, err)

// Expired signature
signature, err = ioutil.ReadFile("./fixtures/expired.signature")
require.NoError(t, err)
content, shortKeyID, err = mech.UntrustedSignatureContents(signature)
require.NoError(t, err)
assert.Equal(t, []byte("This signature is expired.\n"), content)
assert.Equal(t, TestKeyShortID, shortKeyID)

// Corrupt signature
signature, err = ioutil.ReadFile("./fixtures/corrupt.signature")
require.NoError(t, err)
content, shortKeyID, err = mech.UntrustedSignatureContents(signature)
require.NoError(t, err)
assert.Equal(t, []byte(`{"critical":{"identity":{"docker-reference":"testing/manifest"},"image":{"docker-manifest-digest":"sha256:20bf21ed457b390829cdbeec8795a7bea1626991fda603e0d01b4e7f60427e55"},"type":"atomic container signature"},"optional":{"creator":"atomic ","timestamp":1458239713}}`), content)
assert.Equal(t, TestKeyShortID, shortKeyID)

// Valid signature with an unknown key
signature, err = ioutil.ReadFile("./fixtures/unknown-key.signature")
require.NoError(t, err)
content, shortKeyID, err = mech.UntrustedSignatureContents(signature)
require.NoError(t, err)
assert.Equal(t, []byte(`{"critical":{"identity":{"docker-reference":"testing/manifest"},"image":{"docker-manifest-digest":"sha256:20bf21ed457b390829cdbeec8795a7bea1626991fda603e0d01b4e7f60427e55"},"type":"atomic container signature"},"optional":{"creator":"atomic 0.1.13-dev","timestamp":1464633474}}`), content)
assert.Equal(t, "E5476D1110D07803", shortKeyID)
}
Loading