Make fedmsg replays from datagrepper work in most cases #477
Conversation
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 fedora-infra#403 Signed-off-by: Jeremy Cline <[email protected]>
jeremycline
force-pushed
the
message-replay-crypto
branch
from
b7c1a22 to
081baea
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
|
||
| A copy of the dictionary is made and returned if altering the message is necessary. | ||
|
|
||
| I'm so sorry. |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
Perhaps this ought to be an or instead of and?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
It'd be good to add a docblock here.
fedmsg/tests/crypto/test_utils.py
Outdated
| 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.""" |
There was a problem hiding this comment.
Perhaps this comment was meant to say "source_name" instead of "source_version"?
fedmsg/tests/crypto/test_utils.py
Outdated
| 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.""" |
fedmsg/tests/crypto/test_utils.py
Outdated
| 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.""" |
Signed-off-by: Jeremy Cline <[email protected]>
|
I'm not sure why codecov thinks the diff isn't covered, but it is. |
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 [email protected]