Stefan Miklosovic <https://cwiki.apache.org/confluence/display/~stefan.miklosovic>
Hi MAULIN VASAVADA <https://cwiki.apache.org/confluence/display/~maulin.vasavada>, few more observations. I see that you have commented again on JIRA and I am starting to be confused where to comment in relation to recent thread we had about this so I am letting you know that I am ultimately using this communication channel for discussion. In the context of your latest answers on JIRA - your interface makes sense to me, I just want to be sure that we will not forget to add anything which would a respective implementator need in the future and could not use because it is just not exposed. I am not completely sure how to solve this but I think that we just have to stick to our gut feeling that the solution proposed will cover the most scenarios. If we do not feel safe, my idea was to show yet another implementation where the possibility we would left a user behind is minimised. Otherwise, without breaking older implementations used in future releases, we could only introduce methods which would have default implementations. I prefer to have a map instead of common encryption options. On the other hand, I can imagine that the custom implementation would try to bypass some credentials into it (for example how to connect to a respective source of these keystores / truststores) and if we ever decided to have some kind of a tooling around this, e.g. in nodetool, to get a status of "how ssl is configured", we might unintentionally leak security sensitive information (credentials) by displaying them in plaintext in such tooling. We are using JMX for this (I might expand on this if you are not familiar with that mechanism of getting runtime info from Cassandra via JMX). Hence what we might do is to actually not expose that map at all. We are not exposing this kind of information yet in runtime and I do not think we actually have a need for that I just find it important to say. I like the fact that configuration parameters for an implementation are coupled with that factory configuration and it is just a basic map. Since implementations are getting their EncryptionOptions + map of customs, I prefer this instead of putting there whole map of parameters because then you are just again "parsing it" from map in respective constructors. There is nothing wrong with how this is done in your original PR, I would say. The very same pattern of "maps" may be found across the code base, e.g. auditing and similar. On Fri, Jul 9, 2021 at 2:59 PM Maulin Vasavada <maulin.vasav...@gmail.com> wrote: > Stefan Miklosovic > <https://cwiki.apache.org/confluence/display/~stefan.miklosovic> > > I ve taken a look too. Added some comments to PR. > > It would be awesome if we see some 3rd party implementation of this in > action so we know it indeed works as intended. It is strange to just code > up an interface by default logic for which it works but there isnt any > (public) example how to do yet another impl. > > there is a directory called "examples" in the root of the repository. > > > > > On Fri, Jul 9, 2021 at 2:58 PM Maulin Vasavada <maulin.vasav...@gmail.com> > wrote: > >> [image: maulin.vasavada]Maulin Vasavada >> <https://issues.apache.org/jira/secure/ViewProfile.jspa?name=maulin.vasavada> >> added >> a comment - Yesterday - edited >> >> On second thoughts Stefan Miklosovic >> <https://issues.apache.org/jira/secure/ViewProfile.jspa?name=stefan.miklosovic>, >> I feel if we examine the interface properly and make sure of the contract >> and dependencies - input arguments to the methods and construction of the >> implementation (for which I agree there is no good way given an interface) >> object AND the return types/exceptions, it could be evaluated without >> sample of a specific/custom implementation. The premise is very simple - >> Allow SSLContext (in this case JSSE's and Netty's) creation to be >> pluggable. Once we do that the specific implementation should not matter. >> However the Cassandra's current integration point with that pluggable >> interface does matter and need to make sure we are not violating existing >> behavior - example - Caching of the Netty's ssl contexts, invocation of >> context for Inbound/Outbound and Client/Native connections, threads running >> for enabling hot reloading periodically etc. I know its a long answer to >> your question but I have done very similar work for Apache Kafka ( >> reference >> <https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=128650952>) >> and feel confident that it will work for custom implementations (like ours >> is running in production for about 2 years now, albeit private >> implementation). I've consulted many security experts internally and >> externally to validate that - making SSLContext customizable/pluggable is >> the best way to support various specific needs of bigger eco-systems. >> >> >> >> In fact based on my evaluation of the design - I do have two sub options >> <https://cwiki.apache.org/confluence/display/CASSANDRA/CEP-9%3A+Make+SSLContext+creation+pluggable#CEP9:MakeSSLContextcreationpluggable-ImportantnoteaboutcommonSSLconfigurations> >> that >> I seek input from the community on - about consolidating all the encryption >> options as a open ended Map argument coming to the interface's >> implementation vs having a notion of CommonEncryptionOptions to keep some >> of the existing implementation as-is. >> >> On Fri, Jul 9, 2021 at 2:58 PM Maulin Vasavada <maulin.vasav...@gmail.com> >> wrote: >> >>> Hi Sumanth Pasupuleti >>> <https://issues.apache.org/jira/secure/ViewProfile.jspa?name=sumanth.pasupuleti> >>> and Stefan Miklosovic >>> <https://issues.apache.org/jira/secure/ViewProfile.jspa?name=stefan.miklosovic> >>> thanks >>> for comments. Sorry I missed them before since I was just checking DISCUSS >>> thread on the CEP >>> >>> >>> >>> Sumanth, I totally get what you are saying. However I feel the same way >>> as you do that this is just SSLContext pluggability change and your >>> suggestion should be a follow-up on the CEP-9 change. >>> >>> >>> >>> Stefan, your point is valid but I can only verify a custom >>> implementation with our company's internal requirements. However it may be >>> worth to provide a sample integration with HashiCorp Vault for example to >>> fetch keys/certs and have a PoC. Not sure where that sample can be hosted >>> though. >>> >>> Based on the latest discussion around improving CEP process, we may need >>> to copy paste this discussion to DISCUSS thread also. Can you please >>> post/copy your comments and I'd copy mine there? >>> >>> On Fri, Jul 9, 2021 at 2:58 PM Maulin Vasavada < >>> maulin.vasav...@gmail.com> wrote: >>> >>>> [image: stefan.miklosovic]Stefan Miklosovic >>>> <https://issues.apache.org/jira/secure/ViewProfile.jspa?name=stefan.miklosovic> >>>> added >>>> a comment - 01/Jul/21 19:20 >>>> >>>> I ve taken a look too. Added some comments to PR. >>>> >>>> It would be awesome if we see some 3rd party implementation of this in >>>> action so we know it indeed works as intended. It is strange to just code >>>> up an interface by default logic for which it works but there isnt any >>>> (public) example how to do yet another impl. >>>> >>>> On Fri, Jul 9, 2021 at 2:57 PM Maulin Vasavada < >>>> maulin.vasav...@gmail.com> wrote: >>>> >>>>> Sumanth Pasupuleti >>>>> <https://issues.apache.org/jira/secure/ViewProfile.jspa?name=sumanth.pasupuleti> >>>>> added >>>>> a comment - 07/Jun/21 15:13 >>>>> >>>>> >>>>> Maulin Vasavada >>>>> <https://issues.apache.org/jira/secure/ViewProfile.jspa?name=maulin.vasavada> >>>>> left >>>>> a couple of review comments, but lgtm overall. >>>>> >>>>> One of the things I was hoping we can also achieve is to be able to >>>>> provide mechanics to transparently transition from using one SSLFactory >>>>> implementation to another, and using those mechanics, one could do the >>>>> following on their cluster for moving from say Implementation1 to >>>>> Implementation2 >>>>> Stage #1: Current state of being only Implementation1 aware. Use >>>>> keystore and trustmanager of implementation1 >>>>> Stage #2: Start trusting both implementation1 and implementation2. Use >>>>> keystore of implementation1, but use trustmanager of both implementation1 >>>>> and implementation2 (using MultiTrustManagerFactory) (and perform a >>>>> rolling >>>>> restart of the cluster) >>>>> Stage #3: Start using implementation2 for keystore, and perform a >>>>> rolling restart of the cluster >>>>> Stage #4: At this point, all nodes of the cluster are using >>>>> implementation2 for keystore, but trust both implementation1 and >>>>> implementation2, and we can now remove implementation1 from trustmanagers, >>>>> and do a rolling restart. >>>>> >>>>> Since this ticket is about making SSLContext pluggable, I believe this >>>>> is out of scope; cut a separate ticket CASSANDRA-16719 >>>>> <https://issues.apache.org/jira/browse/CASSANDRA-16719> to track that >>>>> work (I did an internal 3.0 patch for this transition work, and I can try >>>>> porting it to 4.x once this ticket is committed) >>>>> >>>>> On Fri, Jul 9, 2021 at 2:56 PM Maulin Vasavada < >>>>> maulin.vasav...@gmail.com> wrote: >>>>> >>>>>> Hi all >>>>>> >>>>>> I wanted to consolidate a couple of comments that started in >>>>>> JIRA/Wiki here to keep it in one place. I'll post different posts as >>>>>> replies for each comment. >>>>>> >>>>>> Thanks >>>>>> Maulin >>>>>> >>>>>> On Tue, Jun 29, 2021 at 1:07 PM Maulin Vasavada < >>>>>> maulin.vasav...@gmail.com> wrote: >>>>>> >>>>>>> ^^^ bumping up ^^^ this thread since people might have more time >>>>>>> reviewing post 4.0 work. Specifically for this >>>>>>> <https://cwiki.apache.org/confluence/display/CASSANDRA/CEP-9%3A+Make+SSLContext+creation+pluggable#CEP9:MakeSSLContextcreationpluggable-ImportantnoteaboutcommonSSLconfigurations> >>>>>>> section in the CEP, I have coded for one option (here >>>>>>> <https://github.com/maulin-vasavada/cassandra/commit/256ad30ecedbc50d66d8a039f8ca9e47074737ce>) >>>>>>> and now will do for another option very soon. >>>>>>> >>>>>>> On Wed, Jun 2, 2021 at 5:11 PM Maulin Vasavada < >>>>>>> maulin.vasav...@gmail.com> wrote: >>>>>>> >>>>>>>> Thank you Dinesh and everybody. Will keep calm and wait for the >>>>>>>> feedback. Meanwhile I am experimenting with various implementation >>>>>>>> options >>>>>>>> for what I put as "will seek community's input >>>>>>>> <https://cwiki.apache.org/confluence/display/CASSANDRA/CEP-9%3A+Make+SSLContext+creation+pluggable#CEP9:MakeSSLContextcreationpluggable-ImportantnoteaboutcommonSSLconfigurations>" >>>>>>>> on the CEP document and learning little bit more about the CircleCI. >>>>>>>> >>>>>>>> On Wed, Jun 2, 2021 at 4:08 PM Dinesh Joshi >>>>>>>> <djos...@icloud.com.invalid> wrote: >>>>>>>> >>>>>>>>> Hi Maulin, >>>>>>>>> >>>>>>>>> Thank you for the CEP & Patch. I’ve been following along albeit >>>>>>>>> silently. Will take a look. It’s just that we’re currently busy so >>>>>>>>> bear >>>>>>>>> with us. >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> >>>>>>>>> Dinesh >>>>>>>>> >>>>>>>>> > On Jun 2, 2021, at 3:28 PM, Maulin Vasavada < >>>>>>>>> maulin.vasav...@gmail.com> wrote: >>>>>>>>> > >>>>>>>>> > Hi all >>>>>>>>> > >>>>>>>>> > ^^^ bump ^^^ I've raised the PR and am waiting for the review. >>>>>>>>> Once I see >>>>>>>>> > that the suggested changes are directionally right I'll start a >>>>>>>>> VOTE thread >>>>>>>>> > on the CEP (unless I am recommended to follow another process). >>>>>>>>> > >>>>>>>>> > Thanks >>>>>>>>> > Maulin >>>>>>>>> > >>>>>>>>> >> On Thu, May 27, 2021 at 1:29 PM Maulin Vasavada < >>>>>>>>> maulin.vasav...@gmail.com> >>>>>>>>> >> wrote: >>>>>>>>> >> >>>>>>>>> >> HI all >>>>>>>>> >> >>>>>>>>> >> I've raised the PR with the changes. Specifically I would >>>>>>>>> appreciate the >>>>>>>>> >> community's input on this section of the CEP >>>>>>>>> >> < >>>>>>>>> https://cwiki.apache.org/confluence/display/CASSANDRA/CEP-9%3A+Make+SSLContext+creation+pluggable#CEP9:MakeSSLContextcreationpluggable-ImportantnoteaboutcommonSSLconfigurations >>>>>>>>> > >>>>>>>>> >> . >>>>>>>>> >> >>>>>>>>> >> Once we get some consensus on the PR (except minor code >>>>>>>>> improvement >>>>>>>>> >> suggestions) I'll start a VOTE thread for the CEP. >>>>>>>>> >> >>>>>>>>> >> I thank all the reviewers of the CEP and the PR in advance and >>>>>>>>> am >>>>>>>>> >> completely excited to contribute to Apache Cassandra. >>>>>>>>> >> >>>>>>>>> >> Thanks >>>>>>>>> >> Maulin >>>>>>>>> >> >>>>>>>>> >> On Thu, May 27, 2021 at 11:04 AM Maulin Vasavada < >>>>>>>>> >> maulin.vasav...@gmail.com> wrote: >>>>>>>>> >> >>>>>>>>> >>> Sounds good Brandon. I'll raise the PR in a couple of hours >>>>>>>>> from now. >>>>>>>>> >>> Thanks. >>>>>>>>> >>> >>>>>>>>> >>> On Thu, May 27, 2021 at 10:14 AM Brandon Williams < >>>>>>>>> dri...@gmail.com> >>>>>>>>> >>> wrote: >>>>>>>>> >>> >>>>>>>>> >>>> You can raise a PR in any state, but review will be a >>>>>>>>> different >>>>>>>>> >>>> matter. I would go ahead and raise it and the testing can be >>>>>>>>> sorted >>>>>>>>> >>>> out from there. >>>>>>>>> >>>> >>>>>>>>> >>>> On Thu, May 27, 2021 at 12:12 PM Maulin Vasavada >>>>>>>>> >>>> <maulin.vasav...@gmail.com> wrote: >>>>>>>>> >>>>> >>>>>>>>> >>>>> Hi all >>>>>>>>> >>>>> >>>>>>>>> >>>>> I think I am close to raising a PR now but my CircleCI job >>>>>>>>> >>>>> < >>>>>>>>> https://app.circleci.com/pipelines/github/maulin-vasavada/cassandra >>>>>>>>> > >>>>>>>>> >>>>> doesn't make progress beyond key tasks success like unit >>>>>>>>> tests, dtests, >>>>>>>>> >>>>> cqlshlibtests. Any recommendation on if we need to see the >>>>>>>>> whole >>>>>>>>> >>>> CircleCI >>>>>>>>> >>>>> job green before raising the PR? >>>>>>>>> >>>>> >>>>>>>>> >>>>> Thanks >>>>>>>>> >>>>> Maulin >>>>>>>>> >>>>> >>>>>>>>> >>>>> On Fri, May 21, 2021 at 8:54 PM Maulin Vasavada < >>>>>>>>> >>>> maulin.vasav...@gmail.com> >>>>>>>>> >>>>> wrote: >>>>>>>>> >>>>> >>>>>>>>> >>>>>> I am almost done with all changes except the code snippet >>>>>>>>> in the >>>>>>>>> >>>>>> EncryptioOptions.java which determines 'enabled' and >>>>>>>>> 'optional' >>>>>>>>> >>>> encryption >>>>>>>>> >>>>>> flags. Will raise the PR soon once I see my CircleCI >>>>>>>>> getting green. >>>>>>>>> >>>>>> >>>>>>>>> >>>>>> On Fri, May 21, 2021 at 9:24 AM Maulin Vasavada < >>>>>>>>> >>>> maulin.vasav...@gmail.com> >>>>>>>>> >>>>>> wrote: >>>>>>>>> >>>>>> >>>>>>>>> >>>>>>> FYI - I am working on PR. I made some changes and trying >>>>>>>>> to run >>>>>>>>> >>>> tests. >>>>>>>>> >>>>>>> >>>>>>>>> >>>>>>> On Tue, May 18, 2021 at 10:14 PM Maulin Vasavada < >>>>>>>>> >>>>>>> maulin.vasav...@gmail.com> wrote: >>>>>>>>> >>>>>>> >>>>>>>>> >>>>>>>> Thanks Nate for reviewing the CEP. Yes for change #3 in >>>>>>>>> the CEP, I >>>>>>>>> >>>> mean >>>>>>>>> >>>>>>>> to have only single Default Impl and that would be a >>>>>>>>> final class, >>>>>>>>> >>>> not >>>>>>>>> >>>>>>>> overridable. It will be basically an internal >>>>>>>>> implementation. I've >>>>>>>>> >>>> updated >>>>>>>>> >>>>>>>> the CEP to reflect this. >>>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>> On Tue, May 18, 2021 at 7:21 PM Nate McCall < >>>>>>>>> zznat...@gmail.com> >>>>>>>>> >>>> wrote: >>>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> Hi Maulin, >>>>>>>>> >>>>>>>>> Thanks for putting this together! >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Took a quick glance, and I can't think of a compelling >>>>>>>>> reason on >>>>>>>>> >>>> why >>>>>>>>> >>>>>>>>> SSLContext should be final and your point about >>>>>>>>> >>>> organization/compliance >>>>>>>>> >>>>>>>>> issues requiring different implementations is a good one. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Per #3 on your proposed changes, I'm keen to only >>>>>>>>> support a single >>>>>>>>> >>>>>>>>> default >>>>>>>>> >>>>>>>>> impl in-tree. I don't think we should be in the business >>>>>>>>> of >>>>>>>>> >>>> picking >>>>>>>>> >>>>>>>>> implementation to support. It looks like this is your >>>>>>>>> intention >>>>>>>>> >>>> as well? >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Thanks again, >>>>>>>>> >>>>>>>>> -Nate >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, May 19, 2021 at 12:05 PM Maulin Vasavada < >>>>>>>>> >>>>>>>>> maulin.vasav...@gmail.com> >>>>>>>>> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> Hi all >>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>>> Starting a discussion thread for the CIP-9 - >>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>> >>>>>>>>> https://cwiki.apache.org/confluence/display/CASSANDRA/CEP-9%3A+Make+SSLContext+creation+pluggable >>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>>> However, while writing the CIP two areas that came up >>>>>>>>> in my mind >>>>>>>>> >>>>>>>>> where I >>>>>>>>> >>>>>>>>>> need to seek guidance apart from the other discussion >>>>>>>>> that we >>>>>>>>> >>>> would >>>>>>>>> >>>>>>>>> have >>>>>>>>> >>>>>>>>>> here, >>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>>> 1. Whether to consider >>>>>>>>> >>>> SSLFactory#tlsInstanceProtocolSubstitution() >>>>>>>>> >>>>>>>>>> < >>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>> >>>>>>>>> https://github.com/apache/cassandra/blob/cassandra-4.0/src/java/org/apache/cassandra/security/SSLFactory.java#L169 >>>>>>>>> >>>>>>>>>>> >>>>>>>>> >>>>>>>>>> for pluggability (noted this on the CIP as well) >>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>>> 2. For Test Plan, apart from Integration Test and local >>>>>>>>> system >>>>>>>>> >>>> test >>>>>>>>> >>>>>>>>> what >>>>>>>>> >>>>>>>>>> would be recommended? >>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>>> Thanks >>>>>>>>> >>>>>>>>>> Maulin >>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>>> >>>> >>>>>>>>> >>>> >>>>>>>>> --------------------------------------------------------------------- >>>>>>>>> >>>> To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org >>>>>>>>> >>>> For additional commands, e-mail: >>>>>>>>> dev-h...@cassandra.apache.org >>>>>>>>> >>>> >>>>>>>>> >>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> --------------------------------------------------------------------- >>>>>>>>> To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org >>>>>>>>> For additional commands, e-mail: dev-h...@cassandra.apache.org >>>>>>>>> >>>>>>>>>
