Thanks for bringing this back to the ML Maulin. Very much appreciated. On Sat, 10 Jul 2021 at 00:04, Maulin Vasavada <maulin.vasav...@gmail.com> wrote:
> 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 > >>>>>>>>>> > >>>>>>>>>> >
