BREAKING: activitypub: validate fetch signatures #399

helene · 2023-06-23T14:30:47Z

helene commented

2023-06-23 14:30:47 +00:00

Enforces HTTP signatures on object fetches, and rejects fetches from blocked instances. This should mean proper and full blocking of remote instances.

This is now default behavior, which makes it a breaking change.
To disable it (mostly for development purposes), allowUnsignedFetches can be set to true in the configuration . It is not the default for development environments as it is important to have as close as possible behavior to real environments for ActivityPub development.

Co-authored-by: nullobsi
Co-authored-by: Norm
Changelog: Added

Enforces HTTP signatures on object fetches, and rejects fetches from blocked instances. This should mean proper and full blocking of remote instances. This is now default behavior, which makes it a breaking change. To disable it (mostly for development purposes), `allowUnsignedFetches` can be set to `true` in the configuration . It is not the default for development environments as it is important to have as close as possible behavior to real environments for ActivityPub development. Co-authored-by: nullobsi Co-authored-by: Norm Changelog: Added

🎉 2

helene added 1 commit 2023-06-23 14:30:48 +00:00

BREAKING: activitypub: validate fetch signatures

ci/woodpecker/pr/lint-foundkey-js Pipeline was successful

Details

ci/woodpecker/pr/lint-client Pipeline was successful

Details

ci/woodpecker/pr/lint-backend Pipeline was successful

Details

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/lint-sw Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

f89a374e5f

Enforces HTTP signatures on object fetches, and rejects fetches from blocked
instances. This should mean proper and full blocking of remote instances.

This is now default behavior, which makes it a breaking change.
To disable it (mostly for development purposes), "meta"."allowUnsignedFetches"
can be set to true. It is not the default for development environments as it
is important to have as close as possible behavior to real environments for
ActivityPub development.

Co-authored-by: nullobsi <me@nullob.si>
Co-authored-by: Norm <normandy@biribiri.dev>
Changelog: Added

Johann150 reviewed 2023-06-23 16:49:51 +00:00

Johann150 left a comment

The points below are things to refactor which are more notes to myself. Of course feel free to apply some of the suggestions.

However something that should be at least thought about before this is merged: This is only somewhat effective at hiding the content. You can obviously still see it in the frontend, scrape it from the API or from <meta> opengraph tags.

The points below are things to refactor which are more notes to myself. Of course feel free to apply some of the suggestions. However something that should be at least thought about before this is merged: This is only somewhat effective at hiding the content. You can obviously still see it in the frontend, scrape it from the API or from `<meta>` opengraph tags.

packages/backend/src/models/entities/meta.ts Outdated

					
				@ -85,0 +85,4 @@

					@Column('boolean', {

						default: false

					})

					public allowUnsignedFetches: boolean;

Johann150 commented

2023-06-23 15:30:07 +00:00

🔧 I think it would be better if this goes into the config file instead of the database. People should not have to manually edit the database to change configuration.

Johann150 marked this conversation as resolved

packages/backend/src/server/activitypub.ts Outdated

					
				@ -59,6 +61,30 @@ export function setResponseType(ctx: Router.RouterContext): void {

					}

				}

				export function handleSignatureResult(ctx: Router.RouterContext, result: SignatureValidationResult): boolean {

Johann150 commented

2023-06-23 16:06:39 +00:00

This function is always used together with validateFetchSignature, and the call signature is always handleSignatureResult(ctx, await validateFetchSignature(ctx.req)). Is there a reason for them being two separate functions?

It would seem sensible to either combine these functions or at least put the function call inside. This reduces the number of parameters and makes it easier to read.

-export function handleSignatureResult(ctx: Router.RouterContext, result: SignatureValidationResult): boolean {
-	switch (result) {
+export function handleSignatureResult(ctx: Router.RouterContext): boolean {
+	switch (await validateFetchSignature(ctx.req)) {

This function is always used together with `validateFetchSignature`, and the call signature is always `handleSignatureResult(ctx, await validateFetchSignature(ctx.req))`. Is there a reason for them being two separate functions? It would seem sensible to either combine these functions or at least put the function call inside. This reduces the number of parameters and makes it easier to read. ```diff -export function handleSignatureResult(ctx: Router.RouterContext, result: SignatureValidationResult): boolean { - switch (result) { +export function handleSignatureResult(ctx: Router.RouterContext): boolean { + switch (await validateFetchSignature(ctx.req)) { ```

Johann150 commented

2023-06-23 16:10:40 +00:00

It could be easier to understand if this was a middleware function that could be added to routes.

Johann150 commented

2023-06-23 16:12:57 +00:00

I don't think this function needs to be exported since it isn't used in other modules/files.

I don't think this function needs to be `export`ed since it isn't used in other modules/files.

helene commented

2023-06-23 16:57:30 +00:00

The export is a mistake, yes. I thought that it would be easier to implement a feature to allow users to block specific instances from accessing their activities, if it was done this way, but having a unified handleSignature shouldn't be a problem for this situation; it would only mean adding a new argument in such a case, which would actually be preferable. I will apply your suggestion.

The `export` is a mistake, yes. I thought that it would be easier to implement a feature to allow users to block specific instances from accessing their activities, if it was done this way, but having a unified `handleSignature` shouldn't be a problem for this situation; it would only mean adding a new argument in such a case, which would actually be preferable. I will apply your suggestion.

Johann150 marked this conversation as resolved

packages/backend/src/server/activitypub/fetch-signature.ts

					
				@ -0,0 +38,4 @@

					return null;

				}

				export async function validateFetchSignature(req: IncomingMessage): Promise<SignatureValidationResult> {

Johann150 commented

2023-06-23 16:10:05 +00:00

This should be refactored together with the code from packages/backend/src/queue/processors/inbox.ts that this was copied from to work out a way that does not duplicate most of the code.

This should be refactored together with the code from `packages/backend/src/queue/processors/inbox.ts` that this was copied from to work out a way that does not duplicate most of the code.

helene commented

2023-06-23 16:59:37 +00:00

I would've liked to replace the inbox signature code with this function, but LD signatures and the reliance on the actor field of an activity (as well as relay logic) would make it difficult without introducing bugs, unwanted behavior, unclean code and/or more network/database requests.

I would've liked to replace the `inbox` signature code with this function, but LD signatures and the reliance on the `actor` field of an activity (as well as relay logic) would make it difficult without introducing bugs, unwanted behavior, unclean code and/or more network/database requests.

Johann150 marked this conversation as resolved

packages/backend/src/server/activitypub/fetch-signature.ts

					
				@ -0,0 +60,4 @@

					if (keyIdLower.startsWith('acct:'))

						return 'invalid';

					const host = toPuny(new URL(keyIdLower).hostname);

Johann150 commented

2023-06-23 16:08:36 +00:00

-	const host = toPuny(new URL(keyIdLower).hostname);
+	const host = extractDbHost(keyIdLower);

and consequently above:

-import { toPuny } from '@/misc/convert-host.js'; 
+import { extractDbHost } from '@/misc/convert-host.js';

```diff - const host = toPuny(new URL(keyIdLower).hostname); + const host = extractDbHost(keyIdLower); ``` and consequently above: ```diff -import { toPuny } from '@/misc/convert-host.js'; +import { extractDbHost } from '@/misc/convert-host.js'; ```

helene commented

2023-06-23 17:02:40 +00:00

I based myself on the inbox code which uses the same code. I will apply the patch to my submission, but the inbox code should probably be patched to match. (PS: I find extractDbHost to be quite a confusing name for such a function)

I based myself on the `inbox` code which uses the same code. I will apply the patch to my submission, but the `inbox` code should probably be patched to match. (PS: I find `extractDbHost` to be quite a confusing name for such a function)

Johann150 commented

2023-06-25 18:34:40 +00:00

Do you have a suggestion for a better name (for future refactoring)?

helene commented

2023-06-25 19:56:24 +00:00

extractHost, extractHostname, punyHost, or something similar? I'm not sure; I mostly think the Db in the name doesn't make much more sense when used in such contexts, even if that's where the function originated from.

`extractHost`, `extractHostname`, `punyHost`, or something similar? I'm not sure; I mostly think the `Db` in the name doesn't make much more sense when used in such contexts, even if that's where the function originated from.

Johann150 marked this conversation as resolved

packages/backend/src/server/activitypub/fetch-signature.ts

					
				@ -0,0 +94,4 @@

					// Verify the HTTP Signature

					const httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem);

					if (httpSignatureValidated === true)

Johann150 commented

2023-06-23 16:02:17 +00:00

Explicitly checking for something to equal true seems unnecessary, since httpSignature.verifySignature has the return type boolean.

-	const httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem);
-	if (httpSignatureValidated === true)
+	if (httpSignature.verifySignature(signature, authUser.key.keyPem))

Explicitly checking for something to equal true seems unnecessary, since `httpSignature.verifySignature` has the return type `boolean`. ```diff - const httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem); - if (httpSignatureValidated === true) + if (httpSignature.verifySignature(signature, authUser.key.keyPem)) ```

helene commented

2023-06-23 17:13:13 +00:00

I want to make sure the code and check condition are clear on the signature verification step, which is common practice on sensitive code. Additionally, while the typings do specify that the return type should be boolean, this is not checked at runtime, meaning that if a truthy-type is returned (for example, a non-empty string, which is common in the codebase), the check would pass. I enforce the value to be strictly the boolean value true instead of being truthy. This is mostly for future-proofing, for example, in case of replacement of the http-signature package in the future, as the dependency itself looks fairly sound on that point.

I want to make sure the code and check condition are clear on the signature verification step, which is common practice on sensitive code. Additionally, while the typings do specify that the return type should be boolean, this is not checked at runtime, meaning that if a truthy-type is returned (for example, a non-empty string, which is common in the codebase), the check would pass. I enforce the value to be strictly the boolean value `true` instead of being truthy. This is mostly for future-proofing, for example, in case of replacement of the `http-signature` package in the future, as the dependency itself looks fairly sound on that point.

Johann150 marked this conversation as resolved

helene added 1 commit 2023-06-23 17:54:01 +00:00

fixup! BREAKING: activitypub: validate fetch signatures

ci/woodpecker/pr/lint-backend Pipeline was successful

Details

ci/woodpecker/pr/lint-foundkey-js Pipeline was successful

Details

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/lint-client Pipeline was successful

Details

ci/woodpecker/pr/lint-sw Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

fe0dde38c3

helene commented

2023-06-23 17:58:03 +00:00

As this is uses fixups, rebase merge is preferable to not pollute Git history

Would close #29.

As this is uses [fixups](https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---fixupamendrewordltcommitgt), rebase merge is preferable to not pollute Git history Would close #29.

Johann150 manually merged commit b600efae0d into main

2023-06-27 19:59:29 +00:00

Johann150 referenced this pull request

2023-06-27 20:00:16 +00:00

Secure fetch #29

norm referenced this pull request

2023-06-28 22:34:55 +00:00

WIP: Secure mode #169

Sign in to join this conversation.

No reviewers