Thanks Stefan for the pointer for the 'examples' directory. Will invest time in coming up with a reference custom implementation.
For your other comments around common encryption options, I agree with you on a challenge in order to prevent secure information getting leaked in logs. Let me create a separate branch and show how interface will change with keeping Common Encryption options + map instead of just the map. Thanks Maulin 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> > > 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 >>>>>>>>>> >>>>>>>>>>
