[VOTE] Reverting r1845377 (Was: [PROPOSAL] Allow plaintext passwords again.)

[Daniel Sahlberg <da...@gmail.com>]

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.


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).


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.

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".


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.

Kind regards,
Daniel Sahlberg

Re: [VOTE] Reverting r1845377 (Was: [PROPOSAL] Allow plaintext passwords again.)

[Mark Phippard <ma...@gmail.com>]

On Sat, Apr 22, 2023 at 4:30 AM Branko Čibej <br...@apache.org> wrote:
>
> On 22.04.2023 10:27, Branko Čibej wrote:
>
> On 21.04.2023 16:43, Johan Corveleyn wrote:
>
>
> 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!

+1. great to see progress on this. Getting the change reverted should
be the priority. If someone volunteers to take up some of the
additional ideas even better but we do not need to block on them.


> 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).
>
>
> There's actually a branch for that, but the feature never made it to trunk.
>
>
> I'll just add that the intent of that branch is to properly encrypt passwords at least on Linux, not to obfuscate them or embed a key in the code. The latter is worse than storing in plain text, because the effect is the same but it adds a veneer of false sense of security.

Feedback we had from users did not agree with this. They would have
seen something as simple as Base64 encoding the string as an
improvement. I believe there is a way to implement this without
creating the impression that the string is cryptographically secure.
For starters, you could just name the property in the file something
like "obfuscatedPassword" or "base64Password" to make it clear.
Kubernetes has used the term "Opaque" for its Base64 encoded but
otherwise plaintext "secrets".

Even if we did use encryption and stored the key in the same file it
would still be better than having a plain text string.

I would agree that we should not suggest that the password is "secure"
if we do this but we have let this argument be the enemy of making
progress for nearly 2 decades when lots of users have said this would
be an improvement for them.

Mark

Re: [VOTE] Reverting r1845377 (Was: [PROPOSAL] Allow plaintext passwords again.)

[Daniel Sahlberg <da...@gmail.com>]

Den lör 22 apr. 2023 kl 10:30 skrev Branko Čibej <br...@apache.org>:

> On 22.04.2023 10:27, Branko Čibej wrote:
>
> On 21.04.2023 16:43, Johan Corveleyn wrote:
>
>
> 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).
>
>
> There's actually a branch for that, but the feature never made it to trunk.
>
>
> I'll just add that the intent of that branch is to properly encrypt
> passwords at least on Linux, not to obfuscate them or embed a key in the
> code. The latter is worse than storing in plain text, because the effect is
> the same but it adds a veneer of false sense of security.
>

I assume you are referring to the master-password branch? That will not
help with the use case some users are having - ie unattended svn actions -
since the user will have to enter the master password instead of the
password. (The use case "not remembering a lot of different passwords while
not storing passwords in clear" is of course solved by a master password).

--- thread reply ---
As you have probably seen, I've committed the revert as r1909351.

I have not made up my mind regarding the other points, but as pointed out
by several others, let's start with reverting and then take it from there.
I will read through all e-mails once more and see if i can sumarize the
discussion somewhat. So far I couldn't see any clear direction.

Kind regards,
Daniel

Re: [VOTE] Reverting r1845377 (Was: [PROPOSAL] Allow plaintext passwords again.)

[Branko Čibej <br...@apache.org>]

On 22.04.2023 10:27, Branko Čibej wrote:
> On 21.04.2023 16:43, Johan Corveleyn wrote:
>>
>>> 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).
>
> There's actually a branch for that, but the feature never made it to 
> trunk.

I'll just add that the intent of that branch is to properly encrypt 
passwords at least on Linux, not to obfuscate them or embed a key in the 
code. The latter is worse than storing in plain text, because the effect 
is the same but it adds a veneer of false sense of security.

-- Brane

Re: [VOTE] Reverting r1845377 (Was: [PROPOSAL] Allow plaintext passwords again.)

[Branko Čibej <br...@apache.org>]

On 21.04.2023 16:43, Johan Corveleyn wrote:
>
>> 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).

There's actually a branch for that, but the feature never made it to trunk.

Re: [VOTE] Reverting r1845377 (Was: [PROPOSAL] Allow plaintext passwords again.)

[Johan Corveleyn <jc...@gmail.com>]

On Sun, Apr 16, 2023 at 11:19 PM Daniel Sahlberg
<da...@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