From dev-return-107752-archive-asf-public=cust-asf.ponee.io@kafka.apache.org Fri Sep 20 20:39:49 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 7EB9D180608 for ; Fri, 20 Sep 2019 22:39:49 +0200 (CEST) Received: (qmail 6891 invoked by uid 500); 20 Sep 2019 20:39:48 -0000 Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@kafka.apache.org Delivered-To: mailing list dev@kafka.apache.org Received: (qmail 6877 invoked by uid 99); 20 Sep 2019 20:39:47 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd4-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 20 Sep 2019 20:39:47 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd4-us-west.apache.org (ASF Mail Server at spamd4-us-west.apache.org) with ESMTP id 54BF1C2394 for ; Fri, 20 Sep 2019 20:39:47 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd4-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.801 X-Spam-Level: * X-Spam-Status: No, score=1.801 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Authentication-Results: spamd4-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-he-de.apache.org ([10.40.0.8]) by localhost (spamd4-us-west.apache.org [10.40.0.11]) (amavisd-new, port 10024) with ESMTP id qShQ_y9uCZhY for ; Fri, 20 Sep 2019 20:39:44 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=2607:f8b0:4864:20::733; helo=mail-qk1-x733.google.com; envelope-from=maulin.vasavada@gmail.com; receiver= Received: from mail-qk1-x733.google.com (mail-qk1-x733.google.com [IPv6:2607:f8b0:4864:20::733]) by mx1-he-de.apache.org (ASF Mail Server at mx1-he-de.apache.org) with ESMTPS id 6146A7DD72 for ; Fri, 20 Sep 2019 20:39:43 +0000 (UTC) Received: by mail-qk1-x733.google.com with SMTP id y189so8660148qkc.3 for ; Fri, 20 Sep 2019 13:39:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=vqaKruqFUJdc0ewPFXA08uV/guJR9scD5TOjHaN4lPk=; b=LAOkomVGmUrG1InayHkfaR9DXwkNJnPVqoRgC0OD3l9+D4/Krh5nQfhAZOH9TaCtRp tKZRN5zKcyVIeaTKvXZv2s4E1sL3fDVCRgOtF/W2gW+XbkGNVMaID52ucqQC6YNxQgo6 rGKZ/rSgBYp/IJj3KTyPFb+PlinDce5b95X0fZevk9a7QcIVMvXSvMlV4o6lc2SHe8GF U850HJJWoNcvj/OjubwHJCRio2B3BnYxGDP2vE0RtDIYgx7pC3lYUlsgyuz9LXPy3vAT YTxAt9o532LcaZ6udm0Tu2ZnD0pS56ku65aZDOEzfLU1BCmLLqhxquSbvezNeuX6JORM xK3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=vqaKruqFUJdc0ewPFXA08uV/guJR9scD5TOjHaN4lPk=; b=uT55QXGeagyY3Et7ebHmQOq0Z6e3GyTOtOjFudvZuGgDks/ax8zE+eBkeIQNOQp/QL GfgP+d944TMXSRRUgXCFrLlMwivGOyMDl59psfpkCDiG+UfZipZY+MzR63MPjMiWmbDl SNpVjsEFLDwEjdiQ1Ju2gJQ6n/CJBBHkE8j7Naqq1ZqRn8zfeNLD6c8OYId3klYvwOpe ckivmodt1YRxmnHXvvylz2TlFaKrdoXArJ+Mh68tyaPOV8vMboeZIOcU3i9TWbEP7QEj RsADwY5LUcPaRgV2gYvQ6uYaXInQvIXxzozqCnm/MZyxXAjH9wieTSh7auItnszxcUoE UH9Q== X-Gm-Message-State: APjAAAW/lBX9Xwnk4JhLA49f1BIcqAf1zMdsgoXcesCl6ZwEL35KHwO4 FN26WzZ1JcXs6vripdukQLXljgVGXTRx1L66q6CL4szSPsM= X-Google-Smtp-Source: APXvYqwmkFoypWUdiR0Bas8yippTg/x4Ue3PUcvATTJvc3X4jtzRi6ufWVqDLnl4Jdec4wGQc9NeZy37ULTqTT5j2F4= X-Received: by 2002:a37:8c84:: with SMTP id o126mr5672135qkd.467.1569011981947; Fri, 20 Sep 2019 13:39:41 -0700 (PDT) MIME-Version: 1.0 References: <4d29a2f0ea9047ba8ef05c9a412a11d4@IBIMAILD.ibi.com> <338d372484ac47b292f4a28b045de8b5@IBIMAILD.ibi.com> <6d0147bccb644e29adfebdfca01b1461@IBIMAILD.ibi.com> <2482c6fe4926434888c6d7cad5e587cc@IBIMAILD.ibi.com> <564fc318026848bd8b06ae9767264e04@IBIMAILD.ibi.com> In-Reply-To: <564fc318026848bd8b06ae9767264e04@IBIMAILD.ibi.com> From: Maulin Vasavada Date: Fri, 20 Sep 2019 13:39:30 -0700 Message-ID: Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible To: dev@kafka.apache.org Content-Type: multipart/alternative; boundary="000000000000d5199f0593021105" --000000000000d5199f0593021105 Content-Type: text/plain; charset="UTF-8" Thanks Clement for your thoughts. According to my current experience rewriting the code twice I would say I did what you suggest in the last point - " We must make an attempt, if only to explain why it fails in the Rejected Alternatives section of the KIP." In the rejected alternatives I already mention why not merge SslFactory and SslEngineFactory and make SslFactory reconfigurable. @Colin can you please express your thoughts on this discussion so far? Since you refactored the SslFactory's code to have SslEngineBuilder I feel you would have more insights into future changes. Thanks Maulin On Fri, Sep 20, 2019 at 7:29 AM Pellerin, Clement wrote: > There will be chaperon code in the base class of the channel builders. > The arguments you gave me are emotional not technical. > The SSL extension point is reconfigurable hence it should extend > Reconfigurable. > We will encounter issues when we try to prototype it that way. > We will solve the issues or backtrack to another solution. > We must make an attempt, if only to explain why it fails in the Rejected > Alternatives section of the KIP. > > -----Original Message----- > From: Maulin Vasavada [mailto:maulin.vasavada@gmail.com] > Sent: Friday, September 20, 2019 2:40 AM > To: dev@kafka.apache.org > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > extensible > > Overall my thinking is - When somebody wants to customize creation of > SSLEngine, most likely they are more expert in dealing with SSL domain > related stuff than "Kafka's reconfigurability" aspect. As a custom > implementation it makes more sense to me to say - Hey I'll control how I > initialize my SSL context/engine and btw if Kafka can give me a way to just > get re-created whenever some set of config keys changes, it is great! This > is similar to my thinking on KIP-486 which is- as a Kafka operator I am > just trying to be compliant to my company's security policies to load > keys/certs in certain way. For that, I should not be penalized by Kafka to > know all about Java security Providers and how to really create SSLContext > object etc given Java already provides a way to feed in KeyStore object > regardless of how I load it. > > On Thu, Sep 19, 2019 at 10:57 PM Maulin Vasavada < > maulin.vasavada@gmail.com> > wrote: > > > Hi Clement > > > > There will be good amount of state in the SslEngineFactory's default > > implementation. Hence I feel we might anyway have a chaperon class to > > provide reconfigurable functionality and will have one more class to host > > the state/behavior of actual SSLContext/SSLEngine creation. While doing > the > > internal rewrite (so far two times) both of the times I reached to the > same > > conclusion.I feel if we leave the reconfigurations to the implementation > - > > it will repeat the same pattern of having two classes to manage it - > since > > most likely they will also have similar state information. Instead keep > > that reconfigurations in SslFactory as is today and just allow "plugin of > > creation of SSLEngine". > > > > One note I would like to make is: You are comparing this to > MetricReporter > > but we have to keep in mind that SSL configuration is inherently more > > complex than a MetricReporters functionality. There are no JSSE > equivalent > > documents needed to be written for MetricReporter for example. So what > > works best for simpler solutions may not work equally well for more > complex > > scenarios. > > > > > > Thanks > > Maulin > > > > > > On Thu, Sep 19, 2019 at 8:36 PM Pellerin, Clement < > > Clement_Pellerin@ibi.com> wrote: > > > >> We will get there eventually but I need to address another point first. > >> > >> My goal is to do exactly what the "other extension points with > >> reconfigurable custom configs" are doing unless there is a good reason > not > >> to. They provide a ready-made solution that will let us reuse code, > avoid > >> pitfalls and show consistency. > >> > >> So far the roadblocks are > >> - the need to enforce mandatory compatibility checks for the keystores > >> and SSL handshake > >> - SslFactory is used in two channel builders. > >> > >> Both of these roadblocks can be overcome by moving the checks to a new > >> common base class of SslChannelBuilder and SaslChannelBuilder. This is > easy > >> since both classes extend Object directly. The new base class is not a > >> public API so any implementation will do. The chaperon class SslFactory > >> disappears and the interface extends Reconfigurable. > >> > >> Does this proposal address all the reasons you had not to do exactly > what > >> other extension points are doing? > >> > >> -----Original Message----- > >> From: Maulin Vasavada [mailto:maulin.vasavada@gmail.com] > >> Sent: Thursday, September 19, 2019 10:21 PM > >> To: dev@kafka.apache.org > >> Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > >> extensible > >> > >> Hi Clement > >> > >> So assuming there are two classes - SslFactory and SslEngineFactory > like I > >> suggested in my detailed post before this, we can use > >> config.getConfiguredInstance() in SslFactory for SslEngineFactory class > >> configuration and then followed by init() method. I don't see a > challenge > >> there. Can you please provide your input on my detailed post along with > >> this recent point I am making? > >> > >> Thanks > >> Maulin > >> > >> On Thu, Sep 19, 2019 at 5:04 PM Maulin Vasavada < > >> maulin.vasavada@gmail.com> > >> wrote: > >> > >> > Hi Clement, > >> > > >> > Thanks for pointing to AbstractConfig. Now I understand what you were > >> > saying. I'll respond by tonight with more thoughts. > >> > > >> > Thanks > >> > Maulin > >> > > >> > On Thu, Sep 19, 2019 at 5:46 AM Pellerin, Clement < > >> > Clement_Pellerin@ibi.com> wrote: > >> > > >> >> I appreciate the effort you put into this. > >> >> > >> >> Lets do this in steps. You had a question on getConfiguredInstance(). > >> >> > >> >> The method getConfiguredInstance(key, Class) implemented in > >> >> AbstractConfig is how the MetricsReporter and other extension points > >> are > >> >> intantiated. Creating the extension point this way calls the default > >> >> constructor which is good. Since the (Re)Configurable interface > >> dictates > >> >> the signature of the configure() method, that forces the addition of > a > >> new > >> >> init(...) method to pass the other constructor arguments. > >> >> > >> >> Do we agree on that before we move on to other issues? > >> >> > >> >> -----Original Message----- > >> >> From: Maulin Vasavada [mailto:maulin.vasavada@gmail.com] > >> >> Sent: Wednesday, September 18, 2019 4:37 PM > >> >> To: dev@kafka.apache.org > >> >> Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > >> >> extensible > >> >> > >> >> Hi Clement > >> >> > >> >> Here are my thoughts based on my latest re-write attempt and > learnings, > >> >> > >> >> 1. I think that it will be a great value to keep both classes > separate > >> - > >> >> SslFactory and SslEngineFactory and having method > >> reconfigurableConfigs() > >> >> in the SslEngineFactory. Here is the reasoning, > >> >> > >> >> a. It is kind of a Decorator pattern to me - even without named like > >> one > >> >> SslFactory is acting as a decorator/surrogate to the SslEngineFactory > >> and > >> >> helping it get created and re-created as needed based on the > >> >> terms/conditions specified by SslEngineFactory (via > >> >> reconfigurableConfigs() > >> >> method) > >> >> > >> >> b. SslEngineFactory will be pluggable class. By keeping the > SslFactory > >> >> reconfigurable with delegation of reconfigurableConfigs() to > >> >> SslEngineFactory it allows the implementation of SslEngineFactory to > be > >> >> worry free of - How Kafka manages reconfigurations. The contract is - > >> >> Kafka's SslFactory will ask the implementation to provide which > >> >> configurations it is ready to be reconfigured for. Rest of the logic > >> for > >> >> triggering and reconfiguring and validation is in SslFactory. > >> >> > >> >> c. The current validation in SslFactory about inter-broker-ssl > >> handshake > >> >> AND verifying that certificate chain doesn't change via dynamic > config > >> >> changes is rightly owned by SslFactory. We should not give > flexibility > >> to > >> >> SslEngineFactory to decide if they want that validation or not. > >> >> > >> >> d. If SslEngineFactory fails to be re-created with new dynamic config > >> >> changes the constructor will throw some exception and the SslFactory > >> will > >> >> fail the validateReconfiguration() call resulting in no-change. Hence > >> the > >> >> validation if the new config is right is still controlled by the > >> >> SslEngineFactory without necessarily having explicit validate method > >> >> (assuming if you had a point about - we should keep validation of > >> changed > >> >> configs in the pluggable class) > >> >> > >> >> > >> >> 2. About the keystore validation in SslFactory - as I mentioned in > >> above > >> >> points, > >> >> > >> >> a. I feel it is Kafka's policy that it wants to mandate that > validation > >> >> regardless of the SslEngineFactory's implementation. I feel that > >> >> regardless > >> >> of customized implementation it is doing a 'logical' enforcement. I > >> don't > >> >> see many cases where you will end up changing certificate chain (I > >> can't > >> >> say the same about SANs entries though. see my below points). Hence > >> that > >> >> validation is reasonable to be generally enforced for dynamic config > >> >> changes. If you change something violating that validation, you can > >> avoid > >> >> making such changes via dynamic configuration and do a rolling > >> restarts of > >> >> the boxes. > >> >> > >> >> b. If the implementation doesn't use keystore then automatically no > >> >> validation will happen. Hence I don't see any issue with > >> >> SslEngineFactory's > >> >> implementations not having requirement to use keystores. > >> >> > >> >> c. There could be an argument however about - what it validates > >> currently > >> >> and is there a scope of change. Example: It validates SANs entries > and > >> >> that > >> >> to me is a challenge because I have had scenarios where I kept adding > >> more > >> >> VIPs in my certs SANs entries without really changing any certificate > >> >> chain. The existing validation will fail that setup unnecessarily. > >> Given > >> >> that - there could be change in SslFactory but that doesn't still > make > >> >> that > >> >> validation eligible to go to SslEngineFactory implementations. > >> >> > >> >> > >> >> 3. I am still in two minds about your point on - not using existing > SSL > >> >> Reconfigurable configs to be used by SslFactory on top of > >> >> SslEngineFactory's reconfigurable configs. The reason for that is- > >> >> > >> >> a. I agree with you on that we should not worry about existing SSL > >> >> reconfigurable configs in new changed code for SslFactory. Why depend > >> on > >> >> something you really don't need. However, Rajini's point is- if we > >> decide > >> >> to add more configs in the SSL reconfigurable configs which may be > >> common > >> >> across SslEngineFactory's implementations, it will make it easier. > >> Again, > >> >> just to make it easier we should not do it upfront. So now you see > why > >> I > >> >> am > >> >> double minded on it while more leaning toward your suggestion. > >> >> > >> >> 4. I think I totally miss what you refer by > >> >> config.getConfiguredInstance(key, Class). Which Kafka existing class > >> you > >> >> are referring to when you do that? Do we have that in KafkaConfig? If > >> you > >> >> can clarify me on that I can think more about your input on it. > >> >> > >> >> 5. Now above all means- > >> >> > >> >> a. We will have createEngine(), reconfigurableConfigs(), keystore(), > >> >> truststore() methods in the SslEngineFactory interface. However the > >> return > >> >> type for keystore and truststore method can't be existing > >> SecurityStore. > >> >> For that I already thought of the replacement with KeystoreHolder > class > >> >> which only contains references to java's KeyStore object and Kafka's > >> >> Password object making it feasible for us to return > non-implementation > >> >> specific return type. > >> >> > >> >> b. We didn't talk about shouldBeRebuilt() so far at all given other > >> >> important conflicts to resolve. We will get to it once we can hash > out > >> >> other stuff. > >> >> > >> >> 6. On Rajini's point on 'push notifications' for client side code > when > >> the > >> >> key/trust store changes, > >> >> > >> >> " - For client-side, custom SslEngineFactory implementations could > >> >> reconfigure themselves, we don't really need SslFactory to be > >> involved > >> >> at all." > >> >> > >> >> I think I am missing something. If we just have SslEngineFactory > >> >> reconfigure itself it will generate new SSLContext and in-turn new > >> >> SSLEgnine but how will it communicate that to the ChannelBuilders? > >> Don't > >> >> they have to refresh the reference to the SslEngineFactory via > >> >> SslFactory's > >> >> reconfigure() method in order to pick up that change? > >> >> > >> >> > >> >> Thanks > >> >> Maulin > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> On Tue, Sep 17, 2019 at 4:49 AM Pellerin, Clement < > >> >> Clement_Pellerin@ibi.com> > >> >> wrote: > >> >> > >> >> > Good point about the two callers of SslFactory. We can move the > >> >> SslEngine > >> >> > validation to a separate class and call it in both places. That > >> >> SslEngine > >> >> > validation class would not be part of the public API and therefore > we > >> >> don't > >> >> > need to fuss about its API. > >> >> > > >> >> > -----Original Message----- > >> >> > From: Maulin Vasavada [mailto:maulin.vasavada@gmail.com] > >> >> > Sent: Tuesday, September 17, 2019 2:28 AM > >> >> > To: dev@kafka.apache.org > >> >> > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine > configuration > >> >> > extensible > >> >> > > >> >> > Hi Clement/Rajini > >> >> > > >> >> > When I read your responses - I swing between both of your > suggestions > >> >> :) I > >> >> > see both of your points. Let me ponder little bit more and give me > >> take > >> >> in > >> >> > a day or so. > >> >> > > >> >> > I tend to agree with Clement in a sense that we need to define > clear > >> >> > responsibilities of classes. Right now I feel it's not clear. > Also, I > >> >> tend > >> >> > to agree to both of you about keystore/truststore validation - the > >> >> conflict > >> >> > I've to propose a clean agreeable solution to. > >> >> > > >> >> > One clarification to Clement is - there are two classes using > >> SslFactory > >> >> > today - SslChannelBuilder and SaslChannelBuilder so we have to keep > >> >> that in > >> >> > mind. However, once we have clear responsibilities of classes, that > >> >> should > >> >> > automatically clear what goes where. > >> >> > > >> >> > Thanks > >> >> > Maulin > >> >> > > >> >> > >> > > >> > > > --000000000000d5199f0593021105--