Re: [VOTE] Merge svn-auth-x509 branch to trunk?

On 6 August 2014 09:49, Ben Reser <ben_at_reser.org> wrote:
> I believe the svn-auth-x509 branch is ready to be merged to trunk. There is no
> BRANCH-README so I'll briefly explain the purpose of the branch.
>
> Currently on trunk we have the `svn auth` command that can list out the
> contents of the auth store. The auth store can include SSL server
> certificates. On trunk in order to display certificates we are writing out the
> details of the cert as separate keys in the auth storage. Many users will have
> certificates without these extra keys and will not get much value out of this
> feature.
>
> Prior to the current implementation there were several other implementations
> that used OpenSSL or Serf to retrieve the information from the certificate but
> these were deemed to be unacceptable.
>
> The purpose of the branch is to replace the dependency on some other code with
> our own X.509 parser. The code started with the parser from TropicSSL and has
> had functionality we did not need removed and has been made more robust in the
> areas we did need.
>
> There are 6 basic pieces to this branch.
>
> 1) The X.509 parser itself and the accessor functions to get at the data in the
> opaque struct that the parser returns. This is the code in the various files
> with x509 in the name. There are some new error codes as well in
> svn_error_codes.h.
>
> 2) New functions for handling converting from UCS-2, UCS-4 and ISO-8859-1 by
> way of utf8proc rather than needing iconv. These are in the various utf named
> files.
>
> 3) Removal of the code that adds the extra keys to the auth store. See the
> ssl_server_trust_providers.c file and svn_config.h.
>
> 4) Adjustments to JavaHL to reflect these changes. Confined to JavaHL files.
>
> 5) Updating the auth command to use the new functions and not the keys on
> trunk. Currently, this code will output extra output if you have the keys.
> This is confined to the auth-cmd.c file.
>
> 6) Our code to convert a checksum into a displayable string has been changed to
> allow optional formatting. This is primarily in the checksum and md5 files.
> But the fallout of this ends up being in most of the other remaining files not
> already mentioned by the above.
>
> You can get the diff with:
> svn diff ^/subversion/trunk_at_1616093 ^/subversion/branches/svn-auth-x509
>
> Per the decision in Berlin 2013, I'm asking for a vote to bring this branch
> into trunk. I believe we should merge this code before 1.9.x so that we can
> avoid the ugly extra keys in the auth files.

I like the branch idea in general.

Several comments on branch code itself:
1. Probably it makes sense to do not deprecate
svn_checksum_to_cstring_display() or have local x509 implementation
for fingerprint formatting because we use
svn_checksum_to_cstring_display() as canonical representation of
checksum in libsvn_fs/libsvn_repos. So I suggest revert r1616093 from
branch and commit attached patch.

This avoid a lot of unrelated changes. I'm ready to do this. Do you
have any objections?

2. There are several new compilation warnings on Windows using VS2013:
[[[
..\..\..\subversion\libsvn_subr\x509parse.c(101): warning C4018: '>' :
signed/unsigned mismatch
..\..\..\subversion\libsvn_subr\x509parse.c(939): warning C4018: '<' :
signed/unsigned mismatch
..\..\..\subversion\libsvn_subr\x509parse.c(946): warning C4389: '!='
: signed/unsigned mismatch
..\..\..\subversion\libsvn_subr\x509parse.c(1054): warning C4389: '!='
: signed/unsigned mismatch
]]]]

Beside of that I'm +1 to merge this branch to trunk.

-- 
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com