On Sun, Apr 16, 2023 at 11:19 PM Daniel Sahlberg
<daniel.l.sahlb...@gmail.com> wrote:
>
> The dicussion died again, but this time I intend make sure we complete it
> once and for all. I've marked the subject as VOTE to hopefully get some
> attention, although I believe votes have already been cast.
Thanks for picking it up once again :-). Let's hope we can land this now ...
> In my mind, it seems we have consensus to revert r1845377 (+1 from Nathan
> Hartman, Evgeny Kotkov, Johan Corveleyn, myself, I'm also considering Karl
> Fogel to have voted for this by making the initial proposition and
> volunteering to do it. No objections).
Indeed, +1 from me.
> Then we have a two other suggestions:
>
> * Changing the default value of store-plaintext-passwords
> Option 1 - set the default value of store-plaintext-passwords to no. For a
> non-edited servers config file, this will put the behaviour of 1.15 in line
> with 1.12-1.14.
> Option 2 - keep store-plaintext-passwords = ask. For a non-edited servers
> config file, the behaviour of 1.15 will be in line with pre-1.12.
>
> I have previously expressed support (but no formal vote, and I will not stand
> in the way of consensus) for option 1, since I was under the impression that
> it was a longterm goal go disable the plaintext password store (I have
> previously commented on how quickly r1845377 was discussed and implemented).
> Nathan has argued that it might give a bad user experience if credentials are
> not stored even though plaintext storage is enabled in the build options. We
> can improve on this by updating the svn --version output to explicitly say if
> plaintext cache is build but runtime disabled.
>
> If I'm counting correctly, Nathan, Evgeny and Johan has expressed +1 for
> option 2.
>
> Considering this, I conclude we have consensus for option 2.
>
>
> * Changing handling of already stored plaintext passwords
> This was discussed in 2022 and there were some suggestions that Subversion
> should not even use plaintext passwords even if they were found in the
> plaintext password store (initially suggested by Mark Phippard as a way to
> soften the impact of reverting r1845377). I'm proposing we choose amoung one
> of the following three options:
>
> Option 1 - do nothing. Keep using the stored passwords.
> Option 2 - add a new runtime config option dont-use-plaintext-passwords
> [default no] global overrides local.
> Option 3 - new compile time option to disable reading/using plaintext
> passwords.
Why a new compile-time option for disabling reading? Why not keep it
simple, and disable the reading of plaintext passwords if the
compile-time option that will be re-introduced by reverting r1845377
is supplied (--disable-plaintext-password-storage)? I think that is
what Mark was suggesting, and I still like it because it's simple and
doesn't add more knobs than necessary.
So maybe:
Option 4 - disable reading/using plaintext passwords if compile-time
/storing/ of plaintext-passwords is configured
(--disable-plaintext-password-storage).
>
> Options 2 and 3 should probably imply disabling storing plaintext passwords
> as well. I think they should also warn if the code finds a stored password
> and suggest the user to delete it using svn auth --remove. (It was hinted
> previously that we would disable the code that searches for the files storing
> the password, however the same files also store the username and we read them
> as an apr_hash_t so we don't really have the option to "not read the
> password").
>
> With option 2 or 3 we let a security consious organisation configure their
> system to their liking. I would love to have them, but I can't avoid the
> feeling that it is security theater: As far as I can tell it is not possible
> to avoid that a user can upload their own version of the svn binary and then
> all bets are off anyway. (On Windows and MacOS it is possible to only allow
> execution of signed binaries, and they don't even store the passwords in
> plaintext anyway).
>
> If we go with either 2 or 3, then I'm perfering option 2 since it will allow
> the administrators to set this without compiling their own version (which
> seems to be a major obstacle, considering the reaction to r1845377). I
> believe Karl Fogel and Mark was also leaning towards doing something during
> the discussion in 2022.
>
> Johan seems to believe option 1 is fine ("these additional \"mitigations\"
> are not absolutely required").
>
> In my mind, it is perfectly acceptable to vote +1 on both option 1 AND one of
> option 2/3. I would interpret that as "do nothing in the short term, but do X
> in the long term".
Agreed. So option 1 follows automatically from reverting r1845377; and
option 2, 3 or 4 can be done later.
>
> My plan is to revert r1845377 during next weekend. For the first bulletpoint
> nothing has to be done, but if consensus changes during the week, I can do
> the work to to implement option 1. For the second bullet point I'd like to
> reach consensus (on the short- and long term plan) by the same time, then if
> we decide on 2/3 we will put it on the wish list for a future release.
Great!
There was one additional suggestion: obfuscate plaintext passwords, or
encrypt them with a key embedded in the code (or even with a key
supplied at compile-time). Though that'd be nice, it might not be as
trivial as it seems at first sight (keeping in mind that old and new
clients might be mixed on someone's system, so we can't just put an
obfuscated password in the "simple" passtype). I wouldn't want this to
block the simple revert of r1845377.
--
Johan
