Re: Switching from SHA1 to a checksum type without known collisions in 1.15 working copy format
On Sat, Jan 13, 2024 at 3:56 PM Nathan Hartman wrote: > Pros: Future-proofing against the real and perceived brokenness of any > hash types. > I meant to write: Pros: Future-proofing against the real and perceived brokenness of any hash types, or the deprecation and later removal of their implementations from our deps. Cheers, Nathan
Re: Switching from SHA1 to a checksum type without known collisions in 1.15 working copy format
On Fri, Jan 12, 2024 at 3:51 PM Johan Corveleyn wrote: > On Fri, Jan 12, 2024 at 12:37 PM Daniel Shahaf > wrote: > ... > > Procedurally, the long hiatus is counterproductive. Neither kfogel nor > > I had the context in our heads, and the cache misses took their toll in > > tuits and in wallclock time. Furthermore, I have less spare time for > > dev@ discussions than I did when I cast the veto (= a year ago next > > Saturday). Going forward it might be preferable for threads not to > > hibernate. > > I agree, but obviously the hibernation is not some deliberate action > by anyone. It's just that most of us here have less spare time for > dev@ discussions (and for SVN development) than before. Especially for > such complex matters, and especially when people feel there are > walking into a minefield. There are only a few active devs left, and > tuits are running low ... > > ... > > That being the case, I have considered whether merging the feature > > branch outweighs letting dev@ take a not-only-/pro forma/ role in > > design discussions. I am of the opinion that it does not, and > > therefore I reäfirrm the veto. > > It has become more clear to me (I was only following tangentially) > that your veto is focused on the development methodology and the lack > of design discussion. Is that a valid reason for a veto? We are low on > resources, someone still finds time to make some progress, no one > blocks it on technical grounds, and then someone vetoes it because we > don't have enough resources? > > That puts us pretty much in deadlock, because we are too low on > resources. Or maybe I misunderstand? > > To be clear: I appreciate your input, Daniel, and your insistence on a > more thorough design discussion. I assume it's coming from a genuine > concern that we formulate problems well, and think hard about possible > solutions (focusing on the precise problem we are trying to solve). > But at the end of the day, if that design discussion doesn't happen > (or not enough to your satisfaction anyway), is that grounds for a > veto? For me it's a tough call, because on the one hand you have a > point, but on the other hand ... you're blocking _some_ progress > because the process behind it is not perfect (which is hard to do with > the 3.25 tuits we have left). > > > P.S. Could that BRANCH-README please state what's the problem the branch > > means to solve, i.e., the goal / acceptance test? "Make it possible to > > «svn add» SHA-1 collisions"? > > I agree that would be a good step. > > I too find it a bit unclear what problem we're actually trying to > solve, apart from a vague feeling that SHA-1 will become more and more > broken over time, and that this will cause fatal injury to SVN (in its > WC, protocol, dump format, or repository). And perhaps the fact that > security auditors are becoming more and more triggered by seeing SHA-1 > (even if they don't understand the way it is used and its > ramifications). Making it possible to 'svn add' SHA-1 collisions is > not it, I think. > > -- > Johan > Johan's reply sums up my thoughts pretty closely. I would very much like to *avoid* all of the following: deadlock, bad feelings, and members of this small community leaving because of deadlocks or bad feelings. I agree that (at the very least), BRANCH-README should define what problem the branch aims to solve, and perhaps that's really the main thing we need to discuss and resolve. Johan touched on one issue with SHA1: regardless how it is actually used in SVN and whether it is adequate for those purposes, there is customer perception. I can imagine, for example, the IT dept of some big $corporation could blacklist SHA1 because it is considered broken for cryptographic purposes. But they could blacklist it for everything. Even though it is safe and effective for our use cases, try explaining that to an admin who is struggling to meet such a blanket policy. I would like to add another reason to think about a post-SHA1 future: I'm writing on mobile so I can't easily grep for things now, but could our dependencies eventually remove the SHA1 implementation? (I just saw something about removal of DSA from some famous lib not too long ago. SHA1 could be next?) When would SHA1 disappear? I don't know, but I consider it plausible to happen in about 5 years. If SHA1 is removed in the future, there will need to be a mad dash to replace it. Or we'll have to add a new dependency to use an alternate implementation. Or we'll have to implement our own SHA1 or copy some code into SVN. All of these seem bad to me. Switching to a different hash is also a bad idea, I think, because it is likely to suffer the same problems as SHA1 later on, as cryptography research proceeds and newer hashes become declared broken. I'll try to describe what I think is a best case scenario: Support multi-hash in 1.15 in format 32 WCs. SHA1 can continue to be the default but we should be careful not to require a SHA1 implementation to
Re: mbox archives
Den fre 12 jan. 2024 kl 12:02 skrev Daniel Shahaf : > I think it should be clear to members of our community how to access our > Collective Memory (= the list archives) in the preferred form for > reading, to borrow ALv2's definition of "Source". I would expect that > to be self-evident from ponymail's UI, and failing that, documented on > our /mailing-lists.html page. > How about the changes in r1915225? (See https://subversion-staging.apache.org/mailing-lists.html#downloading Kind regards, Daniel Sahlberg
Re: Switching from SHA1 to a checksum type without known collisions in 1.15 working copy format
Den lör 13 jan. 2024 kl 00:50 skrev Johan Corveleyn : > On Fri, Jan 12, 2024 at 12:37 PM Daniel Shahaf > wrote: > ... > > Procedurally, the long hiatus is counterproductive. Neither kfogel nor > > I had the context in our heads, and the cache misses took their toll in > > tuits and in wallclock time. Furthermore, I have less spare time for > > dev@ discussions than I did when I cast the veto (= a year ago next > > Saturday). Going forward it might be preferable for threads not to > > hibernate. > > I agree, but obviously the hibernation is not some deliberate action > by anyone. It's just that most of us here have less spare time for > dev@ discussions (and for SVN development) than before. Especially for > such complex matters, and especially when people feel there are > walking into a minefield. There are only a few active devs left, and > tuits are running low ... > I agree with Johan on this. The long hiatus is unfortunate. But it won't help to point fingers at this point. > > ... > > That being the case, I have considered whether merging the feature > > branch outweighs letting dev@ take a not-only-/pro forma/ role in > > design discussions. I am of the opinion that it does not, and > > therefore I reäfirrm the veto. > > It has become more clear to me (I was only following tangentially) > that your veto is focused on the development methodology and the lack > of design discussion. Is that a valid reason for a veto? We are low on > resources, someone still finds time to make some progress, no one > blocks it on technical grounds, and then someone vetoes it because we > don't have enough resources? > > That puts us pretty much in deadlock, because we are too low on > resources. Or maybe I misunderstand? > > To be clear: I appreciate your input, Daniel, and your insistence on a > more thorough design discussion. I assume it's coming from a genuine > concern that we formulate problems well, and think hard about possible > solutions (focusing on the precise problem we are trying to solve). > But at the end of the day, if that design discussion doesn't happen > (or not enough to your satisfaction anyway), is that grounds for a > veto? For me it's a tough call, because on the one hand you have a > point, but on the other hand ... you're blocking _some_ progress > because the process behind it is not perfect (which is hard to do with > the 3.25 tuits we have left). > > > P.S. Could that BRANCH-README please state what's the problem the branch > > means to solve, i.e., the goal / acceptance test? "Make it possible to > > «svn add» SHA-1 collisions"? > > I agree that would be a good step. > > I too find it a bit unclear what problem we're actually trying to > solve, apart from a vague feeling that SHA-1 will become more and more > broken over time, and that this will cause fatal injury to SVN (in its > WC, protocol, dump format, or repository). And perhaps the fact that > security auditors are becoming more and more triggered by seeing SHA-1 > (even if they don't understand the way it is used and its > ramifications). Making it possible to 'svn add' SHA-1 collisions is > not it, I think. > I also agree with this. >From what I remember of the dicsussions earlier there were concerns that a changed file might go undetected if someone change it to another file with a collision with the original file. I think that might be a vaild point, especially if we don't have the pristine files anymore. I'd also like to understand why we need the multi-checksum format instead of just plainly switching to XXX (insert favourite checksuming algorithm here). Does it help us to have multiple types of checksums available? Would we use BOTH as a resort (likelyhood of collision in SHA1 and in XXX at the same time approaching zero)? Does it help backwards/forwards compatibility? Kind regards, Daniel Sahlberg
Re: Changing the permission checks in libsvn_subr/io.c
Den ons 10 jan. 2024 kl 10:45 skrev Stefan Sperling : > On Wed, Jan 10, 2024 at 09:44:51AM +0100, Johan Corveleyn wrote: > > Interesting discussion. I agree it should at least be documented, and > > perhaps be made a bit more clear from the output of 'revert' (but not > > sure how far we can go without breaking compat). Changing the current > > behavior is probably a more risky move, given the maturity of SVN and > > backwards compatibility etc. > > Adding new notification types should not cause compatibility concerns. > If adding new notifications breaks other software which reads command > line client output than this other software has a bug: It should have > been ignoring unknown lines of output in the first place. > > API users would likewise simply need to catch the new notification type > in a switch-like statement and should also be ignoring unknown values. > > I would only see a compatibility problem if an existing notification > about an important event no longer appears. > > In the past we have made significant changes to output from commands, > such as when tree conflict detection was added in 1.6. That by itself > has not resulted in any problems, as far as I know. > Thanks for the feedback (and sorry for mixing up the threads previously saying there was no feedback here). I've made a separate commit r1915215 adding code to catch the inconsistency with the auth bits and report a separate notification. Kind regards, Daniel
Re: Changing the permission checks in libsvn_subr/io.c
Den fre 5 jan. 2024 kl 08:45 skrev Daniel Sahlberg <
daniel.l.sahlb...@gmail.com>:
> Hi,
>
> When researching the spurious revert messages reported by Vincent Lefevre
> [1], I was looking at the code in svn_io__is_finfo_read_only() and
> svn_io_is_file_executable(). It gets the current UID and GID using the APR
> function apr_uid_current() and then compares to see if the owner of the
> file is the current user (then check for user write/execute permissions) or
> if the group owning the file is the current user's group (then check for
> group write/execute permissions) or otherwise check for world write/execute
> permissions.
>
> I think there is a failure mode when a user has write permissions to a
> file through a secondary group, something like this:
>
> [[[
> dsg@daniel-23:~/svn_trunk$ groups
> dsg adm dialout cdrom floppy sudo audio dip video plugdev netdev docker
> dsg@daniel-23:~/svn_trunk$ ls -l README
> -rw-rw-r-- 1 root sudo 2341 Sep 22 2022 README
> dsg@daniel-23:~/svn_trunk$ svn proplist -v README
> Properties on 'README':
> svn:eol-style
> native
> svn:keywords
> LastChangedDate
> dsg@daniel-23:~/svn_trunk$ svn revert README
> Reverted 'README'
> dsg@daniel-23:~/svn_trunk$ ls -l README
> -rw-rw-r-- 1 root sudo 2341 Sep 22 2022 README
> dsg@daniel-23:~/svn_trunk$ svn revert README
> Reverted 'README'
> dsg@daniel-23:~/svn_trunk$ ./subversion/svn/svn revert README
> dsg@daniel-23:~/svn_trunk$ ls -l README
> -rw-rw-r-- 1 root sudo 2341 Sep 22 2022 README
> dsg@daniel-23:~/svn_trunk$
> ]]]
>
> My user dsg has the primary group dsg and, among others, belong to the
> sudo group.
> The README file is owned by root:sudo and has 664 permission. I'm thus
> able to write to the file.
> svn revert will report Reverted, since the file README isn't owned by the
> user DSG or the group DSG and thus svn_io__is_finfo_read_only() will return
> that the file is readonly. Since the file doesn't have svn:needs-lock it
> should be RW and the Reverted message comes from Subversion trying to
> restore the W flag (which fails, thus a second svn revert call will report
> the same message).
> I initially thought about rewriting svn_io__is_finfo_read_only() to look
> also for the user's secondary groups but when asking on dev@apr.a.o if
> there is an APR way of listing a users secondary groups, Joe Orton
> suggested [2] to instead check with access(2).
>
> I've run the testsuite on Ubuntu 23.10 and OpenBSD 7.4. Initially I had an
> error on special_tests.py symlink_destination_change() which contains a
> dangling symlink. It seems the old code was checking for W on the actual
> symlink, while access() is checking on the target (which doesn't exists and
> thus ENOENT). To solve this I added the test for ENOENT in
> svn_io__is_finfo_read_only(). The same test is not needed on
> svn_io_is_file_executable() since it will not be called for a symlink (see
> revert_wc_data()). I'm not sure if this is the right way to solve this
> problem or if a change should be done in revert_wc_data() also for the
> read_only check.
>
> [[[
> Index: subversion/libsvn_subr/io.c
> ===
> --- subversion/libsvn_subr/io.c (revision 1915064)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -2535,7 +2535,14 @@ svn_io__is_finfo_read_only(svn_boolean_t *read_onl
>apr_uid_t uid;
>apr_gid_t gid;
>
> - *read_only = FALSE;
> + *read_only = (access(file_info->fname, W_OK) != 0);
> + /* svn_io__is_finfo_read_only can be called with a dangling
> + * symlink. access() will check the permission on the missing
> + * target and return -1 and errno = ENOENT. Check for ENOENT
> + * and pretend the file is writeable, otherwise we will get
> + * spurious Reverted messages on the symlink.
> + */
> + if (*read_only && errno == ENOENT) *read_only = FALSE;
>
>apr_err = apr_uid_current(, , pool);
>
> @@ -2542,16 +2549,6 @@ svn_io__is_finfo_read_only(svn_boolean_t *read_onl
>if (apr_err)
> return svn_error_wrap_apr(apr_err, _("Error getting UID of process"));
>
> - /* Check write bit for current user. */
> - if (apr_uid_compare(uid, file_info->user) == APR_SUCCESS)
> -*read_only = !(file_info->protection & APR_UWRITE);
> -
> - else if (apr_gid_compare(gid, file_info->group) == APR_SUCCESS)
> -*read_only = !(file_info->protection & APR_GWRITE);
> -
> - else
> -*read_only = !(file_info->protection & APR_WWRITE);
> -
> #else /* WIN32 || __OS2__ || !APR_HAS_USER */
>*read_only = (file_info->protection & APR_FREADONLY);
> #endif
> @@ -2565,27 +2562,7 @@ svn_io__is_finfo_executable(svn_boolean_t *executa
> apr_pool_t *pool)
> {
> #if defined(APR_HAS_USER) && !defined(WIN32) &&!defined(__OS2__)
> - apr_status_t apr_err;
> - apr_uid_t uid;
> - apr_gid_t gid;
> -
> - *executable = FALSE;
> -
> - apr_err = apr_uid_current(, , pool);
> -
> - if (apr_err)
> -return svn_error_wrap_apr(apr_err, _("Error
