diff --git a/msal/oauth2cli/oauth2.py b/msal/oauth2cli/oauth2.py index 680c1bc5..f6b4cdb4 100644 --- a/msal/oauth2cli/oauth2.py +++ b/msal/oauth2cli/oauth2.py @@ -13,7 +13,7 @@ import base64 import sys import functools -import random +import secrets import string import hashlib @@ -277,8 +277,9 @@ def _scope_set(scope): def _generate_pkce_code_verifier(length=43): assert 43 <= length <= 128 + alphabet = string.ascii_letters + string.digits + "-._~" verifier = "".join( # https://tools.ietf.org/html/rfc7636#section-4.1 - random.sample(string.ascii_letters + string.digits + "-._~", length)) + secrets.choice(alphabet) for _ in range(length)) code_challenge = ( # https://tools.ietf.org/html/rfc7636#section-4.2 base64.urlsafe_b64encode(hashlib.sha256(verifier.encode("ascii")).digest()) @@ -488,7 +489,7 @@ def initiate_auth_code_flow( raise ValueError('response_type="token ..." is not allowed') pkce = _generate_pkce_code_verifier() flow = { # These data are required by obtain_token_by_auth_code_flow() - "state": state or "".join(random.sample(string.ascii_letters, 16)), + "state": state or "".join(secrets.choice(string.ascii_letters) for _ in range(16)), "redirect_uri": redirect_uri, "scope": scope, } diff --git a/msal/oauth2cli/oidc.py b/msal/oauth2cli/oidc.py index 01ee7894..1577c81a 100644 --- a/msal/oauth2cli/oidc.py +++ b/msal/oauth2cli/oidc.py @@ -1,7 +1,7 @@ import json import base64 import time -import random +import secrets import string import warnings import hashlib @@ -238,7 +238,7 @@ def initiate_auth_code_flow( # Here we just automatically add it. If the caller do not want id_token, # they should simply go with oauth2.Client. _scope.append("openid") - nonce = "".join(random.sample(string.ascii_letters, 16)) + nonce = "".join(secrets.choice(string.ascii_letters) for _ in range(16)) flow = super(Client, self).initiate_auth_code_flow( scope=_scope, nonce=_nonce_hash(nonce), **kwargs) flow["nonce"] = nonce diff --git a/tests/test_oauth2_security.py b/tests/test_oauth2_security.py new file mode 100644 index 00000000..92749b15 --- /dev/null +++ b/tests/test_oauth2_security.py @@ -0,0 +1,49 @@ +import unittest +from msal.oauth2cli import oauth2 +import string + +class TestTokenGenerationSecurity(unittest.TestCase): + """Verify secure token generation using secrets module.""" + + def test_pkce_verifier_format(self): + """Verify PKCE verifier is valid and uses correct alphabet.""" + pkce = oauth2._generate_pkce_code_verifier(length=43) + verifier = pkce["code_verifier"] + + # Check length + self.assertEqual(len(verifier), 43) + + # Check alphabet compliance (RFC 7636) + allowed_chars = set(string.ascii_letters + string.digits + "-._~") + self.assertTrue(all(c in allowed_chars for c in verifier)) + + # Verify code_challenge exists and is base64url encoded (no padding) + self.assertIn("code_challenge", pkce) + self.assertNotIn(b"=", pkce["code_challenge"]) + + def test_pkce_verifier_custom_length(self): + """Test PKCE verifier with valid custom lengths.""" + for length in [43, 64, 128]: + pkce = oauth2._generate_pkce_code_verifier(length=length) + self.assertEqual(len(pkce["code_verifier"]), length) + + def test_token_uniqueness(self): + """Verify generated tokens are unique (randomness test).""" + # Generate multiple tokens - should all be different + verifiers = [oauth2._generate_pkce_code_verifier()["code_verifier"] + for _ in range(10)] + self.assertEqual(len(set(verifiers)), 10, "Generated tokens should be unique") + + # Also test state generation + from msal.oauth2cli.oauth2 import Client + from unittest.mock import Mock + + client = Client( + server_configuration={"authorization_endpoint": "https://example.com/auth"}, + client_id="test-client", + http_client=Mock() + ) + + states = [client.initiate_auth_code_flow(scope=["test"])["state"] + for _ in range(10)] + self.assertEqual(len(set(states)), 10, "Generated states should be unique") \ No newline at end of file