ACCUMULO-958 - Pluggable encryption in walogs

[Josh Elser <jo...@gmail.com>]

All,

It's been a few days and I haven't seen much chatter at all on 
ACCUMULO-958 [1] since the patch was applied. There are a couple of 
concerns I have that I definitely want to see addressed before a 1.5.0 
release.

- It worries me that the provided patch is fail-open (when we can't load 
the configured encryption strategies/modules, we don't decrypt anything. 
I think for a security-minded database, we should probably be defaulting 
to fail-close; but, that brings up an issue, what happens when we can't 
encrypt a WAL? Do minor compactions fail gracefully? What does Accumulo do?

- John said he had been reviewing the patch before he applied it; it 
bothers me that there was a version of this patch that had been reviewed 
privately for some amount of time when we had already pushed back the 
feature freeze date by a week waiting for features that weren't done.

- The author noted himself with the deprecation of the CryptoModule 
interface that "we anticipate changing [this] in non-backwards 
compatible ways as we explore requirements for encryption in 
Accumulo...". This tells me that implementation of WAL encryption 
overall hasn't been properly thought out.

Given all of this, it gives me great pause to knowingly include this 
patch into a 1.5.0 release. I see no signs that this has been truly 
thought out, there is no default provided encryption strategy for 1.5.0 
with this patch for the WAL and there is still no support at all for 
RFile encryption (no end-to-end Accumulo encryption for a user). All of 
these issues considered make me believe that this is an incomplete 
feature that is not ready for an Apache Accumulo release.

Thoughts?

- Josh

[1] https://issues.apache.org/jira/browse/ACCUMULO-958

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Adam Fuchs <af...@apache.org>]

Josh,

Mike Allen is still working on ACCUMULO-958, and will have an updated patch
in the next couple of weeks. We were hoping to get the more complete
encryption strategy into 1.5, but were not able to complete it by feature
freeze. However, the WAL encryption as is, when configured with the default
settings, should be no harm to 1.5 -- there's no reason to pull it out or
get concerned about it. We just can't advertise it as a feature of 1.5.
This is one of the reasons why some of the methods are marked as deprecated.

The more complete encryption story, which should be in place for the 1.6
release, should be discussed in ACCUMULO-998.

Cheers,
Adam


On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com> wrote:

> All,
>
> It's been a few days and I haven't seen much chatter at all on
> ACCUMULO-958 [1] since the patch was applied. There are a couple of
> concerns I have that I definitely want to see addressed before a 1.5.0
> release.
>
> - It worries me that the provided patch is fail-open (when we can't load
> the configured encryption strategies/modules, we don't decrypt anything. I
> think for a security-minded database, we should probably be defaulting to
> fail-close; but, that brings up an issue, what happens when we can't
> encrypt a WAL? Do minor compactions fail gracefully? What does Accumulo do?
>
> - John said he had been reviewing the patch before he applied it; it
> bothers me that there was a version of this patch that had been reviewed
> privately for some amount of time when we had already pushed back the
> feature freeze date by a week waiting for features that weren't done.
>
> - The author noted himself with the deprecation of the CryptoModule
> interface that "we anticipate changing [this] in non-backwards compatible
> ways as we explore requirements for encryption in Accumulo...". This tells
> me that implementation of WAL encryption overall hasn't been properly
> thought out.
>
> Given all of this, it gives me great pause to knowingly include this patch
> into a 1.5.0 release. I see no signs that this has been truly thought out,
> there is no default provided encryption strategy for 1.5.0 with this patch
> for the WAL and there is still no support at all for RFile encryption (no
> end-to-end Accumulo encryption for a user). All of these issues considered
> make me believe that this is an incomplete feature that is not ready for an
> Apache Accumulo release.
>
> Thoughts?
>
> - Josh
>
> [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<https://issues.apache.org/jira/browse/ACCUMULO-958>
>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Mike Drob <md...@mdrob.com>]

I don't have any concerns with releasing features in 1.5.1, but please be
cognizant that the intent of
ACCUMULO-751<https://issues.apache.org/jira/browse/ACCUMULO-751>was
for wire compatibility across the 1.5.X series.

I'll strongly agree with Keith regarding a stable API - confusing and
constantly changing APIs are the most frustrating aspect of working with
certain projects (mapred/mapreduce, anyone?) and I think that's a path to
avoid.

Mike


On Wed, Jan 30, 2013 at 12:49 PM, <dl...@comcast.net> wrote:

>
>
>
>
>  - Josh has brought up some technical concerns with the patch
>
>
>
>  - Eric said that he would be fine adding it in 1.5.1 after testing
>
>
>
>  - Keith suggests that we don't change the public API for this if it is
> likely to change
>
>
>
> +1 to all of the above. I think it's ok to add portions of a feature as
> they mature, but it doesn't make sense to add something that we know is
> going to drastically change later. Feature branches are great for
> experiments. Regardless of whether somebody wants it, if we deliver it now,
> then they will use and be dependent on a new, non-complete, deprecated
> feature.
>
>
>
> Dave
>
>
>
> ----- Original Message -----
>
>
> From: "Keith Turner" <ke...@deenlo.com>
> To: dev@accumulo.apache.org
> Sent: Wednesday, January 30, 2013 11:10:38 AM
> Subject: Re: ACCUMULO-958 - Pluggable encryption in walogs
>
> On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote:
> > Let me attempt to make another argument for why the 958 patch should be
> > included in 1.5.0. What this patch represents is not an encryption
> solution
> > for WAL, but an experimental extension point that will be used for
> building
> > an encryption solution as a pluggable module. We need to judge its merit
> > based on whether it is a successful experimental extension point or not.
> > There are three main reasons for including the patch in 1.5.0:
> > 1. Test the performance impact of the null cipher solution (default
> > configuration) in all the performance tests we will be running for the
>
> I do experiments all of the time w/o including half done things in a
> release.
>
> Should I include the following in 1.5.0 just so I can experiment with
> it?  I was working on it got sidetracked and never got back to it.  At
> this point I am uncertain of its utility. It needs further
> experimentation.
>
> https://github.com/keith-turner/accumulo/tree/ACCUMULO-551
>
> > 1.5.0 release. If it causes problems there then we can roll it back.
> > 2. Enable the use of this extension after 1.5 is released. External
> > experiments have dependencies on this extension point. Without the
> > extension point we will have to test with unreleased versions of
> Accumulo,
> > which would be less than ideal.
>
> Back to ACCUMULO-551 I did experiements with that with not problem w/o
> including it in a release.  I just created a version of accumulo
> called accumulo-551-snapshot so no one would be confused if they
> encountered it.  What is wrong with the approach?
>
> > 3. It is not harmful and somebody wants it. The reason for wanting this
> > code in is well documented, so you need a very strong reason to throw it
> > out. Otherwise you will encourage forking of the project (which would be
> > bad).
>
> Forking over this seems illogical.
>
> If we leave it in and hide it, then should all of the configuration
> properties be removed?
>
> I would consider the config props to be part of the public API and not
> easily modified in the future.  Since the props may change as the full
> implementation evolves, I think it would make sense to remove them
> from the public API.  If left, we should modify the config to support
> marking the config props as experimental and also modify the code that
> generates config documentation.  I just want to avoid boxing ourselves
> in or having to make things confusing for users.
>
> >
> > Adam
> >
> >
> >
> >
> > On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com>
> wrote:
> >
> >> Some comments about the comments in ACCUMULO-958:
> >>
> >> Josh writes:
> >>
> >> "We still have the ability to review this even after the feature freeze
> >> happens, it's just frustrating from my point of view in generating the
> best
> >> 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty
> darn
> >> quick)."
> >>
> >> John writes:
> >>
> >> "Yes, but we get stuck on x.y.* for a year or so, so it does become a
> race
> >> to get all the features you want to see in the next year."
> >>
> >> As Accumulo matures, we will need to start thinking a little more
> flexibly
> >> about what goes into minor releases.  We have implemented new (small)
> >> features in minor releases before.
> >>
> >> I would have no problem including ACCUMULO-958 into 1.5.1 after a test
> >> phase, and after some basic experience with the feature.  However I'm
> very
> >> uncomfortable including this in 1.5.0 because there is not a single
> test,
> >> and no real implementation behind the factory that anyone would use In
> Real
> >> Life.  Is this an appropriate API?  I have no idea.  Comments in the
> code
> >> about the stability of the interface basically admit that the author
> isn't
> >> completely comfortable with it, either.
> >>
> >> Let's not rush it, and when it is done right, I'm all for putting it
> into
> >> the next release.  For now, I would hold back incorporating these
> changes
> >> until they are more fully implemented. After we branch 1.5, commit this
> to
> >> trunk, and back-port it to the 1.5 branch when experience and tests
> show it
> >> is ready to be released.
> >>
> >> -Eric
> >>
> >>
> >>
> >> On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com>
> wrote:
> >>
> >> > All,
> >> >
> >> > It's been a few days and I haven't seen much chatter at all on
> >> > ACCUMULO-958 [1] since the patch was applied. There are a couple of
> >> > concerns I have that I definitely want to see addressed before a 1.5.0
> >> > release.
> >> >
> >> > - It worries me that the provided patch is fail-open (when we can't
> load
> >> > the configured encryption strategies/modules, we don't decrypt
> anything.
> >> I
> >> > think for a security-minded database, we should probably be
> defaulting to
> >> > fail-close; but, that brings up an issue, what happens when we can't
> >> > encrypt a WAL? Do minor compactions fail gracefully? What does
> Accumulo
> >> do?
> >> >
> >> > - John said he had been reviewing the patch before he applied it; it
> >> > bothers me that there was a version of this patch that had been
> reviewed
> >> > privately for some amount of time when we had already pushed back the
> >> > feature freeze date by a week waiting for features that weren't done.
> >> >
> >> > - The author noted himself with the deprecation of the CryptoModule
> >> > interface that "we anticipate changing [this] in non-backwards
> compatible
> >> > ways as we explore requirements for encryption in Accumulo...". This
> >> tells
> >> > me that implementation of WAL encryption overall hasn't been properly
> >> > thought out.
> >> >
> >> > Given all of this, it gives me great pause to knowingly include this
> >> patch
> >> > into a 1.5.0 release. I see no signs that this has been truly thought
> >> out,
> >> > there is no default provided encryption strategy for 1.5.0 with this
> >> patch
> >> > for the WAL and there is still no support at all for RFile encryption
> (no
> >> > end-to-end Accumulo encryption for a user). All of these issues
> >> considered
> >> > make me believe that this is an incomplete feature that is not ready
> for
> >> an
> >> > Apache Accumulo release.
> >> >
> >> > Thoughts?
> >> >
> >> > - Josh
> >> >
> >> > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
> >> https://issues.apache.org/jira/browse/ACCUMULO-958>
> >> >
> >>
>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Aaron Cordova <aa...@cordovas.org>]

Perhaps another option - it's not uncommon for a company, like Cloudera or RedHat, to add patches to their distribution for things they want to roll out earlier than the broader community wants them, among other reasons. Companies also often backport newer features to older releases.

Although one would certainly want to limit the extend of the patches. 


On Jan 30, 2013, at 12:49 PM, dlmarion@comcast.net wrote:

> 
> 
> 
> 
>  - Josh has brought up some technical concerns with the patch 
> 
> 
> 
>  - Eric said that he would be fine adding it in 1.5.1 after testing 
> 
> 
> 
>  - Keith suggests that we don't change the public API for this if it is likely to change 
> 
> 
> 
> +1 to all of the above. I think it's ok to add portions of a feature as they mature, but it doesn't make sense to add something that we know is going to drastically change later. Feature branches are great for experiments. Regardless of whether somebody wants it, if we deliver it now, then they will use and be dependent on a new, non-complete, deprecated feature. 
> 
> 
> 
> Dave 
> 
> 
> 
> ----- Original Message -----
> 
> 
> From: "Keith Turner" <ke...@deenlo.com> 
> To: dev@accumulo.apache.org 
> Sent: Wednesday, January 30, 2013 11:10:38 AM 
> Subject: Re: ACCUMULO-958 - Pluggable encryption in walogs 
> 
> On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote: 
>> Let me attempt to make another argument for why the 958 patch should be 
>> included in 1.5.0. What this patch represents is not an encryption solution 
>> for WAL, but an experimental extension point that will be used for building 
>> an encryption solution as a pluggable module. We need to judge its merit 
>> based on whether it is a successful experimental extension point or not. 
>> There are three main reasons for including the patch in 1.5.0: 
>> 1. Test the performance impact of the null cipher solution (default 
>> configuration) in all the performance tests we will be running for the 
> 
> I do experiments all of the time w/o including half done things in a release. 
> 
> Should I include the following in 1.5.0 just so I can experiment with 
> it?  I was working on it got sidetracked and never got back to it.  At 
> this point I am uncertain of its utility. It needs further 
> experimentation. 
> 
> https://github.com/keith-turner/accumulo/tree/ACCUMULO-551 
> 
>> 1.5.0 release. If it causes problems there then we can roll it back. 
>> 2. Enable the use of this extension after 1.5 is released. External 
>> experiments have dependencies on this extension point. Without the 
>> extension point we will have to test with unreleased versions of Accumulo, 
>> which would be less than ideal. 
> 
> Back to ACCUMULO-551 I did experiements with that with not problem w/o 
> including it in a release.  I just created a version of accumulo 
> called accumulo-551-snapshot so no one would be confused if they 
> encountered it.  What is wrong with the approach? 
> 
>> 3. It is not harmful and somebody wants it. The reason for wanting this 
>> code in is well documented, so you need a very strong reason to throw it 
>> out. Otherwise you will encourage forking of the project (which would be 
>> bad). 
> 
> Forking over this seems illogical. 
> 
> If we leave it in and hide it, then should all of the configuration 
> properties be removed? 
> 
> I would consider the config props to be part of the public API and not 
> easily modified in the future.  Since the props may change as the full 
> implementation evolves, I think it would make sense to remove them 
> from the public API.  If left, we should modify the config to support 
> marking the config props as experimental and also modify the code that 
> generates config documentation.  I just want to avoid boxing ourselves 
> in or having to make things confusing for users. 
> 
>> 
>> Adam 
>> 
>> 
>> 
>> 
>> On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com> wrote: 
>> 
>>> Some comments about the comments in ACCUMULO-958: 
>>> 
>>> Josh writes: 
>>> 
>>> "We still have the ability to review this even after the feature freeze 
>>> happens, it's just frustrating from my point of view in generating the best 
>>> 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty darn 
>>> quick)." 
>>> 
>>> John writes: 
>>> 
>>> "Yes, but we get stuck on x.y.* for a year or so, so it does become a race 
>>> to get all the features you want to see in the next year." 
>>> 
>>> As Accumulo matures, we will need to start thinking a little more flexibly 
>>> about what goes into minor releases.  We have implemented new (small) 
>>> features in minor releases before. 
>>> 
>>> I would have no problem including ACCUMULO-958 into 1.5.1 after a test 
>>> phase, and after some basic experience with the feature.  However I'm very 
>>> uncomfortable including this in 1.5.0 because there is not a single test, 
>>> and no real implementation behind the factory that anyone would use In Real 
>>> Life.  Is this an appropriate API?  I have no idea.  Comments in the code 
>>> about the stability of the interface basically admit that the author isn't 
>>> completely comfortable with it, either. 
>>> 
>>> Let's not rush it, and when it is done right, I'm all for putting it into 
>>> the next release.  For now, I would hold back incorporating these changes 
>>> until they are more fully implemented. After we branch 1.5, commit this to 
>>> trunk, and back-port it to the 1.5 branch when experience and tests show it 
>>> is ready to be released. 
>>> 
>>> -Eric 
>>> 
>>> 
>>> 
>>> On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com> wrote: 
>>> 
>>>> All, 
>>>> 
>>>> It's been a few days and I haven't seen much chatter at all on 
>>>> ACCUMULO-958 [1] since the patch was applied. There are a couple of 
>>>> concerns I have that I definitely want to see addressed before a 1.5.0 
>>>> release. 
>>>> 
>>>> - It worries me that the provided patch is fail-open (when we can't load 
>>>> the configured encryption strategies/modules, we don't decrypt anything. 
>>> I 
>>>> think for a security-minded database, we should probably be defaulting to 
>>>> fail-close; but, that brings up an issue, what happens when we can't 
>>>> encrypt a WAL? Do minor compactions fail gracefully? What does Accumulo 
>>> do? 
>>>> 
>>>> - John said he had been reviewing the patch before he applied it; it 
>>>> bothers me that there was a version of this patch that had been reviewed 
>>>> privately for some amount of time when we had already pushed back the 
>>>> feature freeze date by a week waiting for features that weren't done. 
>>>> 
>>>> - The author noted himself with the deprecation of the CryptoModule 
>>>> interface that "we anticipate changing [this] in non-backwards compatible 
>>>> ways as we explore requirements for encryption in Accumulo...". This 
>>> tells 
>>>> me that implementation of WAL encryption overall hasn't been properly 
>>>> thought out. 
>>>> 
>>>> Given all of this, it gives me great pause to knowingly include this 
>>> patch 
>>>> into a 1.5.0 release. I see no signs that this has been truly thought 
>>> out, 
>>>> there is no default provided encryption strategy for 1.5.0 with this 
>>> patch 
>>>> for the WAL and there is still no support at all for RFile encryption (no 
>>>> end-to-end Accumulo encryption for a user). All of these issues 
>>> considered 
>>>> make me believe that this is an incomplete feature that is not ready for 
>>> an 
>>>> Apache Accumulo release. 
>>>> 
>>>> Thoughts? 
>>>> 
>>>> - Josh 
>>>> 
>>>> [1] https://issues.apache.org/**jira/browse/ACCUMULO-958< 
>>> https://issues.apache.org/jira/browse/ACCUMULO-958> 
>>>> 
>>> 

Re: ACCUMULO-958 - Pluggable encryption in walogs

[dl...@comcast.net]

 - Josh has brought up some technical concerns with the patch 



 - Eric said that he would be fine adding it in 1.5.1 after testing 



 - Keith suggests that we don't change the public API for this if it is likely to change 



+1 to all of the above. I think it's ok to add portions of a feature as they mature, but it doesn't make sense to add something that we know is going to drastically change later. Feature branches are great for experiments. Regardless of whether somebody wants it, if we deliver it now, then they will use and be dependent on a new, non-complete, deprecated feature. 



Dave 



----- Original Message -----


From: "Keith Turner" <ke...@deenlo.com> 
To: dev@accumulo.apache.org 
Sent: Wednesday, January 30, 2013 11:10:38 AM 
Subject: Re: ACCUMULO-958 - Pluggable encryption in walogs 

On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote: 
> Let me attempt to make another argument for why the 958 patch should be 
> included in 1.5.0. What this patch represents is not an encryption solution 
> for WAL, but an experimental extension point that will be used for building 
> an encryption solution as a pluggable module. We need to judge its merit 
> based on whether it is a successful experimental extension point or not. 
> There are three main reasons for including the patch in 1.5.0: 
> 1. Test the performance impact of the null cipher solution (default 
> configuration) in all the performance tests we will be running for the 

I do experiments all of the time w/o including half done things in a release. 

Should I include the following in 1.5.0 just so I can experiment with 
it?  I was working on it got sidetracked and never got back to it.  At 
this point I am uncertain of its utility. It needs further 
experimentation. 

https://github.com/keith-turner/accumulo/tree/ACCUMULO-551 

> 1.5.0 release. If it causes problems there then we can roll it back. 
> 2. Enable the use of this extension after 1.5 is released. External 
> experiments have dependencies on this extension point. Without the 
> extension point we will have to test with unreleased versions of Accumulo, 
> which would be less than ideal. 

Back to ACCUMULO-551 I did experiements with that with not problem w/o 
including it in a release.  I just created a version of accumulo 
called accumulo-551-snapshot so no one would be confused if they 
encountered it.  What is wrong with the approach? 

> 3. It is not harmful and somebody wants it. The reason for wanting this 
> code in is well documented, so you need a very strong reason to throw it 
> out. Otherwise you will encourage forking of the project (which would be 
> bad). 

Forking over this seems illogical. 

If we leave it in and hide it, then should all of the configuration 
properties be removed? 

I would consider the config props to be part of the public API and not 
easily modified in the future.  Since the props may change as the full 
implementation evolves, I think it would make sense to remove them 
from the public API.  If left, we should modify the config to support 
marking the config props as experimental and also modify the code that 
generates config documentation.  I just want to avoid boxing ourselves 
in or having to make things confusing for users. 

> 
> Adam 
> 
> 
> 
> 
> On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com> wrote: 
> 
>> Some comments about the comments in ACCUMULO-958: 
>> 
>> Josh writes: 
>> 
>> "We still have the ability to review this even after the feature freeze 
>> happens, it's just frustrating from my point of view in generating the best 
>> 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty darn 
>> quick)." 
>> 
>> John writes: 
>> 
>> "Yes, but we get stuck on x.y.* for a year or so, so it does become a race 
>> to get all the features you want to see in the next year." 
>> 
>> As Accumulo matures, we will need to start thinking a little more flexibly 
>> about what goes into minor releases.  We have implemented new (small) 
>> features in minor releases before. 
>> 
>> I would have no problem including ACCUMULO-958 into 1.5.1 after a test 
>> phase, and after some basic experience with the feature.  However I'm very 
>> uncomfortable including this in 1.5.0 because there is not a single test, 
>> and no real implementation behind the factory that anyone would use In Real 
>> Life.  Is this an appropriate API?  I have no idea.  Comments in the code 
>> about the stability of the interface basically admit that the author isn't 
>> completely comfortable with it, either. 
>> 
>> Let's not rush it, and when it is done right, I'm all for putting it into 
>> the next release.  For now, I would hold back incorporating these changes 
>> until they are more fully implemented. After we branch 1.5, commit this to 
>> trunk, and back-port it to the 1.5 branch when experience and tests show it 
>> is ready to be released. 
>> 
>> -Eric 
>> 
>> 
>> 
>> On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com> wrote: 
>> 
>> > All, 
>> > 
>> > It's been a few days and I haven't seen much chatter at all on 
>> > ACCUMULO-958 [1] since the patch was applied. There are a couple of 
>> > concerns I have that I definitely want to see addressed before a 1.5.0 
>> > release. 
>> > 
>> > - It worries me that the provided patch is fail-open (when we can't load 
>> > the configured encryption strategies/modules, we don't decrypt anything. 
>> I 
>> > think for a security-minded database, we should probably be defaulting to 
>> > fail-close; but, that brings up an issue, what happens when we can't 
>> > encrypt a WAL? Do minor compactions fail gracefully? What does Accumulo 
>> do? 
>> > 
>> > - John said he had been reviewing the patch before he applied it; it 
>> > bothers me that there was a version of this patch that had been reviewed 
>> > privately for some amount of time when we had already pushed back the 
>> > feature freeze date by a week waiting for features that weren't done. 
>> > 
>> > - The author noted himself with the deprecation of the CryptoModule 
>> > interface that "we anticipate changing [this] in non-backwards compatible 
>> > ways as we explore requirements for encryption in Accumulo...". This 
>> tells 
>> > me that implementation of WAL encryption overall hasn't been properly 
>> > thought out. 
>> > 
>> > Given all of this, it gives me great pause to knowingly include this 
>> patch 
>> > into a 1.5.0 release. I see no signs that this has been truly thought 
>> out, 
>> > there is no default provided encryption strategy for 1.5.0 with this 
>> patch 
>> > for the WAL and there is still no support at all for RFile encryption (no 
>> > end-to-end Accumulo encryption for a user). All of these issues 
>> considered 
>> > make me believe that this is an incomplete feature that is not ready for 
>> an 
>> > Apache Accumulo release. 
>> > 
>> > Thoughts? 
>> > 
>> > - Josh 
>> > 
>> > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958< 
>> https://issues.apache.org/jira/browse/ACCUMULO-958> 
>> > 
>> 

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Keith Turner <ke...@deenlo.com>]

On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote:
> Let me attempt to make another argument for why the 958 patch should be
> included in 1.5.0. What this patch represents is not an encryption solution
> for WAL, but an experimental extension point that will be used for building
> an encryption solution as a pluggable module. We need to judge its merit
> based on whether it is a successful experimental extension point or not.
> There are three main reasons for including the patch in 1.5.0:
> 1. Test the performance impact of the null cipher solution (default
> configuration) in all the performance tests we will be running for the

I do experiments all of the time w/o including half done things in a release.

Should I include the following in 1.5.0 just so I can experiment with
it?  I was working on it got sidetracked and never got back to it.  At
this point I am uncertain of its utility. It needs further
experimentation.

https://github.com/keith-turner/accumulo/tree/ACCUMULO-551

> 1.5.0 release. If it causes problems there then we can roll it back.
> 2. Enable the use of this extension after 1.5 is released. External
> experiments have dependencies on this extension point. Without the
> extension point we will have to test with unreleased versions of Accumulo,
> which would be less than ideal.

Back to ACCUMULO-551 I did experiements with that with not problem w/o
including it in a release.  I just created a version of accumulo
called accumulo-551-snapshot so no one would be confused if they
encountered it.  What is wrong with the approach?

> 3. It is not harmful and somebody wants it. The reason for wanting this
> code in is well documented, so you need a very strong reason to throw it
> out. Otherwise you will encourage forking of the project (which would be
> bad).

Forking over this seems illogical.

If we leave it in and hide it, then should all of the configuration
properties be removed?

I would consider the config props to be part of the public API and not
easily modified in the future.  Since the props may change as the full
implementation evolves, I think it would make sense to remove them
from the public API.  If left, we should modify the config to support
marking the config props as experimental and also modify the code that
generates config documentation.  I just want to avoid boxing ourselves
in or having to make things confusing for users.

>
> Adam
>
>
>
>
> On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com> wrote:
>
>> Some comments about the comments in ACCUMULO-958:
>>
>> Josh writes:
>>
>> "We still have the ability to review this even after the feature freeze
>> happens, it's just frustrating from my point of view in generating the best
>> 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty darn
>> quick)."
>>
>> John writes:
>>
>> "Yes, but we get stuck on x.y.* for a year or so, so it does become a race
>> to get all the features you want to see in the next year."
>>
>> As Accumulo matures, we will need to start thinking a little more flexibly
>> about what goes into minor releases.  We have implemented new (small)
>> features in minor releases before.
>>
>> I would have no problem including ACCUMULO-958 into 1.5.1 after a test
>> phase, and after some basic experience with the feature.  However I'm very
>> uncomfortable including this in 1.5.0 because there is not a single test,
>> and no real implementation behind the factory that anyone would use In Real
>> Life.  Is this an appropriate API?  I have no idea.  Comments in the code
>> about the stability of the interface basically admit that the author isn't
>> completely comfortable with it, either.
>>
>> Let's not rush it, and when it is done right, I'm all for putting it into
>> the next release.  For now, I would hold back incorporating these changes
>> until they are more fully implemented. After we branch 1.5, commit this to
>> trunk, and back-port it to the 1.5 branch when experience and tests show it
>> is ready to be released.
>>
>> -Eric
>>
>>
>>
>> On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com> wrote:
>>
>> > All,
>> >
>> > It's been a few days and I haven't seen much chatter at all on
>> > ACCUMULO-958 [1] since the patch was applied. There are a couple of
>> > concerns I have that I definitely want to see addressed before a 1.5.0
>> > release.
>> >
>> > - It worries me that the provided patch is fail-open (when we can't load
>> > the configured encryption strategies/modules, we don't decrypt anything.
>> I
>> > think for a security-minded database, we should probably be defaulting to
>> > fail-close; but, that brings up an issue, what happens when we can't
>> > encrypt a WAL? Do minor compactions fail gracefully? What does Accumulo
>> do?
>> >
>> > - John said he had been reviewing the patch before he applied it; it
>> > bothers me that there was a version of this patch that had been reviewed
>> > privately for some amount of time when we had already pushed back the
>> > feature freeze date by a week waiting for features that weren't done.
>> >
>> > - The author noted himself with the deprecation of the CryptoModule
>> > interface that "we anticipate changing [this] in non-backwards compatible
>> > ways as we explore requirements for encryption in Accumulo...". This
>> tells
>> > me that implementation of WAL encryption overall hasn't been properly
>> > thought out.
>> >
>> > Given all of this, it gives me great pause to knowingly include this
>> patch
>> > into a 1.5.0 release. I see no signs that this has been truly thought
>> out,
>> > there is no default provided encryption strategy for 1.5.0 with this
>> patch
>> > for the WAL and there is still no support at all for RFile encryption (no
>> > end-to-end Accumulo encryption for a user). All of these issues
>> considered
>> > make me believe that this is an incomplete feature that is not ready for
>> an
>> > Apache Accumulo release.
>> >
>> > Thoughts?
>> >
>> > - Josh
>> >
>> > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
>> https://issues.apache.org/jira/browse/ACCUMULO-958>
>> >
>>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Josh Elser <jo...@gmail.com>]

Given the discussion that's taken place today on this thread, I'd like 
to bring back around the discussion and refocus my concerns, given 
everyone else's comments, and perhaps generate some sort of summary, 
boiling down to "what next".

First, my own opinions, despite how my actions may/may not have been 
interpreted by others on this list, my reason for reviewing 958, and 
furthermore starting this thread, is purely from a QA standpoint. I 
would have not hesitated to start a similar discussion if the 
contributor was a more-seasoned Accumulo dev than I (of which I consider 
most to be), a coworker, former coworker, friend, stranger. I want to 
strongly bring focus back to the point: I reviewed a patch from a new 
contributor, I saw issues in the patch, gave (in my opinion) positive 
feedback to the contributor and received no further comments after what 
I consider a fair amount of time (5 days). My overarching reason for 
escalating this issue is that I am concerned that the longer we wait to 
*actually* finalize the first 1.5.0 release candidate, the less we will 
actually test.

In less words, I saw *code* (not an individual) which I had issues with, 
received no response to code review after a very quick patch 
application, and wanted to test the waters to see if others also had 
complaints.

(putting on my PMC hat to try to be unbiased, but *please* read for the 
thread for yourself)

Trying to summarize what has been discussed, there is concern from 
developers with the changes in regards to public API changes, lack of 
unit/functional tests, the lack of an actual encryption policy (one that 
encrypts) in the provided changes and the lack of a complete encryption 
solution. On the other side, issues were raised about stifling 
contributions due to a high barrier to entry to Accumulo which could 
cause project division and the provided code is disabled by default 
which shouldn't cause any harm to users/testers.

(putting my hat back on)

To voice my final opinion, I have *no* problem with including code for 
encrypting WALs if someone will publicly sign up to bring the quality of 
that code up to what I consider Accumulo-par. Meaning, someone signs up 
to actively participate in review with the devs and adhere to future 
deadlines of completion (so as to not delay a 1.5.0 release). I also 
expect the creation of some basic functional tests to give "us" some 
warm-fuzzies that the code doesn't create any performance issues. I 
would *like* to see an implementation of encryption using these changes 
(instead of just the promise), but I can understand if this is not 
feasible given the time constraints of a 1.5.0 release (but please 
explicitly tell the rest of us this if it is infeasible). I'm personally 
not super-concerned about new config values being added across a major 
release (public API).

Outside of the scope of just this issue, I'm going to 1) start thinking 
about ways that we can quantify/automate the issue of QA contributions 
to alleviate any future issues stemming from similar situations. 2) the 
next time we have a feature-freeze, we need to be as explicit as 
possible to avoid conflict (how can we make this process better).

- Josh

On 01/30/2013 03:13 PM, Benson Margulies wrote:
> It might be preferable if the patch included an example plugin.
> Otherwise, it's a bit hard to see how an open process can evaluate the
> design decisions, perhaps test a bit of performance, etc. I write this
> without having read a line of code. If this is, in fact, very simple
> and straightforward, then it might be a tempest in a teapot.
>
> On Wed, Jan 30, 2013 at 2:26 PM, Keith Turner <ke...@deenlo.com> wrote:
>> On Wed, Jan 30, 2013 at 2:02 PM, William Slacum
>> <wi...@accumulo.net> wrote:
>>> Adam, I think you need to invert your premise. You need to consider the
>>> benefit to the community of some piece of work before committing back to
>>> the community. A plug-in framework has no value if there are no plug-ins.
>>> Adding in untested, unreviewed layers of indirection for reading and
>> We are CTR.  Any committer should be willing to open discussion and
>> review of any change they make.  I feel that very reasonable questions
>> about this change are not being answered or discussed.
>>
>>> writing data is a bad idea, plain and simple. Furthermore, you cannot say
>>> you are avoiding forking by supplying the patch and then openly state you
>>> are witholding portions that make said patch useful.
>>>
>>> On Wed, Jan 30, 2013 at 1:45 PM, Adam Fuchs <sc...@gmail.com> wrote:
>>>
>>>> Bill,
>>>>
>>>> The release date was not pushed back just for this ticket -- there were
>>>> several other changes that motivated that date change. We can discuss that
>>>> aspect separately from a discussion of ACCUMULO-958, and we need to start a
>>>> separate thread to talk about the remaining milestones before the 1.5.0
>>>> release.
>>>>
>>>> I would also like to amend your statement to be "... the patch has no value
>>>> added [for] general users [without the addition of extensions that are not
>>>> included with the patch]." This is a more accurate yet much weaker premise,
>>>> and you should consider the implications on the broader ecosystem.
>>>>
>>>> It seems to me that the main points against this patch are that it is
>>>> imperfect. I don't think that feature freeze is the time at which we should
>>>> demand perfection. Several valid issues have been raised, which should be
>>>> fixed by code freeze (the date of which is not yet set). However, the
>>>> utility of this work is obvious to me. At the end of the day, what bar are
>>>> we trying to set for inclusion of a patch?
>>>>
>>>> Adam
>>>>
>>>>
>>>>
>>>> On Wed, Jan 30, 2013 at 11:05 AM, William Slacum <
>>>> wilhelm.von.cloud@accumulo.net> wrote:
>>>>
>>>>> Bottom line, the patch has no value added to general users. The idea
>>>> behind
>>>>> pushing back a release date to stuff in unoperational code is very bad
>>>>> practice. It sets a precedent for not considering alternative approaches
>>>>> while simultaneously having no justification for choosing the approach we
>>>>> did. If a specific customer/group/person wants a feature, and that
>>>> feature
>>>>> does not exist yet, the code is freely available to be modified,
>>>>> distributed and open to public review. Adam, I strongly disagree that
>>>>> forking the code is bad, considering the progress that other projects
>>>> make
>>>>> specifically because they have experimental forks (HBase).
>>>>>
>>>>> On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote:
>>>>>
>>>>>> Let me attempt to make another argument for why the 958 patch should be
>>>>>> included in 1.5.0. What this patch represents is not an encryption
>>>>> solution
>>>>>> for WAL, but an experimental extension point that will be used for
>>>>> building
>>>>>> an encryption solution as a pluggable module. We need to judge its
>>>> merit
>>>>>> based on whether it is a successful experimental extension point or
>>>> not.
>>>>>> There are three main reasons for including the patch in 1.5.0:
>>>>>> 1. Test the performance impact of the null cipher solution (default
>>>>>> configuration) in all the performance tests we will be running for the
>>>>>> 1.5.0 release. If it causes problems there then we can roll it back.
>>>>>> 2. Enable the use of this extension after 1.5 is released. External
>>>>>> experiments have dependencies on this extension point. Without the
>>>>>> extension point we will have to test with unreleased versions of
>>>>> Accumulo,
>>>>>> which would be less than ideal.
>>>>>> 3. It is not harmful and somebody wants it. The reason for wanting this
>>>>>> code in is well documented, so you need a very strong reason to throw
>>>> it
>>>>>> out. Otherwise you will encourage forking of the project (which would
>>>> be
>>>>>> bad).
>>>>>>
>>>>>> Adam
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Some comments about the comments in ACCUMULO-958:
>>>>>>>
>>>>>>> Josh writes:
>>>>>>>
>>>>>>> "We still have the ability to review this even after the feature
>>>> freeze
>>>>>>> happens, it's just frustrating from my point of view in generating
>>>> the
>>>>>> best
>>>>>>> 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty
>>>>>> darn
>>>>>>> quick)."
>>>>>>>
>>>>>>> John writes:
>>>>>>>
>>>>>>> "Yes, but we get stuck on x.y.* for a year or so, so it does become a
>>>>>> race
>>>>>>> to get all the features you want to see in the next year."
>>>>>>>
>>>>>>> As Accumulo matures, we will need to start thinking a little more
>>>>>> flexibly
>>>>>>> about what goes into minor releases.  We have implemented new (small)
>>>>>>> features in minor releases before.
>>>>>>>
>>>>>>> I would have no problem including ACCUMULO-958 into 1.5.1 after a
>>>> test
>>>>>>> phase, and after some basic experience with the feature.  However I'm
>>>>>> very
>>>>>>> uncomfortable including this in 1.5.0 because there is not a single
>>>>> test,
>>>>>>> and no real implementation behind the factory that anyone would use
>>>> In
>>>>>> Real
>>>>>>> Life.  Is this an appropriate API?  I have no idea.  Comments in the
>>>>> code
>>>>>>> about the stability of the interface basically admit that the author
>>>>>> isn't
>>>>>>> completely comfortable with it, either.
>>>>>>>
>>>>>>> Let's not rush it, and when it is done right, I'm all for putting it
>>>>> into
>>>>>>> the next release.  For now, I would hold back incorporating these
>>>>> changes
>>>>>>> until they are more fully implemented. After we branch 1.5, commit
>>>> this
>>>>>> to
>>>>>>> trunk, and back-port it to the 1.5 branch when experience and tests
>>>>> show
>>>>>> it
>>>>>>> is ready to be released.
>>>>>>>
>>>>>>> -Eric
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com>
>>>>>> wrote:
>>>>>>>> All,
>>>>>>>>
>>>>>>>> It's been a few days and I haven't seen much chatter at all on
>>>>>>>> ACCUMULO-958 [1] since the patch was applied. There are a couple of
>>>>>>>> concerns I have that I definitely want to see addressed before a
>>>>> 1.5.0
>>>>>>>> release.
>>>>>>>>
>>>>>>>> - It worries me that the provided patch is fail-open (when we can't
>>>>>> load
>>>>>>>> the configured encryption strategies/modules, we don't decrypt
>>>>>> anything.
>>>>>>> I
>>>>>>>> think for a security-minded database, we should probably be
>>>>> defaulting
>>>>>> to
>>>>>>>> fail-close; but, that brings up an issue, what happens when we
>>>> can't
>>>>>>>> encrypt a WAL? Do minor compactions fail gracefully? What does
>>>>> Accumulo
>>>>>>> do?
>>>>>>>> - John said he had been reviewing the patch before he applied it;
>>>> it
>>>>>>>> bothers me that there was a version of this patch that had been
>>>>>> reviewed
>>>>>>>> privately for some amount of time when we had already pushed back
>>>> the
>>>>>>>> feature freeze date by a week waiting for features that weren't
>>>> done.
>>>>>>>> - The author noted himself with the deprecation of the CryptoModule
>>>>>>>> interface that "we anticipate changing [this] in non-backwards
>>>>>> compatible
>>>>>>>> ways as we explore requirements for encryption in Accumulo...".
>>>> This
>>>>>>> tells
>>>>>>>> me that implementation of WAL encryption overall hasn't been
>>>> properly
>>>>>>>> thought out.
>>>>>>>>
>>>>>>>> Given all of this, it gives me great pause to knowingly include
>>>> this
>>>>>>> patch
>>>>>>>> into a 1.5.0 release. I see no signs that this has been truly
>>>> thought
>>>>>>> out,
>>>>>>>> there is no default provided encryption strategy for 1.5.0 with
>>>> this
>>>>>>> patch
>>>>>>>> for the WAL and there is still no support at all for RFile
>>>> encryption
>>>>>> (no
>>>>>>>> end-to-end Accumulo encryption for a user). All of these issues
>>>>>>> considered
>>>>>>>> make me believe that this is an incomplete feature that is not
>>>> ready
>>>>>> for
>>>>>>> an
>>>>>>>> Apache Accumulo release.
>>>>>>>>
>>>>>>>> Thoughts?
>>>>>>>>
>>>>>>>> - Josh
>>>>>>>>
>>>>>>>> [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
>>>>>>> https://issues.apache.org/jira/browse/ACCUMULO-958>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Benson Margulies <bi...@gmail.com>]

It might be preferable if the patch included an example plugin.
Otherwise, it's a bit hard to see how an open process can evaluate the
design decisions, perhaps test a bit of performance, etc. I write this
without having read a line of code. If this is, in fact, very simple
and straightforward, then it might be a tempest in a teapot.

On Wed, Jan 30, 2013 at 2:26 PM, Keith Turner <ke...@deenlo.com> wrote:
> On Wed, Jan 30, 2013 at 2:02 PM, William Slacum
> <wi...@accumulo.net> wrote:
>> Adam, I think you need to invert your premise. You need to consider the
>> benefit to the community of some piece of work before committing back to
>> the community. A plug-in framework has no value if there are no plug-ins.
>> Adding in untested, unreviewed layers of indirection for reading and
>
> We are CTR.  Any committer should be willing to open discussion and
> review of any change they make.  I feel that very reasonable questions
> about this change are not being answered or discussed.
>
>> writing data is a bad idea, plain and simple. Furthermore, you cannot say
>> you are avoiding forking by supplying the patch and then openly state you
>> are witholding portions that make said patch useful.
>>
>> On Wed, Jan 30, 2013 at 1:45 PM, Adam Fuchs <sc...@gmail.com> wrote:
>>
>>> Bill,
>>>
>>> The release date was not pushed back just for this ticket -- there were
>>> several other changes that motivated that date change. We can discuss that
>>> aspect separately from a discussion of ACCUMULO-958, and we need to start a
>>> separate thread to talk about the remaining milestones before the 1.5.0
>>> release.
>>>
>>> I would also like to amend your statement to be "... the patch has no value
>>> added [for] general users [without the addition of extensions that are not
>>> included with the patch]." This is a more accurate yet much weaker premise,
>>> and you should consider the implications on the broader ecosystem.
>>>
>>> It seems to me that the main points against this patch are that it is
>>> imperfect. I don't think that feature freeze is the time at which we should
>>> demand perfection. Several valid issues have been raised, which should be
>>> fixed by code freeze (the date of which is not yet set). However, the
>>> utility of this work is obvious to me. At the end of the day, what bar are
>>> we trying to set for inclusion of a patch?
>>>
>>> Adam
>>>
>>>
>>>
>>> On Wed, Jan 30, 2013 at 11:05 AM, William Slacum <
>>> wilhelm.von.cloud@accumulo.net> wrote:
>>>
>>> > Bottom line, the patch has no value added to general users. The idea
>>> behind
>>> > pushing back a release date to stuff in unoperational code is very bad
>>> > practice. It sets a precedent for not considering alternative approaches
>>> > while simultaneously having no justification for choosing the approach we
>>> > did. If a specific customer/group/person wants a feature, and that
>>> feature
>>> > does not exist yet, the code is freely available to be modified,
>>> > distributed and open to public review. Adam, I strongly disagree that
>>> > forking the code is bad, considering the progress that other projects
>>> make
>>> > specifically because they have experimental forks (HBase).
>>> >
>>> > On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote:
>>> >
>>> > > Let me attempt to make another argument for why the 958 patch should be
>>> > > included in 1.5.0. What this patch represents is not an encryption
>>> > solution
>>> > > for WAL, but an experimental extension point that will be used for
>>> > building
>>> > > an encryption solution as a pluggable module. We need to judge its
>>> merit
>>> > > based on whether it is a successful experimental extension point or
>>> not.
>>> > > There are three main reasons for including the patch in 1.5.0:
>>> > > 1. Test the performance impact of the null cipher solution (default
>>> > > configuration) in all the performance tests we will be running for the
>>> > > 1.5.0 release. If it causes problems there then we can roll it back.
>>> > > 2. Enable the use of this extension after 1.5 is released. External
>>> > > experiments have dependencies on this extension point. Without the
>>> > > extension point we will have to test with unreleased versions of
>>> > Accumulo,
>>> > > which would be less than ideal.
>>> > > 3. It is not harmful and somebody wants it. The reason for wanting this
>>> > > code in is well documented, so you need a very strong reason to throw
>>> it
>>> > > out. Otherwise you will encourage forking of the project (which would
>>> be
>>> > > bad).
>>> > >
>>> > > Adam
>>> > >
>>> > >
>>> > >
>>> > >
>>> > > On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com>
>>> > > wrote:
>>> > >
>>> > > > Some comments about the comments in ACCUMULO-958:
>>> > > >
>>> > > > Josh writes:
>>> > > >
>>> > > > "We still have the ability to review this even after the feature
>>> freeze
>>> > > > happens, it's just frustrating from my point of view in generating
>>> the
>>> > > best
>>> > > > 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty
>>> > > darn
>>> > > > quick)."
>>> > > >
>>> > > > John writes:
>>> > > >
>>> > > > "Yes, but we get stuck on x.y.* for a year or so, so it does become a
>>> > > race
>>> > > > to get all the features you want to see in the next year."
>>> > > >
>>> > > > As Accumulo matures, we will need to start thinking a little more
>>> > > flexibly
>>> > > > about what goes into minor releases.  We have implemented new (small)
>>> > > > features in minor releases before.
>>> > > >
>>> > > > I would have no problem including ACCUMULO-958 into 1.5.1 after a
>>> test
>>> > > > phase, and after some basic experience with the feature.  However I'm
>>> > > very
>>> > > > uncomfortable including this in 1.5.0 because there is not a single
>>> > test,
>>> > > > and no real implementation behind the factory that anyone would use
>>> In
>>> > > Real
>>> > > > Life.  Is this an appropriate API?  I have no idea.  Comments in the
>>> > code
>>> > > > about the stability of the interface basically admit that the author
>>> > > isn't
>>> > > > completely comfortable with it, either.
>>> > > >
>>> > > > Let's not rush it, and when it is done right, I'm all for putting it
>>> > into
>>> > > > the next release.  For now, I would hold back incorporating these
>>> > changes
>>> > > > until they are more fully implemented. After we branch 1.5, commit
>>> this
>>> > > to
>>> > > > trunk, and back-port it to the 1.5 branch when experience and tests
>>> > show
>>> > > it
>>> > > > is ready to be released.
>>> > > >
>>> > > > -Eric
>>> > > >
>>> > > >
>>> > > >
>>> > > > On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com>
>>> > > wrote:
>>> > > >
>>> > > > > All,
>>> > > > >
>>> > > > > It's been a few days and I haven't seen much chatter at all on
>>> > > > > ACCUMULO-958 [1] since the patch was applied. There are a couple of
>>> > > > > concerns I have that I definitely want to see addressed before a
>>> > 1.5.0
>>> > > > > release.
>>> > > > >
>>> > > > > - It worries me that the provided patch is fail-open (when we can't
>>> > > load
>>> > > > > the configured encryption strategies/modules, we don't decrypt
>>> > > anything.
>>> > > > I
>>> > > > > think for a security-minded database, we should probably be
>>> > defaulting
>>> > > to
>>> > > > > fail-close; but, that brings up an issue, what happens when we
>>> can't
>>> > > > > encrypt a WAL? Do minor compactions fail gracefully? What does
>>> > Accumulo
>>> > > > do?
>>> > > > >
>>> > > > > - John said he had been reviewing the patch before he applied it;
>>> it
>>> > > > > bothers me that there was a version of this patch that had been
>>> > > reviewed
>>> > > > > privately for some amount of time when we had already pushed back
>>> the
>>> > > > > feature freeze date by a week waiting for features that weren't
>>> done.
>>> > > > >
>>> > > > > - The author noted himself with the deprecation of the CryptoModule
>>> > > > > interface that "we anticipate changing [this] in non-backwards
>>> > > compatible
>>> > > > > ways as we explore requirements for encryption in Accumulo...".
>>> This
>>> > > > tells
>>> > > > > me that implementation of WAL encryption overall hasn't been
>>> properly
>>> > > > > thought out.
>>> > > > >
>>> > > > > Given all of this, it gives me great pause to knowingly include
>>> this
>>> > > > patch
>>> > > > > into a 1.5.0 release. I see no signs that this has been truly
>>> thought
>>> > > > out,
>>> > > > > there is no default provided encryption strategy for 1.5.0 with
>>> this
>>> > > > patch
>>> > > > > for the WAL and there is still no support at all for RFile
>>> encryption
>>> > > (no
>>> > > > > end-to-end Accumulo encryption for a user). All of these issues
>>> > > > considered
>>> > > > > make me believe that this is an incomplete feature that is not
>>> ready
>>> > > for
>>> > > > an
>>> > > > > Apache Accumulo release.
>>> > > > >
>>> > > > > Thoughts?
>>> > > > >
>>> > > > > - Josh
>>> > > > >
>>> > > > > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
>>> > > > https://issues.apache.org/jira/browse/ACCUMULO-958>
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Keith Turner <ke...@deenlo.com>]

On Wed, Jan 30, 2013 at 2:02 PM, William Slacum
<wi...@accumulo.net> wrote:
> Adam, I think you need to invert your premise. You need to consider the
> benefit to the community of some piece of work before committing back to
> the community. A plug-in framework has no value if there are no plug-ins.
> Adding in untested, unreviewed layers of indirection for reading and

We are CTR.  Any committer should be willing to open discussion and
review of any change they make.  I feel that very reasonable questions
about this change are not being answered or discussed.

> writing data is a bad idea, plain and simple. Furthermore, you cannot say
> you are avoiding forking by supplying the patch and then openly state you
> are witholding portions that make said patch useful.
>
> On Wed, Jan 30, 2013 at 1:45 PM, Adam Fuchs <sc...@gmail.com> wrote:
>
>> Bill,
>>
>> The release date was not pushed back just for this ticket -- there were
>> several other changes that motivated that date change. We can discuss that
>> aspect separately from a discussion of ACCUMULO-958, and we need to start a
>> separate thread to talk about the remaining milestones before the 1.5.0
>> release.
>>
>> I would also like to amend your statement to be "... the patch has no value
>> added [for] general users [without the addition of extensions that are not
>> included with the patch]." This is a more accurate yet much weaker premise,
>> and you should consider the implications on the broader ecosystem.
>>
>> It seems to me that the main points against this patch are that it is
>> imperfect. I don't think that feature freeze is the time at which we should
>> demand perfection. Several valid issues have been raised, which should be
>> fixed by code freeze (the date of which is not yet set). However, the
>> utility of this work is obvious to me. At the end of the day, what bar are
>> we trying to set for inclusion of a patch?
>>
>> Adam
>>
>>
>>
>> On Wed, Jan 30, 2013 at 11:05 AM, William Slacum <
>> wilhelm.von.cloud@accumulo.net> wrote:
>>
>> > Bottom line, the patch has no value added to general users. The idea
>> behind
>> > pushing back a release date to stuff in unoperational code is very bad
>> > practice. It sets a precedent for not considering alternative approaches
>> > while simultaneously having no justification for choosing the approach we
>> > did. If a specific customer/group/person wants a feature, and that
>> feature
>> > does not exist yet, the code is freely available to be modified,
>> > distributed and open to public review. Adam, I strongly disagree that
>> > forking the code is bad, considering the progress that other projects
>> make
>> > specifically because they have experimental forks (HBase).
>> >
>> > On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote:
>> >
>> > > Let me attempt to make another argument for why the 958 patch should be
>> > > included in 1.5.0. What this patch represents is not an encryption
>> > solution
>> > > for WAL, but an experimental extension point that will be used for
>> > building
>> > > an encryption solution as a pluggable module. We need to judge its
>> merit
>> > > based on whether it is a successful experimental extension point or
>> not.
>> > > There are three main reasons for including the patch in 1.5.0:
>> > > 1. Test the performance impact of the null cipher solution (default
>> > > configuration) in all the performance tests we will be running for the
>> > > 1.5.0 release. If it causes problems there then we can roll it back.
>> > > 2. Enable the use of this extension after 1.5 is released. External
>> > > experiments have dependencies on this extension point. Without the
>> > > extension point we will have to test with unreleased versions of
>> > Accumulo,
>> > > which would be less than ideal.
>> > > 3. It is not harmful and somebody wants it. The reason for wanting this
>> > > code in is well documented, so you need a very strong reason to throw
>> it
>> > > out. Otherwise you will encourage forking of the project (which would
>> be
>> > > bad).
>> > >
>> > > Adam
>> > >
>> > >
>> > >
>> > >
>> > > On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com>
>> > > wrote:
>> > >
>> > > > Some comments about the comments in ACCUMULO-958:
>> > > >
>> > > > Josh writes:
>> > > >
>> > > > "We still have the ability to review this even after the feature
>> freeze
>> > > > happens, it's just frustrating from my point of view in generating
>> the
>> > > best
>> > > > 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty
>> > > darn
>> > > > quick)."
>> > > >
>> > > > John writes:
>> > > >
>> > > > "Yes, but we get stuck on x.y.* for a year or so, so it does become a
>> > > race
>> > > > to get all the features you want to see in the next year."
>> > > >
>> > > > As Accumulo matures, we will need to start thinking a little more
>> > > flexibly
>> > > > about what goes into minor releases.  We have implemented new (small)
>> > > > features in minor releases before.
>> > > >
>> > > > I would have no problem including ACCUMULO-958 into 1.5.1 after a
>> test
>> > > > phase, and after some basic experience with the feature.  However I'm
>> > > very
>> > > > uncomfortable including this in 1.5.0 because there is not a single
>> > test,
>> > > > and no real implementation behind the factory that anyone would use
>> In
>> > > Real
>> > > > Life.  Is this an appropriate API?  I have no idea.  Comments in the
>> > code
>> > > > about the stability of the interface basically admit that the author
>> > > isn't
>> > > > completely comfortable with it, either.
>> > > >
>> > > > Let's not rush it, and when it is done right, I'm all for putting it
>> > into
>> > > > the next release.  For now, I would hold back incorporating these
>> > changes
>> > > > until they are more fully implemented. After we branch 1.5, commit
>> this
>> > > to
>> > > > trunk, and back-port it to the 1.5 branch when experience and tests
>> > show
>> > > it
>> > > > is ready to be released.
>> > > >
>> > > > -Eric
>> > > >
>> > > >
>> > > >
>> > > > On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com>
>> > > wrote:
>> > > >
>> > > > > All,
>> > > > >
>> > > > > It's been a few days and I haven't seen much chatter at all on
>> > > > > ACCUMULO-958 [1] since the patch was applied. There are a couple of
>> > > > > concerns I have that I definitely want to see addressed before a
>> > 1.5.0
>> > > > > release.
>> > > > >
>> > > > > - It worries me that the provided patch is fail-open (when we can't
>> > > load
>> > > > > the configured encryption strategies/modules, we don't decrypt
>> > > anything.
>> > > > I
>> > > > > think for a security-minded database, we should probably be
>> > defaulting
>> > > to
>> > > > > fail-close; but, that brings up an issue, what happens when we
>> can't
>> > > > > encrypt a WAL? Do minor compactions fail gracefully? What does
>> > Accumulo
>> > > > do?
>> > > > >
>> > > > > - John said he had been reviewing the patch before he applied it;
>> it
>> > > > > bothers me that there was a version of this patch that had been
>> > > reviewed
>> > > > > privately for some amount of time when we had already pushed back
>> the
>> > > > > feature freeze date by a week waiting for features that weren't
>> done.
>> > > > >
>> > > > > - The author noted himself with the deprecation of the CryptoModule
>> > > > > interface that "we anticipate changing [this] in non-backwards
>> > > compatible
>> > > > > ways as we explore requirements for encryption in Accumulo...".
>> This
>> > > > tells
>> > > > > me that implementation of WAL encryption overall hasn't been
>> properly
>> > > > > thought out.
>> > > > >
>> > > > > Given all of this, it gives me great pause to knowingly include
>> this
>> > > > patch
>> > > > > into a 1.5.0 release. I see no signs that this has been truly
>> thought
>> > > > out,
>> > > > > there is no default provided encryption strategy for 1.5.0 with
>> this
>> > > > patch
>> > > > > for the WAL and there is still no support at all for RFile
>> encryption
>> > > (no
>> > > > > end-to-end Accumulo encryption for a user). All of these issues
>> > > > considered
>> > > > > make me believe that this is an incomplete feature that is not
>> ready
>> > > for
>> > > > an
>> > > > > Apache Accumulo release.
>> > > > >
>> > > > > Thoughts?
>> > > > >
>> > > > > - Josh
>> > > > >
>> > > > > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
>> > > > https://issues.apache.org/jira/browse/ACCUMULO-958>
>> > > > >
>> > > >
>> > >
>> >
>>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[William Slacum <wi...@accumulo.net>]

Adam, I think you need to invert your premise. You need to consider the
benefit to the community of some piece of work before committing back to
the community. A plug-in framework has no value if there are no plug-ins.
Adding in untested, unreviewed layers of indirection for reading and
writing data is a bad idea, plain and simple. Furthermore, you cannot say
you are avoiding forking by supplying the patch and then openly state you
are witholding portions that make said patch useful.

On Wed, Jan 30, 2013 at 1:45 PM, Adam Fuchs <sc...@gmail.com> wrote:

> Bill,
>
> The release date was not pushed back just for this ticket -- there were
> several other changes that motivated that date change. We can discuss that
> aspect separately from a discussion of ACCUMULO-958, and we need to start a
> separate thread to talk about the remaining milestones before the 1.5.0
> release.
>
> I would also like to amend your statement to be "... the patch has no value
> added [for] general users [without the addition of extensions that are not
> included with the patch]." This is a more accurate yet much weaker premise,
> and you should consider the implications on the broader ecosystem.
>
> It seems to me that the main points against this patch are that it is
> imperfect. I don't think that feature freeze is the time at which we should
> demand perfection. Several valid issues have been raised, which should be
> fixed by code freeze (the date of which is not yet set). However, the
> utility of this work is obvious to me. At the end of the day, what bar are
> we trying to set for inclusion of a patch?
>
> Adam
>
>
>
> On Wed, Jan 30, 2013 at 11:05 AM, William Slacum <
> wilhelm.von.cloud@accumulo.net> wrote:
>
> > Bottom line, the patch has no value added to general users. The idea
> behind
> > pushing back a release date to stuff in unoperational code is very bad
> > practice. It sets a precedent for not considering alternative approaches
> > while simultaneously having no justification for choosing the approach we
> > did. If a specific customer/group/person wants a feature, and that
> feature
> > does not exist yet, the code is freely available to be modified,
> > distributed and open to public review. Adam, I strongly disagree that
> > forking the code is bad, considering the progress that other projects
> make
> > specifically because they have experimental forks (HBase).
> >
> > On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote:
> >
> > > Let me attempt to make another argument for why the 958 patch should be
> > > included in 1.5.0. What this patch represents is not an encryption
> > solution
> > > for WAL, but an experimental extension point that will be used for
> > building
> > > an encryption solution as a pluggable module. We need to judge its
> merit
> > > based on whether it is a successful experimental extension point or
> not.
> > > There are three main reasons for including the patch in 1.5.0:
> > > 1. Test the performance impact of the null cipher solution (default
> > > configuration) in all the performance tests we will be running for the
> > > 1.5.0 release. If it causes problems there then we can roll it back.
> > > 2. Enable the use of this extension after 1.5 is released. External
> > > experiments have dependencies on this extension point. Without the
> > > extension point we will have to test with unreleased versions of
> > Accumulo,
> > > which would be less than ideal.
> > > 3. It is not harmful and somebody wants it. The reason for wanting this
> > > code in is well documented, so you need a very strong reason to throw
> it
> > > out. Otherwise you will encourage forking of the project (which would
> be
> > > bad).
> > >
> > > Adam
> > >
> > >
> > >
> > >
> > > On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com>
> > > wrote:
> > >
> > > > Some comments about the comments in ACCUMULO-958:
> > > >
> > > > Josh writes:
> > > >
> > > > "We still have the ability to review this even after the feature
> freeze
> > > > happens, it's just frustrating from my point of view in generating
> the
> > > best
> > > > 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty
> > > darn
> > > > quick)."
> > > >
> > > > John writes:
> > > >
> > > > "Yes, but we get stuck on x.y.* for a year or so, so it does become a
> > > race
> > > > to get all the features you want to see in the next year."
> > > >
> > > > As Accumulo matures, we will need to start thinking a little more
> > > flexibly
> > > > about what goes into minor releases.  We have implemented new (small)
> > > > features in minor releases before.
> > > >
> > > > I would have no problem including ACCUMULO-958 into 1.5.1 after a
> test
> > > > phase, and after some basic experience with the feature.  However I'm
> > > very
> > > > uncomfortable including this in 1.5.0 because there is not a single
> > test,
> > > > and no real implementation behind the factory that anyone would use
> In
> > > Real
> > > > Life.  Is this an appropriate API?  I have no idea.  Comments in the
> > code
> > > > about the stability of the interface basically admit that the author
> > > isn't
> > > > completely comfortable with it, either.
> > > >
> > > > Let's not rush it, and when it is done right, I'm all for putting it
> > into
> > > > the next release.  For now, I would hold back incorporating these
> > changes
> > > > until they are more fully implemented. After we branch 1.5, commit
> this
> > > to
> > > > trunk, and back-port it to the 1.5 branch when experience and tests
> > show
> > > it
> > > > is ready to be released.
> > > >
> > > > -Eric
> > > >
> > > >
> > > >
> > > > On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com>
> > > wrote:
> > > >
> > > > > All,
> > > > >
> > > > > It's been a few days and I haven't seen much chatter at all on
> > > > > ACCUMULO-958 [1] since the patch was applied. There are a couple of
> > > > > concerns I have that I definitely want to see addressed before a
> > 1.5.0
> > > > > release.
> > > > >
> > > > > - It worries me that the provided patch is fail-open (when we can't
> > > load
> > > > > the configured encryption strategies/modules, we don't decrypt
> > > anything.
> > > > I
> > > > > think for a security-minded database, we should probably be
> > defaulting
> > > to
> > > > > fail-close; but, that brings up an issue, what happens when we
> can't
> > > > > encrypt a WAL? Do minor compactions fail gracefully? What does
> > Accumulo
> > > > do?
> > > > >
> > > > > - John said he had been reviewing the patch before he applied it;
> it
> > > > > bothers me that there was a version of this patch that had been
> > > reviewed
> > > > > privately for some amount of time when we had already pushed back
> the
> > > > > feature freeze date by a week waiting for features that weren't
> done.
> > > > >
> > > > > - The author noted himself with the deprecation of the CryptoModule
> > > > > interface that "we anticipate changing [this] in non-backwards
> > > compatible
> > > > > ways as we explore requirements for encryption in Accumulo...".
> This
> > > > tells
> > > > > me that implementation of WAL encryption overall hasn't been
> properly
> > > > > thought out.
> > > > >
> > > > > Given all of this, it gives me great pause to knowingly include
> this
> > > > patch
> > > > > into a 1.5.0 release. I see no signs that this has been truly
> thought
> > > > out,
> > > > > there is no default provided encryption strategy for 1.5.0 with
> this
> > > > patch
> > > > > for the WAL and there is still no support at all for RFile
> encryption
> > > (no
> > > > > end-to-end Accumulo encryption for a user). All of these issues
> > > > considered
> > > > > make me believe that this is an incomplete feature that is not
> ready
> > > for
> > > > an
> > > > > Apache Accumulo release.
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > - Josh
> > > > >
> > > > > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
> > > > https://issues.apache.org/jira/browse/ACCUMULO-958>
> > > > >
> > > >
> > >
> >
>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Keith Turner <ke...@deenlo.com>]

On Wed, Jan 30, 2013 at 1:45 PM, Adam Fuchs <sc...@gmail.com> wrote:
> Bill,
>
> The release date was not pushed back just for this ticket -- there were
> several other changes that motivated that date change. We can discuss that
> aspect separately from a discussion of ACCUMULO-958, and we need to start a
> separate thread to talk about the remaining milestones before the 1.5.0
> release.
>
> I would also like to amend your statement to be "... the patch has no value
> added [for] general users [without the addition of extensions that are not
> included with the patch]." This is a more accurate yet much weaker premise,
> and you should consider the implications on the broader ecosystem.
>
> It seems to me that the main points against this patch are that it is
> imperfect. I don't think that feature freeze is the time at which we should
> demand perfection. Several valid issues have been raised, which should be
> fixed by code freeze (the date of which is not yet set). However, the
> utility of this work is obvious to me. At the end of the day, what bar are
> we trying to set for inclusion of a patch?

The patch is already included.  Its being reviewed.

My main concern is the public API, specifically the config props.  I
think its very likely there will be a desire to change them once this
feature is fully implemented.  Whats the strategy for dealing with
this?

Personally, I am not strongly opposed to including the code.  We
already have useless code floating around in various places.  I am
strongly opposed to the API changes.

>
> Adam
>
>
>
> On Wed, Jan 30, 2013 at 11:05 AM, William Slacum <
> wilhelm.von.cloud@accumulo.net> wrote:
>
>> Bottom line, the patch has no value added to general users. The idea behind
>> pushing back a release date to stuff in unoperational code is very bad
>> practice. It sets a precedent for not considering alternative approaches
>> while simultaneously having no justification for choosing the approach we
>> did. If a specific customer/group/person wants a feature, and that feature
>> does not exist yet, the code is freely available to be modified,
>> distributed and open to public review. Adam, I strongly disagree that
>> forking the code is bad, considering the progress that other projects make
>> specifically because they have experimental forks (HBase).
>>
>> On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote:
>>
>> > Let me attempt to make another argument for why the 958 patch should be
>> > included in 1.5.0. What this patch represents is not an encryption
>> solution
>> > for WAL, but an experimental extension point that will be used for
>> building
>> > an encryption solution as a pluggable module. We need to judge its merit
>> > based on whether it is a successful experimental extension point or not.
>> > There are three main reasons for including the patch in 1.5.0:
>> > 1. Test the performance impact of the null cipher solution (default
>> > configuration) in all the performance tests we will be running for the
>> > 1.5.0 release. If it causes problems there then we can roll it back.
>> > 2. Enable the use of this extension after 1.5 is released. External
>> > experiments have dependencies on this extension point. Without the
>> > extension point we will have to test with unreleased versions of
>> Accumulo,
>> > which would be less than ideal.
>> > 3. It is not harmful and somebody wants it. The reason for wanting this
>> > code in is well documented, so you need a very strong reason to throw it
>> > out. Otherwise you will encourage forking of the project (which would be
>> > bad).
>> >
>> > Adam
>> >
>> >
>> >
>> >
>> > On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com>
>> > wrote:
>> >
>> > > Some comments about the comments in ACCUMULO-958:
>> > >
>> > > Josh writes:
>> > >
>> > > "We still have the ability to review this even after the feature freeze
>> > > happens, it's just frustrating from my point of view in generating the
>> > best
>> > > 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty
>> > darn
>> > > quick)."
>> > >
>> > > John writes:
>> > >
>> > > "Yes, but we get stuck on x.y.* for a year or so, so it does become a
>> > race
>> > > to get all the features you want to see in the next year."
>> > >
>> > > As Accumulo matures, we will need to start thinking a little more
>> > flexibly
>> > > about what goes into minor releases.  We have implemented new (small)
>> > > features in minor releases before.
>> > >
>> > > I would have no problem including ACCUMULO-958 into 1.5.1 after a test
>> > > phase, and after some basic experience with the feature.  However I'm
>> > very
>> > > uncomfortable including this in 1.5.0 because there is not a single
>> test,
>> > > and no real implementation behind the factory that anyone would use In
>> > Real
>> > > Life.  Is this an appropriate API?  I have no idea.  Comments in the
>> code
>> > > about the stability of the interface basically admit that the author
>> > isn't
>> > > completely comfortable with it, either.
>> > >
>> > > Let's not rush it, and when it is done right, I'm all for putting it
>> into
>> > > the next release.  For now, I would hold back incorporating these
>> changes
>> > > until they are more fully implemented. After we branch 1.5, commit this
>> > to
>> > > trunk, and back-port it to the 1.5 branch when experience and tests
>> show
>> > it
>> > > is ready to be released.
>> > >
>> > > -Eric
>> > >
>> > >
>> > >
>> > > On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com>
>> > wrote:
>> > >
>> > > > All,
>> > > >
>> > > > It's been a few days and I haven't seen much chatter at all on
>> > > > ACCUMULO-958 [1] since the patch was applied. There are a couple of
>> > > > concerns I have that I definitely want to see addressed before a
>> 1.5.0
>> > > > release.
>> > > >
>> > > > - It worries me that the provided patch is fail-open (when we can't
>> > load
>> > > > the configured encryption strategies/modules, we don't decrypt
>> > anything.
>> > > I
>> > > > think for a security-minded database, we should probably be
>> defaulting
>> > to
>> > > > fail-close; but, that brings up an issue, what happens when we can't
>> > > > encrypt a WAL? Do minor compactions fail gracefully? What does
>> Accumulo
>> > > do?
>> > > >
>> > > > - John said he had been reviewing the patch before he applied it; it
>> > > > bothers me that there was a version of this patch that had been
>> > reviewed
>> > > > privately for some amount of time when we had already pushed back the
>> > > > feature freeze date by a week waiting for features that weren't done.
>> > > >
>> > > > - The author noted himself with the deprecation of the CryptoModule
>> > > > interface that "we anticipate changing [this] in non-backwards
>> > compatible
>> > > > ways as we explore requirements for encryption in Accumulo...". This
>> > > tells
>> > > > me that implementation of WAL encryption overall hasn't been properly
>> > > > thought out.
>> > > >
>> > > > Given all of this, it gives me great pause to knowingly include this
>> > > patch
>> > > > into a 1.5.0 release. I see no signs that this has been truly thought
>> > > out,
>> > > > there is no default provided encryption strategy for 1.5.0 with this
>> > > patch
>> > > > for the WAL and there is still no support at all for RFile encryption
>> > (no
>> > > > end-to-end Accumulo encryption for a user). All of these issues
>> > > considered
>> > > > make me believe that this is an incomplete feature that is not ready
>> > for
>> > > an
>> > > > Apache Accumulo release.
>> > > >
>> > > > Thoughts?
>> > > >
>> > > > - Josh
>> > > >
>> > > > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
>> > > https://issues.apache.org/jira/browse/ACCUMULO-958>
>> > > >
>> > >
>> >
>>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Adam Fuchs <sc...@gmail.com>]

Bill,

The release date was not pushed back just for this ticket -- there were
several other changes that motivated that date change. We can discuss that
aspect separately from a discussion of ACCUMULO-958, and we need to start a
separate thread to talk about the remaining milestones before the 1.5.0
release.

I would also like to amend your statement to be "... the patch has no value
added [for] general users [without the addition of extensions that are not
included with the patch]." This is a more accurate yet much weaker premise,
and you should consider the implications on the broader ecosystem.

It seems to me that the main points against this patch are that it is
imperfect. I don't think that feature freeze is the time at which we should
demand perfection. Several valid issues have been raised, which should be
fixed by code freeze (the date of which is not yet set). However, the
utility of this work is obvious to me. At the end of the day, what bar are
we trying to set for inclusion of a patch?

Adam



On Wed, Jan 30, 2013 at 11:05 AM, William Slacum <
wilhelm.von.cloud@accumulo.net> wrote:

> Bottom line, the patch has no value added to general users. The idea behind
> pushing back a release date to stuff in unoperational code is very bad
> practice. It sets a precedent for not considering alternative approaches
> while simultaneously having no justification for choosing the approach we
> did. If a specific customer/group/person wants a feature, and that feature
> does not exist yet, the code is freely available to be modified,
> distributed and open to public review. Adam, I strongly disagree that
> forking the code is bad, considering the progress that other projects make
> specifically because they have experimental forks (HBase).
>
> On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote:
>
> > Let me attempt to make another argument for why the 958 patch should be
> > included in 1.5.0. What this patch represents is not an encryption
> solution
> > for WAL, but an experimental extension point that will be used for
> building
> > an encryption solution as a pluggable module. We need to judge its merit
> > based on whether it is a successful experimental extension point or not.
> > There are three main reasons for including the patch in 1.5.0:
> > 1. Test the performance impact of the null cipher solution (default
> > configuration) in all the performance tests we will be running for the
> > 1.5.0 release. If it causes problems there then we can roll it back.
> > 2. Enable the use of this extension after 1.5 is released. External
> > experiments have dependencies on this extension point. Without the
> > extension point we will have to test with unreleased versions of
> Accumulo,
> > which would be less than ideal.
> > 3. It is not harmful and somebody wants it. The reason for wanting this
> > code in is well documented, so you need a very strong reason to throw it
> > out. Otherwise you will encourage forking of the project (which would be
> > bad).
> >
> > Adam
> >
> >
> >
> >
> > On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com>
> > wrote:
> >
> > > Some comments about the comments in ACCUMULO-958:
> > >
> > > Josh writes:
> > >
> > > "We still have the ability to review this even after the feature freeze
> > > happens, it's just frustrating from my point of view in generating the
> > best
> > > 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty
> > darn
> > > quick)."
> > >
> > > John writes:
> > >
> > > "Yes, but we get stuck on x.y.* for a year or so, so it does become a
> > race
> > > to get all the features you want to see in the next year."
> > >
> > > As Accumulo matures, we will need to start thinking a little more
> > flexibly
> > > about what goes into minor releases.  We have implemented new (small)
> > > features in minor releases before.
> > >
> > > I would have no problem including ACCUMULO-958 into 1.5.1 after a test
> > > phase, and after some basic experience with the feature.  However I'm
> > very
> > > uncomfortable including this in 1.5.0 because there is not a single
> test,
> > > and no real implementation behind the factory that anyone would use In
> > Real
> > > Life.  Is this an appropriate API?  I have no idea.  Comments in the
> code
> > > about the stability of the interface basically admit that the author
> > isn't
> > > completely comfortable with it, either.
> > >
> > > Let's not rush it, and when it is done right, I'm all for putting it
> into
> > > the next release.  For now, I would hold back incorporating these
> changes
> > > until they are more fully implemented. After we branch 1.5, commit this
> > to
> > > trunk, and back-port it to the 1.5 branch when experience and tests
> show
> > it
> > > is ready to be released.
> > >
> > > -Eric
> > >
> > >
> > >
> > > On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com>
> > wrote:
> > >
> > > > All,
> > > >
> > > > It's been a few days and I haven't seen much chatter at all on
> > > > ACCUMULO-958 [1] since the patch was applied. There are a couple of
> > > > concerns I have that I definitely want to see addressed before a
> 1.5.0
> > > > release.
> > > >
> > > > - It worries me that the provided patch is fail-open (when we can't
> > load
> > > > the configured encryption strategies/modules, we don't decrypt
> > anything.
> > > I
> > > > think for a security-minded database, we should probably be
> defaulting
> > to
> > > > fail-close; but, that brings up an issue, what happens when we can't
> > > > encrypt a WAL? Do minor compactions fail gracefully? What does
> Accumulo
> > > do?
> > > >
> > > > - John said he had been reviewing the patch before he applied it; it
> > > > bothers me that there was a version of this patch that had been
> > reviewed
> > > > privately for some amount of time when we had already pushed back the
> > > > feature freeze date by a week waiting for features that weren't done.
> > > >
> > > > - The author noted himself with the deprecation of the CryptoModule
> > > > interface that "we anticipate changing [this] in non-backwards
> > compatible
> > > > ways as we explore requirements for encryption in Accumulo...". This
> > > tells
> > > > me that implementation of WAL encryption overall hasn't been properly
> > > > thought out.
> > > >
> > > > Given all of this, it gives me great pause to knowingly include this
> > > patch
> > > > into a 1.5.0 release. I see no signs that this has been truly thought
> > > out,
> > > > there is no default provided encryption strategy for 1.5.0 with this
> > > patch
> > > > for the WAL and there is still no support at all for RFile encryption
> > (no
> > > > end-to-end Accumulo encryption for a user). All of these issues
> > > considered
> > > > make me believe that this is an incomplete feature that is not ready
> > for
> > > an
> > > > Apache Accumulo release.
> > > >
> > > > Thoughts?
> > > >
> > > > - Josh
> > > >
> > > > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
> > > https://issues.apache.org/jira/browse/ACCUMULO-958>
> > > >
> > >
> >
>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[William Slacum <wi...@accumulo.net>]

Bottom line, the patch has no value added to general users. The idea behind
pushing back a release date to stuff in unoperational code is very bad
practice. It sets a precedent for not considering alternative approaches
while simultaneously having no justification for choosing the approach we
did. If a specific customer/group/person wants a feature, and that feature
does not exist yet, the code is freely available to be modified,
distributed and open to public review. Adam, I strongly disagree that
forking the code is bad, considering the progress that other projects make
specifically because they have experimental forks (HBase).

On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <af...@apache.org> wrote:

> Let me attempt to make another argument for why the 958 patch should be
> included in 1.5.0. What this patch represents is not an encryption solution
> for WAL, but an experimental extension point that will be used for building
> an encryption solution as a pluggable module. We need to judge its merit
> based on whether it is a successful experimental extension point or not.
> There are three main reasons for including the patch in 1.5.0:
> 1. Test the performance impact of the null cipher solution (default
> configuration) in all the performance tests we will be running for the
> 1.5.0 release. If it causes problems there then we can roll it back.
> 2. Enable the use of this extension after 1.5 is released. External
> experiments have dependencies on this extension point. Without the
> extension point we will have to test with unreleased versions of Accumulo,
> which would be less than ideal.
> 3. It is not harmful and somebody wants it. The reason for wanting this
> code in is well documented, so you need a very strong reason to throw it
> out. Otherwise you will encourage forking of the project (which would be
> bad).
>
> Adam
>
>
>
>
> On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com>
> wrote:
>
> > Some comments about the comments in ACCUMULO-958:
> >
> > Josh writes:
> >
> > "We still have the ability to review this even after the feature freeze
> > happens, it's just frustrating from my point of view in generating the
> best
> > 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty
> darn
> > quick)."
> >
> > John writes:
> >
> > "Yes, but we get stuck on x.y.* for a year or so, so it does become a
> race
> > to get all the features you want to see in the next year."
> >
> > As Accumulo matures, we will need to start thinking a little more
> flexibly
> > about what goes into minor releases.  We have implemented new (small)
> > features in minor releases before.
> >
> > I would have no problem including ACCUMULO-958 into 1.5.1 after a test
> > phase, and after some basic experience with the feature.  However I'm
> very
> > uncomfortable including this in 1.5.0 because there is not a single test,
> > and no real implementation behind the factory that anyone would use In
> Real
> > Life.  Is this an appropriate API?  I have no idea.  Comments in the code
> > about the stability of the interface basically admit that the author
> isn't
> > completely comfortable with it, either.
> >
> > Let's not rush it, and when it is done right, I'm all for putting it into
> > the next release.  For now, I would hold back incorporating these changes
> > until they are more fully implemented. After we branch 1.5, commit this
> to
> > trunk, and back-port it to the 1.5 branch when experience and tests show
> it
> > is ready to be released.
> >
> > -Eric
> >
> >
> >
> > On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com>
> wrote:
> >
> > > All,
> > >
> > > It's been a few days and I haven't seen much chatter at all on
> > > ACCUMULO-958 [1] since the patch was applied. There are a couple of
> > > concerns I have that I definitely want to see addressed before a 1.5.0
> > > release.
> > >
> > > - It worries me that the provided patch is fail-open (when we can't
> load
> > > the configured encryption strategies/modules, we don't decrypt
> anything.
> > I
> > > think for a security-minded database, we should probably be defaulting
> to
> > > fail-close; but, that brings up an issue, what happens when we can't
> > > encrypt a WAL? Do minor compactions fail gracefully? What does Accumulo
> > do?
> > >
> > > - John said he had been reviewing the patch before he applied it; it
> > > bothers me that there was a version of this patch that had been
> reviewed
> > > privately for some amount of time when we had already pushed back the
> > > feature freeze date by a week waiting for features that weren't done.
> > >
> > > - The author noted himself with the deprecation of the CryptoModule
> > > interface that "we anticipate changing [this] in non-backwards
> compatible
> > > ways as we explore requirements for encryption in Accumulo...". This
> > tells
> > > me that implementation of WAL encryption overall hasn't been properly
> > > thought out.
> > >
> > > Given all of this, it gives me great pause to knowingly include this
> > patch
> > > into a 1.5.0 release. I see no signs that this has been truly thought
> > out,
> > > there is no default provided encryption strategy for 1.5.0 with this
> > patch
> > > for the WAL and there is still no support at all for RFile encryption
> (no
> > > end-to-end Accumulo encryption for a user). All of these issues
> > considered
> > > make me believe that this is an incomplete feature that is not ready
> for
> > an
> > > Apache Accumulo release.
> > >
> > > Thoughts?
> > >
> > > - Josh
> > >
> > > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
> > https://issues.apache.org/jira/browse/ACCUMULO-958>
> > >
> >
>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Adam Fuchs <af...@apache.org>]

Let me attempt to make another argument for why the 958 patch should be
included in 1.5.0. What this patch represents is not an encryption solution
for WAL, but an experimental extension point that will be used for building
an encryption solution as a pluggable module. We need to judge its merit
based on whether it is a successful experimental extension point or not.
There are three main reasons for including the patch in 1.5.0:
1. Test the performance impact of the null cipher solution (default
configuration) in all the performance tests we will be running for the
1.5.0 release. If it causes problems there then we can roll it back.
2. Enable the use of this extension after 1.5 is released. External
experiments have dependencies on this extension point. Without the
extension point we will have to test with unreleased versions of Accumulo,
which would be less than ideal.
3. It is not harmful and somebody wants it. The reason for wanting this
code in is well documented, so you need a very strong reason to throw it
out. Otherwise you will encourage forking of the project (which would be
bad).

Adam




On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <er...@gmail.com> wrote:

> Some comments about the comments in ACCUMULO-958:
>
> Josh writes:
>
> "We still have the ability to review this even after the feature freeze
> happens, it's just frustrating from my point of view in generating the best
> 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty darn
> quick)."
>
> John writes:
>
> "Yes, but we get stuck on x.y.* for a year or so, so it does become a race
> to get all the features you want to see in the next year."
>
> As Accumulo matures, we will need to start thinking a little more flexibly
> about what goes into minor releases.  We have implemented new (small)
> features in minor releases before.
>
> I would have no problem including ACCUMULO-958 into 1.5.1 after a test
> phase, and after some basic experience with the feature.  However I'm very
> uncomfortable including this in 1.5.0 because there is not a single test,
> and no real implementation behind the factory that anyone would use In Real
> Life.  Is this an appropriate API?  I have no idea.  Comments in the code
> about the stability of the interface basically admit that the author isn't
> completely comfortable with it, either.
>
> Let's not rush it, and when it is done right, I'm all for putting it into
> the next release.  For now, I would hold back incorporating these changes
> until they are more fully implemented. After we branch 1.5, commit this to
> trunk, and back-port it to the 1.5 branch when experience and tests show it
> is ready to be released.
>
> -Eric
>
>
>
> On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com> wrote:
>
> > All,
> >
> > It's been a few days and I haven't seen much chatter at all on
> > ACCUMULO-958 [1] since the patch was applied. There are a couple of
> > concerns I have that I definitely want to see addressed before a 1.5.0
> > release.
> >
> > - It worries me that the provided patch is fail-open (when we can't load
> > the configured encryption strategies/modules, we don't decrypt anything.
> I
> > think for a security-minded database, we should probably be defaulting to
> > fail-close; but, that brings up an issue, what happens when we can't
> > encrypt a WAL? Do minor compactions fail gracefully? What does Accumulo
> do?
> >
> > - John said he had been reviewing the patch before he applied it; it
> > bothers me that there was a version of this patch that had been reviewed
> > privately for some amount of time when we had already pushed back the
> > feature freeze date by a week waiting for features that weren't done.
> >
> > - The author noted himself with the deprecation of the CryptoModule
> > interface that "we anticipate changing [this] in non-backwards compatible
> > ways as we explore requirements for encryption in Accumulo...". This
> tells
> > me that implementation of WAL encryption overall hasn't been properly
> > thought out.
> >
> > Given all of this, it gives me great pause to knowingly include this
> patch
> > into a 1.5.0 release. I see no signs that this has been truly thought
> out,
> > there is no default provided encryption strategy for 1.5.0 with this
> patch
> > for the WAL and there is still no support at all for RFile encryption (no
> > end-to-end Accumulo encryption for a user). All of these issues
> considered
> > make me believe that this is an incomplete feature that is not ready for
> an
> > Apache Accumulo release.
> >
> > Thoughts?
> >
> > - Josh
> >
> > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
> https://issues.apache.org/jira/browse/ACCUMULO-958>
> >
>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Eric Newton <er...@gmail.com>]

Some comments about the comments in ACCUMULO-958:

Josh writes:

"We still have the ability to review this even after the feature freeze
happens, it's just frustrating from my point of view in generating the best
1.5.0 candidate possible (we tend to go through x.y.0 releases pretty darn
quick)."

John writes:

"Yes, but we get stuck on x.y.* for a year or so, so it does become a race
to get all the features you want to see in the next year."

As Accumulo matures, we will need to start thinking a little more flexibly
about what goes into minor releases.  We have implemented new (small)
features in minor releases before.

I would have no problem including ACCUMULO-958 into 1.5.1 after a test
phase, and after some basic experience with the feature.  However I'm very
uncomfortable including this in 1.5.0 because there is not a single test,
and no real implementation behind the factory that anyone would use In Real
Life.  Is this an appropriate API?  I have no idea.  Comments in the code
about the stability of the interface basically admit that the author isn't
completely comfortable with it, either.

Let's not rush it, and when it is done right, I'm all for putting it into
the next release.  For now, I would hold back incorporating these changes
until they are more fully implemented. After we branch 1.5, commit this to
trunk, and back-port it to the 1.5 branch when experience and tests show it
is ready to be released.

-Eric



On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com> wrote:

> All,
>
> It's been a few days and I haven't seen much chatter at all on
> ACCUMULO-958 [1] since the patch was applied. There are a couple of
> concerns I have that I definitely want to see addressed before a 1.5.0
> release.
>
> - It worries me that the provided patch is fail-open (when we can't load
> the configured encryption strategies/modules, we don't decrypt anything. I
> think for a security-minded database, we should probably be defaulting to
> fail-close; but, that brings up an issue, what happens when we can't
> encrypt a WAL? Do minor compactions fail gracefully? What does Accumulo do?
>
> - John said he had been reviewing the patch before he applied it; it
> bothers me that there was a version of this patch that had been reviewed
> privately for some amount of time when we had already pushed back the
> feature freeze date by a week waiting for features that weren't done.
>
> - The author noted himself with the deprecation of the CryptoModule
> interface that "we anticipate changing [this] in non-backwards compatible
> ways as we explore requirements for encryption in Accumulo...". This tells
> me that implementation of WAL encryption overall hasn't been properly
> thought out.
>
> Given all of this, it gives me great pause to knowingly include this patch
> into a 1.5.0 release. I see no signs that this has been truly thought out,
> there is no default provided encryption strategy for 1.5.0 with this patch
> for the WAL and there is still no support at all for RFile encryption (no
> end-to-end Accumulo encryption for a user). All of these issues considered
> make me believe that this is an incomplete feature that is not ready for an
> Apache Accumulo release.
>
> Thoughts?
>
> - Josh
>
> [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<https://issues.apache.org/jira/browse/ACCUMULO-958>
>

Re: ACCUMULO-958 - Pluggable encryption in walogs

[Adam Fuchs <sc...@gmail.com>]

Josh,

Mike Allen is still working on ACCUMULO-958, and will have an updated patch
in the next couple of weeks. We were hoping to get the more complete
encryption strategy into 1.5, but were not able to complete it by feature
freeze. However, the WAL encryption as is, when configured with the default
settings, should be no harm to 1.5 -- there's no reason to pull it out or
get concerned about it. We just can't advertise it as a feature of 1.5.
This is one of the reasons why some of the methods are marked as deprecated.

The more complete encryption story, which should be in place for the 1.6
release, should be discussed in ACCUMULO-998.

Cheers,
Adam



On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <jo...@gmail.com> wrote:

> All,
>
> It's been a few days and I haven't seen much chatter at all on
> ACCUMULO-958 [1] since the patch was applied. There are a couple of
> concerns I have that I definitely want to see addressed before a 1.5.0
> release.
>
> - It worries me that the provided patch is fail-open (when we can't load
> the configured encryption strategies/modules, we don't decrypt anything. I
> think for a security-minded database, we should probably be defaulting to
> fail-close; but, that brings up an issue, what happens when we can't
> encrypt a WAL? Do minor compactions fail gracefully? What does Accumulo do?
>
> - John said he had been reviewing the patch before he applied it; it
> bothers me that there was a version of this patch that had been reviewed
> privately for some amount of time when we had already pushed back the
> feature freeze date by a week waiting for features that weren't done.
>
> - The author noted himself with the deprecation of the CryptoModule
> interface that "we anticipate changing [this] in non-backwards compatible
> ways as we explore requirements for encryption in Accumulo...". This tells
> me that implementation of WAL encryption overall hasn't been properly
> thought out.
>
> Given all of this, it gives me great pause to knowingly include this patch
> into a 1.5.0 release. I see no signs that this has been truly thought out,
> there is no default provided encryption strategy for 1.5.0 with this patch
> for the WAL and there is still no support at all for RFile encryption (no
> end-to-end Accumulo encryption for a user). All of these issues considered
> make me believe that this is an incomplete feature that is not ready for an
> Apache Accumulo release.
>
> Thoughts?
>
> - Josh
>
> [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<https://issues.apache.org/jira/browse/ACCUMULO-958>
>