cp-ansible icon indicating copy to clipboard operation
cp-ansible copied to clipboard

[ANSIENG-3807] | Rbac over mTLS

Open rrbadiani opened this issue 1 year ago • 2 comments

Description

This Pr aims to add support for RBAC over mTLS. The includes changes for

  • MDS
  • Broker
  • Controller
  • SR
  • ERP
  • RP
  • Connect
  • KSQL
  • C3
  • Using cert based token retrival
  • Extracting Principal from certs to assign role bindings
  • Molecule tests
  • Setting Principal Mapping Rules
  • Setting impersonation super users for principal propogation

Replicator changes will be raised in seperate PR

Fixes # (issue)

Type of change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [X] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

How Has This Been Tested?

zookeeper

kraft

Checklist:

  • [ ] Any variable/code changes have been validated to be backwards compatible (doesn't break upgrade)
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] If required, I have ensured the changes can be discovered by cp-ansible discovery codebase
  • [ ] My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] My changes generate no new warnings
  • [ ] Any dependent changes have been merged and published in downstream modules

rrbadiani avatar Oct 18 '24 13:10 rrbadiani

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Oct 18 '24 13:10 CLAassistant

:tada: All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

Added some comments, other than those, Open question:

  1. Why are we using SASL_SSL listener with MDS /register endpoint? Please share the doc reference

Suggestions:

  1. Please rename the <component>_to_mds_send_certs_only to a better variable name for code quality. Something like rp_mds_certs_only, connect_mds_certs_only and add the description in docs.md
  2. Please add creating connectors in the molecule tests to validate Connect server mtls and dual auth. Similarly, please add kafka produce/consumer commands in verify.yml over different listeners to validate the listeners.
  3. Please add check to validate if mds_super_user_external_cert_path is defined in a 2 cluster setup. Adding to this, the variable should be renamed to mds_superuser_cert_path and kafka_broker_cert_path should default to this and its principal should be added to super.users. This is similar to the current flow where customer defines mds_super_user or oauth_superuser_principal. This super user principal is the principal that can provide rolebindings and which may or may not be equal to kafka principal, specially in external mds setup.

mansisinha avatar Oct 25 '24 11:10 mansisinha

  1. Why are we using SASL_SSL listener with MDS /register endpoint? Please share the doc reference

The doc reference https://confluentinc.atlassian.net/wiki/spaces/OAAC/pages/3653567526/mTLS+with+RBAC+Gotchas

Suggestions:

  1. Please rename the <component>_to_mds_send_certs_only to a better variable name for code quality. Something like rp_mds_certs_only, connect_mds_certs_only and add the description in docs.md

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 <component>_mds_cert_auth_only ?

  1. Please add creating connectors in the molecule tests to validate Connect server mtls and dual auth. Similarly, please add kafka produce/consumer commands in verify.yml over different listeners to validate the listeners.

Sure will add that

  1. Please add check to validate if mds_super_user_external_cert_path is defined in a 2 cluster setup. Adding to this, the variable should be renamed to mds_superuser_cert_path and kafka_broker_cert_path should default to this and its principal should be added to super.users. This is similar to the current flow where customer defines mds_super_user or oauth_superuser_principal. This super user principal is the principal that can provide rolebindings and which may or may not be equal to kafka principal, specially in external mds setup.
  • Yes agreed validation should be added and principal of this cert should be added to super.user of 2nd cluster
  • On default value of kafka broker cert path being same as mds super user cert. That part I don't get it. Like in case when there are 2 clusters that makes sense for custom certs. but in case when there is single cluster this variable cant be defined or even for 2nd clusters in cases of provided keystore,truststore this cant be defined.

rrbadiani avatar Oct 25 '24 13:10 rrbadiani