Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added some fixes related to algorithm and key choice #109

Merged
merged 6 commits into from
Mar 18, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion jwt/algorithms.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import hmac

from .compat import constant_time_compare, string_types, text_type
from .exceptions import InvalidKeyError

try:
from cryptography.hazmat.primitives import interfaces, hashes
Expand Down Expand Up @@ -68,7 +69,13 @@ class NoneAlgorithm(Algorithm):
operations are required.
"""
def prepare_key(self, key):
return None
if key == '':
key = None

if key is not None:
raise InvalidKeyError('When alg = "none", key value must be None.')

return key

def sign(self, msg, key):
return b''
Expand Down Expand Up @@ -96,6 +103,17 @@ def prepare_key(self, key):
if isinstance(key, text_type):
key = key.encode('utf-8')

invalid_strings = [
b'-----BEGIN PUBLIC KEY-----',
b'-----BEGIN CERTIFICATE-----',
b'ssh-rsa'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware this PR has already been merged, but I wanted to open a brief discussion.

Another possible header is -----BEGIN RSA PUBLIC KEY-----, and there may be others. Is there a more sound way of dealing with this that's somewhat more future-proof? If the check looked like this, would it be too broad?

invalid_strings = [
            b'-----',
            b'ssh-rsa'
]

]

if any([string_value in key for string_value in invalid_strings]):
raise InvalidKeyError(
'The specified key is an assymetric key or x509 certificate and'
' should not be used as an HMAC secret.')

return key

def sign(self, msg, key):
Expand Down
4 changes: 4 additions & 0 deletions jwt/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ class InvalidIssuerError(InvalidTokenError):
pass


class InvalidKeyError(Exception):
pass


# Compatibility aliases (deprecated)
ExpiredSignature = ExpiredSignatureError
InvalidAudience = InvalidAudienceError
Expand Down
21 changes: 21 additions & 0 deletions tests/keys/testkey_rsa.cer
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
-----BEGIN CERTIFICATE-----
MIIDhTCCAm2gAwIBAgIJANE4sir3EkX8MA0GCSqGSIb3DQEBCwUAMFkxCzAJBgNV
BAYTAlVTMQ4wDAYDVQQIDAVUZXhhczEPMA0GA1UEBwwGQXVzdGluMQ4wDAYDVQQK
DAVQeUpXVDEZMBcGA1UECwwQVGVzdCBDZXJ0aWZpY2F0ZTAeFw0xNTAzMTgwMTE2
MTRaFw0xODAzMTcwMTE2MTRaMFkxCzAJBgNVBAYTAlVTMQ4wDAYDVQQIDAVUZXhh
czEPMA0GA1UEBwwGQXVzdGluMQ4wDAYDVQQKDAVQeUpXVDEZMBcGA1UECwwQVGVz
dCBDZXJ0aWZpY2F0ZTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANR4
MwXyb9nDo0K8gsHvDRHpa4jkzRVimVIr3r1K0YZanJmSXQr7giUa/sQjfjpjvKsI
CSUffH3jbo8VYPifS7N/1DgOB3BfZ2B+mqlVxCwBPB5PwC78YveprNQw7gL0BmmG
fpQDcZb8XkBTmUm45M//ZofGi3hisKiS6d6fjoVAUKcLwFAD4PNvjlLYE1t50pY4
3ha9eAfKgJ3hknP8JdJ4vvtUkWVFxUqL83KkDpJWt1tu66y36w+i14I/07A7OLw9
T5yJtc3FXpyk+032CNe27Bvzv1nnMM9jZdfaS+4A6LDa7hd6ICVjatS8p/4oz0J5
Dy6WR8ob7osnGHCNw4kCAwEAAaNQME4wHQYDVR0OBBYEFDR6fVdFxZED6YMmD62W
LlBW+qEBMB8GA1UdIwQYMBaAFDR6fVdFxZED6YMmD62WLlBW+qEBMAwGA1UdEwQF
MAMBAf8wDQYJKoZIhvcNAQELBQADggEBAFwDNwm+lU/kGfWwiWM0Lv2aosXotoiG
TsBSWIn2iYphq0vzlgChcNocN9zkaOz3zc9pcREP6lyqHpE0OEbNucHHDdU1L2he
lLFOLOmkpP5fyPDXs9nKYhO8ygMByEonHm3K/VvCgrsSgJ3JuxMLUxnE55jQXGWV
OqYQNo2J5h93Zd2HTTe19jCz+bbWnRBP5VvLAAAo5YSmk3iroWSPWAKkWOOecJ2Q
/xnRyuWERsfvZiF/m9q7yDJ55LXVVm3Rufmy76SoTnJ2acap+XQNXBH/AxayeLUS
OYmHWH61dUcsQtwXYHYRB8TTtMIwUCXGmthXkDJydEfrGcD0y6APIh8=
-----END CERTIFICATE-----
41 changes: 40 additions & 1 deletion tests/test_algorithms.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import base64

from jwt.algorithms import Algorithm, HMACAlgorithm
from jwt.algorithms import Algorithm, HMACAlgorithm, NoneAlgorithm
from jwt.exceptions import InvalidKeyError

from .compat import unittest
from .utils import ensure_bytes, ensure_unicode, key_path
Expand Down Expand Up @@ -35,6 +36,12 @@ def test_algorithm_should_throw_exception_if_verify_not_impl(self):
with self.assertRaises(NotImplementedError):
algo.verify('message', 'key', 'signature')

def test_none_algorithm_should_throw_exception_if_key_is_not_none(self):
algo = NoneAlgorithm()

with self.assertRaises(InvalidKeyError):
algo.prepare_key('123')

def test_hmac_should_reject_nonstring_key(self):
algo = HMACAlgorithm(HMACAlgorithm.SHA256)

Expand All @@ -49,6 +56,38 @@ def test_hmac_should_accept_unicode_key(self):

algo.prepare_key(ensure_unicode('awesome'))

@unittest.skipIf(not has_crypto, 'Not supported without cryptography library')
def test_hmac_should_throw_exception_if_key_is_pem_public_key(self):
algo = HMACAlgorithm(HMACAlgorithm.SHA256)

with self.assertRaises(InvalidKeyError):
with open(key_path('testkey2_rsa.pub.pem'), 'r') as keyfile:
algo.prepare_key(keyfile.read())

@unittest.skipIf(not has_crypto, 'Not supported without cryptography library')
def test_hmac_should_throw_exception_if_key_is_x509_certificate(self):
algo = HMACAlgorithm(HMACAlgorithm.SHA256)

with self.assertRaises(InvalidKeyError):
with open(key_path('testkey_rsa.cer'), 'r') as keyfile:
algo.prepare_key(keyfile.read())

@unittest.skipIf(not has_crypto, 'Not supported without cryptography library')
def test_hmac_should_throw_exception_if_key_is_ssh_public_key(self):
algo = HMACAlgorithm(HMACAlgorithm.SHA256)

with self.assertRaises(InvalidKeyError):
with open(key_path('testkey_rsa.pub'), 'r') as keyfile:
algo.prepare_key(keyfile.read())

@unittest.skipIf(not has_crypto, 'Not supported without cryptography library')
def test_hmac_should_throw_exception_if_key_is_x509_cert(self):
algo = HMACAlgorithm(HMACAlgorithm.SHA256)

with self.assertRaises(InvalidKeyError):
with open(key_path('testkey2_rsa.pub.pem'), 'r') as keyfile:
algo.prepare_key(keyfile.read())

@unittest.skipIf(not has_crypto, 'Not supported without cryptography library')
def test_rsa_should_parse_pem_public_key(self):
algo = RSAAlgorithm(RSAAlgorithm.SHA256)
Expand Down