Thanks, Rajini -- I updated the KIP to fix this. Ron
On Wed, Sep 19, 2018 at 4:54 AM Rajini Sivaram <rajinisiva...@gmail.com> wrote: > I should have said `security configs` instead of `channel configs`. > > The KIP says: > > - The configuration option this KIP proposes to add to enable > server-side expired-connection-kill is '*connections.max.reauth.ms > <http://connections.max.reauth.ms>*' (not prefixed with listener prefix > or SASL mechanism name – this is a single value for the cluster) > - The '*connections.max.reauth.ms <http://connections.max.reauth.ms>*' > configuration option will not be dynamically changeable; restarts will > be > required if the value is to be changed. However, if a new listener is > dynamically added, the value could be set for that listener at that > time. > > Those statements are contradictory. Perhaps the first one should say `it > may be optionally prefixed with the listener name`? > > On Tue, Sep 18, 2018 at 3:55 PM, Ron Dagostino <rndg...@gmail.com> wrote: > > > HI Rajini. The KIP is updated as summarized below, and I will start a > vote > > immediately. > > > > <<<It may be useful to have a metric of expired connections killed > > Ok, agreed. I called it expired-connections-killed-total > > > > <<<For `successful-v0-authentication-{rate,total}`, we probably want > > <<<version as a tag rather in the name. > > Ok, agreed. I kept existing metrics unchanged but added an additional > tag > > to the V0 metrics so they are separate. > > > > <<<Not sure if we need four of these > > <<<(rate/total with success/failure). Perhaps just success/total is > > sufficient? > > Ok, agreed, just kept the successful total. > > > > <<<For the session lifetime config, we don't need to require a listener > or > > <<<mechanism prefix > > Ok, agreed, the config is now cluster-wide. > > > > <<<For all channel configs, we allow an optional listener prefix, > > <<<so we should do the same here > > Not sure what this is referring to. We don't have channel configs here, > > right? > > > > <<<The KIP says connections are terminated on requests not related to > > <<<re-authentication (ApiVersionsRequest, SaslHandshakeRequest, and > > <<<SaslAuthenticateRequest). We can skip for ApiVersionsRequest for > > <<<re-authentication, so that doesn't need to be in the list. > > Yes, I was planning on that optimization; agreed, I removed it from the > > list > > > > <<<we allow new listeners > > <<<to be added dynamically and all configs for the listener can be added > > <<<dynamically (with the listener prefix). I think we want to allow that > > for > > <<<this config > > <<<We should mention this in the KIP, though in terms of implementation, > I > > <<<would leave that for a separate JIRA (it doesn't need to be > implemented > > at > > <<<the same time). > > Ok, agreed > > > > Thanks again for all the feedback and discussion. > > > > Ron > > > > On Tue, Sep 18, 2018 at 6:43 AM Rajini Sivaram <rajinisiva...@gmail.com> > > wrote: > > > > > Hi Ron, > > > > > > Thanks for the updates. The KIP looks good. A few comments and minor > > points > > > below, but feel free to start vote to try and get it into 2.1.0. More > > > community feedback will be really useful. > > > > > > 1) It may be useful to have a metric of expired connections killed by > the > > > broker. There could be a client implementation that doesn't support > > > re-authentications, but happens to use the latest version of > > > SaslAuthenticateRequest. Or cases where re-authentication didn't happen > > on > > > time. > > > > > > 2) For `successful-v0-authentication-{rate,total}`, we probably want > > > version as a tag rather in the name. Not sure if we need four of these > > > (rate/total with success/failure). Perhaps just success/total is > > > sufficient? > > > > > > 3) For the session lifetime config, we don't need to require a listener > > or > > > mechanism prefix. In most cases, we would expect a single config on the > > > broker-side. For all channel configs, we allow an optional listener > > prefix, > > > so we should do the same here. > > > > > > 4) The KIP says connections are terminated on requests not related to > > > re-authentication (ApiVersionsRequest, SaslHandshakeRequest, and > > > SaslAuthenticateRequest). We can skip for ApiVersionsRequest for > > > re-authentication, so that doesn't need to be in the list. > > > > > > 5) The KIP says that the new config will not be dynamically updatable. > We > > > have a very limited set of configs that are dynamically updatable for > an > > > existing listener. And we don't want to add this config to the list > since > > > we don't expect this value to change frequently. But we allow new > > listeners > > > to be added dynamically and all configs for the listener can be added > > > dynamically (with the listener prefix). I think we want to allow that > for > > > this config (i.e. add a new OAuth listener with re-authentication > > enabled). > > > We should mention this in the KIP, though in terms of implementation, I > > > would leave that for a separate JIRA (it doesn't need to be implemented > > at > > > the same time). > > > > > > > > > > > > On Tue, Sep 18, 2018 at 3:06 AM, Ron Dagostino <rndg...@gmail.com> > > wrote: > > > > > > > HI again, Rajini. Would we ever want the max session time to be > > > different > > > > across different SASL mechanisms? I'm wondering, now that we are > > > > supporting all SASL mechanisms via this KIP, if we still need to > prefix > > > > this config with the "[listener].[mechanism]." prefix. I've kept the > > > > prefix in the KIP for now, but it would be easier to just set it once > > for > > > > all mechanisms, and I don't see that as being a problem. Let me know > > > what > > > > you think. > > > > > > > > Ron > > > > > > > > On Mon, Sep 17, 2018 at 9:51 PM Ron Dagostino <rndg...@gmail.com> > > wrote: > > > > > > > > > Hi Rajini. The KIP is updated. Aside from a once-over to make > sure > > it > > > > is > > > > > all accurate, I think we need to confirm the metrics. The decision > > to > > > > not > > > > > reject authentications that use tokens with too-long a lifetime > > allowed > > > > the > > > > > metrics to be simpler. I decided that in addition to tracking > these > > > > > metrics on the broker: > > > > > > > > > > failed-reauthentication-{rate,total} and > > > > > successful-reauthentication-{rate,total} > > > > > > > > > > we simply need one more set of broker metrics to track the subset > of > > > > > clients clients that are not upgraded to v2.1.0 and are still > using a > > > V0 > > > > > SaslAuthenticateRequest: > > > > > > > > > > failed-v0-authentication-{rate,total} and > > > > > successful-v0-authentication-{rate,total} > > > > > > > > > > See the Migration section of the KIP for details of how this would > be > > > > used. > > > > > > > > > > I wonder if we need a broker metric documenting the number of > > "expired" > > > > > sessions killed by the broker since it would be the same as > > > > > successful-v0-authentication-total. I've eliminated that from the > > KIP > > > > for > > > > > now. Thoughts? > > > > > > > > > > There is also a client-side metric for re-authentication latency > > > tracking > > > > > (still unnamed -- do you have a preference?) > > > > > > > > > > I think we're close to being able to put this KIP up for a vote. > > > > > > > > > > Ron > > > > > > > > > > > > > > > On Mon, Sep 17, 2018 at 2:45 PM Ron Dagostino <rndg...@gmail.com> > > > wrote: > > > > > > > > > >> <<<extra field alongside errors, not in the opaque body as > mentioned > > > in > > > > >> the KIP > > > > >> Right, that was a concern I had mentioned in a follow-up email. > > Agree > > > > it > > > > >> should go alongside errors. > > > > >> > > > > >> <<<SCRAM for delegation token based authentication where > credentials > > > > have > > > > >> <<<an expiry time, so we do need to be able to set individual > > > credential > > > > >> <<<lifetimes, where the server knows the expiry time but client > may > > > not > > > > >> Ah, ok, I was not aware that this case existed. Agreed, for > > > > consistency, > > > > >> server will always send it back to the client. > > > > >> > > > > >> <<<I was trying to avoid this altogether [SaslClient negotiated > > > property > > > > >> for lifetime] > > > > >> Since we now agree that the server will send it back, yes, there > is > > no > > > > >> need for this. > > > > >> > > > > >> <<<wasn't entirely clear about the purpose of > > > > >> [sasl.login.refresh.reauthenticate.enable] > > > > >> I think this might have been more useful when we weren't > necessarily > > > > >> going to support all SASL mechanisms and/or when the broker was > not > > > > going > > > > >> to advertise the fact that it supported re-authentication. You > are > > > > >> correct, now that we support it for all SASL mechanisms and we are > > > > bumping > > > > >> an API version, I think it is okay to simply enable it wherever > both > > > the > > > > >> client and server meet the required versions. > > > > >> > > > > >> <<<wasn't entirely clear about the purpose of [ > > > > connections.max.reauth.ms] > > > > >> <<<wasn't expecting that we would reject > > > > >> <<<tokens or tickets simply because they were too long-lived > > > > >> <<<tickets are granted by a 3rd party authority > > > > >> <<<Client re-authenticates even though token was not > > > > >> <<<refreshed. Does this matter? > > > > >> I was going under the assumption that it would matter, but based > on > > > your > > > > >> pushback I just realized that the same functionality can be > > > implemented > > > > as > > > > >> part of token validation if there is a desire to limit token > > lifetimes > > > > to a > > > > >> certain max value (and the token validator has to be provided in > > > > production > > > > >> anyway since all we provide out-of-the-box is the unsecured > > > > validator). So > > > > >> I'm willing to abandon this check as part of re-authentication. > > > > >> > > > > >> I'll adjust the KIP accordingly a bit later. Thanks for the > > continued > > > > >> feedback/discussion. > > > > >> > > > > >> Ron > > > > >> > > > > >> > > > > >> > > > > >> > > > > >> On Mon, Sep 17, 2018 at 2:10 PM Rajini Sivaram < > > > rajinisiva...@gmail.com > > > > > > > > > >> wrote: > > > > >> > > > > >>> Hi Ron, > > > > >>> > > > > >>> > > > > >>> *1) Is there a reason to communicate this value back to the > client > > > when > > > > >>> the > > > > >>> client already knows it? It's an extra network round-trip, and > > since > > > > the > > > > >>> SASL round-tripsare defined by the spec I'm not certain adding an > > > extra > > > > >>> round-trip is acceptable.* > > > > >>> > > > > >>> I wasn't suggesting an extra round-trip for propagating session > > > > >>> lifetime. I > > > > >>> was expecting session lifetime to be added to the last > > > > SASL_AUTHENTICATE > > > > >>> response from the broker. Because SASL is a challenge-response > > > > mechanism, > > > > >>> SaslServer knows when the response being sent is the last one and > > > hence > > > > >>> can > > > > >>> send the session lifetime in the response (in the same way as we > > > > >>> propagate > > > > >>> errors). I was expecting this to be added as an extra field > > alongside > > > > >>> errors, not in the opaque body as mentioned in the KIP. The > opaque > > > byte > > > > >>> array strictly conforms to the SASL mechanism wire protocol and > we > > > want > > > > >>> to > > > > >>> keep it that way. > > > > >>> > > > > >>> As you have said, we don't need server to propagate session > > lifetime > > > > for > > > > >>> OAUTHBEARER since client knows the token lifetime. But server > also > > > > knows > > > > >>> the credential lifetime and by having the server decide the > > lifetime, > > > > we > > > > >>> can use the same code path for all mechanisms. If we only have a > > > > constant > > > > >>> max lifetime in the server, for PLAIN and SCRAM we will end up > > having > > > > the > > > > >>> same lifetime for all credentials with no ability to set actual > > > expiry. > > > > >>> We > > > > >>> use SCRAM for delegation token based authentication where > > credentials > > > > >>> have > > > > >>> an expiry time, so we do need to be able to set individual > > credential > > > > >>> lifetimes, where the server knows the expiry time but client may > > not. > > > > >>> > > > > >>> *2) I also just realized that if the client is to learn the > > > credential > > > > >>> lifetime we wouldn't want to put special-case code in the > > > Authenticator > > > > >>> for > > > > >>> GSSAPI and OAUTHBEARER; we would want to expose the value > > > generically, > > > > >>> probably as a negotiated property on the SaslClient instance.* > > > > >>> > > > > >>> I was trying to avoid this altogether. Client doesn't need to > know > > > > >>> credential lifetime. Server asks the client to re-authenticate > > within > > > > its > > > > >>> session lifetime. > > > > >>> > > > > >>> 3) From the KIP, I wasn't entirely clear about the purpose of the > > two > > > > >>> configs: > > > > >>> > > > > >>> sasl.login.refresh.reauthenticate.enable: Do we need this? Client > > > > knows > > > > >>> if > > > > >>> broker version supports re-authentication based on the > > > > SASL_AUTHENTICATE > > > > >>> version returned in ApiVersionsResponse. Client knows if broker > is > > > > >>> configured to enable re-authentication based on session lifetime > > > > returned > > > > >>> in SaslAuthenticateResponse. If broker has re-authentication > > > configured > > > > >>> and > > > > >>> client supports re-authentication, you would always want > > > > re-authenticate. > > > > >>> So I wasn't sure why we need a config to opt in or out on the > > > > >>> client-side. > > > > >>> > > > > >>> > > > > >>> connections.max.reauth.ms: We obviously need a broker-side > config. > > > Not > > > > >>> entirely sure about the semantics of the config that drives > > > > >>> re-authentication. In particular, I wasn't expecting that we > would > > > > reject > > > > >>> tokens or tickets simply because they were too long-lived. Since > > > tokens > > > > >>> or > > > > >>> tickets are granted by a 3rd party authority, I am not sure if > > > clients > > > > >>> will > > > > >>> always have control over the lifetime. Do we need to support any > > more > > > > >>> scenarios than these: > > > > >>> > > > > >>> > > > > >>> A) reauth.ms=10,credential.lifetime.ms=10 : Broker sets > > > > >>> session.lifetime=10, > > > > >>> so this works. > > > > >>> > > > > >>> B) reauth.ms=10, credential.lifetime.ms=5 : Broker sets > > > > >>> session.lifetime=5, > > > > >>> so this works. > > > > >>> C) reauth.ms=10, credential.lifetime.ms=20 : Broker sets > > > > >>> session.lifetime=10. Client re-authenticates even though token > was > > > not > > > > >>> refreshed. Does this matter? > > > > >>> D) reauth.ms=Long.MAX_VALUE, credential.lifetime.ms=10: Broker > > sets > > > > >>> session.lifetime=10, client re-authenticates based on credential > > > > expiry. > > > > >>> E) reauth.ms=0 (default), credential.lifetime.ms=10 : Broker > sets > > > > >>> session.lifetime=0, Broker doesn't terminate sessions, client > > doesn't > > > > >>> re-authenticate. We generate useful metrics. > > > > >>> F) reauth.ms=0 (default),no lifetime for credential (e.g. > PLAIN): > > > > Broker > > > > >>> sets session.lifetime=0, Broker doesn't terminate sessions, > client > > > > >>> doesn't > > > > >>> re-authenticate > > > > >>> G) reauth.ms=10,no lifetime for credential (e.g. PLAIN) : Broker > > > sets > > > > >>> session.lifetime=10. Client re-authenticates. > > > > >>> > > > > >>> I would have thought that D) is the typical scenario for > > > OAuth/Kerberos > > > > >>> to > > > > >>> respect token expiry time. G) would be typical scenario for PLAIN > > to > > > > >>> force > > > > >>> re-authenication at regular intervals. A/B/C are useful to force > > > > >>> re-authentication in scenarios where you might check for > credential > > > > >>> revocation in the server. And E/F are useful to disable > > > > re-authentication > > > > >>> and generate metrics (also the default behaviour useful during > > > > >>> migration). > > > > >>> Have I missed something? > > > > >>> > > > > >>> > > > > >>> On Mon, Sep 17, 2018 at 4:27 PM, Ron Dagostino < > rndg...@gmail.com> > > > > >>> wrote: > > > > >>> > > > > >>> > Hi yet again, Rajini. I also just realized that if the client > is > > > to > > > > >>> learn > > > > >>> > the credential lifetime we wouldn't want to put special-case > code > > > in > > > > >>> the > > > > >>> > Authenticator for GSSAPI and OAUTHBEARER; we would want to > expose > > > the > > > > >>> value > > > > >>> > generically, probably as a negotiated property on the > SaslClient > > > > >>> instance. > > > > >>> > We might be talking about making the negotiated property key > part > > > of > > > > >>> the > > > > >>> > public API. In other words, the SaslClient would be > responsible > > > for > > > > >>> > exposing the credential (i.e. token or ticket) lifetime at a > > > > well-known > > > > >>> > negotiated property name, such as "Credential.Lifetime" and > > > putting a > > > > >>> Long > > > > >>> > value there if there is a lifetime. That well-klnown key (e.g. > > > > >>> > "Credential.Lifetime") would be part of the public API, right? > > > > >>> > > > > > >>> > Ron > > > > >>> > > > > > >>> > On Mon, Sep 17, 2018 at 11:03 AM Ron Dagostino < > > rndg...@gmail.com> > > > > >>> wrote: > > > > >>> > > > > > >>> > > Hi again, Rajini. After thinking about this a little while, > it > > > > >>> occurs to > > > > >>> > > me that maybe the communication of max session lifetime > should > > > > occur > > > > >>> via > > > > >>> > > SASL_HANDSHAKE after all. Here's why. The value > communicated > > is > > > > >>> the max > > > > >>> > > session lifetime allowed, and the client can assume it is the > > the > > > > >>> session > > > > >>> > > lifetime for that particular session unless the particular > SASL > > > > >>> mechanism > > > > >>> > > could result in a shorter session that would be obvious to > the > > > > >>> client and > > > > >>> > > the server. In particular, for OAUTHBEARER, the session > > lifetime > > > > >>> will be > > > > >>> > > the token lifetime, which the client and server will both > know. > > > Is > > > > >>> > there a > > > > >>> > > reason to communicate this value back to the client when the > > > client > > > > >>> > already > > > > >>> > > knows it? It's an extra network round-trip, and since the > SASL > > > > >>> > round-trips > > > > >>> > > are defined by the spec I'm not certain adding an extra > > > round-trip > > > > is > > > > >>> > > acceptable. Even if we decide we can add it, it helps with > > > latency > > > > >>> if we > > > > >>> > > don't. > > > > >>> > > > > > > >>> > > Kerberos may be a bit different -- I don't know if the broker > > can > > > > >>> learn > > > > >>> > > the session lifetime. If it can then the same thing holds -- > > > both > > > > >>> client > > > > >>> > > and server will know the session lifetime and there is no > > reason > > > to > > > > >>> > > communicate it back. If the server can't learn the lifetime > > > then I > > > > >>> don't > > > > >>> > > think adding an extra SASL_AUTHENTICATE round trip is going > to > > > > help, > > > > >>> > anyway. > > > > >>> > > > > > > >>> > > Also, by communicating the max session lifetime in the > > > > SASL_HANDSHAKE > > > > >>> > > response, both OAUTHBEARER and GSSAPI clients will be able to > > > know > > > > >>> before > > > > >>> > > sending any SASL_AUTHENTICATE requests whether their > credential > > > > >>> violates > > > > >>> > > the maximum. This allows a behaving client to give a good > > error > > > > >>> message. > > > > >>> > > A malicious client would ignore the value and send a > > longer-lived > > > > >>> > > credential, and then that would be rejected on the server > side. > > > > >>> > > > > > > >>> > > I'm still good with ExpiringCredential not needing to be > > public. > > > > >>> > > > > > > >>> > > What do you think? > > > > >>> > > > > > > >>> > > Ron > > > > >>> > > > > > > >>> > > Ron > > > > >>> > > > > > > >>> > > Ron > > > > >>> > > > > > > >>> > > On Mon, Sep 17, 2018 at 9:13 AM Ron Dagostino < > > rndg...@gmail.com > > > > > > > > >>> wrote: > > > > >>> > > > > > > >>> > >> Hi Rajini. I've updated the KIP to reflect the decision to > > > fully > > > > >>> > support > > > > >>> > >> this for all SASL mechanisms and to not require the > > > > >>> ExpiringCredential > > > > >>> > >> interface to be public. > > > > >>> > >> > > > > >>> > >> Ron > > > > >>> > >> > > > > >>> > >> On Mon, Sep 17, 2018 at 6:55 AM Ron Dagostino < > > > rndg...@gmail.com> > > > > >>> > wrote: > > > > >>> > >> > > > > >>> > >>> Actually, I think the metric remains the same assuming the > > > > >>> > >>> authentication fails when the token lifetime is too long. > > > > >>> Negative > > > > >>> > max > > > > >>> > >>> config on server counts what would have been killed because > > > maybe > > > > >>> > client > > > > >>> > >>> re-auth is not turned on; positive max enables the kill, > > which > > > is > > > > >>> > counted > > > > >>> > >>> by a second metric as proposed. The current proposal had > > > already > > > > >>> > stated > > > > >>> > >>> that a non-zero value would disallow an authentication > with a > > > > token > > > > >>> > that > > > > >>> > >>> has too large a lifetime, and that is still the case, I > > think. > > > > But > > > > >>> > let’s > > > > >>> > >>> make sure we are on the same page about all this. > > > > >>> > >>> > > > > >>> > >>> Ron > > > > >>> > >>> > > > > >>> > >>> > On Sep 17, 2018, at 6:42 AM, Ron Dagostino < > > > rndg...@gmail.com> > > > > >>> > wrote: > > > > >>> > >>> > > > > > >>> > >>> > Hi Rajini. I see what you are saying. The background > > login > > > > >>> refresh > > > > >>> > >>> thread does have to factor into the decision for > OAUTHBEARER > > > > >>> because > > > > >>> > there > > > > >>> > >>> is no new token to re-authenticate with until that refresh > > > > succeeds > > > > >>> > >>> (similarly with Kerberos), but I think you are right that > the > > > > >>> interface > > > > >>> > >>> doesn’t necessarily have to be public. The server does > > decide > > > > the > > > > >>> > time for > > > > >>> > >>> PLAIN and the SCRAM-related mechanisms under the current > > > > proposal, > > > > >>> but > > > > >>> > it > > > > >>> > >>> is done in SaslHandshakeResponse, and at that point the > > server > > > > >>> won’t > > > > >>> > have > > > > >>> > >>> any token yet; making it happen via SaslAuthenticateRequest > > at > > > > the > > > > >>> > very end > > > > >>> > >>> does allow the server to know everything for all > mechanisms. > > > > >>> > >>> > > > > > >>> > >>> > I see two potential issues to discuss. First, the server > > > must > > > > >>> > >>> communicate a time that exceeds the (token or ticket) > refresh > > > > >>> period > > > > >>> > on the > > > > >>> > >>> client. Assuming it communicates the expiration time of > the > > > > >>> > token/ticket, > > > > >>> > >>> that’s the best it can do. So I think that’ll be fine. > > > > >>> > >>> > > > > > >>> > >>> > The second issue is what happens if the server is > > configured > > > to > > > > >>> > accept > > > > >>> > >>> a max value — say, an hour — and the token is good for > > > longer. I > > > > >>> > assumed > > > > >>> > >>> that the client should not be allowed to authenticate in > the > > > > first > > > > >>> > place > > > > >>> > >>> because it could then simply re-authenticate with the same > > > token > > > > >>> after > > > > >>> > an > > > > >>> > >>> hour, which defeats the motivations for this KIP. So do we > > > agree > > > > >>> the > > > > >>> > max > > > > >>> > >>> token lifetime allowed will be enforced at authentication > > time? > > > > >>> > Assuming > > > > >>> > >>> so, then we need to discuss migration. > > > > >>> > >>> > > > > > >>> > >>> > The current proposal includes a metric that can be used > to > > > > >>> identify > > > > >>> > if > > > > >>> > >>> an OAUTHBEARER client is misconfigured — the number of > > > > connections > > > > >>> that > > > > >>> > >>> would have been killed (could be non-zero when the > configured > > > max > > > > >>> is a > > > > >>> > >>> negative number). Do we still want this type of an > > indication, > > > > and > > > > >>> if > > > > >>> > so, > > > > >>> > >>> is it still done the same way — a negative number for max, > > but > > > > >>> instead > > > > >>> > of > > > > >>> > >>> counting the number of kills that would have been done it > > > counts > > > > >>> the > > > > >>> > number > > > > >>> > >>> of authentications that would have been failed? > > > > >>> > >>> > > > > > >>> > >>> > Ron > > > > >>> > >>> > > > > > >>> > >>> >> On Sep 17, 2018, at 6:06 AM, Rajini Sivaram < > > > > >>> > rajinisiva...@gmail.com> > > > > >>> > >>> wrote: > > > > >>> > >>> >> > > > > >>> > >>> >> Hi Ron, > > > > >>> > >>> >> > > > > >>> > >>> >> Thinking a bit more about other SASL mechanisms, I think > > the > > > > >>> issue > > > > >>> > is > > > > >>> > >>> that > > > > >>> > >>> >> in the current proposal, clients decide the > > > re-authentication > > > > >>> > period. > > > > >>> > >>> For > > > > >>> > >>> >> mechanisms like PLAIN or SCRAM, we would actually want > > > server > > > > to > > > > >>> > >>> determine > > > > >>> > >>> >> the re-authentication interval. For example, the PLAIN > or > > > > SCRAM > > > > >>> > >>> database > > > > >>> > >>> >> could specify the expiry time for each principal, or the > > > > broker > > > > >>> > could > > > > >>> > >>> be > > > > >>> > >>> >> configured with a single expiry time for all principals > of > > > > that > > > > >>> > >>> mechanism. > > > > >>> > >>> >> For OAuth, brokers do know the expiry time. Haven't > > figured > > > > out > > > > >>> what > > > > >>> > >>> to do > > > > >>> > >>> >> with Kerberos, but in any case for broker-side > connection > > > > >>> > termination > > > > >>> > >>> on > > > > >>> > >>> >> expiry, we need the broker to know/decide the expiry > time. > > > So > > > > I > > > > >>> > would > > > > >>> > >>> like > > > > >>> > >>> >> to suggest a slightly different approach to managing > > > > >>> > >>> re-authentications. > > > > >>> > >>> >> > > > > >>> > >>> >> 1) Instead of changing SASL_HANDSHAKE version number, we > > > bump > > > > up > > > > >>> > >>> >> SASL_AUTHENTICATE version number. > > > > >>> > >>> >> 2) In the final SASL_AUTHENTICATE response, broker > returns > > > the > > > > >>> > expiry > > > > >>> > >>> time > > > > >>> > >>> >> of this authenticated session. This is the interval > after > > > > which > > > > >>> > >>> broker will > > > > >>> > >>> >> terminate the connection If it wasn't re-authenticated. > > > > >>> > >>> >> 3) We no longer need the public interface change to add > ` > > > > >>> > >>> >> > > > org.apache.kafka.common.security.expiring.ExpiringCredential` > > > > >>> for > > > > >>> > the > > > > >>> > >>> >> client-side. Instead we schedule the next > > re-authentication > > > on > > > > >>> the > > > > >>> > >>> client > > > > >>> > >>> >> based on the expiry time provided by the server (some > time > > > > >>> earlier > > > > >>> > >>> than the > > > > >>> > >>> >> expiry). > > > > >>> > >>> >> 4) If client uses SASL_AUTHENTICATE v0, broker will not > > > return > > > > >>> > expiry > > > > >>> > >>> time. > > > > >>> > >>> >> The connection will be terminated if that feature is > > enabled > > > > >>> (the > > > > >>> > >>> same code > > > > >>> > >>> >> path as client failing to re-authenticte on time). > > > > >>> > >>> >> > > > > >>> > >>> >> Thoughts? > > > > >>> > >>> >> > > > > >>> > >>> >>> On Sat, Sep 15, 2018 at 3:06 AM, Ron Dagostino < > > > > >>> rndg...@gmail.com> > > > > >>> > >>> wrote: > > > > >>> > >>> >>> > > > > >>> > >>> >>> Hi everyone. I've updated the KIP to reflect all > > > discussion > > > > to > > > > >>> > date, > > > > >>> > >>> >>> including the decision to go with the low-level > approach. > > > > This > > > > >>> > >>> latest > > > > >>> > >>> >>> version of the KIP includes the above " > > > > >>> connections.max.reauth.ms" > > > > >>> > >>> >>> proposal, > > > > >>> > >>> >>> which I know has not been discussed yet. It mentions > new > > > > >>> metrics, > > > > >>> > >>> some of > > > > >>> > >>> >>> which may not have been discussed either (and names are > > > > missing > > > > >>> > from > > > > >>> > >>> some > > > > >>> > >>> >>> of them). Regardless, this new version is the closest > > yet > > > > to a > > > > >>> > >>> version > > > > >>> > >>> >>> that can be put to a vote next week. > > > > >>> > >>> >>> > > > > >>> > >>> >>> Ron > > > > >>> > >>> >>> > > > > >>> > >>> >>>> On Fri, Sep 14, 2018 at 8:59 PM Ron Dagostino < > > > > >>> rndg...@gmail.com> > > > > >>> > >>> wrote: > > > > >>> > >>> >>>> > > > > >>> > >>> >>>> Minor correction: I'm proposing " > > > connections.max.reauth.ms" > > > > >>> as > > > > >>> > the > > > > >>> > >>> >>>> broker-side configuration property, not " > > > > >>> > >>> >>>> connections.max.expired.credentials.ms". > > > > >>> > >>> >>>> > > > > >>> > >>> >>>> Ron > > > > >>> > >>> >>>> > > > > >>> > >>> >>>>> On Fri, Sep 14, 2018 at 8:40 PM Ron Dagostino < > > > > >>> rndg...@gmail.com > > > > >>> > > > > > > >>> > >>> wrote: > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> Hi Rajini. I'm going to choose * > > > > >>> > >>> connections.max.expired.credentials.ms > > > > >>> > >>> >>>>> <http://connections.max.expired.credentials.ms>* as > > the > > > > >>> option > > > > >>> > >>> name > > > > >>> > >>> >>>>> because it is consistent with the comment I made in > the > > > > >>> section > > > > >>> > >>> about > > > > >>> > >>> >>> how > > > > >>> > >>> >>>>> to get the client and server to agree on credential > > > > lifetime: > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> "We could add a new API call so that clients could > ask > > > > >>> servers > > > > >>> > for > > > > >>> > >>> the > > > > >>> > >>> >>>>> lifetime they use, or we could extend the > > > > >>> > >>> SaslHandshakeRequest/Response > > > > >>> > >>> >>> API > > > > >>> > >>> >>>>> call to include that information in the server's > > > response – > > > > >>> the > > > > >>> > >>> client > > > > >>> > >>> >>>>> would then adopt that value" > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> We set the config option value on the broker (with > the > > " > > > > >>> > >>> >>>>> listener.name.mechanism." prefix), and we will return > > the > > > > >>> > >>> configured > > > > >>> > >>> >>>>> value in the SaslHandshakeResponse (requiring a wire > > > format > > > > >>> > change > > > > >>> > >>> in > > > > >>> > >>> >>>>> addition to a version bump). The value (referred to > as > > > "X" > > > > >>> > below) > > > > >>> > >>> can > > > > >>> > >>> >>> be > > > > >>> > >>> >>>>> negative, zero, or positive and is to be interpreted > as > > > > >>> follows: > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> X = 0: Fully disable. The server will respond to > > > > >>> > >>> re-authentications, > > > > >>> > >>> >>> but > > > > >>> > >>> >>>>> it won't kill any connections due to expiration, and > it > > > > won't > > > > >>> > track > > > > >>> > >>> >>> either > > > > >>> > >>> >>>>> of the 2 metrics mentioned below. > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> Now, a couple of definitions for when X != 0: > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> 1) We define the *maximum allowed expiration time* to > > be > > > > the > > > > >>> time > > > > >>> > >>> >>>>> determined by the point when (re-)authentication > occurs > > > > plus > > > > >>> |X| > > > > >>> > >>> >>>>> milliseconds. > > > > >>> > >>> >>>>> 2) We define the *requested expiration time* to be > the > > > > >>> maximum > > > > >>> > >>> allowed > > > > >>> > >>> >>>>> expiration time except for the case of an OAuth > bearer > > > > >>> token, in > > > > >>> > >>> which > > > > >>> > >>> >>> case > > > > >>> > >>> >>>>> it is the point at which the token expires. > (Kerberos > > > > >>> presumably > > > > >>> > >>> also > > > > >>> > >>> >>>>> specifies a ticket lifetime, but frankly I am not in > a > > > > >>> position > > > > >>> > to > > > > >>> > >>> do > > > > >>> > >>> >>> any > > > > >>> > >>> >>>>> Kerberos-related coding in time for a 2.1.0 release > and > > > > would > > > > >>> > >>> prefer to > > > > >>> > >>> >>>>> ignore this piece of information for this KIP -- > would > > it > > > > be > > > > >>> > >>> acceptable > > > > >>> > >>> >>> to > > > > >>> > >>> >>>>> have someone else add it later?). > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> Based on these definitions, we define the behavior as > > > > >>> follows: > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> *X > 0: Fully enable* > > > > >>> > >>> >>>>> A) The server will reject any > > > > >>> authentication/re-authentication > > > > >>> > >>> attempt > > > > >>> > >>> >>>>> when the requested expiration time is after the > maximum > > > > >>> allowed > > > > >>> > >>> >>> expiration > > > > >>> > >>> >>>>> time (which could only happen with an OAuth bearer > > token, > > > > >>> > assuming > > > > >>> > >>> we > > > > >>> > >>> >>> skip > > > > >>> > >>> >>>>> dealing with Kerberos for now). > > > > >>> > >>> >>>>> B) The server will kill connections that are used > > beyond > > > > the > > > > >>> > >>> requested > > > > >>> > >>> >>>>> expiration time. > > > > >>> > >>> >>>>> C) A broker metric will be maintained that documents > > the > > > > >>> number > > > > >>> > >>> >>>>> connections killed by the broker. This metric will > be > > > > >>> non-zero > > > > >>> > if > > > > >>> > >>> a > > > > >>> > >>> >>> client > > > > >>> > >>> >>>>> is connecting to the broker with re-authentication > > either > > > > >>> > >>> unavailable > > > > >>> > >>> >>> (i.e. > > > > >>> > >>> >>>>> an older client) or disabled. > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> *X < 0: Partially enable* > > > > >>> > >>> >>>>> A) Same as above: the server will reject any > > > > >>> > >>> >>>>> authentication/re-authentication attempt when the > > > > requested > > > > >>> > >>> expiration > > > > >>> > >>> >>> time > > > > >>> > >>> >>>>> is after the maximum allowed expiration time (which > > could > > > > >>> only > > > > >>> > >>> happen > > > > >>> > >>> >>> with > > > > >>> > >>> >>>>> an OAuth bearer token, assuming we skip dealing with > > > > >>> Kerberos for > > > > >>> > >>> now). > > > > >>> > >>> >>>>> B) The server will **not** kill connections that are > > used > > > > >>> beyond > > > > >>> > >>> the > > > > >>> > >>> >>>>> requested expiration time. > > > > >>> > >>> >>>>> C) A broker metric will be maintained that documents > > the > > > > >>> number > > > > >>> > of > > > > >>> > >>> API > > > > >>> > >>> >>>>> requests unrelated to re-authentication that are made > > > over > > > > a > > > > >>> > >>> connection > > > > >>> > >>> >>>>> beyond the requested expiration time. This metric > will > > > be > > > > >>> > helpful > > > > >>> > >>> for > > > > >>> > >>> >>>>> operators to ensure that all clients are properly > > > upgraded > > > > >>> and > > > > >>> > >>> >>>>> re-authenticating before fully enabling the > server-side > > > > >>> > >>> >>>>> expired-connection-kill functionality (i.e. by > changing > > > the > > > > >>> value > > > > >>> > >>> from a > > > > >>> > >>> >>>>> negative number to a positive number): this metric > will > > > be > > > > >>> zero > > > > >>> > >>> across > > > > >>> > >>> >>> all > > > > >>> > >>> >>>>> brokers when it is safe to fully enable the > server-side > > > > >>> feature. > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> Thoughts? > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> Ron > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>> On Fri, Sep 14, 2018 at 11:59 AM Rajini Sivaram < > > > > >>> > >>> >>> rajinisiva...@gmail.com> > > > > >>> > >>> >>>>> wrote: > > > > >>> > >>> >>>>> > > > > >>> > >>> >>>>>> It will be good to shorten the config name. We have > a > > > > config > > > > >>> > >>> named ` > > > > >>> > >>> >>>>>> connections.max.idle.ms`. We could add something > > like ` > > > > >>> > >>> >>>>>> connections.max.expired.credentials.ms`. As you > said, > > > it > > > > >>> would > > > > >>> > be > > > > >>> > >>> >>>>>> prefixed > > > > >>> > >>> >>>>>> with listener and SASL mechanism name. We should be > > able > > > > to > > > > >>> > >>> support > > > > >>> > >>> >>>>>> connection termination in future even with SSL, so > > > perhaps > > > > >>> we > > > > >>> > >>> don't > > > > >>> > >>> >>> want > > > > >>> > >>> >>>>>> the `sasl.server.` prefix (we just need to validate > > > > whether > > > > >>> the > > > > >>> > >>> config > > > > >>> > >>> >>> is > > > > >>> > >>> >>>>>> supported for the protocol). You can choose whether > a > > > > >>> boolean > > > > >>> > >>> flag or a > > > > >>> > >>> >>>>>> time interval makes more sense. > > > > >>> > >>> >>>>>> > > > > >>> > >>> >>>>>> For the client-side, the KIP explains how to make it > > > work > > > > >>> for > > > > >>> > >>> other > > > > >>> > >>> >>>>>> mechanisms and we can leave it that. Not convinced > > about > > > > >>> > >>> server-side > > > > >>> > >>> >>>>>> though. It seems to me that we probably would > require > > > API > > > > >>> > changes > > > > >>> > >>> to > > > > >>> > >>> >>> make > > > > >>> > >>> >>>>>> it work. Basically the proposed approach works only > > for > > > > >>> > >>> OAUTHBEARER. > > > > >>> > >>> >>>>>> Since > > > > >>> > >>> >>>>>> we have to bump up SaslHandshakeRequest version in > > this > > > > >>> KIP, it > > > > >>> > >>> will be > > > > >>> > >>> >>>>>> good to work out if we need to change the request or > > > flow > > > > to > > > > >>> > make > > > > >>> > >>> this > > > > >>> > >>> >>>>>> work > > > > >>> > >>> >>>>>> for other mechanisms. I haven't figured out exactly > > what > > > > is > > > > >>> > >>> needed, but > > > > >>> > >>> >>>>>> will think about it and get back next week. In the > > > > >>> meantime, you > > > > >>> > >>> can > > > > >>> > >>> >>> get > > > > >>> > >>> >>>>>> the KIP up-to-date with the config, migration path > > etc. > > > > and > > > > >>> get > > > > >>> > it > > > > >>> > >>> >>> ready > > > > >>> > >>> >>>>>> to > > > > >>> > >>> >>>>>> start vote next week. > > > > >>> > >>> >>>>>> > > > > >>> > >>> >>>>>> > > > > >>> > >>> >>>>>> > > > > >>> > >>> >>>>>> > > > > >>> > >>> >>>>>> > > > > >>> > >>> >>>>>> On Fri, Sep 14, 2018 at 1:24 PM, Ron Dagostino < > > > > >>> > rndg...@gmail.com > > > > >>> > >>> > > > > > >>> > >>> >>>>>> wrote: > > > > >>> > >>> >>>>>> > > > > >>> > >>> >>>>>>> Hi Rajini. > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> Agreed, we will bump the SaslHandshakeRequest API > > > number > > > > >>> (no > > > > >>> > wire > > > > >>> > >>> >>>>>> format > > > > >>> > >>> >>>>>>> change, of course). > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> Agreed, responding to re-authentication will always > > be > > > > >>> enabled > > > > >>> > >>> on the > > > > >>> > >>> >>>>>>> broker since it is initiated by the client. > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> Agreed, connection termination by the broker will > be > > > > >>> opt-in. > > > > >>> > I'm > > > > >>> > >>> >>>>>> thinking > > > > >>> > >>> >>>>>>> *sasl.server.disconnect.expired.credential. > > connections. > > > > >>> > enable*, > > > > >>> > >>> >>>>>> prefixed > > > > >>> > >>> >>>>>>> with listener and SASL mechanism name in lower-case > > so > > > it > > > > >>> can > > > > >>> > be > > > > >>> > >>> >>>>>> opt-in at > > > > >>> > >>> >>>>>>> the granularity of a SASL mechanism; for example, " > > > > >>> > >>> >>>>>>> listener.name.sasl_ssl.oauthbearer.sasl.server. > > > > >>> > >>> >>>>>>> disconnect.expired.credential. > > connections.enable=true > > > > >>> > >>> >>>>>>> ". It is long, so if you have a preference for a > > > shorter > > > > >>> name > > > > >>> > >>> let me > > > > >>> > >>> >>>>>> know > > > > >>> > >>> >>>>>>> and we'll go with it instead. > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> Agreed, additional metrics will be helpful. I'll > add > > > > them > > > > >>> to > > > > >>> > the > > > > >>> > >>> >>> KIP. > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> <<<need to work out exactly how this works with all > > > SASL > > > > >>> > >>> mechanisms > > > > >>> > >>> >>>>>>> <<<not sure we have the data required on the broker > > or > > > a > > > > >>> way of > > > > >>> > >>> >>>>>>> <<< extending the [GSSAPI] mechanism > > > > >>> > >>> >>>>>>> Not sure I agree here. The paragraph I added to > the > > > KIP > > > > >>> > >>> describes > > > > >>> > >>> >>> how > > > > >>> > >>> >>>>>> I > > > > >>> > >>> >>>>>>> think this can be done. Given that description, do > > you > > > > >>> still > > > > >>> > >>> feel > > > > >>> > >>> >>>>>> Kerberos > > > > >>> > >>> >>>>>>> will not be possible? If Kerberos presents a > > > significant > > > > >>> > hurdle > > > > >>> > >>> >>> then I > > > > >>> > >>> >>>>>>> don't think that should prevent us from doing it > for > > > > other > > > > >>> > >>> mechanisms > > > > >>> > >>> >>>>>> -- I > > > > >>> > >>> >>>>>>> would rather state that it isn't supported with > > GSSAPI > > > > due > > > > >>> to > > > > >>> > >>> >>>>>> limitations > > > > >>> > >>> >>>>>>> in Kerberos itself than not have it for > OAUTHBEARER, > > > for > > > > >>> > example. > > > > >>> > >>> >>> And > > > > >>> > >>> >>>>>>> right now I don't think we need more than a > > high-level > > > > >>> > >>> description of > > > > >>> > >>> >>>>>> how > > > > >>> > >>> >>>>>>> this could be done. In other words, I think we > have > > > this > > > > >>> > >>> covered, > > > > >>> > >>> >>>>>> unless > > > > >>> > >>> >>>>>>> there is a fundamental problem due to Kerberos not > > > making > > > > >>> data > > > > >>> > >>> (i.e. > > > > >>> > >>> >>>>>> ticket > > > > >>> > >>> >>>>>>> expiration) available on the server. I want to > > submit > > > > >>> this > > > > >>> > KIP > > > > >>> > >>> for > > > > >>> > >>> >>> a > > > > >>> > >>> >>>>>> vote > > > > >>> > >>> >>>>>>> early next week in the hope of having it accepted > by > > > the > > > > >>> > Monday, > > > > >>> > >>> 9/24 > > > > >>> > >>> >>>>>>> deadline for the 2.1.0 release, and if that > happens I > > > > >>> believe I > > > > >>> > >>> can > > > > >>> > >>> >>>>>> get a > > > > >>> > >>> >>>>>>> PR into really good shape soon thereafter (I'm > > working > > > on > > > > >>> it > > > > >>> > >>> now). > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> Ron > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>> On Fri, Sep 14, 2018 at 5:57 AM Rajini Sivaram < > > > > >>> > >>> >>>>>> rajinisiva...@gmail.com> > > > > >>> > >>> >>>>>>> wrote: > > > > >>> > >>> >>>>>>> > > > > >>> > >>> >>>>>>>> Hi Ron, > > > > >>> > >>> >>>>>>>> > > > > >>> > >>> >>>>>>>> 1) Yes, we should update the version of > > > > >>> > `SaslHandshakeRequest`. > > > > >>> > >>> >>>>>> Clients > > > > >>> > >>> >>>>>>>> would attempt to re-authenticate only if broker's > > > > >>> > >>> >>>>>> SaslHandshakeRequest > > > > >>> > >>> >>>>>>>> version >=2. > > > > >>> > >>> >>>>>>>> 2) I think we should enable broker's > > re-authentication > > > > and > > > > >>> > >>> >>> connection > > > > >>> > >>> >>>>>>>> termination code regardless of client version. > > > > >>> > Re-authentication > > > > >>> > >>> >>>>>> could be > > > > >>> > >>> >>>>>>>> always enabled since it is triggered only by > clients > > > and > > > > >>> > broker > > > > >>> > >>> >>> could > > > > >>> > >>> >>>>>>> just > > > > >>> > >>> >>>>>>>> handle it. Connection termination should be > opt-in, > > > but > > > > >>> should > > > > >>> > >>> work > > > > >>> > >>> >>>>>> with > > > > >>> > >>> >>>>>>>> clients of any version. If turned on and clients > are > > > of > > > > an > > > > >>> > older > > > > >>> > >>> >>>>>> version, > > > > >>> > >>> >>>>>>>> this would simply force reconnection, which should > > be > > > > ok. > > > > >>> > >>> >>> Additional > > > > >>> > >>> >>>>>>>> metrics to monitor expired connections may be > useful > > > > >>> anyway. > > > > >>> > >>> >>>>>>>> > > > > >>> > >>> >>>>>>>> We also need to work out exactly how this works > with > > > all > > > > >>> SASL > > > > >>> > >>> >>>>>> mechanisms. > > > > >>> > >>> >>>>>>>> In particular, we have ensure that we can make it > > work > > > > >>> with > > > > >>> > >>> >>> Kerberos > > > > >>> > >>> >>>>>>> since > > > > >>> > >>> >>>>>>>> we use the implementation from the JRE and I am > not > > > sure > > > > >>> we > > > > >>> > have > > > > >>> > >>> >>> the > > > > >>> > >>> >>>>>> data > > > > >>> > >>> >>>>>>>> required on the broker or a way of extending the > > > > >>> mechanism. > > > > >>> > >>> >>>>>>>> > > > > >>> > >>> >>>>>>>> Thoughts? > > > > >>> > >>> >>>>>>>> > > > > >>> > >>> >>>>>>>> On Thu, Sep 13, 2018 at 6:56 PM, Ron Dagostino < > > > > >>> > >>> rndg...@gmail.com> > > > > >>> > >>> >>>>>>> wrote: > > > > >>> > >>> >>>>>>>> > > > > >>> > >>> >>>>>>>>> Hi Rajini. I'm thinking about how we deal with > > > > >>> migration. > > > > >>> > >>> For > > > > >>> > >>> >>>>>>> example, > > > > >>> > >>> >>>>>>>>> let's say we have an existing 2.0.0 cluster using > > > > >>> > >>> >>> SASL/OAUTHBEARER > > > > >>> > >>> >>>>>> and > > > > >>> > >>> >>>>>>> we > > > > >>> > >>> >>>>>>>>> want to use this feature. The desired end state > is > > > to > > > > >>> have > > > > >>> > all > > > > >>> > >>> >>>>>> clients > > > > >>> > >>> >>>>>>>> and > > > > >>> > >>> >>>>>>>>> brokers migrated to a version that supports the > > > feature > > > > >>> (call > > > > >>> > >>> it > > > > >>> > >>> >>>>>> 2.x) > > > > >>> > >>> >>>>>>>> with > > > > >>> > >>> >>>>>>>>> the feature turned on. We need to document the > > > > supported > > > > >>> > >>> path(s) > > > > >>> > >>> >>>>>> to > > > > >>> > >>> >>>>>>> this > > > > >>> > >>> >>>>>>>>> end state. > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> Here are a couple of scenarios with implications: > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> 1) *Migrate client to 2.x and turn the feature on > > but > > > > >>> broker > > > > >>> > is > > > > >>> > >>> >>>>>> still > > > > >>> > >>> >>>>>>> at > > > > >>> > >>> >>>>>>>>> 2.0.* In this case the client is going to get an > > > error > > > > >>> when > > > > >>> > it > > > > >>> > >>> >>>>>> sends > > > > >>> > >>> >>>>>>> the > > > > >>> > >>> >>>>>>>>> SaslHandshakeRequest. We want to avoid this. It > > > seems > > > > >>> to me > > > > >>> > >>> the > > > > >>> > >>> >>>>>>> client > > > > >>> > >>> >>>>>>>>> needs to know if the broker has been upgraded to > > the > > > > >>> > necessary > > > > >>> > >>> >>>>>> version > > > > >>> > >>> >>>>>>>>> before trying to re-authenticate, which makes me > > > > believe > > > > >>> we > > > > >>> > >>> need > > > > >>> > >>> >>> to > > > > >>> > >>> >>>>>>> bump > > > > >>> > >>> >>>>>>>>> the API version number in 2.x and the client has > to > > > > >>> check to > > > > >>> > >>> see > > > > >>> > >>> >>>>>> if the > > > > >>> > >>> >>>>>>>> max > > > > >>> > >>> >>>>>>>>> version the broker supports meets a minimum > > standard > > > > >>> before > > > > >>> > >>> >>> trying > > > > >>> > >>> >>>>>> to > > > > >>> > >>> >>>>>>>>> re-authenticate. Do you agree? > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> 2) *Migrate broker to 2.x and turn the feature on > > but > > > > >>> client > > > > >>> > is > > > > >>> > >>> >>>>>> still > > > > >>> > >>> >>>>>>> at > > > > >>> > >>> >>>>>>>>> 2.0*. In this case the broker is going to end up > > > > killing > > > > >>> > >>> >>>>>> connections > > > > >>> > >>> >>>>>>>>> periodically. Again, we want to avoid this. > It's > > > > >>> tempting > > > > >>> > to > > > > >>> > >>> >>> say > > > > >>> > >>> >>>>>>>> "don't > > > > >>> > >>> >>>>>>>>> do this" but I wonder if we can require > > installations > > > > to > > > > >>> > >>> upgrade > > > > >>> > >>> >>>>>> all > > > > >>> > >>> >>>>>>>>> clients before turning the feature on at the > > brokers. > > > > >>> > >>> Certainly > > > > >>> > >>> >>>>>> we can > > > > >>> > >>> >>>>>>>> say > > > > >>> > >>> >>>>>>>>> "don't do this" for inter-broker clients -- in > > other > > > > >>> words, > > > > >>> > we > > > > >>> > >>> >>> can > > > > >>> > >>> >>>>>> say > > > > >>> > >>> >>>>>>>> that > > > > >>> > >>> >>>>>>>>> all brokers in a cluster should be upgraded > before > > > the > > > > >>> > feature > > > > >>> > >>> is > > > > >>> > >>> >>>>>>> turned > > > > >>> > >>> >>>>>>>> on > > > > >>> > >>> >>>>>>>>> for any one of them -- but I don't know about our > > > > >>> ability to > > > > >>> > >>> say > > > > >>> > >>> >>>>>> it for > > > > >>> > >>> >>>>>>>>> non-broker clients. > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> Now we consider the cases where both the brokers > > and > > > > the > > > > >>> > >>> clients > > > > >>> > >>> >>>>>> have > > > > >>> > >>> >>>>>>>> been > > > > >>> > >>> >>>>>>>>> upgraded to 2.x. When and where should the > feature > > > be > > > > >>> turned > > > > >>> > >>> on? > > > > >>> > >>> >>>>>> The > > > > >>> > >>> >>>>>>>>> inter-broker case is interesting because I don't > > > think > > > > >>> think > > > > >>> > we > > > > >>> > >>> >>> can > > > > >>> > >>> >>>>>>>> require > > > > >>> > >>> >>>>>>>>> installations to restart every broker with a new > > > config > > > > >>> where > > > > >>> > >>> the > > > > >>> > >>> >>>>>>> feature > > > > >>> > >>> >>>>>>>>> is turned on at the same time. Do you agree that > > we > > > > >>> cannot > > > > >>> > >>> >>> require > > > > >>> > >>> >>>>>>> this > > > > >>> > >>> >>>>>>>>> and therefore must support installations > restarting > > > > >>> brokers > > > > >>> > >>> with > > > > >>> > >>> >>> a > > > > >>> > >>> >>>>>> new > > > > >>> > >>> >>>>>>>>> config at whatever pace they need -- which may be > > > quite > > > > >>> slow > > > > >>> > >>> >>>>>> relative > > > > >>> > >>> >>>>>>> to > > > > >>> > >>> >>>>>>>>> token lifetimes? Assuming you do agree, then > there > > > is > > > > >>> going > > > > >>> > to > > > > >>> > >>> >>> be > > > > >>> > >>> >>>>>> the > > > > >>> > >>> >>>>>>>> case > > > > >>> > >>> >>>>>>>>> where some brokers are going to have the feature > > > turned > > > > >>> on > > > > >>> > and > > > > >>> > >>> >>> some > > > > >>> > >>> >>>>>>>> clients > > > > >>> > >>> >>>>>>>>> (definitely inter-broker clients at a minimum) > are > > > > going > > > > >>> to > > > > >>> > >>> have > > > > >>> > >>> >>>>>> the > > > > >>> > >>> >>>>>>>>> feature turned off. > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> The above discussion assumes a single config on > the > > > > >>> broker > > > > >>> > side > > > > >>> > >>> >>>>>> that > > > > >>> > >>> >>>>>>>> turns > > > > >>> > >>> >>>>>>>>> on both the inter-broker client re-authentication > > > > >>> feature as > > > > >>> > >>> well > > > > >>> > >>> >>>>>> as > > > > >>> > >>> >>>>>>> the > > > > >>> > >>> >>>>>>>>> server-side expired-connection-kill feature, but > > now > > > > I'm > > > > >>> > >>> thinking > > > > >>> > >>> >>>>>> we > > > > >>> > >>> >>>>>>> need > > > > >>> > >>> >>>>>>>>> the ability to turn these features on > > independently, > > > > plus > > > > >>> > maybe > > > > >>> > >>> >>> we > > > > >>> > >>> >>>>>>> need a > > > > >>> > >>> >>>>>>>>> way to monitor the number of "active, expired" > > > > >>> connections to > > > > >>> > >>> the > > > > >>> > >>> >>>>>>> server > > > > >>> > >>> >>>>>>>> so > > > > >>> > >>> >>>>>>>>> that operators can be sure that all clients have > > been > > > > >>> > >>> >>>>>> upgraded/enabled > > > > >>> > >>> >>>>>>>>> before turning on the server-side > > > > expired-connection-kill > > > > >>> > >>> >>> feature. > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> So the migration plan would be as follows: > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> 1) Upgrade all brokers to 2.x. > > > > >>> > >>> >>>>>>>>> 2) After all brokers are upgraded, turn on > > > > >>> re-authentication > > > > >>> > >>> for > > > > >>> > >>> >>>>>> all > > > > >>> > >>> >>>>>>>>> brokers at whatever rate is desired -- just > > > eventually, > > > > >>> at > > > > >>> > some > > > > >>> > >>> >>>>>> point, > > > > >>> > >>> >>>>>>>> get > > > > >>> > >>> >>>>>>>>> the client-side feature turned on for all brokers > > so > > > > that > > > > >>> > >>> >>>>>> inter-broker > > > > >>> > >>> >>>>>>>>> connections are re-authenticating. > > > > >>> > >>> >>>>>>>>> 3) In parallel with (1) and (2) above, upgrade > > > clients > > > > >>> to 2.x > > > > >>> > >>> and > > > > >>> > >>> >>>>>> turn > > > > >>> > >>> >>>>>>>>> their re-authentication feature on. Clients will > > > check > > > > >>> the > > > > >>> > API > > > > >>> > >>> >>>>>> version > > > > >>> > >>> >>>>>>>> and > > > > >>> > >>> >>>>>>>>> only re-authenticate to a broker that has also > been > > > > >>> upgraded > > > > >>> > >>> >>> (note > > > > >>> > >>> >>>>>> that > > > > >>> > >>> >>>>>>>> the > > > > >>> > >>> >>>>>>>>> ability of a broker to respond to a > > re-authentication > > > > >>> cannot > > > > >>> > be > > > > >>> > >>> >>>>>> turned > > > > >>> > >>> >>>>>>>> off > > > > >>> > >>> >>>>>>>>> -- it is always on beginning with version 2.x, > and > > it > > > > >>> just > > > > >>> > sits > > > > >>> > >>> >>>>>> there > > > > >>> > >>> >>>>>>>> doing > > > > >>> > >>> >>>>>>>>> nothing if it isn't exercised by an enabled > client) > > > > >>> > >>> >>>>>>>>> 4) After (1), (2), and (3) are complete, check > the > > > 2.x > > > > >>> broker > > > > >>> > >>> >>>>>> metrics > > > > >>> > >>> >>>>>>> to > > > > >>> > >>> >>>>>>>>> confirm that there are no "active, expired" > > > > connections. > > > > >>> > Once > > > > >>> > >>> >>> you > > > > >>> > >>> >>>>>> are > > > > >>> > >>> >>>>>>>>> satisfied that (1), (2), and (3) are indeed > > complete > > > > you > > > > >>> can > > > > >>> > >>> >>>>>> enable the > > > > >>> > >>> >>>>>>>>> server-side expired-connection-kill feature on > each > > > > >>> broker > > > > >>> > via > > > > >>> > >>> a > > > > >>> > >>> >>>>>>> restart > > > > >>> > >>> >>>>>>>> at > > > > >>> > >>> >>>>>>>>> whatever pace is desired. > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> Comments? > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> Ron > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>> On Wed, Sep 12, 2018 at 4:48 PM Ron Dagostino < > > > > >>> > >>> rndg...@gmail.com > > > > >>> > >>> >>>> > > > > >>> > >>> >>>>>>> wrote: > > > > >>> > >>> >>>>>>>>> > > > > >>> > >>> >>>>>>>>>> Ok, I am tempted to just say we go with the > > > low-level > > > > >>> > approach > > > > >>> > >>> >>>>>> since > > > > >>> > >>> >>>>>>> it > >
