crypto/tls: better error messages when PEM inputs are switched.

This change causes the types of skipped PEM blocks to be recorded when
no certificate or private-key data is found in a PEM input. This allows
for better error messages to be return in the case of common errors like
switching the certifiate and key inputs to X509KeyPair.

Fixes #11092

Change-Id: Ifc155a811cdcddd93b5787fe16a84c972011f2f7
Reviewed-on: https://go-review.googlesource.com/14054
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

tls.go

@@ -12,6 +12,7 @@ import (
 	"crypto/x509"
 	"encoding/pem"
 	"errors"
+	"fmt"
 	"io/ioutil"
 	"net"
 	"strings"
@@ -182,32 +183,50 @@ func LoadX509KeyPair(certFile, keyFile string) (Certificate, error) {
 // X509KeyPair parses a public/private key pair from a pair of
 // PEM encoded data.
 func X509KeyPair(certPEMBlock, keyPEMBlock []byte) (Certificate, error) {
-	var cert Certificate
-	var certDERBlock *pem.Block
 	fail := func(err error) (Certificate, error) { return Certificate{}, err }
+
+	var cert Certificate
+	var skippedBlockTypes []string
 	for {
+		var certDERBlock *pem.Block
 		certDERBlock, certPEMBlock = pem.Decode(certPEMBlock)
 		if certDERBlock == nil {
 			break
 		}
 		if certDERBlock.Type == "CERTIFICATE" {
 			cert.Certificate = append(cert.Certificate, certDERBlock.Bytes)
+		} else {
+			skippedBlockTypes = append(skippedBlockTypes, certDERBlock.Type)
 		}
 	}
 
 	if len(cert.Certificate) == 0 {
-		return fail(errors.New("crypto/tls: failed to parse certificate PEM data"))
+		if len(skippedBlockTypes) == 0 {
+			return fail(errors.New("crypto/tls: failed to find any PEM data in certificate input"))
+		} else if len(skippedBlockTypes) == 1 && strings.HasSuffix(skippedBlockTypes[0], "PRIVATE KEY") {
+			return fail(errors.New("crypto/tls: failed to find certificate PEM data in certificate input, but did find a private key; PEM inputs may have been switched"))
+		} else {
+			return fail(fmt.Errorf("crypto/tls: failed to find \"CERTIFICATE\" PEM block in certificate input after skipping PEM blocks of the following types: %v", skippedBlockTypes))
+		}
 	}
 
+	skippedBlockTypes = skippedBlockTypes[:0]
 	var keyDERBlock *pem.Block
 	for {
 		keyDERBlock, keyPEMBlock = pem.Decode(keyPEMBlock)
 		if keyDERBlock == nil {
-			return fail(errors.New("crypto/tls: failed to parse key PEM data"))
+			if len(skippedBlockTypes) == 0 {
+				return fail(errors.New("crypto/tls: failed to find any PEM data in key input"))
+			} else if len(skippedBlockTypes) == 1 && skippedBlockTypes[0] == "CERTIFICATE" {
+				return fail(errors.New("crypto/tls: found a certificate rather than a key in the PEM for the private key"))
+			} else {
+				return fail(fmt.Errorf("crypto/tls: failed to find PEM block with type ending in \"PRIVATE KEY\" in key input after skipping PEM blocks of the following types: %v", skippedBlockTypes))
+			}
 		}
 		if keyDERBlock.Type == "PRIVATE KEY" || strings.HasSuffix(keyDERBlock.Type, " PRIVATE KEY") {
 			break
 		}
+		skippedBlockTypes = append(skippedBlockTypes, keyDERBlock.Type)
 	}
 
 	var err error

tls_test.go

@@ -104,6 +104,38 @@ func TestX509KeyPair(t *testing.T) {
 	}
 }
 
+func TestX509KeyPairErrors(t *testing.T) {
+	_, err := X509KeyPair([]byte(rsaKeyPEM), []byte(rsaCertPEM))
+	if err == nil {
+		t.Fatalf("X509KeyPair didn't return an error when arguments were switched")
+	}
+	if subStr := "been switched"; !strings.Contains(err.Error(), subStr) {
+		t.Fatalf("Expected %q in the error when switching arguments to X509KeyPair, but the error was %q", subStr, err)
+	}
+
+	_, err = X509KeyPair([]byte(rsaCertPEM), []byte(rsaCertPEM))
+	if err == nil {
+		t.Fatalf("X509KeyPair didn't return an error when both arguments were certificates")
+	}
+	if subStr := "certificate"; !strings.Contains(err.Error(), subStr) {
+		t.Fatalf("Expected %q in the error when both arguments to X509KeyPair were certificates, but the error was %q", subStr, err)
+	}
+
+	const nonsensePEM = `
+-----BEGIN NONSENSE-----
+Zm9vZm9vZm9v
+-----END NONSENSE-----
+`
+
+	_, err = X509KeyPair([]byte(nonsensePEM), []byte(nonsensePEM))
+	if err == nil {
+		t.Fatalf("X509KeyPair didn't return an error when both arguments were nonsense")
+	}
+	if subStr := "NONSENSE"; !strings.Contains(err.Error(), subStr) {
+		t.Fatalf("Expected %q in the error when both arguments to X509KeyPair were nonsense, but the error was %q", subStr, err)
+	}
+}
+
 func TestX509MixedKeyPair(t *testing.T) {
 	if _, err := X509KeyPair([]byte(rsaCertPEM), []byte(ecdsaKeyPEM)); err == nil {
 		t.Error("Load of RSA certificate succeeded with ECDSA private key")