From c2ac761b0d2ceca1e90474718ffc60d1c8bc511b Mon Sep 17 00:00:00 2001 From: ncabatoff Date: Wed, 26 Aug 2020 16:05:56 -0400 Subject: [PATCH] secrets/ssh: allow algorithm_signer to use the key's default algo (#9824) (#9841) --- builtin/logical/ssh/backend_test.go | 249 +++++++++++++++++++++++++--- builtin/logical/ssh/path_roles.go | 25 +-- builtin/logical/ssh/path_sign.go | 3 - 3 files changed, 238 insertions(+), 39 deletions(-) diff --git a/builtin/logical/ssh/backend_test.go b/builtin/logical/ssh/backend_test.go index dd136be97035..baf3f5bf26c0 100644 --- a/builtin/logical/ssh/backend_test.go +++ b/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", @@ -669,29 +681,89 @@ 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) @@ -747,7 +819,7 @@ cKumubUxOfFdy1ZvAAAAEm5jY0BtYnAudWJudC5sb2NhbA== testCase := logicaltest.TestCase{ LogicalBackend: b, Steps: []logicaltest.TestStep{ - configCaStep(), + configCaStep(caPublicKey, caPrivateKey), testRoleWrite(t, "testcarole", roleOptions), logicaltest.TestStep{ Operation: logical.UpdateOperation, @@ -759,6 +831,10 @@ cKumubUxOfFdy1ZvAAAAEm5jY0BtYnAudWJudC5sb2NhbA== }, Check: func(resp *logical.Response) error { + // Tolerate nil response if an error was expected + if expectError && resp == nil { + return nil + } signedKey := strings.TrimSpace(resp.Data["signed_key"].(string)) if signedKey == "" { @@ -796,6 +872,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() @@ -808,7 +1005,7 @@ func TestBackend_AbleToRetrievePublicKey(t *testing.T) { testCase := logicaltest.TestCase{ LogicalBackend: b, Steps: []logicaltest.TestStep{ - configCaStep(), + configCaStep(testCAPublicKey, testCAPrivateKey), logicaltest.TestStep{ Operation: logical.ReadOperation, @@ -893,7 +1090,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", @@ -936,7 +1133,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", @@ -983,7 +1180,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, @@ -1052,7 +1249,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", @@ -1101,7 +1298,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", @@ -1129,13 +1326,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, }, } } diff --git a/builtin/logical/ssh/path_roles.go b/builtin/logical/ssh/path_roles.go index dd2430f9cfa5..708e46d516f0 100644 --- a/builtin/logical/ssh/path_roles.go +++ b/builtin/logical/ssh/path_roles.go @@ -3,9 +3,7 @@ package ssh import ( "context" "fmt" - "golang.org/x/crypto/ssh" "strings" - "time" "github.com/hashicorp/errwrap" @@ -13,6 +11,7 @@ import ( "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) diff --git a/builtin/logical/ssh/path_sign.go b/builtin/logical/ssh/path_sign.go index 3e7146e11e0c..fc8c1f29627e 100644 --- a/builtin/logical/ssh/path_sign.go +++ b/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")