Repository: globus/globus-sdk-python
Pull request #154 (2017-03-08 05:23:07)

Improve Native App PKCE support
* Generate `code_verifier` and `code_challenge` strings that are RFC 7636 compliant:
  * 43-128 characters in length.
  * Contain only the following characters: `a-z`, `A-Z`, `-`, `_`, `.`, `~`.

Needs to wait until Auth handles code verifiers without the padding.




Patch:
diff --git a/globus_sdk/auth/oauth2_native_app.py b/globus_sdk/auth/oauth2_native_app.py
index daa8f4d7..b32bad8e 100644
--- a/globus_sdk/auth/oauth2_native_app.py
+++ b/globus_sdk/auth/oauth2_native_app.py
@@ -1,7 +1,8 @@
 import logging
-import uuid
 import hashlib
 import base64
+import re
+import os
 from six.moves.urllib.parse import urlencode
 
 from globus_sdk.base import slash_join
@@ -18,18 +19,38 @@ def make_native_app_challenge(verifier=None):
     of it. The challenge is sent at the start of the flow, and the secret is
     sent at the end, proving that the same client that started the flow is
     continuing it.
-    Hashing is always done with simple SHA256
+
+    Hashing is always done with simple SHA256.
+
+    See RFC 7636 for details.
+
+    **Parameters**
+
+        ``verifier`` (*string*)
+        The code verifier string used to construct the code challenge.
+        Must be at least 43 characters long and not longer than 128 characters.
+        Must only contain the following characters: [a-zA-Z0-9~_.-].
     """
-    if not verifier:
+
+    if verifier:
+        if not 43 <= len(verifier) <= 128:
+            raise ValueError(
+                'verifier must be 43-128 characters long: {}'.format(
+                    len(verifier)))
+        if bool(re.search(r'[^a-zA-Z0-9~_.-]', verifier)):
+            raise ValueError('verifier contained invalid characters')
+    else:
         logger.info(('Autogenerating verifier secret. On low-entropy systems '
                      'this may be insecure'))
 
-    # unless provided, the "secret" is just a UUID4
-    code_verifier = verifier or str(uuid.uuid4())
+    code_verifier = (verifier or
+                     base64.urlsafe_b64encode(
+                         os.urandom(32)).decode('utf-8').rstrip('='))
     # hash it, pull out a digest
     hashed_verifier = hashlib.sha256(code_verifier.encode('utf-8')).digest()
-    # urlsafe base64 encode that hash
-    code_challenge = base64.urlsafe_b64encode(hashed_verifier).decode('utf-8')
+    # urlsafe base64 encode that hash and strip the padding
+    code_challenge = base64.urlsafe_b64encode(
+        hashed_verifier).decode('utf-8').rstrip('=')
 
     # return the verifier and the encoded hash
     return code_verifier, code_challenge


Test patch:
diff --git a/tests/integration/test_auth_client_flow.py b/tests/integration/test_auth_client_flow.py
index fd5f1f4a..c6e8ffa3 100644
--- a/tests/integration/test_auth_client_flow.py
+++ b/tests/integration/test_auth_client_flow.py
@@ -40,12 +40,12 @@ class AuthClientIntegrationTests(CapturedIOTestCase):
         # starting flow with specified paramaters
         flow_manager = globus_sdk.auth.GlobusNativeAppFlowManager(
             ac, requested_scopes="scopes", redirect_uri="uri",
-            state="state", verifier="verifier", refresh_tokens=True)
+            state="state", verifier=("a" * 43), refresh_tokens=True)
         ac.current_oauth2_flow_manager = flow_manager
 
         # get url_and validate results
         url_res = ac.oauth2_get_authorize_url()
-        verifier, remade_challenge = make_native_app_challenge("verifier")
+        verifier, remade_challenge = make_native_app_challenge("a" * 43)
         expected_vals = [ac.base_url + "v2/oauth2/authorize?",
                          "client_id=" + ac.client_id,
                          "redirect_uri=" + "uri",
diff --git a/tests/integration/test_native_client_flow.py b/tests/integration/test_native_client_flow.py
index e9e09cbc..c07a8c40 100644
--- a/tests/integration/test_native_client_flow.py
+++ b/tests/integration/test_native_client_flow.py
@@ -65,7 +65,7 @@ class NativeAppAuthClientIntegrationTests(CapturedIOTestCase):
         # confirms flow initialized with specified values
         flow = self.nac.oauth2_start_flow(
             requested_scopes="scopes", redirect_uri="uri",
-            state="state", verifier="verifier", refresh_tokens=True)
+            state="state", verifier=("v" * 43), refresh_tokens=True)
         self.assertIsInstance(flow, GlobusNativeAppFlowManager)
         self.assertEqual(flow.redirect_uri, "uri")
         self.assertEqual(flow.requested_scopes, "scopes")
@@ -74,7 +74,7 @@ class NativeAppAuthClientIntegrationTests(CapturedIOTestCase):
 
         # confirm client can get url via flow
         url_res = self.nac.oauth2_get_authorize_url()
-        verifier, remade_challenge = make_native_app_challenge("verifier")
+        verifier, remade_challenge = make_native_app_challenge("v" * 43)
         expected_vals = [self.nac.base_url + "v2/oauth2/authorize?",
                          "client_id=" + self.nac.client_id,
                          "redirect_uri=" + "uri",
diff --git a/tests/unit/test_oauth2_authorization_code.py b/tests/unit/test_oauth2_authorization_code.py
index 1e0a46b8..5c531719 100644
--- a/tests/unit/test_oauth2_authorization_code.py
+++ b/tests/unit/test_oauth2_authorization_code.py
@@ -20,7 +20,7 @@ class GlobusAuthorizationCodeFlowManagerTests(CapturedIOTestCase):
         self.ac.base_url = "base_url/"
         self.flow_manager = globus_sdk.auth.GlobusNativeAppFlowManager(
             self.ac, requested_scopes="scopes", redirect_uri="uri",
-            state="state", verifier="verifier")
+            state="state")
 
     def test_get_authorize_url(self):
         """
diff --git a/tests/unit/test_oauth2_native_app.py b/tests/unit/test_oauth2_native_app.py
index 0826a81f..fb060c90 100644
--- a/tests/unit/test_oauth2_native_app.py
+++ b/tests/unit/test_oauth2_native_app.py
@@ -24,7 +24,7 @@ class GlobusNativeAppFlowManagerTests(CapturedIOTestCase):
         self.ac.base_url = "base_url/"
         self.flow_manager = globus_sdk.auth.GlobusNativeAppFlowManager(
             self.ac, requested_scopes="scopes", redirect_uri="uri",
-            state="state", verifier="verifier")
+            state="state")
 
     def test_make_native_app_challenge(self):
         """
@@ -32,27 +32,46 @@ class GlobusNativeAppFlowManagerTests(CapturedIOTestCase):
         Sanity checks results
         """
         # with verifier
-        input_verifier = "verifier"
+        input_verifier = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFG"
         output_verifier, challenge = make_native_app_challenge(
             input_verifier)
 
         # sanity check verifier equality and hashing
         self.assertEqual(input_verifier, output_verifier)
         re_hash = hashlib.sha256(input_verifier.encode('utf-8')).digest()
-        remade_challenge = base64.urlsafe_b64encode(re_hash).decode('utf-8')
+        remade_challenge = base64.urlsafe_b64encode(
+            re_hash).decode('utf-8').rstrip('=')
         self.assertEqual(challenge, remade_challenge)
 
         # without verifier
         verifier, challenge = make_native_app_challenge()
 
         # sanity check verifier format and hashing
-        self.assertEqual(len(verifier), 36)
-        for i in [8, 13, 18, 23]:
-            self.assertEqual(verifier[i], "-")
+        self.assertEqual(len(verifier), 43)
+        self.assertNotIn('=', verifier)
+
         re_hash = hashlib.sha256(verifier.encode('utf-8')).digest()
-        remade_challenge = base64.urlsafe_b64encode(re_hash).decode('utf-8')
+        remade_challenge = base64.urlsafe_b64encode(
+            re_hash).decode('utf-8').rstrip('=')
         self.assertEqual(challenge, remade_challenge)
 
+    def test_make_native_app_challenge_invalid_verifier(self):
+        """
+        Should raise a ValueError if passed-in verifier is an
+        invalid length or contains an invalid character.
+        """
+        invalid_verifiers = [
+            's' * 42,
+            'l' * 129,
+            'a valid length but contains invalid characters=',
+            '=abcdefghijklmnopqrstuvwxyz0123456789ABCDEFG',
+            '.',
+            '~_.'
+        ]
+
+        for verifier in invalid_verifiers:
+            self.assertRaises(ValueError, make_native_app_challenge, verifier)
+
     def test_get_authorize_url(self):
         """
         Creates an authorize url, confirms results match object values
@@ -88,7 +107,7 @@ class GlobusNativeAppFlowManagerTests(CapturedIOTestCase):
         """
         flow_with_prefill = globus_sdk.auth.GlobusNativeAppFlowManager(
             self.ac, requested_scopes="scopes", redirect_uri="uri",
-            state="state", verifier="verifier", prefill_named_grant="test")
+            state="state", prefill_named_grant="test")
 
         authorize_url = flow_with_prefill.get_authorize_url()
 
@@ -96,7 +115,7 @@ class GlobusNativeAppFlowManagerTests(CapturedIOTestCase):
 
         flow_without_prefill = globus_sdk.auth.GlobusNativeAppFlowManager(
             self.ac, requested_scopes="scopes", redirect_uri="uri",
-            state="state", verifier="verifier")
+            state="state")
 
         authorize_url = flow_without_prefill.get_authorize_url()