feat: Handle hostkeys like OpenSSH

fix: memory leak
fix: add defaults after parsing
fix: set defaults in ssh_bind_listen
tests: add test for checking default hostkey paths
remove: null check for hostkey paths, can't happen since we set defaults now
examples: ssh_server remove "no default keys", default hostkeys set in ssh_bind_listen

Signed-off-by: Gauravsingh Sisodia <xaerru@gmail.com>
Reviewed-by: Sahana Prasad <sahana@redhat.com>

examples/ssh_server.c

@@ -45,32 +45,10 @@ The goal is to show the API in action.
 #define BUF_SIZE 1048576
 #endif
 
-#ifndef KEYS_FOLDER
-#ifdef _WIN32
-#define KEYS_FOLDER
-#else
-#define KEYS_FOLDER "/etc/ssh/"
-#endif
-#endif
-
 #define SESSION_END (SSH_CLOSED | SSH_CLOSED_ERROR)
 #define SFTP_SERVER_PATH "/usr/lib/sftp-server"
 #define AUTH_KEYS_MAX_LINE_SIZE 2048
 
-static void set_default_keys(ssh_bind sshbind,
-                             int rsa_already_set,
-                             int ecdsa_already_set) {
-    if (!rsa_already_set) {
-        ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY,
-                             KEYS_FOLDER "ssh_host_rsa_key");
-    }
-    if (!ecdsa_already_set) {
-        ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY,
-                             KEYS_FOLDER "ssh_host_ecdsa_key");
-    }
-    ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY,
-                         KEYS_FOLDER "ssh_host_ed25519_key");
-}
 #define DEF_STR_SIZE 1024
 char authorizedkeys[DEF_STR_SIZE] = {0};
 char username[128] = "myuser";
@@ -145,14 +123,6 @@ static struct argp_option options[] = {
         .doc   = "Set expected password.",
         .group = 0
     },
-    {
-        .name  = "no-default-keys",
-        .key   = 'n',
-        .arg   = NULL,
-        .flags = 0,
-        .doc   = "Do not set default key locations.",
-        .group = 0
-    },
     {
         .name  = "verbose",
         .key   = 'v',
@@ -169,30 +139,19 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state) {
     /* Get the input argument from argp_parse, which we
      * know is a pointer to our arguments structure. */
     ssh_bind sshbind = state->input;
-    static int no_default_keys = 0;
-    static int rsa_already_set = 0, ecdsa_already_set = 0;
 
     switch (key) {
-        case 'n':
-            no_default_keys = 1;
-            break;
         case 'p':
             ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_BINDPORT_STR, arg);
             break;
         case 'k':
             ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, arg);
-            /* We can't track the types of keys being added with this
-               option, so let's ensure we keep the keys we're adding
-               by just not setting the default keys */
-            no_default_keys = 1;
             break;
         case 'r':
             ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, arg);
-            rsa_already_set = 1;
             break;
         case 'e':
             ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, arg);
-            ecdsa_already_set = 1;
             break;
         case 'a':
             strncpy(authorizedkeys, arg, DEF_STR_SIZE-1);
@@ -219,13 +178,6 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state) {
                 /* Not enough arguments. */
                 argp_usage (state);
             }
-
-            if (!no_default_keys) {
-                set_default_keys(sshbind,
-                                 rsa_already_set,
-                                 ecdsa_already_set);
-            }
-
             break;
         default:
             return ARGP_ERR_UNKNOWN;
@@ -242,10 +194,8 @@ static int parse_opt(int argc, char **argv, ssh_bind sshbind) {
     int ecdsa_already_set = 0;
     int key;
 
-    while((key = getopt(argc, argv, "a:e:k:np:P:r:u:v")) != -1) {
+    while((key = getopt(argc, argv, "a:e:k:p:P:r:u:v")) != -1) {
-        if (key == 'n') {
+        if (key == 'p') {
-            no_default_keys = 1;
-        } else if (key == 'p') {
             ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_BINDPORT_STR, optarg);
         } else if (key == 'k') {
             ssh_bind_options_set(sshbind, SSH_BIND_OPTIONS_HOSTKEY, optarg);
@@ -281,7 +231,6 @@ static int parse_opt(int argc, char **argv, ssh_bind sshbind) {
                "  -e, --ecdsakey=FILE        Set the ecdsa key (deprecated alias for 'k').\n"
                "  -k, --hostkey=FILE         Set a host key.  Can be used multiple times.\n"
                "                             Implies no default keys.\n"
-               "  -n, --no-default-keys      Do not set default key locations.\n"
                "  -p, --port=PORT            Set the port to bind.\n"
                "  -P, --pass=PASSWORD        Set expected password.\n"
                "  -r, --rsakey=FILE          Set the rsa key (deprecated alias for 'k').\n"

src/bind.c

@@ -149,14 +149,6 @@ ssh_bind ssh_bind_new(void) {
 static int ssh_bind_import_keys(ssh_bind sshbind) {
   int rc;
 
-  if (sshbind->ecdsakey == NULL &&
-      sshbind->rsakey == NULL &&
-      sshbind->ed25519key == NULL) {
-      ssh_set_error(sshbind, SSH_FATAL,
-                    "ECDSA, ED25519, or RSA host key file must be set");
-      return SSH_ERROR;
-  }
-
 #ifdef HAVE_ECC
   if (sshbind->ecdsa == NULL && sshbind->ecdsakey != NULL) {
       rc = ssh_pki_import_privkey_file(sshbind->ecdsakey,
@@ -225,12 +217,28 @@ static int ssh_bind_import_keys(ssh_bind sshbind) {
   return SSH_OK;
 }
 
-int ssh_bind_listen(ssh_bind sshbind)
+int ssh_bind_listen(ssh_bind sshbind) {
-{
+    const char *host;
-    const char *host = NULL;
     socket_t fd;
     int rc;
 
+    /* Apply global bind configurations, if it hasn't been applied before */
+    rc = ssh_bind_options_parse_config(sshbind, NULL);
+    if (rc != 0) {
+        ssh_set_error(sshbind, SSH_FATAL,"Could not parse global config");
+        return SSH_ERROR;
+    }
+
+    /* Set default hostkey paths if no hostkey was found before */
+    if (sshbind->ecdsakey == NULL &&
+        sshbind->rsakey == NULL &&
+        sshbind->ed25519key == NULL) {
+
+        sshbind->ecdsakey = strdup("/etc/ssh/ssh_host_ecdsa_key");
+        sshbind->rsakey = strdup("/etc/ssh/ssh_host_rsa_key");
+        sshbind->ed25519key = strdup("/etc/ssh/ssh_host_ed25519_key");
+    }
+
     /* Apply global bind configurations, if it hasn't been applied before */
     rc = ssh_bind_options_parse_config(sshbind, NULL);
     if (rc != 0) {
@@ -424,13 +432,6 @@ int ssh_bind_accept_fd(ssh_bind sshbind, ssh_session session, socket_t fd)
         return SSH_ERROR;
     }
 
-    /* Apply global bind configurations, if it hasn't been applied before */
-    rc = ssh_bind_options_parse_config(sshbind, NULL);
-    if (rc != 0) {
-        ssh_set_error(sshbind, SSH_FATAL,"Could not parse global config");
-        return SSH_ERROR;
-    }
-
     session->server = 1;
 
     /* Copy options from bind to session */

tests/unittests/CMakeLists.txt

@@ -84,13 +84,10 @@ if (UNIX AND NOT WIN32)
         torture_threads_pki_rsa
     )
     if (WITH_SERVER)
-        # Not working correctly
+        set(LIBSSH_THREAD_UNIT_TESTS
-        # add_cmocka_test(torture_server_x11 torture_server_x11.c ${TEST_TARGET_LIBRARIES})
+            ${LIBSSH_THREAD_UNIT_TESTS}
-        # the signals are not testable under cmocka
+            torture_unit_server
-        # set(LIBSSH_THREAD_UNIT_TESTS
+        )
-        #     ${LIBSSH_THREAD_UNIT_TESTS}
-        #     torture_unit_server
-        # )
     endif (WITH_SERVER)
 endif (UNIX AND NOT WIN32)
 

tests/unittests/torture_unit_server.c

@@ -10,11 +10,13 @@
 #include <signal.h>
 
 #include <libssh/libssh.h>
+#include <libssh/bind.h>
 #include "torture.h"
 #include "torture_key.h"
 
 #define TEST_SERVER_PORT 2222
 
+#if 0
 struct test_state {
     const char *hostkey;
     char *hostkey_path;
@@ -107,7 +109,7 @@ static void test_ssh_accept_interrupt(void **state)
     struct test_state *ts = (struct test_state *)*state;
     int rc;
     pthread_t client_pthread, interrupt_pthread;
-    ssh_bind sshbind;
+    ssh_bind sshbind = NULL;
     ssh_session server;
 
     /* Create server */
@@ -145,14 +147,47 @@ static void test_ssh_accept_interrupt(void **state)
     rc = pthread_join(client_pthread, NULL);
     assert_int_equal(rc, 0);
 }
+#endif
+
+
+static void test_default_hostkey_paths(void **state)
+{
+    int rc;
+    ssh_bind sshbind = NULL;
+
+    /* state not used */
+    (void)state;
+
+    /* Create server */
+    rc = ssh_init();
+    assert_int_equal(rc, 0);
+
+    sshbind = ssh_bind_new();
+    assert_non_null(sshbind);
+
+    /* This will fail because we don't have permission to import keys unless we run as root
+     * TODO: Implement some filesystem wrapper, that would allow this check to pass by
+     * reading the keys from some accessible test location */
+    ssh_bind_listen(sshbind);
+
+    assert_string_equal(sshbind->rsakey, "/etc/ssh/ssh_host_rsa_key");
+    assert_string_equal(sshbind->ecdsakey, "/etc/ssh/ssh_host_ecdsa_key");
+    assert_string_equal(sshbind->ed25519key, "/etc/ssh/ssh_host_ed25519_key");
+
+    /* Cleanup */
+    ssh_bind_free(sshbind);
+    ssh_finalize();
+}
 
 int torture_run_tests(void)
 {
     int rc;
     const struct CMUnitTest tests[] = {
+        cmocka_unit_test(test_default_hostkey_paths),
+        /* Not working correctly the signals are not testable under cmocka
         cmocka_unit_test_setup_teardown(test_ssh_accept_interrupt,
                                         setup,
-                                        teardown)
+                                        teardown) */
     };
 
     rc = cmocka_run_group_tests(tests, NULL, NULL);