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

MGMT-17267: add etcd encryption support #160

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mresvanis
Copy link
Member

@mresvanis mresvanis commented Aug 14, 2024

This PR adds OpenShift etcd encryption support for recert:

  • recert now auto-discovers the seed encryption config and reads OCP encrypted resources using those keys, during recertify and ocp_postprocess
    • supports AES-GCM (openssl doesn't support AES-GCM, so not FIPS compliant anyway, we'll use Rust libs)
    • supports AES-CBC (via openssl for FIPS compliance)
  • add cmd line args for target's {kube,openshift,oauth}-apiserver encryption configs to keep target's keys and encrypt all resources with those before committing to etcd
  • when seed encryption is enabled and the {kube,openshift,oauth}-encryption-config cmd line args have not been provided, generate new ones, encrypt target's resources and put them in etcd and in the filesystem
  • ensure no control-plane rollouts occur after recert

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Aug 14, 2024

@mresvanis: This pull request references MGMT-17267 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

This PR adds OpenShift etcd encryption support for recert:

  • enable recert to read/edit/write OCP encrypted resources using the current encryption keys, during recertify and ocp_postprocess
    • AES-GCM
    • AES-CBC
  • rotate the encryption keys
  • encrypt all resources with the newly created encryption keys before committing to etcd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Copy link

openshift-ci bot commented Aug 14, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@mresvanis mresvanis requested a review from omertuc August 14, 2024 15:36
Copy link

openshift-ci bot commented Aug 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mresvanis

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

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Aug 16, 2024

@mresvanis: This pull request references MGMT-17267 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

This PR adds OpenShift etcd encryption support for recert:

  • enable recert to read/edit/write OCP encrypted resources using the current encryption keys, during recertify and ocp_postprocess
    • AES-GCM
    • AES-CBC
  • rotate the encryption keys
  • encrypt all resources with the newly created encryption keys before committing to etcd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@mresvanis mresvanis force-pushed the mgmt-17267-etcd-encryption branch 2 times, most recently from 815b53a to 82525ef Compare August 16, 2024 10:32
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Aug 16, 2024

@mresvanis: This pull request references MGMT-17267 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

This PR adds OpenShift etcd encryption support for recert:

  • enable recert to read/edit/write OCP encrypted resources using the current encryption keys, during recertify and ocp_postprocess
    • AES-GCM
    • AES-CBC
  • rotate the encryption keys
  • encrypt resources to be encrypted with the newly created encryption keys before committing to etcd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Aug 16, 2024

@mresvanis: This pull request references MGMT-17267 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

This PR adds OpenShift etcd encryption support for recert:

  • enable recert to read/edit/write OCP encrypted resources using the current encryption keys, during recertify and ocp_postprocess
    • AES-GCM
    • AES-CBC
  • rotate the encryption keys
  • encrypt resources to be encrypted with the newly created encryption keys before committing to etcd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Copy link
Member

@omertuc omertuc left a comment

Choose a reason for hiding this comment

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

Very cool, mostly nits and one very annoying request - to switch to openssl and drop the dependencies, for FIPS reasons

Cargo.toml Show resolved Hide resolved
src/encrypt.rs Outdated Show resolved Hide resolved
src/encrypt.rs Outdated Show resolved Hide resolved
src/encrypt.rs Outdated Show resolved Hide resolved
src/recert.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/recert.rs Outdated Show resolved Hide resolved
src/recert.rs Outdated Show resolved Hide resolved
src/recert.rs Outdated Show resolved Hide resolved
@omertuc
Copy link
Member

omertuc commented Aug 16, 2024

Is there any reason why we ask the user to provide us with the encryption config for kube-apiserver while we fetch the encryption config for openshift-apiserver and the oauth-apiserver on our own? What stops us from simply fetching the encryption config for kube-apiserver, instead of asking the user to provide us with this config?

@mresvanis
Copy link
Member Author

What stops us from simply fetching the encryption config for kube-apiserver, instead of asking the user to provide us with this config?

The openshift-kube-apiserver/encryption-config is a Secret, which is encrypted. That leads us to the filesystem encryption-config file (under /etc/kubernetes/static-pod-resources/kube-apiserver-pod-<latest>/secrets/encryption-config/encryption-config) to fetch all the encryption details for the kube-apiserver encrypted resources (i.e. Secrets and ConfigMaps) from. Because it's a file and we're using recert mostly in a container, where we mount everything we need from the filesystem, I thought it best to have a path argument (which I will refactor to also accept the content of that file, as suggested here). Does this make sense?

@omertuc
Copy link
Member

omertuc commented Aug 16, 2024

What stops us from simply fetching the encryption config for kube-apiserver, instead of asking the user to provide us with this config?

The openshift-kube-apiserver/encryption-config is a Secret, which is encrypted. That leads us to the filesystem encryption-config file (under /etc/kubernetes/static-pod-resources/kube-apiserver-pod-<latest>/secrets/encryption-config/encryption-config) to fetch all the encryption details for the kube-apiserver encrypted resources (i.e. Secrets and ConfigMaps) from. Because it's a file and we're using recert mostly in a container, where we mount everything we need from the filesystem, I thought it best to have a path argument (which I will refactor to also accept the content of that file, as suggested here). Does this make sense?

Yep, that makes perfect sense. Could you please explain that in a (non-documenting //) comment above the encryption_config parameter?

And add a reference to the /etc/kubernetes/static-pod-resources/kube-apiserver-pod-<latest>/secrets/encryption-config/encryption-config in the yes-documenting (///) part of the parameter comment

@mresvanis mresvanis force-pushed the mgmt-17267-etcd-encryption branch 6 times, most recently from 0337506 to 650c511 Compare August 19, 2024 08:32
@mresvanis mresvanis force-pushed the mgmt-17267-etcd-encryption branch 2 times, most recently from 4cad28b to a720d72 Compare September 13, 2024 18:04
src/config/cli.rs Outdated Show resolved Hide resolved
@mresvanis mresvanis force-pushed the mgmt-17267-etcd-encryption branch 2 times, most recently from 4f7d329 to bfea2bb Compare September 25, 2024 13:27
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Sep 26, 2024

@mresvanis: This pull request references MGMT-17267 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

This PR adds OpenShift etcd encryption support for recert:

  • enable recert to read/edit/write OCP encrypted resources using the current encryption keys, during recertify and ocp_postprocess
    • AES-GCM (openssl doesn't support AES-GCM, so not FIPS compliant anyway, we'll use Rust libs)
    • AES-CBC (via openssl for FIPS compliance)
  • use seed encryption config to decrypt resources
  • use target encryption config to encrypt resources before committing to etcd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Sep 26, 2024

@mresvanis: This pull request references MGMT-17267 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

This PR adds OpenShift etcd encryption support for recert:

  • enable recert to read/edit/write OCP encrypted resources using the current encryption keys, during recertify and ocp_postprocess
    • AES-GCM (openssl doesn't support AES-GCM, so not FIPS compliant anyway, we'll use Rust libs)
    • AES-CBC (via openssl for FIPS compliance)
  • use seed encryption config to decrypt resources during cert regeneration and OCP post-processing
  • use target encryption config to encrypt resources before committing to etcd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Oct 17, 2024

@mresvanis: This pull request references MGMT-17267 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

This PR adds OpenShift etcd encryption support for recert:

  • recert now auto-discovers the seed encryption config and reads OCP encrypted resources using those keys, during recertify and ocp_postprocess
    • supports AES-GCM (openssl doesn't support AES-GCM, so not FIPS compliant anyway, we'll use Rust libs)
    • supports AES-CBC (via openssl for FIPS compliance)
  • add cmd line args for target's {kube,openshift,oauth}-apiserver encryption configs to keep target's keys and encrypt all resources with those before committing to etcd
  • when seed encryption is enabled and the {kube,openshift,oauth}-encryption-config cmd line args have not been provided, generate new ones, encrypt target's resources and put them in etcd and in the filesystem
  • ensure no control-plane rollouts occur after recert

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@mresvanis
Copy link
Member Author

/test e2e-aws-ovn-single-node-recert-parallel

Copy link
Member

@omertuc omertuc left a comment

Choose a reason for hiding this comment

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

Amazing work, mostly superficial comments

src/encrypt/aescbc.rs Outdated Show resolved Hide resolved
src/encrypt/aescbc.rs Outdated Show resolved Hide resolved
src/encrypt/aescbc.rs Outdated Show resolved Hide resolved
src/encrypt/aescbc.rs Outdated Show resolved Hide resolved
src/encrypt/aescbc.rs Outdated Show resolved Hide resolved
src/k8s_etcd.rs Outdated Show resolved Hide resolved
src/k8s_etcd.rs Outdated Show resolved Hide resolved
src/recert.rs Outdated Show resolved Hide resolved
src/recert.rs Outdated Show resolved Hide resolved
src/recert.rs Outdated Show resolved Hide resolved
@mresvanis mresvanis force-pushed the mgmt-17267-etcd-encryption branch 5 times, most recently from e6a5cbf to 49bccae Compare November 13, 2024 12:27
@mresvanis
Copy link
Member Author

/test baremetalds-sno-recert-cluster-rename

Copy link
Member

@omertuc omertuc left a comment

Choose a reason for hiding this comment

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

Thanks, small change

src/encrypt.rs Show resolved Hide resolved
@mresvanis mresvanis force-pushed the mgmt-17267-etcd-encryption branch 3 times, most recently from cc3c1ec to e2fe947 Compare November 18, 2024 10:15
src/encrypt.rs Outdated Show resolved Hide resolved
@omertuc
Copy link
Member

omertuc commented Nov 23, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 23, 2024
@mresvanis
Copy link
Member Author

/test e2e-aws-ovn-single-node-recert-parallel

@mresvanis
Copy link
Member Author

@omertuc I'm not yet sure about what's happening with the conformance tests, but I believe it's related to OCP 4.18 and not this PR.

@omertuc
Copy link
Member

omertuc commented Nov 25, 2024

Compare with #168

@mresvanis
Copy link
Member Author

mresvanis commented Nov 26, 2024

Compare with #168

We have almost the same results, PR 168:

# pull-ci-rh-ecosystem-edge-recert-main-e2e-aws-ovn-single-node-recert-parallel
: [sig-architecture] platform pods in ns/openshift-kube-apiserver should not exit an excessive amount of times expand_more	0s
: [sig-architecture] platform pods in ns/openshift-kube-controller-manager should not exit an excessive amount of times expand_more	0s
: [sig-architecture] platform pods in ns/openshift-kube-scheduler should not exit an excessive amount of times expand_more	0s
: [sig-node] static pods should start after being created expand_more	0s
: [sig-api-machinery][Feature:APIServer][Late] kubelet terminates kube-apiserver gracefully extended [Suite:openshift/conformance/parallel] expand_more

vs this PR 160

# pull-ci-rh-ecosystem-edge-recert-main-e2e-aws-ovn-single-node-recert-parallel
: [sig-architecture] platform pods in ns/openshift-kube-apiserver should not exit an excessive amount of times expand_more	0s
: [sig-architecture] platform pods in ns/openshift-kube-controller-manager should not exit an excessive amount of times expand_more	0s
: [sig-architecture] platform pods in ns/openshift-kube-scheduler should not exit an excessive amount of times expand_more	0s
: [sig-api-machinery][Feature:APIServer][Late] kubelet terminates kube-apiserver gracefully extended [Suite:openshift/conformance/parallel] expand_more

I'm now retesting the serial conformance tests.

@mresvanis
Copy link
Member Author

/test e2e-aws-ovn-single-node-recert-serial

Copy link

openshift-ci bot commented Nov 27, 2024

@mresvanis: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ovn-single-node-recert-parallel a03c0db link true /test e2e-aws-ovn-single-node-recert-parallel
ci/prow/e2e-aws-ovn-single-node-recert-serial a03c0db link true /test e2e-aws-ovn-single-node-recert-serial

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants