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

Signature verification #299

Merged
merged 92 commits into from
Jun 1, 2023

Conversation

mediaformat
Copy link
Collaborator

@mediaformat mediaformat commented Apr 2, 2023

federated servers ... SHOULD NOT TRUST content received from a server other than the content's origin without some form of verification. SocialCG

This PR implements http signatures and digests verification thereby preventing impersonation and other MITM attacks.

Features:

  • Verify signatures of all incoming POST requests (comments)
  • Provides an Application actor (sometimes known as a Service actor) to sign all outgoing GET requests (instead of leaking the identity of users making the requests)
  • Optionally verify incoming GET requests. This is also known as Secure Mode. And is a useful feature in deterring harassment. (Blocked users are prevented from accessing profiles, and posts, and therefore cannot redistribute or interact with them).

Tested:

  • Mastodon
  • Akkoma
  • GoToSocial
  • Friendica
  • Calckey
  • Lemmy seems to not work at all. lemmy seem to have a allow list, that blocks any other instance.
  • Pixelfed

@mediaformat mediaformat marked this pull request as ready for review April 5, 2023 02:49
@mediaformat
Copy link
Collaborator Author

I'll make a separate PR for LD Signatures

@pfefferle
Copy link
Member

Is the LD signature thing something we have to support?

@mediaformat
Copy link
Collaborator Author

mediaformat commented Apr 6, 2023

[LD Signatures] ... are used in the following situations:
When running a self-destruct sequence to send Delete activities to all known peers, the payload will use LD Signatures because HTTP Signatures will not be available. Receiving servers will process the signature by validating it against the locally cached actor key, since the HTTP server will no longer be hosting old actor information.
https://docs.joinmastodon.org/spec/security/#ld

Considering the above, to properly verify the LD signature on an Actor Delete activity, we would need to have a cached copy of their actor key, or perhaps try to fetch this actor.

The side effect of an Actor delete would be to remove their comments locally.

@pfefferle
Copy link
Member

pfefferle commented Apr 12, 2023

@mediaformat should we require the signature verification?

Since the signature is not part of the spec, maybe we should add a label verified or unverified to the reply and maybe auto approve the verified instead of block unverified messages. Another idea is, to make the verification configurable. Or maybe both?

@mediaformat
Copy link
Collaborator Author

mediaformat commented Apr 15, 2023

the signature is not part of the spec

Not officially, no. But this seems to be because the Editors had pressure to finalize the spec... and the HTTP Signatures spec itself was a working draft.

B. Security Considerations
Unfortunately at the time of standardization, there are no strongly agreed upon mechanisms for authentication. Some possible directions for authentication are laid out in the Social Web Community Group Authentication and Authorization best practices report.

I think this part is important (emphasis my own):

B.2 Verification
... federated servers also SHOULD NOT TRUST content received from a server other than the content's origin without some form of verification.

What do you think?

@mediaformat
Copy link
Collaborator Author

make the verification configurable

We could do this for the GET requests, similar to Mastodon's Authorized Fetch

As a result, through the authentication mechanism and avoiding re-distribution mechanisms that do not have your server in the loop, it becomes possible to enforce who can and cannot retrieve even public content from your server, e.g. servers whose domains you have blocked.

@pfefferle
Copy link
Member

@mediaformat is it easier if the signature class would be an object instead of a static helper class?

@pfefferle
Copy link
Member

So then I will run it on my live server for some more days and then we can merge it 😍

pfefferle
pfefferle previously approved these changes May 25, 2023
@pfefferle pfefferle self-requested a review May 25, 2023 12:17
@pfefferle pfefferle requested a review from jeherve May 25, 2023 12:18
pfefferle
pfefferle previously approved these changes May 31, 2023
Copy link
Member

@pfefferle pfefferle left a comment

Choose a reason for hiding this comment

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

No issues so far! I will merge it and if we find issues, we can improve on that.

@jeherve jeherve dismissed their stale review May 31, 2023 06:07

Dismissing my review as to not be a blocker, but I still think some tests would be nice.

@pfefferle
Copy link
Member

@mediaformat could you add some tests, then I would merge it?

@mediaformat
Copy link
Collaborator Author

mediaformat commented May 31, 2023

@pfefferle test included!

@pfefferle
Copy link
Member

Thanks @mediaformat :)

Copy link
Collaborator Author

@mediaformat mediaformat left a comment

Choose a reason for hiding this comment

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

I won't have any time to dedicate for the coming weeks, hopefully someone can help fix the test_rest_activity_signature test case

includes/class-signature.php Outdated Show resolved Hide resolved
@pfefferle
Copy link
Member

Awesome! Seems to work like a charm and tests are green!

Thanks @mediaformat !

@pfefferle pfefferle merged commit bfe5381 into Automattic:master Jun 1, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants