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

Allow 'raw' PKCS1 signatures #103

Closed

Conversation

joostrijneveld
Copy link
Contributor

This provides more convenient compatability with other libraries that use such raw signatures in intermediate results. In particular, it matches the behavior of openssl rsautl -sign.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 91.867% when pulling d8bc7ac on joostrijneveld:raw-rsa-signatures into 8affa13 on sybrenstuvel:master.

@coveralls
Copy link

coveralls commented Oct 31, 2017

Coverage Status

Coverage increased (+0.02%) to 92.431% when pulling 69ccc2b on joostrijneveld:raw-rsa-signatures into 83e273b on sybrenstuvel:master.

@sybrenstuvel
Copy link
Owner

Please also update the documentation so that our users actually know about this feature and how to use it.

@joostrijneveld
Copy link
Contributor Author

Good call. I've added a line to highlight this in the usage.rst documentation.

This provides more convenient compatability with other libraries
that use such raw signatures in intermediate results.

In particular, it matches the behavior of `openssl rsautl -sign`
@joostrijneveld
Copy link
Contributor Author

Not sure if you get notifications of new commits, so for completeness; I've added a test and a changelog entry :)

@0x2b3bfa0
Copy link

@sybrenstuvel: could you merge this PR?

@vstoykov
Copy link

vstoykov commented Nov 3, 2018

Probably it should be rebased first because there are conflicts

@joostrijneveld
Copy link
Contributor Author

@vstoykov Since the conflict is in a recently released CHANGELOG.txt update, that would mean starting a new release candidate (following up on 4.0). I'm not sure if I'm the right person to make that decision.

@sybrenstuvel
Copy link
Owner

There is one thing that bugs me, and that's the change in behaviour of _find_method_hash(). Now this no longer raises a VerificationError but just returns "RAW". This means that bad signatures can all of a sudden be accepted as correct. This is a deal breaker, as it makes this code insecure by default.

The documented behaviour of raising an exception in case of an error has been removed in this PR, without changing the documentation and adding huge big red letters that the API has changed. In any case I would reject such an API change, as such errors should never be silenced.

Copy link
Owner

@sybrenstuvel sybrenstuvel left a comment

Choose a reason for hiding this comment

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

Rejecting this PR as it silently ignores signature errors (making the code insecure-by-default), changes the semantics of functions without updating the docstring of those functions.

@joostrijneveld
Copy link
Contributor Author

I agree, and would suggest to close this PR on that account; I had overlooked that marking signatures without an ASN1 tag as 'RAW' rather than raising an error was changing API behavior, and I see no obvious way to prevent doing so.

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

Successfully merging this pull request may close these issues.

None yet

5 participants