feat: add auth and tls for mirroring connections #4002

fraidev · 2024-05-10T18:55:48Z

In this PR I used the same logic for auth on SC for SPU.
Also added methods to validate by CN of the certificate, not only by scopes. Allowing to check if the remote has the same name as the CN of the certificate.

Testing

Happy path

On home side:

$ flvd cluster start --k8 --develop --tls --domain fluvio.local --server-key ./tls/certs/server.key --server-cert ./tls/certs/server.crt --ca-cert ./tls/certs/ca.crt --client-cert ./tls/certs/client-root.crt --client-key ./tls/certs/client-
root.key
$ flvd remote register user1
$ echo '[                                                                                                                                                                                                               
    "user1"
]' > devices.json 
$ flvd topic create mirror-topic --mirror-apply devices.json
$ flvd remote export user1 --file user1.json --key tls/certs/client-user1.key --cert tls/certs/client-user1.crt

On remote side:

$ flvd cluster start
$ flvd home connect --file user1.json

Then check:

the topics and partitions are on both sides with: flvd topic list and flvd partition list
only topics that are mirror topics and assigned to the remote1 are sync on remote side.
try to produce on the topic on remote side and consume on home side.
check status on remote side with: flvd home status
check status on home side with: flvd remote list

Unhappy path

Do the same using invalid certs or valid certs with another CN (e.g. tls/certs/client-root.crt).

fraidev · 2024-05-19T05:24:09Z

k8-util/helm/fluvio-sys/templates/crd_mirror.yaml

-                      type: string
-                    privateKey:
-                      type: string
+                        tls:


Should this be a secret?

Is this metadata used by the edge or home? Why is tls config needed here?

Is this metadata used by the edge or home?

Edge is storing it as Home.

Why is tls config needed here?

To connect to home with TLS.

fraidev · 2024-05-19T05:25:51Z

crates/fluvio-sc/Cargo.toml

@@ -47,6 +47,7 @@ tracing = { workspace = true }


 # Fluvio dependencies
+fluvio = { workspace = true }


Is too bad to add this dependence?
We need DomainConnector::try_from, I even tried to move it to fluvio-auth for example, but this method needs the build features of fluvio package.

If just need DomainConnector this can be imported from fluvio-future

True, but we need fluvio::config::TlsPolicy there too.
Same problem, this is hard to move because use build features of fluvio package.

sehz · 2024-05-20T14:26:19Z

crates/fluvio-auth/src/x509/authenticator.rs

@@ -36,15 +36,15 @@ impl ScopeBindings {

 #[derive(Debug)]
 pub struct X509Authenticator {


Initial scope of the authenticator was for only for SC. Should not overload these with two different purpose. Probably simpler rename original X509Authenticator as X509AuthenticatorSC and move to SC. And use this crate only for common auth utilities and libraries.

based on other conversation, this should need to be changed since remote can be added to scope

sehz · 2024-05-20T14:27:24Z

crates/fluvio-controlplane-metadata/src/mirror/spec.rs

@@ -75,4 +74,25 @@ pub struct Home {
    pub id: String,
    pub remote_id: String,
    pub public_endpoint: String,
+    #[cfg_attr(feature = "use_serde", serde(skip_serializing_if = "Option::is_none"))]
+    pub tls: Option<ClientTls>,


TLS should be mandatory

@sehz we had this conversation on Friday. TLS should not be mandatory, as there are 3 use cases.

It always been mandatory to have TLS for these connections:

Mirror Connection (Between Edge and Home)

Home

Edge doesn't not need to have TLS

I understand that run a mirroring in production without TLS is super risky, but maybe the user wants to test it locally first without certificate overhead.

Maybe we should just print warns that this is not a secure connection?

As we discussed on Friday, when I test this locally, I don't need TLS connection. Adding TLS in this situation makes testing locally a huge undertaking. It is similar to asking users to install K8 to test locally.

@fraidev please schedule a meeting.

So there are two different questions:
(1) What strategy for Authentication (TLS, None, etc).
(2) Whether this flag is needed which will be depend on answering first question. If this is needed, name should be change to so intent is clear.

Decision: Edge authentication is optional for mirroring,. However TLS is mandatory if auth is required

But in this case, TLS should be attached to Edge not to home.

Edge cluster is storing TLS in a record of struct Home

sehz · 2024-05-20T14:28:07Z

crates/fluvio-sc/Cargo.toml

@@ -47,6 +47,7 @@ tracing = { workspace = true }


 # Fluvio dependencies
+fluvio = { workspace = true }


If just need DomainConnector this can be imported from fluvio-future

sehz · 2024-05-20T14:29:24Z

crates/fluvio-sc/src/config/sc_config.rs

@@ -29,6 +29,7 @@ pub struct ScConfig {
    pub namespace: String,
    pub x509_auth_scopes: Option<PathBuf>,
    pub white_list: HashSet<String>,
+    pub tls: bool,


x509 requires TLS, so it redundant configuration

Do you mean x509_auth_scopes? I can run a cluster with TLS without scopes.

No I meant, why do need another tis config as TLS is already implied in the existing config.

sehz · 2024-05-20T14:31:20Z

crates/fluvio-sc/src/controllers/mirroring/controller.rs

@@ -195,3 +223,23 @@ impl<C: MetadataItem> RemoteMirrorController<C> {
        }
    }
 }
+
+fn option_tlspolicy(home: &Home) -> Option<TlsPolicy> {


TLS should mandatory

Not mandatory.

This is not needed anyway since SCConfig and Remote config are two different things.

sehz · 2024-05-20T14:33:35Z

crates/fluvio-sc/src/init.rs

+                info!("using spu remote authorization");
+                start_public_server(AuthGlobalContext::new(
+                    ctx,
+                    Arc::new(RemoteAuthorization::new()),


Hmm. Can't have two different policy. Instead this should be unified

True, make sense I just make the BasicRbacPolicy optional for BasicAuthorization?

use whatever as default. no need for change

I think it's needed two different authorizations, I can't use basic authorization because I don't want its allow_type_action implementation when there is no scopes.

scope should be part of auth context

sehz · 2024-05-20T14:33:56Z

crates/fluvio-sc/src/services/auth/remote.rs

+use fluvio_auth::x509::X509Identity;
+
+#[derive(Debug, Clone)]
+pub struct RemoteAuthorization {}


This should be unified

sehz · 2024-05-20T14:35:52Z

crates/fluvio-spu/src/services/auth/root.rs

+pub struct SpuRootAuthorization {}
+
+#[async_trait]
+impl Authorization for SpuRootAuthorization {


Why do you need SPU Authorization since it allows for everything? Let's not try to re-map same semantics as SC which may not be same meaning

Because I am calling an AuthContext method on spu now, the allow_remote_id.

Should I rename this default authorization?

Instead of adding as special case, just make allow_remote_id as part of scope. That way, no need to change existing auth

I don't undestand.

should I not check authorization like it:

// authorization check if !auth_ctx .auth .allow_remote_id(&req_msg.request.remote_cluster_id) { warn!( "identity mismatch for remote_id: {}", req_msg.request.remote_cluster_id ); return; }

Something like this:

if let Ok(authorized) = auth_ctx .auth .allow_instance_action(RemoteSpec::Edge, InstanceAction::Update, edge_id) .await {

oh got it, this seems better.

But we keep needing a default Authorization for SPU when TLS is disabled, which should be?

Auth context is always there. It will be configured differently on whether we configure with TLS or not.
If no TLS then it will be set to Root level authorization ( means you can do whatever you want). If TLS then authorization is derived from TLS certs

nacardin

We don't need to store the Edge's client certs as metadata right?

feat: add mirroring tls feat: check cn cert name when export mirror feat: mirror authorization on sc feat: mirror authorization on spu wip debug logs fix: active peer ssl verify mode for spu tls chore: fmt and clippy

fraidev force-pushed the mirroring_step_5 branch 8 times, most recently from a27be62 to 1b66907 Compare May 17, 2024 04:11

fraidev force-pushed the mirroring_step_5 branch 2 times, most recently from 5875d9d to 19db26a Compare May 19, 2024 03:48

fraidev changed the title ~~feat: add mirroring tls~~ feat: add auth and tls for mirroring connections May 19, 2024

fraidev force-pushed the mirroring_step_5 branch 4 times, most recently from 4674750 to 2c5b19a Compare May 19, 2024 05:03

fraidev marked this pull request as ready for review May 19, 2024 05:23

fraidev commented May 19, 2024

View reviewed changes

fraidev requested review from sehz, digikata and nacardin May 19, 2024 05:26

sehz requested changes May 20, 2024

View reviewed changes

nacardin reviewed May 20, 2024

View reviewed changes

fraidev added 2 commits May 21, 2024 17:45

feat: add auth and tls for mirroring connections

7476042

feat: add mirroring tls feat: check cn cert name when export mirror feat: mirror authorization on sc feat: mirror authorization on spu wip debug logs fix: active peer ssl verify mode for spu tls chore: fmt and clippy

chore: move authorizations to fluvio-auth

b2dc395

fraidev force-pushed the mirroring_step_5 branch from 1960b48 to b2dc395 Compare May 21, 2024 20:45

chore: check mirroring auth using scopes and policy

b07a841

		@@ -47,6 +47,7 @@ tracing = { workspace = true }


		# Fluvio dependencies
		fluvio = { workspace = true }

		@@ -36,15 +36,15 @@ impl ScopeBindings {

		#[derive(Debug)]
		pub struct X509Authenticator {

feat: add auth and tls for mirroring connections #4002

Are you sure you want to change the base?

feat: add auth and tls for mirroring connections #4002

Conversation

fraidev commented May 10, 2024 • edited

Testing

Happy path

Unhappy path

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sehz May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajhunyady May 20, 2024 • 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

fraidev May 21, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fraidev May 20, 2024 • 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

fraidev May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sehz May 20, 2024 • edited

Choose a reason for hiding this comment

fraidev May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nacardin left a comment

Choose a reason for hiding this comment

fraidev commented May 10, 2024 •

edited

sehz May 20, 2024 •

edited

ajhunyady May 20, 2024 •

edited

fraidev May 21, 2024 •

edited

fraidev May 20, 2024 •

edited

fraidev May 20, 2024 •

edited

sehz May 20, 2024 •

edited

fraidev May 20, 2024 •

edited