That's an interesting approach - it looks good to me at first glance.
Should we consider creating a new KIP to initiate a proper discussion?
People might miss the discussion here due to the KIP title
On Thu, Feb 21, 2019 at 6:52 PM Ron Dagostino <rndg...@gmail.com> wrote:
> My gut says a building block that takes a configuration as input and
> exposes a representation of the potential issues found would be a good
> starting point. Then this could be leveraged as part of a command line
> tool that emits the data to stdout, or it could be used during broker
> startup such that any potential issues found are logged (or, more
> aggressively, cause the broker to not start if that is desired). The
> return could be something as simple as an
> EnumSet<PotentialConfigSecurityIssue>:
>
> enum PotentialConfigSecurityIssue {
> OAUTHBEARER_UNSECURE_TOKENS_ALLOWED, // when OAUTHBEARER is in
> sasl.enabled.mechanisms but server callback handler is the unsecured
> validator
> PLAINTEXT_LISTENER, // if there is a PLAINTEXT listener
> SSL_CLIENT_AUTH_NOT_REQUIRED, // when SSL listener is enabled but
> ssl.client.auth is not required
> ETC
> }
>
> Or it could be a Set of instances where each instance implements an
> interface that returns one of these enum values via a getType() method;
> this allows each instance to potentially be a different class and hold
> additional information (like the PLAINTEXT listener's address in case that
> is something that requires additional validation, though how the validation
> would be plugged in is a separate issue that I can't figure out at the
> moment).
>
> It feels to me that the EnumSet<> approach is the simplest place to start,
> and it might be best to allow anything more complicated to fall out as
> experience with the simple starting point builds. Making the EnumSet<>
> part of the public API would then allow anyone to build upon it as they see
> fit.
>
> Ron
>
>
>
> On Thu, Feb 21, 2019 at 12:57 PM Stanislav Kozlovski <
> stanis...@confluent.io>
> wrote:
>
> > I think that is a solid idea. The closest thing I can think of is David's
> > PR about duplicate config key logging -
> > https://github.com/apache/kafka/pull/6104
> >
> > We could either continue the pattern of checking on broker startup and
> > logging a warning or create a separate tool that analyzes the configs.
> >
> > On Thu, Feb 21, 2019 at 3:16 PM Rajini Sivaram <rajinisiva...@gmail.com>
> > wrote:
> >
> > > Hi Ron,
> > >
> > > Yes, a security sanity check tool could be quite useful. Let's see what
> > > others think.
> > >
> > > Thanks,
> > >
> > > Rajini
> > >
> > >
> > > On Thu, Feb 21, 2019 at 1:49 PM Ron Dagostino <rndg...@gmail.com>
> wrote:
> > >
> > > > HI Rajini and Stan. Thanks for the feedback.
> > > >
> > > > Stan, regarding the proposed config name, I couldn't think of
> anything
> > > so I
> > > > just threw in something outrageous in the hopes that it would give a
> > > sense
> > > > of what I was talking about while perhaps making folks chuckle a bit.
> > > >
> > > > Rajini, I definitely see your point. It probably doesn't make sense
> to
> > > > address this one particular issue (if we can even consider it an
> issue)
> > > > when in fact it is part of a pattern that has been explicitly agreed
> > upon
> > > > as being appropriate.
> > > >
> > > > Maybe a security sanity check tool that scans the config and flags
> any
> > of
> > > > these items you mentioned, plus the OAUTHBEARER one and any others we
> > can
> > > > think of, would be useful? That way the out-of-the-box experience
> can
> > > > remain straightforward while some of the security risk that comes as
> a
> > > > byproduct can be mitigated.
> > > >
> > > > Ron
> > > >
> > > > Ron
> > > >
> > > > On Thu, Feb 21, 2019 at 8:02 AM Rajini Sivaram <
> > rajinisiva...@gmail.com>
> > > > wrote:
> > > >
> > > > > Hi Ron,
> > > > >
> > > > > Thanks for the KIP. How is this different from other scenarios:
> > > > >
> > > > > 1. Our default is to use a PLAINTEXT listener. If you forget to
> > > change
> > > > > that, anyone has access to your cluster
> > > > > 2. You may add a PLAINTEXT listener to the list of listeners in
> > > > > production. May be you intended it for an interface that was
> > > protected
> > > > > using network segmentation, but entered the wrong address.
> > > > > 3. You are very security conscious and add an SSL listener. You
> > must
> > > > be
> > > > > secure now right? Our default is `ssl.client.auth=none`, which
> > means
> > > > any
> > > > > one can connect.
> > > > > 4. You use the built-in insecure PLAIN callback that stores
> > > cleartext
> > > > > passwords on the file system. Or enable SASL/PLAIN without SSL.
> > > > >
> > > > > At the moment, our defaults are intended to make it easy to get
> > started
> > > > > quickly. If we want to make brokers secure by default, we need an
> > > > approach
> > > > > that works across the board. I am not sure we have a specific issue
> > > with
> > > > > OAUTHBEARER apart from the fact that we don't provide a secure
> > > > alternative.
> > > > >
> > > > >
> > > > >
> > > > > On Thu, Feb 21, 2019 at 12:05 PM Stanislav Kozlovski <
> > > > > stanis...@confluent.io>
> > > > > wrote:
> > > > >
> > > > > > Hey Ron, thanks for the KIP.
> > > > > >
> > > > > > I believe the proposed configuration setting
> > > > > > `yes.virginia.i.really.do
> > > > > >
> > > > >
> > > >
> > >
> >
> .want.to.allow.unsecured.oauthbearer.tokens.because.this.is.not.a.production.cluster`
> > > > > > might be too verbose. I acknowledge that we do not want to enable
> > > this
> > > > in
> > > > > > production but we could maybe compromise on a more normal name.
> > > > > >
> > > > > > I am wondering whether it would be more worth it to replace the
> > > default
> > > > > > implementation with a secure one. Disabling it by default can be
> > seen
> > > > as
> > > > > > just kicking the can down the road
> > > > > >
> > > > > > Best,
> > > > > > Stanislav
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Wed, Feb 20, 2019 at 5:31 PM Ron Dagostino <rndg...@gmail.com
> >
> > > > wrote:
> > > > > >
> > > > > > > Hi everyone. I created KIP-432: Additional Broker-Side Opt-In
> for
> > > > > > Default,
> > > > > > > Unsecure SASL/OAUTHBEARER Implementation
> > > > > > > <
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=103091238
> > > > > > > >
> > > > > > > (
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=103091238
> > > > > > > ).
> > > > > > > The motivation for this KIPis as follows:
> > > > > > >
> > > > > > > The default implementation of SASL/OAUTHBEARER, as per KIP-255
> > > > > > > <
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=75968876
> > > > > > > >,
> > > > > > > is unsecured. This is useful for development and testing
> > purposes,
> > > > and
> > > > > > it
> > > > > > > provides a great out-of-the-box experience, but it must not be
> > used
> > > > in
> > > > > > > production because it allows the client to authenticate with
> any
> > > > > > principal
> > > > > > > name it wishes. To enable the default unsecured
> SASL/OAUTHBEARER
> > > > > > > implementation on the broker side simply requires the addition
> of
> > > > > > > OAUTHBEARER to the sasl.enabled.mechanisms configuration value
> > (for
> > > > > > > example:
> > > > > > > sasl.enabled.mechanisms=GSSAPI,OAUTHBEARER instead of simply
> > > > > > > sasl.enabled.mechanisms=GSSAPI). To secure the implementation
> > > > requires
> > > > > > the
> > > > > > > explicit setting of the
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> listener.name.{sasl_plaintext|sasl_ssl}.oauthbearer.sasl.{login,server}.callback.handler.class
> > > > > > > properties on the broker. The question then arises: what if
> > > someone
> > > > > > > either accidentally or maliciously appended OAUTHBEARER to the
> > > > > > > sasl.enabled.mechanisms configuration value? Doing so would
> > enable
> > > > the
> > > > > > > unsecured implementation on the broker, and clients could then
> > > > > > authenticate
> > > > > > > with any principal name they desired.
> > > > > > >
> > > > > > > This KIP proposes to add an additional opt-in configuration
> > > property
> > > > on
> > > > > > the
> > > > > > > broker side for the default, unsecured SASL/OAUTHBEARER
> > > > implementation
> > > > > > such
> > > > > > > that simply adding OAUTHBEARER to the sasl.enabled.mechanisms
> > > > > > configuration
> > > > > > > value would be insufficient to enable the feature. This
> > additional
> > > > > > opt-in
> > > > > > > broker configuration property would have to be explicitly set
> to
> > > true
> > > > > > > before the default unsecured implementation would successfully
> > > > > > authenticate
> > > > > > > users, and the name of this configuration property would
> > explicitly
> > > > > > > indicate that the feature is not secure and must not be used in
> > > > > > > production. Adding this explicit opt-in is a breaking change;
> > > > existing
> > > > > > > uses of the unsecured implementation would have to update their
> > > > > > > configuration to include this explicit opt-in property before
> > their
> > > > > > cluster
> > > > > > > would accept unsecure tokens again. Note that this would only
> > > result
> > > > > in
> > > > > > a
> > > > > > > breaking change in production if the unsecured feature is
> either
> > > > > > > accidentally or maliciously enabled there; it is assumed that
> 1)
> > > this
> > > > > > will
> > > > > > > probably not happen to anyone; and 2) if it does happen to
> > someone
> > > it
> > > > > > > almost certainly would not impact sanctioned clients but would
> > > > instead
> > > > > > > impact malicious clients only (if there were any).
> > > > > > >
> > > > > > >
> > > > > > > Ron
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Best,
> > > > > > Stanislav
> > > > > >
> > > > >
> > > >
> > >
> >
> >
> > --
> > Best,
> > Stanislav
> >
>
--
Best,
Stanislav
