-
Notifications
You must be signed in to change notification settings - Fork 89
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
Refactoring inbox processing to smaller tasks #647
Conversation
Seems like a decent approach - could you rebase onto |
Rebased. Probably fixes #642 |
Sorry for the test spam there. I don't have a working test environment locally.. The test fail in signatures confuses me. The code should still raise VerificationError on an invalid signature - what has changed there is that:
Reverting the little changes I made to core/signatures.py anyway.. |
Can't see why the signature test continues to fail. |
The signature tests are quite picky but it's also for a good reason (the signing code took a lot of work to get accurate to how other servers wanted to do things). I don't have the capacity to debug this locally for a while, sadly, so if you get a chance to that might help. |
The reason I started looking into the signatures at all was that I was seeing daily failures in LD signature verification from messages I would see Mastodon servers having accepted. I spent a lot of time trying to decipher why -- the only thing I could find out was that sometimes sample JSON would show up with different RDF encoding to what pyld would produce, but the whole process is very opaque and I could get no explanation to that at all. But in the process I also found out that Mastodon doesn't recommend implementing LD Signatures at all, because their currently implemented version is obsolete, and that Mastodon only refuses to forward LD sigs which don't check out, but doesn't refuse the message itself. Which is what I did here as well:
I only noticed now that while the signature check still works for correct LD Sigs, the test case for a manipulated message doesn't raise an exception as the test expects. Except... the implementation still raises an exception in the same cases as before, and running the code in the wild, I see messages in my logs about LD signatures being stripped of messages when they don't check out. At first blush, it would seem to me the test is broken. Except.. I didn't change the test either, and it must have worked before. I know, I should have a working local test environment and having run the tests before making any changes - or reproduce them without this patch. I haven't gotten around to setting up an env with postgres available for the tests, though :( |
It is the test that is busted. The test document:
has no |
Ahh, nice catch. I had no idea Mastodon had deprecated LD sigs - I wonder what the situation is like for other servers. |
My understanding is that Mastodon servers with AUTHORIZED_FETCH enabled won't produce LD Sigs at all - because the idea of the feature is to require subscribers to authenticate with the source server to see the content, and removing LD Sigs creates plausible deniability over the content. I had a quick peek at Pixelfed and Firefish source. Pixelfed doesn't appear to contain LD Signature support at all, and Firefish apparently only attaches LD Sigs to messages in the relay service. Makes sense, actually. |
Yeah, that does make sense. The ones I'd be mostly thinking that did this would be gotosocial and pleroma; they tend to be the most prevalent in terms of content I've seen. |
I found only HTTP signature implementation in both gotosocial and pleroma, no sign of LD sig in either. And for whatever it's worth, now that (valid) LD sigs are retained on the inbox messages, scanning that table (sorry for the pretty nasty SQL here), I only see LD sigs on Mastodon and Hometown servers (the latter being a fork of Mastodon). The invalid LD sigs are logged, but that being a text file I didn't bother checking all of them for software type. On a spot check, I see a several of those log messages caused by the same Mastodon servers I see valid sigs from -- that being an indication that LD Sigs aren't a stable thing due to something in the normalization/RDF translation process.
|
Great, thanks - sounds like there's really not much point continuing to verify or implement them, really... |
(I sent a PR with the first commit separetely, but this builds on top and uses logging for providing diagnostics)
This fixes random rejects of incoming messages, because LD Signatures just don't always check correctly due to mysterious differences in the JSON->RDF translation
This also means that we can't verify the signature of the first ActivityPub message we see from a new actor - because the key isn't yet known. Stator will fetch it in the second stage of processing and it'll be available for further messages.
Without this change, my server was sporadically bogged down by storms of messages from blocked domains, because fetch_actor was called before checking for domain blocks.
also because it slowed fetch_actor down a lot, and frequently caused database integrity errors as multiple threads were trying to insert the same data