Skip to content
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

Restrict AdminClient permission for KafkaHealthIndicator #423

Closed
xiabai84 opened this issue Oct 13, 2021 · 4 comments · Fixed by #810
Closed

Restrict AdminClient permission for KafkaHealthIndicator #423

xiabai84 opened this issue Oct 13, 2021 · 4 comments · Fixed by #810
Assignees
Labels
status: pr submitted A pull request has been submitted for the issue type: enhancement New feature or request

Comments

@xiabai84
Copy link

Expected Behavior

By enabling kafka.health.check Micronaut should only get the essential information, which is used for making health check.

Actual Behaviour

As request result from AdminClient the DescribeClusterResult also contains sensitive information like ACLs, which could lead to a security issue. This request should be restricted by default configuration.

Steps To Reproduce

  1. Set ACLs for client user and kafka topics.
  2. Revoke DescribeCluster permission from client user.
  3. Enable kafka.health.check
  4. Start applicaiton

Environment Information

  • MacOS
  • JDK 11

Example Application

No response

Version

3.0.0

@jonas-grgt
Copy link

In a way I think this is in a way related to #440

I was thinking of introducing different health strategies through a property.

If your user does not have DescribeClusterResult ACL how do your propose to do the health check ? Also what risks are there if the cluster ACLs are fetched upon receiving the DescribeClusterResult result ?

@xiabai84
Copy link
Author

xiabai84 commented Dec 20, 2021

Hey @jonasgeiregat,
I follow the point of issue #440 . For me min.insync.replicas would be a good reason to send out a kind of alert, if the topic can't reach the predefined number of replicas after certain time.

Of course this doesn't mean that we don't need "replication.factor" at all, but we don't have any other options in Kafka... In my opinion this shouldn't be placed as default configuration.

To your answer:
DescribeClusterResult requires a cluster wide permission in a multi tenant kerberized kafka cluster.
And the cluster administrator normally don't want to assign such permission to each tenant/client, if they even don't know each other. And for a client, which only perform read or write task, this DescribeCluster could be just too much.
In Germany we call it "Regulatorik" (which is a part of GDPR in EU), this is something that people don't always understand, but must follow anyway...

Well, that was a lot of political blabla... I think the proper way is to overwrite KafkaHealthIndicator, but as a easy workaround without any coding, we could just enable common micronaut health endpoints:

endpoints:
  health:
    details-visible: ANONYMOUS
    enabled: true
    sensitive: false

and set "kafka.streams.health.enabled" to "false" and "kafka.streams.health.streams.enabled" to "true" in application.yml. With this solution we can at least still have a monitoring on streams health.

kafka:
  ....
  streams:
      health:
        enabled: false
        streams:
          enabled: true
      default:
        auto.offset.reset: earliest
        processing.guarantee: exactly_once
        sasl.jaas.config: xxxxxxx
  ...

@jonas-grgt
Copy link

jonas-grgt commented Dec 24, 2021

I'm not sure giving a client the DescribeClusterResult ACL would violate one of the GDPR rules. As no personal or identifiable information seems to be exposed by the DescribeClusterResult ACL. But then again I'm not a GDPR expert.

In any case I would propose to have a set of HealthIndicators available, the current one still being the default by through configuration could be overridden.

The following would enable a kafka HealthIndicator based upon the min.insync.replicas cluster property.

This then would also solve #440

kafka:
  health:
    enabled: true
    strategy: insync_replicas

If the maintainers agree with such a solutions or anything similar I would love to add this as a contribution

@graemerocher
Copy link
Contributor

PRs welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: pr submitted A pull request has been submitted for the issue type: enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants