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 validating unicode signatures. #456

Merged
merged 8 commits into from Aug 9, 2017

Conversation

Projects
None yet
4 participants
@ralphbean
Contributor

ralphbean commented Aug 3, 2017

I just noticed this in the logs of program driven by fedmsg.tail_messages.
The signature was unicode, and so this type check failed every time.

This seems to have been inadvertently broken in f0553d2 ?

@codecov-io

This comment has been minimized.

codecov-io commented Aug 4, 2017

Codecov Report

Merging #456 into develop will increase coverage by 0.42%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #456      +/-   ##
===========================================
+ Coverage    58.48%   58.91%   +0.42%     
===========================================
  Files           29       29              
  Lines         1821     1835      +14     
  Branches       300      303       +3     
===========================================
+ Hits          1065     1081      +16     
+ Misses         668      667       -1     
+ Partials        88       87       -1
Impacted Files Coverage Δ
fedmsg/crypto/x509.py 88.04% <100%> (+2.98%) ⬆️
fedmsg/crypto/x509_ng.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3206ee0...31a6e42. Read the comment docs.

@jeremycline

Thanks for the PR!

This change needs tests to demonstrate the issue before it can be merged.

@@ -113,7 +113,7 @@ def fail(reason):
for field in ['signature', 'certificate']:
if field not in message:
return fail("No %r field found." % field)
if not isinstance(message[field], str):
if not isinstance(message[field], basestring):

This comment has been minimized.

@jeremycline

jeremycline Aug 4, 2017

Member

basestring is undefined in Python 3. While this code will never actually run in Python 3 since it's the M2Crypto implementation, this makes the API unclear and it should either be six.text_type (unicode and str in Python 2 and Python 3, respectively) or six.binary_type (str and bytes in Python 2 and Python 3, respectively).

Since we don't know the encoding at this point, I believe it should be six.text_type.

This comment has been minimized.

@ralphbean

ralphbean Aug 4, 2017

Contributor

That's legit.

The underlying issue is (I think) that some routines pass unicode signatures in here, while others pass byte-strings.

I guess I'm with you.. that we should enforce "unicode" as the api with six.text_type and then go looking for any places that may erroneously call this with byte strings.

ralphbean and others added some commits Aug 3, 2017

Allow validating unicode signatures.
I just noticed this in the logs of program driven by `fedmsg.tail_messages`.
The signature was unicode, and so this type check failed every time.

This seems to have been inadvertently broken in f0553d2 ?
Assert the signature/certificate fields are unicode
This checks the certificate and signature fields in messages to ensure
they're text. If they're not, an error is logged and we do our best to
decode the message as UTF-8. This decoding is a short-term measure until
the users of the APIs have been hunted down and fixed.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Move fedmsg.crypto.x509 ``log`` to ``_log``
This is mostly because it makes writing tests easier, and also because
it shouldn't be a public symbol.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Tests for PR 456
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Add two tests asserting the correct sign API
``fedmsg.crypto.sign`` should return unicode text signatures and
certificates, but it currently does not. These tests assert the correct
API and are marked as expected failures.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Add the encoding header to test_x509.py
There are non-ascii characters in the file so it needs an encoding
header.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>

@jeremycline jeremycline force-pushed the ralphbean:feature/unicode-signature branch from 561bf39 to 06d0959 Aug 9, 2017

jeremycline added some commits Aug 9, 2017

Remove some unreachable code
The dictionary is checked for both keys above and thus this block cannot
be reached.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Add a test to assert the validate API accepts text signatures
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
@puiterwijk

This comment has been minimized.

Member

puiterwijk commented Aug 9, 2017

Quick glance says that this looks okay to me.

@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Aug 9, 2017

Quick glance says that this looks okay to me.

Same.

@jeremycline jeremycline merged commit 3136115 into fedora-infra:develop Aug 9, 2017

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 58.91% (+0.42%) compared to 3206ee0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ralphbean ralphbean deleted the ralphbean:feature/unicode-signature branch Aug 9, 2017

jeremycline added a commit that referenced this pull request Aug 9, 2017

Assert the signature/certificate fields are unicode
This checks the certificate and signature fields in messages to ensure
they're text. If they're not, an error is logged and we do our best to
decode the message as UTF-8. This decoding is a short-term measure until
the users of the APIs have been hunted down and fixed.

This is a cherry-pick from
#456

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
@jeremycline

This comment has been minimized.

Member

jeremycline commented Aug 14, 2017

This patch was backported to 0.19.1

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