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

Conversation

mark-adams
Copy link
Contributor

Added validations for the following:

  • Asymmetric keys and x509 certificates cannot be used as HMAC keys
  • encode() and decode() should not accept a value for key if the algorithm is "none"

@mark-adams mark-adams force-pushed the algo-validation-fixes branch from 75ab144 to aa88601 Compare March 17, 2015 20:23
@@ -96,6 +103,11 @@ def prepare_key(self, key):
if isinstance(key, text_type):
key = key.encode('utf-8')

if (b'-----BEGIN PUBLIC KEY-----' in key or b'-----BEGIN CERTIFICATE-----' in key):
raise InvalidAlgorithmError(
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be InvalidKeyError or InvalidAlgorithmError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be InvalidKeyError... changing now

@jpadilla jpadilla added this to the v1.0.0 milestone Mar 18, 2015
jpadilla added a commit that referenced this pull request Mar 18, 2015
Added some fixes related to algorithm and key choice
@jpadilla jpadilla merged commit 88a9fc5 into jpadilla:master Mar 18, 2015
@mark-adams
Copy link
Contributor Author

FYI, I did some performance tests (500,000 encode()s) on HMAC before and after the change (invalid_strings check for public key and certs).

Without new checks: 32.63310008600092
With new checks: 34.56768506300068

Looks like this change slows down HMAC encode by about 6%.

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'
]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants