You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Reser <be...@reser.org> on 2014/08/06 07:49:11 UTC
[VOTE] Merge svn-auth-x509 branch to trunk?
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.
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Aug 19, 2014 at 06:24:18PM +0100, Ben Reser wrote:
> On 8/8/14 1:14 AM, Ben Reser wrote:
> > On 8/5/14 10:49 PM, Ben Reser 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.
> >
> > Cleaned up several warnings based on feedback from Ivan and things I found
> > while looking at that as of r1616643.
>
> FYI I'm cleaning up the branch some more based on the discussion that we've had
> at the SHF hackathon. So let's consider this vote to be closed and I'll open a
> new one here in the next few days.
>
> Thanks for the feedback so far.
Any news on this topic?
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/8/14 1:14 AM, Ben Reser wrote:
> On 8/5/14 10:49 PM, Ben Reser 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.
>
> Cleaned up several warnings based on feedback from Ivan and things I found
> while looking at that as of r1616643.
FYI I'm cleaning up the branch some more based on the discussion that we've had
at the SHF hackathon. So let's consider this vote to be closed and I'll open a
new one here in the next few days.
Thanks for the feedback so far.
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/5/14 10:49 PM, Ben Reser 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.
Cleaned up several warnings based on feedback from Ivan and things I found
while looking at that as of r1616643.
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Branko Čibej <br...@wandisco.com>.
On 06.08.2014 07:49, Ben Reser wrote:
> 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.
Fully agree, and +1, but I'll declare that vote non-binding because I
helped with some minor changes on the branch.
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/7/14 4:34 PM, Ben Reser wrote:
> I expected that change would just shift the warnings around but it removes the
> warnings entirely. I'm still not sure it's right. We're setting a ptrdiff_t
> with the value from an apr_size_t. Shouldn't that result in a possible loss of
> data since we're setting a signed value (that should be the same size) with an
> unsigned value? At least I was under the impression that ptrdiff_t and size_t
> were both 32-bits on 32-bit platforms and 64-bits on 64-bit platforms.
>
> The casts seem safer to me because I know that the cast is safe because the
> difference from the two pointers can never be bigger than an apr_size_t because
> our buffer size is provided via an apr_size_t.
Ignore the attachment to the previous mail. I was going to attach the patch
but decided to test the fix on Windows myself. The attached patch was missing
the stddef.h include.
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Branko Čibej <br...@wandisco.com>.
On 08.08.2014 03:43, Ben Reser wrote:
> On 8/7/14 5:58 PM, Branko Čibej wrote:
>> I've seen platforms where size_t was smaller than ptrdiff_t; but usually
>> they're the same size. The rules of type promotion in C state that an a value
>> of a signed type can be promoted to a value of the same-sized unsigned type
>> without truncation, whereas the opposite is not true. That's why you don't get
>> warnings here on most usual platforms. But the unusual platforms where size_t
>> is smaller than ptrdiff_t could be a problem.
> I'm not going signed -> unsigned. I'm going unsigned -> signed (specifically
> apr_size_t to ptrdiff_t).
>
> Specifically:
> [[[
> svn_error_t *
> svn_x509_parse_cert(svn_x509_certinfo_t **certinfo,
> const char *buf,
> apr_size_t buflen,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)
> {
> svn_error_t *err;
> ptrdiff_t len;
> const unsigned char *p;
> const unsigned char *end;
> x509_cert *crt;
> svn_x509_certinfo_t *ci;
> svn_stringbuf_t *namebuf;
>
> crt = apr_pcalloc(scratch_pool, sizeof(*crt));
> p = (const unsigned char *)buf;
> len = buflen;
> end = p + len;
> ]]]
>
> Note the next to last line where I assign the ptrdiff_t len with the value from
> the apr_size_t buflen.
>
> Unless I'm missing something that ought to be producing a warning should it not?
No, why? C compilers typically do not warn about possible overflow in
arithmetic operations, and in this case there is no loss of
representation if size_t and ptrdiff_t are the same size. Regardless of
their actual sizes, a ptrdiff_t is guaranteed to be able to represent
all the bits of a size_t, because MAX(size_t) is the architecture's
limit for in-memory object sizes, and ptrdiff_t is required to always be
able to represent the distance between two pointers within the same
in-memory object.
Furthermore, any half-sane compiler knows that the value of 'len' cannot
be larger than size_t represents, in your case, even if ptrdiff_t is
larger than size_t.
I suspect that's way more standardese than is good for the digestion at
one sitting.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/7/14 5:58 PM, Branko Čibej wrote:
> I've seen platforms where size_t was smaller than ptrdiff_t; but usually
> they're the same size. The rules of type promotion in C state that an a value
> of a signed type can be promoted to a value of the same-sized unsigned type
> without truncation, whereas the opposite is not true. That's why you don't get
> warnings here on most usual platforms. But the unusual platforms where size_t
> is smaller than ptrdiff_t could be a problem.
I'm not going signed -> unsigned. I'm going unsigned -> signed (specifically
apr_size_t to ptrdiff_t).
Specifically:
[[[
svn_error_t *
svn_x509_parse_cert(svn_x509_certinfo_t **certinfo,
const char *buf,
apr_size_t buflen,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
svn_error_t *err;
ptrdiff_t len;
const unsigned char *p;
const unsigned char *end;
x509_cert *crt;
svn_x509_certinfo_t *ci;
svn_stringbuf_t *namebuf;
crt = apr_pcalloc(scratch_pool, sizeof(*crt));
p = (const unsigned char *)buf;
len = buflen;
end = p + len;
]]]
Note the next to last line where I assign the ptrdiff_t len with the value from
the apr_size_t buflen.
Unless I'm missing something that ought to be producing a warning should it not?
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Branko Čibej <br...@wandisco.com>.
On 08.08.2014 01:34, Ben Reser wrote:
> On 8/7/14 12:16 PM, Branko Čibej wrote:
>> On 07.08.2014 19:03, Ben Reser wrote:
>>> This appears to be because pointers are unsigned and apr_size_t is signed.
>> You mean the other way around, surely.
> Yes, thinko.
>
>>> Guess we can just cast the pointer arithmetic to apr_size_t. So they end up
>>> looking like this respectively:
>>>
>>> if (*len > (apr_size_t)(end - *p))
>>> if (len != (apr_size_t)(end - p))
>> There's a reason why ptrdiff_t exists. Maybe we should use it? I'm not fond of
>> adding casts to code to silence warnings that could be fixed by using the
>> correct type throughout.
> I expected that change would just shift the warnings around but it removes the
> warnings entirely. I'm still not sure it's right. We're setting a ptrdiff_t
> with the value from an apr_size_t. Shouldn't that result in a possible loss of
> data since we're setting a signed value (that should be the same size) with an
> unsigned value? At least I was under the impression that ptrdiff_t and size_t
> were both 32-bits on 32-bit platforms and 64-bits on 64-bit platforms.
I've seen platforms where size_t was smaller than ptrdiff_t; but usually
they're the same size. The rules of type promotion in C state that an a
value of a signed type can be promoted to a value of the same-sized
unsigned type without truncation, whereas the opposite is not true.
That's why you don't get warnings here on most usual platforms. But the
unusual platforms where size_t is smaller than ptrdiff_t could be a problem.
There's another option, and that's to just use int for sizes. The point
is that the distance between two pointers is a signed value, and int is
guaranteed to be the same size or smaller than size_t.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/7/14 12:16 PM, Branko Čibej wrote:
> On 07.08.2014 19:03, Ben Reser wrote:
>> This appears to be because pointers are unsigned and apr_size_t is signed.
>
> You mean the other way around, surely.
Yes, thinko.
>> Guess we can just cast the pointer arithmetic to apr_size_t. So they end up
>> looking like this respectively:
>>
>> if (*len > (apr_size_t)(end - *p))
>> if (len != (apr_size_t)(end - p))
>
> There's a reason why ptrdiff_t exists. Maybe we should use it? I'm not fond of
> adding casts to code to silence warnings that could be fixed by using the
> correct type throughout.
I expected that change would just shift the warnings around but it removes the
warnings entirely. I'm still not sure it's right. We're setting a ptrdiff_t
with the value from an apr_size_t. Shouldn't that result in a possible loss of
data since we're setting a signed value (that should be the same size) with an
unsigned value? At least I was under the impression that ptrdiff_t and size_t
were both 32-bits on 32-bit platforms and 64-bits on 64-bit platforms.
The casts seem safer to me because I know that the cast is safe because the
difference from the two pointers can never be bigger than an apr_size_t because
our buffer size is provided via an apr_size_t.
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Branko Čibej <br...@wandisco.com>.
On 07.08.2014 19:03, Ben Reser wrote:
> This appears to be because pointers are unsigned and apr_size_t is signed.
You mean the other way around, surely.
> Guess we can just cast the pointer arithmetic to apr_size_t. So they end up
> looking like this respectively:
>
> if (*len > (apr_size_t)(end - *p))
> if (len != (apr_size_t)(end - p))
There's a reason why ptrdiff_t exists. Maybe we should use it? I'm not
fond of adding casts to code to silence warnings that could be fixed by
using the correct type throughout.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/10/14 7:35 AM, Ben Reser wrote:
> 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.
>
> 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.
Ivan sent me the certificate. This appears to be a bug in the X.509 parser.
Haven't worked out what yet though.
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/12/14 4:39 PM, Branko Čibej wrote:
> This thing against flags and booleans that change API behaviour does have a
> very good argument going for it: it makes the code less obvious for the sake of
> saving a few function names.
>
> Consider the standard example, which (these days) would be an API to a DOM-like
> object that can be displayed or not; to query its state, you'd have a method called
>
> bool is_visible();
>
> To change the state, the "flaggy" design would be
>
> void set_visibility(bool);
>
> but the obviously more readable design uses two methods,
>
> void show();
> void hide();
>
> With the former, you always have to worry about what the boolean parameter
> actually means; with the latter, there can be no doubt.
>
> So in the case of displaying the contents of checksums, first of all something
> called
>
> svn_checksum_to_cstring_display
>
> is clearly a broken API, especially if it's used to format the checksum for the
> protocol, i.e., has nothing to do with displaying it at all. The docstring says
> nothing about displaying; and there's no difference between this function and
> svn_checksum_to_cstring, other than that the latter accepts (and possibly
> returns) a NULL, where the former does not.
Agree with you so far. Yes svn_checksum_to_cstring() accepts and returns NULL.
>
> Then we have svn_checksum_serialize and svn_checksum_deserialize, which I would
> expect to be always used for the protocol;
These are used by the wc indirectly by way of svn_sqlite__bind_checksum. The
key difference here is they include the type and the digest itself in the
output, allowing the svn_checksum_t value to be stored in the database and then
recreated from that, without assuming any particular type. This is probably
what we should have been doing for the protocol and the repository, but we
didn't and this is a relatively new (1.7) API. I'd suggest that FSX should
start using this but since it's probably going to be more binary there's
probably going to be more compact serialization used for it.
> and svn_checksum_parse_hex, where
> it's not clear if it's the inverse of svn_checksum_to_cstring, or the _display
> variant, or both, or WTF.
svn_checksum_parse_hex takes a hash type (which is usually inferred from where
the hex string is coming from) and a hex string and produces a svn_checksum_t,
it can be used to parse the output from either svn_checksum_to_cstring or
svn_checksum_to_cstring_display.
The difference between svn_checksum_to_cstring and
svn_checksum_to_cstring_display is how they handle a digest that is all zeros.
svn_checksum_to_cstring() returns NULL while svn_checksum_to_cstring_display
returns a string with the appropriate number of zeros in it. There are places
where one or the other variant is useful.
svn_checksum_parse_hex handles both NULL or a string with all zeros as the
digest the same.
> In other words, at least one of these APIs needs to be deprecated because it
> has a terrible name, and all of them need better documentation.
The documentation is sufficient to understand what's going on but it's not
entirely obvious until you read all of it. So yes we could improve this by
adding hints like that svn_checksum_parse_hex() can handle the output of either
svn_checksum_to_cstring() or svn_checksum_to_cstring_display().
> If we want a
> function that's intended specifically for printing fingerprints, then by all
> means call it
>
> svn_checksum_fingerprint
>
> although, really,
A fingerprint is just another name for a hash/checksum/digest. The problem
here is that there are several variations of how to display this data. It is
highly useful to try and display this data in the common format used for the
context. For instance, almost everything displays X.509 cert fingerprints as
SHA1 hashes with upper case hex digits with a colon between the characters for
each byte.
On the other hand you have software like GPG and PGP where the standard is to
use upper case hex digits with a space between every 2 bytes of the hash.
Then you have software like sha1sum, md5sum, etc... that just gives you a
stream of all lower case hex digits with no separators.
If you're wanting to manually verify that two match you can sit there and try
to read one and compare it to the other which is an error prone operation even
if you are using a format that has some sort of periodic separator. Or if you
happen to have both in the computer you can copy one or the other out and put
it in a editor and then search to see if the other one matches (or use the
search in your terminal). This of course only works if you have them formatted
the same.
So creating a function named svn_checksum_fingerprint() does absolutely nothing
to improve the situation. It's just as ambiguous as the current situation.
If we want to avoid flags and booleans then I guess we need to come up with
names for the styles like:
svn_checksum_fingerprint_x509style()
or
svn_checksum_fingerprint_uppercolons()
etc...
I'll try to come up with some final names, but can we at least agree that as
long as they're reasonably descriptive we don't need a week long thread to
agree to names?
> I don't know why such a thing would have to be in
> libsvn_subr, or perhaps in svn_checksum.h; I could imagine such a thing living
> in subr but exposed in svn_cmdline.h, called svn_cmdline_fingerprint or some such.
What does this have anything to do with the command line? The svn_cmdline.h
stuff is all things that either handle input or output to a terminal. The few
functions we have that handle character set encoding are there so they can
handle the detection of the output encoding (done in svn_cmdline_init()) and
use the correct encoding automagically.
I see no reason why this sort of formatting function shouldn't return UTF-8 and
then the command line tools use the svn_cmdline_* functions to output it.
Whatever formatting functions we provide for svn_checksum_t should live in
svn_checksum.h.
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Branko Čibej <br...@wandisco.com>.
On 12.08.2014 23:47, Ben Reser wrote:
> On 8/12/14 12:56 PM, Ivan Zhakov wrote:
>> My concerns are the following:
>> 1. Avoid unrelated branch changes
>> 2. Have some function for converting checksum to canonical form:
>> a) one option is just leave svn_checksum_to_cstring_display() and
>> add svn_checksum_to_cstring_display_ex()
>> b) another option is deprecate svn_checksum_to_cstring_display(),
>> replacing existing callers with
>> new svn_checksum_to_cstring_canonical(), but I think it's
>> better do this on trunk to make review easier.
> Per our discussion I'll just remove the svn_checksum_* changes on the branch.
> After we get the X.509 stuff merged to trunk I'll commit something that changes
> the checksum output formatting and that accommodates most of your comments. I
> am still inclined to use flags so that the function can be used in multiple
> differing cases.
This thing against flags and booleans that change API behaviour does
have a very good argument going for it: it makes the code less obvious
for the sake of saving a few function names.
Consider the standard example, which (these days) would be an API to a
DOM-like object that can be displayed or not; to query its state, you'd
have a method called
bool is_visible();
To change the state, the "flaggy" design would be
void set_visibility(bool);
but the obviously more readable design uses two methods,
void show();
void hide();
With the former, you always have to worry about what the boolean
parameter actually means; with the latter, there can be no doubt.
So in the case of displaying the contents of checksums, first of all
something called
svn_checksum_to_cstring_display
is clearly a broken API, especially if it's used to format the checksum
for the protocol, i.e., has nothing to do with displaying it at all. The
docstring says nothing about displaying; and there's no difference
between this function and svn_checksum_to_cstring, other than that the
latter accepts (and possibly returns) a NULL, where the former does not.
Then we have svn_checksum_serialize and svn_checksum_deserialize, which
I would expect to be always used for the protocol; and
svn_checksum_parse_hex, where it's not clear if it's the inverse of
svn_checksum_to_cstring, or the _display variant, or both, or WTF.
In other words, at least one of these APIs needs to be deprecated
because it has a terrible name, and all of them need better
documentation. If we want a function that's intended specifically for
printing fingerprints, then by all means call it
svn_checksum_fingerprint
although, really, I don't know why such a thing would have to be in
libsvn_subr, or perhaps in svn_checksum.h; I could imagine such a thing
living in subr but exposed in svn_cmdline.h, called
svn_cmdline_fingerprint or some such.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/12/14 12:56 PM, Ivan Zhakov wrote:
> My concerns are the following:
> 1. Avoid unrelated branch changes
> 2. Have some function for converting checksum to canonical form:
> a) one option is just leave svn_checksum_to_cstring_display() and
> add svn_checksum_to_cstring_display_ex()
> b) another option is deprecate svn_checksum_to_cstring_display(),
> replacing existing callers with
> new svn_checksum_to_cstring_canonical(), but I think it's
> better do this on trunk to make review easier.
Per our discussion I'll just remove the svn_checksum_* changes on the branch.
After we get the X.509 stuff merged to trunk I'll commit something that changes
the checksum output formatting and that accommodates most of your comments. I
am still inclined to use flags so that the function can be used in multiple
differing cases.
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 13 August 2014 01:24, Branko Čibej <br...@wandisco.com> wrote:
> On 12.08.2014 21:56, Ivan Zhakov wrote:
>
> On 11 August 2014 20:51, Ben Reser <be...@reser.org> wrote:
>
> On 8/11/14 1:59 AM, Ivan Zhakov wrote:
>
> 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.
>
> How about svn_checksum_to_cstring_canonical() which is just this macro:
> #define svn_checksum_to_cstring_canonical(checksum, pool)
> svn_checksum_to_cstring_display2((checksum), SVN_CHECKSUM_CSTRING_LOWER,
> (pool))
>
> I found macro as unnecessary hack in this case, while I like
> svn_checksum_to_cstring_canonical() name. Do you have any reasons
> against just making it public function.
>
> 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)
>
> I think we've spent more time talking about this than the time it takes to
> review those changes. Those are really easy changes since all they're doing
> is
> adding the SVN_CHECKSUM_CSTRING_LOWER and changing
> svn_checksum_to_cstring_display to svn_checksum_to_cstring_display2.
>
> 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.
>
> Does the macro I suggest above resolve the problem for you? I'll be happy
> to
> go find those cases and change to them, before or after the branch merges.
> But
> I don't really want to bother with that if you're not willing to budge on
> the
> leaving svn_checksum_to_cstring_display() alone.
>
> My concerns are the following:
> 1. Avoid unrelated branch changes
> 2. Have some function for converting checksum to canonical form:
> a) one option is just leave svn_checksum_to_cstring_display() and
> add svn_checksum_to_cstring_display_ex()
> b) another option is deprecate svn_checksum_to_cstring_display(),
> replacing existing callers with
> new svn_checksum_to_cstring_canonical(), but I think it's
> better do this on trunk to make review easier.
>
> Btw it would be nice to have tests for
> svn_checksum_to_cstring_display2()/svn_checksum_to_cstring_display_ex().
>
>
> At this point, just reverting the change that introduced
> svn_checksum_to_cstring_display2 from the x509 branch would resolve your
> objections, right, Ivan?
Yes, it does.
> We can live with not having a canonical checksum
> representation for display purposes that's different from the one in our
> wire protocol, and we can certainly address this mess separately from the
> x509 parser — which is, after all, the main purpose of the branch.
>
Yes, but I suggest just add tiny static function with nice printing in
auth-cmd.c: simple solution for release.
> To clarify, I would be against releasing the authn code as it is, with the
> extra info stored in the authn cache just because we don't have a cert
> parser on trunk; and I think the branch, even without the display changes,
> solves that problem just fine.
>
Agree.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/12/14 3:55 PM, Bert Huijben wrote:
> Since WC-NG we tried not to introduce new functions with flag arguments as in
> general functions like that are hard to maintain, while it is easy to rev
> functions to add another separate argument. (Another less preferred option is
> using a struct with separate args)
>
> I remember arguments from gstein but I have a hard time finding a mail reference.
>
> I think this would be the first new flag style argument in a public function
> since 1.2 or so.... If possible I would try using a different pattern here.
My goal was to avoid adding a bunch of separate arguments. The current
upper/lower and colons/no-colons isn't too bad. But I was thinking there might
be other formats that maybe didn't use hex or something like that we might want
to support in the future. But if we're trying to avoid flags I can just have
two booleans for the same functionality.
RE: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Bert Huijben <be...@qqmail.nl>.
Since WC-NG we tried not to introduce new functions with flag arguments as in general functions like that are hard to maintain, while it is easy to rev functions to add another separate argument. (Another less preferred option is using a struct with separate args)
I remember arguments from gstein but I have a hard time finding a mail reference.
I think this would be the first new flag style argument in a public function since 1.2 or so.... If possible I would try using a different pattern here.
I like the intermediate option of getting at least the feature merged to trunk without this function. I don't see any arguments against that.
Bert
-----Original Message-----
From: "Branko Čibej" <br...@wandisco.com>
Sent: 12/08/2014 23:25
To: "dev@subversion.apache.org" <de...@subversion.apache.org>
Subject: Re: [VOTE] Merge svn-auth-x509 branch to trunk?
On 12.08.2014 21:56, Ivan Zhakov wrote:
On 11 August 2014 20:51, Ben Reser <be...@reser.org> wrote:
On 8/11/14 1:59 AM, Ivan Zhakov wrote:
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.
How about svn_checksum_to_cstring_canonical() which is just this macro:
#define svn_checksum_to_cstring_canonical(checksum, pool)
svn_checksum_to_cstring_display2((checksum), SVN_CHECKSUM_CSTRING_LOWER, (pool))
I found macro as unnecessary hack in this case, while I like
svn_checksum_to_cstring_canonical() name. Do you have any reasons
against just making it public function.
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)
I think we've spent more time talking about this than the time it takes to
review those changes. Those are really easy changes since all they're doing is
adding the SVN_CHECKSUM_CSTRING_LOWER and changing
svn_checksum_to_cstring_display to svn_checksum_to_cstring_display2.
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.
Does the macro I suggest above resolve the problem for you? I'll be happy to
go find those cases and change to them, before or after the branch merges. But
I don't really want to bother with that if you're not willing to budge on the
leaving svn_checksum_to_cstring_display() alone.
My concerns are the following:
1. Avoid unrelated branch changes
2. Have some function for converting checksum to canonical form:
a) one option is just leave svn_checksum_to_cstring_display() and
add svn_checksum_to_cstring_display_ex()
b) another option is deprecate svn_checksum_to_cstring_display(),
replacing existing callers with
new svn_checksum_to_cstring_canonical(), but I think it's
better do this on trunk to make review easier.
Btw it would be nice to have tests for
svn_checksum_to_cstring_display2()/svn_checksum_to_cstring_display_ex().
At this point, just reverting the change that introduced svn_checksum_to_cstring_display2 from the x509 branch would resolve your objections, right, Ivan? We can live with not having a canonical checksum representation for display purposes that's different from the one in our wire protocol, and we can certainly address this mess separately from the x509 parser — which is, after all, the main purpose of the branch.
To clarify, I would be against releasing the authn code as it is, with the extra info stored in the authn cache just because we don't have a cert parser on trunk; and I think the branch, even without the display changes, solves that problem just fine.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Branko Čibej <br...@wandisco.com>.
On 12.08.2014 21:56, Ivan Zhakov wrote:
> On 11 August 2014 20:51, Ben Reser <be...@reser.org> wrote:
>> On 8/11/14 1:59 AM, Ivan Zhakov wrote:
>>> 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.
>> How about svn_checksum_to_cstring_canonical() which is just this macro:
>> #define svn_checksum_to_cstring_canonical(checksum, pool)
>> svn_checksum_to_cstring_display2((checksum), SVN_CHECKSUM_CSTRING_LOWER, (pool))
>>
> I found macro as unnecessary hack in this case, while I like
> svn_checksum_to_cstring_canonical() name. Do you have any reasons
> against just making it public function.
>
>>> 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)
>> I think we've spent more time talking about this than the time it takes to
>> review those changes. Those are really easy changes since all they're doing is
>> adding the SVN_CHECKSUM_CSTRING_LOWER and changing
>> svn_checksum_to_cstring_display to svn_checksum_to_cstring_display2.
>>
>>> 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.
>> Does the macro I suggest above resolve the problem for you? I'll be happy to
>> go find those cases and change to them, before or after the branch merges. But
>> I don't really want to bother with that if you're not willing to budge on the
>> leaving svn_checksum_to_cstring_display() alone.
>>
> My concerns are the following:
> 1. Avoid unrelated branch changes
> 2. Have some function for converting checksum to canonical form:
> a) one option is just leave svn_checksum_to_cstring_display() and
> add svn_checksum_to_cstring_display_ex()
> b) another option is deprecate svn_checksum_to_cstring_display(),
> replacing existing callers with
> new svn_checksum_to_cstring_canonical(), but I think it's
> better do this on trunk to make review easier.
>
> Btw it would be nice to have tests for
> svn_checksum_to_cstring_display2()/svn_checksum_to_cstring_display_ex().
At this point, just reverting the change that introduced
svn_checksum_to_cstring_display2 from the x509 branch would resolve your
objections, right, Ivan? We can live with not having a canonical
checksum representation for display purposes that's different from the
one in our wire protocol, and we can certainly address this mess
separately from the x509 parser — which is, after all, the main purpose
of the branch.
To clarify, I would be against releasing the authn code as it is, with
the extra info stored in the authn cache just because we don't have a
cert parser on trunk; and I think the branch, even without the display
changes, solves that problem just fine.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane@wandisco.com
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 11 August 2014 20:51, Ben Reser <be...@reser.org> wrote:
> On 8/11/14 1:59 AM, Ivan Zhakov wrote:
>> 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.
>
> How about svn_checksum_to_cstring_canonical() which is just this macro:
> #define svn_checksum_to_cstring_canonical(checksum, pool)
> svn_checksum_to_cstring_display2((checksum), SVN_CHECKSUM_CSTRING_LOWER, (pool))
>
I found macro as unnecessary hack in this case, while I like
svn_checksum_to_cstring_canonical() name. Do you have any reasons
against just making it public function.
>> 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)
>
> I think we've spent more time talking about this than the time it takes to
> review those changes. Those are really easy changes since all they're doing is
> adding the SVN_CHECKSUM_CSTRING_LOWER and changing
> svn_checksum_to_cstring_display to svn_checksum_to_cstring_display2.
>
>> 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.
>
> Does the macro I suggest above resolve the problem for you? I'll be happy to
> go find those cases and change to them, before or after the branch merges. But
> I don't really want to bother with that if you're not willing to budge on the
> leaving svn_checksum_to_cstring_display() alone.
>
My concerns are the following:
1. Avoid unrelated branch changes
2. Have some function for converting checksum to canonical form:
a) one option is just leave svn_checksum_to_cstring_display() and
add svn_checksum_to_cstring_display_ex()
b) another option is deprecate svn_checksum_to_cstring_display(),
replacing existing callers with
new svn_checksum_to_cstring_canonical(), but I think it's
better do this on trunk to make review easier.
Btw it would be nice to have tests for
svn_checksum_to_cstring_display2()/svn_checksum_to_cstring_display_ex().
--
Ivan Zhakov
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/11/14 1:59 AM, Ivan Zhakov wrote:
> 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.
How about svn_checksum_to_cstring_canonical() which is just this macro:
#define svn_checksum_to_cstring_canonical(checksum, pool)
svn_checksum_to_cstring_display2((checksum), SVN_CHECKSUM_CSTRING_LOWER, (pool))
> 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)
I think we've spent more time talking about this than the time it takes to
review those changes. Those are really easy changes since all they're doing is
adding the SVN_CHECKSUM_CSTRING_LOWER and changing
svn_checksum_to_cstring_display to svn_checksum_to_cstring_display2.
> 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.
Does the macro I suggest above resolve the problem for you? I'll be happy to
go find those cases and change to them, before or after the branch merges. But
I don't really want to bother with that if you're not willing to budge on the
leaving svn_checksum_to_cstring_display() alone.
>>> 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.
I'll fix this as well (actually I already fixed it but I'm seeing error leak
aborts that appear to be pre-existing that I haven't figured out yet, they may
be from my local changes that turn on the automatic pager).
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 10 August 2014 18:35, Ben Reser <be...@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
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
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.
Yes it creates some code churn, but I already did the work to deal with the
deprecation points.
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.
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_*.
> 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.
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.
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 August 2014 21:03, Ben Reser <be...@reser.org> wrote:
> On 8/7/14 4:10 AM, Ivan Zhakov wrote:
>> 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?
>
> The biggest reason I went ahead and deprecated the old function is to try and
> drive us to think about what format we're outputting. Quite a few of the
> places we're outputting the rather difficult to read and compare format to end
> users. We should probably change that but I think it's well outside the scope
> of the branch so I didn't do it.
>
> As you point out there are also a number of places where the formatting is part
> of our format. It's not just the repository, but it's also used in the
> filenames for pristines.
>
> I think I would much rather have these sorts of uses move to some other
> function name (or maybe a macro) since frankly
> svn_checksum_to_cstring_display() seems like the wrong name for a function
> that's outputting as you put it "canonical representations of checksums", which
> are not ended to be displayed.
>
> I certainly was surprised by all the places we ended up using it and felt that
> someone could have easily broken the repository by making a change to the
> formatting.
>
> I'm not fond of the idea of making that format private to the auth command
> because then 3rd parties have to duplicate it to match our output and we can't
> use it in other places that I think we probably ought to be using.
>
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.
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.
2. May be 'svn auth' should print warning and continue on certificate
parsing error?
>> ..\..\..\subversion\libsvn_subr\x509parse.c(939): warning C4018: '<' :
>> signed/unsigned mismatch
>> ..\..\..\subversion\libsvn_subr\x509parse.c(946): warning C4389: '!='
>> : signed/unsigned mismatch
>
> This is an easy fix. int i should be apr_size_t i;
>
Great!
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/7/14 4:10 AM, Ivan Zhakov wrote:
> 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?
The biggest reason I went ahead and deprecated the old function is to try and
drive us to think about what format we're outputting. Quite a few of the
places we're outputting the rather difficult to read and compare format to end
users. We should probably change that but I think it's well outside the scope
of the branch so I didn't do it.
As you point out there are also a number of places where the formatting is part
of our format. It's not just the repository, but it's also used in the
filenames for pristines.
I think I would much rather have these sorts of uses move to some other
function name (or maybe a macro) since frankly
svn_checksum_to_cstring_display() seems like the wrong name for a function
that's outputting as you put it "canonical representations of checksums", which
are not ended to be displayed.
I certainly was surprised by all the places we ended up using it and felt that
someone could have easily broken the repository by making a change to the
formatting.
I'm not fond of the idea of making that format private to the auth command
because then 3rd parties have to duplicate it to match our output and we can't
use it in other places that I think we probably ought to be using.
> 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(1054): warning C4389: '!='
> : signed/unsigned mismatch
This appears to be because pointers are unsigned and apr_size_t is signed.
Guess we can just cast the pointer arithmetic to apr_size_t. So they end up
looking like this respectively:
if (*len > (apr_size_t)(end - *p))
if (len != (apr_size_t)(end - p))
> ..\..\..\subversion\libsvn_subr\x509parse.c(939): warning C4018: '<' :
> signed/unsigned mismatch
> ..\..\..\subversion\libsvn_subr\x509parse.c(946): warning C4389: '!='
> : signed/unsigned mismatch
This is an easy fix. int i should be apr_size_t i;
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 6 August 2014 09:49, Ben Reser <be...@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
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Ben Reser <be...@reser.org>.
On 8/6/14 2:09 PM, Stefan Fuhrmann wrote:
> What would a worst-case failure scenario look like? Could a faulty
> parser result in the auth store reporting keys that the user does not
> want to trust (e.g. by stitching together random portions of the file)?
I'll go ahead and answer the question I think you're asking first and then I'll
talk about some other security related details that I'm not sure if you're
asking about or not.
Stitching together random bits of a certificate is not likely to result in
something that this parser would accept. It's not so much a generic ASN.1
parser that just happens to be parsing X.509 as it is an X.509 parser that has
some ASN.1 logic hard coded to be able to work. So if things are out of place
in the slightest bit it's going to fail.
Most of the tricky bits of this was figuring out how to properly deal with
character set encodings. There rare 5 character sets that get used in X.509:
UTF8String (we do nothing other than validate it's UTF-8)
BMPString (this is UCS-2, easy to convert to UTF-8)
UniversalString (this is UCS-4, also easy to convert to UTF-8)
IA5String (this is ASCII)
TeletexString/T61String (this is a mess, everyone just treats it as ISO-8859-1
even though it's not. We follow the norm for everyone else).
I suppose it's possible that we have a bug in the character set encodings and
that causes us to display data that's just wrong enough that the user doesn't
get the correct view. But I think this is not likely. Converting these
character sets is largely a matter of following rules, they're all subsets of
Unicode so we just have to alter the encoding not the code points being used.
I think if we had an error here it would be obvious.
The only other thing we represent is the dates. There are some possible
ambiguities in the date format but they are tested for and we've implemented
with the RFC says.
I'm not sure though what sort of scenario you're envisioning with this
question. The parser has nothing to do with the data that gets put into the
auth store (for now, more on that later) nor does it have anything to do with
deciding if the certificates are ok. It may have something to do with
prompting but I'll talk about that later.
There is a note in the documentation for the svn_x509_parse_cert() function
that says that this function doesn't validate the data it returns. This is
because the parser does not validate that the certificate is valid, it just
returns what's in it. We don't check to make sure the issuer signature is
correct or that the issuer is trusted. Part of the reason for this is we may
not even have the issuer certificate to do such checking, we only store the
server certificate, not the whole chain. Since we're not using OpenSSL we
don't necessarily have access to the same trust store as Serf would be using.
We also don't look for extensions that we don't know about that are marked with
the critical flag, which in general would mean we should error for the purpose
of validation if we don't know how to handle them.
None of this is the slightest bit unusual for a system displaying a certificate
to a user. The openssl x509 command does not do any of this when you ask it to
display a certificate. Serf may return an error to us saying the certificate
is untrusted for various reasons but will always return the parsed content of
the file which may indeed have a bogus Issuer Distinguished Name.
The only place we end up using the parser outside of handling the `svn auth`
command is in JavaHL which uses it to parse the cert in the case of needing to
prompt for trust because the existing callbacks (which fill in
svn_auth_ssl_server_cert_info_t) don't provide all the data our parser does
(which fills in svn_x509_certinfo_t) and Branko wanted to used the same class
for wrapping the return from our X.509 parser and when prompting for a cert, so
he used the raw cert provided by the callbacks to call our parser. The biggest
difference being that we only provide a single hostname which is whichever one
we decided happens to have matched (in the case of Subject Alt Names) or the
Common Name if none of them match.
I don't see this usage as risky from a security standpoint. JavaHL still
provides the certificate failures and the information we already provide the
callback is just as unreliable as the information from our parser. The only
risk I see here is if the X.509 parser has bugs in it with some edge cases in
the certs that I'm unaware of and didn't test then it's possible this might
break when used with a certificate we used to work with. But it should be
fairly trivial to fix since it just means fixing the parser. Doing this same
sort of thing to our command line client is probably a good idea because then
we can display all the hostnames the certificate can match when prompting for
trust. But I'd rather give the parser some real world exposure before
inserting it into this important of a code path.
Branko has suggested that we provide the svn_x509_certinfo_t value to all the
callback providers, which could have been done by changing this API but I
didn't feel it was worthwhile to change these APIs when I believe we'll want to
change them much more extensively. As things stand right now the current
design of the ssl server trust store is very bad in light of Subject Alternate
Names.
Certs are cached based on the auth realm including the hostname. This means
that for instance if you're using a server with a wildcard hostname that you're
going to end up storing the certs as many times as hostnames you end up using
(googlecode is good example of where this happens, since they have a wildcard
and every project gets it's own hostname). This behavior also makes things
ugly if we want to allow users to add certs via a command to be trusted. Since
they have to know the authentication realm to cache the cert from. An
alternative that solves both of these is to cache based on fingerprint of the
certificate. Which is something I'd like to pursue, possibly in 1.10.x.
We'll probably use the parser to display certs before adding them as trusted
via such a command. Partly because running the parser lets us see if the file
they're giving us at least looks like a certificate and partly to let them
confirm they're adding the cert they want. In that case I still don't think
these are huge issues because if they're adding a cert to trust it's not going
to be validating via a trust chain. So the user has to be validating the cert
some other way (hopefully via the fingerprint).
The only other possible risk is the whole buffer overflow situation. ASN.1 is
a length,tag,data format. So it's possible to send a length that is longer
than the data. The parser keeps track of the end based on the length of the
buffer it's told and if things don't match up it returns a
SVN_ERR_ASN1_LENGTH_MISMATCH error. So I believe this is handled.
So in summary, from a security perspective I don't think there is a worst case
scenario. From a trust perspective only thing that could really go wrong is if
we decode things wrong and they end up decided to trust something they wouldn't
in the JavaHL case or decide to leave a trust they'd previously allowed when
reviewing them in the svn auth case. I find that highly unlikely.
Re: [VOTE] Merge svn-auth-x509 branch to trunk?
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Aug 6, 2014 at 7:49 AM, Ben Reser <be...@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.
>
Hi Ben,
If I understand it correctly, the new parser is mainly intended to
improve our UI. To a degree that in itself will already result in
higher security.
What would a worst-case failure scenario look like? Could a faulty
parser result in the auth store reporting keys that the user does not
want to trust (e.g. by stitching together random portions of the file)?
-- Stefan^2.