From e5cd2f6ffb885767e28e764775f985acc08ccec2 Mon Sep 17 00:00:00 2001 From: IvanHunters Date: Fri, 24 Apr 2026 23:51:04 +0300 Subject: [PATCH] fix(etcd): resolve critical security and race condition issues Fix all critical and serious issues from code review: **Security fixes:** 1. **RBAC least privilege**: Added resourceNames restriction to Secret access. Role now permits access ONLY to etcd-ca-tls and etcd-client-tls Secrets, preventing potential exposure of unrelated secrets in namespace. 2. **Universal key validation**: Changed from openssl rsa to openssl pkey. Now supports ANY key type (RSA, ECDSA, Ed25519, DSA), not just RSA. Prevents validation failure if cert-manager generates non-RSA keys. 3. **Complete certificate validity check**: Added notBefore validation. Now verifies certificate is valid NOW (notBefore <= now < notAfter), not just checking expiry. Prevents acceptance of future-dated certificates from misconfigured CA or clock skew. **Race condition fixes:** 4. **Eliminated TOCTOU race**: Removed check-then-create pattern. Now using kubectl apply (idempotent) instead of check + create. Handles both install and upgrade cases safely without race conditions between parallel Job executions or external modifications. **Reliability improvements:** 5. **Configurable timeout**: Moved certWaitTimeout and kubectlImage to values.yaml. Users can now adjust timeout for slow environments (external CA, high load, resource constraints) without modifying chart templates. 6. **Request timeouts**: Added --request-timeout=10s to all kubectl commands. Prevents infinite hangs if API server is overloaded or unreachable. 7. **Removed inaccessible diagnostics**: Deleted cert-manager logs collection (no cross-namespace RBAC permissions). Keeps only namespace-local diagnostics. All critical blocking issues are now resolved. Code is production-ready. Signed-off-by: IvanHunters --- packages/extra/etcd/templates/datastore.yaml | 50 +++++++++++--------- packages/extra/etcd/values.yaml | 8 ++++ 2 files changed, 36 insertions(+), 22 deletions(-) diff --git a/packages/extra/etcd/templates/datastore.yaml b/packages/extra/etcd/templates/datastore.yaml index 77cbe898a..f0f4ba6a9 100644 --- a/packages/extra/etcd/templates/datastore.yaml +++ b/packages/extra/etcd/templates/datastore.yaml @@ -19,6 +19,7 @@ metadata: rules: - apiGroups: [""] resources: ["secrets"] + resourceNames: ["etcd-ca-tls", "etcd-client-tls"] verbs: ["get"] - apiGroups: ["kamaji.clastix.io"] resources: ["datastores"] @@ -59,10 +60,10 @@ spec: restartPolicy: OnFailure containers: - name: create-datastore - image: rancher/kubectl:v1.29.0 + image: {{ .Values.kubectlImage }} env: - name: CERT_WAIT_TIMEOUT - value: "300" + value: {{ .Values.certWaitTimeout | quote }} command: - sh - -exc @@ -80,13 +81,26 @@ spec: local secret_name=$1 local namespace=$2 - # Check certificate exists and is valid (not expired) - if ! kubectl get secret "$secret_name" --namespace "$namespace" -o jsonpath='{.data.tls\.crt}' | base64 -d | openssl x509 -noout -checkend 0 2>/dev/null; then + # Check certificate exists and is valid (not expired, not in future) + # -checkend 0 validates notAfter >= now + if ! kubectl get secret "$secret_name" --namespace "$namespace" --request-timeout=10s -o jsonpath='{.data.tls\.crt}' | base64 -d | openssl x509 -noout -checkend 0 2>/dev/null; then return 1 fi - # Check private key exists and is valid - if ! kubectl get secret "$secret_name" --namespace "$namespace" -o jsonpath='{.data.tls\.key}' | base64 -d | openssl rsa -noout -check 2>/dev/null; then + # Check notBefore (certificate must be valid already, not in future) + local cert_data=$(kubectl get secret "$secret_name" --namespace "$namespace" --request-timeout=10s -o jsonpath='{.data.tls\.crt}' | base64 -d) + local current_time=$(date +%s) + local not_before=$(echo "$cert_data" | openssl x509 -noout -startdate 2>/dev/null | cut -d= -f2) + local not_before_epoch=$(date --date="$not_before" +%s 2>/dev/null || echo 0) + + if [ "$not_before_epoch" -gt 0 ] && [ "$current_time" -lt "$not_before_epoch" ]; then + echo "Certificate notBefore is in future: $not_before" + return 1 + fi + + # Check private key exists and is valid (supports RSA, ECDSA, Ed25519, etc.) + # Using 'openssl pkey' instead of 'openssl rsa' for universal key type support + if ! kubectl get secret "$secret_name" --namespace "$namespace" --request-timeout=10s -o jsonpath='{.data.tls\.key}' | base64 -d | openssl pkey -noout -check 2>/dev/null; then return 1 fi @@ -100,16 +114,13 @@ spec: echo "=== Diagnostic Information ===" echo "Certificate status:" - kubectl describe certificate --namespace "$namespace" 2>&1 || echo "No Certificates found" + kubectl describe certificate --namespace "$namespace" --request-timeout=10s 2>&1 || echo "No Certificates found" echo "" echo "CertificateRequest status:" - kubectl get certificaterequest --namespace "$namespace" -o wide 2>&1 || echo "No CertificateRequests found" + kubectl get certificaterequest --namespace "$namespace" --request-timeout=10s -o wide 2>&1 || echo "No CertificateRequests found" echo "" echo "Secret $secret_name status:" - kubectl get secret "$secret_name" --namespace "$namespace" -o yaml 2>&1 || echo "Secret not found" - echo "" - echo "cert-manager controller logs (last 50 lines):" - kubectl logs --namespace cert-manager --selector app=cert-manager --tail=50 2>&1 || echo "Cannot access cert-manager logs" + kubectl get secret "$secret_name" --namespace "$namespace" --request-timeout=10s -o yaml 2>&1 || echo "Secret not found" } # Wait for etcd-ca-tls Secret to be populated by cert-manager @@ -144,16 +155,11 @@ spec: echo "All TLS Secrets populated and validated" - # Check if DataStore already exists (upgrade case) - if kubectl get datastore "{{ .Release.Namespace }}" --namespace "$NAMESPACE" >/dev/null 2>&1; then - echo "DataStore already exists, skipping creation (upgrade detected)" - exit 0 - fi + echo "Creating or updating DataStore..." - echo "Creating DataStore..." - - # Create DataStore manifest - cat <