After thinking on this a little bit, maybe this would be an option:
add default methods serialize and deserialize to the KafkaPrincipalBuilder
interface, these could be very short:
default String serialize(KafkaPrincipal principal) {
return principal.toString();
}
default KafkaPrincipal deserialize(String principalString) {
return SecurityUtils.parseKafkaPrincipal(principalString);
}
This would mean that all existing implementations of that interface
are unaffected, as this code is pretty much what is currently being
used when their principals need to be serialized.
But it offers people using custom principals the chance to override
these methods and ensure that all information gets serialized for
delegation tokens or request forwarding.
Wherever we need to de/serialize principals (for example in the
DelegationTokenManager [1]) we obtain an instance of the configured
PrincipalBuilder class and use that to do the actual work.
What do you think?
Best regards,
Sönke
[1]
https://github.com/apache/kafka/blob/33d06082117d971cdcddd4f01392006b543f3c01/core/src/main/scala/kafka/server/DelegationTokenManager.scala#L122
On Thu, 23 Apr 2020 at 17:42, Boyang Chen <reluctanthero...@gmail.com>
wrote:
> Thanks all,
>
> IIUC, the necessity of doing the audit log on the controller side is
> because we need to make sure the authorized resource modifications
> eventually arrive on the target broker side, but is that really necessary?
>
> I'm thinking the possibility of doing the audit log on the forwarding
> broker side, which could simplify the discussion of principal serialization
> here. The other option I could think of is to serialize the entire audit
> log message if we were supposed to approve, and pass it as part of the
> Envelope.
>
> Let me know if you think either of these approaches would work.
>
> On Thu, Apr 23, 2020 at 7:01 AM Sönke Liebau
> <soenke.lie...@opencore.com.invalid> wrote:
>
> > I agree that this would be useful to have and shouldn't create issues in
> > 99% of all cases. But it would be a breaking change to a public API.
> > I had a quick look at the two large projects that come to mind which
> might
> > be affected: Ranger and Sentry - both seem to operate directly with
> > KafkaPrincipal instead of subclassing it. But anybody who
> > extended KafkaPrincipal would probably need to update their code..
> >
> > Writing this sparked the thought that this issue should also concern
> > delegation tokens, as Principals need to be stored/sent around for those
> > too.
> > Had a brief look at the code and for Delegation Tokens we seem to use
> > SecurityUtils#parseKafkaPrincipal[1] which would ignore any additional
> > information from custom Principals.
> >
> > We'll probably want to at least add a note on that to the docs - unless
> it
> > is there already, I've only looked for about 30 seconds..
> >
> > Best regards,
> > Sönke
> >
> >
> > [1]
> >
> >
> https://github.com/apache/kafka/blob/e9fcfe4fb7b9ae2f537ce355fe2ab51a58034c64/clients/src/main/java/org/apache/kafka/common/utils/SecurityUtils.java#L52
> >
> > On Thu, 23 Apr 2020 at 14:35, Colin McCabe <cmcc...@apache.org> wrote:
> >
> > > Hmm... Maybe we need to add some way to serialize and deserialize
> > > KafkaPrincipal subclasses to/from string. We could add a method to
> > > KafkaPrincipalBuilder#deserialize and a method
> KafkaPrincipal#serialize,
> > I
> > > suppose.
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Thu, Apr 23, 2020, at 02:14, Tom Bentley wrote:
> > > > Hi folks,
> > > >
> > > > Colin wrote:
> > > >
> > > > > The final broker knows it can trust the principal name in the
> > envelope
> > > > > (since EnvelopeRequest requires CLUSTERACTION on CLUSTER). So it
> can
> > > use
> > > > > that principal name for authorization (given who you are, what can
> > you
> > > > > do?) The forwarded principal name will also be used for logging.
> > > > >
> > > >
> > > > My understanding (and I'm happy to be corrected) is that a custom
> > > > authoriser might rely on the KafkaPrincipal instance being a subclass
> > of
> > > > KafkaPrincipal (e.g. the subclass has extra fields with the
> principal's
> > > > "roles"). So you can't construct a KafkaPrinicpal on the controller
> > which
> > > > would be guaranteed to work for arbitrary authorizers. You have to
> > > perform
> > > > authorization on the first broker (rejecting some of the batched
> > > requests),
> > > > forward the authorized ones to the controller, which then has to
> trust
> > > that
> > > > the authorization has been done and make the ZK writes without
> > > > authorization. Because the controller cannot invoke the authorizer
> that
> > > > means that the authorizer audit logging (on the controller) would not
> > > > include these operations. But they would be in the audit logging from
> > the
> > > > original broker, so the information would not be lost.
> > > >
> > > > There's also a problem with using the constructed principal for other
> > > > logging (i.e. non authorizer logging) on the controller: There's
> > nothing
> > > > stopping a custom KafkaPrincipal subclass from overriding toString()
> to
> > > > return something different from "${type}:${name}". If you're
> building a
> > > > "fake" KafkaPrincipal on the controller from the type and name then
> its
> > > > toString() would be "wrong". A solution to this would be to also
> > > serialize
> > > > the toString() into the envelope and have some ProxiedKafkaPrincipal
> > > class
> > > > which you instantiate on the controller which has overridden toString
> > to
> > > > return the right value. Obviously you could optimize this using an
> > > optional
> > > > field so you only serialize the toString() if it differed from the
> > value
> > > > you'd get from KafkaPrincipal.toString(). Maybe non-audit logging
> using
> > > the
> > > > wrong string value of a principal is sufficiently minor that this
> isn't
> > > > even worth trying to solve.
> > > >
> > > > Kind regards,
> > > >
> > > > Tom
> > > >
> > > >
> > > > On Wed, Apr 22, 2020 at 10:59 PM Sönke Liebau
> > > > <soenke.lie...@opencore.com.invalid> wrote:
> > > >
> > > > > Hi Colin,
> > > > >
> > > > > thanks for your summary! Just one question - and I may be missing
> an
> > > > > obvious point here..
> > > > > You write:
> > > > >
> > > > > "The initial broker should do authentication (who are you?) and
> come
> > up
> > > > > with a principal name. Then it creates an envelope request, which
> > will
> > > > > contain that principal name, and sends it along with the unmodified
> > > > > original request to the final broker. [... ] The final broker
> knows
> > > it
> > > > > can trust the principal name in the envelope (since EnvelopeRequest
> > > > > requires CLUSTERACTION on CLUSTER). So it can use that principal
> > name
> > > for
> > > > > authorization (given who you are, what can you do?) "
> > > > >
> > > > > My understanding is, that you don't want to serialize the Principal
> > > (due to
> > > > > the discussed issues with custom principals) but reduce the
> principal
> > > down
> > > > > to a string representation that would be used for logging and
> > > > > authorization?
> > > > > If that understanding is correct then I don't think we could use
> the
> > > > > regular Authorizer on the target broker, because that would need
> the
> > > actual
> > > > > principal object to work on.
> > > > >
> > > > > Also, a thought that just occurred to me, we might actually need to
> > log
> > > > > different principal strings for the case of queries like
> AlterConfigs
> > > > > (mentioned by Rajini) which may contain multiple resources. Take an
> > > LDAP
> > > > > authorizer that grants access based on group membership - the same
> > > > > alterconfig request may contain resources that are authorized based
> > on
> > > > > group1 as well as resources authorized based on membership in
> group 2
> > > ..
> > > > > And in all cases we'd need to log the specific reason I think..
> > > > >
> > > > > Basically I think that we might have a hard time properly
> authorizing
> > > and
> > > > > logging without being able to forward the entire principal.. but
> > > again, I
> > > > > might be heading down an entirely wrong path here :)
> > > > >
> > > > > Best regards,
> > > > > Sönke
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On Wed, 22 Apr 2020 at 23:13, Guozhang Wang <wangg...@gmail.com>
> > > wrote:
> > > > >
> > > > > > Colin, Boyang: thanks for the updates, I agree that an
> > > EnvelopeRequest
> > > > > > would be a less vulnerable approach than optional fields, and I'm
> > > just
> > > > > > wondering if we would keep the EnvelopeRequest for a long time. I
> > was
> > > > > > thinking that, potentially if we require clients to be on newer
> > > version
> > > > > > when talking to a 3.0+ (assuming 3.0 is the bridge release)
> > brokers,
> > > then
> > > > > > we do not need to keep this code for too long, but I think that
> > > would be
> > > > > a
> > > > > > very hasty compatibility breaking so maybe we indeed need to keep
> > > this
> > > > > > forwarding mechanism many years.
> > > > > >
> > > > > > Regarding future use cases, I think the example that Boyang
> > > mentioned may
> > > > > > not be very practical honestly, because when there's a
> connectivity
> > > > > issue,
> > > > > > it is either a network partition between "controller, A | B", or
> > > > > > "controller | A, B". In other words, if the controller can talk
> to
> > A,
> > > > > then
> > > > > > very likely A would not be able to talk to B either... anyways,
> > > since the
> > > > > > forwarding would be there for a sufficiently long time, I think
> > > keeping
> > > > > the
> > > > > > additional envelope makes sense.
> > > > > >
> > > > > >
> > > > > > Guozhang
> > > > > >
> > > > > > On Wed, Apr 22, 2020 at 1:47 PM Boyang Chen <
> > > reluctanthero...@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Thanks Colin for the summary! And Guozhang, regarding the
> future
> > > use
> > > > > > cases,
> > > > > > > consider a scenario where there are temporary connectivity
> issue
> > > > > between
> > > > > > > controller to a fellow broker A, the controller could then
> > leverage
> > > > > > another
> > > > > > > healthy broker B to do a forwarding request to A in order to
> > > maintain a
> > > > > > > communication.
> > > > > > >
> > > > > > > Even for KIP-590 scope, the forwarding mechanism shall not be
> > > transit
> > > > > as
> > > > > > we
> > > > > > > do need to support older version of admin clients for a couple
> of
> > > years
> > > > > > > IIUC.
> > > > > > >
> > > > > > > Boyang
> > > > > > >
> > > > > > > On Wed, Apr 22, 2020 at 1:29 PM Colin McCabe <
> cmcc...@apache.org
> > >
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > I guess the way I see this working is that the request gets
> > sent
> > > from
> > > > > > the
> > > > > > > > client, to the initial broker, and then forwarded to the
> final
> > > > > broker.
> > > > > > > >
> > > > > > > > The initial broker should do authentication (who are you?)
> and
> > > come
> > > > > up
> > > > > > > > with a principal name. Then it creates an envelope request,
> > > which
> > > > > will
> > > > > > > > contain that principal name, and sends it along with the
> > > unmodified
> > > > > > > > original request to the final broker. (I agree with Tom and
> > > Rajini
> > > > > > that
> > > > > > > we
> > > > > > > > can't forward the information needed to do authentication on
> > the
> > > > > final
> > > > > > > > broker, but I also think we don't need to, since we can do it
> > on
> > > the
> > > > > > > > initial broker.)
> > > > > > > >
> > > > > > > > The final broker knows it can trust the principal name in the
> > > > > envelope
> > > > > > > > (since EnvelopeRequest requires CLUSTERACTION on CLUSTER).
> So
> > > it can
> > > > > > use
> > > > > > > > that principal name for authorization (given who you are,
> what
> > > can
> > > > > you
> > > > > > > > do?) The forwarded principal name will also be used for
> > logging.
> > > > > > > >
> > > > > > > > One question is why we need an EnvelopeRequest. Well, if we
> > > don't
> > > > > have
> > > > > > > an
> > > > > > > > EnvelopeRequest, we need somewhere else to put the forwarded
> > > > > principal
> > > > > > > > name. I don't think overriding an existing field (like
> > > clientId) is
> > > > > a
> > > > > > > good
> > > > > > > > option for this. It's messy, and loses information. It also
> > > raises
> > > > > > the
> > > > > > > > question of how the final broker knows that the clientId in
> the
> > > > > > received
> > > > > > > > message is not "really" a clientId, but is a principal name.
> > > Without
> > > > > > an
> > > > > > > > envelope, there's no way to clearly mark a request as
> > forwarded,
> > > so
> > > > > > > there's
> > > > > > > > no reason for the final broker to treat this differently
> than a
> > > > > regular
> > > > > > > > clientId (or whatever).
> > > > > > > >
> > > > > > > > We talked about using optional fields to contain the
> forwarded
> > > > > > principal
> > > > > > > > name, but this is also messy and potentially dangerous.
> Older
> > > > > brokers
> > > > > > > will
> > > > > > > > simply ignore the optional fields, which could result in them
> > > > > executing
> > > > > > > > operations as the wrong principal. Of course, this would
> > > require a
> > > > > > > > misconfiguration in order to happen, but it still seems
> better
> > > to set
> > > > > > up
> > > > > > > > the system so that this misconfiguration is detected, rather
> > than
> > > > > > > silently
> > > > > > > > ignored.
> > > > > > > >
> > > > > > > > It's true that the need for forwarding is "temporary" in some
> > > sense,
> > > > > > > since
> > > > > > > > we only need it for older clients. However, we will want to
> > > support
> > > > > > > these
> > > > > > > > older clients for many years to come.
> > > > > > > >
> > > > > > > > I agree that the usefulness of EnvelopeRequest is limited by
> it
> > > > > being a
> > > > > > > > superuser-only request at the moment. Perhaps there are some
> > > changes
> > > > > > to
> > > > > > > > how custom principals work that would allow us to get around
> > > this in
> > > > > > the
> > > > > > > > future. We should think about that so that we have this
> > > > > functionality
> > > > > > in
> > > > > > > > the future if it's needed.
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > >
> > > > > > > > On Wed, Apr 22, 2020, at 11:21, Guozhang Wang wrote:
> > > > > > > > > Hello Gwen,
> > > > > > > > >
> > > > > > > > > The purpose here is for maintaining compatibility old
> > clients,
> > > who
> > > > > do
> > > > > > > not
> > > > > > > > > have functionality to do re-routing admin requests
> > themselves.
> > > New
> > > > > > > > clients
> > > > > > > > > can of course do this themselves by detecting who's the
> > > controller.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Hello Colin / Boyang,
> > > > > > > > >
> > > > > > > > > Regarding the usage of the envelope, I'm curious what are
> the
> > > > > > potential
> > > > > > > > > future use cases that would require request forwarding and
> > > hence
> > > > > > > envelope
> > > > > > > > > would be useful? Originally I thought that the forwarding
> > > mechanism
> > > > > > is
> > > > > > > > only
> > > > > > > > > temporary as we need it for the bridge release, and moving
> > > forward
> > > > > we
> > > > > > > > will
> > > > > > > > > get rid of this to simplify the code base. If we do have
> > valid
> > > use
> > > > > > > cases
> > > > > > > > in
> > > > > > > > > the future which makes us believe that request forwarding
> > would
> > > > > > > actually
> > > > > > > > be
> > > > > > > > > a permanent feature retained on the broker side, I'm on
> board
> > > with
> > > > > > > adding
> > > > > > > > > the envelope request protocol.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Guozhang
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Wed, Apr 22, 2020 at 8:22 AM Gwen Shapira <
> > > g...@confluent.io>
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hey Boyang,
> > > > > > > > > >
> > > > > > > > > > Sorry if this was already discussed, but I didn't see
> this
> > as
> > > > > > > rejected
> > > > > > > > > > alternative:
> > > > > > > > > >
> > > > > > > > > > Until now, we always did client side routing - the client
> > > itself
> > > > > > > found
> > > > > > > > the
> > > > > > > > > > controller via metadata and directed requests
> accordingly.
> > > > > Brokers
> > > > > > > that
> > > > > > > > > > were not the controller, rejected those requests.
> > > > > > > > > >
> > > > > > > > > > Why did we decide to move to broker side routing? Was the
> > > > > > client-side
> > > > > > > > > > option discussed and rejected somewhere and I missed it?
> > > > > > > > > >
> > > > > > > > > > Gwen
> > > > > > > > > >
> > > > > > > > > > On Fri, Apr 3, 2020, 4:45 PM Boyang Chen <
> > > > > > reluctanthero...@gmail.com
> > > > > > > >
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hey all,
> > > > > > > > > > >
> > > > > > > > > > > I would like to start off the discussion for KIP-590, a
> > > > > follow-up
> > > > > > > > > > > initiative after KIP-500:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-590%3A+Redirect+Zookeeper+Mutation+Protocols+to+The+Controller
> > > > > > > > > > >
> > > > > > > > > > > This KIP proposes to migrate existing Zookeeper
> mutation
> > > paths,
> > > > > > > > including
> > > > > > > > > > > configuration, security and quota changes, to
> > > controller-only
> > > > > by
> > > > > > > > always
> > > > > > > > > > > routing these alterations to the controller.
> > > > > > > > > > >
> > > > > > > > > > > Let me know your thoughts!
> > > > > > > > > > >
> > > > > > > > > > > Best,
> > > > > > > > > > > Boyang
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > -- Guozhang
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > -- Guozhang
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Sönke Liebau
> > > > > Partner
> > > > > Tel. +49 179 7940878
> > > > > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel -
> Germany
> > > > >
> > > >
> > >
> >
> >
> > --
> > Sönke Liebau
> > Partner
> > Tel. +49 179 7940878
> > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
> >
>
--
Sönke Liebau
Partner
Tel. +49 179 7940878
OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
