Hi Ron, Thanks for the KIP. This is going to be a really useful feature to tighten security.
I have a few comments/questions: 1. Retries: What is the state of a client connection in between re-authentication retries? Can it continue to process requests? Also, can you describe scenarios where retries are useful? 2. Metrics: I think we should add separate re-authentication success/failure metrics. 3. Implementation: Hmm... The PR looks quite a lot different from what I hoped we would do. In particular, I am concerned about network and security layer code escaping into the client/broker implementation layers. "*We leave it to the owners of the **NetworkClient instances to define how to inject such requests by providing an implementation of the interface to the SaslChannelBuilder, which in turn provides it to the * *SaslClientAuthenticator*" NetworkClient shouldn't have to deal with authentication and changes to every use (AdminClient/KafkaConsumer/KafkaProducer/Broker) to deal with security protocols and SASL mechanisms will be hard to maintain. And what about Connect and Streams - will they need changing as well? Have we considered implementing re-authentication within the network/security layer, like we do authentication? I think we should try and find a way to move channels back to authentication/re-authentication phase so that the rest of the codebase doesn't have special code to handle SASL. I haven't looked into how this can be done, but it doesn't feel impossible. Thoughts? On Mon, Sep 3, 2018 at 10:54 PM, Ron Dagostino <rndg...@gmail.com> wrote: > I just realized there was one place in the KIP that stated that retries > could occur indefinitely (when a client attempts to change identity, which > we arbitrarily disallow). This was a mistake, a holdover from a prior > draft of the KIP. This is now fixed. Retries are never allowed > indefinitely. > > <<<if a connection originally authenticates as User:user1, an attempt to > re-authenticate as anything else (e.g. User:user2) will fail. > <<<Retry is allowed indefinitely in this case. > >>>if a connection originally authenticates as User:user1, an attempt to > re-authenticate as anything else (e.g. User:user2) will fail. > >>>Retry is allowed in this case (still subject to the expiration of the > original credential as described above) > > Ron > > > On Mon, Sep 3, 2018 at 5:35 PM Ron Dagostino <rndg...@gmail.com> wrote: > > > Thanks for the feedback, Stanislav. > > > > <<<I am not too familiar with the networking code to evaluate your > > solution. > > Yeah, I wasn't familiar with it when I started, either, and I am hoping > > people who are intimately familiar with it will take a close look. Some > of > > that code seems to be very central to the core of Kafka, and injecting > > re-authentication attempts into the flow of replica fetching, sending > > transaction markers, and producing or consuming messages is not > something I > > am convinced is acceptable under all circumstances. To be clear, > though, I > > am not saying it is problematic, either -- I just don't have enough > > experience or familiarity to know. I really do want additional eyes on > > this if possible. > > > > Regarding your question about retries, here's the part of the KIP that > > deals with those: > > > > If a re-authentication attempt should fail then the connection will be > >> told to retry after some delay depending on how many retries have been > >> attempted so far: after some small amount of time for the first retry > (e.g. > >> 1 minute), double that for the second retry, and 4 times the initial > delay > >> for every retry thereafter. Retry attempts generally occur at least > until > >> the current credential expires (but not indefinitely – and of course > they > >> won't continue if one of them actually succeeds). There are certain > errors > >> that result in retries not being attempted (i.e. if some internal state > >> doesn't make sense, which generally should not happen). > > > > > > <<<Would we retry endlessly? > > No. There may be at most one retry after the client token expires. So > if > > a token expires after an hour, and several retry attempts fail including > > one at minute 59, then one last attempt will be made at the 63-minute > mark. > > > > <<<Since the functionality for brokers to close connections is outside > the > > scope of this KIP, what effect would > > <<<success/failure in re-authentication have > > Practically speaking, unless authorization is based on the contents of > the > > token rather than ACLs, the ultimate success or failure of > > re-authentication has no effect. The intent is definitely to follow this > > KIP with another one to add the ability for brokers to close connections > > that use expired credentials, and then at that point the client would > have > > to successfully re-authenticate to avoid the connection being forcibly > > closed. > > > > Ron > > > > > > On Mon, Sep 3, 2018 at 12:58 PM Stanislav Kozlovski < > > stanis...@confluent.io> wrote: > > > >> Hi Ron, > >> > >> I really like this KIP, it is very needed. > >> I am still looking through it but unfortunately I am not too familiar > with > >> the networking code to evaluate your solution. > >> > >> I'd like to ask what happens if re-authentication consistently fails. > >> Would > >> we retry endlessly? Since the functionality for brokers to close > >> connections is outside the scope of this KIP, what effect would > >> success/failure in re-authentication have? I think it's worth noting in > >> the > >> KIP > >> > >> I also think the rejected alternative of initiating a new connection > >> should > >> stay rejected. I am not aware of anything currently limiting the client > to > >> connect to the same broker, but I think it would be best if we kept > >> Kafka's > >> options open (e.g addition of a load balancer in the future) and not > >> introduce additional client-broker statefulness. > >> > >> Thanks, > >> Stanislav > >> > >> On Tue, Aug 28, 2018 at 5:28 PM Ron Dagostino <rndg...@gmail.com> > wrote: > >> > >> > Hi everyone. I created KIP 368: Allow SASL Connections to Periodically > >> > Re-Authenticate > >> > < > >> > > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-368% > 3A+Allow+SASL+Connections+to+Periodically+Re-Authenticate > >> > > > >> > ( > >> > > >> > > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-368% > 3A+Allow+SASL+Connections+to+Periodically+Re-Authenticate > >> > ). > >> > The motivation for this KIP is as follows: > >> > > >> > The adoption of KIP-255: OAuth Authentication via SASL/OAUTHBEARER > >> > < > >> https://cwiki.apache.org/confluence/pages/viewpage.action? > pageId=75968876 > >> > > >> > in > >> > release 2.0.0 creates the possibility of using information in the > bearer > >> > token to make authorization decisions. Unfortunately, however, Kafka > >> > connections are long-lived, so there is no ability to change the > bearer > >> > token associated with a particular connection. Allowing SASL > >> connections > >> > to periodically re-authenticate would resolve this. In addition to > this > >> > motivation there are two others that are security-related. First, to > >> > eliminate access to Kafka for connected clients, the current > >> requirement is > >> > to remove all authorizations (i.e. remove all ACLs). This is > necessary > >> > because of the long-lived nature of the connections. It is > >> operationally > >> > simpler to shut off access at the point of authentication, and with > the > >> > release of KIP-86: Configurable SASL Callback Handlers > >> > < > >> > > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-86%3A+ > Configurable+SASL+callback+handlers > >> > > > >> > it > >> > is going to become more and more likely that installations will > >> > authenticate users against external directories (e.g. via LDAP). The > >> > ability to stop Kafka access by simply disabling an account in an LDAP > >> > directory (for example) is desirable. The second motivating factor > for > >> > re-authentication related to security is that the use of short-lived > >> tokens > >> > is a common OAuth security recommendation, but issuing a short-lived > >> token > >> > to a Kafka client (or a broker when OAUTHBEARER is the inter-broker > >> > protocol) currently has no benefit because once a client is connected > >> to a > >> > broker the client is never challenged again and the connection may > >> remain > >> > intact beyond the token expiration time (and may remain intact > >> indefinitely > >> > under perfect circumstances). This KIP proposes adding the ability > for > >> > clients (and brokers when OAUTHBEARER is the inter-broker protocol) to > >> > re-authenticate their connections to brokers and have the new bearer > >> token > >> > appear on their session rather than the old one. > >> > > >> > The description of this KIP is actually quite straightforward from a > >> > functionality perspective; from an implementation perspective, though, > >> the > >> > KIP is not so straightforward, so it includes a pull request with a > >> > proposed implementation. See https://github.com/apache/kafk > a/pull/5582 > >> . > >> > > >> > Ron > >> > > >> > >> > >> -- > >> Best, > >> Stanislav > >> > > >
