refactoring, expand test cases, adjust to authorized key file, minor dead code adjustments

Author: JacobBarthelmeh

.github/workflows/windows-cert-store-test.yml

@@ -180,10 +180,19 @@ jobs:
         grep -q -- '-lncrypt' Makefile
 
     - name: Rejects a non-Windows host and a missing --enable-certs
+      # Each assertion exits explicitly: bash errexit exempts a command
+      # inverted with '!', and the step status comes from the last command.
       run: |
-        ! ./configure --enable-certs --enable-windows-cert-store $WOLFSSL_CACHE
-        ! ./configure --host=x86_64-w64-mingw32 --enable-windows-cert-store \
-            $WOLFSSL_CACHE
+        if ./configure --enable-certs --enable-windows-cert-store \
+            $WOLFSSL_CACHE; then
+          echo 'ERROR: configure should have failed on a non-Windows host'
+          exit 1
+        fi
+        if ./configure --host=x86_64-w64-mingw32 \
+            --enable-windows-cert-store $WOLFSSL_CACHE; then
+          echo 'ERROR: configure should have failed without --enable-certs'
+          exit 1
+        fi
 
   test:
     needs: build
@@ -205,23 +214,32 @@ jobs:
             client_key_source: x509
             key_algorithm: rsa
             test_name: "Server-Store-Client-X509"
+          # key_algorithm is the server host key; client_key_algorithm is the
+          # testuser client certificate key. Both are stated explicitly on the
+          # store-client entries so neither depends on which key renewcerts.sh
+          # happens to copy.
           - server_key_source: file
             client_key_source: store
             key_algorithm: rsa
+            client_key_algorithm: ecdsa
             test_name: "Server-File-Client-Store"
           - server_key_source: store
             client_key_source: store
             key_algorithm: rsa
+            client_key_algorithm: ecdsa
             test_name: "Server-Store-Client-Store"
           - server_key_source: store
             client_key_source: x509
             key_algorithm: ecdsa
             test_name: "Server-Store-Client-X509-ECDSA"
+          # RSA client certificate, covering the x509v3-ssh-rsa user-auth and
+          # client-side RSA cert store signing paths that the ECDSA entries
+          # above cannot reach.
           - server_key_source: file
             client_key_source: store
             key_algorithm: rsa
-            client_key_algorithm: ecdsa
-            test_name: "Server-File-Client-Store-ECDSA"
+            client_key_algorithm: rsa
+            test_name: "Server-File-Client-Store-RSA"
 
     steps:
     - uses: actions/checkout@v4
@@ -254,12 +272,20 @@ jobs:
         cd keys
         bash renewcerts.sh testuser
 
-        # renewcerts.sh always gives testuser fred's RSA key. Re-issue it with
-        # an EC key when the client store entry is meant to be ECDSA.
-        if [ "${{ matrix.client_key_algorithm }}" = "ecdsa" ]; then
+        # renewcerts.sh copies fred's key, which is EC prime256v1, so testuser
+        # comes out ECDSA. Re-issue it explicitly for whichever algorithm the
+        # matrix entry asks for, rather than inheriting whatever fred's key
+        # happens to be.
+        ALG="${{ matrix.client_key_algorithm }}"
+        if [ -n "$ALG" ]; then
           touch index.txt
           sed 's/fred/testuser/g' renewcerts.cnf > renewcerts-testuser.cnf
-          openssl ecparam -name prime256v1 -genkey -noout -out testuser-key.pem
+          if [ "$ALG" = "rsa" ]; then
+            openssl genrsa -out testuser-key.pem 2048
+          else
+            openssl ecparam -name prime256v1 -genkey -noout \
+                -out testuser-key.pem
+          fi
           openssl req -subj "/C=US/ST=WA/L=Seattle/O=wolfSSL Inc/OU=Development/CN=testuser/emailAddress=testuser@example.com" \
               -key testuser-key.pem -out testuser-cert.csr \
               -config renewcerts-testuser.cnf -new -nodes
@@ -268,7 +294,11 @@ jobs:
               -CA ca-cert-ecc.pem -CAkey ca-key-ecc.pem -out testuser-cert.pem \
               -set_serial 7
           openssl x509 -in testuser-cert.pem -outform DER -out testuser-cert.der
-          openssl ec -in testuser-key.pem -outform DER -out testuser-key.der
+          if [ "$ALG" = "rsa" ]; then
+            openssl rsa -in testuser-key.pem -outform DER -out testuser-key.der
+          else
+            openssl ec -in testuser-key.pem -outform DER -out testuser-key.der
+          fi
           rm -f renewcerts-testuser.cnf testuser-cert.csr index.*
         fi
         cd ..
@@ -278,6 +308,29 @@ jobs:
           ls -la keys/
           exit 1
         fi
+
+        # Assert the key really is the algorithm this entry asked for, so a
+        # change to renewcerts.sh cannot silently turn an entry into a
+        # duplicate of another one. Unset means whatever renewcerts.sh gives,
+        # which is fred's EC key.
+        EXPECT="${{ matrix.client_key_algorithm }}"
+        [ -n "$EXPECT" ] || EXPECT=ecdsa
+        if openssl rsa -inform DER -in keys/testuser-key.der -noout 2>/dev/null
+        then
+          ACTUAL=rsa
+        elif openssl ec -inform DER -in keys/testuser-key.der -noout 2>/dev/null
+        then
+          ACTUAL=ecdsa
+        else
+          echo "ERROR: testuser-key.der is neither RSA nor EC"
+          exit 1
+        fi
+        if [ "$ACTUAL" != "$EXPECT" ]; then
+          echo "ERROR: testuser client key is $ACTUAL, expected $EXPECT"
+          exit 1
+        fi
+        echo "testuser client key algorithm: $ACTUAL"
+
         echo "CLIENT_CERT_FILE=keys/testuser-cert.der" >> $GITHUB_ENV
         echo "CLIENT_KEY_FILE=keys/testuser-key.der" >> $GITHUB_ENV
 
@@ -493,8 +546,8 @@ jobs:
         }
 
         if ("${{ matrix.server_key_source }}" -eq "store") {
-            # The certificate is part of the store entry; do NOT specify
-            # HostCertificate separately.
+            # The certificate is part of the store entry. HostKey and
+            # HostCertificate alongside HostKeyStore are rejected at startup.
             $configContent += @"
 
         HostKeyStore My

apps/wolfsshd/auth.c

@@ -643,6 +643,46 @@ static int IsAbsoluteAuthKeysPath(const char* path)
     return ret;
 }
 
+#if defined(WOLFSSH_CERTS) && (defined(WOLFSSL_FPKI) || defined(_WIN32))
+/* True when the AuthorizedKeysFile pattern is guaranteed to resolve to a
+ * different file for every account, which is what makes an entry in it an
+ * implicit user-to-credential binding. A relative pattern resolves under the
+ * account's home directory, and an absolute one qualifies only when it carries
+ * a %u or %h token. An absolute pattern with neither (e.g.
+ * "/etc/ssh/authorized_keys_all") is one shared file for every account and
+ * binds a credential to nothing. */
+static int IsPerUserAuthKeysPattern(const char* pattern)
+{
+    word32 i;
+    word32 patSz;
+
+    if (pattern == NULL || *pattern == '\0') {
+        /* the built-in ~/.ssh/authorized_keys default */
+        return 1;
+    }
+
+    if (!IsAbsoluteAuthKeysPath(pattern)) {
+        return 1;
+    }
+
+    patSz = (word32)WSTRLEN(pattern);
+    for (i = 0; (i + 1) < patSz; i++) {
+        if (pattern[i] != '%') {
+            continue;
+        }
+        if (pattern[i + 1] == 'u' || pattern[i + 1] == 'h') {
+            return 1;
+        }
+        /* "%%" is a literal percent, step over both characters */
+        if (pattern[i + 1] == '%') {
+            i++;
+        }
+    }
+
+    return 0;
+}
+#endif /* WOLFSSH_CERTS && (WOLFSSL_FPKI || _WIN32) */
+
 /* Resolve the authorized keys file path for a user. The pattern is passed in
  * explicitly so concurrent authentications cannot race on it, and its tokens
  * are expanded so each user resolves to a distinct path. */
@@ -2090,11 +2130,16 @@ static int RequestAuthentication(WS_UserAuthData* authData,
     if (ret == WOLFSSH_USERAUTH_SUCCESS &&
         authData->type == WOLFSSH_USERAUTH_PUBLICKEY) {
         /* Bind the certificate to the requested user name via UPN with FPKI or
-         * CN without FPKI. Only done when relying on the CA; an
-         * AuthorizedKeysFile entry is itself an explicit user to cert binding
-         * and is checked below. */
+         * CN without FPKI. Skipped only when a per-user AuthorizedKeysFile is
+         * configured, because such an entry is itself an explicit user to cert
+         * binding and is checked below. A shared AuthorizedKeysFile (an
+         * absolute pattern with no %u or %h) resolves to one file for every
+         * account and binds the certificate to nothing, so the identity check
+         * still has to run. */
         if (authData->sf.publicKey.isCert &&
-                !wolfSSHD_ConfigGetAuthKeysFileSet(usrConf)) {
+                !(wolfSSHD_ConfigGetAuthKeysFileSet(usrConf) &&
+                  IsPerUserAuthKeysPattern(
+                      wolfSSHD_ConfigGetAuthKeysFile(usrConf)))) {
             DecodedCert* dCert;
         #ifdef WOLFSSH_SMALL_STACK
             dCert = (DecodedCert*)WMALLOC(sizeof(DecodedCert), NULL,

apps/wolfsshd/configuration.c

@@ -1862,8 +1862,10 @@ int wolfSSHD_ConfigSetSystemCA(WOLFSSHD_CONFIG* conf, const char* value)
             conf->useSystemCA = 0;
         }
         else {
-            wolfSSH_Log(WS_LOG_INFO, "[SSHD] System CAs unexpected flag");
-            ret = WS_FATAL_ERROR;
+            wolfSSH_Log(WS_LOG_ERROR,
+                "[SSHD] wolfSSH_TrustedSystemCAKeys: expected 'yes' or 'no', "
+                "got '%s'", value);
+            ret = WS_BAD_ARGUMENT;
         }
     }
 
@@ -1903,8 +1905,10 @@ int wolfSSHD_ConfigSetUserCAStore(WOLFSSHD_CONFIG* conf, const char* value)
             conf->useUserCAStore = 0;
         }
         else {
-            wolfSSH_Log(WS_LOG_INFO, "[SSHD] User CA store unexpected flag");
-            ret = WS_FATAL_ERROR;
+            wolfSSH_Log(WS_LOG_ERROR,
+                "[SSHD] wolfSSH_TrustedUserCAStore: expected 'yes' or 'no', "
+                "got '%s'", value);
+            ret = WS_BAD_ARGUMENT;
         }
     }