You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2008/04/20 23:57:56 UTC
Re: svn commit: r30723 - in
branches/dont-save-plaintext-passwords-by-default: .
subversion/include subversion/libsvn_subr subversion/svn
On Sun, Apr 20, 2008 at 04:42:56PM -0700, David Glasser wrote:
> Hmm, why not just have the "global" servers section be the normal
> place to configure this? Why use the other config file at all?
Because most users will look at the 'config' file first, I guess.
Also, there, it's right next to 'store-passwords'.
The docstrings reference each other, so I think it's sorta OK...
The other idea I had was to have the option be valid in both
the config file and the servers file [global] section, and
have svn print a warning when the two disagree and fall back
to 'prompt'. But I discarded that as overkill.
--
Stefan Sperling <st...@elego.de> Software Monkey
German law requires the following banner :(
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de CEO: Olaf Wagner
Store password unencrypted (yes/no)? No
Re: svn commit: r30723 - in branches/dont-save-plaintext-passwords-by-default: . subversion/include subversion/libsvn_subr subversion/svn
Posted by Travis <sv...@castle.fastmail.fm>.
On Apr 22, 2008, at 4:41 AM, Stefan Sperling wrote:
> On Mon, Apr 21, 2008 at 10:50:25PM +0200, Stefan Sperling wrote:
>> On Mon, Apr 21, 2008 at 10:44:19AM -0700, David Glasser wrote:
>>> On Mon, Apr 21, 2008 at 6:31 AM, C. Michael Pilato
>>> <cm...@collab.net> wrote:
>>>> Stefan Sperling wrote:
>>>>
>>>>> On Sun, Apr 20, 2008 at 04:42:56PM -0700, David Glasser wrote:
>>>>>
>>>>>> Hmm, why not just have the "global" servers section be the normal
>>>>>> place to configure this? Why use the other config file at all?
>>>>>>
>>>>>
>>>>> Because most users will look at the 'config' file first, I guess.
>>>>> Also, there, it's right next to 'store-passwords'.
>>>>>
>>>>> The docstrings reference each other, so I think it's sorta OK...
>>>>>
>>>>> The other idea I had was to have the option be valid in both
>>>>> the config file and the servers file [global] section, and
>>>>> have svn print a warning when the two disagree and fall back
>>>>> to 'prompt'. But I discarded that as overkill.
>>>>>
>>>>
>>>> That we ever had the store-passwords option in the 'config'
>>>> file instead of
>>>> in the 'servers' file (bound to RA-related things) might have
>>>> been a
>>>> mistake. Let's evaluate the correct location for this new
>>>> option without
>>>> concern for the dubious decisions of the past.
>>>
>>> +1
>>
>> Right, so if we stored 'store-plaintext-passwords' in 'servers' only,
>> we should also move 'store-passwords' and 'store-auth-creds' to
>> 'servers'.
>> That is, the whole [auth] section of the 'config' file will go to the
>> 'servers' file, with a comment in 'config' informing users about
>> the move.
>>
>> Agreed?
> ...
> Does anyone oppose moving the whole [auth] section from 'config' to
> 'servers' for consistency?
Yay, please do. Having it in 'config' always struck me as wrong
because there's no reason I necessarily want to have the same
settings when accessing all servers. I might want to prevent store-
auth-creds for some specific servers (high security, very infrequent
use) but allow it for my workaday projects (it's almost impossible to
do without when using the command-line client).
Also, simply looking for the settings, it's more natural to look in
'server' for the [auth] settings since those are all about
credentials for communication with a given server, like the http-
proxy-* and ssl* settings. For my apache/dav served repositories,
the repository itself has no authentication/authorization information.
-Travis
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30723 - in branches/dont-save-plaintext-passwords-by-default: . subversion/include subversion/libsvn_subr subversion/svn
Posted by Karl Fogel <kf...@red-bean.com>.
Stefan Sperling <st...@elego.de> writes:
>> Might be good to do this deprecation/move on a separate branch, merge
>> that to trunk, then incorporate into the dont-save-plaintext branch and
>> DTRT. They're kind of separate changes, I think.
>
> I've already done the move in my branch WC now, basically about to
> commit the change, and I'm too lazy right now to untangle these two
> admittedly separate topics. I hope that is OK? :)
Sure, no big deal IMHO.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30723 - in
branches/dont-save-plaintext-passwords-by-default: .
subversion/include subversion/libsvn_subr subversion/svn
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Apr 22, 2008 at 02:33:18PM -0400, Karl Fogel wrote:
> Stefan Sperling <st...@elego.de> writes:
> > I will deprecate the old location in the comments, like this:
> >
> > "[auth]" NL
> > "### Set store-passwords to 'no' to avoid storing passwords in the" NL
> > "### auth/ area of your config directory. It defaults to 'yes'," NL
> > "### but Subversion will never save your password to disk in" NL
> > "### plaintext unless you tell it to (see below)." NL
> > "### Note that this option only prevents saving of *new* passwords;" NL
> > "### it doesn't invalidate existing passwords. (To do that, remove" NL
> > "### the cache files by hand as described in the Subversion book.)" NL
> > + "### NOTE: This option can now be specified in the 'servers' file" NL
> > + "### in your config directory. This location for this option has" NL
> > + "### been deprecated. Anything specified here is overridden by" NL
> > + "### settings specified in the 'servers' file." NL
> > "# store-passwords = no" NL
>
> +1, but put the deprecation notice at the top of the block, so people
> see it and then know that everything they read after that is about
> something deprecated anyway.
Yes, that's a good idea.
> > No, the code enforces this. But note that having all the [auth]
> > settings in 'servers' makes much more sense anyway, because
> > there, they can be configured on a per-server basis.
> >
> > It also aligns much more naturally with the layering, since
> > authentication is done only if the RA layer is entered anyway,
> > which has always been getting its configuration from 'servers'.
> >
> > The [auth] section in 'config' was a mistake, it would have
> > been much more natural to put it in 'servers' in the first place.
>
> Okay, I'm convinced.
>
> Might be good to do this deprecation/move on a separate branch, merge
> that to trunk, then incorporate into the dont-save-plaintext branch and
> DTRT. They're kind of separate changes, I think.
I've already done the move in my branch WC now, basically about to
commit the change, and I'm too lazy right now to untangle these two
admittedly separate topics. I hope that is OK? :)
--
Stefan Sperling <st...@elego.de> Software Monkey
German law requires the following banner :(
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de CEO: Olaf Wagner
Store password unencrypted (yes/no)? No
Re: svn commit: r30723 - in branches/dont-save-plaintext-passwords-by-default: . subversion/include subversion/libsvn_subr subversion/svn
Posted by Karl Fogel <kf...@red-bean.com>.
Stefan Sperling <st...@elego.de> writes:
> I will deprecate the old location in the comments, like this:
>
> "[auth]" NL
> "### Set store-passwords to 'no' to avoid storing passwords in the" NL
> "### auth/ area of your config directory. It defaults to 'yes'," NL
> "### but Subversion will never save your password to disk in" NL
> "### plaintext unless you tell it to (see below)." NL
> "### Note that this option only prevents saving of *new* passwords;" NL
> "### it doesn't invalidate existing passwords. (To do that, remove" NL
> "### the cache files by hand as described in the Subversion book.)" NL
> + "### NOTE: This option can now be specified in the 'servers' file" NL
> + "### in your config directory. This location for this option has" NL
> + "### been deprecated. Anything specified here is overridden by" NL
> + "### settings specified in the 'servers' file." NL
> "# store-passwords = no" NL
+1, but put the deprecation notice at the top of the block, so people
see it and then know that everything they read after that is about
something deprecated anyway.
> No, the code enforces this. But note that having all the [auth]
> settings in 'servers' makes much more sense anyway, because
> there, they can be configured on a per-server basis.
>
> It also aligns much more naturally with the layering, since
> authentication is done only if the RA layer is entered anyway,
> which has always been getting its configuration from 'servers'.
>
> The [auth] section in 'config' was a mistake, it would have
> been much more natural to put it in 'servers' in the first place.
Okay, I'm convinced.
Might be good to do this deprecation/move on a separate branch, merge
that to trunk, then incorporate into the dont-save-plaintext branch and
DTRT. They're kind of separate changes, I think.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30723 - in
branches/dont-save-plaintext-passwords-by-default: .
subversion/include subversion/libsvn_subr subversion/svn
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Apr 22, 2008 at 10:10:45AM -0400, Karl Fogel wrote:
> Stefan Sperling <st...@elego.de> writes:
> > Does anyone oppose moving the whole [auth] section from 'config' to
> > 'servers' for consistency?
>
> As long as we maintain compatibility...
I will deprecate the old location in the comments, like this:
"[auth]" NL
"### Set store-passwords to 'no' to avoid storing passwords in the" NL
"### auth/ area of your config directory. It defaults to 'yes'," NL
"### but Subversion will never save your password to disk in" NL
"### plaintext unless you tell it to (see below)." NL
"### Note that this option only prevents saving of *new* passwords;" NL
"### it doesn't invalidate existing passwords. (To do that, remove" NL
"### the cache files by hand as described in the Subversion book.)" NL
+ "### NOTE: This option can now be specified in the 'servers' file" NL
+ "### in your config directory. This location for this option has" NL
+ "### been deprecated. Anything specified here is overridden by" NL
+ "### settings specified in the 'servers' file." NL
"# store-passwords = no" NL
> But why is there a connection between which file the "[auth]" section
> lives in (a user-visible question) and which URLs we use that data for
> (an internal code-flow issue)?
Because 'config' is read before the subcommand is run, which
then figures out whether to contact a server, and which one.
While reading 'servers', we need to know which server we're contacting,
because we may need to match a server group:
[groups]
server1 = *.tigris.org
[server1]
store-plaintext-passwords = no
[global]
store-plaintext-passwords = yes
> The choice of which config file to use should be made based on what will
> make most sense for users, who don't know or care how our code is
> organized internally. It sounds like these two issues are maybe getting
> mixed together, in your comments above?
No, the code enforces this. But note that having all the [auth]
settings in 'servers' makes much more sense anyway, because
there, they can be configured on a per-server basis.
It also aligns much more naturally with the layering, since
authentication is done only if the RA layer is entered anyway,
which has always been getting its configuration from 'servers'.
The [auth] section in 'config' was a mistake, it would have
been much more natural to put it in 'servers' in the first place.
--
Stefan Sperling <st...@elego.de> Software Monkey
German law requires the following banner :(
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de CEO: Olaf Wagner
Store password unencrypted (yes/no)? No
Re: svn commit: r30723 - in branches/dont-save-plaintext-passwords-by-default: . subversion/include subversion/libsvn_subr subversion/svn
Posted by Karl Fogel <kf...@red-bean.com>.
Stefan Sperling <st...@elego.de> writes:
> Actually, storing 'store-plaintext-passwords' in 'servers' is *required*
> to fix the following bug I just realised exists (and is really silly
> once you realise it's there, I do really stupid things sometimes):
>
> In svn/main.c on the branch, we're currently grabbing URLs from the
> command line so we can use those to match server groups in the 'servers'
> file. The approach is totally naive and wrong. We're not even checking
> (and we can't, in main.c) which of the URLs we're actually connecting to
> (they might be part of, say, a log message argument), and on top of that,
> the URL we're connecting to might not even be retrieved from the command
> line! "svn ci ..." causes auth data to be cached for repos that require
> auth only for write operations, like our own repo does. Doh!
>
> So this is clearly an issue of "layer violation". 'store-plaintext-passwords'
> has to move out of 'config' and svn_cmdline_setup_auth_baton, and go into
> 'servers', and be evaluated by RA layers. This probably means that quite a
> bit of code will need to be refactored/rewritten on the branch :/
>
> Does anyone oppose moving the whole [auth] section from 'config' to
> 'servers' for consistency?
As long as we maintain compatibility...
But why is there a connection between which file the "[auth]" section
lives in (a user-visible question) and which URLs we use that data for
(an internal code-flow issue)?
The choice of which config file to use should be made based on what will
make most sense for users, who don't know or care how our code is
organized internally. It sounds like these two issues are maybe getting
mixed together, in your comments above?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30723 - in
branches/dont-save-plaintext-passwords-by-default: .
subversion/include subversion/libsvn_subr subversion/svn
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Apr 21, 2008 at 10:50:25PM +0200, Stefan Sperling wrote:
> On Mon, Apr 21, 2008 at 10:44:19AM -0700, David Glasser wrote:
> > On Mon, Apr 21, 2008 at 6:31 AM, C. Michael Pilato <cm...@collab.net> wrote:
> > > Stefan Sperling wrote:
> > >
> > > > On Sun, Apr 20, 2008 at 04:42:56PM -0700, David Glasser wrote:
> > > >
> > > > > Hmm, why not just have the "global" servers section be the normal
> > > > > place to configure this? Why use the other config file at all?
> > > > >
> > > >
> > > > Because most users will look at the 'config' file first, I guess.
> > > > Also, there, it's right next to 'store-passwords'.
> > > >
> > > > The docstrings reference each other, so I think it's sorta OK...
> > > >
> > > > The other idea I had was to have the option be valid in both
> > > > the config file and the servers file [global] section, and
> > > > have svn print a warning when the two disagree and fall back
> > > > to 'prompt'. But I discarded that as overkill.
> > > >
> > >
> > > That we ever had the store-passwords option in the 'config' file instead of
> > > in the 'servers' file (bound to RA-related things) might have been a
> > > mistake. Let's evaluate the correct location for this new option without
> > > concern for the dubious decisions of the past.
> >
> > +1
>
> Right, so if we stored 'store-plaintext-passwords' in 'servers' only,
> we should also move 'store-passwords' and 'store-auth-creds' to 'servers'.
> That is, the whole [auth] section of the 'config' file will go to the
> 'servers' file, with a comment in 'config' informing users about the move.
>
> Agreed?
Actually, storing 'store-plaintext-passwords' in 'servers' is *required*
to fix the following bug I just realised exists (and is really silly
once you realise it's there, I do really stupid things sometimes):
In svn/main.c on the branch, we're currently grabbing URLs from the
command line so we can use those to match server groups in the 'servers'
file. The approach is totally naive and wrong. We're not even checking
(and we can't, in main.c) which of the URLs we're actually connecting to
(they might be part of, say, a log message argument), and on top of that,
the URL we're connecting to might not even be retrieved from the command
line! "svn ci ..." causes auth data to be cached for repos that require
auth only for write operations, like our own repo does. Doh!
So this is clearly an issue of "layer violation". 'store-plaintext-passwords'
has to move out of 'config' and svn_cmdline_setup_auth_baton, and go into
'servers', and be evaluated by RA layers. This probably means that quite a
bit of code will need to be refactored/rewritten on the branch :/
Does anyone oppose moving the whole [auth] section from 'config' to
'servers' for consistency?
--
Stefan Sperling <st...@elego.de> Software Monkey
German law requires the following banner :(
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de CEO: Olaf Wagner
Store password unencrypted (yes/no)? No
Re: svn commit: r30723 - in
branches/dont-save-plaintext-passwords-by-default: .
subversion/include subversion/libsvn_subr subversion/svn
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Apr 21, 2008 at 10:44:19AM -0700, David Glasser wrote:
> On Mon, Apr 21, 2008 at 6:31 AM, C. Michael Pilato <cm...@collab.net> wrote:
> > Stefan Sperling wrote:
> >
> > > On Sun, Apr 20, 2008 at 04:42:56PM -0700, David Glasser wrote:
> > >
> > > > Hmm, why not just have the "global" servers section be the normal
> > > > place to configure this? Why use the other config file at all?
> > > >
> > >
> > > Because most users will look at the 'config' file first, I guess.
> > > Also, there, it's right next to 'store-passwords'.
> > >
> > > The docstrings reference each other, so I think it's sorta OK...
> > >
> > > The other idea I had was to have the option be valid in both
> > > the config file and the servers file [global] section, and
> > > have svn print a warning when the two disagree and fall back
> > > to 'prompt'. But I discarded that as overkill.
> > >
> >
> > That we ever had the store-passwords option in the 'config' file instead of
> > in the 'servers' file (bound to RA-related things) might have been a
> > mistake. Let's evaluate the correct location for this new option without
> > concern for the dubious decisions of the past.
>
> +1
Right, so if we stored 'store-plaintext-passwords' in 'servers' only,
we should also move 'store-passwords' and 'store-auth-creds' to 'servers'.
That is, the whole [auth] section of the 'config' file will go to the
'servers' file, with a comment in 'config' informing users about the move.
Agreed?
--
Stefan Sperling <st...@elego.de> Software Monkey
German law requires the following banner :(
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de CEO: Olaf Wagner
Store password unencrypted (yes/no)? No
Re: svn commit: r30723 - in branches/dont-save-plaintext-passwords-by-default: . subversion/include subversion/libsvn_subr subversion/svn
Posted by David Glasser <gl...@davidglasser.net>.
On Mon, Apr 21, 2008 at 6:31 AM, C. Michael Pilato <cm...@collab.net> wrote:
> Stefan Sperling wrote:
>
> > On Sun, Apr 20, 2008 at 04:42:56PM -0700, David Glasser wrote:
> >
> > > Hmm, why not just have the "global" servers section be the normal
> > > place to configure this? Why use the other config file at all?
> > >
> >
> > Because most users will look at the 'config' file first, I guess.
> > Also, there, it's right next to 'store-passwords'.
> >
> > The docstrings reference each other, so I think it's sorta OK...
> >
> > The other idea I had was to have the option be valid in both
> > the config file and the servers file [global] section, and
> > have svn print a warning when the two disagree and fall back
> > to 'prompt'. But I discarded that as overkill.
> >
>
> That we ever had the store-passwords option in the 'config' file instead of
> in the 'servers' file (bound to RA-related things) might have been a
> mistake. Let's evaluate the correct location for this new option without
> concern for the dubious decisions of the past.
+1
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30723 - in branches/dont-save-plaintext-passwords-by-default:
. subversion/include subversion/libsvn_subr subversion/svn
Posted by "C. Michael Pilato" <cm...@collab.net>.
Stefan Sperling wrote:
> On Sun, Apr 20, 2008 at 04:42:56PM -0700, David Glasser wrote:
>> Hmm, why not just have the "global" servers section be the normal
>> place to configure this? Why use the other config file at all?
>
> Because most users will look at the 'config' file first, I guess.
> Also, there, it's right next to 'store-passwords'.
>
> The docstrings reference each other, so I think it's sorta OK...
>
> The other idea I had was to have the option be valid in both
> the config file and the servers file [global] section, and
> have svn print a warning when the two disagree and fall back
> to 'prompt'. But I discarded that as overkill.
That we ever had the store-passwords option in the 'config' file instead of
in the 'servers' file (bound to RA-related things) might have been a
mistake. Let's evaluate the correct location for this new option without
concern for the dubious decisions of the past.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand