K8 CSR support #387

aryan16 · 2023-02-08T22:31:20Z

Added support for K8 CSR and custom signer

aryan16 · 2023-02-08T22:50:53Z

@howardjohn I will move the client creation to the common level (probably when we create the configs) ?

hzxuzhonghu

Why do we have a different k8s csr support from sidecar? We send request to istiod and istiod will do this for sidecar

src/identity/customca.rs

aryan16 · 2023-02-09T18:36:44Z

Why do we have a different k8s csr support from sidecar? We send request to istiod and istiod will do this for sidecar

IMO Ztunnel has all the capabilities to make a CSR request directly. Why to add an extra complexity of sending req to istiod.
cc : @howardjohn

Cargo.toml

src/identity/customca.rs

src/identity/manager.rs

src/identity/customca.rs

src/identity/manager.rs

hzxuzhonghu · 2023-02-14T01:48:37Z

@aryan16 How can we support VM installation using this mode?

aryan16 · 2023-02-14T23:39:54Z

@aryan16 How can we support VM installation using this mode?

If VM is able to talk to K8s api server then no issues, if not we won't be using this feature instead use the existing GRPC call to istiod.

JimmyCYJ · 2023-02-22T01:16:28Z

src/app.rs

    if config.fake_ca {
        let cert_manager = identity::mock::new_secret_manager(Duration::from_secs(86400));
        build_with_cert(config, cert_manager).await
-    } else {
+    } else if config.custom_signer.is_some() {


Did we validate the config before it is passed to build()?

you mean the custom signer? No we are not validating the custom signer, do we need it?

src/identity/customca.rs

src/config.rs

howardjohn · 2023-02-28T16:51:40Z

src/identity/manager.rs

@@ -414,6 +415,19 @@ impl SecretManager<CaClient> {
    }
 }

+impl SecretManager<CustomSigner> {
+    pub async fn origin(cfg: crate::config::Config) -> Result<Self, anyhow::Error> {


what does "origin" mean?

Associated function - https://doc.rust-lang.org/rust-by-example/fn/methods.html.

I know it is a function, I just do not get the name?

oh I saw the same name at many places in rust example codes as a replacement of new. Do you think I should update it?

src/identity/manager.rs

src/app.rs

src/identity/customca.rs

howardjohn · 2023-02-28T17:17:09Z

src/identity/customca.rs

+
+async fn check_signed_cert(csr: &Api<CertificateSigningRequest>, csr_name: &String) -> Result<k8s_openapi::ByteString, anyhow::Error> {
+    let lp = ListParams::default().fields(&format!("{}{}", "metadata.name=", csr_name)).timeout(30);
+    let mut stream = csr.watch(&lp, "0").await?.boxed();


just watch has a race condition; if it was finished before we start the watch it will break. Need a List+Watch

I would overall prefer a long lived watched on a label we set on each CSR but ok with list+watch approach like we do in chiron/utils.go in the short term. Note you need to put ResourceVersion as well.

Updated, PTAL

src/identity/customca.rs

src/tls/boring.rs

linux-foundation-easycla · 2023-03-01T00:23:07Z

The committers listed above are authorized under a signed CLA.

✅ login: aryan16 / name: Aryan Gupta (16bd692)

istio-testing · 2023-03-04T14:51:58Z

PR needs rebase.

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/test-infra repository.

costinm · 2023-03-30T12:39:37Z

What's next ? The DNS proxy ( which is REQUIRED if the underlying network is not secure ) ? Integration with the external SDS provider using UDS ?

Can we please add the agent and just use it ? No need for ztunnel to duplicate all the integrations and do everything ( in a slightly different way ).

A design doc on such changes with focus on security will also be greatly appreciated:

who approves the CSRs and on what criteria ? If Istiod, why not use the existing code that also generates the CSR
VMs - how does it change the install and security requirements ? We didn't use k8s signing from VMs because
it required VMs to have K8S SA and access - which means higher escalation risks and permissions.
what are the benefits compared to using the current method and letting Istiod generate the k8s sign request ?

But even if we conclude direct integration with k8s from the proxy is best ( and I can be convinced it's a more
standard protocol than our gRPC call to Istiod ) - we should still do it in the agent, so both sidecars and ambient
are consistent and use whatever is best.

costinm · 2023-03-30T12:45:22Z

On a related note - ztunnel should eventually match the integrations we have, including SDS and CSI-based providers ( in both dedicated mode and in future if we implement delegation ).

I saw the FAKE provider - IMO it should be replaced (or we should add) with a mode that reads from /var/run/secrets... ( same default as istio-agent ), and fallback to SDS using the well-known path that the Envoy is using ( for Spire and all other integrations ).
And once SDS is added - we can maybe remove the call to Istiod and keep ztunnel focused on proxying, not implementing multiple CA protocols, just XDS and files.

costinm · 2023-03-30T12:51:14Z

src/identity/manager.rs

+impl SecretManager<CustomSigner> {
+    pub async fn origin(cfg: crate::config::Config) -> Result<Self, anyhow::Error> {
+        let k8s_client = tls::create_k8s_client(cfg.k8s_ca_crt).await;
+        match k8s_client {


This won't work on VMs.

Pretty sure there is a way to initialize k8s client in rust that works with both 'in cluster' and kubeconfig.

Same for the tokens - outside of K8S you need all the auth methods, the VM may use MDS or external commands to authenticate ( gcloud, etc ).

costinm · 2023-03-30T12:52:15Z

src/tls/boring.rs

+pub async fn create_k8s_client(root_cert: RootCert) -> Result<kube::Client, anyhow::Error> {
+    let mut conn = ssl::SslConnector::builder(ssl::SslMethod::tls_client())?;
+    conn.set_verify(ssl::SslVerifyMode::PEER);
+    conn.set_alpn_protos(Alpn::H2.encode())?;


Is this the only SSL connection we create in ztunnel ? Any chance we could use the same code - or client library code ?

istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 8, 2023

aryan16 marked this pull request as draft February 8, 2023 22:31

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 8, 2023

aryan16 force-pushed the K8s-csr branch from 62e9a44 to 4f66eee Compare February 8, 2023 22:34

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 8, 2023

aryan16 force-pushed the K8s-csr branch from 4f66eee to 4e5667f Compare February 8, 2023 22:49

aryan16 requested a review from howardjohn February 8, 2023 22:50

aryan16 removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 8, 2023

aryan16 marked this pull request as ready for review February 8, 2023 22:53

aryan16 requested review from liminw, shankgan and JimmyCYJ February 8, 2023 23:00

hzxuzhonghu reviewed Feb 9, 2023

View reviewed changes

src/identity/customca.rs Show resolved Hide resolved

briansonnenberg reviewed Feb 9, 2023

View reviewed changes

src/identity/customca.rs Outdated Show resolved Hide resolved

howardjohn reviewed Feb 10, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

src/identity/customca.rs Outdated Show resolved Hide resolved

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 10, 2023

liminw reviewed Feb 10, 2023

View reviewed changes

src/identity/manager.rs Outdated Show resolved Hide resolved

src/identity/customca.rs Show resolved Hide resolved

ymesika reviewed Feb 13, 2023

View reviewed changes

src/identity/manager.rs Outdated Show resolved Hide resolved

aryan16 force-pushed the K8s-csr branch from 4e5667f to c615f97 Compare February 14, 2023 23:32

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 14, 2023

aryan16 force-pushed the K8s-csr branch 2 times, most recently from 90cf030 to bb4ba58 Compare February 14, 2023 23:38

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 16, 2023

aryan16 force-pushed the K8s-csr branch 2 times, most recently from 9f7fa6c to b832255 Compare February 21, 2023 23:10

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 21, 2023

aryan16 force-pushed the K8s-csr branch from b832255 to 361479f Compare February 21, 2023 23:28

JimmyCYJ reviewed Feb 22, 2023

View reviewed changes

aryan16 requested review from JimmyCYJ and howardjohn February 22, 2023 17:17

shankgan reviewed Feb 22, 2023

View reviewed changes

src/identity/customca.rs Outdated Show resolved Hide resolved

shankgan reviewed Feb 22, 2023

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

JimmyCYJ reviewed Feb 22, 2023

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

howardjohn reviewed Feb 28, 2023

View reviewed changes

aryan16 force-pushed the K8s-csr branch from 361479f to 16bd692 Compare March 1, 2023 00:23

aryan16 force-pushed the K8s-csr branch 2 times, most recently from 59c4c2a to 72b7541 Compare March 1, 2023 18:49

K8 CSR support

11c6189

aryan16 force-pushed the K8s-csr branch from 72b7541 to 11c6189 Compare March 1, 2023 21:01

istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 1, 2023

aryan16 requested a review from howardjohn March 2, 2023 22:02

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 4, 2023

costinm reviewed Mar 30, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

K8 CSR support #387

K8 CSR support #387

aryan16 commented Feb 8, 2023

aryan16 commented Feb 8, 2023

hzxuzhonghu left a comment

aryan16 commented Feb 9, 2023

hzxuzhonghu commented Feb 14, 2023

aryan16 commented Feb 14, 2023

JimmyCYJ Feb 22, 2023 •

edited

aryan16 Feb 22, 2023

howardjohn Feb 28, 2023

aryan16 Mar 1, 2023

howardjohn Mar 1, 2023

aryan16 Mar 1, 2023

howardjohn Feb 28, 2023

aryan16 Mar 1, 2023

linux-foundation-easycla bot commented Mar 1, 2023 •

edited

istio-testing commented Mar 4, 2023

costinm commented Mar 30, 2023

costinm commented Mar 30, 2023

costinm Mar 30, 2023

costinm Mar 30, 2023

K8 CSR support #387

Are you sure you want to change the base?

K8 CSR support #387

Conversation

aryan16 commented Feb 8, 2023

aryan16 commented Feb 8, 2023

hzxuzhonghu left a comment

Choose a reason for hiding this comment

aryan16 commented Feb 9, 2023

hzxuzhonghu commented Feb 14, 2023

aryan16 commented Feb 14, 2023

JimmyCYJ Feb 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Mar 1, 2023 • edited

istio-testing commented Mar 4, 2023

costinm commented Mar 30, 2023

costinm commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimmyCYJ Feb 22, 2023 •

edited

linux-foundation-easycla bot commented Mar 1, 2023 •

edited