🐛 Fixed RSA key generation to use 2048-bit keys #24958
Conversation
fixes #24831 - Updated RSA key generation from 1024 to 2048 bits to meet RS512 algorithm requirements - Fixed ghost_private_key/ghost_public_key generation in settings model - Fixed members_private_key/members_public_key generation in settings model - Fixed fallback key generation in MembersConfigProvider - Added migration to regenerate existing keys with proper 2048-bit length - Ensures compatibility with strict JWT libraries like jose that enforce key length requirements
- Replaced brittle text-matching tests with functional tests - Added crypto module verification of actual key bit length (2048 bits) - Added tests to verify key pairs work together for signing/verification - Added JWT RS512 algorithm compatibility tests - Fixed timing issues in MembersConfigProvider tests - Properly stubbed global logging module where needed
- Added configuration tests to verify settings.js uses 2048-bit keys - Added test to verify MembersConfigProvider fallback uses 2048-bit keys - Added test to ensure no 1024-bit configurations remain - Verified both ghost and members keys are properly configured
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
community
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ghost/core/core/server/data/migrations/versions/6.0/2025-09-22-00-00-00-regenerate-rsa-keys-2048.js (2)
11-11: Move logging inside the migration execution to avoid side‑effects on require.Logging at module scope will run when the migration file is loaded, not when it executes. Prefer logging as a first migration step within
combineTransactionalMigrations.Apply this diff:
-module.exports = combineTransactionalMigrations( - removeSetting('ghost_private_key'), - removeSetting('ghost_public_key'), - removeSetting('members_private_key'), - removeSetting('members_public_key') -); - -logging.info('Migration: Removing RSA keys to regenerate with 2048-bit length on next startup'); +module.exports = combineTransactionalMigrations( + async function logRegeneration() { + logging.info('Migration: Removing RSA keys to regenerate with 2048-bit length on next startup'); + }, + removeSetting('ghost_private_key'), + removeSetting('ghost_public_key'), + removeSetting('members_private_key'), + removeSetting('members_public_key') +);
4-9: Plan a short rollover window to avoid abrupt token invalidation.Consider temporarily serving both old and new public keys via JWKS (dual‑key rotation) to reduce breakage for logged‑in members and magic links.
ghost/core/core/server/services/members/MembersConfigProvider.js (2)
71-76: Nit: clarify warning to reflect either key missing and that the pair is temporary.Improves operator logs when only one key is absent and makes the fallback nature explicit.
- logging.warn('Could not find members_private_key, using dynamically generated keypair'); + logging.warn('Could not find members_private_key or members_public_key, generating a temporary 2048-bit keypair');
73-75: Optional: consider using Node’s built‑in crypto key generation long‑term.
crypto.generateKeyPairSync('rsa', {modulusLength: 2048, ...})removes the external dependency. Note: it emits SPKI/PKCS#8 PEMs (“BEGIN PUBLIC KEY”), which may require small downstream adjustments and test updates.ghost/core/core/server/models/settings.js (1)
3-3: Minor: keep naming consistent with Members provider.Here it’s
keypair, elsewherecreateKeypair. Consider unifying to one name for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ghost/core/core/server/data/migrations/versions/6.0/2025-09-22-00-00-00-regenerate-rsa-keys-2048.js(1 hunks)ghost/core/core/server/models/settings.js(2 hunks)ghost/core/core/server/services/members/MembersConfigProvider.js(1 hunks)ghost/core/test/e2e-api/members/jwks-well-known.test.js(1 hunks)ghost/core/test/integration/settings/settings-key-generation.test.js(1 hunks)ghost/core/test/unit/server/services/members/MembersConfigProvider.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
ghost/core/test/unit/server/services/members/MembersConfigProvider.test.js (2)
ghost/core/test/e2e-api/members/jwks-well-known.test.js (9)
should(2-2)require(3-3)crypto(1-1)publicKeyObj(87-90)keyDetails(93-93)jose(58-58)keyStore(68-68)jwks(24-24)jwks(64-64)ghost/core/test/integration/settings/settings-key-generation.test.js (14)
should(1-1)crypto(2-2)jwt(3-3)publicKeyObj(42-45)keyDetails(48-48)payload(70-73)token(76-80)decoded(83-86)decoded(143-145)jose(4-4)keyStore(118-118)jwk(119-119)testToken(134-137)jwks(122-122)
ghost/core/test/e2e-api/members/jwks-well-known.test.js (2)
ghost/core/test/integration/settings/settings-key-generation.test.js (7)
crypto(2-2)jwks(122-122)jwk(119-119)jose(4-4)keyStore(118-118)publicKeyObj(42-45)keyDetails(48-48)ghost/core/test/unit/server/services/members/MembersConfigProvider.test.js (7)
crypto(3-3)jwks(120-120)jwk(107-107)jose(102-102)keyStore(106-106)publicKeyObj(48-51)keyDetails(54-54)
ghost/core/test/integration/settings/settings-key-generation.test.js (2)
ghost/core/test/e2e-api/members/jwks-well-known.test.js (8)
require(3-3)crypto(1-1)jose(58-58)publicKeyObj(87-90)keyDetails(93-93)keyStore(68-68)jwks(24-24)jwks(64-64)ghost/core/test/unit/server/services/members/MembersConfigProvider.test.js (11)
crypto(3-3)jwt(4-4)jose(102-102)publicKeyObj(48-51)keyDetails(54-54)payload(62-65)payload(126-126)keyStore(106-106)jwk(107-107)jwks(120-120)testToken(109-118)
ghost/core/core/server/data/migrations/versions/6.0/2025-09-22-00-00-00-regenerate-rsa-keys-2048.js (1)
ghost/core/core/server/services/members/MembersConfigProvider.js (2)
require(2-2)logging(1-1)
ghost/core/core/server/services/members/MembersConfigProvider.js (1)
ghost/core/core/server/models/settings.js (1)
keypair(3-3)
🪛 Gitleaks (8.28.0)
ghost/core/test/integration/settings/settings-key-generation.test.js
[high] 38-39: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (13)
ghost/core/core/server/data/migrations/versions/6.0/2025-09-22-00-00-00-regenerate-rsa-keys-2048.js (1)
4-9: Key removal via transactional migration looks correct.Good coverage of both Ghost and Members key pairs.
ghost/core/core/server/services/members/MembersConfigProvider.js (1)
73-75: Upgrade to 2048‑bit keypair is correct for RS512.Matches RS512 requirements and aligns with tests.
ghost/core/core/server/models/settings.js (2)
29-31: Use of 2048‑bit default Members keypair LGTM.Meets RS512 requirements and is cached per‑process via the closure.
39-41: Use of 2048‑bit default Ghost keypair LGTM.Consistent with Members and tests.
ghost/core/test/unit/server/services/members/MembersConfigProvider.test.js (3)
36-56: Good: verifies 2048‑bit fallback keys and PEM types.Solid coverage of the core regression.
101-128: Good: strict jose/JWKS round‑trip validation.Catches the original 1024‑bit incompatibility with RS512.
130-149: Good: module‑level logging stub and restore.Prevents noisy logs and asserts the expected warning.
ghost/core/test/e2e-api/members/jwks-well-known.test.js (2)
83-95: Excellent modulus verification with Node crypto.Asserting
modulusLength === 2048guarantees RS512 compatibility.
20-22: No change required — tests use agent base paths so they target the mounted JWKS endpoints. The test agents prepend apiURL (MembersAPITestAgent uses "/members/", GhostAPITestAgent uses "/ghost/"), and the code mounts well-known at /members/.well-known (core/core/frontend/web/site.js -> membersService.api.middleware.wellKnown) and /ghost/.well-known (core/core/server/web/parent/backend.js -> require('../well-known')), so .get('/.well-known/jwks.json') is correct.ghost/core/test/integration/settings/settings-key-generation.test.js (4)
57-65: Ghost and Members 2048‑bit generation verified.Directly validates the upgraded defaults.
100-111: Idempotency check is valuable.Confirms no unintended key churn on repeated defaults population.
36-40: Heads‑up: secret scanner false positive on PEM regex.Static analysis may flag the RSA PEM regex as a “private key”. Add an allowlist for tests to avoid noise.
Example gitleaks config snippet:
title = "ghost-gitleaks-config" [allowlist] description = "Allow RSA PEM regex in tests" regexes = [ '''^-----BEGIN RSA PRIVATE KEY-----''', '''^-----BEGIN RSA PUBLIC KEY-----''' ] paths = [ '''ghost/core/test/.*settings-key-generation\.test\.js''', '''ghost/core/test/.*MembersConfigProvider\.test\.js''' ]
133-148: Good: RS512 + JWKS round‑trip with jose.Covers the strict path and ensures future regressions are caught.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
ghost/core/test/unit/server/services/members/MembersConfigProvider.test.js (6)
46-55: Don’t assert PEM headers; allow PKCS8/SPKI tooHeader checks make the test brittle if the provider switches encodings (PKCS1 vs PKCS8/SPKI). Validate via crypto instead.
Apply this diff:
- // Verify they are valid RSA keys - config.publicKey.should.match(/^-----BEGIN RSA PUBLIC KEY-----/); - config.privateKey.should.match(/^-----BEGIN RSA PRIVATE KEY-----/); // Use crypto to verify the key size const publicKeyObj = crypto.createPublicKey({ key: config.publicKey, format: 'pem' }); + // Validate the algorithm type without relying on PEM header format + publicKeyObj.asymmetricKeyType.should.equal('rsa');
56-59: Prefer “>= 2048” over strict equalityFuture‑proof against upgrades to 3072/4096. Also assert
keyDetailsexists.Apply this diff:
- // Verify the modulus length is 2048 bits - const keyDetails = publicKeyObj.asymmetricKeyDetails; - keyDetails.modulusLength.should.equal(2048, 'Fallback keys should be 2048 bits'); + // Verify the modulus length is at least 2048 bits + const keyDetails = publicKeyObj.asymmetricKeyDetails; + should.exist(keyDetails); + (keyDetails.modulusLength >= 2048).should.be.true('Fallback keys should be at least 2048 bits');
79-83: Also verify audience claimYou set
audiencewhen signing; verify it to fully exercise config.Apply this diff:
- const decoded = jwt.verify(token, config.publicKey, { - algorithms: ['RS512'], - issuer: config.issuer - }); + const decoded = jwt.verify(token, config.publicKey, { + algorithms: ['RS512'], + issuer: config.issuer, + audience: config.issuer + });
112-121: Assert ‘kid’ propagationConfirm the JWT header ‘kid’ matches the generated JWK to tighten the JWKS flow.
Apply this diff:
const testToken = jwt.sign( {sub: 'test-member'}, config.privateKey, { algorithm: 'RS512', keyid: jwk.kid, issuer: config.issuer, audience: config.issuer } ); + const decodedHeader = jwt.decode(testToken, {complete: true}).header; + decodedHeader.kid.should.equal(jwk.kid);
133-152: Relax logging assertion to avoid brittlenessLog copy and call counts can vary. Check that a warning was logged and contains the key name.
Apply this diff:
- warnStub.calledOnce.should.be.true(); - warnStub.firstCall.args[0].should.match(/Could not find members_private_key/); + warnStub.called.should.be.true(); + String(warnStub.firstCall.args[0]).should.match(/members_private_key/);
1-6: Hoist node-jose importMinor cleanliness: require once at top for consistency with other tests.
Apply this diff:
const should = require('should'); const sinon = require('sinon'); const crypto = require('crypto'); const jwt = require('jsonwebtoken'); +const jose = require('node-jose'); const MembersConfigProvider = require('../../../../../core/server/services/members/MembersConfigProvider'); @@ - const jose = require('node-jose');Also applies to: 105-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/test/e2e-api/members/jwks-well-known.test.js(1 hunks)ghost/core/test/unit/server/services/members/MembersConfigProvider.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/e2e-api/members/jwks-well-known.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
ghost/core/test/unit/server/services/members/MembersConfigProvider.test.js (2)
ghost/core/test/e2e-api/members/jwks-well-known.test.js (8)
require(3-3)crypto(1-1)publicKeyObj(90-93)keyDetails(96-96)jose(61-61)keyStore(71-71)jwks(27-27)jwks(67-67)ghost/core/test/integration/settings/settings-key-generation.test.js (13)
crypto(2-2)jwt(3-3)publicKeyObj(42-45)keyDetails(48-48)payload(70-73)token(76-80)decoded(83-86)decoded(143-145)jose(4-4)keyStore(118-118)jwk(119-119)testToken(134-137)jwks(122-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Inspect Docker Image
- GitHub Check: Cursor Bugbot
- GitHub Check: Setup
- GitHub Check: Setup
🔇 Additional comments (1)
ghost/core/test/unit/server/services/members/MembersConfigProvider.test.js (1)
7-10: Good suite setup and timeout10s global timeout is appropriate for 2048‑bit RSA generation.
ErisDS
left a comment
ErisDS
left a comment
There was a problem hiding this comment.
There's an ongoing discussion in slack about the high-level approach. Looking at this I noticed some unexpected implementation details, so calling these out separately.
| // Check required JWK properties for RSA keys | ||
| should.exist(jwk.kty); | ||
| should.exist(jwk.n); | ||
| should.exist(jwk.e); |
There was a problem hiding this comment.
Is there a reason this test isn't using snapshots? Is it pure AI smash?
| @@ -0,0 +1,115 @@ | |||
| const crypto = require('crypto'); | |||
| const should = require('should'); | |||
There was a problem hiding this comment.
Should is deprecated, the intention is to use node:assert for all new tests.
| removeSetting('ghost_private_key'), | ||
| removeSetting('ghost_public_key'), | ||
| removeSetting('members_private_key'), | ||
| removeSetting('members_public_key') |
There was a problem hiding this comment.
As I understand all of our settings should be created using migrations. That was an agreement that was made several years ago when we reworked the settings table.
Therefore it seems wrong to remove a setting we want and not re-add it.
|
I got hit by this issue, so I'm excited to see a fix in the works! BUT... did I understand right that API keys would have to be regenerated? Does that include the custom integration keys and staff tokens? If keys are going to get invalidated, that's the sort of thing that I'd hope for a month or more of lead time and a LOT of effort to let people using Ghost know that it's coming. Admin API keys get used for a TON of different stuff, including subscriber logins, mission-critical integrations, automated newsletter generations, headless setups, static site setups, apps, search integrations, Discord integrations, etc etc etc etc. And every single Zapier integration out there, many set up by marginally-technical folks who will NOT understand that you're going to break their stuff until you actually break it. If it's feasible, I'd much prefer a situation where existing API keys and staff tokens continue to work, either until 7.x, or at least with a couple months of lead time. Please don't break everything I've built this year all at once, please? 🥹 If it's "just" the jwks but not the custom integrations changing, that'll only break half a dozen things I've built this year.... Apologies if I've misunderstood. You've got me fearing for my sanity here. |
fixes #24831
Ghost's existing keys only used 1024 bits, which do not meet RS512 compatibility. For those using popular libraries like
joseto verify tokens, they might see errors.NOTE: Because we're generating new keys with the migration, we'd expect possible disruptions - existing magic links would not work, currently logged-in members need to re-auth, API tokens would need regenerating, etc. We could create an old key with fallback behavior to try to mitigate this, if necessary.