[GitHub] [trafficcontrol] JBevillC commented on a change in pull request #3382: Add enhanced X509 Certificate/Private RSA Key validation to Traffic Ops (AddSSLKeys Endpoint)

[GitBox <gi...@apache.org>]

JBevillC commented on a change in pull request #3382: Add enhanced X509 Certificate/Private RSA Key validation to Traffic Ops (AddSSLKeys Endpoint)
URL: https://github.com/apache/trafficcontrol/pull/3382#discussion_r263867505
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/deliveryservice/keys.go
 ##########
 @@ -268,29 +273,107 @@ WHERE r.pattern = $2
 	return xmlID, true, nil
 }
 
+// decode the private key
+// check for proper algorithm.
+// check for correct number of keys
+// return private key object, cleaned private key PEM, or any errors.
+func decodePrivateKey(pemPrivateKey string) (*rsa.PrivateKey, string, error) {
+
+	// Remove any white space before decoding
+	trimmedPrivateKey := strings.TrimSpace(pemPrivateKey)
+
+	// Check for proper key count before attempting to decode.
+	blockCount := strings.Count(trimmedPrivateKey, "\n-----END")
+	if blockCount < 1 {
+		return nil, "", errors.New("no private keys found")
+	} else if blockCount > 1 {
+		return nil, "", errors.New("multiple private keys found")
+	}
+
+	// Attempt to decode pem encoded text into PEM block.
+	block, _ := pem.Decode([]byte(trimmedPrivateKey))
+	if block == nil {
+		return nil, "", errors.New("could not decode pem-encoded private key")
+	}
+
+	// Check for encrypted keys or other unsupported key types
+	if strings.Contains(block.Type, "ENCRYPTED") {
+		return nil, "", errors.New("encrypted private key not supported")
+	}
+
+	// Check block headers for encryption.
+	for _, value := range block.Headers {
+		if strings.Contains(value, "ENCRYPTED") {
+			return nil, "", errors.New("encrypted private key not supported")
+		}
+	}
+
+	// Attempt to parse PEM block as a PKCS#1 or PKCS#8 formatted RSA Private Key.
+	if privateKey, err := x509.ParsePKCS8PrivateKey(block.Bytes); privateKey != nil && err == nil {
+
+		if rsaPrivateKey, ok := privateKey.(*rsa.PrivateKey); privateKey != nil && ok {
+			// Found PKCS #8 RSA private key.
+			return rsaPrivateKey, trimmedPrivateKey, nil
+		} else {
+		  keyType := reflect.TypeOf(privateKey)
+			return nil, "", errors.New("private key algorithm not supported: " +  keyType.Name())
+		}
+	} else if rsaPrivateKey, err := x509.ParsePKCS1PrivateKey(block.Bytes); rsaPrivateKey != nil && err == nil {
+		// Found PKCS #1 RSA private key.
+		return rsaPrivateKey, trimmedPrivateKey, nil
+	}
+
+	return nil, "", errors.New("unk")
+}
+
 // verify the server certificate chain and return the
 // certificate and its chain in the proper order. Returns a verified
 // and ordered certificate and CA chain.
 // If the cert verification returns UnknownAuthorityError, return true to
 // indicate that the certs are signed by an unknown authority (e.g. self-signed).
-func verifyCertificate(certificate string, rootCA string) (string, bool, error) {
+func verifyCertKeyPair(certificate string, privateKey string, rootCA string) (string, string, bool, error) {
+
+	// decode private key, check algorithm,
+	rsaPrivateKey, privateKeyPem,  err := decodePrivateKey(privateKey)
+	if err != nil {
+		return "", "", false, err
+	}
+
 	// decode, verify, and order certs for storage
 	certs := strings.SplitAfter(certificate, PemCertEndMarker)
 	if len(certs) <= 1 {
-		return "", false, errors.New("no certificate chain to verify")
+		return "", "", false, errors.New("no certificate chain to verify")
 	}
 
 	// decode and verify the server certificate
 	block, _ := pem.Decode([]byte(certs[0]))
 	if block == nil {
-		return "", false, errors.New("could not decode pem-encoded server certificate")
+		return "", "",  false, errors.New("could not decode pem-encoded server certificate")
 	}
 	cert, err := x509.ParseCertificate(block.Bytes)
 	if err != nil {
-		return "", false, errors.New("could not parse the server certificate: " + err.Error())
+		return "", "", false, errors.New("could not parse the server certificate: " + err.Error())
 	}
+
+	// Check that the server certificate has the proper public key algorithm.
+	if cert.PublicKeyAlgorithm != x509.RSA {
 
 Review comment:
   @rob05c yeah, my understanding  according to @dneuman64 is that EC signed x509 certificate support does not yet work with the Traffic Router.  My tests with Traffic Router and non-EC DSA keys also did not seem to work and apparently x509 + non-EC DSA authentication is quite rare based on what I've read on the subject.  

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services