libopenstorage / secrets

Openstorage support for Key Management Systems
Apache License 2.0
16 stars 16 forks source link

configureTLS() does not handle map[string]interface{} that contains a boolean #51

Open bnevis-i opened 3 years ago

bnevis-i commented 3 years ago

The InitVaultKMS() function in vault/vault.go takes a map. One of the parameters that it can take is vaultCAVerify: (string-bool).

Example:

    kmsID := "vault-kms"
    config := map[string]interface{}{
        "vaultCAVerify":       "false",
    }
    secrets := map[string]string{}

    kms, err := InitVaultKMS(kmsID, config, secrets)

An upstream package, github.com/ceph/ceph-csi expects this value to be a string and returns the error failed to initialize Vault connection: configuration option not valid: expected string for "vaultCAVerify", but got bool if a boolean is supplied. The upstream project parses the string into a boolean and then inverts it, passing a bool to InitVaultKMS. (This can be considered its own bug and I will also file one there.)

The first few lines of configureTLS read:

func configureTLS(config *api.Config, secretConfig map[string]interface{}) error {
    tlsConfig := api.TLSConfig{}
    skipVerify := getVaultParam(secretConfig, api.EnvVaultInsecure)
    if skipVerify != "" {
        insecure, err := strconv.ParseBool(skipVerify)
        if err != nil {
            return ErrInvalidSkipVerify
        }
        tlsConfig.Insecure = insecure
    }

Clearly, getVaultParam() is expected to return a string, regardless of the type in secretConfig, which is declared as map[string]interface{}. A few lines later, ParseBool() is used to parse the value. In the failure scenario, skipVerify is returned as neither "true" nor "false" but the empty string.

This is caused by the following error in getVaultParam()

        tokenStr, ok := tokenIntf.(string)
        if !ok {
            return ""
        }

The cast to string does not stringize a boolean, but simply fails the cast.

Suggested remedy is to attempt cast to bool, and run strconv.FormatBool to return a string boolean.

adityadani commented 3 years ago

@bnevis-i Thanks for reporting this issue.

The getVaultParam() function is a misnomer since it always returns a string and could possibly be renamed to getVaultParamString(). For the api.EnvVaultInsecure parameter, I would suggest not using this function and instead directly handle the bool/string value.

Feel free to send out a PR to address this issue or we will fix it in a few days.