-
Notifications
You must be signed in to change notification settings - Fork 506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add auth and tls for mirroring connections #4002
Conversation
a27be62
to
1b66907
Compare
5875d9d
to
19db26a
Compare
4674750
to
2c5b19a
Compare
type: string | ||
privateKey: | ||
type: string | ||
tls: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a secret?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this metadata used by the edge or home? Why is tls
config needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -47,6 +47,7 @@ tracing = { workspace = true } | |||
|
|||
|
|||
# Fluvio dependencies | |||
fluvio = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If just need DomainConnector
this can be imported from fluvio-future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but we need fluvio::config::TlsPolicy
there too.
Same problem, this is hard to move because use build features of fluvio package.
@@ -36,15 +36,15 @@ impl ScopeBindings { | |||
|
|||
#[derive(Debug)] | |||
pub struct X509Authenticator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on other conversation, this should need to be changed since remote
can be added to scope
@@ -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>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLS should be mandatory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sehz we had this conversation on Friday. TLS should not be mandatory, as there are 3 use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fraidev please schedule a meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decision: Edge authentication is optional for mirroring,. However TLS is mandatory if auth is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in this case, TLS should be attached to Edge not to home.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edge cluster is storing TLS in a record of struct Home
@@ -47,6 +47,7 @@ tracing = { workspace = true } | |||
|
|||
|
|||
# Fluvio dependencies | |||
fluvio = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If just need DomainConnector
this can be imported from fluvio-future
@@ -29,6 +29,7 @@ pub struct ScConfig { | |||
pub namespace: String, | |||
pub x509_auth_scopes: Option<PathBuf>, | |||
pub white_list: HashSet<String>, | |||
pub tls: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x509 requires TLS, so it redundant configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean x509_auth_scopes? I can run a cluster with TLS without scopes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I meant, why do need another tis
config as TLS is already implied in the existing config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where?
@@ -195,3 +223,23 @@ impl<C: MetadataItem> RemoteMirrorController<C> { | |||
} | |||
} | |||
} | |||
|
|||
fn option_tlspolicy(home: &Home) -> Option<TlsPolicy> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLS should mandatory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not mandatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed anyway since SCConfig and Remote config are two different things.
crates/fluvio-sc/src/init.rs
Outdated
info!("using spu remote authorization"); | ||
start_public_server(AuthGlobalContext::new( | ||
ctx, | ||
Arc::new(RemoteAuthorization::new()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Can't have two different policy. Instead this should be unified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, make sense I just make the BasicRbacPolicy optional for BasicAuthorization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use whatever as default. no need for change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope should be part of auth context
use fluvio_auth::x509::X509Identity; | ||
|
||
#[derive(Debug, Clone)] | ||
pub struct RemoteAuthorization {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be unified
pub struct SpuRootAuthorization {} | ||
|
||
#[async_trait] | ||
impl Authorization for SpuRootAuthorization { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I am calling an AuthContext
method on spu now, the allow_remote_id
.
Should I rename this default authorization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding as special case, just make allow_remote_id
as part of scope. That way, no need to change existing auth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this:
if let Ok(authorized) = auth_ctx
.auth
.allow_instance_action(RemoteSpec::Edge, InstanceAction::Update, edge_id)
.await
{
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh got it, this seems better.
But we keep needing a default Authorization for SPU when TLS is disabled, which should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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:
On remote side:
Then check:
flvd topic list
andflvd partition list
flvd home status
flvd remote list
Unhappy path
Do the same using invalid certs or valid certs with another CN (e.g. tls/certs/client-root.crt).