-
-
Notifications
You must be signed in to change notification settings - Fork 21
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.
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
base: main
Are you sure you want to change the base?
Conversation
- Use publicKey (singular) and assertionMethods for proper key attachment - Generate both Ed25519 and RSA keys for broader compatibility - Handle dual-key JSON storage format - Maintain backwards compatibility with existing single-key accounts The previous implementation used publicKeys (plural) which is not a valid Fedify property, causing federation to fail. This fix ensures keys are properly attached to the Person object, enabling authentication with other ActivityPub servers.
|
@schuettc I'm not seeing that public keys are not attached to the Person objects 🤔 You're right that the plural is deprecated by fedify, but it does still work and is correctly attached - is there a specific bug you're running into without these changes? If we can get replication steps that would be awesome |
| await exportJwk(draft.apPublicKey), | ||
| ), | ||
| ap_private_key: draft.apPrivateKey | ||
| ap_public_key: (draft as any).dualKeyPairs |
There was a problem hiding this comment.
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
| ap_private_key: draft.apPrivateKey | ||
| ap_public_key: (draft as any).dualKeyPairs | ||
| ? JSON.stringify({ | ||
| keys: await Promise.all( |
There was a problem hiding this comment.
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:
- Migrating all columns to store multiple keys
- Adding new columns for the new keys
| private readonly accountRepository: KnexAccountRepository, | ||
| private readonly fedifyContextFactory: FedifyContextFactory, | ||
| private readonly generateKeyPair: () => Promise<CryptoKeyPair> = generateCryptoKeyPair, | ||
| private readonly generateKeyPair: () => Promise<{ publicKey: CryptoKey, privateKey: CryptoKey }[]> = async () => { |
There was a problem hiding this comment.
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?
allouis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I 100% understand the bug-fix here, this seems to primarily be adding support for the object integrity proofs feature, is that right?
If there is a bug fix here, it would be great to get it identified & merged separately to the feature, as I think it will be a smaller changeset
Fix key attachment bug and add dual-key support for ActivityPub federation
Description
This PR addresses a bug that prevents the Ghost ActivityPub implementation from federating with other ActivityPub servers. The issue stems from cryptographic keys not being properly attached to the Person object, causing authentication failures when other servers attempt to verify signatures.
Additionally, this PR enhances compatibility across the Fediverse by implementing dual-key support (both Ed25519 and RSA), ensuring broader interoperability with different ActivityPub implementations.
The Problem
Current Behavior
The current implementation uses
publicKeys(plural) property which is not recognized by Fedify:This results in:
Root Cause
According to the Fedify documentation, the Person class expects:
publicKey(singular) - Contains aCryptographicKeyinstance for HTTP SignaturesassertionMethods(plural) - An array ofMultikeyinstances for Object Integrity ProofsThe current code uses
publicKeys(plural) which is not a valid Person property in Fedify.The Solution
1. Fixed Key Attachment
Updated
actorDispatcherto use the correct Fedify properties:2. Dual-Key Support
Implemented generation of both Ed25519 and RSA keys for maximum compatibility:
3. Backwards Compatibility
The implementation maintains full backwards compatibility:
keypairDispatcherhandles both formats transparentlyChanges Made
Files Modified (6 files total)
src/dispatchers.ts
actorDispatcherto attach keys using correct propertieskeypairDispatcherto handle dual-key JSON formatsrc/account/account.service.ts
src/account/account.repository.knex.ts
src/account/account.service.unit.test.ts
src/test/account-entity-test-helpers.ts
src/test/crypto-key-pair.ts
Testing
Automated Tests
All tests pass with minimal modifications to support key arrays:
yarn test:types)yarn test:unit)yarn test:integration)Modified test files to support dual-key arrays:
src/test/crypto-key-pair.ts- Generate dual keys for testssrc/test/account-entity-test-helpers.ts- Use first key from arraysrc/account/account.service.unit.test.ts- Updated type signatureProduction Verification
These changes are running in production at https://subaud.io:
Dual-key types confirmed:
z6MkzgghFederation capability - The site is discoverable and keys are properly formatted for federation.
Issues
Resolves: #1230