kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boyang Chen <reluctanthero...@gmail.com>
Subject Re: [DISCUSS] KIP-590: Redirect Zookeeper Mutation Protocols to The Controller
Date Wed, 29 Apr 2020 16:20:47 GMT
Thanks for the proposed idea Sonke. I reviewed it and had some offline
discussion with Colin, Rajini and Mathew.

We do need to add serializability to the PrincipalBuilder interface, but we
should not make any default implementation which could go wrong and messy
up with the security in a production environment if the user neglects it.
Instead we need to make it required and backward incompatible. So I
integrated your proposed methods and expand the Envelope RPC with a couple
of more fields for audit log purpose as well.

Since the KafkaPrincipal builder serializability is a binary incompatible
change, I propose (also stated in the KIP) the following implementation
plan:

   1. For next *2.x* release:
      1. Get new admin client forwarding changes
      2. Get the Envelope RPC implementation
      3. Get the forwarding path working and validate the function with
      fake principals in testing environment, without actual triggering in the
      production system
   2. For next *3.0 *release:
      1. Introduce serializability to PrincipalBuilder
      2. Turn on forwarding path in production and perform end-to-end
      testing


I think this is the first time we need to introduce a KIP without having it
fully accepted in next release. Let me know if this sounds reasonable.

On Fri, Apr 24, 2020 at 1:00 AM Sönke Liebau
<soenke.liebau@opencore.com.invalid> wrote:

> 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 <reluctanthero104@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.liebau@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 <cmccabe@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.liebau@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 <wangguoz@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 <
> > > > reluctanthero104@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 <
> > cmccabe@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
<
> > > > gwen@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 <
> > > > > > > reluctanthero104@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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message