If my understanding is correct, it seems the current code on the gpg-agent
branch essentially uses the gpg-agent as a glorified password prompt.
Here's what the code seems to be doing:
Storing a password:
Nothing happens.
Retreiving a password:
Step 1: svn contacts the gpg-agent to find out the passphrase for the private
PGP key the agent is managing
Step 2: svn treats this passphrase as the repository password, and sends
it to the server.
This is very wrong, if not outright dangerous.
Nobody should use their PGP passphrase as a Subversion password.
The passphrase is sacred, and svn should not ever send it to the server.
It could be easily leaked in case someone is using http:// to connect
to a repository, for instance. This behaviour of svn can lead to the private
PGP key being compromised.
Here's what I think should happen instead:
Storing a password:
Step 1: svn encrypts the password using the user's public PGP key and
stores the encrypted form somewhere in the ~/.subversion/auth area
Retreiving a password:
Step 1: svn contacts the gpg-agent to find out the passphrase for the
private pgp key the agent is managing. If the agent cannot be
contacted svn asks the user for the passphrase.
Step 2: svn uses this passphrase to decrypt the user's PGP private key
Step 3: svn uses this private key to decrypt the password stored
inside the ~/.subversion/auth area
Step 4: svn sends the decrypted password to the server
The GPGME library will probably help with these steps:
http://www.gnupg.org/gpgme.html
I haven't checked, but it's possible that this library will talk to
the GPG agent on behalf of Subversion.
Also, it could happen that the block of memory containing the passphrase
is swapped out to disk. This should be prevented if at all possible,
e.g. by using mlock(). The passphrase should be overwritten with random
data right after it has been used.
The feature could be renamed from "gpg-agent password store" to
"gpgme password store" or "pgp-encrypted password store", to take
account of the fact that the gpg-agent itself doesn't provide encryption
services.
I'm strongly -1 on merging the branch to trunk in its current form.
I'm sorry I didn't see this problem during my initial review of the code.
Thanks,
Stefan
Received on 2010-10-17 16:03:54 CEST