secrets/ssh: allow algorithm_signer to use the key's default algo

builtin/logical/ssh/backend_test.go

@@ -103,14 +103,26 @@ Si1nAoGBAL47wWinv1cDTnh5mm0mybz3oI2a6V9aIYCloQ/EFcvtahyR/gyB8qNF
 lObH9Faf0WGdnACZvTz22U9gWhw79S0SpDV31tC5Kl8dXHFiZ09vYUKkYmSd/kms
 SeKWrUkryx46LVf6NMhkyYmRqCEjBwfOozzezi5WbiJy6nn54GQt
 -----END RSA PRIVATE KEY-----
+`
+
+	testCAPublicKeyEd25519 = `ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIO1S6g5Bib7vT8eoFnvTl3dZSjOQL/GkH1nkRcDS9++a ca
+`
+
+	testCAPrivateKeyEd25519 = `-----BEGIN OPENSSH PRIVATE KEY-----
+b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
+QyNTUxOQAAACDtUuoOQYm+70/HqBZ705d3WUozkC/xpB9Z5EXA0vfvmgAAAIhfRuszX0br
+MwAAAAtzc2gtZWQyNTUxOQAAACDtUuoOQYm+70/HqBZ705d3WUozkC/xpB9Z5EXA0vfvmg
+AAAEBQYa029SP/7AGPFQLmzwOc9eCoOZuwCq3iIf2C6fj9j+1S6g5Bib7vT8eoFnvTl3dZ
+SjOQL/GkH1nkRcDS9++aAAAAAmNhAQID
+-----END OPENSSH PRIVATE KEY-----
 `
 
 	// testPublicKeyInstall is the public key that is installed in the
 	// admin account's authorized_keys
 	testPublicKeyInstall = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC9i+hFxZHGo6KblVme4zrAcJstR6I0PTJozW286X4WyvPnkMYDQ5mnhEYC7UWCvjoTWbPEXPX7NjhRtwQTGD67bV+lrxgfyzK1JZbUXK4PwgKJvQD+XyyWYMzDgGSQY61KUSqCxymSm/9NZkPU3ElaQ9xQuTzPpztM4ROfb8f2Yv6/ZESZsTo0MTAkp8Pcy+WkioI/uJ1H7zqs0EA4OMY4aDJRu0UtP4rTVeYNEAuRXdX+eH4aW3KMvhzpFTjMbaJHJXlEeUm2SaX5TNQyTOvghCeQILfYIL/Ca2ij8iwCmulwdV6eQGfd4VDu40PvSnmfoaE38o6HaPnX0kUcnKiT"
 
-	dockerImageTagSupportsRSA   = "8.1_p1-r0-ls20"
-	dockerImageTagSupportsNoRSA = "8.3_p1-r0-ls21"
+	dockerImageTagSupportsRSA1   = "8.1_p1-r0-ls20"
+	dockerImageTagSupportsNoRSA1 = "8.3_p1-r0-ls21"
 )
 
 func prepareTestContainer(t *testing.T, tag, caPublicKeyPEM string) (func(), string) {
@@ -125,7 +137,7 @@ func prepareTestContainer(t *testing.T, tag, caPublicKeyPEM string) (func(), str
 	}
 
 	if tag == "" {
-		tag = dockerImageTagSupportsNoRSA
+		tag = dockerImageTagSupportsNoRSA1
 	}
 	resource, err := pool.RunWithOptions(&dockertest.RunOptions{
 		Repository: "linuxserver/openssh-server",
@@ -668,29 +680,33 @@ func TestSSHBackend_CredsForZeroAddressRoles_dynamic(t *testing.T) {
 
 func TestSSHBackend_CA(t *testing.T) {
 	testCases := []struct {
-		name        string
-		tag         string
-		algoSigner  string
-		expectError bool
+		name         string
+		tag          string
+		caPublicKey  string
+		caPrivateKey string
+		algoSigner   string
+		expectError  bool
 	}{
-		{"defaultSignerSSHDSupport", dockerImageTagSupportsRSA, "", false},
-		{"rsaSignerSSHDSupport", dockerImageTagSupportsRSA, ssh.SigAlgoRSA, false},
-		{"rsa2SignerSSHDSupport", dockerImageTagSupportsRSA, ssh.SigAlgoRSASHA2256, false},
-		{"rsa2SignerNoSSHDSupport", dockerImageTagSupportsNoRSA, ssh.SigAlgoRSASHA2256, false},
-		{"defaultSignerNoSSHDSupport", dockerImageTagSupportsNoRSA, "", true},
+		{"RSAKey/EmptyAlgoSigner/ImageSupportsRSA1", dockerImageTagSupportsRSA1, testCAPublicKey, testCAPrivateKey, "", false},
+		{"RSAKey/EmptyAlgoSigner/ImageSupportsNoRSA1", dockerImageTagSupportsNoRSA1, testCAPublicKey, testCAPrivateKey, "", true},
+		{"RSAKey/RSA1AlgoSigner/ImageSupportsRSA1", dockerImageTagSupportsRSA1, testCAPublicKey, testCAPrivateKey, ssh.SigAlgoRSA, false},
+		{"RSAKey/RSASHA2256AlgoSigner/ImageSupportsRSA1", dockerImageTagSupportsRSA1, testCAPublicKey, testCAPrivateKey, ssh.SigAlgoRSASHA2256, false},
+		{"RSAKey/RSASHA2256AlgoSigner/ImageSupportsNoRSA1", dockerImageTagSupportsNoRSA1, testCAPublicKey, testCAPrivateKey, ssh.SigAlgoRSASHA2256, false},
+		{"ed25519Key/EmptyAlgoSigner/ImageSupportsRSA1", dockerImageTagSupportsRSA1, testCAPublicKeyEd25519, testCAPrivateKeyEd25519, "", false},
+		{"ed25519Key/EmptyAlgoSigner/ImageSupportsNoRSA1", dockerImageTagSupportsNoRSA1, testCAPublicKeyEd25519, testCAPrivateKeyEd25519, "", false},
+		{"ed25519Key/RSA1AlgoSigner/ImageSupportsRSA1", dockerImageTagSupportsRSA1, testCAPublicKeyEd25519, testCAPrivateKeyEd25519, ssh.SigAlgoRSA, true},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			testSSHBackend_CA(t, tc.tag, tc.algoSigner, tc.expectError)
+			testSSHBackend_CA(t, tc.tag, tc.caPublicKey, tc.caPrivateKey, tc.algoSigner, tc.expectError)
 		})
 	}
 }
 
-func testSSHBackend_CA(t *testing.T, dockerImageTag, algorithmSigner string, expectError bool) {
-	cleanup, sshAddress := prepareTestContainer(t, dockerImageTag, testCAPublicKey)
+func testSSHBackend_CA(t *testing.T, dockerImageTag, caPublicKey, caPrivateKey, algorithmSigner string, expectError bool) {
+	cleanup, sshAddress := prepareTestContainer(t, dockerImageTag, caPublicKey)
 	defer cleanup()
-
 	config := logical.TestBackendConfig()
 
 	b, err := Factory(context.Background(), config)
@@ -746,7 +762,7 @@ cKumubUxOfFdy1ZvAAAAEm5jY0BtYnAudWJudC5sb2NhbA==
 	testCase := logicaltest.TestCase{
 		LogicalBackend: b,
 		Steps: []logicaltest.TestStep{
-			configCaStep(),
+			configCaStep(caPublicKey, caPrivateKey),
 			testRoleWrite(t, "testcarole", roleOptions),
 			logicaltest.TestStep{
 				Operation: logical.UpdateOperation,
@@ -795,6 +811,127 @@ cKumubUxOfFdy1ZvAAAAEm5jY0BtYnAudWJudC5sb2NhbA==
 	logicaltest.Test(t, testCase)
 }
 
+func TestSSHBackend_CAUpgradeAlgorithmSigner(t *testing.T) {
+	cleanup, sshAddress := prepareTestContainer(t, dockerImageTagSupportsRSA1, testCAPublicKey)
+	defer cleanup()
+	config := logical.TestBackendConfig()
+
+	b, err := Factory(context.Background(), config)
+	if err != nil {
+		t.Fatalf("Cannot create backend: %s", err)
+	}
+
+	testKeyToSignPrivate := `-----BEGIN OPENSSH PRIVATE KEY-----
+b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAABFwAAAAdzc2gtcn
+NhAAAAAwEAAQAAAQEAwn1V2xd/EgJXIY53fBTtc20k/ajekqQngvkpFSwNHW63XNEQK8Ll
+FOCyGXoje9DUGxnYs3F/ohfsBBWkLNfU7fiENdSJL1pbkAgJ+2uhV9sLZjvYhikrXWoyJX
+LDKfY12LjpcBS2HeLMT04laZ/xSJrOBEJHGzHyr2wUO0NUQUQPUODAFhnHKgvvA4Uu79UY
+gcdThF4w83+EAnE4JzBZMKPMjzy4u1C0R/LoD8DuapHwX6NGWdEUvUZZ+XRcIWeCOvR0ne
+qGBRH35k1Mv7k65d7kkE0uvM5Z36erw3tdoszxPYf7AKnO1DpeU2uwMcym6xNwfwynKjhL
+qL/Mgi4uRwAAA8iAsY0zgLGNMwAAAAdzc2gtcnNhAAABAQDCfVXbF38SAlchjnd8FO1zbS
+T9qN6SpCeC+SkVLA0dbrdc0RArwuUU4LIZeiN70NQbGdizcX+iF+wEFaQs19Tt+IQ11Ikv
+WluQCAn7a6FX2wtmO9iGKStdajIlcsMp9jXYuOlwFLYd4sxPTiVpn/FIms4EQkcbMfKvbB
+Q7Q1RBRA9Q4MAWGccqC+8DhS7v1RiBx1OEXjDzf4QCcTgnMFkwo8yPPLi7ULRH8ugPwO5q
+kfBfo0ZZ0RS9Rln5dFwhZ4I69HSd6oYFEffmTUy/uTrl3uSQTS68zlnfp6vDe12izPE9h/
+sAqc7UOl5Ta7AxzKbrE3B/DKcqOEuov8yCLi5HAAAAAwEAAQAAAQABns2yT5XNbpuPOgKg
+1APObGBchKWmDxwNKUpAVOefEScR7OP3mV4TOHQDZlMZWvoJZ8O4av+nOA/NUOjXPs0VVn
+azhBvIezY8EvUSVSk49Cg6J9F7/KfR1WqpiTU7CkQUlCXNuz5xLUyKdJo3MQ/vjOqeenbh
+MR9Wes4IWF1BVe4VOD6lxRsjwuIieIgmScW28FFh2rgsEfO2spzZ3AWOGExw+ih757hFz5
+4A2fhsQXP8m3r8m7iiqcjTLWXdxTUk4zot2kZEjbI4Avk0BL+wVeFq6f/y+G+g5edqSo7j
+uuSgzbUQtA9PMnGxhrhU2Ob7n3VGdya7WbGZkaKP8zJhAAAAgQC3bJurmOSLIi3KVhp7lD
+/FfxwXHwVBFALCgq7EyNlkTz6RDoMFM4eOTRMDvsgWxT+bSB8R8eg1sfgY8rkHOuvTAVI5
+3oEYco3H7NWE9X8Zt0lyhO1uaE49EENNSQ8hY7R3UIw5becyI+7ZZxs9HkBgCQCZzSjzA+
+SIyAoMKM261AAAAIEA+PCkcDRp3J0PaoiuetXSlWZ5WjP3CtwT2xrvEX9x+ZsDgXCDYQ5T
+osxvEKOGSfIrHUUhzZbFGvqWyfrziPe9ypJrtCM7RJT/fApBXnbWFcDZzWamkQvohst+0w
+XHYCmNoJ6/Y+roLv3pzyFUmqRNcrQaohex7TZmsvHJT513UakAAACBAMgBXxH8DyNYdniX
+mIXEto4GqMh4rXdNwCghfpyWdJE6vCyDt7g7bYMq7AQ2ynSKRtQDT/ZgQNfSbilUq3iXz7
+xNZn5U9ndwFs90VmEpBup/PmhfX+Gwt5hQZLbkKZcgQ9XrhSKdMxVm1yy/fk0U457enlz5
+cKumubUxOfFdy1ZvAAAAEm5jY0BtYnAudWJudC5sb2NhbA==
+-----END OPENSSH PRIVATE KEY-----
+`
+	testKeyToSignPublic := `ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDCfVXbF38SAlchjnd8FO1zbST9qN6SpCeC+SkVLA0dbrdc0RArwuUU4LIZeiN70NQbGdizcX+iF+wEFaQs19Tt+IQ11IkvWluQCAn7a6FX2wtmO9iGKStdajIlcsMp9jXYuOlwFLYd4sxPTiVpn/FIms4EQkcbMfKvbBQ7Q1RBRA9Q4MAWGccqC+8DhS7v1RiBx1OEXjDzf4QCcTgnMFkwo8yPPLi7ULRH8ugPwO5qkfBfo0ZZ0RS9Rln5dFwhZ4I69HSd6oYFEffmTUy/uTrl3uSQTS68zlnfp6vDe12izPE9h/sAqc7UOl5Ta7AxzKbrE3B/DKcqOEuov8yCLi5H `
+
+	// Old role entries between 1.4.3 and 1.5.2 had algorithm_signer default to
+	// ssh-rsa if not provided.
+	roleOptionsOldEntry := map[string]interface{}{
+		"allow_user_certificates": true,
+		"allowed_users":           "*",
+		"default_extensions": []map[string]string{
+			{
+				"permit-pty": "",
+			},
+		},
+		"key_type":         "ca",
+		"default_user":     testUserName,
+		"ttl":              "30m0s",
+		"algorithm_signer": ssh.SigAlgoRSA,
+	}
+
+	// Upgrade entry by overwriting algorithm_signer with an empty value
+	roleOptionsUpgradedEntry := map[string]interface{}{
+		"allow_user_certificates": true,
+		"allowed_users":           "*",
+		"default_extensions": []map[string]string{
+			{
+				"permit-pty": "",
+			},
+		},
+		"key_type":         "ca",
+		"default_user":     testUserName,
+		"ttl":              "30m0s",
+		"algorithm_signer": "",
+	}
+
+	testCase := logicaltest.TestCase{
+		LogicalBackend: b,
+		Steps: []logicaltest.TestStep{
+			configCaStep(testCAPublicKey, testCAPrivateKey),
+			testRoleWrite(t, "testcarole", roleOptionsOldEntry),
+			testRoleWrite(t, "testcarole", roleOptionsUpgradedEntry),
+			logicaltest.TestStep{
+				Operation: logical.UpdateOperation,
+				Path:      "sign/testcarole",
+				ErrorOk:   false,
+				Data: map[string]interface{}{
+					"public_key":       testKeyToSignPublic,
+					"valid_principals": testUserName,
+				},
+
+				Check: func(resp *logical.Response) error {
+
+					signedKey := strings.TrimSpace(resp.Data["signed_key"].(string))
+					if signedKey == "" {
+						return errors.New("no signed key in response")
+					}
+
+					privKey, err := ssh.ParsePrivateKey([]byte(testKeyToSignPrivate))
+					if err != nil {
+						return fmt.Errorf("error parsing private key: %v", err)
+					}
+
+					parsedKey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(signedKey))
+					if err != nil {
+						return fmt.Errorf("error parsing signed key: %v", err)
+					}
+					certSigner, err := ssh.NewCertSigner(parsedKey.(*ssh.Certificate), privKey)
+					if err != nil {
+						return err
+					}
+
+					err = testSSH(t, testUserName, sshAddress, ssh.PublicKeys(certSigner), "date")
+					if err != nil {
+						return err
+					}
+
+					return nil
+				},
+			},
+		},
+	}
+
+	logicaltest.Test(t, testCase)
+}
+
 func TestBackend_AbleToRetrievePublicKey(t *testing.T) {
 
 	config := logical.TestBackendConfig()
@@ -807,7 +944,7 @@ func TestBackend_AbleToRetrievePublicKey(t *testing.T) {
 	testCase := logicaltest.TestCase{
 		LogicalBackend: b,
 		Steps: []logicaltest.TestStep{
-			configCaStep(),
+			configCaStep(testCAPublicKey, testCAPrivateKey),
 
 			logicaltest.TestStep{
 				Operation:       logical.ReadOperation,
@@ -892,7 +1029,7 @@ func TestBackend_ValidPrincipalsValidatedForHostCertificates(t *testing.T) {
 	testCase := logicaltest.TestCase{
 		LogicalBackend: b,
 		Steps: []logicaltest.TestStep{
-			configCaStep(),
+			configCaStep(testCAPublicKey, testCAPrivateKey),
 
 			createRoleStep("testing", map[string]interface{}{
 				"key_type":                "ca",
@@ -935,7 +1072,7 @@ func TestBackend_OptionsOverrideDefaults(t *testing.T) {
 	testCase := logicaltest.TestCase{
 		LogicalBackend: b,
 		Steps: []logicaltest.TestStep{
-			configCaStep(),
+			configCaStep(testCAPublicKey, testCAPrivateKey),
 
 			createRoleStep("testing", map[string]interface{}{
 				"key_type":                 "ca",
@@ -982,7 +1119,7 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) {
 	testCase := logicaltest.TestCase{
 		LogicalBackend: b,
 		Steps: []logicaltest.TestStep{
-			configCaStep(),
+			configCaStep(testCAPublicKey, testCAPrivateKey),
 			createRoleStep("weakkey", map[string]interface{}{
 				"key_type":                "ca",
 				"allow_user_certificates": true,
@@ -1051,7 +1188,7 @@ func TestBackend_CustomKeyIDFormat(t *testing.T) {
 	testCase := logicaltest.TestCase{
 		LogicalBackend: b,
 		Steps: []logicaltest.TestStep{
-			configCaStep(),
+			configCaStep(testCAPublicKey, testCAPrivateKey),
 
 			createRoleStep("customrole", map[string]interface{}{
 				"key_type":                 "ca",
@@ -1100,7 +1237,7 @@ func TestBackend_DisallowUserProvidedKeyIDs(t *testing.T) {
 	testCase := logicaltest.TestCase{
 		LogicalBackend: b,
 		Steps: []logicaltest.TestStep{
-			configCaStep(),
+			configCaStep(testCAPublicKey, testCAPrivateKey),
 
 			createRoleStep("testing", map[string]interface{}{
 				"key_type":                "ca",
@@ -1128,13 +1265,13 @@ func TestBackend_DisallowUserProvidedKeyIDs(t *testing.T) {
 	logicaltest.Test(t, testCase)
 }
 
-func configCaStep() logicaltest.TestStep {
+func configCaStep(caPublicKey, caPrivateKey string) logicaltest.TestStep {
 	return logicaltest.TestStep{
 		Operation: logical.UpdateOperation,
 		Path:      "config/ca",
 		Data: map[string]interface{}{
-			"public_key":  testCAPublicKey,
-			"private_key": testCAPrivateKey,
+			"public_key":  caPublicKey,
+			"private_key": caPrivateKey,
 		},
 	}
 }

builtin/logical/ssh/path_roles.go

@@ -3,16 +3,15 @@ package ssh
 import (
 	"context"
 	"fmt"
-	"golang.org/x/crypto/ssh"
 	"strings"
-
 	"time"
 
 	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/cidrutil"
 	"github.com/hashicorp/vault/sdk/helper/parseutil"
 	"github.com/hashicorp/vault/sdk/logical"
+	"golang.org/x/crypto/ssh"
 )
 
 const (
@@ -334,10 +333,9 @@ func pathRoles(b *backend) *framework.Path {
                                 `,
 			},
 			"algorithm_signer": &framework.FieldSchema{
-				Type:    framework.TypeString,
-				Default: ssh.SigAlgoRSA,
+				Type: framework.TypeString,
 				Description: `
-				When supplied, this value specifies a signing algorithm for the key.  Possible values: 
+				When supplied, this value specifies a signing algorithm for the key. Possible values: 
 				ssh-rsa, rsa-sha2-256, rsa-sha2-512.
 				`,
 				DisplayAttrs: &framework.DisplayAttributes{
@@ -479,11 +477,18 @@ func (b *backend) pathRoleWrite(ctx context.Context, req *logical.Request, d *fr
 			KeyOptionSpecs:  keyOptionSpecs,
 		}
 	} else if keyType == KeyTypeCA {
-		algorithmSigner := d.Get("algorithm_signer").(string)
-		switch algorithmSigner {
-		case ssh.SigAlgoRSA, ssh.SigAlgoRSASHA2256, ssh.SigAlgoRSASHA2512:
-		default:
-			return nil, fmt.Errorf("unknown algorithm signer %q", algorithmSigner)
+		algorithmSigner := ""
+		algorithmSignerRaw, ok := d.GetOk("algorithm_signer")
+		if ok {
+			algorithmSigner = algorithmSignerRaw.(string)
+			switch algorithmSigner {
+			case ssh.SigAlgoRSA, ssh.SigAlgoRSASHA2256, ssh.SigAlgoRSASHA2512:
+			case "":
+				// This case is valid, and the sign operation will use the signer's
+				// default algorithm.
+			default:
+				return nil, fmt.Errorf("unknown algorithm signer %q", algorithmSigner)
+			}
 		}
 
 		role, errorResponse := b.createCARole(allowedUsers, d.Get("default_user").(string), algorithmSigner, d)

builtin/logical/ssh/path_sign.go

@@ -530,9 +530,6 @@ func (b *creationBundle) sign() (retCert *ssh.Certificate, retErr error) {
 	certificateBytes := out[:len(out)-4]
 
 	algo := b.Role.AlgorithmSigner
-	if algo == "" {
-		algo = ssh.SigAlgoRSA
-	}
 	sig, err := sshAlgorithmSigner.SignWithAlgorithm(rand.Reader, certificateBytes, algo)
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate signed SSH key: sign error")