Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
On Apr 6, 2012 11:29 PM, "Hyrum K Wright" wrote: > > On Fri, Apr 6, 2012 at 8:17 PM, Greg Stein wrote: > > On Fri, Apr 6, 2012 at 17:36, Hyrum K Wright wrote: > >> On Fri, Apr 6, 2012 at 4:31 PM, Greg Stein wrote: >... > >>> I should add some canonical assertions for all the relpath params, and > >>> get you to fix that bug :-) > >> > >> Go for it! You're welcome to use the branch, but it'd probably be > >> best for these checks to end up on trunk, either originally, or via a > >> cherrypick merge from the branch. > > > > r1310655 on trunk. With shims activated: 339 failures now :-) > > Do the failures happen on trunk or the branch? (I suspect the branch > as well as the trunk, knowing you. :P ) "On trunk" ... if you update the branch, then there, too. :-P (told ya not to pass URLs into Ev2 as relpaths ;-) ) >... > > Sounds like some new tests are going to come along :-P > > Thanks for volunteering. :) I think you misunderstood just who was volunteering :-) Cheers, -g
Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
On Fri, Apr 6, 2012 at 8:17 PM, Greg Stein wrote: > On Fri, Apr 6, 2012 at 17:36, Hyrum K Wright > wrote: >> On Fri, Apr 6, 2012 at 4:31 PM, Greg Stein wrote: >>> On Fri, Apr 6, 2012 at 16:53, Hyrum K Wright >>> wrote: ... Just so you know, I would have expected this section to crash Ev2 in copy test 67, since there are several directories being added, but the children array is (currently) empty. I wonder if there is a problem with the internal Ev2 state checking. >>> >>> I'm suspicious that the checks are not working because of the "pass >>> URLs as relpath" that you're doing. >>> >>> I should add some canonical assertions for all the relpath params, and >>> get you to fix that bug :-) >> >> Go for it! You're welcome to use the branch, but it'd probably be >> best for these checks to end up on trunk, either originally, or via a >> cherrypick merge from the branch. > > r1310655 on trunk. With shims activated: 339 failures now :-) Do the failures happen on trunk or the branch? (I suspect the branch as well as the trunk, knowing you. :P ) > I only added relpath assertions. I did not write the code to check for > improper parent/child addition order. I'll do that in a second commit. > >> I plan on fixing the various ### comments in copy.c on the branch, as >> those are issues I've identified with the current code. It's >> unfortunate that we don't have tests which test these cases. :( > > Sounds like some new tests are going to come along :-P Thanks for volunteering. :) -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
On Fri, Apr 6, 2012 at 17:36, Hyrum K Wright wrote: > On Fri, Apr 6, 2012 at 4:31 PM, Greg Stein wrote: >> On Fri, Apr 6, 2012 at 16:53, Hyrum K Wright >> wrote: >>>... >>> Just so you know, I would have expected this section to crash Ev2 in >>> copy test 67, since there are several directories being added, but the >>> children array is (currently) empty. I wonder if there is a problem >>> with the internal Ev2 state checking. >> >> I'm suspicious that the checks are not working because of the "pass >> URLs as relpath" that you're doing. >> >> I should add some canonical assertions for all the relpath params, and >> get you to fix that bug :-) > > Go for it! You're welcome to use the branch, but it'd probably be > best for these checks to end up on trunk, either originally, or via a > cherrypick merge from the branch. r1310655 on trunk. With shims activated: 339 failures now :-) I only added relpath assertions. I did not write the code to check for improper parent/child addition order. I'll do that in a second commit. > I plan on fixing the various ### comments in copy.c on the branch, as > those are issues I've identified with the current code. It's > unfortunate that we don't have tests which test these cases. :( Sounds like some new tests are going to come along :-P Cheers, -g
Re: Ev2 RA protocols
On Fri, Apr 6, 2012 at 20:29, Ivan Zhakov wrote: > On Sat, Apr 7, 2012 at 01:10, Greg Stein wrote: > [...] >> The plan is a new opaque structure that holds delete/modify/add >> markers for relpaths. It doesn't need copy/move information, but just >> what will be done at the relpaths. > > Bikeshed: may be 'svn_editor_plan_t' -> 'svn_editor_outline_t' ? Sure, that could work too. Cheers, -g
Re: Ev2 RA protocols
On Sat, Apr 7, 2012 at 01:10, Greg Stein wrote: [...] > > The plan is a new opaque structure that holds delete/modify/add > markers for relpaths. It doesn't need copy/move information, but just > what will be done at the relpaths. Bikeshed: may be 'svn_editor_plan_t' -> 'svn_editor_outline_t' ? -- Ivan Zhakov
Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
On Fri, Apr 6, 2012 at 4:31 PM, Greg Stein wrote:
> On Fri, Apr 6, 2012 at 16:53, Hyrum K Wright
> wrote:
>> On Fri, Apr 6, 2012 at 3:48 PM, wrote:
>>> Author: hwright
>>> Date: Fri Apr 6 20:48:32 2012
>>> New Revision: 1310581
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1310581&view=rev
>>> Log:
>>> On the ev2-export branch:
>>> Directly add any new directories, rather than through the path_info structs.
>>>
>>> * subversion/libsvn_client/copy.c
>>> (path_driver_info_t): Remove dir_add member.
>>> (drive_single_path): Don't check for the dir_add flag.
>>> (drive_editor): Handle the new dirs before the various copy/move paths.
>>> (repos_to_repos_copy): Don't bother putting the new directoris into the
>>> set of paths to be operated on, just give them to drive_editor().
>>>
>>> Modified:
>>> subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>>>
>>> Modified: subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>>> URL:
>>> http://svn.apache.org/viewvc/subversion/branches/ev2-export/subversion/libsvn_client/copy.c?rev=1310581&r1=1310580&r2=1310581&view=diff
>>> ==
>>> --- subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>>> (original)
>>> +++ subversion/branches/ev2-export/subversion/libsvn_client/copy.c Fri Apr
>>> 6 20:48:32 2012
>>> @@ -532,7 +532,6 @@ typedef struct path_driver_info_t
>>> svn_node_kind_t src_kind;
>>> svn_revnum_t src_revnum;
>>> svn_boolean_t resurrection;
>>> - svn_boolean_t dir_add;
>>> svn_string_t *mergeinfo; /* the new mergeinfo for the target */
>>> } path_driver_info_t;
>>>
>>> @@ -627,19 +626,9 @@ drive_single_path(svn_editor_t *editor,
>>> apr_pool_t *scratch_pool)
>>> {
>>> apr_hash_t *props = apr_hash_make(scratch_pool);
>>> - apr_array_header_t *children = apr_array_make(scratch_pool, 1,
>>> - sizeof(const char *));
>>> svn_boolean_t do_delete = FALSE;
>>> svn_boolean_t do_add = FALSE;
>>>
>>> - /* Check to see if we need to add the path as a directory. */
>>> - if (path_info->dir_add)
>>> - {
>>> - return svn_error_trace(svn_editor_add_directory(editor, path,
>>> children,
>>> - props,
>>> - SVN_INVALID_REVNUM));
>>> - }
>>> -
>>> /* If this is a resurrection, we know the source and dest paths are
>>> the same, and that our driver will only be calling us once. */
>>> if (path_info->resurrection)
>>> @@ -702,6 +691,7 @@ static svn_error_t *
>>> drive_editor(svn_editor_t *editor,
>>> apr_array_header_t *paths,
>>> apr_hash_t *action_hash,
>>> + apr_array_header_t *new_dirs,
>>> svn_boolean_t is_move,
>>> svn_revnum_t youngest,
>>> apr_pool_t *scratch_pool)
>>> @@ -710,6 +700,23 @@ drive_editor(svn_editor_t *editor,
>>> apr_pool_t *iterpool;
>>> int i;
>>>
>>> + if (new_dirs)
>>> + {
>>> + apr_array_header_t *children = apr_array_make(scratch_pool, 1,
>>> + sizeof(const char *));
>>> + apr_hash_t *props = apr_hash_make(scratch_pool);
>>> +
>>> + /* These are directories which we just need to add. */
>>> + for (i = 0; i < new_dirs->nelts; i++)
>>> + {
>>> + const char *dir = APR_ARRAY_IDX(new_dirs, i, const char *);
>>> +
>>> + /* ### The children param needs to be populated correctly. */
>>> + SVN_ERR(svn_editor_add_directory(editor, dir, children, props,
>>> + SVN_INVALID_REVNUM));
>>
>> Greg,
>>
>> Just so you know, I would have expected this section to crash Ev2 in
>> copy test 67, since there are several directories being added, but the
>> children array is (currently) empty. I wonder if there is a problem
>> with the internal Ev2 state checking.
>
> I'm suspicious that the checks are not working because of the "pass
> URLs as relpath" that you're doing.
>
> I should add some canonical assertions for all the relpath params, and
> get you to fix that bug :-)
Go for it! You're welcome to use the branch, but it'd probably be
best for these checks to end up on trunk, either originally, or via a
cherrypick merge from the branch.
I plan on fixing the various ### comments in copy.c on the branch, as
those are issues I've identified with the current code. It's
unfortunate that we don't have tests which test these cases. :(
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
On Fri, Apr 6, 2012 at 16:53, Hyrum K Wright wrote:
> On Fri, Apr 6, 2012 at 3:48 PM, wrote:
>> Author: hwright
>> Date: Fri Apr 6 20:48:32 2012
>> New Revision: 1310581
>>
>> URL: http://svn.apache.org/viewvc?rev=1310581&view=rev
>> Log:
>> On the ev2-export branch:
>> Directly add any new directories, rather than through the path_info structs.
>>
>> * subversion/libsvn_client/copy.c
>> (path_driver_info_t): Remove dir_add member.
>> (drive_single_path): Don't check for the dir_add flag.
>> (drive_editor): Handle the new dirs before the various copy/move paths.
>> (repos_to_repos_copy): Don't bother putting the new directoris into the
>> set of paths to be operated on, just give them to drive_editor().
>>
>> Modified:
>> subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>>
>> Modified: subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/branches/ev2-export/subversion/libsvn_client/copy.c?rev=1310581&r1=1310580&r2=1310581&view=diff
>> ==
>> --- subversion/branches/ev2-export/subversion/libsvn_client/copy.c (original)
>> +++ subversion/branches/ev2-export/subversion/libsvn_client/copy.c Fri Apr
>> 6 20:48:32 2012
>> @@ -532,7 +532,6 @@ typedef struct path_driver_info_t
>> svn_node_kind_t src_kind;
>> svn_revnum_t src_revnum;
>> svn_boolean_t resurrection;
>> - svn_boolean_t dir_add;
>> svn_string_t *mergeinfo; /* the new mergeinfo for the target */
>> } path_driver_info_t;
>>
>> @@ -627,19 +626,9 @@ drive_single_path(svn_editor_t *editor,
>> apr_pool_t *scratch_pool)
>> {
>> apr_hash_t *props = apr_hash_make(scratch_pool);
>> - apr_array_header_t *children = apr_array_make(scratch_pool, 1,
>> - sizeof(const char *));
>> svn_boolean_t do_delete = FALSE;
>> svn_boolean_t do_add = FALSE;
>>
>> - /* Check to see if we need to add the path as a directory. */
>> - if (path_info->dir_add)
>> - {
>> - return svn_error_trace(svn_editor_add_directory(editor, path,
>> children,
>> - props,
>> - SVN_INVALID_REVNUM));
>> - }
>> -
>> /* If this is a resurrection, we know the source and dest paths are
>> the same, and that our driver will only be calling us once. */
>> if (path_info->resurrection)
>> @@ -702,6 +691,7 @@ static svn_error_t *
>> drive_editor(svn_editor_t *editor,
>> apr_array_header_t *paths,
>> apr_hash_t *action_hash,
>> + apr_array_header_t *new_dirs,
>> svn_boolean_t is_move,
>> svn_revnum_t youngest,
>> apr_pool_t *scratch_pool)
>> @@ -710,6 +700,23 @@ drive_editor(svn_editor_t *editor,
>> apr_pool_t *iterpool;
>> int i;
>>
>> + if (new_dirs)
>> + {
>> + apr_array_header_t *children = apr_array_make(scratch_pool, 1,
>> + sizeof(const char *));
>> + apr_hash_t *props = apr_hash_make(scratch_pool);
>> +
>> + /* These are directories which we just need to add. */
>> + for (i = 0; i < new_dirs->nelts; i++)
>> + {
>> + const char *dir = APR_ARRAY_IDX(new_dirs, i, const char *);
>> +
>> + /* ### The children param needs to be populated correctly. */
>> + SVN_ERR(svn_editor_add_directory(editor, dir, children, props,
>> + SVN_INVALID_REVNUM));
>
> Greg,
>
> Just so you know, I would have expected this section to crash Ev2 in
> copy test 67, since there are several directories being added, but the
> children array is (currently) empty. I wonder if there is a problem
> with the internal Ev2 state checking.
I'm suspicious that the checks are not working because of the "pass
URLs as relpath" that you're doing.
I should add some canonical assertions for all the relpath params, and
get you to fix that bug :-)
> Also, this currently feeds paths to Ev2 child-first. For instance,
> copy test 67 calls this with 'X/Y/Z', 'X/Y', and 'X', in that order.
> The shims reorder things so that it gets done properly, but I wonder
> if there shouldn't be some state checks in Ev2 to ensure parents are
> added before children.
That wouldn't be too hard. We could mark the parent as "only allowed
to modify" (as long as it hasn't already been marked as
"added/unalterable").
Cheers,
-g
Re: APR hash order ruby test failure
On Fri, Apr 6, 2012 at 11:32 AM, Joe Swatosh wrote:
> On Wed, Mar 28, 2012 at 2:30 AM, Philip Martin
> wrote:
>> Philip Martin writes:
>>
>>> There is another failure in the ruby testsuite:
>>>
>>> http://ci.apache.org/builders/svn-x64-ubuntu-gcc/builds/4626
>>>
>>> 1) Failure:
>>> test_changelists_get_with_block(SvnClientTest)
>>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_client.rb:2296:in
>>> `assert_changelists'
>>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/util.rb:204:in
>>> `make_context'
>>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_client.rb:2288:in
>>> `assert_changelists'
>>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_client.rb:2349:in
>>> `test_changelists_get_with_block':
>>> <{nil=>
>>> ["/tmp/d20120322-8616-qtl2ah/wc",
>>> "/tmp/d20120322-8616-qtl2ah/wc/hello1.txt",
>>> "/tmp/d20120322-8616-qtl2ah/wc/hello2.txt"]}> expected but was
>>> <{nil=>
>>> ["/tmp/d20120322-8616-qtl2ah/wc",
>>> "/tmp/d20120322-8616-qtl2ah/wc/hello2.txt",
>>> "/tmp/d20120322-8616-qtl2ah/wc/hello1.txt"]}>.
>>
>> Another failure that looks like a similar problem in a different place:
>>
>> 1) Failure:
>> test_diff_callbacks_for_backward_compatibility(SvnWcTest)
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_wc.rb:756:in
>> `assert_diff_callbacks'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/svn/wc.rb:136:in
>> `_open'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/svn/wc.rb:114:in
>> `open'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_wc.rb:711:in
>> `assert_diff_callbacks'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/svn/ra.rb:52:in
>> `open'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_wc.rb:699:in
>> `assert_diff_callbacks'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/util.rb:204:in
>> `make_context'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_wc.rb:696:in
>> `assert_diff_callbacks'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_wc.rb:764:in
>> `test_diff_callbacks_for_backward_compatibility':
>> <[[:dir_props_changed,
>> "/tmp/d20120327-16259-17t4nz8/wc",
>> [#> @name="svn:entry:committed-date",
>> @value=nil>,
>> #> @name="svn:entry:committed-rev",
>> @value=nil>,
>> #> @value=nil>,
>> #]],
>> [:file_changed,
>> "/tmp/d20120327-16259-17t4nz8/wc/dir/hello.txt",
>> [#]],
>> [:file_added, "/tmp/d20120327-16259-17t4nz8/wc/dir/hello2.txt", []]]>
>> expected but was
>> <[[:dir_props_changed,
>> "/tmp/d20120327-16259-17t4nz8/wc",
>> [#> @name="svn:entry:committed-date",
>> @value=nil>,
>> #> @name="svn:entry:committed-rev",
>> @value=nil>,
>> #> @value=nil>,
>> #]],
>> [:file_added, "/tmp/d20120327-16259-17t4nz8/wc/dir/hello2.txt", []],
>> [:file_changed,
>> "/tmp/d20120327-16259-17t4nz8/wc/dir/hello.txt",
>> [#]]]>.
>>
>> We fix these by converting the arrays to sets so that the order is
>> irrelevant. The open question is whether we do this solely within the
>> testsuite, changing both sides of the comparison, or whether we change
>> the bindings to return a set and adjust the testsuite to match.
>>
>
> I think I've addressed the first failure in r1310535. I will continue
> looking into the wc failure.
>
I think r1310594 takes care of the wc failures in the Ruby bindings.
I'm not seeing other test failures locally, please let me know if
others start failing
--
Joe
Re: Ev2 RA protocols
On Fri, Apr 6, 2012 at 14:24, Hyrum K Wright wrote: > On Fri, Apr 6, 2012 at 1:10 PM, Greg Stein wrote: >> >> On Apr 6, 2012 2:05 PM, "Hyrum K Wright" wrote: >>> >>> In case folks haven't been following commits@, there's been a bit of >>> work on the ev2-export branch on implementing Ev2 drivers for commits. >>> While we're not quite ready for it yet, at some point we'll need to >>> start thinking about how to marshall Ev2 drives over the wire to >>> waiting servers. As this design work is largely orthogonal to the >>> current implementation efforts, and my familiarity with our existing >>> protocols is pretty weak, I'd invite other folks to start thinking >>> about these issues and discussing them here. >> >> Already ahead of you :-) >> >> I was going to suggest a new RA API rather than rev'ing get_commit_editor. >> By going that route, the coding can occur on trunk, and we migrate as >> appropriate, leaving the delta editor in place. > > Yep, that's the same conclusion I reached a few days ago. ;) I'm thinking of a signature kind of like this: svn_ra_begin_commit( svn_editor_t **editor, const svn_editor_plan_t *plan, apr_hash_t *revprops, svn_commit_callback2_t callback, void *callback_baton, apr_hash_t *lock_tokens, svn_boolean_t keep_locks, result_pool, scratch_pool) It looks very similar to the get_commit_editor() function, except for taking a plan. That will be delivered to the server, and if it comes back "ok", then you get an editor back which you then drive (much like today). The plan is used for out-of-dateness checks and authorization (ie. "okay to proceed"), and to avoid sending content to the server when the checksum matches (we just close the CONTENTS stream rather than deltify/deliver to the server). With this approach, we don't need to worry about Ev2 random order or holding back content streams. The plan is a new opaque structure that holds delete/modify/add markers for relpaths. It doesn't need copy/move information, but just what will be done at the relpaths. The server does authz, and makes sure the revision is proper for deletes and modifications, and that an add/add conflict has not occurred. Children of an added directory do not need to be stored in the plan. Result checksums are stored for each modified/added file. We may find some other bits to store, but that's the basics. It is constructed by the commit drivers. If an RA layer has not (yet) been coded to deal with the commit plan, then some common backwards-compat code can do the right mix of open_file/open_dir/add_dir/delete/etc that we normally use at this part of a commit. (this may require storing copyfrom information to pass to add_file/add_dir). Point here is that we shouldn't need any shims; just this interop code for third-party RA layers, or for our layers which have not been updated. > >> This would be the new "commit plan, followed by drive" design we discussed. >> >> The new RA entrypoint(s) could even have a default implementation in ra/ >> that would use the original delta editor. Thus, we don't even have to >> implement the new interface for all layers. > > This is a possibility. The current commit_editor4() on the ev2-export > branch wraps the Ev1 implementation using the shims. Well... I'm not talking about using the shims, in favor of compatibility code that is attuned to the commit process. And the fact that we're already got a bunch of file/dir batons from the planning step. That's my thinking so far... Cheers, -g
Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
On Fri, Apr 6, 2012 at 3:48 PM, wrote:
> Author: hwright
> Date: Fri Apr 6 20:48:32 2012
> New Revision: 1310581
>
> URL: http://svn.apache.org/viewvc?rev=1310581&view=rev
> Log:
> On the ev2-export branch:
> Directly add any new directories, rather than through the path_info structs.
>
> * subversion/libsvn_client/copy.c
> (path_driver_info_t): Remove dir_add member.
> (drive_single_path): Don't check for the dir_add flag.
> (drive_editor): Handle the new dirs before the various copy/move paths.
> (repos_to_repos_copy): Don't bother putting the new directoris into the
> set of paths to be operated on, just give them to drive_editor().
>
> Modified:
> subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>
> Modified: subversion/branches/ev2-export/subversion/libsvn_client/copy.c
> URL:
> http://svn.apache.org/viewvc/subversion/branches/ev2-export/subversion/libsvn_client/copy.c?rev=1310581&r1=1310580&r2=1310581&view=diff
> ==
> --- subversion/branches/ev2-export/subversion/libsvn_client/copy.c (original)
> +++ subversion/branches/ev2-export/subversion/libsvn_client/copy.c Fri Apr 6
> 20:48:32 2012
> @@ -532,7 +532,6 @@ typedef struct path_driver_info_t
> svn_node_kind_t src_kind;
> svn_revnum_t src_revnum;
> svn_boolean_t resurrection;
> - svn_boolean_t dir_add;
> svn_string_t *mergeinfo; /* the new mergeinfo for the target */
> } path_driver_info_t;
>
> @@ -627,19 +626,9 @@ drive_single_path(svn_editor_t *editor,
> apr_pool_t *scratch_pool)
> {
> apr_hash_t *props = apr_hash_make(scratch_pool);
> - apr_array_header_t *children = apr_array_make(scratch_pool, 1,
> - sizeof(const char *));
> svn_boolean_t do_delete = FALSE;
> svn_boolean_t do_add = FALSE;
>
> - /* Check to see if we need to add the path as a directory. */
> - if (path_info->dir_add)
> - {
> - return svn_error_trace(svn_editor_add_directory(editor, path, children,
> - props,
> - SVN_INVALID_REVNUM));
> - }
> -
> /* If this is a resurrection, we know the source and dest paths are
> the same, and that our driver will only be calling us once. */
> if (path_info->resurrection)
> @@ -702,6 +691,7 @@ static svn_error_t *
> drive_editor(svn_editor_t *editor,
> apr_array_header_t *paths,
> apr_hash_t *action_hash,
> + apr_array_header_t *new_dirs,
> svn_boolean_t is_move,
> svn_revnum_t youngest,
> apr_pool_t *scratch_pool)
> @@ -710,6 +700,23 @@ drive_editor(svn_editor_t *editor,
> apr_pool_t *iterpool;
> int i;
>
> + if (new_dirs)
> + {
> + apr_array_header_t *children = apr_array_make(scratch_pool, 1,
> + sizeof(const char *));
> + apr_hash_t *props = apr_hash_make(scratch_pool);
> +
> + /* These are directories which we just need to add. */
> + for (i = 0; i < new_dirs->nelts; i++)
> + {
> + const char *dir = APR_ARRAY_IDX(new_dirs, i, const char *);
> +
> + /* ### The children param needs to be populated correctly. */
> + SVN_ERR(svn_editor_add_directory(editor, dir, children, props,
> + SVN_INVALID_REVNUM));
Greg,
Just so you know, I would have expected this section to crash Ev2 in
copy test 67, since there are several directories being added, but the
children array is (currently) empty. I wonder if there is a problem
with the internal Ev2 state checking.
Also, this currently feeds paths to Ev2 child-first. For instance,
copy test 67 calls this with 'X/Y/Z', 'X/Y', and 'X', in that order.
The shims reorder things so that it gets done properly, but I wonder
if there shouldn't be some state checks in Ev2 to ensure parents are
added before children.
-Hyrum
> + }
> + }
> +
> iterpool = svn_pool_create(scratch_pool);
> for (i = 0; i < paths->nelts; i++)
> {
> @@ -1071,24 +1078,7 @@ repos_to_repos_copy(const apr_array_head
> else
> message = "";
>
> - /* Setup our PATHS for the path-based editor drive. */
> - /* First any intermediate directories. */
> - if (make_parents)
> - {
> - for (i = 0; i < new_dirs->nelts; i++)
> - {
> - const char *relpath = APR_ARRAY_IDX(new_dirs, i, const char *);
> - path_driver_info_t *info = apr_pcalloc(pool, sizeof(*info));
> -
> - info->dst_path = relpath;
> - info->dir_add = TRUE;
> -
> - APR_ARRAY_PUSH(paths, const char *) = relpath;
> - apr_hash_set(action_hash, relpath, APR_HASH_KEY_STRING, info);
> - }
> - }
> -
> - /* Then our copy destinations and move sources (if any). */
> + /* Setup our copy destinations and move sources (if any). */
> for (i = 0; i < path_inf
Re: buildbot failure in ASF Buildbot on svn-x64-ubuntu-gcc
On Fri, Apr 6, 2012 at 11:02 AM, Hyrum K Wright wrote: > *sigh* > > I spoke in haste. It turns out my ruby-fu is so weak (and the code is > question is convoluted by the plethora of yield statements) that I > feel uncomfortable digging into this in any reasonable timeframe. I > *think* Daniel's patch would be useful, but I'm really in no position > to say. > > http://svn.haxx.se/dev/archive-2012-03/0503.shtml > > -Hyrum > > On Thu, Apr 5, 2012 at 3:15 PM, Hyrum K Wright > wrote: >> Is that the final plan? If so, I'll *learn* enough Ruby to implement >> it, just to get the bots quiet again. :P >> >> -Hyrum >> >> On Thu, Apr 5, 2012 at 3:10 PM, Philip Martin >> wrote: >>> We discussed it last month: >>> >>> http://svn.haxx.se/dev/archive-2012-03/0669.shtml >>> >>> Converting the lists to sets in the testsuite seems to be the preferred >>> option, but I don't know enough Ruby to implement it. >>> >>> Greg Stein writes: >>> This is an ordering failure in the Ruby bindings, presumably due to APR hash order. Is anybody working on fixing the Ruby bindings to be order-independent for cases like this? Cheers, -g On Thu, Apr 5, 2012 at 15:00, wrote: > The Buildbot has detected a new failure on builder svn-x64-ubuntu-gcc > while building ASF Buildbot. > Full details are available at: > http://ci.apache.org/builders/svn-x64-ubuntu-gcc/builds/4748 > > Buildbot URL: http://ci.apache.org/ > > Buildslave for this Build: svn-x64-ubuntu > > Build Reason: scheduler > Build Source Stamp: [branch subversion/trunk] 1309995 > Blamelist: hwright,mattiase > > BUILD FAILED: failed Test bindings > > sincerely, > -The Buildbot > > > >>> >>> -- >>> uberSVN: Apache Subversion Made Easy >>> http://www.uberSVN.com >> >> >> >> -- >> >> uberSVN: Apache Subversion Made Easy >> http://www.uberSVN.com/ > > > > -- > > uberSVN: Apache Subversion Made Easy > http://www.uberSVN.com/ Sorry I'm so slow. I think I've fixed the client failures and I'm looking at the wc failure now -- Joe
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 04/06/2012 01:44 PM, Justin Erenkrantz wrote: > On Fri, Apr 6, 2012 at 8:09 AM, Greg Hudson wrote: >> I also want to caution that PBKDF2 does not provide strong protection >> against offline dictionary attacks. Most cryptographic methods provide >> exponential protection--I do a little bit more work to make you do twice >> as much work. PBKDF2 provides only linear protection--I do twice as >> much work to make you do twice as much work. It does not make >> dictionary attacks "impossible" in the same sense that AES-128 makes >> decryption without knowing the key "impossible". > > Is it worth looking at scrypt[1] instead of PBKDF2? -- justin Possibly. It depends on whether you care about things like NIST review (PBKDF2 is recommended in NIST SP 800-132) versus the theoretical advantages of a less heavily scrutinized algorithm. That's always a tough choice. The fundamental nature of scrypt isn't different from the fundamental nature of PBKDF2; both seek to add a fixed multiplier to the cost of both the legitimate user and the attacker. scrypt is designed to make it more difficult to use massively parallel hardware to mount the attack, by requiring more memory (if I skimmed the paper correctly).
Re: APR hash order ruby test failure
On Wed, Mar 28, 2012 at 2:30 AM, Philip Martin
wrote:
> Philip Martin writes:
>
>> There is another failure in the ruby testsuite:
>>
>> http://ci.apache.org/builders/svn-x64-ubuntu-gcc/builds/4626
>>
>> 1) Failure:
>> test_changelists_get_with_block(SvnClientTest)
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_client.rb:2296:in
>> `assert_changelists'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/util.rb:204:in
>> `make_context'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_client.rb:2288:in
>> `assert_changelists'
>> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_client.rb:2349:in
>> `test_changelists_get_with_block':
>> <{nil=>
>> ["/tmp/d20120322-8616-qtl2ah/wc",
>> "/tmp/d20120322-8616-qtl2ah/wc/hello1.txt",
>> "/tmp/d20120322-8616-qtl2ah/wc/hello2.txt"]}> expected but was
>> <{nil=>
>> ["/tmp/d20120322-8616-qtl2ah/wc",
>> "/tmp/d20120322-8616-qtl2ah/wc/hello2.txt",
>> "/tmp/d20120322-8616-qtl2ah/wc/hello1.txt"]}>.
>
> Another failure that looks like a similar problem in a different place:
>
> 1) Failure:
> test_diff_callbacks_for_backward_compatibility(SvnWcTest)
> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_wc.rb:756:in
> `assert_diff_callbacks'
> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/svn/wc.rb:136:in
> `_open'
> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/svn/wc.rb:114:in
> `open'
> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_wc.rb:711:in
> `assert_diff_callbacks'
> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/svn/ra.rb:52:in
> `open'
> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_wc.rb:699:in
> `assert_diff_callbacks'
> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/util.rb:204:in
> `make_context'
> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_wc.rb:696:in
> `assert_diff_callbacks'
> /var/lib/buildbot/svn-buildslave/svn-x64-ubuntu/build/subversion/bindings/swig/ruby/test/test_wc.rb:764:in
> `test_diff_callbacks_for_backward_compatibility':
> <[[:dir_props_changed,
> "/tmp/d20120327-16259-17t4nz8/wc",
> [# @name="svn:entry:committed-date",
> @value=nil>,
> # @name="svn:entry:committed-rev",
> @value=nil>,
> #,
> #]],
> [:file_changed,
> "/tmp/d20120327-16259-17t4nz8/wc/dir/hello.txt",
> [#]],
> [:file_added, "/tmp/d20120327-16259-17t4nz8/wc/dir/hello2.txt", []]]>
> expected but was
> <[[:dir_props_changed,
> "/tmp/d20120327-16259-17t4nz8/wc",
> [# @name="svn:entry:committed-date",
> @value=nil>,
> # @name="svn:entry:committed-rev",
> @value=nil>,
> #,
> #]],
> [:file_added, "/tmp/d20120327-16259-17t4nz8/wc/dir/hello2.txt", []],
> [:file_changed,
> "/tmp/d20120327-16259-17t4nz8/wc/dir/hello.txt",
> [#]]]>.
>
> We fix these by converting the arrays to sets so that the order is
> irrelevant. The open question is whether we do this solely within the
> testsuite, changing both sides of the comparison, or whether we change
> the bindings to return a set and adjust the testsuite to match.
>
I think I've addressed the first failure in r1310535. I will continue
looking into the wc failure.
--
Joe
Re: Ev2 RA protocols
On Fri, Apr 6, 2012 at 1:10 PM, Greg Stein wrote: > > On Apr 6, 2012 2:05 PM, "Hyrum K Wright" wrote: >> >> In case folks haven't been following commits@, there's been a bit of >> work on the ev2-export branch on implementing Ev2 drivers for commits. >> While we're not quite ready for it yet, at some point we'll need to >> start thinking about how to marshall Ev2 drives over the wire to >> waiting servers. As this design work is largely orthogonal to the >> current implementation efforts, and my familiarity with our existing >> protocols is pretty weak, I'd invite other folks to start thinking >> about these issues and discussing them here. > > Already ahead of you :-) > > I was going to suggest a new RA API rather than rev'ing get_commit_editor. > By going that route, the coding can occur on trunk, and we migrate as > appropriate, leaving the delta editor in place. Yep, that's the same conclusion I reached a few days ago. ;) > This would be the new "commit plan, followed by drive" design we discussed. > > The new RA entrypoint(s) could even have a default implementation in ra/ > that would use the original delta editor. Thus, we don't even have to > implement the new interface for all layers. This is a possibility. The current commit_editor4() on the ev2-export branch wraps the Ev1 implementation using the shims. -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: Ev2 RA protocols
On Apr 6, 2012 2:05 PM, "Hyrum K Wright" wrote: > > In case folks haven't been following commits@, there's been a bit of > work on the ev2-export branch on implementing Ev2 drivers for commits. > While we're not quite ready for it yet, at some point we'll need to > start thinking about how to marshall Ev2 drives over the wire to > waiting servers. As this design work is largely orthogonal to the > current implementation efforts, and my familiarity with our existing > protocols is pretty weak, I'd invite other folks to start thinking > about these issues and discussing them here. Already ahead of you :-) I was going to suggest a new RA API rather than rev'ing get_commit_editor. By going that route, the coding can occur on trunk, and we migrate as appropriate, leaving the delta editor in place. This would be the new "commit plan, followed by drive" design we discussed. The new RA entrypoint(s) could even have a default implementation in ra/ that would use the original delta editor. Thus, we don't even have to implement the new interface for all layers. Cheers, -g
Ev2 RA protocols
In case folks haven't been following commits@, there's been a bit of work on the ev2-export branch on implementing Ev2 drivers for commits. While we're not quite ready for it yet, at some point we'll need to start thinking about how to marshall Ev2 drives over the wire to waiting servers. As this design work is largely orthogonal to the current implementation efforts, and my familiarity with our existing protocols is pretty weak, I'd invite other folks to start thinking about these issues and discussing them here. (In a word: "Help!") -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: buildbot failure in ASF Buildbot on svn-x64-ubuntu-gcc
*sigh* I spoke in haste. It turns out my ruby-fu is so weak (and the code is question is convoluted by the plethora of yield statements) that I feel uncomfortable digging into this in any reasonable timeframe. I *think* Daniel's patch would be useful, but I'm really in no position to say. http://svn.haxx.se/dev/archive-2012-03/0503.shtml -Hyrum On Thu, Apr 5, 2012 at 3:15 PM, Hyrum K Wright wrote: > Is that the final plan? If so, I'll *learn* enough Ruby to implement > it, just to get the bots quiet again. :P > > -Hyrum > > On Thu, Apr 5, 2012 at 3:10 PM, Philip Martin > wrote: >> We discussed it last month: >> >> http://svn.haxx.se/dev/archive-2012-03/0669.shtml >> >> Converting the lists to sets in the testsuite seems to be the preferred >> option, but I don't know enough Ruby to implement it. >> >> Greg Stein writes: >> >>> This is an ordering failure in the Ruby bindings, presumably due to >>> APR hash order. >>> >>> Is anybody working on fixing the Ruby bindings to be order-independent >>> for cases like this? >>> >>> Cheers, >>> -g >>> >>> On Thu, Apr 5, 2012 at 15:00, wrote: The Buildbot has detected a new failure on builder svn-x64-ubuntu-gcc while building ASF Buildbot. Full details are available at: http://ci.apache.org/builders/svn-x64-ubuntu-gcc/builds/4748 Buildbot URL: http://ci.apache.org/ Buildslave for this Build: svn-x64-ubuntu Build Reason: scheduler Build Source Stamp: [branch subversion/trunk] 1309995 Blamelist: hwright,mattiase BUILD FAILED: failed Test bindings sincerely, -The Buildbot >>> >> >> -- >> uberSVN: Apache Subversion Made Easy >> http://www.uberSVN.com > > > > -- > > uberSVN: Apache Subversion Made Easy > http://www.uberSVN.com/ -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On Fri, Apr 6, 2012 at 8:09 AM, Greg Hudson wrote: > I also want to caution that PBKDF2 does not provide strong protection > against offline dictionary attacks. Most cryptographic methods provide > exponential protection--I do a little bit more work to make you do twice > as much work. PBKDF2 provides only linear protection--I do twice as > much work to make you do twice as much work. It does not make > dictionary attacks "impossible" in the same sense that AES-128 makes > decryption without knowing the key "impossible". Is it worth looking at scrypt[1] instead of PBKDF2? -- justin 1. http://www.tarsnap.com/scrypt.html
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 06.04.2012 17:07, C. Michael Pilato wrote: > On 04/06/2012 11:02 AM, Branko Čibej wrote: >>> *sigh* I hadn't considered stale, compromised data not yet purged or >>> overwritten. Does SQLite's VACUUM statement help with this problem? >>> http://sqlite.org/lang_vacuum.html >> Vacuum will reorder the pages in the file to fill holes, but will then >> truncate the database file without first overwriting it with random >> crud. So, no, that's not good enough. > Hrm. Given the statements that the "VACUUM command rebuilds the entire > database" and that the "VACUUM command works by copying the contents of the > database into a temporary database file and then overwriting the original > with the contents of the temporary file", I would have expected better. That's actually even worse, "overwriting the original" most likely means it does a FS-level rename, this leaving the /entire/ contents of the original file on disk. :) -- Brane
Re: Compressed Pristines (Summary)
On Wed, Apr 4, 2012 at 1:28 PM, Ashod Nakashian
wrote:
> I feel this is indeed what we're closing on, at least for an initial working
> demo. But I'd like to hear more agreements before committing to this path. I
> know some did show support for this approach, but it's hard to track them in
> the noise.
>
> So to make it easier, let's either voice support to this suggestion and
> commit to an implementation, or voice objection with at least reasons and
> possibly alternative action. Silence is passive agreement, so the onus on
> those opposing ;-)
I just read the Google doc - glad to see progress here - a few comments:
First off, if I understand correctly, I do have to say that I'm not at
all a fan of having a large pristine file spread out across multiple
on-disk compressed pack files. I don't think that makes a whole lot
of sense - I think it'd be simplest (when we hit the heuristic to put
it on-disk rather than in SQLite) to keep it to just one file. I
don't get why we'd want to have a big image pristine file (say a PSD
file) split out into say 20 smaller files on disk. Why? It just
seems we're going to introduce a lot of complexity for very little
return. The whole point of stashing the small files directly into
SQLite's pristine.db is to make the small files SQLite's problem and
not the on-disk FS (and reduce sub-block issues) - with that in place,
I think we're not going to need to throw multiple files into the same
pack file. It'll just get too confusing, IMO, to keep track of which
file offsets to use. (For a large file that already hits the size
trigger, we know that - worst case scenario - we might lose one FS
block. Yawn.) We can make the whole strategy simpler if we follow
that.
I'm with Greg in thinking that we don't need the pack index files -
but, I think I'll go further and reiterate that I think that there
should just be a 1:1 correspondence between the pack file and the
pristine file. What's the real advantage of having multiple large
pristines in one pack file (and that we constantly *append* to)? And,
with append FS ops with multiple files in one pack file, we rely on
our WC/FS locking strategy to be 100% perfect or we have a hosed pack
file. Ugh. I think it just adds unnecessary complexity. I think
it'll be far simpler to have 1:1 correspondence with a pack file to a
single large pristine. We'll have enough complexity already to just
find the small files sitting in SQLite rather than on-disk.
Given that 1:1 correspondence, I do think that the original file-size
and the complete checksum should be stored in the custom pack file
on-disk. It'll make it so that we could easily validate whether the
pack file is corrupt or not by using file-size (as a first-order
check) and checksum (as second-order). The thinking here is that if
the checksum is not in the file contents, but only in the file name
(or the pristine.db), the file system could very easily lose the
filename (hello ext3/4!) - this would allow us to verify the integrity
of the pack file and reassociate it if it gets dis-associated. This
is less of an issue with the client as it can always refetch - but, if
the server code ends up using the same on-disk format (as hinted in
the Google Doc)...then, I think this is important to have in the file
format from the beginning.
I definitely think that we should store the full 64-bit length
svn_filesize_t and not be cute and assume no one has a file larger
than 1TB.
I'll disagree with Greg for now and say that it's probably best to
just pack everything and not try to be cute about not packing certain
file types - I think that's a premature optimization right now. I
think the complexity of having a mixed pristine collection with some
files packed and some files unpacked is odd (and some files in SQLite
and some files on-disk). Maybe end up adding a no-op compression type
to the file format (IOW, tell gzip to do a no-op inflate via
Z_NO_COMPRESSION). Maybe. I just doubt it's worth the additional
complexity though. ("Is this pristine file compressed?" "I don't
know." Argh!) Making those assumptions based on file extensions or
even magic bits can be a bit awkward - case in point is PDF...some
times it'll compress well, some times it won't. So, best off just
always compressing it. =)
My $.02. -- justin
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 04/06/2012 10:55 AM, Greg Stein wrote: >> In other words, changing the master passphrase only requires decrypting >> and re-encrypting one 256-bit encryption key, not the whole credentials >> store. > PKBDF2 is in the current design to make dict attacks computationally > "impossible". Assuming we keep that, then the above value would be fed > in as the secret to PKBDF2, rather than MP or sha1(MP) ? If I understand you correctly, that wouldn't make sense. PBKDF2 is designed to provide some resistance against offline dictionary attacks against a weak secret, at the cost of computational power for legitimate users. If you have a strong secret, there's no point in running it through PBKDF2. Under the suggested architecture, you'd use PBKDF2(MP) to decrypt the master key, and then use the master key to decrypt the individual passwords. I also want to caution that PBKDF2 does not provide strong protection against offline dictionary attacks. Most cryptographic methods provide exponential protection--I do a little bit more work to make you do twice as much work. PBKDF2 provides only linear protection--I do twice as much work to make you do twice as much work. It does not make dictionary attacks "impossible" in the same sense that AES-128 makes decryption without knowing the key "impossible". If a system can be designed to prevent offline dictionary attacks entirely, that's much better. But for this application, that's probably impossible, since it's easy to distinguish a valid result (a password, which will be printable ASCII) from garbage.
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 04/06/2012 11:02 AM, Branko Čibej wrote: >> *sigh* I hadn't considered stale, compromised data not yet purged or >> overwritten. Does SQLite's VACUUM statement help with this problem? >> http://sqlite.org/lang_vacuum.html > > Vacuum will reorder the pages in the file to fill holes, but will then > truncate the database file without first overwriting it with random > crud. So, no, that's not good enough. Hrm. Given the statements that the "VACUUM command rebuilds the entire database" and that the "VACUUM command works by copying the contents of the database into a temporary database file and then overwriting the original with the contents of the temporary file", I would have expected better. Are you sure about this? (And note that I'm not talking about the auto_vacuum feature, which apparently works differently.) -- C. Michael Pilato CollabNet <> www.collab.net <> Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 06.04.2012 16:55, Greg Stein wrote: > On Apr 6, 2012 2:06 AM, "Branko Čibej" wrote: >> On 06.04.2012 00:38, C. Michael Pilato wrote: >>> I've been also frustrated when considering the situation that occurs > when a >>> user changes his/her master password, forcing a re-encryption of all > cached >>> credentials using the new password. >> You could do what whole-disk encryption systems do: only the encyprtion >> key is encrypted by the master passphrase, actual data are encrypted by >> that key. This allows different users with different passphrases to >> decrypt the same data, since they only decrypt a wrapped copy of the >> same encryption key. >> >> In other words, changing the master passphrase only requires decrypting >> and re-encrypting one 256-bit encryption key, not the whole credentials >> store. > PKBDF2 is in the current design to make dict attacks computationally > "impossible". Assuming we keep that, then the above value would be fed in > as the secret to PKBDF2, rather than MP or sha1(MP) ? That's the idea, yes. -- Brane
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 06.04.2012 16:13, C. Michael Pilato wrote: > On 04/06/2012 02:06 AM, Branko Čibej wrote: >> On 06.04.2012 00:38, C. Michael Pilato wrote: >>> I've been also frustrated when considering the situation that occurs when a >>> user changes his/her master password, forcing a re-encryption of all cached >>> credentials using the new password. >> You could do what whole-disk encryption systems do: only the encyprtion >> key is encrypted by the master passphrase, actual data are encrypted by >> that key. This allows different users with different passphrases to >> decrypt the same data, since they only decrypt a wrapped copy of the >> same encryption key. >> >> In other words, changing the master passphrase only requires decrypting >> and re-encrypting one 256-bit encryption key, not the whole credentials >> store. > That's a neat concept. I presume that the encryption key is just some > random data (since the user never needs to know it)? Yup. You want to generate that with a crypto-grade random generator, of coruse. Another bit that you do not want to reinvent. OpenSSL can do that for you, for example; any sane crypto package will have an API that generates good random bits for symmetric keys. >>> Is there anyone who is game for helping me tackle a new design for our >>> client-side authn cache using SQLite? >> This makes me wonder if we couldn't perhaps keep the whole thing as an >> in-memory-not-disk-backed SQLite database, then encrypt and dump the >> whole SQLite memory snapshot to disk. The real trouble with that >> approach is that debugging the database using the SQLite command-line >> tools would be impossible, everything would have to happen through the >> SVN API. >> >> (The point here is that, in this way, we could "easily" atomically >> re-encrypt everything with a new master passphrase, as opposed to doing >> it transactionally within an ordinary SQLite database -- because we >> don't have control over what SQLite does with the free space in the >> file, and it'd be really, really bad if snippets of data that had been >> encrypted by the old, presumably compromised, passphrase ended up >> sitting around on disk.) > *sigh* I hadn't considered stale, compromised data not yet purged or > overwritten. Does SQLite's VACUUM statement help with this problem? > http://sqlite.org/lang_vacuum.html Vacuum will reorder the pages in the file to fill holes, but will then truncate the database file without first overwriting it with random crud. So, no, that's not good enough. -- Brane
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 04/06/2012 10:47 AM, Greg Stein wrote: > Correct. Still useful, but even if memory is compromised, the SHA1 is > not reversible. The original MP cannot be recovered for other uses. Just as a reminder, SHA-1 is not recommended for use in new applications at this time (http://csrc.nist.gov/groups/ST/hash/policy.html).
Re: svn commit: r1308372 - /subversion/trunk/subversion/libsvn_subr/crypto.c
On Apr 6, 2012 10:45 AM, "C. Michael Pilato" wrote: > > On 04/02/2012 07:52 PM, Greg Stein wrote: >... > > To be honest, one of my intended updates is to move *all* of the > > #ifdef stuff into crypto.c's functions. We've had problems where > > functions only appeared within certain build targets (notably under > > Windows). I think that the functions should always be available, but > > just return errors if the underlying library support is not present. > > Thus, callers never need to understand the vagaries of when something > > is/not available. Throw in an svn_crypto__is_available(), and it makes > > things even a bit easier. > > > > Any issues with that approach, that you can see? > > It's not just the functions that have to deal with this. It's also the data > types. Of course, that's all the more reason why your introduction of > svn_crypto__ctx_t is a Good Thing -- we can #ifdef the > conditionally-available APR members (leaving an empty-but-defined structure, > if need be). Right. The extra indirection of the typedef was to support the conditional implementation, in addition to potentially holding an RNG state. Note that structs need at least one element; "int empty;" works just fine. Cheers, -g
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On Apr 6, 2012 2:06 AM, "Branko Čibej" wrote: > > On 06.04.2012 00:38, C. Michael Pilato wrote: > > I've been also frustrated when considering the situation that occurs when a > > user changes his/her master password, forcing a re-encryption of all cached > > credentials using the new password. > > You could do what whole-disk encryption systems do: only the encyprtion > key is encrypted by the master passphrase, actual data are encrypted by > that key. This allows different users with different passphrases to > decrypt the same data, since they only decrypt a wrapped copy of the > same encryption key. > > In other words, changing the master passphrase only requires decrypting > and re-encrypting one 256-bit encryption key, not the whole credentials > store. PKBDF2 is in the current design to make dict attacks computationally "impossible". Assuming we keep that, then the above value would be fed in as the secret to PKBDF2, rather than MP or sha1(MP) ? Cheers, -g
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On Apr 6, 2012 10:36 AM, "C. Michael Pilato" wrote: > > On 04/06/2012 04:22 AM, Greg Stein wrote: > > Yeah, I switched the master passphrase param to an svn_string_t on the > > probable outcome that we would immediately SHA1 the thing, and then use the > > resulting hash as the nominal password. That would avoid having the > > plaintext in memory (and yes, I recognize it is quite possible that other > > copies exist; gotta start somewhere, and provide a data flow that avoids the > > requirement of plaintext). > > To be clear, Greg, you're talking about something a little bit than Brane's > "whole-disk encryption via encrypted keys" approach, right? IIUC, you're > saying that we'll simply SHA1 the user-provided password plaintext master > passphrase for the purpose of not holding that passphrase in memory. It's > sha1(MP), then, that is the secret in our encryption/decryption steps. > > I'm not quite sure how this helps with the situation Brane has raised -- > we'll still be holding the actual encryption secret in memory, it just now > looks less like a human-readable passphrase. But maybe that's the critical > difference? Correct. Still useful, but even if memory is compromised, the SHA1 is not reversible. The original MP cannot be recovered for other uses. Cheers, -g
Re: svn commit: r1308372 - /subversion/trunk/subversion/libsvn_subr/crypto.c
On 04/02/2012 07:52 PM, Greg Stein wrote: > On Mon, Apr 2, 2012 at 10:57, wrote: >> Author: cmpilato >> Date: Mon Apr 2 14:57:14 2012 >> New Revision: 1308372 >> >> URL: http://svn.apache.org/viewvc?rev=1308372&view=rev >> Log: >> Some cleanups and minor tweaks to the crypto code. >> >> * subversion/libsvn_subr/crypto.c >> (crypto_init): Remove unnecessary #if-wrapping (APU_HAVE_CRYPTO). >>While here, re-wrap (in a different sense of the word) some code. > > To be honest, one of my intended updates is to move *all* of the > #ifdef stuff into crypto.c's functions. We've had problems where > functions only appeared within certain build targets (notably under > Windows). I think that the functions should always be available, but > just return errors if the underlying library support is not present. > Thus, callers never need to understand the vagaries of when something > is/not available. Throw in an svn_crypto__is_available(), and it makes > things even a bit easier. > > Any issues with that approach, that you can see? It's not just the functions that have to deal with this. It's also the data types. Of course, that's all the more reason why your introduction of svn_crypto__ctx_t is a Good Thing -- we can #ifdef the conditionally-available APR members (leaving an empty-but-defined structure, if need be). I'll see if I can't make this change now. -- C. Michael Pilato CollabNet <> www.collab.net <> Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 04/06/2012 04:22 AM, Greg Stein wrote: > Yeah, I switched the master passphrase param to an svn_string_t on the > probable outcome that we would immediately SHA1 the thing, and then use the > resulting hash as the nominal password. That would avoid having the > plaintext in memory (and yes, I recognize it is quite possible that other > copies exist; gotta start somewhere, and provide a data flow that avoids the > requirement of plaintext). To be clear, Greg, you're talking about something a little bit than Brane's "whole-disk encryption via encrypted keys" approach, right? IIUC, you're saying that we'll simply SHA1 the user-provided password plaintext master passphrase for the purpose of not holding that passphrase in memory. It's sha1(MP), then, that is the secret in our encryption/decryption steps. I'm not quite sure how this helps with the situation Brane has raised -- we'll still be holding the actual encryption secret in memory, it just now looks less like a human-readable passphrase. But maybe that's the critical difference? -- C. Michael Pilato CollabNet <> www.collab.net <> Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: svn commit: r1310005 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
On Fri, Apr 6, 2012 at 8:53 AM, Greg Stein wrote:
>
> On Apr 6, 2012 8:56 AM, "Hyrum K Wright" wrote:
>>
>> On Thu, Apr 5, 2012 at 9:48 PM, Greg Stein wrote:
>> >
>> > On Apr 5, 2012 2:43 PM, wrote:
>> >>
>> >> Author: hwright
>> >> Date: Thu Apr 5 18:43:20 2012
>> >> New Revision: 1310005
>> >>
>> >> URL: http://svn.apache.org/viewvc?rev=1310005&view=rev
>> >> Log:
>> >> On the ev2-export branch:
>> >> Use an Ev2-style driver to handle repos->repos copies.
>> >>
>> >> Part of this commit is rather bogus, namely the bit where we still do a
>> >> delete+add. This *should* be a move, and will be adjusted to such in a
>> >> future commit, but the fact that it doesn't cause any tests to fail as
>> >> currently implemented is somewhat strange (the Ev2 internal checks
>> >> should
>> >> catch this, methinks).
>> >
>> > It can't.
>> >
>> > $ svn rm file
>> > $ svn copy file@REV newpath
>> >
>> > Totally fine sequence of operations. The user *could* have moved it, but
>> > that's a different story...
>>
>> Those aren't repos->repos operations. This chuck of code is only for
>> repos->repos copy/move.
>
> Sure, but I don't see how Ev2 can possibly know that, and throw an error
> "should have used move".
Ah. I got my wires crossed and thought you were referring to the
comment in the code ("we should use 'move' here") and not the comment
in the log message ("Ev2 should detect this problem"). You are right,
Ev2 can't detect this, but the caller should still use the move action
to accomplish it.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 04/06/2012 02:06 AM, Branko Čibej wrote: > On 06.04.2012 00:38, C. Michael Pilato wrote: >> I've been also frustrated when considering the situation that occurs when a >> user changes his/her master password, forcing a re-encryption of all cached >> credentials using the new password. > > You could do what whole-disk encryption systems do: only the encyprtion > key is encrypted by the master passphrase, actual data are encrypted by > that key. This allows different users with different passphrases to > decrypt the same data, since they only decrypt a wrapped copy of the > same encryption key. > > In other words, changing the master passphrase only requires decrypting > and re-encrypting one 256-bit encryption key, not the whole credentials > store. That's a neat concept. I presume that the encryption key is just some random data (since the user never needs to know it)? >> Is there anyone who is game for helping me tackle a new design for our >> client-side authn cache using SQLite? > > This makes me wonder if we couldn't perhaps keep the whole thing as an > in-memory-not-disk-backed SQLite database, then encrypt and dump the > whole SQLite memory snapshot to disk. The real trouble with that > approach is that debugging the database using the SQLite command-line > tools would be impossible, everything would have to happen through the > SVN API. > > (The point here is that, in this way, we could "easily" atomically > re-encrypt everything with a new master passphrase, as opposed to doing > it transactionally within an ordinary SQLite database -- because we > don't have control over what SQLite does with the free space in the > file, and it'd be really, really bad if snippets of data that had been > encrypted by the old, presumably compromised, passphrase ended up > sitting around on disk.) *sigh* I hadn't considered stale, compromised data not yet purged or overwritten. Does SQLite's VACUUM statement help with this problem? http://sqlite.org/lang_vacuum.html -- C. Michael Pilato CollabNet <> www.collab.net <> Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 04/05/2012 10:33 PM, Greg Stein wrote: >> If not, any suggestions on where the master passphrase fetch/store >> bits might best fit in? > > A new callback. But you definitely need a DSO option so core svn does not > have GNOME/KDE dependencies. Instead, they load a small DSO that implements > the master get/set functionality. Maybe a tiny vtable. > > I think the OS-based ones are not DSO since there is no heavy dep chain to > be concerned about. > > Dunno where GPG comes in. Is there a library and heavy deps associated with > that? You are correct. Today we have DSO options for GNOME/KDE, and simple #if-wrapping for Win32 and MacOS. GPG Agent doesn't have the lib/heavy deps, as the code communicates with the agent not through a custom API, but directly via socket I/O. Not sure what you're envisioning when you say "a new callback". >> I mean, do third-party clients really need to pick and choose which >> providers they want to use? > > Not the types of auth, but the client needs a way to prompt. The client_ctx > prompt callback may be enough, but I dunno (does that support two inputs? > such as username and password). We have several different kinds of prompting callbacks offered by the various providers at this point, and I believe those are required. But I wonder if they can't all be lumped into one giant authn prompt callback vtable. What about other benefits of the existing system? * third-party authn providers can be written and used * authn providers can be ordered according to a client's desires Are there any known clients taking advantage of these features? -- C. Michael Pilato CollabNet <> www.collab.net <> Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: svn commit: r1310005 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
On Apr 6, 2012 8:56 AM, "Hyrum K Wright" wrote: > > On Thu, Apr 5, 2012 at 9:48 PM, Greg Stein wrote: > > > > On Apr 5, 2012 2:43 PM, wrote: > >> > >> Author: hwright > >> Date: Thu Apr 5 18:43:20 2012 > >> New Revision: 1310005 > >> > >> URL: http://svn.apache.org/viewvc?rev=1310005&view=rev > >> Log: > >> On the ev2-export branch: > >> Use an Ev2-style driver to handle repos->repos copies. > >> > >> Part of this commit is rather bogus, namely the bit where we still do a > >> delete+add. This *should* be a move, and will be adjusted to such in a > >> future commit, but the fact that it doesn't cause any tests to fail as > >> currently implemented is somewhat strange (the Ev2 internal checks should > >> catch this, methinks). > > > > It can't. > > > > $ svn rm file > > $ svn copy file@REV newpath > > > > Totally fine sequence of operations. The user *could* have moved it, but > > that's a different story... > > Those aren't repos->repos operations. This chuck of code is only for > repos->repos copy/move. Sure, but I don't see how Ev2 can possibly know that, and throw an error "should have used move". Cheers, -g
Re: svn commit: r1310005 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
On Thu, Apr 5, 2012 at 9:48 PM, Greg Stein wrote: > > On Apr 5, 2012 2:43 PM, wrote: >> >> Author: hwright >> Date: Thu Apr 5 18:43:20 2012 >> New Revision: 1310005 >> >> URL: http://svn.apache.org/viewvc?rev=1310005&view=rev >> Log: >> On the ev2-export branch: >> Use an Ev2-style driver to handle repos->repos copies. >> >> Part of this commit is rather bogus, namely the bit where we still do a >> delete+add. This *should* be a move, and will be adjusted to such in a >> future commit, but the fact that it doesn't cause any tests to fail as >> currently implemented is somewhat strange (the Ev2 internal checks should >> catch this, methinks). > > It can't. > > $ svn rm file > $ svn copy file@REV newpath > > Totally fine sequence of operations. The user *could* have moved it, but > that's a different story... Those aren't repos->repos operations. This chuck of code is only for repos->repos copy/move. -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On Apr 6, 2012 3:58 AM, "Branko Čibej" wrote: > > On 06.04.2012 09:51, Daniel Shahaf wrote: > > Branko Čibej wrote on Fri, Apr 06, 2012 at 08:06:32 +0200: > >> This makes me wonder if we couldn't perhaps keep the whole thing as an > >> in-memory-not-disk-backed SQLite database, then encrypt and dump the > >> whole SQLite memory snapshot to disk. The real trouble with that > >> approach is that debugging the database using the SQLite command-line > >> tools would be impossible, everything would have to happen through the > >> SVN API. > > Presumably we'd write a tools/dev/ helper that decrypts the memory > > snapshot and dumps it to an on-disk SQLite db? > > This really has other problems, too. Actually, /any/ passphrase-based > system we use has it: "in-memory" does not, by itself, imply that the > unencrypted data never end up on disk. At the very least, the > unencrypted bits need to be stored in locked, access-protected memory, > so that they don't get swapped out and can't be accessed by (non-root) > users. > > OS-provided password storage systems typically already account for this. > And, whilst Subversion doesn't take these precautions with individual > passwords, a passphrase that protects a number of different credentials > needs more attention to preventing plaintext leaks. Yeah, I switched the master passphrase param to an svn_string_t on the probable outcome that we would immediately SHA1 the thing, and then use the resulting hash as the nominal password. That would avoid having the plaintext in memory (and yes, I recognize it is quite possible that other copies exist; gotta start somewhere, and provide a data flow that avoids the requirement of plaintext). Cheers, -g
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
On 06.04.2012 09:51, Daniel Shahaf wrote: > Branko Čibej wrote on Fri, Apr 06, 2012 at 08:06:32 +0200: >> This makes me wonder if we couldn't perhaps keep the whole thing as an >> in-memory-not-disk-backed SQLite database, then encrypt and dump the >> whole SQLite memory snapshot to disk. The real trouble with that >> approach is that debugging the database using the SQLite command-line >> tools would be impossible, everything would have to happen through the >> SVN API. > Presumably we'd write a tools/dev/ helper that decrypts the memory > snapshot and dumps it to an on-disk SQLite db? This really has other problems, too. Actually, /any/ passphrase-based system we use has it: "in-memory" does not, by itself, imply that the unencrypted data never end up on disk. At the very least, the unencrypted bits need to be stored in locked, access-protected memory, so that they don't get swapped out and can't be accessed by (non-root) users. OS-provided password storage systems typically already account for this. And, whilst Subversion doesn't take these precautions with individual passwords, a passphrase that protects a number of different credentials needs more attention to preventing plaintext leaks. -- Brane
Re: Master passphrase approach, authn storage, cobwebs in C-Mike's head, ...
Branko Čibej wrote on Fri, Apr 06, 2012 at 08:06:32 +0200: > This makes me wonder if we couldn't perhaps keep the whole thing as an > in-memory-not-disk-backed SQLite database, then encrypt and dump the > whole SQLite memory snapshot to disk. The real trouble with that > approach is that debugging the database using the SQLite command-line > tools would be impossible, everything would have to happen through the > SVN API. Presumably we'd write a tools/dev/ helper that decrypts the memory snapshot and dumps it to an on-disk SQLite db?
