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

Make fedmsg replays from datagrepper work in most cases #477

Merged
merged 2 commits into from Sep 6, 2017

Conversation

Projects
None yet
2 participants
@jeremycline
Member

jeremycline commented Sep 5, 2017

Message playback was not working because the messages did not pass
validation. The reason they didn't pass validation is datanommer mutates
the original fedmsg. This breaks the x509 signature. This change tries
to undo all the changes datanommer makes, but there are several cases
where it is impossible to know what the correct value for a key is so
some messages might still fail signature validation.

The longer term fix is to change how datanommer works so that it does
not alter the message in any way. Additionally, the fedmsg signature
approach could be made much less fragile.

For those that come after me, I am sorry about all the things I did in
this patch.

fixes #403

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

Make fedmsg replays from datagrepper work in most cases
Message playback was not working because the messages did not pass
validation. The reason they didn't pass validation is datanommer mutates
the original fedmsg. This breaks the x509 signature. This change tries
to undo all the changes datanommer makes, but there are several cases
where it is impossible to know what the correct value for a key is so
some messages might still fail signature validation.

The longer term fix is to change how datanommer works so that it does
not alter the message in any way. Additionally, the fedmsg signature
approach could be made much less fragile.

For those that come after me, I am sorry about all the things I did in
this patch.

fixes #403

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

@jeremycline jeremycline force-pushed the jeremycline:message-replay-crypto branch from b7c1a22 to 081baea Sep 6, 2017

@codecov

This comment has been minimized.

codecov bot commented Sep 6, 2017

Codecov Report

Merging #477 into develop will not change coverage.
The diff coverage is 57.89%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #477   +/-   ##
========================================
  Coverage    58.82%   58.82%           
========================================
  Files           29       29           
  Lines         1831     1831           
  Branches       303      303           
========================================
  Hits          1077     1077           
  Misses         667      667           
  Partials        87       87
Impacted Files Coverage Δ
fedmsg/crypto/utils.py 58.46% <57.89%> (ø) ⬆️

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 8f0a741...6578298. Read the comment docs.

A copy of the dictionary is made and returned if altering the message is necessary.
I'm so sorry.

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 6, 2017

Member

lol. This does all sound pretty crazy!

dict: A copy of the provided message, with the datagrepper-related keys removed
if they were present.
"""
if not ('source_name' in message and 'source_version' in message):

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 6, 2017

Member

Perhaps this ought to be an or instead of and?

This comment has been minimized.

@jeremycline

jeremycline Sep 6, 2017

Member

datanommer always adds both, according to the code. My fear was that if a message uses either one of the keys (they're both pretty generic), we'd mess with it. We'll still do the wrong thing if they use both, but this reduces the likelihood.

It's not a great solution. It's not even a good solution, but I don't know what else to do.

# datanommer adds the headers field to the message in all cases.
# This is a huge problem because if the signature was generated with a 'headers'
# key set and we delete it here, messages will fail validation, but if we don't
# messages will fail validation if they didn't have a 'headers' key set.

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 6, 2017

Member

So here's another crazy idea: if headers is null and validation fals, should we try adding a null headers back and see if that version passes validation?

This comment has been minimized.

@jeremycline

jeremycline Sep 6, 2017

Member

We could, and I considered that. The reason I opted not to is because this is already way more complicated than it should be and even if we did that we'd still be wrong plenty of times. What happens when someone sets that to null rather than an empty object? What happens when messages get signed with floating point timestamps rather than ints? datanommer truncates those. What happens when datanommer adds a new field and forgets to adjust this?

I'd rather have datanommer make strong promises about not altering messages.

self.hub = mock.Mock(config=self.config)
self.consumer = DummyConsumer(self.hub)
def test_backlog_message_validation(self):

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 6, 2017

Member

It'd be good to add a docblock here.

self.assertTrue(original_message is utils.fix_datagrepper_message(original_message))
def test_no_source_name(self):
"""Assert messages missing the "source_version" key are untouched."""

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 6, 2017

Member

Perhaps this comment was meant to say "source_name" instead of "source_version"?

self.assertTrue(original_message is utils.fix_datagrepper_message(original_message))
def test_no_source_version(self):
"""Assert messages missing the "source_name" key are untouched."""

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 6, 2017

Member

Similar here.

self.assertTrue(original_message is utils.fix_datagrepper_message(original_message))
def test_no_timestamp(self):
"""Assert messages missing the "source_name" key are untouched."""

This comment has been minimized.

@bowlofeggs

bowlofeggs Sep 6, 2017

Member

Comment is copypasta'd.

Fix the test comments from #477
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
@jeremycline

This comment has been minimized.

Member

jeremycline commented Sep 6, 2017

I'm not sure why codecov thinks the diff isn't covered, but it is.

@jeremycline jeremycline merged commit 04d94be into fedora-infra:develop Sep 6, 2017

2 of 3 checks passed

codecov/patch 57.89% of diff hit (target 100%)
Details
codecov/project 58.82% remains the same compared to 8f0a741
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jeremycline jeremycline deleted the jeremycline:message-replay-crypto branch Sep 6, 2017

sijis added a commit to sijis/fedmsg that referenced this pull request Jan 8, 2018

Fix the test comments from fedora-infra#477
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment