From ea068414dccf8d52400164dd1ea8c10d305756ad Mon Sep 17 00:00:00 2001 From: Aveline <352441+ym@users.noreply.github.com> Date: Thu, 11 Sep 2025 23:32:40 +0200 Subject: [PATCH] feat: validate ssh public key before saving (#794) * feat: validate ssh public key before saving * fix: TestValidSSHKeyTypes --- internal/utils/ssh.go | 71 +++++++++++++ internal/utils/ssh_test.go | 208 +++++++++++++++++++++++++++++++++++++ jsonrpc.go | 29 ++++-- 3 files changed, 297 insertions(+), 11 deletions(-) create mode 100644 internal/utils/ssh.go create mode 100644 internal/utils/ssh_test.go diff --git a/internal/utils/ssh.go b/internal/utils/ssh.go new file mode 100644 index 0000000..e4602ff --- /dev/null +++ b/internal/utils/ssh.go @@ -0,0 +1,71 @@ +package utils + +import ( + "fmt" + "slices" + "strings" + + "golang.org/x/crypto/ssh" +) + +// ValidSSHKeyTypes is a list of valid SSH key types +// +// Please make sure that all the types in this list are supported by dropbear +// https://github.com/mkj/dropbear/blob/003c5fcaabc114430d5d14142e95ffdbbd2d19b6/src/signkey.c#L37 +// +// ssh-dss is not allowed here as it's insecure +var ValidSSHKeyTypes = []string{ + ssh.KeyAlgoRSA, + ssh.KeyAlgoED25519, + ssh.KeyAlgoECDSA256, + ssh.KeyAlgoECDSA384, + ssh.KeyAlgoECDSA521, +} + +// ValidateSSHKey validates authorized_keys file content +func ValidateSSHKey(sshKey string) error { + // validate SSH key + var ( + hasValidPublicKey = false + lastError = fmt.Errorf("no valid SSH key found") + ) + for _, key := range strings.Split(sshKey, "\n") { + key = strings.TrimSpace(key) + + // skip empty lines and comments + if key == "" || strings.HasPrefix(key, "#") { + continue + } + + parsedPublicKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key)) + if err != nil { + lastError = err + continue + } + + if parsedPublicKey == nil { + continue + } + + parsedType := parsedPublicKey.Type() + textType := strings.Fields(key)[0] + + if parsedType != textType { + lastError = fmt.Errorf("parsed SSH key type %s does not match type in text %s", parsedType, textType) + continue + } + + if !slices.Contains(ValidSSHKeyTypes, parsedType) { + lastError = fmt.Errorf("invalid SSH key type: %s", parsedType) + continue + } + + hasValidPublicKey = true + } + + if !hasValidPublicKey { + return lastError + } + + return nil +} diff --git a/internal/utils/ssh_test.go b/internal/utils/ssh_test.go new file mode 100644 index 0000000..f89cb90 --- /dev/null +++ b/internal/utils/ssh_test.go @@ -0,0 +1,208 @@ +package utils + +import ( + "strings" + "testing" +) + +func TestValidateSSHKey(t *testing.T) { + tests := []struct { + name string + sshKey string + expectError bool + errorMsg string + }{ + { + name: "valid RSA key", + sshKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDiYUb9Fy2vlPfO+HwubnshimpVrWPoePyvyN+jPC5gWqZSycjMy6Is2vFVn7oQc72bkY0wZalspT5wUOwKtltSoLpL7vcqGL9zHVw4yjYXtPGIRd3zLpU9wdngevnepPQWTX3LvZTZfmOsrGoMDKIG+Lbmiq/STMuWYecIqMp7tUKRGS8vfAmpu6MsrN9/4UTcdWWXYWJQQn+2nCyMz28jYlWRsKtqFK6owrdZWt8WQnPN+9Upcf2ByQje+0NLnpNrnh+yd2ocuVW9wQYKAZXy7IaTfEJwd5m34sLwkqlZTaBBcmWJU+3RfpYXE763cf3rUoPIGQ8eUEBJ8IdM4vhp test@example.com", + expectError: false, + }, + { + name: "valid ED25519 key", + sshKey: "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIBSbM8wuD5ab0nHsXaYOqaD3GLLUwmDzSk79Xi/N+H2j test@example.com", + expectError: false, + }, + { + name: "valid ECDSA key", + sshKey: "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBAlTkxIo4mXBR+gEX0Q74BpYX4bFFHoX+8Uz7tsob8HvsnMvsEE+BW9h9XrbWX4/4ppL/o6sHbvsqNr9HcyKfdc= test@example.com", + expectError: false, + }, + { + name: "multiple valid keys", + sshKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDiYUb9Fy2vlPfO+HwubnshimpVrWPoePyvyN+jPC5gWqZSycjMy6Is2vFVn7oQc72bkY0wZalspT5wUOwKtltSoLpL7vcqGL9zHVw4yjYXtPGIRd3zLpU9wdngevnepPQWTX3LvZTZfmOsrGoMDKIG+Lbmiq/STMuWYecIqMp7tUKRGS8vfAmpu6MsrN9/4UTcdWWXYWJQQn+2nCyMz28jYlWRsKtqFK6owrdZWt8WQnPN+9Upcf2ByQje+0NLnpNrnh+yd2ocuVW9wQYKAZXy7IaTfEJwd5m34sLwkqlZTaBBcmWJU+3RfpYXE763cf3rUoPIGQ8eUEBJ8IdM4vhp test@example.com\nssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIBSbM8wuD5ab0nHsXaYOqaD3GLLUwmDzSk79Xi/N+H2j test@example.com", + expectError: false, + }, + { + name: "valid key with comment", + sshKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDiYUb9Fy2vlPfO+HwubnshimpVrWPoePyvyN+jPC5gWqZSycjMy6Is2vFVn7oQc72bkY0wZalspT5wUOwKtltSoLpL7vcqGL9zHVw4yjYXtPGIRd3zLpU9wdngevnepPQWTX3LvZTZfmOsrGoMDKIG+Lbmiq/STMuWYecIqMp7tUKRGS8vfAmpu6MsrN9/4UTcdWWXYWJQQn+2nCyMz28jYlWRsKtqFK6owrdZWt8WQnPN+9Upcf2ByQje+0NLnpNrnh+yd2ocuVW9wQYKAZXy7IaTfEJwd5m34sLwkqlZTaBBcmWJU+3RfpYXE763cf3rUoPIGQ8eUEBJ8IdM4vhp user@example.com", + expectError: false, + }, + { + name: "valid key with options and comment (we don't support options yet)", + sshKey: "command=\"echo hello\" ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDiYUb9Fy2vlPfO+HwubnshimpVrWPoePyvyN+jPC5gWqZSycjMy6Is2vFVn7oQc72bkY0wZalspT5wUOwKtltSoLpL7vcqGL9zHVw4yjYXtPGIRd3zLpU9wdngevnepPQWTX3LvZTZfmOsrGoMDKIG+Lbmiq/STMuWYecIqMp7tUKRGS8vfAmpu6MsrN9/4UTcdWWXYWJQQn+2nCyMz28jYlWRsKtqFK6owrdZWt8WQnPN+9Upcf2ByQje+0NLnpNrnh+yd2ocuVW9wQYKAZXy7IaTfEJwd5m34sLwkqlZTaBBcmWJU+3RfpYXE763cf3rUoPIGQ8eUEBJ8IdM4vhp user@example.com", + expectError: true, + }, + { + name: "empty string", + sshKey: "", + expectError: true, + errorMsg: "no valid SSH key found", + }, + { + name: "whitespace only", + sshKey: " \n\t \n ", + expectError: true, + errorMsg: "no valid SSH key found", + }, + { + name: "comment only", + sshKey: "# This is a comment\n# Another comment", + expectError: true, + errorMsg: "no valid SSH key found", + }, + { + name: "invalid key format", + sshKey: "not-a-valid-ssh-key", + expectError: true, + }, + { + name: "invalid key type", + sshKey: "ssh-dss AAAAB3NzaC1kc3MAAACBAOeB...", + expectError: true, + errorMsg: "invalid SSH key type: ssh-dss", + }, + { + name: "unsupported key type", + sshKey: "ssh-rsa-cert-v01@openssh.com AAAAB3NzaC1yc2EAAAADAQABAAABgQC7vbqajDhA...", + expectError: true, + errorMsg: "invalid SSH key type: ssh-rsa-cert-v01@openssh.com", + }, + { + name: "malformed key data", + sshKey: "ssh-rsa invalid-base64-data", + expectError: true, + }, + { + name: "type mismatch", + sshKey: "ssh-rsa AAAAC3NzaC1lZDI1NTE5AAAAIGomKoH...", + expectError: true, + errorMsg: "parsed SSH key type ssh-ed25519 does not match type in text ssh-rsa", + }, + { + name: "mixed valid and invalid keys", + sshKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDiYUb9Fy2vlPfO+HwubnshimpVrWPoePyvyN+jPC5gWqZSycjMy6Is2vFVn7oQc72bkY0wZalspT5wUOwKtltSoLpL7vcqGL9zHVw4yjYXtPGIRd3zLpU9wdngevnepPQWTX3LvZTZfmOsrGoMDKIG+Lbmiq/STMuWYecIqMp7tUKRGS8vfAmpu6MsrN9/4UTcdWWXYWJQQn+2nCyMz28jYlWRsKtqFK6owrdZWt8WQnPN+9Upcf2ByQje+0NLnpNrnh+yd2ocuVW9wQYKAZXy7IaTfEJwd5m34sLwkqlZTaBBcmWJU+3RfpYXE763cf3rUoPIGQ8eUEBJ8IdM4vhp test@example.com\ninvalid-key\nssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIBSbM8wuD5ab0nHsXaYOqaD3GLLUwmDzSk79Xi/N+H2j test@example.com", + expectError: false, + }, + { + name: "valid key with empty lines and comments", + sshKey: "# Comment line\n\nssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDiYUb9Fy2vlPfO+HwubnshimpVrWPoePyvyN+jPC5gWqZSycjMy6Is2vFVn7oQc72bkY0wZalspT5wUOwKtltSoLpL7vcqGL9zHVw4yjYXtPGIRd3zLpU9wdngevnepPQWTX3LvZTZfmOsrGoMDKIG+Lbmiq/STMuWYecIqMp7tUKRGS8vfAmpu6MsrN9/4UTcdWWXYWJQQn+2nCyMz28jYlWRsKtqFK6owrdZWt8WQnPN+9Upcf2ByQje+0NLnpNrnh+yd2ocuVW9wQYKAZXy7IaTfEJwd5m34sLwkqlZTaBBcmWJU+3RfpYXE763cf3rUoPIGQ8eUEBJ8IdM4vhp test@example.com\n# Another comment\n\t\n", + expectError: false, + }, + { + name: "all invalid keys", + sshKey: "invalid-key-1\ninvalid-key-2\nssh-dss AAAAB3NzaC1kc3MAAACBAOeB...", + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateSSHKey(tt.sshKey) + + if tt.expectError { + if err == nil { + t.Errorf("ValidateSSHKey() expected error but got none") + } else if tt.errorMsg != "" && !strings.ContainsAny(err.Error(), tt.errorMsg) { + t.Errorf("ValidateSSHKey() error = %v, expected to contain %v", err, tt.errorMsg) + } + } else { + if err != nil { + t.Errorf("ValidateSSHKey() unexpected error = %v", err) + } + } + }) + } +} + +func TestValidSSHKeyTypes(t *testing.T) { + expectedTypes := []string{ + "ssh-rsa", + "ssh-ed25519", + "ecdsa-sha2-nistp256", + "ecdsa-sha2-nistp384", + "ecdsa-sha2-nistp521", + } + + if len(ValidSSHKeyTypes) != len(expectedTypes) { + t.Errorf("ValidSSHKeyTypes length = %d, expected %d", len(ValidSSHKeyTypes), len(expectedTypes)) + } + + for _, expectedType := range expectedTypes { + found := false + for _, actualType := range ValidSSHKeyTypes { + if actualType == expectedType { + found = true + break + } + } + if !found { + t.Errorf("ValidSSHKeyTypes missing expected type: %s", expectedType) + } + } +} + +// TestValidateSSHKeyEdgeCases tests edge cases and boundary conditions +func TestValidateSSHKeyEdgeCases(t *testing.T) { + tests := []struct { + name string + sshKey string + expectError bool + }{ + { + name: "key with only type", + sshKey: "ssh-rsa", + expectError: true, + }, + { + name: "key with type and empty data", + sshKey: "ssh-rsa ", + expectError: true, + }, + { + name: "key with type and whitespace data", + sshKey: "ssh-rsa \t ", + expectError: true, + }, + { + name: "key with multiple spaces between type and data", + sshKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDiYUb9Fy2vlPfO+HwubnshimpVrWPoePyvyN+jPC5gWqZSycjMy6Is2vFVn7oQc72bkY0wZalspT5wUOwKtltSoLpL7vcqGL9zHVw4yjYXtPGIRd3zLpU9wdngevnepPQWTX3LvZTZfmOsrGoMDKIG+Lbmiq/STMuWYecIqMp7tUKRGS8vfAmpu6MsrN9/4UTcdWWXYWJQQn+2nCyMz28jYlWRsKtqFK6owrdZWt8WQnPN+9Upcf2ByQje+0NLnpNrnh+yd2ocuVW9wQYKAZXy7IaTfEJwd5m34sLwkqlZTaBBcmWJU+3RfpYXE763cf3rUoPIGQ8eUEBJ8IdM4vhp test@example.com", + expectError: false, + }, + { + name: "key with tabs", + sshKey: "\tssh-rsa\tAAAAB3NzaC1yc2EAAAADAQABAAABAQDiYUb9Fy2vlPfO+HwubnshimpVrWPoePyvyN+jPC5gWqZSycjMy6Is2vFVn7oQc72bkY0wZalspT5wUOwKtltSoLpL7vcqGL9zHVw4yjYXtPGIRd3zLpU9wdngevnepPQWTX3LvZTZfmOsrGoMDKIG+Lbmiq/STMuWYecIqMp7tUKRGS8vfAmpu6MsrN9/4UTcdWWXYWJQQn+2nCyMz28jYlWRsKtqFK6owrdZWt8WQnPN+9Upcf2ByQje+0NLnpNrnh+yd2ocuVW9wQYKAZXy7IaTfEJwd5m34sLwkqlZTaBBcmWJU+3RfpYXE763cf3rUoPIGQ8eUEBJ8IdM4vhp test@example.com", + expectError: false, + }, + { + name: "very long line", + sshKey: "ssh-rsa " + string(make([]byte, 10000)), + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateSSHKey(tt.sshKey) + + if tt.expectError { + if err == nil { + t.Errorf("ValidateSSHKey() expected error but got none") + } + } else { + if err != nil { + t.Errorf("ValidateSSHKey() unexpected error = %v", err) + } + } + }) + } +} diff --git a/jsonrpc.go b/jsonrpc.go index ff3a4b1..61f28df 100644 --- a/jsonrpc.go +++ b/jsonrpc.go @@ -17,6 +17,7 @@ import ( "go.bug.st/serial" "github.com/jetkvm/kvm/internal/usbgadget" + "github.com/jetkvm/kvm/internal/utils" ) type JSONRPCRequest struct { @@ -429,21 +430,27 @@ func rpcGetSSHKeyState() (string, error) { } func rpcSetSSHKeyState(sshKey string) error { - if sshKey != "" { - // Create directory if it doesn't exist - if err := os.MkdirAll(sshKeyDir, 0700); err != nil { - return fmt.Errorf("failed to create SSH key directory: %w", err) - } - - // Write SSH key to file - if err := os.WriteFile(sshKeyFile, []byte(sshKey), 0600); err != nil { - return fmt.Errorf("failed to write SSH key: %w", err) - } - } else { + if sshKey == "" { // Remove SSH key file if empty string is provided if err := os.Remove(sshKeyFile); err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to remove SSH key file: %w", err) } + return nil + } + + // Validate SSH key + if err := utils.ValidateSSHKey(sshKey); err != nil { + return err + } + + // Create directory if it doesn't exist + if err := os.MkdirAll(sshKeyDir, 0700); err != nil { + return fmt.Errorf("failed to create SSH key directory: %w", err) + } + + // Write SSH key to file + if err := os.WriteFile(sshKeyFile, []byte(sshKey), 0600); err != nil { + return fmt.Errorf("failed to write SSH key: %w", err) } return nil