-
Notifications
You must be signed in to change notification settings - Fork 405
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
[ANSIENG-3807] | Rbac over mTLS #1804
base: 7.8.x
Are you sure you want to change the base?
Conversation
* [ANSIENG-4229] | adding new user facing variables for rbac over mtls * [try-mtls] | modified default listener and get auth token * [ANSIENG-4229] | adding 2 small scenarios for testing * [try-mtls] | [ANSIENG-4229] | modifying the default values of ssl_client_authentication and ssl_mutual_auth * [try-mtls] | fixing issues in set principal for mtls and health checks * [try-mtls] | fixing mds client auth properties * [try-mtls] | adding ldap+mtls sceanrio * [try-mtls] | changing os to rhel9 and java to 17 for mtls tests * [try-mtls] | fix listener auth issue * [try-mtls] | fix health checks for broker * [try-mtls] | adding impersonation super users in bk customer properties * [try-mtls] | unmasking secrets for better logging * [try-mtls] | fix mds health check and config validations to include auth mode mtls * [try-mtls] | fix get authorization tokens * [try-mtls] | temporary removal of ssl endpoint identification * [try-mtls] | fix verify of new molecule scenarios for mtls * [try-mtls] | chaning erp listener to oauthbearer listener for impersonation token * [try-mtls] | fixing qoutes in erp listener name * [try-mtls] | removing ssl client auth verification from controller as it is only mds server property * [try-mtls] | adding impersonation super and protected users in mds properties * [try-mtls] | modify ssl client authentication for listeners based on inventory file * [try-mtls] | principal mapping rules on listeners * [try-mtls] | overriding principal mapping rules in molecule * [try-mtls] | fix impersonation protected user and remove config override for impersonation super user * [try-mtls] | remove principal mapping rules * [try-mtls] | fixing qoutes around client auth mode in listeners * [try-mtls] | adding sr in mtls only setup * [try-mtls] | fixing sr mtls * [try-mtls] | fix ldap detection and add extra $ in molecule for escape reasons * Fix SR RBAC (#70) * [ANSIENG-4229] | adding new user facing variables for rbac over mtls * [try-mtls] | modified default listener and get auth token * [ANSIENG-4229] | adding 2 small scenarios for testing * [try-mtls] | [ANSIENG-4229] | modifying the default values of ssl_client_authentication and ssl_mutual_auth * [try-mtls] | fixing issues in set principal for mtls and health checks * [try-mtls] | fixing mds client auth properties * [try-mtls] | adding ldap+mtls sceanrio * [try-mtls] | changing os to rhel9 and java to 17 for mtls tests * [try-mtls] | fix listener auth issue * [try-mtls] | fix health checks for broker * [try-mtls] | adding impersonation super users in bk customer properties * [try-mtls] | unmasking secrets for better logging * [try-mtls] | fix mds health check and config validations to include auth mode mtls * [try-mtls] | fix get authorization tokens * [try-mtls] | temporary removal of ssl endpoint identification * [try-mtls] | fix verify of new molecule scenarios for mtls * [try-mtls] | chaning erp listener to oauthbearer listener for impersonation token * [try-mtls] | fixing qoutes in erp listener name * [try-mtls] | removing ssl client auth verification from controller as it is only mds server property * [try-mtls] | adding impersonation super and protected users in mds properties * [try-mtls] | modify ssl client authentication for listeners based on inventory file * [try-mtls] | principal mapping rules on listeners * [try-mtls] | overriding principal mapping rules in molecule * [try-mtls] | fix impersonation protected user and remove config override for impersonation super user * [try-mtls] | remove principal mapping rules * [try-mtls] | fixing qoutes around client auth mode in listeners * [try-mtls] | adding sr in mtls only setup * [try-mtls] | fixing sr mtls * [try-mtls] | fix ldap detection and add extra $ in molecule for escape reasons --------- Co-authored-by: Mansi Jain <[email protected]> * [ANSIENG-4233] | added mtls configs for connect * [ANSIENG-4233] | comment fix * [try-mtls] | fix sr changes * [ANSIENG-4233] | config fix for listener authentication * [ANSIENG-4233] | add config for connectors * [ANSIENG-4233] | code fix * [ANSIENG-4233] | code revert * [try-mtls] | modify molecule scenario to add super user and principal mapping rules * [ANSIENG-4233] | add connector mtls config * [ANSIENG-4236] | add connect replicator mtls config * [ANSIENG-4233] | delegate token fetch to broker for connector * [ANSIENG-4236] | property fix * [ANSIENG-4235] | mtls configs * [ANSIENG-4235] | mtls configs * [ANSIENG-4236] | mtls configs * [ANSIENG-4236] | mtls configs * [ANSIENG-4235] | mtls configs * [ANSIENG-4235] | property fix * [ANSIENG-4236] | add test for replicator * [pm-rules] | handle default principal mapping rules * [pm-rules] | principal mapping rules in mds, erp, listeners, sr * [pm-rules] | removing config overrides from mtls onnly scenario as principal mapping rules are added by variables * [pm-rules] | principal mapping rules fix to get proper super user * [pm-rules] | adding listener level control over principal mapping rules * [pm-rules] | fix principal mapping rules in listeners * [mtls-rp] | add mtls support in erp,rp * [mtls-rp] | fix kafka rest license issue * [ANSIENG-4235] | add eol * Connect mtls ansieng 4233 (#72) * [ANSIENG-4233] | added mtls configs for connect * [ANSIENG-4233] | comment fix * [ANSIENG-4233] | config fix for listener authentication * [ANSIENG-4233] | add config for connectors * [ANSIENG-4233] | code fix * [ANSIENG-4233] | code revert * [ANSIENG-4233] | add connector mtls config * [ANSIENG-4233] | delegate token fetch to broker for connector * Revert "Connect mtls ansieng 4233 (#72)" (#76) This reverts commit 57cc5e6. * [try-mtls] | c3 mtls support * [try-mtls] | adding c3 in mtls scenario * [try-mtls] | fix c3 bugs * [try-mtls] | adding impersonation users to molecule scenarios * [try-mtls] | fix kafka rest listener and conlfuent.license config * [try-mtls] | file based login in mtls only scenario added * [try-mtls] | fix oauth and ldap scenarios and confluent.license * [try-mtls] | send certs in kafka broker tasks for register cluster * [try-mtls] | fixing register cluster to run on internal token listener so it has sasl_ssl protocol instead of ssl for rbac over mtls * [mtls-connect] | making retries in get auth token configurable and increasing the default wait time as mds takes more time in upgrades * [mtls-connect] | adding ksql connect in molecule scenarios * [mtls-connect] | fix erp pm rules * [mtls-connect] | remove cyclic dependency in ssl_client_authentication and ssl_mutual_auth_enabled * [mtls-connect] | remove set fact for mtls old var * [mtls-connect] | add when conditions for extract principal --------- Co-authored-by: Mansi Jain <[email protected]>
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
…abled and ssl_client_authentication and askign for user confirmation for setting requested
…instead of oauth_enabled and ldap_with_oauth_enabled varaibles
…ving rolebindings to the principal for mtls
… and broker based on external mds setup or single cluster setup
…l client authentication enabled
…y certs to communicate to mds even when mds has support for oauth/ldap
- name: Define the new ssl_client_authentication variable | ||
set_fact: | ||
to_be_ssl_client_authentication: >- | ||
{%- if deployment_strategy == 'parallel' -%} |
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.
or not rbac_enabled|bool should also be there
{{ | ||
('ldap' in auth_mode) | ternary("User:" + mds_super_user|default('mds'), "") + | ||
(auth_mode == 'ldap_with_oauth') | ternary(";", "") + | ||
('oauth' in auth_mode) | ternary("User:" + oauth_superuser_principal, "") |
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.
mtls super user should also be added.
validate_certs: false | ||
headers: | ||
Content-Type: application/json | ||
Authorization: "Bearer {{ authorization_token }}" |
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 we have authorization token in mtls only ?
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 got added as I copied the above task and created this one.
Although i dont think it makes any difference if we pass the token or not. If we pass it the principal will be taken from the token. and token was given for kafka broker cert. And if dont pass the token principal will be taken from the cert of kafka broker. so same principal will be used either way.
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.
agreed, it would pass the same token and does not make any difference on the output, but since this is misleading, I think we should remove the token field
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.
cool
client_cert: "{{ kafka_broker_cert_path if send_client_cert|bool else omit }}" | ||
client_key: "{{ kafka_broker_key_path if send_client_cert|bool else omit }}" | ||
body_format: json | ||
# Cant use internal listener as register cluster doesnt support SSL protocol |
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.
could you share any reference/docs for this conclusion? if we are using mds endpoint, SSL should be sufficient afaik. SASL_SSL was needed for kafka rest api
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.
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.
Also to clarify the SASL_SSL protocol is needed in the body of this post request as the value of "protocol" parameter and doesnt mean the request to /registry/clusters endpoint must have some sasl credentials
@@ -9,6 +9,8 @@ | |||
oauth_password: "{{ kafka_connect_oauth_password }}" | |||
ldap_user: "{{ kafka_connect_ldap_user }}" | |||
ldap_password: "{{ kafka_connect_ldap_password }}" | |||
mds_mtls_client_cert: "{{ kafka_connect_cert_path }}" |
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.
the variable name is misleading, we are not sending mds certs here, we send component wise cert for each component. Please rename it to mtls_client_cert
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.
by mds_mtls_client_cert i meant cert of MDS's client for mtls. and not cert of MDS. But I get that this might be misleading name. mtls_client_cert
makes sense. will change this
@@ -212,9 +212,12 @@ ssl_enabled: false | |||
### Set this variable to customize expiration days for certificate authority. Applies for all components of Confluent Platform. | |||
certificate_authority_expiration_days: 365 | |||
|
|||
### Boolean to enable mTLS Authentication on all components. Configures all components to use mTLS for authentication into Kafka | |||
### Boolean to enable mTLS Authentication on all components. Configures all components to use mTLS for authentication into Kafka. No need to define this anymore, use ssl_client_authentication instead. |
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 can instead mark this variable as deprecated
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 had a little confusion there as internally at most places we do continue to use this variable. Just at the end when we need the value we use the new variable. Even in that case should we mark it deprecated ?
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.
from customer's pov, this will still count as deprecated. It's more of an internal variable for us in that case. Also, if we do mark it as deprecated, let's updated all the molecule scenarios to replace this bool with ssl_client_authetication (even for non rbac mtls)
@@ -700,10 +734,20 @@ kafka_broker_custom_client_properties: {} | |||
### Boolean to enable the embedded rest proxy within Kafka. NOTE- Embedded Rest Proxy must be enabled if RBAC is enabled and Confluent Server must be enabled | |||
kafka_broker_rest_proxy_enabled: "{{confluent_server_enabled and not ccloud_kafka_enabled }}" | |||
|
|||
### Property of ERP as MDS client. Can be set to true when ssl_client_authentication is not none. When set to true will not send oauth token or ldap creds to MDS even when MDS server has support for accepting them. Keeping false means if MDS has oauth and mtls support client will send both oauth token and cert | |||
kafka_broker_rest_proxy_to_mds_send_certs_only: false |
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 variable naming should be better, it's too long for a variable name. Please rename to something like rp_mds_certs_only
and add the details in the docs
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 thought all variables for ERP have the prefix kafka_broker_rest_proxy_ so I also kept the same prefix. Length wise yes its long but there are many longer variables.
Added some comments, other than those, Open question:
Suggestions:
|
The doc reference https://confluentinc.atlassian.net/wiki/spaces/OAAC/pages/3653567526/mTLS+with+RBAC+Gotchas
The variable name should make the purpose clear. If we shorten the name to _mds_certs_only by this name it cant convey full meaning. How about the name
Sure will add that
|
Description
This Pr aims to add support for RBAC over mTLS.
The includes changes for
Fixes # (issue)
Type of change
How Has This Been Tested?
zookeeper
kraft
Checklist: