Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for accepting cloud-provider credentials as EnvVar's #123

Closed
mrIncompetent opened this issue Mar 1, 2018 · 12 comments · Fixed by #153
Closed

Add support for accepting cloud-provider credentials as EnvVar's #123

mrIncompetent opened this issue Mar 1, 2018 · 12 comments · Fixed by #153
Assignees
Labels
priority/normal Not that urgent, but is important

Comments

@mrIncompetent
Copy link
Contributor

mrIncompetent commented Mar 1, 2018

The Machine object accepts multiple sources for cloudProviderSpec fields:

  • Direct value
...
spec:
...
  providerConfig:
    cloudProvider: "aws"
    cloudProviderSpec:
      accessKeyId: "foo"
  • Secret ref
...
spec:
...
  providerConfig:
    cloudProvider: "aws"
    cloudProviderSpec:
      accessKeyId:
        secretKeyRef:
          namespace: kube-system
          name: machine-controller-aws
          key: accessKeyId
  • ConfigMap ref
...
spec:
...
  providerConfig:
    cloudProvider: "aws"
    cloudProviderSpec:
      accessKeyId:
        configMapKeyRef:
          namespace: kube-system
          name: machine-controller-aws
          key: accessKeyId

It should also be possible to pass in the secret values implicitly as environment variable.
The secret values differ from cloud provider.

  • AWS
    • Access Key ID
    • Secret Access Key
  • Hetzner
    • Token
  • Digitalocean
    • Token
  • OpenStack
    • Username
    • Password

Each secret field needs to have one specific environment key. Like AWS_ACCESS_KEY_ID.
During the processing of the cloudProviderSpec we would need to check if the environment variable is set, and if so we need to use this value.

Reason: In scenarios where the master components is managed by an external entity (Loodse kubermatic/ SAP Gardener) it might not be possible to expose the cloud provider specific secrets to the users.

@mrIncompetent mrIncompetent added the priority/normal Not that urgent, but is important label Mar 1, 2018
@alvaroaleman
Copy link
Contributor

alvaroaleman commented Mar 5, 2018

Wouldn't the appropriate fix for that scenario be to put the credentials into a secret that is exposed to the machine-controller but not the users?

@mrIncompetent
Copy link
Contributor Author

This would mean the end user would get a admin account with access to everything except that secret.
That would be quite complicated to solve in Kubernetes. Also it would be complicated to maintain that set of permissions

@alvaroaleman
Copy link
Contributor

That is not complicated at all, the user gets an account that has a clusterrole with * on /machine.k8s.io/machines. He can't be admin anyways because then he could just get the secret from the machine-controller deployment or pod.

And that is the exact way Kubernetes assumes to solve this kind of problems:

Putting this information in a secret is safer and more flexible than putting it verbatim in a pod definition or in a docker image

Ref: https://kubernetes.io/docs/concepts/configuration/secret/

@mrIncompetent
Copy link
Contributor Author

In scenarios like Loodse's kubermatic or SAP's gardener (and even RedHats OpenShift Operator), the user has no access to the control plane.
Therefore the user has no access to the deployment of the machine-controller.

@alvaroaleman
Copy link
Contributor

The only thing we should probably change is to add events and/or conditions to machines whenever we do something

@alvaroaleman
Copy link
Contributor

In scenarios like Loodse's kubermatic or SAP's gardener (and even RedHats OpenShift Operator), the user has no access to the control plane.
Therefore the user has no access to the deployment of the machine-controller.

Ah. So this is primarily about storing credentials in a different cluster than the one that contains the machines, not about getting them from the environment

@mrIncompetent
Copy link
Contributor Author

Its about the machine-controller running in a different environment (Kubernetes or even Systemd for example).

@alvaroaleman
Copy link
Contributor

I see. So how would you name these env vars? Does something like that look fine to you?

spec:
  cloudProvider: openstack
  cloudProviderSpec:
    password:
      machineControllerEnvRef: <name-of-the-env-var>

Just as a sidenote, this opens a potential attack vendor when someone configures a server he or she controls as api endpoint for the cloudprovider, then tries random machineControllerEnvRef values that could be configured in the machine-controller as credentials and logs that server-side.

We could probably lock that down by having a whitelist and/or only allowing a combination of credentials and api endpoint to be used that way in the relevant providers (just Openstack I think).

@mrIncompetent
Copy link
Contributor Author

I would not add this to the ConfigVarString.
I would rather add this functionality to the Resolver as a fallback.

The resolver accepts 3 parameters:

  • RawConfig
  • Config
  • Prefix (a string like openstack, aws, etc.)

Via Reflection we iterate over all fields of the RawConfig:

  • Secret ref? -> Resolve to Secret
  • ConfigMap ref -> Resolve to ConfigMap
  • Value set? -> Resolve to value
  • Nothing until here? Try Prefix_+name of the field

This way we then also could eliminate missing resolve code within the cloud providers.

Though this might be a bit non obvious? If its too magic, an implementation with something like machineControllerEnvRef should be fine as well.

@alvaroaleman
Copy link
Contributor

A global prefix for the resolver has two issues:

  • multi-tenancy does not work, you can only use one account per cloud provider and machine-controller
  • field name collisions are possible if more than one cloud provider has a field of a given name (e.G. token

The latter could be solved by adding the providerName to the spec, but I think the former can not be solved without using some value that is taken from the field.

Also we must be able to have constraints on field combinations, e.G. if you take user from the env you must also take password and api endpoint

@mrIncompetent
Copy link
Contributor Author

mrIncompetent commented Mar 5, 2018

A global prefix for the resolver has two issues:

  • multi-tenancy does not work, you can only use one account per cloud provider and machine-controller

True, preferable over risking security issues IMO

  • field name collisions are possible if more than one cloud provider has a field of a given name (e.G. token

When using the provider name as prefix, that should not occur.

But i haven't considered the downsides here completely.

It might be simple adding support for a second kube-client?
Though this would break support for machines in the customer cluster...

@mrIncompetent
Copy link
Contributor Author

mrIncompetent commented Apr 10, 2018

A possible way would be to extend the ConfigVarString & ConfigVarBool type to look like:

cloudProviderSpec:
  accessKeyId:
    staticRef:
      envVarName: "MACHINE_AWS_ACCESS_KEY_ID"

Though all env vars must begin with the prefix MACHINE_
Just to avoid leak credentials from the machine-controller pod itself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/normal Not that urgent, but is important
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants