Azure / kubernetes-keyvault-flexvol

Azure keyvault integration with Kubernetes via a Flex Volume
MIT License
253 stars 84 forks source link

Why do we need service principal to have RBAC-reader on vault? #78

Closed vplauzon closed 4 years ago

vplauzon commented 5 years ago

Using the Key Vault REST API, we just need the service principal to have read (secrets / keys / certificates) policy in the vault.

We do not need to have Azure Control Plane / RBAC Reader on the Key Vault itself.

So why do we need to give that privilege? Principle of Least Privilege would dictate to just have the policy, no?

ritazh commented 5 years ago

@vplauzon You are referring to this step right? az role assignment create --role Reader --assignee <principalid> --scope /subscriptions/<subscriptionid>/resourcegroups/<resourcegroup>/providers/Microsoft.KeyVault/vaults/<keyvaultname>

This is required by https://github.com/Azure/kubernetes-keyvault-flexvol/blob/master/azurekeyvault-flexvolume/keyvaultFlexvolumeAdapter.go#L145-L146 to get and validate the keyvault client.

vplauzon commented 5 years ago

I see. This allows us to validate the Key Vault does exist I suppose.

The Key Vault API doesn't require RBAC access. It is a different plane.

For instance, in this blog, I use the Key Vault REST API while having only policies access. If you run the sample (ARM template), you'll see the Logic App Managed Identity isn't given any access on the Key Vault resource itself.

It is a minor inconvenience, but one I did step into. It adds to the number of mistakes one can do when configuring the feature as I did.

ritazh commented 5 years ago

That's right, we need to first validate the key vault exists before getting content from it. Hence we need access in both places. Closing this issue for now. Feel free to reopen if you have other questions.

infogulch commented 5 years ago

we need to first validate the key vault exists before getting content from it

Why is this a requirement? You can validate the provided Keyvault name meets the officially documented name restrictions (specifically, Key Vault names are selected by the user and are globally unique. and Key Vault name must be a 3-24 character string, containing only 0-9, a-z, A-Z, and -.), and if it does just try to connect. You don't need to be able to 'see' it to attempt to access it. The act of connecting validates that the bearer token represents a user that is allowed to connect to that keyvault.

The reason why this matters is because many azure governance strategies restrict the ability for random users to assign arbitrary RBAC roles to other random users. In this kind of environment, "just add the RBAC rule" is an extremely onerous thing to ask of someone that doesn't have full access to control RBAC.

Especially in the case where you're using Pod Identity, and new PI-"users" come in and out of existence according to the needs of the cluster. This means that the only thing that a cluster developer would need is the ability to deploy Pod Identity services to the cluster, and have Contributor role on the keyvault. The Pod Identity itself wouldn't need any RBAC assignments.

ritazh commented 5 years ago

Thanks for the feedback @infogulch! Regarding

many azure governance strategies restrict the ability for random users to assign arbitrary RBAC roles to other random users.

What about assigning permission to the key vault objects? Are these also restricted? e.g.:

az keyvault set-policy -n $KV_NAME --key-permissions get --spn <YOUR SPN CLIENT ID>
infogulch commented 5 years ago

Setting access policies in an azure keyvault only requires the Contributor role on the scope of the keyvault.

Whereas assigning an RBAC Reader role on the keyvault to another user (e.g. giving the PI-user Reader role) requires Owner permission on the scope.

Conceptually, keyvault is authenticated against your AAD tenant, but it's authorized based on the access policy, not RBAC.

infogulch commented 5 years ago

Based on quickly scanning the repo, it appears that the only thing that needs to change to eliminate all need for users to alter RBAC is simplifying the keyvaultFlexvolumeAdapter.getVaultURL() method to return the vault endpoint based on the name after validating it conforms to the restrictions published in the documentation.

Something like this: (untested)

func (adapter *KeyvaultFlexvolumeAdapter) getVaultURL() (vaultURL *string, err error) {
    // See docs for validation spec: https://docs.microsoft.com/en-us/azure/key-vault/about-keys-secrets-and-certificates#objects-identifiers-and-versioning
    if match, _ := regexp.MatchString("[-a-zA-Z0-9]{3,24}", adapter.options.vaultName); !match {
        return nil, errors.Errorf("Invalid vault name: %q, must match [-a-zA-Z0-9]{3,24}")
    }
    // Note: The ".vault.azure.net" endpoint tld may depend on something else like 
    // adapter.options.cloudName.  e.g. govcloud may use a different endpoint.
    // I'm not familiar enough to know or not.
    return "https://"+adapter.options.vaultName+".vault.azure.net/", nil
}
timja commented 4 years ago

I've implemented a PR for this: https://github.com/Azure/kubernetes-keyvault-flexvol/pull/128