Merge lp:~vila/bzr/1606203-long-auth into lp:bzr/2.7

[Vincent Ladeuil]

Fix lp:1606203 caused by using a user/pass combination for http auth longer than ~57 chars. (Vincent Ladeuil)

This fixes bug #1606203 caused by using a user/pass combination for http auth longer than ~57 chars.

[Colin Watson (cjwatson)]

review: Approve

[Richard Wilbur (richard-wilbur)]

Vincent, What did the problem turn out to actually involve? Was it credentials "longer than ~57 chars" as the bug says or "creating a header value with an embedded '\n'" as a comment in the patch says? Or was it a combination of the two?

If it was the embedded '\n', how did that get in there?

[Colin Watson (cjwatson)]

str.encode('base64') is equivalent to base64.encodestring(str), which is a legacy interface (https://docs.python.org/2/library/base64.html#base64.encodestring) that returns "one or more lines of base64-encoded data". In particular, it splits the input string into 57-character chunks and encodes each one using binascii.b2a_base64, which adds a newline after each encoded chunk. Thus, if the input string is longer than 57 characters, str.encode('base64').strip() will end up containing embedded newlines.

base64.encodestring exists to encode strings according to RFC 1521's base64 Content-Transfer-Encoding, which has a 76-character limit for each encoded line. It does not make sense to use this for HTTP headers. For that, the base64 module's modern interface, which encodes according to RFC 3548, is more appropriate; and Vincent's patch switches to that.

[Richard Wilbur (richard-wilbur)]

@Colin, thanks for the explanation.
@Vincent, thanks for the patch.
+1

review: Approve

[Richard Wilbur (richard-wilbur)]

sent to pqm by email

=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py	2016-02-01 18:06:32 +0000
+++ bzrlib/tests/test_http.py	2016-09-09 13:24:23 +0000
@@ -260,6 +260,16 @@
         self.assertEqual('basic', scheme)
         self.assertEqual('realm="Thou should not pass"', remainder)
 
+    def test_build_basic_header_with_long_creds(self):
+        handler = _urllib2_wrappers.BasicAuthHandler()
+        user = 'user' * 10  # length 40
+        password = 'password' * 5  # length 40
+        header = handler.build_auth_header(
+            dict(user=user, password=password), None)
+        # https://bugs.launchpad.net/bzr/+bug/1606203 was caused by incorrectly
+        # creating a header value with an embedded '\n'
+        self.assertFalse('\n' in header)
+
     def test_basic_extract_realm(self):
         scheme, remainder = self.parse_header(
             'Basic realm="Thou should not pass"',

=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
--- bzrlib/transport/http/_urllib2_wrappers.py	2016-01-31 12:55:31 +0000
+++ bzrlib/transport/http/_urllib2_wrappers.py	2016-09-09 13:24:23 +0000
@@ -48,6 +48,7 @@
 # actual code more or less do that, tests should be written to
 # ensure that.
 
+import base64
 import errno
 import httplib
 import os
@@ -1491,7 +1492,7 @@
 
     def build_auth_header(self, auth, request):
         raw = '%s:%s' % (auth['user'], auth['password'])
-        auth_header = 'Basic ' + raw.encode('base64').strip()
+        auth_header = 'Basic ' + base64.b64encode(raw)
         return auth_header
 
     def extract_realm(self, header_value):