Verify activity's signature #256

Merged

Plume_migration_agent merged 8 commits from verify-signature into master

trinity-1686a commented

Owner

Verify signature for incoming activity. This is still work in progress
Fix #10

Todo:

Verify HTTP signature
Verify JSON-ld signature

Federation status at this time:

itself
older Plume
other ActivityPub implementations (Mastoodn, Pleroma tested)

Side-note:
As this as the ability to totally break federation, we should wait to be sure it does work with at least Mastodon, Pleroma and older Plume before merging this

Verify signature for incoming activity. This is still work in progress Fix #10 Todo: - [X] Verify HTTP signature - [x] Verify JSON-ld signature Federation status at this time: - [X] itself - [x] older Plume - [x] other ActivityPub implementations (Mastoodn, Pleroma tested) Side-note: As this as the ability to totally break federation, we should wait to be sure it does work with at least Mastodon, Pleroma and older Plume before merging this

👍 1 ❤️ 1

trinity-1686a reviewed

src/routes/instance.rs

trinity-1686a commented

Poster

Owner

This should probably go somewhere else, as it will be used on both the shared inbox and the personal one, which is defined in another file

trinity-1686a commented

Poster

Owner

The same as to be added on personal inboxes

elegaanz (Migrated from github.com) reviewed

plume-common/src/activity_pub/mod.rs

elegaanz (Migrated from github.com) commented

Why not self == SignatureValidity::Valid?

Why not `self == SignatureValidity::Valid`?

trinity-1686a reviewed

plume-common/src/activity_pub/mod.rs

trinity-1686a commented

Poster

Owner

Because PartialEq is not implemented for SignatureValidity? Joking, it can be derived. I just copied the whole match from src/instance.rs, moved the todo somewhere-else, and basically forgot there were an shorter and cleaner way

Because `PartialEq` is not implemented for SignatureValidity? Joking, it can be derived. I just copied the whole match from [src/instance.rs](https://github.com/Plume-org/Plume/blob/0a5d435249f9f4b4df7f42fc40578af302a91954/src/routes/instance.rs#L218), moved the todo somewhere-else, and basically forgot there were an shorter and cleaner way

igalic (Migrated from github.com) reviewed

igalic (Migrated from github.com) left a comment

general code style

plume-common/src/activity_pub/mod.rs

igalic (Migrated from github.com) commented

this entire tree could be turned into a negative match to return early, and get rid of the else part
it's very hard to follow this way, imo

this entire tree could be turned into a negative match to return early, and get rid of the `else` part it's very hard to follow this way, imo

igalic (Migrated from github.com) reviewed

plume-common/src/activity_pub/mod.rs

igalic (Migrated from github.com) commented

this is a general question
i see a lot of this type of code in our codebase:

is early return something that's frowned upon in rust, or has it just not been considered a possibility here?

this is a general question i see a lot of this type of code in our codebase: is ***early return*** something that's frowned upon in rust, or has it just not been considered a possibility here?

trinity-1686a reviewed

plume-common/src/activity_pub/mod.rs

trinity-1686a commented

Poster

Owner

I don't know if it's frowned upon, but this is something that I don't like personally, however I agree this would be much easier to read with early returns, I'll modify that at some point

igalic (Migrated from github.com) reviewed

plume-common/src/activity_pub/mod.rs

igalic (Migrated from github.com) commented

thank you for explaining
maybe we can find a better way to deal with the trees ;)
maybe it's just me (or my dyslexia), but i find them very hard to read and follow

thank you for explaining maybe we can find a better way to deal with the trees ;) maybe it's just me (or my dyslexia), but i find them very hard to read and follow

igalic commented (Migrated from github.com)

i'm kinda surprised no one's written a json-ld crate so far

trinity-1686a reviewed

plume-common/src/activity_pub/mod.rs

trinity-1686a commented

Poster

Owner

@igalic I tried to flatten the tree as you suggested, you might want to take a look and tell me if it's better

trinity-1686a commented

Poster

Owner

@BaptisteGelez is it possible you try to deploy this branch on your test instance to test federation with Mastodon, Pleroma and maybe other activity-pub enabled softwares?

igalic (Migrated from github.com) approved these changes

igalic (Migrated from github.com) left a comment

💜 thank you so much this really makes it a lot clearer what's going on!

elegaanz commented (Migrated from github.com)

@fdb-hiroshima baptiste.gelez.xyz is now running this branch. If you need anything else (logs for instance) tell me.

trinity-1686a commented

Poster

Owner

@BaptisteGelez could you update your instance to the latest commit, and if it still don't work, give me back the generated logs?

Reviewers

The pull request has been merged as 8fdb55a501.

You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.

git checkout -b verify-signature master

git pull origin verify-signature

Step 2:

Merge the changes and update on Forgejo.

git checkout master

git merge --no-ff verify-signature

git push origin master

No reviewers

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Loading…

There is no content yet.

Labels Milestones

Verify activity's signature #256

Reviewers

Step 1:

Step 2: