Merge lp:~florian-dorn/bzr/Base64CredentialStore into lp:bzr

[Florian Dorn]

Implements a base64 credential store (fixes bug #788015), for obfruscating passwords in authentication.conf

[Martin Pool (mbp)]

Hi, Florian, thanks for this contribution.

Could you please complete <http://www.canonical.com/contributors>?

[question] I wonder how people will discover they can use this. It seems to me we ought to update at least the developer docs that say this is not implemented; add it to news and whatsnew; also perhaps add it into the user guide about authentication. Perhaps this should even become the default?

+ __doc__ = """Base64 credential store for the authentication.conf file"""

[fix] A literal string occurring as the first thing in a class or other block is automatically the docstring in Python.

[tweak] Could you please mention here that the point of this class is to obfuscate the passwords stored in the file?

[fix] This ought to be tested; there is a test_decode_password and TestPlainTextCredentialStore in tests/test_config.py. If you're having trouble adding one just ask and we will help or do it.

I wonder how people are going to discover that this exists? I think

review: Needs Fixing

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 6/6/2011 4:46 AM, Martin Pool wrote:
> Review: Needs Fixing
> Hi, Florian, thanks for this contribution.
>
> Could you please complete <http://www.canonical.com/contributors>?
>
> [question] I wonder how people will discover they can use this. It seems to me we ought to update at least the developer docs that say this is not implemented; add it to news and whatsnew; also perhaps add it into the user guide about authentication. Perhaps this should even become the default?
>
> + __doc__ = """Base64 credential store for the authentication.conf file"""

>
> [fix] A literal string occurring as the first thing in a class or other block is automatically the docstring in Python.

We use "__doc__" for Command classes because -OO doesn't strip it out.
We might be using it similarly here. If a doc string ends up in 'bzr
help' text, we want to use __doc__ = ...

>
> [tweak] Could you please mention here that the point of this class is to obfuscate the passwords stored in the file?
>
> [fix] This ought to be tested; there is a test_decode_password and TestPlainTextCredentialStore in tests/test_config.py. If you're having trouble adding one just ask and we will help or do it.
>
> I wonder how people are going to discover that this exists? I think

Would it show up under "bzr help authentication"? If not, it probably
should.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3s43YACgkQJdeBCYSNAAOKHACeN0ZHUveykAFo9dckb8e0tkMm
RP4AoNiJSYE7DR8dJx1zD+1QB+pGkKjb
=D6By
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

@Florian: Do you need help here ?

How do you use this store ? You put your encoded credentials into authentication.conf manually ?

review: Needs Information

[Florian Dorn (florian-dorn)]

@Vincent:
the base64 encoded password can be generated with:
echo -n "secret" | base64
and put there manually

[Martin Pool (mbp)]

On 7 June 2011 00:25, John Arbash Meinel <email address hidden> wrote:

>> [question] I wonder how people will discover they can use this.  It seems to me we ought to update at least the developer docs that say this is not implemented; add it to news and whatsnew; also perhaps add it into the user guide about authentication.  Perhaps this should even become the default?
>>
>> +    __doc__ = """Base64 credential store for the authentication.conf file"""
>
>
>>
>> [fix] A literal string occurring as the first thing in a class or other block is automatically the docstring in Python.
>
> We use "__doc__" for Command classes because -OO doesn't strip it out.
> We might be using it similarly here. If a doc string ends up in 'bzr
> help' text, we want to use __doc__ = ...

[aside] Right, but if this is going to be user documentation it ought
to be written in a different way, describing how to use it, not what
the class is. The parent AuthenticationConfig doesn't declare an
explicity __doc__. Perhaps we should move away from that to say
instead help="""..."""

[fix] At the moment there is "bzr help authentication" apparently
taken from a static file, and that ought to be updated to explain how
to use it.

[Vincent Ladeuil (vila)]

> @Vincent:
> the base64 encoded password can be generated with:
> echo -n "secret" | base64
> and put there manually

Great, that's worth adding to the doc (you need to specify password_encoding=base64 too right ?).

I'll put the proposal in the 'Work In Progress' state. When you want it to be reviewed again, just push your new changes to lp and put the proposal back to the 'Needs Review' state.

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2012-03-29 12:27:52 +0000
+++ bzrlib/config.py	2012-04-03 15:08:28 +0000
@@ -76,6 +76,7 @@
 from cStringIO import StringIO
 import os
 import sys
+import base64
 
 import bzrlib
 from bzrlib.decorators import needs_write_lock
@@ -2130,6 +2131,15 @@
                                    help=PlainTextCredentialStore.__doc__)
 credential_store_registry.default_key = 'plain'
 
+class Base64CredentialStore(CredentialStore):
+    __doc__ = """Base64 credential store for the authentication.conf file"""
+    
+    def decode_password(self, credentials):
+        """See CredentialStore.decode_password."""
+        return base64.decodestring(credentials['password'])
+
+credential_store_registry.register('base64', Base64CredentialStore,
+                                   help=Base64CredentialStore.__doc__)
 
 class BzrDirConfig(object):
 

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2012-03-29 12:27:52 +0000
+++ bzrlib/tests/test_config.py	2012-04-03 15:08:28 +0000
@@ -4815,6 +4815,14 @@
         decoded = plain_text.decode_password(dict(password='secret'))
         self.assertEquals('secret', decoded)
 
+class TestBase64CredentialStore(tests.TestCase):
+
+    def test_decode_password(self):
+        import base64
+        r = config.credential_store_registry
+        plain_text = r.get_credential_store('base64')
+        decoded = plain_text.decode_password(dict(password=base64.encodestring('secret-pass')))
+        self.assertEquals('secret-pass', decoded)
 
 # FIXME: Once we have a way to declare authentication to all test servers, we
 # can implement generic tests.

=== modified file 'doc/developers/authentication-ring.txt'
--- doc/developers/authentication-ring.txt	2010-11-12 22:46:28 +0000
+++ doc/developers/authentication-ring.txt	2012-04-03 15:08:28 +0000
@@ -158,7 +158,7 @@
 
 Encoding passwords in ``base64``, while weak, provides protection against
 accidental reading (if an administrator have to look into the file, he will not
-see the passwords in clear).(Not implemented yet).
+see the passwords in clear).
 
 This specification intends to ease the authentication providing, not to secure
 it in the best possible way.
@@ -266,6 +266,7 @@
         scheme=https
         host=home.net
         user=joe
+        # Obtain the base64 encoded password by running 'echo -n "secret-pass" | base64'
         password='c2VjcmV0LXBhc3M='
         password_encoding=base64
         verify_certificates=no # Still searching a free certificate provider