Skip to content
New issue

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

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

Already on GitHub? # to your account

Feature: Configuring rest components for TLS #622

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Johnaius
Copy link

@Johnaius Johnaius commented Feb 5, 2025

This pull request introduces the configuration of certificates for TLS in REST
before enabling TLS, changes to control plane will have to be pulled in as well. I will link PR here

Description

REST API Component:

  • TLS Configuration:
  1. Added support for enabling TLS by configuring certificate paths and volume mounts.
  2. Removed the --dummy-certificates argument when TLS is enabled.
  3. Introduced new command-line arguments for specifying certificate and key files.
  4. Included configuration to mount the CA certificate for client verification with core agent (commented out for now).

CSI and Diskpool Operator:

Added support for reading CA certificate and configuring TLS for HTTPS endpoints.
Introduced new command-line arguments for specifying the CA certificate file path.

Values.yaml:

Added enable TLS switch. And configured components to only load tls info when enabled.

Certs/Secrets :

Introduced a script to create certificates and a Kubernetes Secret for testing purposes.
Further discussion needed on how certificate deployment will be handled (e.g., using cert-manager or manual setup).

Motivation and Context

These changes enhance the security of the REST API, CSI, and Diskpool Operator components by enabling TLS communication, ensuring secure data transmission

How Has This Been Tested?

I have deployed these changes along with the changes I implemented in the control plane to an AKS cluster. I need some direction on how to appropriately test.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added unit tests to cover my changes.

Copy link
Contributor

@tiagolobocastro tiagolobocastro left a comment

Choose a reason for hiding this comment

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

I think call-home also needs to have the certificate?
Also, the kubectl plugin?

@@ -0,0 +1,4163 @@
# Source: cert-manager/templates/templates.out
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to apply the CRDs? Are they not installed via cert-manager itself?
Handling CRDs via helm has been painful in the past

Copy link
Author

Choose a reason for hiding this comment

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

Do you remember what the pain points were and why? I'll look into it, cert-manager installs fine without any certificate or issuer etc. resources being created. But when attempting to deploy with certs in the chart I was seeing errors where these resources were attempting to be created before the CRDs were installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think mostly to do with updates of the CRD. CC @niladrih and @Abhinandan-Purkait

But when attempting to deploy with certs in the chart I was seeing errors where these resources were attempting to be created before the CRDs were installed.

Yeah, makes sense..
What were the resources being created? Is it something we can configure to be done by the pods rather than via helm?

Copy link
Author

@Johnaius Johnaius Feb 11, 2025

Choose a reason for hiding this comment

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

I'm trying to create issuer and certificate resources. We will create a root cert file and seperate files for each cert. I created a new branch here where i'm working on getting cert-manager installed as a subchart as well as certificates. You can see the server-root-cert and rest api cert here

Comment on lines +4 to +5
NAMESPACE="openebs"
APP_NAME="api-rest"
Copy link
Contributor

Choose a reason for hiding this comment

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

This would have to be configurable?

Copy link
Author

Choose a reason for hiding this comment

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

for clarification, I created this cert file for testing purposes only until we get cert-manager installed... The plan is to not rely on this script and to use cert-manager to create all the certs. I am open to discuss and direction on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, though might be worth keeping for devel

# Create a self-signed root CA
echo "Creating a self-signed root CA"
openssl genrsa -out "${CERT_DIR}/ca.key" 4096
openssl req -x509 -new -nodes -key "${CERT_DIR}/ca.key" -sha256 -days 3650 -out "${CERT_DIR}/ca.crt" -subj "/CN=api-rest-ca" -addext "subjectAltName=DNS:${NAMESPACE}-${APP_NAME}-${NAMESPACE}.svc.cluster.local,DNS:${NAMESPACE}-${APP_NAME},DNS:${NAMESPACE}-${APP_NAME}-${NAMESPACE}.svc"
Copy link
Contributor

Choose a reason for hiding this comment

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

cluster.local needs to be configurable?

echo "Creating a TLS certificate for the API REST"
openssl genrsa -out "${CERT_DIR}/server.key" 4096
openssl req -new -key "${CERT_DIR}/server.key" -out "${CERT_DIR}/server.csr" -subj "/CN=${NAMESPACE}-${APP_NAME}" -addext "subjectAltName=DNS:${NAMESPACE}-${APP_NAME}-${NAMESPACE}.svc.cluster.local,DNS:${NAMESPACE}-${APP_NAME},DNS:${NAMESPACE}-${APP_NAME}-${NAMESPACE}.svc"
openssl x509 -req -in "${CERT_DIR}/server.csr" -CA "${CERT_DIR}/ca.crt" -CAkey "${CERT_DIR}/ca.key" -CAcreateserial -out "${CERT_DIR}/server.crt" -days 3650 -sha256 -extfile <(printf "subjectAltName=DNS:${NAMESPACE}-${APP_NAME}-${NAMESPACE}.svc.cluster.local,DNS:${NAMESPACE}-${APP_NAME},DNS:${NAMESPACE}-${APP_NAME}-${NAMESPACE}.svc")
Copy link
Member

Choose a reason for hiding this comment

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

Should the time period be configurable?

@@ -0,0 +1,4163 @@
# Source: cert-manager/templates/templates.out
Copy link
Contributor

Choose a reason for hiding this comment

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

I think mostly to do with updates of the CRD. CC @niladrih and @Abhinandan-Purkait

But when attempting to deploy with certs in the chart I was seeing errors where these resources were attempting to be created before the CRDs were installed.

Yeah, makes sense..
What were the resources being created? Is it something we can configure to be done by the pods rather than via helm?

@Johnaius Johnaius requested review from a team as code owners February 13, 2025 23:25
@Johnaius
Copy link
Author

I think call-home also needs to have the certificate? Also, the kubectl plugin?

I just added configurations for callhome. Not sure how I can get the certs to the kubectl plugin on first glance, I see this but where are the args being passed in?

@Johnaius
Copy link
Author

Looks like csi-node will need to be configured as well

@tiagolobocastro
Copy link
Contributor

I think call-home also needs to have the certificate? Also, the kubectl plugin?

I just added configurations for callhome. Not sure how I can get the certs to the kubectl plugin on first glance, I see this but where are the args being passed in?

That's using a separate crate as it's going via the apiserver.
We have two crates for this, kube-forward and kube-proxy, here.

@Johnaius
Copy link
Author

Also I had a heck of a time getting cert-manager up and running. I hacked together a job to create certs that runs post install which can be seen here. The issue was that certificates were trying to be created before CRD's were installed and before the cert-manager-webhook container was created. (from the few times I observed it, it took around 90s). I feel like there's a better way and wasn't sure if my hack is acceptable. I asked for advice on the cert-manager slack hopefully that will be fruitful.

Signed-off-by: John Zakrzewski <Jozakrzewski@microsoft.com>

configure tls for REST

Signed-off-by: John Zakrzewski <Jozakrzewski@microsoft.com>

update script to include DNS names

Signed-off-by: John Zakrzewski <Jozakrzewski@microsoft.com>

disable tls in values.yaml, remove cert-manager

Signed-off-by: John Zakrzewski <Jozakrzewski@microsoft.com>

adding readme

Signed-off-by: John Zakrzewski <Jozakrzewski@microsoft.com>

feat: cert-manager-install

Signed-off-by: John Zakrzewski <Jozakrzewski@microsoft.com>

feat: configure callhome for tls with restapi

Signed-off-by: John Zakrzewski <Jozakrzewski@microsoft.com>

chore: configuring cert ingestion for csi-node daemonset

chore: add certificate job, clean up to enable tls successfully
@Johnaius
Copy link
Author

Johnaius commented Feb 18, 2025

I decided to include my cert-manager install hack here, it works... This seems to be a known issue when installing as a subchart with not much resolution. Would it be better to encourage users to install cert-manager manually as part of enabling tls if they don't already have it installed on cluster? Is there a way to check if cert-manager is installed in cluster and install it if not if tls is enabled? Without my post job hack, this is the error Error: INSTALLATION FAILED: unable to build kubernetes objects from release manifest: [resource mapping not found for name: "root-ca" namespace: "openebs" from "": no matches for kind "Certificate" in version "cert-manager.io/v1" ensure CRDs are installed first, resource mapping not found for name: "rest-api-server" namespace: "openebs" from "": no matches for kind "Certificate" in version "cert-manager.io/v1" ensure CRDs are installed first, resource mapping not found for name: "ca-issuer" namespace: "openebs" from "": no matches for kind "Issuer" in version "cert-manager.io/v1" ensure CRDs are installed first] helm.go:86: 2025-02-18 16:42:13.619842851 -0500 EST m=+9.246526924 [debug] [resource mapping not found for name: "root-ca" namespace: "openebs" from "": no matches for kind "Certificate" in version "cert-manager.io/v1" ensure CRDs are installed first, resource mapping not found for name: "rest-api-server" namespace: "openebs" from "": no matches for kind "Certificate" in version "cert-manager.io/v1" ensure CRDs are installed first, resource mapping not found for name: "ca-issuer" namespace: "openebs" from "": no matches for kind "Issuer" in version "cert-manager.io/v1" ensure CRDs are installed first] you can view some discussion on cert-manager slack here and helm slack here

bors-openebs-mayastor bot pushed a commit to openebs/mayastor-control-plane that referenced this pull request Feb 19, 2025
927: Feature: Configuring rest components for TLS r=tiagolobocastro a=Johnaius

This PR configures components using rest protocols to speak tls.  It is dependent on [this PR in extensions repo](openebs/mayastor-extensions#622). 

### Rest-api Clients 
- Modified certificate loading to use pkcs8_private_keys instead of rsa_private_keys.  This was a change I made to get the generated certificates to work, they were apparently created in pkcs8 format, not rsa - I attempted to convert them with no luck, and found the pkcs8_private_keys in the [rustl-pemfile crate](https://docs.rs/rustls-pemfile/latest/rustls_pemfile/)...
### Csi-controller and Diskpool operator:
- Added support for TLS configuration based on CA certificate path.
- if certs are provided, use https, if not use http.
-  error handling for HTTPS connections without a certificate etc




Co-authored-by: John Zakrzewski <Jozakrzewski@microsoft.com>
@tiagolobocastro
Copy link
Contributor

Would a pre-install hook help?
Otherwise, maybe it should just be a separate prerequisite that users have to install when enabling tls.
@niladrih @Abhinandan-Purkait ?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants