Code review feedback.

* Move some things in to a more specific module for security keys.
* Comment on the security key defaults.
* Rework code so that flag checks are independent for security keys.

Author: vcsjones

lib/ssh_data/public_key.rb

@@ -20,14 +20,6 @@ module PublicKey
       ALGO_ED25519, ALGO_SKECDSA256, ALGO_SKED25519
     ]
 
-    DEFAULT_SK_VERIFY_OPTS = {
-      user_presence_required: true,
-      user_verification_required: false
-    }
-
-    SK_FLAG_USER_PRESENCE     = 0b001
-    SK_FLAG_USER_VERIFICATION = 0b100
-
     # Parse an OpenSSH public key in authorized_keys format (see sshd(8) manual
     # page).
     #

lib/ssh_data/public_key/security_key.rb

@@ -1,6 +1,16 @@
 module SSHData
   module PublicKey
     module SecurityKey
+
+      # Defaults to match OpenSSH, user presence is required by verification is not.
+      DEFAULT_SK_VERIFY_OPTS = {
+        user_presence_required: true,
+        user_verification_required: false
+      }
+
+      SK_FLAG_USER_PRESENCE     = 0b001
+      SK_FLAG_USER_VERIFICATION = 0b100
+
       def build_signing_blob(application, signed_data, signature)
         read = 0
         sig_algo, raw_sig, signature_read = Encoding.decode_signature(signature)

lib/ssh_data/public_key/skecdsa.rb

@@ -48,13 +48,12 @@ def verify(signed_data, signature, **opts)
 
         result = openssl.verify(digest.new, openssl_sig, blob)
 
-        if opts[:user_presence_required] && (sk_flags & SK_FLAG_USER_PRESENCE != SK_FLAG_USER_PRESENCE)
-          false
-        elsif opts[:user_verification_required] && (sk_flags & SK_FLAG_USER_VERIFICATION != SK_FLAG_USER_VERIFICATION)
-          false
-        else
-          result
-        end
+        # We don't know that the flags are correct until after we've validated the signature
+        # which embeds the flags, so always verify the signature first.
+        return false if opts[:user_presence_required] && (sk_flags & SK_FLAG_USER_PRESENCE != SK_FLAG_USER_PRESENCE)
+        return false if opts[:user_verification_required] && (sk_flags & SK_FLAG_USER_VERIFICATION != SK_FLAG_USER_VERIFICATION)
+
+        result
       end
 
       def ==(other)

lib/ssh_data/public_key/sked25519.rb

@@ -42,13 +42,12 @@ def verify(signed_data, signature, **opts)
             false
           end
 
-        if opts[:user_presence_required] && (sk_flags & SK_FLAG_USER_PRESENCE != SK_FLAG_USER_PRESENCE)
-          false
-        elsif opts[:user_verification_required] && (sk_flags & SK_FLAG_USER_VERIFICATION != SK_FLAG_USER_VERIFICATION)
-          false
-        else
-          result
-        end
+        # We don't know that the flags are correct until after we've validated the signature
+        # which embeds the flags, so always verify the signature first.
+        return false if opts[:user_presence_required] && (sk_flags & SK_FLAG_USER_PRESENCE != SK_FLAG_USER_PRESENCE)
+        return false if opts[:user_verification_required] && (sk_flags & SK_FLAG_USER_VERIFICATION != SK_FLAG_USER_VERIFICATION)
+
+        result
       end
 
       def ==(other)