On 6 August 2014 09:49, Ben Reser <b...@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@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
Index: subversion/svn/auth-cmd.c
===================================================================
--- subversion/svn/auth-cmd.c (revision 1616447)
+++ subversion/svn/auth-cmd.c (working copy)
@@ -27,6 +27,8 @@
#include <apr_getopt.h>
#include <apr_fnmatch.h>
#include <apr_tables.h>
+#include <apr_md5.h>
+#include <apr_sha1.h>
#include "svn_private_config.h"
@@ -167,6 +169,34 @@
return SVN_NO_ERROR;
}
+static const char *
+format_cert_fingerprint(const svn_checksum_t *fingerprint,
+ apr_pool_t *pool)
+{
+ apr_size_t digest_size;
+ apr_size_t i;
+ svn_stringbuf_t *buf;
+ static const char *hex = "0123456789ABCDEF";
+
+ if (fingerprint->kind == svn_checksum_md5)
+ digest_size = APR_MD5_DIGESTSIZE;
+ else if(fingerprint->kind == svn_checksum_sha1)
+ digest_size = APR_SHA1_DIGESTSIZE;
+ else
+ return NULL;
+
+ buf = svn_stringbuf_create_ensure(digest_size * 3, pool);
+
+ for (i = 0; i < digest_size; i++)
+ {
+ svn_stringbuf_appendbyte(buf, hex[fingerprint->digest[i] >> 4]);
+ svn_stringbuf_appendbyte(buf, hex[fingerprint->digest[i] & 0x0f]);
+ svn_stringbuf_appendbyte(buf, ':');
+ }
+
+ return buf->data;
+}
+
static svn_error_t *
show_cert(const svn_string_t *pem_cert, apr_pool_t *scratch_pool)
{
@@ -193,10 +223,8 @@
SVN_ERR(svn_cmdline_printf(scratch_pool, _("Issuer: %s\n"),
svn_x509_certinfo_get_issuer(certinfo)));
SVN_ERR(svn_cmdline_printf(scratch_pool, _("Fingerprint: %s\n"),
- svn_checksum_to_cstring_display2(
+ format_cert_fingerprint(
svn_x509_certinfo_get_digest(certinfo),
- SVN_CHECKSUM_CSTRING_UPPER |
- SVN_CHECKSUM_CSTRING_COLONS,
scratch_pool)));
hostnames = svn_x509_certinfo_get_hostnames(certinfo);
