Repository navigation
feat(tls): support mTLS with cert-manager - #297
Conversation
50a1c63 to
d182003
Compare
GatewayJ
left a comment
GatewayJ
left a comment
There was a problem hiding this comment.
Please address the six inline findings before merge. On d1820032, make test and make clippy passed locally. make e2e-check failed because the generated mTLS readiness probe uses TCP. I also reproduced HTTP peer URLs on a four-node mTLS Tenant configuration and a spurious StatefulSet update after applying Kubernetes probe defaults.
|
|
||
| #[serde(default)] | ||
| #[x_kube(validation = Rule::new("self == oldSelf").message("enableMtls is immutable"))] | ||
| pub enable_mtls: bool, |
There was a problem hiding this comment.
[P1] Require a TLS mode and HTTPS peer URLs when enableMtls is set. A Tenant that sets only tls.enableMtls: true gets the default mode: disabled, so the operator deploys it over HTTP. With mode: certManager, omitting enableInternodeHttps still produced http:// peer URLs in RUSTFS_VOLUMES for a four-node Tenant. Those peers cannot connect to the TLS listener. Please reject the disabled-mode combination at admission and ensure distributed mTLS Tenants use HTTPS for every peer URL.
There was a problem hiding this comment.
- Removed redundant
enableMtls:caTrust.clientCaSecretRef(was not realize that this will enable mTLS early) is now the single source of truth for enabling mTLS (is_mtls_enabled()), so the CRD schema indeploy/rustfs-operator/crds/tenant-crd.yamlremains unchanged frommain. - Reconciler guard: Added a reconciler guard in
reconcile_tlsthat blocksmode: disabledwithReason::CertificateInvalidwhenclientCaSecretRefis configured (reconcile_tls_blocks_disabled_mode_when_mtls_is_enabled). - Forced HTTPS peer URLs for mTLS:
is_internode_https_enabled()andTlsPlan::rollout_certificatesnow forceinternode_scheme = "https"whenever mTLS is enabled, even ifenableInternodeHttpsis omitted orfalse. Added unit test coverage inmtls_plan_uses_https_peer_urls_even_when_enable_internode_https_is_false.
| http_probe("/health", tls_plan.probe_scheme) | ||
| }), | ||
| readiness_probe: Some(if tls_plan.mtls_enabled { | ||
| tcp_probe() |
There was a problem hiding this comment.
[P1] Check /health/ready with the client certificate. A TCP connection succeeds as soon as port 9000 accepts connections, including while RustFS is still waiting for read quorum. Kubernetes can then route requests to the Pod and advance a StatefulSet rollout too early. make e2e-check fails here with mTLS readiness probe should use exec, while the new workload unit test expects TCP. Please probe /health/ready using the mounted client certificate and key, and have both tests check that behavior.
There was a problem hiding this comment.
- Added
mtls_readiness_probeinsrc/types/v1alpha1/tls.rsand wiredreadiness_probeinsrc/types/v1alpha1/tenant/workloads.rsto probehttps://127.0.0.1:9000/health/readyviaexec(curl --cert <mount_path>/client_cert.pem --key <mount_path>/client_key.pem ...) by default, while keepingliveness_probeandstartup_probeontcpSocket(:9000). - Updated both the workload unit test (
mtls_tls_plan_switches_probes_and_triggers_statefulset_updateinsrc/types/v1alpha1/tenant/workloads.rs) and the e2e test (mtls_tls_plan_projects_client_identity_and_uses_tcp_and_exec_probesine2e/tests/cert_manager_tls.rs) to verify thatreadiness_probeusesexecand passes--certand--keyagainsthttps://127.0.0.1:9000/health/ready. - For distroless container images that do not ship
curl, we also added an opt-inTENANT_MTLS_READINESS_PROBE=tcpenvironment variable override (with documentation in the user guide noting the caveat thattcpSocketsucceeds as soon as port9000accepts connections and does not wait for/health/readyread quorum) and opened [Feature] Support distroless-compatible /health/ready probing when server mTLS is enabled rustfs#9256 to track native distroless/health/readyprobing under mTLS.
| if config.is_mtls_enabled() | ||
| && ca_trust.client_ca_secret_ref.is_none() | ||
| && explicit_ca_bytes.is_none() | ||
| && !trust_system_ca |
There was a problem hiding this comment.
[P1] Require a CA bundle for inbound client verification. With enableMtls: true, caTrust.source: SystemCa, and no clientCaSecretRef, this condition lets reconciliation continue without mounting either client_ca.crt or ca.crt. RustFS 1.0.4 uses system roots for outbound connections and fails to initialize the server verifier when both files are absent. Please report CaBundleMissing before creating the workload in this configuration. RustFS verifier
There was a problem hiding this comment.
Addressed by removing the separate enableMtls flag and keeping caTrust.clientCaSecretRef as the single source of truth for enabling mTLS. Because mTLS is enabled if and only if clientCaSecretRef is configured, reconcile_cert_manager_tls always validates and projects client_ca.crt before creating the workload (and reports CaBundleMissing / CaBundleInvalid if the referenced client CA Secret is missing or invalid).
| crates(serde_json = "k8s_openapi::serde_json") | ||
| )] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[x_kube(validation = Rule::new("(has(self.tls) && has(self.tls.enableMtls) && self.tls.enableMtls) == (has(oldSelf.tls) && has(oldSelf.tls.enableMtls) && oldSelf.tls.enableMtls)").message("tls.enableMtls is immutable"))] |
There was a problem hiding this comment.
[P1] Protect transitions of the effective mTLS mode. With enableMtls left at false, adding caTrust.clientCaSecretRef makes is_mtls_enabled() return true, and removing it switches mTLS off again. Both updates pass this rule. During a rolling update, new Pods can require client certificates that the old Pods do not present. Please validate transitions using the effective mode, including the legacy CA configuration, while permitting CA rotation on a Tenant that already enforces mTLS.
There was a problem hiding this comment.
Addressed by reverting the spec.tls.enableMtls field and the enableMtls CEL rule on TenantSpec, keeping caTrust.clientCaSecretRef as the sole mechanism for enabling mTLS without introducing a second conflicting flag in the CRD schema.
| } | ||
|
|
||
| // Check container probes (mTLS switches HTTP probes to TCP/exec probes) | ||
| if serde_json::to_value(&existing_container.liveness_probe)? |
There was a problem hiding this comment.
[P2] Account for Kubernetes defaults when comparing probes. I reproduced this on a Tenant with TLS disabled. Starting with the generated StatefulSet, setting only the readiness probe's timeoutSeconds=1, periodSeconds=10, successThreshold=1, and failureThreshold=3 changed statefulset_needs_update from false to true. The API server supplies those values, so each reconciliation can submit the same StatefulSet again. Please normalize the defaults before comparing and cover an API-defaulted probe in the regression test.
There was a problem hiding this comment.
- Added
normalize_probe_for_comparisoninsrc/types/v1alpha1/tenant/workloads.rsto normalize Kubernetes API server defaults (timeoutSeconds: 1,periodSeconds: 10,successThreshold: 1,failureThreshold: 3,initialDelaySeconds: 0, andhttpGet.scheme: "HTTP") before comparingliveness_probe,readiness_probe, andstartup_probe. - Added regression coverage in
api_server_defaulted_probes_do_not_trigger_statefulset_update.
| items.push(key_to_path("tls.key", &server_key_path(domain.as_deref()))); | ||
| } | ||
| if mtls_enabled && certificate.domains.iter().any(Option::is_none) { | ||
| items.push(key_to_path("tls.crt", RUSTFS_CLIENT_CERT_FILE)); |
There was a problem hiding this comment.
[P2] Verify that the default certificate can authenticate as a client. This projects the server certificate as the peer identity, and the operator also uses it for admin and STS calls. An explicit usages: ["server auth"] or a clientCaSecretRef that does not trust the certificate's issuer still lets TLS reach Ready. Peers and the operator then fail the client-auth handshake. Please check the issued certificate's client-auth eligibility and chain against the effective client CA before reporting TLS Ready, and cover both failure cases.
There was a problem hiding this comment.
- In
validate_certificate_entries, if explicitcertManager.usagesare configured on the default certificate while mTLS is enabled and do not include"client auth", reconciliation blocks withReason::CertificateInvalid. - Added
validate_default_certificate_client_authinsrc/reconcile/tls.rs, which parses the issued default certificate withwebpki::EndEntityCertand callsverify_for_usage(..., webpki::KeyUsage::client_auth(), ...)against the trust anchors parsed from the configuredclientCaSecretRefbundle before reporting TLS Ready. - Added unit test coverage for both failure modes in
certificate_entries_rejects_default_cert_missing_client_auth_usage_when_mtls_enabledandvalidate_default_certificate_client_auth_checks_eku_and_issuer_chain.
c561411 to
d182003
Compare
d182003 to
33fd84a
Compare
|
@GatewayJ I will need to do more tests before publishing this to review further. Thanks |
|
@GatewayJ I've addressed comments and ready to review, PTAL. Thank you |
|
@boylee1111, do you consider the current changes ready to merge, or do you still have changes or tests planned? You mentioned that the PR is ready for another review. If so, could you mark it as Ready for review on GitHub? |
|
@GatewayJ yes, marked as ready to review |
Type of Change
Related Issues
/health/readyprobing under mTLS: [Feature] Support distroless-compatible /health/ready probing when server mTLS is enabled rustfs#9256Summary of Changes
Complete end-to-end mutual TLS (mTLS) support with
cert-managerwhencaTrust.clientCaSecretRef(or legacycertManager.caSecretRef) is configured on aTenant, covering both client-to-Pod S3/Admin API connections and Pod-to-Pod internode RPC connections on:9000without introducing redundant CRD fields:src/types/v1alpha1/tls.rs,src/reconcile/pool_lifecycle.rs):TlsConfig::is_mtls_enabled()driven bycaTrust.clientCaSecretRef(and legacycertManager.caSecretRef).internode_scheme = "https"whenever mTLS is enabled so distributedRUSTFS_VOLUMESpeer URLs always usehttps://.RUSTFS_SERVER_MTLS_ENABLE=true, projectclient_ca.crtfromclientCaSecretRef, and project the default certificate'stls.crtandtls.keytoclient_cert.pemandclient_key.pem(alongsiderustfs_cert.pemandrustfs_key.pem) for outbound Pod-to-Pod connections.src/reconcile/tls.rs):tls.mode: disabledwhenclientCaSecretRefis configured (Reason::CertificateInvalid).spec.usageson managed defaultCertificateresources to["server auth", "client auth"]when mTLS is enabled andcertManager.usagesis omitted; reject explicitcertManager.usageson the default certificate that omit"client auth"when mTLS is enabled.clientCaSecretReftrust anchors forwebpki::KeyUsage::client_auth()before reporting TLSReady.mtls_enabledintls_hashto trigger StatefulSet rollout on changes.src/types/v1alpha1/tenant/workloads.rs,src/types/v1alpha1/tenant/helper.rs):livenessProbeandstartupProbetotcpSocketon port9000andreadinessProbeto anexecprobe (curl -skf --cert <mountPath>/client_cert.pem --key <mountPath>/client_key.pem https://127.0.0.1:9000/health/ready) by default when mTLS is enabled.TENANT_MTLS_READINESS_PROBE=tcp(viatenant.spec.envor operator environment) to fall back totcpSocketon port9000for distroless container images withoutcurl(tracked in [Feature] Support distroless-compatible /health/ready probing when server mTLS is enabled rustfs#9256).timeoutSeconds,periodSeconds,successThreshold,failureThreshold,initialDelaySeconds,httpGet.scheme) when comparing container probes instatefulset_needs_update_with_tls_plan_and_cluster_domain.crates/rustfs-admin/src/lib.rs,src/sts/rustfs_client.rs):RustfsAdminClient::new_with_base_url_tls_materialandload_tenant_tls_client_identityso the operator presents the Tenant's default certificate (status.certificates.tls.serverSecretRef) as its client identity when reconciling buckets/users/policies, pool lifecycle, monitoring, and STS against mTLS-enabled Tenants.docs/operator-user-guide.md,docs/operator-user-guide.zh-CN.md,deploy/rustfs-operator/README.md, andCHANGELOG.md.examples/cert-manager-mtls-tenant.yamland updateexamples/cert-manager-ca-trust-tenant.yaml.Certificateusages andclient_authchain verification, mTLS probes (both defaultexecandTENANT_MTLS_READINESS_PROBE=tcp), probe default normalization, and operator client identity loading.Checklist
make pre-commit(fmt-check + clippy + test + console-lint + console-fmt-check)[Unreleased](if user-visible change)Impact
caTrust.clientCaSecretRefis used as the single source of truth for enabling mTLS.Verification
make release-metadata-check fmt-check clippy test e2e-checkAlso verified live on a 4-node Kind cluster (
kind-rustfs-e2e) withcert-manager(v1.16.2) andrustfs/rustfs:1.0.4, as well as a 3-node distroless appliance cluster (TENANT_MTLS_READINESS_PROBE=tcp):Certificateissued withusages: ["server auth", "client auth"]1/1 Running(Ready) withreadQuorum: trueandwriteQuorum: trueover HTTP/2 + mTLStlsv13 alert certificate required; requests withclient_cert.pem/client_key.pemsucceeded withHTTP/2 200spec.buckets) succeeded over mTLS (ProvisioningReady=True)