Code review feedback

pkg/pki/ssh/encode.go

@@ -13,7 +13,7 @@ const (
 	pemType   = "SSH SIGNATURE"
 )
 
-func Armor(s *ssh.Signature, p ssh.PublicKey) string {
+func Armor(s *ssh.Signature, p ssh.PublicKey) []byte {
 	sig := WrappedSig{
 		Version:       1,
 		PublicKey:     string(p.Marshal()),
@@ -27,47 +27,50 @@ func Armor(s *ssh.Signature, p ssh.PublicKey) string {
 		Type:  pemType,
 		Bytes: ssh.Marshal(sig),
 	})
-	return string(enc)
+	return enc
 }
 
-func Decode(s string) (*ssh.Signature, ssh.PublicKey, error) {
-	pemBlock, _ := pem.Decode([]byte(s))
+func Decode(b []byte) (*Signature, error) {
+	pemBlock, _ := pem.Decode(b)
 	if pemBlock == nil {
-		return nil, nil, errors.New("unable to decode pem file")
+		return nil, errors.New("unable to decode pem file")
 	}
 
 	if pemBlock.Type != pemType {
-		return nil, nil, fmt.Errorf("wrong pem block type: %s. Expected SSH-SIGNATURE", pemBlock.Type)
+		return nil, fmt.Errorf("wrong pem block type: %s. Expected SSH-SIGNATURE", pemBlock.Type)
 	}
 
 	// Now we unmarshal it into the Signature block
 	sig := WrappedSig{}
 	if err := ssh.Unmarshal(pemBlock.Bytes, &sig); err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	if sig.Version != 1 {
-		return nil, nil, fmt.Errorf("unsupported signature version: %d", sig.Version)
+		return nil, fmt.Errorf("unsupported signature version: %d", sig.Version)
 	}
 	if string(sig.MagicHeader[:]) != magicHeader {
-		return nil, nil, fmt.Errorf("invalid magic header: %s", sig.MagicHeader)
+		return nil, fmt.Errorf("invalid magic header: %s", sig.MagicHeader)
 	}
 	if sig.Namespace != "file" {
-		return nil, nil, fmt.Errorf("invalid signature namespace: %s", sig.Namespace)
+		return nil, fmt.Errorf("invalid signature namespace: %s", sig.Namespace)
 	}
 	// TODO: Also check the HashAlgorithm type here.
 
 	// Now we can unpack the Signature and PublicKey blocks
 	sshSig := ssh.Signature{}
 	if err := ssh.Unmarshal([]byte(sig.Signature), &sshSig); err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 	// TODO: check the format here (should be rsa-sha512)
 
 	pk, err := ssh.ParsePublicKey([]byte(sig.PublicKey))
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
-	return &sshSig, pk, nil
+	return &Signature{
+		signature: &sshSig,
+		pk:        pk,
+	}, nil
 }

pkg/pki/ssh/sign.go

@@ -56,20 +56,20 @@ func sign(s ssh.AlgorithmSigner, m io.Reader) (*ssh.Signature, error) {
 	return sig, nil
 }
 
-func Sign(sshPrivateKey string, data io.Reader) (string, error) {
+func Sign(sshPrivateKey string, data io.Reader) ([]byte, error) {
 	s, err := ssh.ParsePrivateKey([]byte(sshPrivateKey))
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
 	as, ok := s.(ssh.AlgorithmSigner)
 	if !ok {
-		return "", err
+		return nil, err
 	}
 
 	sig, err := sign(as, data)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
 	armored := Armor(sig, s.PublicKey())

pkg/pki/ssh/sign_test.go

@@ -112,17 +112,17 @@ func TestFromOpenSSH(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err := Verify(bytes.NewReader(data), string(sigBytes), []byte(sshPublicKey)); err != nil {
+	if err := Verify(bytes.NewReader(data), sigBytes, []byte(sshPublicKey)); err != nil {
 		t.Error(err)
 	}
 
 	// It should not verify if we check against the other public key
-	if err := Verify(bytes.NewReader(data), string(sigBytes), []byte(otherSshPublicKey)); err == nil {
+	if err := Verify(bytes.NewReader(data), sigBytes, []byte(otherSshPublicKey)); err == nil {
 		t.Error("expected error with incorrect key")
 	}
 
 	// It should not verify if the data is tampered
-	if err := Verify(strings.NewReader("bad data"), string(sigBytes), []byte(sshPublicKey)); err == nil {
+	if err := Verify(strings.NewReader("bad data"), sigBytes, []byte(sshPublicKey)); err == nil {
 		t.Error("expected error with incorrect data")
 	}
 }
@@ -190,7 +190,7 @@ func TestRoundTrip(t *testing.T) {
 	}
 
 	// Now check it against an invalid signature data.
-	if err := Verify(bytes.NewReader(data), "invalid signature!", []byte(sshPublicKey)); err == nil {
+	if err := Verify(bytes.NewReader(data), []byte("invalid signature!"), []byte(sshPublicKey)); err == nil {
 		t.Error("expected error!")
 	}
 

pkg/pki/ssh/ssh.go

@@ -17,7 +17,6 @@ limitations under the License.
 package ssh
 
 import (
-	"bytes"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -36,14 +35,11 @@ func NewSignature(r io.Reader) (*Signature, error) {
 	if err != nil {
 		return nil, err
 	}
-	sig, pk, err := Decode(string(b))
+	sig, err := Decode(b)
 	if err != nil {
 		return nil, err
 	}
-	return &Signature{
-		signature: sig,
-		pk:        pk,
-	}, nil
+	return sig, nil
 }
 
 // CanonicalValue implements the pki.Signature interface
@@ -57,11 +53,6 @@ func (s Signature) Verify(r io.Reader, k interface{}) error {
 		return fmt.Errorf("ssh signature has not been initialized")
 	}
 
-	message, err := ioutil.ReadAll(r)
-	if err != nil {
-		return err
-	}
-
 	key, ok := k.(*PublicKey)
 	if !ok {
 		return fmt.Errorf("Invalid public key type for: %v", k)
@@ -75,7 +66,7 @@ func (s Signature) Verify(r io.Reader, k interface{}) error {
 	if err != nil {
 		return err
 	}
-	return Verify(bytes.NewReader(message), string(cs), ck)
+	return Verify(r, cs, ck)
 }
 
 // PublicKey contains an ssh PublicKey
@@ -101,7 +92,7 @@ func NewPublicKey(r io.Reader) (*PublicKey, error) {
 // CanonicalValue implements the pki.PublicKey interface
 func (k PublicKey) CanonicalValue() ([]byte, error) {
 	if k.key == nil {
-		return nil, fmt.Errorf("x509 public key has not been initialized")
+		return nil, fmt.Errorf("ssh public key has not been initialized")
 	}
 	return ssh.MarshalAuthorizedKey(k.key), nil
 }

pkg/pki/ssh/verify.go

@@ -7,8 +7,8 @@ import (
 	"golang.org/x/crypto/ssh"
 )
 
-func Verify(message io.Reader, armoredSignature string, publicKey []byte) error {
-	decodedSignature, _, err := Decode(armoredSignature)
+func Verify(message io.Reader, armoredSignature []byte, publicKey []byte) error {
+	decodedSignature, err := Decode(armoredSignature)
 	if err != nil {
 		return err
 	}
@@ -32,5 +32,5 @@ func Verify(message io.Reader, armoredSignature string, publicKey []byte) error
 	}
 	signedMessage := ssh.Marshal(toVerify)
 	signedMessage = append([]byte(magicHeader), signedMessage...)
-	return desiredPk.Verify(signedMessage, decodedSignature)
+	return desiredPk.Verify(signedMessage, decodedSignature.signature)
 }

tests/e2e_test.go

@@ -206,13 +206,7 @@ func TestSSH(t *testing.T) {
 		"--public-key", pubPath, "--pki-format", "ssh")
 	outputContains(t, out, "Created entry at")
 
-	// Output looks like "Created entry at $URL/UUID", so grab the UUID:
-	url := strings.Split(strings.TrimSpace(out), " ")[3]
-	splitUrl := strings.Split(url, "/")
-	uuid := splitUrl[len(splitUrl)-1]
-
-	// Wait and check it.
-	time.Sleep(3 * time.Second)
+	uuid := getUUIDFromUploadOutput(t, out)
 
 	out = runCli(t, "verify", "--artifact", artifactPath, "--signature", sigPath,
 		"--public-key", pubPath, "--pki-format", "ssh")