tls: Support separate certificate key file

If a found ws-certs.d/ *.crt/*.cert file has a corresponding *.key file,
use that instead of expecting a merged private key in the certificate
file. This makes it easier to use Cockpit with LetsEncrypt or other
automatically maintained TLS certificates.

These days we mostly only need that for cockpit-tls. But fix it for
cockpit-ws as well, so that we keep remotectl happy (it complains
otherwise that it cannot find the key).

Drop the explicit `keyfile` parameter from `connection_crypto_init()`,
and let it try the two "separate key file" and "combined cert file"
cases by itself. Do the same EAFP approach in cockpit-ws.

Fixes #8922
Closes #13388

doc/guide/https.xml

@@ -30,11 +30,15 @@
 
     <para>Cockpit will load a certificate from the <code>/etc/cockpit/ws-certs.d</code>
       directory. It will use the last file with a <code>.cert</code> or <code>.crt</code>
-      extension in alphabetical order. The file should contain at least two
-      OpenSSL style PEM blocks. First one or more <literal>BEGIN CERTIFICATE</literal>
-      blocks for the server certificate and the intermediate certificate authorities
-      and a last one containing a <literal>BEGIN PRIVATE KEY</literal> or similar.
-      The key may not be encrypted. For example:</para>
+      extension in alphabetical order. The file should contain one or more OpenSSL
+      style <literal>BEGIN CERTIFICATE</literal> blocks for the server certificate and
+      the intermediate certificate authorities.</para>
+
+    <para>The private key can either be contained in the same <code>.cert</code>/<code>.crt</code>
+    file as an additional <literal>BEGIN PRIVATE KEY</literal> or similar block, or in
+    a separate file with the same name as the certificate, but with a <code>.key</code>
+    suffix instead. The key must not be encrypted. For example, a merged file looks like this:</para>
+
 <programlisting>
 -----BEGIN CERTIFICATE-----
 MIIDUzCCAjugAwIBAgIJAPXW+CuNYS6QMA0GCSqGSIb3DQEBCwUAMD8xKTAnBgNV

src/common/cockpitwebcertificate.c

@@ -112,6 +112,31 @@ cockpit_certificate_locate (char **error)
   return NULL;
 }
 
+/**
+ * cockpit_certificate_key_path:
+ *
+ * Return key file path for given certfile, i. e. replace ".crt" or ".cert"
+ * suffix with ".key". Invalid names exit the program. All usages of this
+ * function in our code control the file name, so that should not happen.
+ */
+char *
+cockpit_certificate_key_path (const char *certfile)
+{
+  int len = strlen (certfile);
+  char *keypath = NULL;
+
+  /* .cert suffix case: chop off suffix, append ".key" */
+  if (len > 5 && strcmp (certfile + len - 5, ".cert") == 0)
+    asprintfx (&keypath, "%.*s.key", len - 5, certfile);
+  /* *.crt suffix case */
+  else if (len > 4 && strcmp (certfile + len - 4, ".crt") == 0)
+    asprintfx (&keypath, "%.*s.key", len - 4, certfile);
+  else
+    errx (EXIT_FAILURE, "internal error: invalid certificate file name: %s", certfile);
+
+  return keypath;
+}
+
 /**
  * cockpit_certificate_parse:
  *

src/common/cockpitwebcertificate.h

@@ -21,6 +21,7 @@
 #define __COCKPIT_WEBCERTIFICATE_H__
 
 char *  cockpit_certificate_locate   (char **error);
+char *  cockpit_certificate_key_path (const char *certfile);
 int     cockpit_certificate_parse    (const char *file, char **cert, char **key);
 
 #endif /* __COCKPIT_WEBCERTIFICATE_H__ */

src/common/test-webcertificate.c

@@ -94,6 +94,22 @@ test_locate (void)
   rmdir (workdir);
 }
 
+static void
+test_keypath (void)
+{
+  char *path;
+
+  path = cockpit_certificate_key_path ("/etc/cockpit/ws-certs.d/50-good.cert");
+  g_assert_cmpstr (path, ==, "/etc/cockpit/ws-certs.d/50-good.key");
+  g_free (path);
+  path = cockpit_certificate_key_path ("a.cert");
+  g_assert_cmpstr (path, ==, "a.key");
+  g_free (path);
+  path = cockpit_certificate_key_path ("a.crt");
+  g_assert_cmpstr (path, ==, "a.key");
+  g_free (path);
+}
+
 int
 main (int argc,
       char *argv[])
@@ -101,6 +117,7 @@ main (int argc,
   cockpit_test_init (&argc, &argv);
 
   g_test_add_func ("/webcertificate/locate", test_locate);
+  g_test_add_func ("/webcertificate/keypath", test_keypath);
 
   return g_test_run ();
 }

src/tls/connection.c

@@ -771,17 +771,18 @@ set_x509_key_from_combined_file (gnutls_certificate_credentials_t x509_cred,
  * support for connections. If this function is not called, the server
  * will only be able to handle http requests.
  *
+ * The certificate file must either contain the key as well, or end with
+ * "*.crt" or "*.cert" and have a corresponding "*.key" file.
+ *
  * @certfile: Server TLS certificate file; cannot be %NULL
- * @keyfile: Server TLS key file; if the key is merged into @certfile, set this
- *           to %NULL.
  * @request_mode: Whether to ask for client certificates
  */
 void
 connection_crypto_init (const char *certfile,
-                        const char *keyfile,
                         gnutls_certificate_request_t request_mode)
 {
   int ret;
+  char *keyfile;
 
   assert (certfile != NULL);
   assert (parameters.x509_cred == NULL);
@@ -790,10 +791,18 @@ connection_crypto_init (const char *certfile,
   if (ret != GNUTLS_E_SUCCESS)
     errx (EXIT_FAILURE, "gnutls_certificate_allocate_credentials failed: %s", gnutls_strerror (ret));
 
-  if (keyfile)
-    ret = gnutls_certificate_set_x509_key_file (parameters.x509_cred, certfile, keyfile, GNUTLS_X509_FMT_PEM);
-  else
-    ret = set_x509_key_from_combined_file (parameters.x509_cred, certfile);
+  /* check if we have a separate key file */
+  keyfile = cockpit_certificate_key_path (certfile);
+  assert (keyfile);
+  ret = gnutls_certificate_set_x509_key_file (parameters.x509_cred, certfile, keyfile, GNUTLS_X509_FMT_PEM);
+
+  /* if not, fall back to combined file */
+  if (ret == GNUTLS_E_FILE_ERROR)
+    {
+      debug(CONNECTION, "connection_crypto_init: %s does not exist, falling back to combined cert+key", keyfile);
+      ret = set_x509_key_from_combined_file (parameters.x509_cred, certfile);
+    }
+  free (keyfile);
 
   if (ret != GNUTLS_E_SUCCESS)
     errx (EXIT_FAILURE, "Failed to initialize server certificate: %s", gnutls_strerror (ret));

src/tls/connection.h

@@ -31,7 +31,6 @@ connection_set_directories (const char *wsinstance_sockdir,
 
 void
 connection_crypto_init (const char *certfile,
-                        const char *keyfile,
                         gnutls_certificate_request_t request_mode);
 
 void

src/tls/main.c

@@ -121,7 +121,7 @@ main (int argc, char **argv)
       if (cockpit_conf_bool ("WebService", "ClientCertAuthentication", false))
         client_cert_mode = GNUTLS_CERT_REQUEST;
 
-      connection_crypto_init (certfile, NULL, client_cert_mode);
+      connection_crypto_init (certfile, client_cert_mode);
       free (certfile);
     }
 

src/tls/test-server.c

@@ -41,8 +41,8 @@
 
 #define SOCKET_ACTIVATION_HELPER BUILDDIR "/socket-activation-helper"
 #define COCKPIT_WS BUILDDIR "/cockpit-ws"
+/* this has a corresponding mock-server.key */
 #define CERTFILE SRCDIR "/src/bridge/mock-server.crt"
-#define KEYFILE SRCDIR "/src/bridge/mock-server.key"
 #define CERTKEYFILE SRCDIR "/src/ws/mock-combined.crt"
 #define CERTCHAINKEYFILE SRCDIR "/test/verify/files/cert-chain.cert"
 
@@ -64,7 +64,6 @@ typedef struct {
 
 typedef struct {
   const char *certfile;
-  const char *keyfile;
   int cert_request_mode;
   int idle_timeout;
   const char *client_crt;
@@ -74,12 +73,10 @@ typedef struct {
 
 static const TestFixture fixture_separate_crt_key = {
   .certfile = CERTFILE,
-  .keyfile = KEYFILE,
 };
 
 static const TestFixture fixture_separate_crt_key_client_cert = {
   .certfile = CERTFILE,
-  .keyfile = KEYFILE,
   .cert_request_mode = GNUTLS_CERT_REQUEST,
   .client_crt = CLIENT_CERTFILE,
   .client_key = CLIENT_KEYFILE,
@@ -88,7 +85,6 @@ static const TestFixture fixture_separate_crt_key_client_cert = {
 
 static const TestFixture fixture_expired_client_cert = {
   .certfile = CERTFILE,
-  .keyfile = KEYFILE,
   .cert_request_mode = GNUTLS_CERT_REQUEST,
   .client_crt = CLIENT_EXPIRED_CERTFILE,
   .client_key = CLIENT_KEYFILE,
@@ -97,7 +93,6 @@ static const TestFixture fixture_expired_client_cert = {
 
 static const TestFixture fixture_alternate_client_cert = {
   .certfile = CERTFILE,
-  .keyfile = KEYFILE,
   .cert_request_mode = GNUTLS_CERT_REQUEST,
   .client_crt = ALTERNATE_CERTFILE,
   .client_key = ALTERNATE_KEYFILE,
@@ -358,7 +353,7 @@ setup (TestCase *tc, gconstpointer data)
 
   server_init (tc->ws_socket_dir, tc->runtime_dir, fixture ? fixture->idle_timeout : 0, server_port);
   if (fixture && fixture->certfile)
-    connection_crypto_init (fixture->certfile, fixture->keyfile, fixture->cert_request_mode);
+    connection_crypto_init (fixture->certfile, fixture->cert_request_mode);
 
   tc->server_addr.sin_family = AF_INET;
   tc->server_addr.sin_port = htons (server_port);

src/ws/cockpitcertificate.c

@@ -380,21 +380,42 @@ cockpit_certificate_load (const gchar *cert_path,
                           GError **error)
 {
   int r;
-  g_autofree gchar *certs = NULL;
-  g_autofree gchar *key = NULL;
-  g_autofree gchar *combined = NULL;
+  g_autofree gchar *key_path = cockpit_certificate_key_path (cert_path);
   GTlsCertificate *cert;
+  GError *key_error = NULL;
 
-  r = cockpit_certificate_parse (cert_path, &certs, &key);
-  if (r < 0)
+  /* check if we have a separate .key file */
+  cert = g_tls_certificate_new_from_files (cert_path, key_path, &key_error);
+  if (cert)
     {
-      g_set_error (error, G_IO_ERROR, g_io_error_from_errno (-r), "Failed to load %s: %s", cert_path, g_strerror (-r));
-      return NULL;
+      g_debug ("loaded separate cert %s and key %s", cert_path, key_path);
+    }
+  else if (g_error_matches (key_error, G_FILE_ERROR, G_FILE_ERROR_NOENT))
+    {
+      /* combined cert+key file */
+      g_autofree gchar *certs = NULL;
+      g_autofree gchar *key = NULL;
+      g_autofree gchar *combined = NULL;
+
+      g_debug ("%s does not exist, falling back to combined certificate", key_path);
+      g_clear_error (&key_error);
+
+      r = cockpit_certificate_parse (cert_path, &certs, &key);
+      if (r < 0)
+        {
+          g_set_error (error, G_IO_ERROR, g_io_error_from_errno (-r), "Failed to load %s: %s", cert_path, g_strerror (-r));
+          return NULL;
+        }
+
+      /* Gio only has constructors for parsing certs and key from one string, so combine them */
+      combined = g_strconcat (certs, key, NULL);
+      cert = g_tls_certificate_new_from_pem (combined, -1, error);
+    }
+  else
+    {
+      g_propagate_error (error, key_error);
     }
 
-  /* Gio only has constructors for parsing certs and key from one string, so combine them */
-  combined = g_strconcat (certs, key, NULL);
-  cert = g_tls_certificate_new_from_pem (combined, -1, error);
   if (cert == NULL)
     g_prefix_error (error, "%s: ", cert_path);
   else

test/verify/check-connection

@@ -322,24 +322,28 @@ class TestConnection(MachineCase):
         m.upload(["verify/files/cert-chain.cert"], "/etc/cockpit/ws-certs.d")
         m.execute("! selinuxenabled || chcon --type svirt_sandbox_file_t /etc/cockpit/ws-certs.d/cert-chain.cert")
 
-        # This should also reset the file context
-        m.restart_cockpit()
-
-        # Should use the new certificates and entire chain should show up
-        output = m.execute('openssl s_client -connect 172.27.0.15:9090 2>&1')
-        self.assertIn("DONE", output)
-        self.assertRegex(output, "s:/?CN *= *localhost")
-        self.assertRegex(output, "1 s:/?OU *= *Intermediate")
-
-        # *.crt file also works; added in PR #13388
-        if m.image not in ["rhel-8-1-distropkg", "rhel-8-2-distropkg"]:
-            m.execute ("mv /etc/cockpit/ws-certs.d/cert-chain.cert /etc/cockpit/ws-certs.d/cert-chain.crt")
+        def check_cert_chain():
+            # This should also reset the file context
             m.restart_cockpit()
             output = m.execute('openssl s_client -connect 172.27.0.15:9090 2>&1')
             self.assertIn("DONE", output)
             self.assertRegex(output, "s:/?CN *= *localhost")
             self.assertRegex(output, "1 s:/?OU *= *Intermediate")
 
+        check_cert_chain()
+
+        # *.crt file also works; added in PR #13388
+        if m.image not in ["rhel-8-1-distropkg", "rhel-8-2-distropkg"]:
+            m.execute ("mv /etc/cockpit/ws-certs.d/cert-chain.cert /etc/cockpit/ws-certs.d/cert-chain.crt")
+            check_cert_chain()
+
+        # separate *.key file instead of merged .cert; added in PR #13388
+        if m.image not in ["rhel-8-1-distropkg", "rhel-8-2-distropkg"]:
+            m.execute("sed -n '/---BEGIN PRIVATE KEY/,$ p' /etc/cockpit/ws-certs.d/cert-chain.crt > /etc/cockpit/ws-certs.d/cert-chain.key")
+            m.execute("sed -i '/---BEGIN PRIVATE KEY/,$ d' /etc/cockpit/ws-certs.d/cert-chain.crt")
+            check_cert_chain()
+
+
         # login handler: correct password
         m.execute("curl -k -c cockpit.jar -s --head --header 'Authorization: Basic {}' https://127.0.0.1:9090/cockpit/login".format(
             base64.b64encode(b"admin:foobar").decode(), ))