You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jonathan Kamens <jo...@tamalesoftware.com> on 2008/02/13 23:43:27 UTC
CGI script for self-administering password in svnserve passwd files
(I'm not on the dev list, so please CC me on your response.)
Greetings,
We're in the process of setting up Subversion in-house, and we want to
use svnserve in stand-alone mode (don't want to use WebDAV because it's
slower than svnserve, and don't want to use SSH because it's slower, has
less access control and requires creating accounts for all the users on
the Subversion server).
When we started looking into how to set this up, we couldn't find any
way to allow users to change their own repository passwords. Since we
want users to be able to change their passwords, and since we don't want
them to have to come to us to do it, I wrote a little Perl CGI script to
allow them to do it themselves (so sue me, I haven't gotten around to
learning Python yet :-).
Perhaps we were wrong in believing that there was no supported way to do
this. Perhaps someone else has already written such a CGI script which
we simply missed in our search. But if neither of these things is true
:-), then I'm wondering if there's any interest in including this script
as a contributed tool in the Subversion distribution.
I've attached the script for your consideration. Any feedback would be
appreciated.
Jonathan Kamens
RE: CGI script for self-administering password in svnserve passwd files
Posted by Jonathan Kamens <jo...@tamalesoftware.com>.
Thanks for the response.
I don't agree that unlocking is unnecessary, since locks are released
automatically when a file handle is closed.
I'll talk to the muckitymucks here about whether I can use the BSD or
MIT license instead of GPL. I don't think it'll be a problem, but I'll
confirm.
I agree with all of your other proposed changes and will try to get them
done and submit a revised version of the script to the list ASAP. It
might not be for a week or two since we're feverishly working on
finishing up our migration to Svn and there's a tight deadline on that,
so the work on that project needs to take precedence.
The two code reviews I've gotten for this script from people on the list
illustrate one of the most important reasons why I like giving away the
software that I write whenever possible. If it's interesting to people,
I can be sure that I'm going to get feedback telling me exactly what I
can and should do to improve it :-).
Thanks,
jik
-----Original Message-----
From: james82@gmail.com [mailto:james82@gmail.com] On Behalf Of David
James
Sent: Friday, February 22, 2008 11:04 AM
To: Jonathan Kamens
Cc: dev@subversion.tigris.org
Subject: Re: CGI script for self-administering password in svnserve
passwd files
On Thu, Feb 21, 2008 at 12:42 PM, Jonathan Kamens
<jo...@tamalesoftware.com> wrote:
> Does the fact that there has been no response to the email below mean
> that there is no interest in distributing this CGI script with
> Subversion or that I have failed to attract the attention of the
people
> who would be involved in deciding whether to do so?
>
> If the latter, can anyone suggest how I might attract their
attention?
>
> Thanks,
>
> jik
Hi Jonathan,
This script looks like it'd be a useful thing to include in the
contrib directory. To get script this into the contribution you need a
committer to sponsor you for partial commit access. I could probably
do that for you but I think we need to clean up the script to make it
a bit easier to review. My suggestions are included below.
I only noticed two functional issues with the script:
1) It'd be a good idea to unlock $fh when you are done with it, per
http://perldoc.perl.org/functions/flock.html
2) It'd probably be a good idea to die if %repos isn't set, rather
than defaulting to some test repository.
License compatibility: Could you release your program under a more
permissive license, such as BSD/MIT? Just asking. (GPL license
compatibility issues are often a pain, and if you use a more
permissive license then we don't need to worry about that.)
I also have a few stylistic suggestions:
1) I think that the locking of $fh should be moved from
lock_password_file function into the update_password_file function.
When I first read the script I missed the fact that the two functions
are operating on the same global variable. After you do this, could
you make the "$fh" variable a local variable for the
"update_password_file" function? This will get rid of a (potentially
confusing) global variable.
2) I found it a bit confusing that all of the functions modify the
global variable '$error'. Could we instead update the functions to
call an appropriate function which prints out an error message (in
helpful HTML format, with any appropriate forms included) and exits?
3) Can we split the program into functions which represent the tasks
which need to be done? Also, let's get rid of the 'doit' function. See
below.
Here's a list of the tasks your script does:
- read_config: Read the raw config from disk and validate it.
- read_form_input: Read the form input from the user and validate it.
- check_password: Make sure that the user password is correct
- update_password_file: Update the password file
- print_html: Print the HTML form (and any errors or messages).
Maybe you could create functions for each of the above? Include a
short comment (similar to the above) at the top of each function.
Here's an example of what your updated program (with functions) might
look like:
my (%repos) = read_config();
my ($passwd_file, %input) = read_form_input(%repos);
my ($updated_password) = 0;
if ($passwd_file) {
check_password($passwd_file, %input);
update_password_file($passwd_file, %input);
$updated_password = 1;
}
print_html($updated_password, %input);
Of course your final program might look different -- I'm just trying
to give you an idea.
HTH,
David
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: CGI script for self-administering password in svnserve passwd files
Posted by Jonathan Kamens <jo...@tamalesoftware.com>.
David (et al),
I've updated my password-changing CGI script significantly based on your
feedback. The new version is attached. Please take a look and let me
know if you're ready to sponsor me so I can commit it into the contrib
area.
Thanks,
jik
-----Original Message-----
From: james82@gmail.com [mailto:james82@gmail.com] On Behalf Of David
James
Sent: Friday, February 22, 2008 11:04 AM
To: Jonathan Kamens
Cc: dev@subversion.tigris.org
Subject: Re: CGI script for self-administering password in svnserve
passwd files
On Thu, Feb 21, 2008 at 12:42 PM, Jonathan Kamens
<jo...@tamalesoftware.com> wrote:
> Does the fact that there has been no response to the email below mean
> that there is no interest in distributing this CGI script with
> Subversion or that I have failed to attract the attention of the
people
> who would be involved in deciding whether to do so?
>
> If the latter, can anyone suggest how I might attract their
attention?
>
> Thanks,
>
> jik
Hi Jonathan,
This script looks like it'd be a useful thing to include in the
contrib directory. To get script this into the contribution you need a
committer to sponsor you for partial commit access. I could probably
do that for you but I think we need to clean up the script to make it
a bit easier to review. My suggestions are included below.
I only noticed two functional issues with the script:
1) It'd be a good idea to unlock $fh when you are done with it, per
http://perldoc.perl.org/functions/flock.html
2) It'd probably be a good idea to die if %repos isn't set, rather
than defaulting to some test repository.
License compatibility: Could you release your program under a more
permissive license, such as BSD/MIT? Just asking. (GPL license
compatibility issues are often a pain, and if you use a more
permissive license then we don't need to worry about that.)
I also have a few stylistic suggestions:
1) I think that the locking of $fh should be moved from
lock_password_file function into the update_password_file function.
When I first read the script I missed the fact that the two functions
are operating on the same global variable. After you do this, could
you make the "$fh" variable a local variable for the
"update_password_file" function? This will get rid of a (potentially
confusing) global variable.
2) I found it a bit confusing that all of the functions modify the
global variable '$error'. Could we instead update the functions to
call an appropriate function which prints out an error message (in
helpful HTML format, with any appropriate forms included) and exits?
3) Can we split the program into functions which represent the tasks
which need to be done? Also, let's get rid of the 'doit' function. See
below.
Here's a list of the tasks your script does:
- read_config: Read the raw config from disk and validate it.
- read_form_input: Read the form input from the user and validate it.
- check_password: Make sure that the user password is correct
- update_password_file: Update the password file
- print_html: Print the HTML form (and any errors or messages).
Maybe you could create functions for each of the above? Include a
short comment (similar to the above) at the top of each function.
Here's an example of what your updated program (with functions) might
look like:
my (%repos) = read_config();
my ($passwd_file, %input) = read_form_input(%repos);
my ($updated_password) = 0;
if ($passwd_file) {
check_password($passwd_file, %input);
update_password_file($passwd_file, %input);
$updated_password = 1;
}
print_html($updated_password, %input);
Of course your final program might look different -- I'm just trying
to give you an idea.
HTH,
David
Re: CGI script for self-administering password in svnserve passwd files
Posted by David Glasser <gl...@davidglasser.net>.
jik pinged me again about his script.
It strikes me as a useful script, but I am concerned that in a
relatively cursory scan of his first draft I found several serious
security holes. While our standard for contrib scripts is low, I do
worry about distributing insecure CGIs in our tarball. He fixed
everything I pointed out, but I haven't gotten a chance to look at his
latest version, and more eyes is better.
I do think that it would be a useful thing to have in contrib, but I'd
be much more comfortable if somebody else did a security audit first.
--dave
2008/3/27 Jonathan Kamens <jo...@tamalesoftware.com>:
> Actually, there was a bug in the version I just sent out, which I
> realized while walking to the bus :-). Attached is a fixed version.
>
> jik
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
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: CGI script for self-administering password in svnserve passwd files
Posted by Jonathan Kamens <jo...@tamalesoftware.com>.
Actually, there was a bug in the version I just sent out, which I
realized while walking to the bus :-). Attached is a fixed version.
jik
Re: CGI script for self-administering password in svnserve passwd files
Posted by David James <ja...@cs.toronto.edu>.
On Thu, Feb 21, 2008 at 12:42 PM, Jonathan Kamens
<jo...@tamalesoftware.com> wrote:
> Does the fact that there has been no response to the email below mean
> that there is no interest in distributing this CGI script with
> Subversion or that I have failed to attract the attention of the people
> who would be involved in deciding whether to do so?
>
> If the latter, can anyone suggest how I might attract their attention?
>
> Thanks,
>
> jik
Hi Jonathan,
This script looks like it'd be a useful thing to include in the
contrib directory. To get script this into the contribution you need a
committer to sponsor you for partial commit access. I could probably
do that for you but I think we need to clean up the script to make it
a bit easier to review. My suggestions are included below.
I only noticed two functional issues with the script:
1) It'd be a good idea to unlock $fh when you are done with it, per
http://perldoc.perl.org/functions/flock.html
2) It'd probably be a good idea to die if %repos isn't set, rather
than defaulting to some test repository.
License compatibility: Could you release your program under a more
permissive license, such as BSD/MIT? Just asking. (GPL license
compatibility issues are often a pain, and if you use a more
permissive license then we don't need to worry about that.)
I also have a few stylistic suggestions:
1) I think that the locking of $fh should be moved from
lock_password_file function into the update_password_file function.
When I first read the script I missed the fact that the two functions
are operating on the same global variable. After you do this, could
you make the "$fh" variable a local variable for the
"update_password_file" function? This will get rid of a (potentially
confusing) global variable.
2) I found it a bit confusing that all of the functions modify the
global variable '$error'. Could we instead update the functions to
call an appropriate function which prints out an error message (in
helpful HTML format, with any appropriate forms included) and exits?
3) Can we split the program into functions which represent the tasks
which need to be done? Also, let's get rid of the 'doit' function. See
below.
Here's a list of the tasks your script does:
- read_config: Read the raw config from disk and validate it.
- read_form_input: Read the form input from the user and validate it.
- check_password: Make sure that the user password is correct
- update_password_file: Update the password file
- print_html: Print the HTML form (and any errors or messages).
Maybe you could create functions for each of the above? Include a
short comment (similar to the above) at the top of each function.
Here's an example of what your updated program (with functions) might look like:
my (%repos) = read_config();
my ($passwd_file, %input) = read_form_input(%repos);
my ($updated_password) = 0;
if ($passwd_file) {
check_password($passwd_file, %input);
update_password_file($passwd_file, %input);
$updated_password = 1;
}
print_html($updated_password, %input);
Of course your final program might look different -- I'm just trying
to give you an idea.
HTH,
David
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: CGI script for self-administering password in svnserve passwd files
Posted by Jonathan Kamens <jo...@tamalesoftware.com>.
Does the fact that there has been no response to the email below mean
that there is no interest in distributing this CGI script with
Subversion or that I have failed to attract the attention of the people
who would be involved in deciding whether to do so?
If the latter, can anyone suggest how I might attract their attention?
Thanks,
jik
-----Original Message-----
From: Jonathan Kamens
Sent: Thursday, February 14, 2008 11:37 AM
To: dev@subversion.tigris.org
Subject: Re: CGI script for self-administering password in svnserve
passwd files
Thanks for the code review!
On 02/13/2008 10:54 PM, David Glasser wrote:
> Big security hole: you take the "username" parameter directly from
> user input and interpolate it into a regexp. Don't do that :-)
>
Right you are. Fixed.
> Also, you probably want to update the file atomically (with a temp
> file and a rename). Wouldn't want svnserve to read a half-written
> file...
>
I had resisted doing this because I didn't want to require that the
password file be in its own directory that's writeable by apache, but I
suppose you're right that it's necessary, so fixed.
> And looks like if the user doesn't pass in a "repo" parameter,
> $passwd_file isn't defined... exciting times.
Not really exciting times, just an internal error, but I've now made
this more explicit.
> Not to mention the
> errors that happen if various parameters are the string "0" (though
> admittedly that's a rather poor username or password).
>
Wow, you're paranoid :-). Fixed, I believe.
I also added a few additional test cases and made the test suite work
again (some last-minute changes I made before posting the script broke
the tests).
I also added "-T" to the #! line to enable taint checks, for a slightly
higher level of paranoia.
A new version is attached, along with a diff. Please take a look.
Is there any interest in shipping this script with the distribution? If
there is, I'd be happy to commit to maintaining it.
jik
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: CGI script for self-administering password in svnserve passwd
files
Posted by Jonathan Kamens <Jo...@tamalesoftware.com>.
Thanks for the code review!
On 02/13/2008 10:54 PM, David Glasser wrote:
> Big security hole: you take the "username" parameter directly from
> user input and interpolate it into a regexp. Don't do that :-)
>
Right you are. Fixed.
> Also, you probably want to update the file atomically (with a temp
> file and a rename). Wouldn't want svnserve to read a half-written
> file...
>
I had resisted doing this because I didn't want to require that the
password file be in its own directory that's writeable by apache, but I
suppose you're right that it's necessary, so fixed.
> And looks like if the user doesn't pass in a "repo" parameter,
> $passwd_file isn't defined... exciting times.
Not really exciting times, just an internal error, but I've now made
this more explicit.
> Not to mention the
> errors that happen if various parameters are the string "0" (though
> admittedly that's a rather poor username or password).
>
Wow, you're paranoid :-). Fixed, I believe.
I also added a few additional test cases and made the test suite work
again (some last-minute changes I made before posting the script broke
the tests).
I also added "-T" to the #! line to enable taint checks, for a slightly
higher level of paranoia.
A new version is attached, along with a diff. Please take a look.
Is there any interest in shipping this script with the distribution? If
there is, I'd be happy to commit to maintaining it.
jik
Re: CGI script for self-administering password in svnserve passwd files
Posted by David Glasser <gl...@davidglasser.net>.
2008/2/13 Jonathan Kamens <jo...@tamalesoftware.com>:
>
>
>
>
> (I'm not on the dev list, so please CC me on your response.)
>
>
>
> Greetings,
>
>
>
> We're in the process of setting up Subversion in-house, and we want to use
> svnserve in stand-alone mode (don't want to use WebDAV because it's slower
> than svnserve, and don't want to use SSH because it's slower, has less
> access control and requires creating accounts for all the users on the
> Subversion server).
>
>
>
> When we started looking into how to set this up, we couldn't find any way to
> allow users to change their own repository passwords. Since we want users
> to be able to change their passwords, and since we don't want them to have
> to come to us to do it, I wrote a little Perl CGI script to allow them to do
> it themselves (so sue me, I haven't gotten around to learning Python yet
> :-).
>
>
>
> Perhaps we were wrong in believing that there was no supported way to do
> this. Perhaps someone else has already written such a CGI script which we
> simply missed in our search. But if neither of these things is true :-),
> then I'm wondering if there's any interest in including this script as a
> contributed tool in the Subversion distribution.
>
>
>
> I've attached the script for your consideration. Any feedback would be
> appreciated.
Big security hole: you take the "username" parameter directly from
user input and interpolate it into a regexp. Don't do that :-)
Also, you probably want to update the file atomically (with a temp
file and a rename). Wouldn't want svnserve to read a half-written
file...
And looks like if the user doesn't pass in a "repo" parameter,
$passwd_file isn't defined... exciting times. Not to mention the
errors that happen if various parameters are the string "0" (though
admittedly that's a rather poor username or password).
--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: CGI script for self-administering password in svnserve passwd
files
Posted by John Peacock <jo...@havurah-software.org>.
Jonathan Kamens wrote:
> I think from what you wrote below, you may actually be suggesting to put
> all the users' public keys into one account and have all the users
> access via that account, which would avoid both of the issues noted
> above but isn't really what the FAQ answer to which you referred says to do.
Yes, I forgot that the FAQ doesn't spell that out in that fashion (I
think I even wrote a patch once to change that). Yes, the files are all
owned by the svn service user, so there are no rights issues at all.
> We're only planning on allowing Subversion access to people who are
> either on our LAN or VPN'd in, so I'm not sure we want to take on the
> overhead of requiring SSH to access the repository.
Since you didn't describe that you were only allowing LAN-local access,
then there isn't any point in going to svn+ssh; it is really so much
easier/more secure when most of your access is from remote users.
John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: CGI script for self-administering password in svnserve passwd
files
Posted by Jonathan Kamens <Jo...@tamalesoftware.com>.
This requires making sure that the files in the repo are all assigned to
a Subversion group and all users who are supposed to be able to access
the repo are members of that group as well, right?
Are there potential umask issues, or does Subversion take care of
ensuring that group permission bits on the files and directories in the
repository aren't messed up by user umasks?
I think from what you wrote below, you may actually be suggesting to put
all the users' public keys into one account and have all the users
access via that account, which would avoid both of the issues noted
above but isn't really what the FAQ answer to which you referred says to do.
In any case, there's still a performance issue, since SSH's encryption
and small TCP/IP windows (except in current development SSH sources and
custom SSH binaries patched with the hpn-ssh patches) make transfers
over LANs much, much slower than they should be.
We're only planning on allowing Subversion access to people who are
either on our LAN or VPN'd in, so I'm not sure we want to take on the
overhead of requiring SSH to access the repository.
Thanks for the info!
jik
On 02/14/2008 06:56 AM, John Peacock wrote:
> Jonathan Kamens wrote:
>
>> We’re in the process of setting up Subversion in-house, and we want to
>> use svnserve in stand-alone mode (don’t want to use WebDAV because it’s
>> slower than svnserve, and don’t want to use SSH because it’s slower, has
>> less access control and requires creating accounts for all the users on
>> the Subversion server).
>>
>
> Just FYI, there is absolutely no reason to eliminate svv+ssh from consideration
> because you don't want to give all users logins:
>
> http://subversion.tigris.org/faq.html#ssh-svnserve-location
>
> That is actually a much more secure methodology, since it doesn't require the
> user to remember any password - other than their private key's, if they set one
> - which is much more secure than any password.
>
> And doing it that way has no effect on whether you use the internal access
> control available to svnserve.
>
>
> John
>
Re: CGI script for self-administering password in svnserve passwd
files
Posted by John Peacock <jo...@havurah-software.org>.
Jonathan Kamens wrote:
> We’re in the process of setting up Subversion in-house, and we want to
> use svnserve in stand-alone mode (don’t want to use WebDAV because it’s
> slower than svnserve, and don’t want to use SSH because it’s slower, has
> less access control and requires creating accounts for all the users on
> the Subversion server).
Just FYI, there is absolutely no reason to eliminate svv+ssh from consideration
because you don't want to give all users logins:
http://subversion.tigris.org/faq.html#ssh-svnserve-location
That is actually a much more secure methodology, since it doesn't require the
user to remember any password - other than their private key's, if they set one
- which is much more secure than any password.
And doing it that way has no effect on whether you use the internal access
control available to svnserve.
John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org