Option for protecting keystore passphrase

[rhonnava]

Currently in Nortary Signer, the signing keys are AES wrapped with a password and the cipher text is persisted in a keystore.

However, the password used to AES wrap the private keys is as an environment variable.

If using the MySQL DB as a key store to persist the wrapped private keys, then the storage configruation in the signer configuration file looks as follows:

"storage": {
    "backend": "mysql",
    "db_url": "user:pass@tcp(notarymysql:3306)/databasename?parseTime=true",
    "default_alias": "passwordalias1"
}

• In cmd/notary-signer/config.go the default_alias field is concatenated with an envPrefix string 'NOTARYSIGNER\' and the resulting string ' NOTARYSIGNER' gives the name of the environment variable that Notary signer looks for in the getEnv function.
• The value of this environment variable contains the passphrase used to wrap the signing private keys and store in a key store like MySQL database, etc.

With this method of protecting private keys, the private keys are persisted as AES wrapped cipher text, but the passphrase used to wrap the private key is still available in clear text in the environemt. The problem with this method is that the private keys on the signer are as secure as the environment variable. Infact, the whole signer is as secure as this environment variable.

Solution

As we see, there is a need to protect the passphrase. There are several options, but firstly, we need to start with an additional field in the storage configuration to configure the passphrase protection method. Please find below, the same storage configuration example with an additional field 'passphrase_protect_method'.

"storage": {
    "backend": "mysql",
    "db_url": "user:pass@tcp(notarymysql:3306)/databasename?parseTime=true",
    "default_alias": "",
    "passphrase_protect_method" : "<none/hsm/other future methods>"
}

Options for protection

• None. This is the enviroment variable way of storing the passphrase. This will be default if nothing else is specified.
• HSMs. Hardware security modules have been time tested to be better at safeguarding sensitive data like passphrases from disclosure and from tampering. This would mean integrating with a PKCS11 interface. Additional configuration specific to PKCS11 needs to be configured.
• Existing open source vault solutions like Vault, KeyWhiz, etc.
• Other future solutions.

Proposed changes in code

Callback functions as follows will be implemented for each type of protection mechanism:

type initPasswordProtect(protectMethod string)(status bool, err error)

type protectPassword func(alias string, createNew bool, password string) (status bool, err error)

type retrievePassword func(alias string) (password string, err error)

• This will be called back in passphraseRetriever function in cmd/notary-signer/config.go to retrieve the password.

[endophage]

In principle I agree with adding better ways to retrieve passwords. My initial suggestion would be to not simply add callbacks, but define an interface and implement the appropriate concrete types implementing that interface for each mechanism.

type KeyWrapper interface {
    Encrypt(key []byte) ([]byte, error)
    Decrypt(ciphertext []byte) ([]byte, error)
}

type PasswordWrapper struct {
    password string
}

func NewPasswordWrapper(alias string) PasswordWrapper {
    return PasswordWrapper{
        password: os.Getenv(alias)
    }
}

func (pw PasswordWrapper) encrypt(key []byte) ([]byte, error) {
    // encrypt and return ciphertext
}

func (pw PasswordWrapper) decrypt(ciphertext []byte) ([]byte, error) {
    // decrypt and return plaintext key
}

IMO this is clearer than using a bunch of closures to maintain state. There was a reason we did the existing password retriever as a closure, but as we propose alternative mechanisms, it's becoming clearer that the closure has outlived it's loc brevity.

[rhonnava]

Thanks for the feedback. I have now updated it to use interfaces. Please let me know your thought on the below code snippet.

I was thinking in terms of having a PasswordStore interface that will abstract away all the password protection details from the config code.

The password store code will look like this:

package password

import "os"

type PasswordStore interface {
    getPassword() (string, error)
    setPassword(newPassword string) error
}

func NewPasswordStore(protectType string, alias string) PasswordStore {
    switch protectType {
    case "HSM":
        return HSMPasswordStore{
            alias:             alias,
            encryptedPassword: os.Getenv(alias),
            //Initialize the HSM specific variables here
        }
    case "VAULT":
        return VaultPasswordStore{
            alias:             alias,
            encryptedPassword: os.Getenv(alias),
            //Initialize the Vault specific variables here
        }
    case "NIL":
        fallthrough
    default:
        return DefaultPasswordStore{
            alias:             alias,
            encryptedPassword: os.Getenv(alias),
        }
    }
}

/************************************** DefaultPasswordStore **************************/
type DefaultPasswordStore struct {
    alias             string
    encryptedPassword string
}

//The default password store just stores the clear password in the ENV variable as is (like the current code)
func (pw DefaultPasswordStore) setPassword(newPassword string) error {
    os.Setenv(alias, newPassword)

    return nil
}

//The default password store just retrieves the clear password from the ENV variable as is
func (pw DefaultPasswordStore) getPassword() (string, error) {
    return pw.encryptedPassword, nil
}

/************************************** HSMPasswordStore (Example code) **************************/
type HSMPasswordStore struct {
    alias             string
    encryptedPassword string
    // Other parameters for HSM integration go here
}

//The HSM password store first encrypts the newPassword and stores the desensitized passwordAsCipherText it in the ENV variable
func (pw HSMPasswordStore) setPassword(newPassword string) error {
    passwordAsCipherText, error := encryptInHSM(newPassword)

    if nil != error {
        return error
    }

    os.Setenv(alias, passwordAsCipherText)
    return nil
}

//The HSM password store retrieves the encrypted password from the ENV variable, decrypts it and returns the clear password
func (pw HSMPasswordStore) getPassword() (string, error) {
    passwordAsCipherText := os.Getenv(pw.encryptedPassword)
    passwordInClear, error = decryptInHSM(passwordAsCipherText)

    if nil != error {
        return "", error
    }

    return passwordInClear, nil
}

/************************************** VaultPasswordStore (Example code) **************************/
type VaultPasswordStore struct {
    alias string
    //In a Vault password store the encryptedPassword parameter contains the handle to the password stored in the Vault
    encryptedPassword string
    // Other parameters for Vault integration go here
}

//The Vault password store first stores the password in the Vault and stores the handle (reference) to that password in the ENV variable
func (pw VaultPasswordStore) setPassword(newPassword string) error {
    handle, error := storeInVault(newPassword)

    if nil != error {
        return error
    }

    os.Setenv(alias, handle)

    return nil
}

//The Vault password store retrieves the handle from the ENV variable, exchanges it for the actual password stored  in the Vault and return the clear password
func (pw VaultPasswordStore) getPassword() (string, error) {
    passwordInClear, error = fetchFromHSM(pw.encryptedPassword)

    if nil != error {
        return "", error
    }

    return passwordInClear, nil
}

The caller code in config.go will look like this:

// func getEnv(env string) string {
//  v := viper.New()
//  utils.SetupViper(v, envPrefix)
//  return v.GetString(strings.ToUpper(env))
// }

func passphraseRetriever(keyName, alias string, protectType string, createNew bool, attempts int) (passphrase string, giveup bool, err error) {
    // passphrase = getEnv(alias)

    envVariable := strings.ToUpper(envPrefix + alias)

    passwordStore := NewPasswordStore(protectType, envVariable)
    passphrase, err := passwordStore.getPassword()

    if passphrase == "" || err != nil {
        return "", false, errors.New("expected env variable to not be empty: " + alias)
    }

    return passphrase, false, nil
}

The config will look like this:

"storage": {
        "backend": "mysql",
        "db_url": "user:pass@tcp(notarymysql:3306)/databasename?parseTime=true",
        "default_alias": "",
        "passphrase_protect_method" : "<NIL/HSM/VAULT/other future methods>"
}

[rhonnava]

@endophage Please let me know if something like this would add value

[endophage]

Conceptually I think all the maintainers would agree to this improvement. I'd like broader feedback on the design before you start work on anything.

The things that jump out at me in the proposed design:

• We should be able to break up password retrieval from password encryption. The current design appears to make assumptions that, for example, a password in vault will also be encrypted with the HSM. This may not be true. I would anticipate two types implementing the same interface where one knows where to read the password from (i.e. the environment), and the other knows how to unlock it (i.e. decrypt it with the HSM). These can then be arbitrarily composed based on a specific use case.
• getPassword appears to limit a given store to a single password. This would break our current models and is very inflexible. The current notary signer code permits the rotation of the password and implements re-encryption of keys on access with a new password if a rotation has happened. It supports an arbitrary number of historical passwords.
• In relation to the comments on getPassword, the setPassword function should also take an alias. This is the name by which the password will be referenced, it may be displayed to the user in the case of the CLI to remind them which password they need (i.e. the "root" key password, vs the "targets" key password).
• I'm skeptical that a single constructor will work consistently for all the stores. An HSM requires very different configuration parameters to Vault. I'd recommend not trying to bundle all the type instantiation into a single constructor function. Have individual constructors and let the high level app configuration determine which one to use, with which parameters, and then inject the instantiated type where required.

cc @cyli @riyazdf @HuKeping @ecordell

[ecordell]

I'd recommend not trying to bundle all the type instantiation into a single constructor function.

Agreed, different HSMs may have fairly diverging config. This actually seems like a good place to try out plugins - users with unusual key storage won't be blocked on notary PRs, and notary won't be responsible for supporting all possible backends.

[endophage]

@ecordell I'd be good with plugins for this, though we're holding on updating to Go 1.8 until docker/docker gets there (and they're holding off until some bugs are fixed in Go 1.8.1 apparently). We should have the 2 existing use cases (CLI stdin and env vars) built in and that should help validate the design/interface.

[rhonnava]

@endophage @ecordell Thanks for the feedback. Will update the interface based on these.

Wanted confirmation on one thing. If plugins is the direction to go towards for this feature, then given that plugins are supported only Golang 1.8 onwards, with this change, Notary will not be buildable in Go 1.7?

[endophage]

That is correct, once we update to Go 1.8 (which we're holding on until Go 1.8.1), Notary will not be build-able with Go 1.7.

For the moment, it should be possible to progress with the interface and built in use cases (CLI stdin, and env vars). When we move to Go 1.8.1 the plugin support will simply be another implementer of the interface.

[rhonnava]

@ecordell @endophage I have updated as follows in the interface below, based on your feedback

• Split up the interface into two cohesive parts: Password protection and password storage.
• Taking in alias as a parameter to getPassword and setPassword functions. The alias says which key password to set and get from the password store.
• Only the the default password store will be implemented for now. Will implement the plugin loaders that load plugins for PasswordStore and PasswordProtector interfaces once Notary is updated to Go 1.8.1.
• Storage config will have two new fields, passphrase_protect_method and passphrase_store_method which can have only two options for each of them: DEFAULT/PLUGIN. It will not have any mention of HSM/Vault/Any other mechanism in the code or config of Notary. These specifics will be left to the discretion of the users who want to use the external storage/protection mechanism at runtime.
• As each HSM/Vault/other external mechanism can have fairly divergent configurations, leaving details of the config to the plugin. loadConfig API called on the plugin will load the configuration in an implementation within the plugin.

/************************************** immediate code change **************************/

type PasswordStore interface {
    getPassword(alias string) (string, error)
    setPassword(alias string, newPassword string) error
}

type PasswordProtector interface {
    encrypt(clearText string) (string, error)
    decrypt(cipherText string) (string, error)
}

func NewDefaultPasswordStore() PasswordStore {
    return DefaultPasswordStore{}
}

type DefaultPasswordStore struct {
}

//The default password store just stores the clear password in the ENV variable as is (like the current code)
func (pw DefaultPasswordStore) setPassword(alias string, newPassword string) error {
    envVariable := strings.ToUpper(envPrefix + alias)
    error := os.Setenv(envVariable, newPassword)

    return error
}

//The default password store just retrieves the clear password from the ENV variable as is
func (pw DefaultPasswordStore) getPassword(alias string) (string, error) {
    envVariable := strings.ToUpper(envPrefix + alias)
    return os.Getenv(envVariable), nil
}

/************************************** future code with plugin support **************************/

type PluginPasswordStore struct {
    p Plugin
}

func NewPluginPasswordStore() PasswordStore {
        return PluginPasswordStore{
            p, error := plugin.Open("plugin .so path and name")
            // Option 1: Plugin will load its configuration in an init() function which will be invoked on plugin.Open
            // Option 2: In the plugin interface we specify a loadPlugin function which will be called after plugin.Open. The plugin implements this method with details that relate to the plugin.
    }
}

func (pw PluginPasswordStore) setPassword(alias string, newPassword string) error {
    //Call setPassword on the plugin
}

func (pw PluginPasswordStore) getPassword(alias string) (string, error) {
    //Call getPassword on the plugin
}

type PluginPasswordProtector struct {
    p Plugin
}

func NewPluginPasswordProtector() PasswordProtector {
        return PluginPasswordProtector{
            p, _ := plugin.Open("plugin .so path and name")
            // Option 1: Plugin will load its configuration in an init() function which will be invoked on plugin.Open
            // Option 2: In the plugin interface we specify a loadPlugin function which will be called in after plugin.Open. The plugin implements this method for details that relate to the plugin.
    }
}

func (pp PluginPasswordProtector) encrypt(clearText string) (string, error) {
    //Call the encrypt function on the plugin
}

func (pp PluginPasswordProtector) decrypt(cipherText string) (string, error) {
    //Call the decrypt function on the plugin
}

[endophage]

We're going to be pretty busy up to and through DockerCon so I apologize if this doesn't get the response it deserves for a couple of weeks. We will however be having a maintainers meetup at the conference which will be a great time for the maintainers to discuss this and give you a clear "yes" or a final few suggestions.

[rhonnava]

@endophage Thanks for this update

[rhonnava]

@endophage Please let me know if this was discussed during DcokerCon and if there was any decision on it?

[rhonnava]

@ecordell @endophage Any feedback on the above interface? Would it be better to go over this on a pull request?

[endophage]

Hey, sorry for the delayed response. I think the immediate proposed changes are good to build and PR. I think the future stuff probably needs a little experimentation as we look at how Go plugins are best integrated (they're still new to all of us :-)