Vault SSH: Mandate default_user. Other refactoring

builtin/logical/ssh/communicator.go

@@ -246,8 +246,6 @@ func (c *comm) scpSession(scpCommand string, f func(io.Writer, *bufio.Reader) er
 
 		return err
 	}
-
-	log.Printf("scp stderr (length %d): %s", stderr.Len(), stderr.String())
 	return nil
 }
 
@@ -292,7 +290,6 @@ func scpUploadFile(dst string, src io.Reader, w io.Writer, r *bufio.Reader, fi *
 
 		mode = 0644
 
-		log.Println("Copying input data into temporary file so we can read the length")
 		if _, err := io.Copy(tf, src); err != nil {
 			return err
 		}

builtin/logical/ssh/path_creds_create.go

@@ -3,7 +3,6 @@ package ssh
 import (
 	"fmt"
 	"net"
-	"strconv"
 	"time"
 
 	"github.com/hashicorp/vault/helper/uuid"
@@ -155,13 +154,7 @@ func (b *backend) GenerateDynamicCredential(req *logical.Request, role *sshRole,
 		return "", "", fmt.Errorf("error reading the host key: %s", err)
 	}
 
-	// Generate RSA key pair
+	dynamicPublicKey, dynamicPrivateKey, err := generateRSAKeys(role.KeyBits)
-	keyBits, err := strconv.Atoi(role.KeyBits)
-	if err != nil {
-		return "", "", fmt.Errorf("error reading key bit size: %s", err)
-	}
-
-	dynamicPublicKey, dynamicPrivateKey, err := generateRSAKeys(keyBits)
 	if err != nil {
 		return "", "", fmt.Errorf("error generating key: %s", err)
 	}
@@ -180,7 +173,7 @@ func (b *backend) GenerateDynamicCredential(req *logical.Request, role *sshRole,
 	}
 
 	// Add the public key to authorized_keys file in target machine
-	err = installPublicKeyInTarget(role.AdminUser, publicKeyFileName, username, ip, role.Port, hostKey.Key)
+	err = installPublicKeyInTarget(role.AdminUser, publicKeyFileName, username, ip, role.Port, hostKey.Key, true)
 	if err != nil {
 		return "", "", fmt.Errorf("error adding public key to authorized_keys file in target")
 	}

builtin/logical/ssh/path_roles.go

@@ -3,7 +3,6 @@ package ssh
 import (
 	"fmt"
 	"net"
-	"strconv"
 	"strings"
 
 	"github.com/hashicorp/vault/logical"
@@ -12,7 +11,6 @@ import (
 
 const KeyTypeOTP = "otp"
 const KeyTypeDynamic = "dynamic"
-const KeyBitsRSA = "2048"
 
 func pathRoles(b *backend) *framework.Path {
 	return &framework.Path{
@@ -20,39 +18,68 @@ func pathRoles(b *backend) *framework.Path {
 		Fields: map[string]*framework.FieldSchema{
 			"role": &framework.FieldSchema{
 				Type: framework.TypeString,
-				Description: "Name of the role",
+				Description: `
+				[Required for both types]
+				Name of the role being created.`,
 			},
 			"key": &framework.FieldSchema{
 				Type: framework.TypeString,
-				Description: "Named key in Vault",
+				Description: `
+				[Required for dynamic type] [Not applicable for otp type]
+				Name of the registered key in Vault. Before creating the role, use the
+				'keys/' endpoint to create a named key.`,
 			},
 			"admin_user": &framework.FieldSchema{
 				Type: framework.TypeString,
-				Description: "Admin user at target address",
+				Description: `
+				[Required for dynamic type] [Not applicable for otp type]
+				Admin user at remote host. The shared key being registered should be
+				for this user and should have root privileges. Everytime a dynamic 
+				credential is being generated for other users, Vault uses this admin
+				username to login to remote host and install the generated credential
+				for the other user.`,
 			},
 			"default_user": &framework.FieldSchema{
 				Type: framework.TypeString,
-				Description: "Default user to whom the dynamic key is installed",
+				Description: `
+				[Required for both types]
+				Default username for which a credential will be generated.
+				When the endpoint 'creds/' is used without a username, this
+				value will be used as default username.`,
 			},
 			"cidr_list": &framework.FieldSchema{
 				Type: framework.TypeString,
-				Description: "Comma separated CIDR blocks and IP addresses",
+				Description: `
+				[Required for both types]
+				Comma separated list of CIDR blocks for which the role is applicable for.
+				CIDR blocks can belong to more than one role.`,
 			},
 			"port": &framework.FieldSchema{
 				Type: framework.TypeInt,
-				Description: "Port number for SSH connection",
+				Description: `
+				[Optional for both types]
+				Port number for SSH connection. Default is '22'.`,
 			},
 			"key_type": &framework.FieldSchema{
 				Type: framework.TypeString,
-				Description: "one-time-password or dynamic-key",
+				Description: `
+				[Required for both types] 
+				Type of key used to login to hosts. It can be either 'otp' or 'dynamic'.
+				'otp' type requires agent to be installed in remote hosts.`,
 			},
 			"key_bits": &framework.FieldSchema{
-				Type:        framework.TypeString,
+				Type: framework.TypeInt,
-				Description: "number of bits in keys",
+				Description: `
+				[Optional for dynamic type] [Not applicable for otp type]
+				Length of the RSA dynamic key in bits. It can be one of 1024, 2048 or 4096.`,
 			},
 			"install_script": &framework.FieldSchema{
 				Type: framework.TypeString,
-				Description: "script that installs public key in target",
+				Description: `
+				[Optional for dynamic type][Not-applicable for otp type]
+				Script used to install and uninstall public keys in the target machine.
+				The inbuilt default install script will be for Linux hosts. For sample
+				script, refer the project's documentation website.`,
 			},
 		},
 
@@ -73,6 +100,11 @@ func (b *backend) pathRoleWrite(req *logical.Request, d *framework.FieldData) (*
 		return logical.ErrorResponse("Missing role name"), nil
 	}
 
+	defaultUser := d.Get("default_user").(string)
+	if defaultUser == "" {
+		return logical.ErrorResponse("Missing default user"), nil
+	}
+
 	cidrList := d.Get("cidr_list").(string)
 	if cidrList == "" {
 		return logical.ErrorResponse("Missing CIDR blocks"), nil
@@ -95,25 +127,20 @@ func (b *backend) pathRoleWrite(req *logical.Request, d *framework.FieldData) (*
 	}
 	keyType = strings.ToLower(keyType)
 
-	var entry *logical.StorageEntry
 	var err error
+	var roleEntry sshRole
 	if keyType == KeyTypeOTP {
 		adminUser := d.Get("admin_user").(string)
 		if adminUser != "" {
 			return logical.ErrorResponse("Admin user not required for OTP type"), nil
 		}
 
-		defaultUser := d.Get("default_user").(string)
+		roleEntry = sshRole{
-		if defaultUser == "" {
-			return logical.ErrorResponse("Missing default user"), nil
-		}
-
-		entry, err = logical.StorageEntryJSON(fmt.Sprintf("roles/%s", roleName), sshRole{
 			DefaultUser: defaultUser,
 			CIDRList:    cidrList,
 			KeyType:     KeyTypeOTP,
 			Port:        port,
-		})
+		}
 	} else if keyType == KeyTypeDynamic {
 		keyName := d.Get("key").(string)
 		if keyName == "" {
@@ -134,23 +161,15 @@ func (b *backend) pathRoleWrite(req *logical.Request, d *framework.FieldData) (*
 			return logical.ErrorResponse("Missing admin username"), nil
 		}
 
-		defaultUser := d.Get("default_user").(string)
+		keyBits := d.Get("key_bits").(int)
-		if defaultUser == "" {
+		if keyBits != 0 && keyBits != 1024 && keyBits != 2048 && keyBits != 4096 {
-			defaultUser = adminUser
+			return logical.ErrorResponse("Invalid key_bits field"), nil
+		}
+		if keyBits == 0 {
+			keyBits = 2048
 		}
 
-		keyBits := d.Get("key_bits").(string)
+		roleEntry = sshRole{
-		if keyBits != "" {
-			_, err := strconv.Atoi(keyBits)
-			if err != nil {
-				return logical.ErrorResponse("Key bits should be an integer"), nil
-			}
-		}
-		if keyBits == "" {
-			keyBits = KeyBitsRSA
-		}
-
-		entry, err = logical.StorageEntryJSON(fmt.Sprintf("roles/%s", roleName), sshRole{
 			KeyName:       keyName,
 			AdminUser:     adminUser,
 			DefaultUser:   defaultUser,
@@ -159,11 +178,12 @@ func (b *backend) pathRoleWrite(req *logical.Request, d *framework.FieldData) (*
 			KeyType:       KeyTypeDynamic,
 			KeyBits:       keyBits,
 			InstallScript: installScript,
-		})
+		}
 	} else {
 		return logical.ErrorResponse("Invalid key type"), nil
 	}
 
+	entry, err := logical.StorageEntryJSON(fmt.Sprintf("roles/%s", roleName), roleEntry)
 	if err != nil {
 		return nil, err
 	}
@@ -224,7 +244,7 @@ func (b *backend) pathRoleDelete(req *logical.Request, d *framework.FieldData) (
 type sshRole struct {
 	KeyType       string `mapstructure:"key_type" json:"key_type"`
 	KeyName       string `mapstructure:"key" json:"key"`
-	KeyBits       string `mapstructure:"key_bits" json:"key_bits"`
+	KeyBits       int    `mapstructure:"key_bits" json:"key_bits"`
 	AdminUser     string `mapstructure:"admin_user" json:"admin_user"`
 	DefaultUser   string `mapstructure:"default_user" json:"default_user"`
 	CIDRList      string `mapstructure:"cidr_list" json:"cidr_list"`
@@ -237,8 +257,8 @@ Manage the 'roles' that can be created with this backend.
 `
 
 const pathRoleHelpDesc = `
-This path allows you to manage the roles that are used to create
+This path allows you to manage the roles that are used to generate 
-keys. These roles will be having privileged access to all
+credentials. These roles will be having privileged access to all
 the hosts mentioned by CIDR blocks. For example, if the backend
 is mounted at "ssh" and the role is created at "ssh/roles/web",
 then a user could request for a new key at "ssh/creds/web" for the
@@ -249,37 +269,4 @@ should have root access in all the hosts represented by the 'cidr_list'
 field. When the user requests key for an IP, the key will be installed
 for the user mentioned by 'default_user' field. The 'key' field takes
 a named key which can be configured by 'ssh/keys/' endpoint.
-
-Role Options:
-
-  -key_type		This can be either 'otp' or 'dynamic'. 'otp' key requires
-  			agent to be installed in target machine. Required field for
-			both types.
-
-  -key			Name of the key registered using 'keys/' endpoint. Required
-  			field for 'dynamic' type. Not applicable for 'otp' type.
-
-  -admin_user		Username at the target which is having root privileges. This
-  			username will be used to install keys for other unprivileged
-			users. Required field for 'dynamic' type. Not applicable for
-			'otp' type.
-
-  -default_user		When keys are created using '/creds' endpoint with only the
-  			IP address, by default, this username is used to create the
-			credentials. Required for 'otp' type. Optional for 'dynamic' type.
-
-  -cidr_list		CIDR block for which is role is applicable for. Required field
-  			for both types.
-
-  -port			Port number for SSH connections. Default is '22'. Optional for
-  			both types.
-
-  -key_bits		Length of RSa dynamic key in bits. Optional for 'dynamic' type.
-  			Not applicable for 'otp' type.
-
-  -install_script	Script used to install and uninstall public keys in the target
-  			machine. Required for 'dynamic' type. Not applicable for 'otp'
-			type.
-			[For Linux, refer https://github.com/hashicorp/vault/tree/master/
-			builtin/logical/ssh/scripts/key-install-linux.sh]
 `

builtin/logical/ssh/path_verify.go

@@ -73,5 +73,5 @@ target machine to check if the key provided by the client
 to establish the SSH connection is valid or not.
 
 This key will be a one-time-password. The vault server responds
-that the key is valid only once (hence one-time).
+that the key is valid and then deletes it, hence the key is OTP. 
 `

builtin/logical/ssh/secret_dynamic_key.go

@@ -128,7 +128,8 @@ func (b *backend) secretDynamicKeyRevoke(req *logical.Request, d *framework.Fiel
 	}
 
 	// Remove the public key from authorized_keys file in target machine
-	err = uninstallPublicKeyInTarget(adminUser, dynamicPublicKeyFileName, username, ip, port, hostKey.Key)
+	// The last param 'false' indicates that the key should be uninstalled.
+	err = installPublicKeyInTarget(adminUser, dynamicPublicKeyFileName, username, ip, port, hostKey.Key, false)
 	if err != nil {
 		return nil, fmt.Errorf("error removing public key from authorized_keys file in target")
 	}

builtin/logical/ssh/util.go

@@ -79,9 +79,13 @@ func generateRSAKeys(keyBits int) (publicKeyRsa string, privateKeyRsa string, er
 	return
 }
 
-// Concatenates the public present in that target machine's home
+// Installs or uninstalls the dynamic key in the remote host. The parameterized script
-// folder to ~/.ssh/authorized_keys file
+// will install or uninstall the key. The remote host is assumed to be Linux,
-func installPublicKeyInTarget(adminUser, publicKeyFileName, username, ip string, port int, hostkey string) error {
+// and hence the path of the authorized_keys file is hard coded to resemble Linux.
+// Installing and uninstalling the keys means that the public key is appended or
+// removed from authorized_keys file.
+// The param 'install' if false, uninstalls the key.
+func installPublicKeyInTarget(adminUser, publicKeyFileName, username, ip string, port int, hostkey string, install bool) error {
 	session, err := createSSHPublicKeysSession(adminUser, ip, port, hostkey)
 	if err != nil {
 		return fmt.Errorf("unable to create SSH Session using public keys: %s", err)
@@ -94,9 +98,15 @@ func installPublicKeyInTarget(adminUser, publicKeyFileName, username, ip string,
 	authKeysFileName := fmt.Sprintf("/home/%s/.ssh/authorized_keys", username)
 	scriptFileName := fmt.Sprintf("%s.sh", publicKeyFileName)
 
+	var installOption string
+	if install {
+		installOption = "install"
+	} else {
+		installOption = "uninstall"
+	}
 	// Give execute permissions to install script, run and delete it.
 	chmodCmd := fmt.Sprintf("chmod +x %s", scriptFileName)
-	scriptCmd := fmt.Sprintf("./%s install %s %s", scriptFileName, publicKeyFileName, authKeysFileName)
+	scriptCmd := fmt.Sprintf("./%s %s %s %s", scriptFileName, installOption, publicKeyFileName, authKeysFileName)
 	rmCmd := fmt.Sprintf("rm -f %s", scriptFileName)
 	targetCmd := fmt.Sprintf("%s;%s;%s", chmodCmd, scriptCmd, rmCmd)
 
@@ -104,32 +114,6 @@ func installPublicKeyInTarget(adminUser, publicKeyFileName, username, ip string,
 	return nil
 }
 
-// Removes the installed public key from the authorized_keys file
-// in target machine
-func uninstallPublicKeyInTarget(adminUser, publicKeyFileName, username, ip string, port int, hostKey string) error {
-	session, err := createSSHPublicKeysSession(adminUser, ip, port, hostKey)
-	if err != nil {
-		return fmt.Errorf("unable to create SSH Session using public keys: %s", err)
-	}
-	if session == nil {
-		return fmt.Errorf("invalid session object")
-	}
-	defer session.Close()
-
-	authKeysFileName := fmt.Sprintf("/home/%s/.ssh/authorized_keys", username)
-	scriptFileName := fmt.Sprintf("%s.sh", publicKeyFileName)
-
-	// Give execute permissions to install script, run and delete it.
-	chmodCmd := fmt.Sprintf("chmod +x %s", scriptFileName)
-	scriptCmd := fmt.Sprintf("./%s uninstall %s %s", scriptFileName, publicKeyFileName, authKeysFileName)
-	rmCmd := fmt.Sprintf("rm -f %s", scriptFileName)
-	targetCmd := fmt.Sprintf("%s;%s;%s", chmodCmd, scriptCmd, rmCmd)
-
-	session.Run(targetCmd)
-
-	return nil
-}
-
 // Takes an IP address and role name and checks if the IP is part
 // of CIDR blocks belonging to the role.
 func roleContainsIP(s logical.Storage, roleName string, ip string) (bool, error) {