Principal exclusion feature doesn't work if the principal contains a comma (such as Distinguished Name)
k-wall opened this issue · 16 comments
The plugin's configuration supports client.quota.callback.static.excluded.principal.name.list which allows the user to pass a list of principals that should not be subject to the quota. The entries in the list are comma separated. This scheme does not allow for the possibility that the principal name may include a comma, as is the case with DNs.
For instance, one might want to exclude mirrormaker from the quota.
client.quota.callback.static.excluded.principal.name.list=CN=my-cluster-cruise-control,O=io.strimzi
This will not have the intended effect as CN=my-cluster-cruise-control and O=io.strimzi will be interpreted as referring to separate principals. There's no way to escape the comma (the plugin leans on ConfigDef#parseType to marshal a string into a list, but there is no escape mechanism).
taking inspiration from kafka.security.authorizer.AclAuthorizer#configure, we see that Kafka itself passes principal lists as ; separated, perhaps for this very reason.
Edit: https://issues.apache.org/jira/browse/KAFKA-2838 it seems Kafka itself had exactly this issue.
I suspect this ought to be triaged. I think it'll need an API change to fix and a plan to deal with the deprecated property.
I would probably just change it and do a patch release 🤷. WDYT @ppatierno?
@k-wall I am not sure about how the deprecation would work. When we deprecate something, we add the new thing while keeping the old one for some releases.
Are you going to support comma and semi colon for some time? But documenting that the comma one won't work with DNs?
@scholzj regarding just fixing with a patch release, aren't we breaking all deployment where users are using quota plugin with the current comma separated list? Isn't it against our usual approach.
Are you going to support comma and semi colon for some time? But documenting that the comma one won't work with DNs?
That's certainly how I was imagining it would work:
- have two lists in the config
- parse each list
- Merge the results
- Continue on as normal
I suspect that merging is probably overkill users would either deploy the csv version or semi colon version but not both.
I don't expect there is any need to validate the lists are distinct.
@scholzj regarding just fixing with a patch release, aren't we breaking all deployment where users are using quota plugin with the current comma separated list? Isn't it against our usual approach.
Yes. But do you actually know anyone using it?
What are the other options?
- Deprecate the existing field and add a new one with
;and carry both of them for god knows how long? (I assume this is what @k-wall meant) - Add a new option to configure the separator that would default to
,but can be changed to;. And you would need to carry that forever.
I'm not sure these are worth the effort given the situation and what we know about its users. So I personally would just call it a bug and fix it 🤷. I mean, we just completely changed it between 0.2.0 and 0.3.0, so does change from , to ; really matter?
I meant introduce a new property semi-colon separated. The old property would only be consulted if the new one is absent. Mark the old property as deprecated and write a warning to the log if it used.
I've been thinking about this overnight. DNs are a complete minefield.
- splitting on semicolons isn't actually completely safe. Semicolons may appear in the DN (escaped).
- DNs aren't case sensitive (https://www.rfc-editor.org/rfc/rfc2251.txt) and string comparison isn't a reliable way to test equality.
That said, I don't think quota plugin should care about either of these. AFAICS Kafka's own super.users property handling suffers exactly the same issues. If it is good enough for Kafka, I think it is okay for plugin to follow the same approach.
I am saying I think the simple split on semicolon is good enough.
There is also small change to the way how the plugin is enabled and the default check interval.
And as @scholzj mentioned, the plugin was completely changed in 0.3.0.
So how about changing the separator to ; from , and mention it in the documentation (release notes) and maybe release it under 0.4.0? That way it will not be just patch release, but new release with new stuff and fixes?
curious to know what others think at my observation above that switching separate wouldn't be a complete solution. do we care?
curious to know what others think at my observation above that switching separate wouldn't be a complete solution. do we care?
We rely on the same for the Authorization plugins. So I guess you are right that it might not be sufficient. But we get way more screwed up by this in the authorization plugins than here. So if Kafka is happy with this, not sure I see it as a problem.
If this is free, I would like to pick it and fix it. Thanks
Based on the discussion above, what do you think about my proposal:
So how about changing the separator to ; from , and mention it in the documentation (release notes) and maybe release it under 0.4.0? That way it will not be just patch release, but new release with new stuff and fixes?
Does it makes sense or is it a bug fix that should be part of 0.3.1?
Also about the way how it should be fixed:
Should we have two options how to configure the principals (using commas and semicolons and merge those two)? I think that having support for both would cause issue like:
- specifying just one principal using DN ->
client.quota.callback.static.excluded.principal.name.list=CN=my-cluster-cruise-control,O=io.strimzi-> the first mechanism will split it by;where there will be nothing, the second will split it by,which will do the exact thing as now - combining the separators -
client.quota.callback.static.excluded.principal.name.list=foo;bar,baz
Having separate config option for the comma and semicolon separators would fix these, but we had to deprecate the comma one and then, after release (few releases) we would need to delete it. But based on the cadence of the releases of the plugin it's unknown when it would be.
Or finally, just change it to semicolon separator and mention it in the README.md ?
@ppatierno @scholzj @k-wall @SamBarker WDYT?
If you change it that way and treat it as a bug, it should be a patch release. This should be probably discussed on a community call or through a proposal to get a clear idea about the thoughts and result.
If you change it that way and treat it as a bug, it should be a patch release. This should be probably discussed on a community call or through a proposal to get a clear idea about the thoughts and result.
I will add it to the agenda of next community call. If needed, I can create a proposal.
Discussed on the community call on 18.4.: This should be treated as a bug. We should change the separator for the existing option and do a 0.3.1 patch release.