BREAKING: activitypub: validate fetch signatures #399

Manually merged
Johann150 merged 2 commits from helene/FoundKey:feature/verify-fetch-signatures into main 2023-06-27 19:59:29 +00:00
6 changed files with 21 additions and 32 deletions
Showing only changes of commit fe0dde38c3 - Show all commits

View file

@ -1,11 +0,0 @@
export class allowUnsignedFetches1687460719276 {
name = 'allowUnsignedFetches1687460719276'
async up(queryRunner) {
await queryRunner.query(`ALTER TABLE "meta" ADD "allowUnsignedFetches" bool default false`);
}
async down(queryRunner) {
await queryRunner.query(`ALTER TABLE "meta" DROP COLUMN "allowUnsignedFetches"`);
}
}

View file

@ -61,6 +61,7 @@ export function loadConfig(): Config {
proxyRemoteFiles: false,
maxFileSize: 262144000, // 250 MiB
maxNoteTextLength: 3000,
allowUnsignedFetches: false,
}, config);
mixin.version = meta.version;

View file

@ -68,6 +68,8 @@ export type Source = {
notFound?: string;
error?: string;
};
allowUnsignedFetches?: boolean;
};
/**

View file

@ -82,11 +82,6 @@ export class Meta {
})
public blockedHosts: string[];
@Column('boolean', {
default: false
})
public allowUnsignedFetches: boolean;
@Column('varchar', {
length: 512,
nullable: true,

View file

@ -61,11 +61,14 @@ export function setResponseType(ctx: Router.RouterContext): void {
}
}
export function handleSignatureResult(ctx: Router.RouterContext, result: SignatureValidationResult): boolean {
async function handleSignature(ctx: Router.RouterContext): Promise<boolean> {
const result = await validateFetchSignature(ctx.req);
switch (result) {
// Fetch signature verification is disabled.
case 'always':
ctx.set('Cache-Control', 'public, max-age=180');
return true;
// Fetch signature verification succeeded.
case 'valid':
ctx.set('Cache-Control', 'no-store');
return true;
@ -92,7 +95,7 @@ router.post('/users/:user/inbox', json(), inbox);
// note
router.get('/notes/:note', async (ctx, next) => {
if (!isActivityPubReq(ctx)) return await next();
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
if (!(await handleSignature(ctx))) return;
const note = await Notes.findOneBy({
id: ctx.params.note,
@ -129,7 +132,7 @@ router.get('/notes/:note/activity', async ctx => {
ctx.redirect(`/notes/${ctx.params.note}`);
return;
}
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
if (!(await handleSignature(ctx))) return;
const note = await Notes.findOneBy({
id: ctx.params.note,
@ -149,25 +152,25 @@ router.get('/notes/:note/activity', async ctx => {
// outbox
router.get('/users/:user/outbox', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
if (!(await handleSignature(ctx))) return;
return await Outbox(ctx);
});
// followers
router.get('/users/:user/followers', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
if (!(await handleSignature(ctx))) return;
return await Followers(ctx);
});
// following
router.get('/users/:user/following', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
if (!(await handleSignature(ctx))) return;
return await Following(ctx);
});
// featured
router.get('/users/:user/collections/featured', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
if (!(await handleSignature(ctx))) return;
return await Featured(ctx);
});
@ -224,7 +227,7 @@ router.get('/users/:user', async (ctx, next) => {
// validate and enforce signatures.
if (user == null || !isInstanceActor(user))
{
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
if (!(await handleSignature(ctx))) return;
}
else if (isInstanceActor(user))
{
@ -237,7 +240,7 @@ router.get('/users/:user', async (ctx, next) => {
router.get('/@:user', async (ctx, next) => {
if (!isActivityPubReq(ctx)) return await next();
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
if (!(await handleSignature(ctx))) return;
const user = await Users.findOneBy({
usernameLower: ctx.params.user.toLowerCase(),
@ -270,7 +273,7 @@ router.get('/emojis/:emoji', async ctx => {
// like
router.get('/likes/:like', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
if (!(await handleSignature(ctx))) return;
const reaction = await NoteReactions.findOneBy({ id: ctx.params.like });
if (reaction == null) {
@ -294,7 +297,7 @@ router.get('/likes/:like', async ctx => {
// follow
router.get('/follows/:follower/:followee', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
if (!(await handleSignature(ctx))) return;
// This may be used before the follow is completed, so we do not
// check if the following exists.

View file

@ -1,12 +1,12 @@
import httpSignature from '@peertube/http-signature';
import { toPuny } from '@/misc/convert-host.js';
import { fetchMeta } from '@/misc/fetch-meta.js';
import { extractDbHost } from '@/misc/convert-host.js';
import { shouldBlockInstance } from '@/misc/should-block-instance.js';
import { authUserFromKeyId, getAuthUser } from '@/remote/activitypub/misc/auth-user.js';
import { getApId, isActor } from '@/remote/activitypub/type.js';
import { StatusError } from '@/misc/fetch.js';
import { Resolver } from '@/remote/activitypub/resolver.js';
import { createPerson } from '@/remote/activitypub/models/person.js';
import config from '@/config/index.js';
export type SignatureValidationResult = 'missing' | 'invalid' | 'rejected' | 'valid' | 'always';
@ -39,10 +39,9 @@ async function resolveKeyId(keyId: string, resolver: Resolver): Promise<AuthUser
}
export async function validateFetchSignature(req: IncomingMessage): Promise<SignatureValidationResult> {
Johann150 marked this conversation as resolved
Review

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.
Review

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.
const meta = await fetchMeta();
let signature;
if (meta.allowUnsignedFetches === true)
if (config.allowUnsignedFetches === true)
return 'always';
try {
@ -60,7 +59,7 @@ export async function validateFetchSignature(req: IncomingMessage): Promise<Sign
if (keyIdLower.startsWith('acct:'))
return 'invalid';
const host = toPuny(new URL(keyIdLower).hostname);
const host = extractDbHost(keyIdLower);
Johann150 marked this conversation as resolved
Review
-	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'; ```
Review

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)
Review

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

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

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.
// Reject if the host is blocked.
if (await shouldBlockInstance(host))