On 10 August 2014 18:35, Ben Reser <ben_at_reser.org> wrote:
> On 8/10/14 5:03 AM, Ivan Zhakov wrote:
>> I agree svn_checksum_to_cstring_display() is wrong name and the proper
>> solution to make separated (probably) optimized function for
>> converting checksum to canonical string representation. But this is
>> definitely out of scope of this branch. I've gone ahead, reverted
>> r1616093 from branch and implemented svn_x509_fingerprint_to_display()
>> for converting cert fingerprints to display string. This is pubic and
>> tested function so other Subversion clients could use it. Please let
>> me know you're not happy with my solution.
>
> Using svn_x509_fingerprint_to_display() on a svn_checksum_t makes no sense to
> me. Like I said in my last email I think we should be using the colon
> separators on some other places in our output that are entirely unrelated to
> the X.509 parser. It seems ridiculous to suggest that those places in the
> future should be calling an svn_x509 function to do this. I just really don't
> see what's wrong with what I did.
>
I agree that using svn_x509_fingerprint_to_display() on svn_checksum_t
is not ideal solution. My rationale was to create function that
formats checksum in the way fingerprints should be formatted. But your
points are also valid. I'm fine with both approach.
> Yes it creates some code churn, but I already did the work to deal with the
> deprecation points.
>
My primary concerns was that with svn_checksum_to_cstring_display2()
we have to care at every place to use proper flags to get some
canonical representation for protocol/storage.
> If the concern is about that the new function possibly outputs things wrong and
> we need tests then fine let's add some tests.
>
> If the concern is about performance then let's work on the performance.
>
No, I don't care about performance of course. My point about
performance was because svn_checksum_to_cstring_display() seems to be
originally optimized a bit, because it doesn't use svn_stringbuf_t
which is safer.
> But really it seems to me like we're just arguing about naming. If you just
> really want to leave the svn_checksum_to_cstring_display() alone then fine, but
> at least put whatever new public function you're making in svn_checksum_*.
>
Exactly. I want to leave svn_checksum_to_cstring_display() and do not
change all callers on the branch:
1. It's a little bit out of scope of the branch (many unrelated
changes, that should be reviewed that means less focus on x509
changes)
2. We currently use svn_checksum_to_cstring_display() as canonical
representation of checksum in many places. And replacing them with
calls to functions with flags is error prone since we have to care to
use the same flags.
Brane asked me to reverted my branch changes for some reason, while I
just wanted to help you:
http://svn.haxx.se/dev/archive-2014-08/0113.shtml
I've reverted my branch changes in r1617225. So I'm leaving solution
of svn_checksum_to_cstring_display() problem to you. Please let me
know when you are finished and I'll review branch again.
I'm -1 on merging this branch until the
svn_checksum_to_cstring_display2() issue is resolved.
>> Btw I've noticed problem that svn auth ends with 'svn: E240018:
>> Certificate signature mismatch' error for one of certificate stored on
>> my laptop. There are two questions here:
>> 1. Why Subverison fails to parse this certificate? I could email this
>> certificate file privately if you're interested.
>
> There shouldn't be any such certificate that's valid (at least that's using the
> Internet profile for X.509). There are two places that the signature algorithm
> are specified in the certificate. First in the Certificate sequence and again
> in the TBSCertificate sequence. According to the X.509 RFC these MUST always
> be the same OID (see section 4.1.1.2 and 4.1.2.3 or RFC 5280).
>
> So yes I'd be interested in seeing the certificate.
>
I've sent certificate to you.
> If there really are such certificates we can loosen this check since it's not
> really important to how we're using the X.509 parser right now.
>
>> 2. May be 'svn auth' should print warning and continue on certificate
>> parsing error?
>
> Yes that should be the case.
>
Great, that would be great to have.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Received on 2014-08-11 11:00:49 CEST