Skip to content

Fix key attachment bug and add dual-key support #1316

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix key attachment bug and add dual-key support #1316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions src/account/account.repository.knex.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,24 @@ export class KnexAccountRepository {
ap_outbox_url: draft.apOutbox?.href ?? null,
ap_following_url: draft.apFollowing?.href ?? null,
ap_liked_url: draft.apLiked?.href ?? null,
ap_public_key: JSON.stringify(
await exportJwk(draft.apPublicKey),
),
ap_private_key: draft.apPrivateKey
ap_public_key: (draft as any).dualKeyPairs
Copy link
Collaborator

@allouis allouis Sep 22, 2025

Choose a reason for hiding this comment

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

We shouldn't be casting to any here, the draft type should be updated

? JSON.stringify({
keys: await Promise.all(
Copy link
Collaborator

@allouis allouis Sep 22, 2025

Choose a reason for hiding this comment

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

I don't love the idea of storing two different data types in the same column here.

I think we need a discussion about either:

  1. Migrating all columns to store multiple keys
  2. Adding new columns for the new keys

(draft as any).dualKeyPairs.map((kp: { publicKey: CryptoKey, privateKey: CryptoKey }) =>
exportJwk(kp.publicKey)
)
)
})
: JSON.stringify(await exportJwk(draft.apPublicKey)),
ap_private_key: (draft as any).dualKeyPairs
? JSON.stringify({
keys: await Promise.all(
(draft as any).dualKeyPairs.map((kp: { publicKey: CryptoKey, privateKey: CryptoKey }) =>
exportJwk(kp.privateKey)
)
)
})
: draft.apPrivateKey
? JSON.stringify(await exportJwk(draft.apPrivateKey))
: null,
custom_fields: draft.customFields
Expand Down
34 changes: 23 additions & 11 deletions src/account/account.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,12 @@ export class AccountService {
private readonly events: AsyncEvents,
private readonly accountRepository: KnexAccountRepository,
private readonly fedifyContextFactory: FedifyContextFactory,
private readonly generateKeyPair: () => Promise<CryptoKeyPair> = generateCryptoKeyPair,
private readonly generateKeyPair: () => Promise<{ publicKey: CryptoKey, privateKey: CryptoKey }[]> = async () => {
Copy link
Collaborator

@allouis allouis Sep 22, 2025

Choose a reason for hiding this comment

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

This method name needs updating to reflect the new return type (singular -> plural).

Why have we stopped using the CryptoKeyPair type here?

// Generate both Ed25519 and RSA for maximum compatibility
const ed25519Keys = await generateCryptoKeyPair('Ed25519');
const rsaKeys = await generateCryptoKeyPair('RSASSA-PKCS1-v1_5');
return [ed25519Keys, rsaKeys];
},
) {}

/**
Expand Down Expand Up @@ -178,7 +183,7 @@ export class AccountService {
site: Site,
internalAccountData: InternalAccountData,
): Promise<AccountType> {
const keyPair = await this.generateKeyPair();
const keyPairs = await this.generateKeyPair();

const normalizedHost = site.host.replace(/^www\./, '');

Expand All @@ -192,10 +197,13 @@ export class AccountService {
avatarUrl: parseURL(internalAccountData.avatar_url),
bannerImageUrl: parseURL(internalAccountData.banner_image_url),
customFields: null,
apPublicKey: keyPair.publicKey,
apPrivateKey: keyPair.privateKey,
apPublicKey: keyPairs[0].publicKey,
apPrivateKey: keyPairs[0].privateKey,
});

// Attach dual keys for repository to handle
(draft as any).dualKeyPairs = keyPairs;

try {
const account = await this.accountRepository.create(draft);
const returnVal = await this.getByInternalId(account.id);
Expand Down Expand Up @@ -241,18 +249,22 @@ export class AccountService {
)?.ap_private_key;

if (!hasPrivateKey) {
const newKeyPair = await this.generateKeyPair();
const newKeyPairs = await this.generateKeyPair();
await this.db('accounts')
.where({
id: existingAccount.id,
})
.update({
ap_public_key: JSON.stringify(
await exportJwk(newKeyPair.publicKey),
),
ap_private_key: JSON.stringify(
await exportJwk(newKeyPair.privateKey),
),
ap_public_key: JSON.stringify({
keys: await Promise.all(
newKeyPairs.map(kp => exportJwk(kp.publicKey))
)
}),
ap_private_key: JSON.stringify({
keys: await Promise.all(
newKeyPairs.map(kp => exportJwk(kp.privateKey))
)
}),
});
}

Expand Down
2 changes: 1 addition & 1 deletion src/account/account.service.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ describe('AccountService', () => {
let asyncEvents: AsyncEvents;
let knexAccountRepository: KnexAccountRepository;
let fedifyContextFactory: FedifyContextFactory;
let generateKeyPair: () => Promise<CryptoKeyPair>;
let generateKeyPair: () => Promise<{ publicKey: CryptoKey, privateKey: CryptoKey }[]>;
let accountService: AccountService;

beforeEach(() => {
Expand Down
41 changes: 30 additions & 11 deletions src/dispatchers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ export const actorDispatcher = (

const account = await accountService.getDefaultAccountForSite(site);

// Get key pairs for proper attachment
const keyPairs = await ctx.getActorKeyPairs(identifier);

const person = new Person({
id: new URL(account.ap_id),
name: account.name,
Expand All @@ -69,9 +72,9 @@ export const actorDispatcher = (
followers: new URL(account.ap_followers_url),
liked: new URL(account.ap_liked_url),
url: new URL(account.url || account.ap_id),
publicKeys: (await ctx.getActorKeyPairs(identifier)).map(
(key) => key.cryptographicKey,
),
// FIX: Use correct Fedify properties for keys
publicKey: keyPairs[0]?.cryptographicKey, // For HTTP Signatures
assertionMethods: keyPairs.map((kp) => kp.multikey), // For Object Integrity Proofs
});

return person;
Expand Down Expand Up @@ -99,16 +102,32 @@ export const keypairDispatcher = (
}

try {
const publicKeys = JSON.parse(account.ap_public_key);
const privateKeys = JSON.parse(account.ap_private_key);

// Handle dual-key format
if (publicKeys.keys && privateKeys.keys) {
const keyPairs = [];
for (let i = 0; i < publicKeys.keys.length; i++) {
keyPairs.push({
publicKey: await importJwk(
publicKeys.keys[i] as JsonWebKey,
'public',
),
privateKey: await importJwk(
privateKeys.keys[i] as JsonWebKey,
'private',
),
});
}
return keyPairs;
}

// Fallback for single key format (backwards compatibility)
return [
{
publicKey: await importJwk(
JSON.parse(account.ap_public_key) as JsonWebKey,
'public',
),
privateKey: await importJwk(
JSON.parse(account.ap_private_key) as JsonWebKey,
'private',
),
publicKey: await importJwk(publicKeys as JsonWebKey, 'public'),
privateKey: await importJwk(privateKeys as JsonWebKey, 'private'),
},
];
} catch (_err) {
Expand Down
10 changes: 5 additions & 5 deletions src/test/account-entity-test-helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export async function createInternalAccountDraftData(overrides: {
bannerImageUrl: URL | null;
customFields: Record<string, string> | null;
}) {
const keyPair = await generateTestCryptoKeyPair();
const keyPairs = await generateTestCryptoKeyPair();

return {
isInternal: true as const,
Expand All @@ -23,8 +23,8 @@ export async function createInternalAccountDraftData(overrides: {
avatarUrl: overrides.avatarUrl,
bannerImageUrl: overrides.bannerImageUrl,
customFields: overrides.customFields,
apPublicKey: keyPair.publicKey,
apPrivateKey: keyPair.privateKey,
apPublicKey: keyPairs[0].publicKey,
apPrivateKey: keyPairs[0].privateKey,
};
}

Expand All @@ -44,7 +44,7 @@ export async function createExternalAccountDraftData(overrides: {
apFollowing?: URL | null;
apLiked?: URL | null;
}) {
const keyPair = await generateTestCryptoKeyPair();
const keyPairs = await generateTestCryptoKeyPair();

return {
isInternal: false as const,
Expand All @@ -62,7 +62,7 @@ export async function createExternalAccountDraftData(overrides: {
apOutbox: overrides.apOutbox || null,
apFollowing: overrides.apFollowing || null,
apLiked: overrides.apLiked || null,
apPublicKey: keyPair.publicKey,
apPublicKey: keyPairs[0].publicKey,
};
}

Expand Down
14 changes: 9 additions & 5 deletions src/test/crypto-key-pair.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
import { generateCryptoKeyPair } from '@fedify/fedify';

let cachedKeyPair: Promise<CryptoKeyPair> | null = null;
let cachedKeyPairs: Promise<CryptoKeyPair[]> | null = null;

export async function generateTestCryptoKeyPair() {
if (cachedKeyPair !== null) {
return cachedKeyPair;
if (cachedKeyPairs !== null) {
return cachedKeyPairs;
}

cachedKeyPair = generateCryptoKeyPair();
cachedKeyPairs = (async () => {
const ed25519Keys = await generateCryptoKeyPair('Ed25519');
const rsaKeys = await generateCryptoKeyPair('RSASSA-PKCS1-v1_5');
return [ed25519Keys, rsaKeys];
})();

return cachedKeyPair;
return cachedKeyPairs;
}