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 encryption providers proposal #1213

Merged
merged 4 commits into from
Feb 11, 2021

Conversation

moelsayed
Copy link
Contributor

@moelsayed moelsayed commented Jan 12, 2021

What this PR does / why we need it:
A proposal draft to add Encryption Providers support in KubeOne.
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1212

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@kubermatic-bot kubermatic-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 12, 2021
@kron4eg kron4eg self-assigned this Jan 12, 2021
@kubermatic-bot kubermatic-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2021
@kron4eg
Copy link
Member

kron4eg commented Jan 15, 2021

/lgtm
/assign @xmudrii

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2021
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b7ac88af2bee1fd2c1a40cdd0c3e142f4c387e6a

@kron4eg kron4eg removed the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2021
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2021
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b7ac88af2bee1fd2c1a40cdd0c3e142f4c387e6a

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2021
@kron4eg
Copy link
Member

kron4eg commented Jan 20, 2021

/hold

@kubermatic-bot kubermatic-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2021
kind: KubeOneCluster
features:
encryptionProviders:
enabled: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping in mind future API extensions that will be inevitably required, simple boolean flag is not enough. Flags are needed per provider type, and not only flags but possible config options too.

So we are looking at 3 possible providers:

  • none
  • static (the one for being implemented now)
  • kms (the one for being implemented later)

Those should be expressed as additional objects inside encryptionProviders, like

#### VERY DRAFT, MUCH FANTASY ####
####### only for demo purpose #######
apiVersion: kubeone.io/v1beta1
kind: KubeOneCluster
features:
  encryptionProviders:
    - kind: none
    - kind: static
    - kind: kms
      endpoint: unix:///tmp/socketfile.sock
      cachesize: 100
      timeout: 3s

Alternatively, we could not reimplement parts of what's already done in upstream and "just" provide the

apiVersion: kubeone.io/v1beta1
kind: KubeOneCluster
features:
  encryptionConfiguration: |
    apiVersion: apiserver.config.k8s.io/v1
    kind: EncryptionConfiguration
    resources:
    - resources:
      - secrets
      providers:
      - kms:
          name: myKmsPlugin
          endpoint: unix:///tmp/socketfile.sock
          cachesize: 100
          timeout: 3s
      - identity: {}

Given that second option, we can actually extract the endpoint from KMS providers and automatically mount them to the kube-apiserver pod ( with extra mounts in kubeadm config).

Provided that we can detect changes in encryptionConfiguration (compare actual file to encryptionConfiguration`, we can automate and orchestrate kubeapi pod restarts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Providing ONLY encryptionConfiguration we could give all encryption options to the end user, including KMS support in one shot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with what @kron4eg said, but in that case, I'd leave the enable field in the API and implement it like @moelsayed initially proposed. For example:

features:
  encryptionProviders:
    # enable/disable 
    enable: true
    # setting this value will disable automated rotation.
    customProviderFile: |
      apiVersion: apiserver.config.k8s.io/v1
      kind: EncryptionConfiguration
      ...

The difference between the originally proposed API and this API is that I've removed the rotate field. Instead of the rotate field, we would keep the CLI flag as it's currently proposed.

  • If enable is set to false
    • Do nothing if the feature has never been enabled
    • Otherwise, disable the feature
  • If enable is set to true:
    • If customProviderFile is not set, generate it ourselves and handle key management
    • If customProviderFile is set, upload it to the instance. Additionally, if KMS is configured, create the appropriate volumes on the API server pods
    • Configure the API server and everything else as described in the proposal

Note: if customProviderFile is set and the CLI flag is set, KubeOne would either show a warning or fail.

In this case, we can quickly implement support for all setups. Technically, users could use Terraform/Ansible to deploy KMS and with such API, they could actually use it in their clusters. I think that it wouldn't require too much additional effort on our side, but we could solve everything (besides deploying KMS, which should be discussed) in a single run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kron4eg @xmudrii I updated the proposal accordingly. Please check.

For the initial implementation, I will stop at implementing the the custom config basic functionality. I will create a new issue/PR to address deploying the KMS and creating the required volume mounts for the API server. I am considering that out of scope for the initial implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moelsayed Can you create an Epic for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xmudrii there is already an epic to track the proposal and the implementation: #769

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moelsayed I was thinking about an Epic for addressing the KMS deployment and the volume creation. You can also extend the current Epic. I only see issues relevant to the initial implementation in the Epic, but please correct me if I'm wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if:

  • enable: true, but no customProviderFile is given (e.g. it's empty)?
  • enable: true, but no customProviderFile is given and install/apply is run, and then customProviderFile becomes defined?

Copy link
Contributor Author

@moelsayed moelsayed Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enable: true, but no customProviderFile is given (e.g. it's empty)?

We manage everything automatically. A new config file is created and pushed to the nodes. In this case key rotation is supported.

enable: true, but no customProviderFile is given and install/apply is run, and then customProviderFile becomes defined?

K1 should push the defined custom file but it will refuse to the rotate the keys if the rotate flag is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xmudrii I updated the epic with an issue to add KMS #1242.

@kron4eg kron4eg removed approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Jan 20, 2021
@kubermatic-bot kubermatic-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 4, 2021
@kubermatic-bot kubermatic-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 4, 2021
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve
/hold
if @kron4eg wants to take a look as well

kind: KubeOneCluster
features:
encryptionProviders:
enabled: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moelsayed Can you create an Epic for this?

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2021
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 7a63f9e46ff8cf0819386706364abef0ed49d909

Copy link
Member

@kron4eg kron4eg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@kron4eg
Copy link
Member

kron4eg commented Feb 11, 2021

/hold cancel

@kubermatic-bot kubermatic-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2021
@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kron4eg, moelsayed, xmudrii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubermatic-bot kubermatic-bot merged commit b9fa601 into kubermatic:master Feb 11, 2021
hwuethrich added a commit to hwuethrich/kubeone that referenced this pull request Aug 3, 2021
* upstream/master: (21 commits)
  Improve the installation script (kubermatic#1253)
  Update README.md (kubermatic#1250)
  Add the changelog for the v1.2.0-beta.1 release (kubermatic#1249)
  Fix credentials in addons (kubermatic#1248)
  fix(docs): fix broken master documentation link (kubermatic#1246)
  Add encryption providers proposal (kubermatic#1213)
  Use Docker for restarting API server on Flatcar (kubermatic#1245)
  Add the Kubernetes 1.20 jobs (kubermatic#1244)
  Restart unhealthy API servers when provisioning/upgrading clusters (kubermatic#1243)
  Add rsync on CentOS and Amazon Linux (kubermatic#1240)
  Update machine-controller to v1.25.0 (kubermatic#1238)
  Update the kubeone-e2e image (kubermatic#1239)
  Update KubeOne CI jobs (kubermatic#1237)
  Disallow and deprecate the PodPresets feature (kubermatic#1236)
  Fix confusing default in OpenIDConnect (kubermatic#1235)
  Add debian support (kubermatic#1233)
  Drop mounting flexvolume plugins into the openstack CCM (kubermatic#1234)
  Remove CoreOS (kubermatic#1232)
  Add the changelog for the v1.2.0-beta.0 release (kubermatic#1230)
  Add containerRuntime API to the full config (kubermatic#1229)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal for adding Encryption Providers support in KubeOne.
4 participants