From 24672019074a30384bcfa7cdc6af148e39338ef3 Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Tue, 15 Oct 2024 09:55:19 +0200 Subject: [PATCH 01/16] feat: remove seed from internal storage if we have it from config --- lib/core/security/tokenRepository.ts | 35 ++++++++--- lib/kuzzle/internalIndexHandler.js | 35 +++++------ lib/kuzzle/kuzzle.ts | 3 - test/kuzzle/internalIndexHandler.test.js | 77 ++++++++++++++++-------- test/mocks/internalIndexHandler.mock.js | 1 - 5 files changed, 97 insertions(+), 54 deletions(-) diff --git a/lib/core/security/tokenRepository.ts b/lib/core/security/tokenRepository.ts index 9c9c1fdeb0..b5ce8de283 100644 --- a/lib/core/security/tokenRepository.ts +++ b/lib/core/security/tokenRepository.ts @@ -136,6 +136,24 @@ export class TokenRepository extends ObjectRepository { global.kuzzle.onAsk("core:security:token:verify", (hash) => this.verifyToken(hash), ); + + // ? those checks are necessary to detect JWT seed changes and delete existing token if necessary + const existingTokens = await global.kuzzle.ask( + "core:cache:internal:searchKeys", + "repos/kuzzle/token/*", + ); + + if (existingTokens.length > 0) { + try { + const [, token] = existingTokens[0].split("#"); + await this.verifyToken(token); + } catch (e) { + // ? seed has changed + if (e.id === "security.token.invalid") { + await global.kuzzle.ask("core:cache:internal:del", existingTokens); + } + } + } } /** @@ -189,8 +207,10 @@ export class TokenRepository extends ObjectRepository { async generateToken( user: User, { - algorithm = global.kuzzle.config.security.jwt.algorithm, - expiresIn = global.kuzzle.config.security.jwt.expiresIn, + algorithm = global.kuzzle.config.security.authToken.algorithm ?? + global.kuzzle.config.security.jwt.algorithm, + expiresIn = global.kuzzle.config.security.authToken.expiresIn ?? + global.kuzzle.config.security.jwt.expiresIn, bypassMaxTTL = false, type = "authToken", singleUse = false, @@ -211,7 +231,8 @@ export class TokenRepository extends ObjectRepository { const maxTTL = type === "apiKey" ? global.kuzzle.config.security.apiKey.maxTTL - : global.kuzzle.config.security.jwt.maxTTL; + : global.kuzzle.config.security.authToken.maxTTL ?? + global.kuzzle.config.security.jwt.maxTTL; if ( !bypassMaxTTL && @@ -318,14 +339,14 @@ export class TokenRepository extends ObjectRepository { throw new jwt.JsonWebTokenError("Invalid token"); } } catch (err) { - if (err instanceof jwt.TokenExpiredError) { - throw securityError.get("expired"); - } - if (err instanceof jwt.JsonWebTokenError) { throw securityError.get("invalid"); } + if (err instanceof jwt.TokenExpiredError) { + throw securityError.get("expired"); + } + throw securityError.getFrom(err, "verification_error", err.message); } diff --git a/lib/kuzzle/internalIndexHandler.js b/lib/kuzzle/internalIndexHandler.js index e944284f86..72358e8378 100644 --- a/lib/kuzzle/internalIndexHandler.js +++ b/lib/kuzzle/internalIndexHandler.js @@ -111,6 +111,7 @@ class InternalIndexHandler extends Store { const bootstrapped = await this.exists("config", this._BOOTSTRAP_DONE_ID); if (bootstrapped) { + await this._initSecret(); return; } @@ -150,7 +151,7 @@ class InternalIndexHandler extends Store { await this.createInitialValidations(); debug("Bootstrapping JWT secret"); - await this._persistSecret(); + await this._initSecret(); // Create datamodel version await this.create( @@ -202,24 +203,24 @@ class InternalIndexHandler extends Store { await Bluebird.all(promises); } - async getSecret() { - const response = await this.get("config", this._JWT_SECRET_ID); + async _initSecret() { + const { authToken, jwt } = global.kuzzle.config.security; + const configSeed = + authToken && authToken.secret ? authToken.secret : jwt && jwt.secret; - return response._source.seed; - } - - async _persistSecret() { - const seed = - global.kuzzle.config.security.jwt.secret || - crypto.randomBytes(512).toString("hex"); + let storedSeed = await this.exists("config", this._JWT_SECRET_ID); - await this.create( - "config", - { seed }, - { - id: this._JWT_SECRET_ID, - }, - ); + if (!configSeed && !storedSeed) { + storedSeed = crypto.randomBytes(512).toString("hex"); + await this.create( + "config", + { seed: storedSeed }, + { id: this._JWT_SECRET_ID }, + ); + } + global.kuzzle.secret = configSeed + ? configSeed + : (await this.get("config", this._JWT_SECRET_ID))._source.seed; } } diff --git a/lib/kuzzle/kuzzle.ts b/lib/kuzzle/kuzzle.ts index 683c1701ae..937b45d6a8 100644 --- a/lib/kuzzle/kuzzle.ts +++ b/lib/kuzzle/kuzzle.ts @@ -259,9 +259,6 @@ class Kuzzle extends KuzzleEventEmitter { // This will init the cluster module if enabled this.id = await this.initKuzzleNode(); - // Secret used to generate JWTs - this.secret = await this.internalIndex.getSecret(); - this.vault = vault.load(options.vaultKey, options.secretsFile); await this.validation.init(); diff --git a/test/kuzzle/internalIndexHandler.test.js b/test/kuzzle/internalIndexHandler.test.js index 1b68a85e52..ae6080bb9e 100644 --- a/test/kuzzle/internalIndexHandler.test.js +++ b/test/kuzzle/internalIndexHandler.test.js @@ -52,6 +52,8 @@ describe("#kuzzle/InternalIndexHandler", () => { internalIndexHandler = new InternalIndexHandler(); + sinon.stub(internalIndexHandler, "_initSecret").resolves(); + await internalIndexHandler.init(); should(kuzzle.ask).calledWith( @@ -113,9 +115,11 @@ describe("#kuzzle/InternalIndexHandler", () => { kuzzle.ask.withArgs("core:storage:private:document:exist").resolves(true); sinon.stub(internalIndexHandler, "_bootstrapSequence").resolves(); + sinon.stub(internalIndexHandler, "_initSecret").resolves(); await internalIndexHandler.init(); + should(internalIndexHandler._initSecret).called(); should(internalIndexHandler._bootstrapSequence).not.called(); should(kuzzle.ask).not.calledWith( @@ -174,13 +178,13 @@ describe("#kuzzle/InternalIndexHandler", () => { it("should trigger a complete bootstrap of the internal structures", async () => { sinon.stub(internalIndexHandler, "createInitialSecurities"); sinon.stub(internalIndexHandler, "createInitialValidations"); - sinon.stub(internalIndexHandler, "_persistSecret"); + sinon.stub(internalIndexHandler, "_initSecret"); await internalIndexHandler._bootstrapSequence(); should(internalIndexHandler.createInitialSecurities).called(); should(internalIndexHandler.createInitialValidations).called(); - should(internalIndexHandler._persistSecret).called(); + should(internalIndexHandler._initSecret).called(); should(kuzzle.ask).calledWith( "core:storage:private:document:create", @@ -309,7 +313,7 @@ describe("#kuzzle/InternalIndexHandler", () => { }); }); - describe("#_persistSecret", () => { + describe("#_initSecret", () => { const randomBytesMock = sinon.stub().returns(Buffer.from("12345")); before(() => { @@ -332,10 +336,22 @@ describe("#kuzzle/InternalIndexHandler", () => { it("should use the configured seed, if one is present", async () => { kuzzle.config.security.jwt.secret = "foobar"; + kuzzle.ask + .withArgs( + "core:storage:private:document:get", + internalIndexName, + "config", + internalIndexHandler._JWT_SECRET_ID, + ) + .resolves({ + _source: { + seed: "foobar", + }, + }); - await internalIndexHandler._persistSecret(); + await internalIndexHandler._initSecret(); - should(kuzzle.ask).calledWith( + should(kuzzle.ask).not.calledWith( "core:storage:private:document:create", internalIndexName, "config", @@ -347,7 +363,35 @@ describe("#kuzzle/InternalIndexHandler", () => { }); it("should auto-generate a new random seed if none is present in the config file", async () => { - await internalIndexHandler._persistSecret(); + kuzzle.ask + .withArgs( + "core:storage:private:document:exists", + internalIndexName, + "config", + internalIndexHandler._JWT_SECRET_ID, + ) + .resolves(false); + kuzzle.ask + .withArgs( + "core:storage:private:document:create", + internalIndexName, + "config", + internalIndexHandler._JWT_SECRET_ID, + ) + .resolves(); + kuzzle.ask + .withArgs( + "core:storage:private:document:get", + internalIndexName, + "config", + internalIndexHandler._JWT_SECRET_ID, + ) + .resolves({ + _source: { + seed: randomBytesMock().toString("hex"), + }, + }); + await internalIndexHandler._initSecret(); should(kuzzle.ask).calledWith( "core:storage:private:document:create", @@ -365,26 +409,7 @@ describe("#kuzzle/InternalIndexHandler", () => { kuzzle.ask.withArgs("core:storage:private:document:create").rejects(err); - return should(internalIndexHandler._persistSecret()).rejectedWith(err); - }); - }); - - describe("#getSecret", () => { - it("should fetch the secret seed from the storage space", async () => { - kuzzle.ask.withArgs("core:storage:private:document:get").resolves({ - _source: { - seed: "foobar", - }, - }); - - await should(internalIndexHandler.getSecret()).fulfilledWith("foobar"); - - should(kuzzle.ask).calledWith( - "core:storage:private:document:get", - internalIndexName, - "config", - internalIndexHandler._JWT_SECRET_ID, - ); + return should(internalIndexHandler._initSecret()).rejectedWith(err); }); }); }); diff --git a/test/mocks/internalIndexHandler.mock.js b/test/mocks/internalIndexHandler.mock.js index dd27ca740a..9b75fb023a 100644 --- a/test/mocks/internalIndexHandler.mock.js +++ b/test/mocks/internalIndexHandler.mock.js @@ -11,7 +11,6 @@ class InternalIndexHandlerMock extends InternalIndexHandler { sinon.stub(this, "init"); sinon.stub(this, "createInitialSecurities").resolves(); sinon.stub(this, "createInitialValidations").resolves(); - sinon.stub(this, "getSecret").resolves(); } } From 7b4b3360a6f80e42c40ecc6398b8d12c31478c7e Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Tue, 15 Oct 2024 09:56:38 +0200 Subject: [PATCH 02/16] docs: add warning for security concerns on deploying in prod with custom secret --- .../getting-started/deploy-your-application/index.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/doc/2/guides/getting-started/deploy-your-application/index.md b/doc/2/guides/getting-started/deploy-your-application/index.md index 48b44c29a0..c9da850166 100644 --- a/doc/2/guides/getting-started/deploy-your-application/index.md +++ b/doc/2/guides/getting-started/deploy-your-application/index.md @@ -48,6 +48,18 @@ This deployment does not use any SSL encryption (HTTPS). A production deployment must include a reverse proxy to securize the connection with SSL. ::: +::: warning +#### Production Deployment: Auth Token Secret + +For every production deployment of Kuzzle, it is essential to set the kuzzle_security__authToken__secret environment variable. This ensures that the JWT secrets used for authenticating requests are generated externally and not stored in Elasticsearch. By managing the secret through an environment variable, you enhance security, prevent potential data exposure, and ensure tokens remain valid only as long as the secret remains unchanged. + +Important: If the `kuzzle_security__authToken__secret` value is changed when Kuzzle restarts, all existing tokens will be invalidated. This ensures that only tokens signed with the current secret remain valid, adding an extra layer of security. + +For default configuration values, you can refer to [the sample Kuzzle configuration file](https://github.com/kuzzleio/kuzzle/blob/master/.kuzzlerc.sample.jsonc). + +Note: If the secret is not set, Kuzzle will fallback to a less secure method of generating and storing the secret, which is not recommended for production environments. +::: + ## Prepare our Docker Compose deployment We are going to write a `docker-compose.yml` file that describes our services. From c9cb2b869a1e94ff19a8812415985290d7efb95f Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Wed, 30 Oct 2024 11:39:45 +0100 Subject: [PATCH 03/16] fix: do not store api keys in internal index, use fingerprint instead --- lib/core/security/tokenRepository.ts | 93 +++++++++++----------------- lib/model/storage/apiKey.js | 34 ++++++---- lib/model/storage/baseModel.js | 3 +- test/model/storage/apiKey.test.js | 12 +++- 4 files changed, 68 insertions(+), 74 deletions(-) diff --git a/lib/core/security/tokenRepository.ts b/lib/core/security/tokenRepository.ts index b5ce8de283..de916e9b0a 100644 --- a/lib/core/security/tokenRepository.ts +++ b/lib/core/security/tokenRepository.ts @@ -30,8 +30,8 @@ import { Token } from "../../model/security/token"; import { User } from "../../model/security/user"; import ApiKey from "../../model/storage/apiKey"; import debugFactory from "../../util/debug"; -import { Mutex } from "../../util/mutex"; import { ObjectRepository } from "../shared/ObjectRepository"; +import { sha256 } from "../../util/crypto"; const securityError = kerror.wrap("security", "token"); const debug = debugFactory("kuzzle:bootstrap:tokens"); @@ -61,8 +61,6 @@ export class TokenRepository extends ObjectRepository { } async init() { - await this.loadApiKeys(); - /** * Assign an existing token to a user. Stores the token in Kuzzle's cache. * @param {String} hash - JWT @@ -272,10 +270,21 @@ export class TokenRepository extends ObjectRepository { if (type === "apiKey") { encodedToken = Token.APIKEY_PREFIX + encodedToken; - } else { - encodedToken = Token.AUTH_PREFIX + encodedToken; + + // For API keys, we don't persist the token + const expiresAt = + parsedExpiresIn === -1 ? -1 : Date.now() + parsedExpiresIn; + return new Token({ + _id: `${user._id}#${encodedToken}`, + expiresAt, + jwt: encodedToken, + ttl: parsedExpiresIn, + userId: user._id, + }); } + encodedToken = Token.AUTH_PREFIX + encodedToken; + // Persist regular tokens return this.persistForUser(encodedToken, user._id, { singleUse, ttl: parsedExpiresIn, @@ -329,11 +338,13 @@ export class TokenRepository extends ObjectRepository { return this.anonymousToken; } + const isApiKey = token.startsWith(Token.APIKEY_PREFIX); + const tokenWithoutPrefix = this.removeTokenPrefix(token); + let decoded = null; try { - decoded = jwt.verify(this.removeTokenPrefix(token), global.kuzzle.secret); - + decoded = jwt.verify(tokenWithoutPrefix, global.kuzzle.secret); // probably forged token => throw without providing any information if (!decoded._id) { throw new jwt.JsonWebTokenError("Invalid token"); @@ -350,7 +361,23 @@ export class TokenRepository extends ObjectRepository { throw securityError.getFrom(err, "verification_error", err.message); } - let userToken: Token; + if (isApiKey) { + const fingerprint = sha256(token); + + const apiKey = await ApiKey.load(decoded._id, fingerprint); + + const userToken = new Token({ + _id: `${decoded._id}#${token}`, + expiresAt: apiKey.expiresAt, + jwt: token, + ttl: apiKey.ttl, + userId: decoded._id, + }); + + return userToken; + } + + let userToken; try { userToken = await this.loadForUser(decoded._id, token); @@ -358,7 +385,6 @@ export class TokenRepository extends ObjectRepository { if (err instanceof UnauthorizedError) { throw err; } - throw securityError.getFrom(err, "verification_error", err.message); } @@ -457,55 +483,6 @@ export class TokenRepository extends ObjectRepository { await Promise.all(promises); } - /** - * Loads authentication token from API key into Redis - */ - private async loadApiKeys() { - const mutex = new Mutex("ApiKeysBootstrap", { - timeout: -1, - ttl: 30000, - }); - - await mutex.lock(); - - try { - const bootstrapped = await global.kuzzle.ask( - "core:cache:internal:get", - BOOTSTRAP_DONE_KEY, - ); - - if (bootstrapped) { - debug("API keys already in cache. Skip."); - return; - } - - debug("Loading API keys into Redis"); - - const promises = []; - - await ApiKey.batchExecute({ match_all: {} }, (documents) => { - for (const { _source } of documents) { - promises.push( - this.persistForUser(_source.token, _source.userId, { - singleUse: false, - ttl: _source.ttl, - }), - ); - } - }); - - await Promise.all(promises); - - await global.kuzzle.ask( - "core:cache:internal:store", - BOOTSTRAP_DONE_KEY, - 1, - ); - } finally { - await mutex.unlock(); - } - } - /** * The repository main class refreshes automatically the TTL * of accessed entries, letting only unaccessed entries expire diff --git a/lib/model/storage/apiKey.js b/lib/model/storage/apiKey.js index 108d17e336..e7a21bc2a7 100644 --- a/lib/model/storage/apiKey.js +++ b/lib/model/storage/apiKey.js @@ -49,7 +49,7 @@ class ApiKey extends BaseModel { serialize({ includeToken = false } = {}) { const serialized = super.serialize(); - if (!includeToken) { + if (!includeToken && this.token) { delete serialized._source.token; } @@ -107,7 +107,6 @@ class ApiKey extends BaseModel { description, expiresAt: token.expiresAt, fingerprint, - token: token.jwt, ttl: token.ttl, userId: user._id, }, @@ -116,6 +115,8 @@ class ApiKey extends BaseModel { await apiKey.save({ refresh, userId: creatorId }); + apiKey.token = token.jwt; + return apiKey; } @@ -127,16 +128,27 @@ class ApiKey extends BaseModel { * * @returns {ApiKey} */ - static async load(userId, id) { - const apiKey = await super.load(id); - - if (userId !== apiKey.userId) { - throw kerror.get("services", "storage", "not_found", id, { - message: `ApiKey "${id}" not found for user "${userId}".`, - }); + static async load(userId, fingerprint) { + const query = { + term: { + userId, + }, + }; + + const apiKeys = await super.search({ query }); + if (apiKeys.length > 0) { + const apiKey = apiKeys.find((a) => a.fingerprint === fingerprint); + if (userId !== apiKey.userId) { + throw kerror.get("services", "storage", "not_found", fingerprint, { + message: `ApiKey "${fingerprint}" not found for user "${userId}".`, + }); + } + + return apiKey; } - - return apiKey; + throw kerror.get("services", "storage", "not_found", fingerprint, { + message: `ApiKey "${fingerprint}" not found for user "${userId}".`, + }); } /** diff --git a/lib/model/storage/baseModel.js b/lib/model/storage/baseModel.js index bb0ce0a49d..97e014593b 100644 --- a/lib/model/storage/baseModel.js +++ b/lib/model/storage/baseModel.js @@ -218,7 +218,6 @@ class BaseModel { searchBody, options, ); - return resp.hits.map((hit) => this._instantiateFromDb(hit)); } @@ -232,7 +231,7 @@ class BaseModel { static truncate({ refresh } = {}) { return this.deleteByQuery({ match_all: {} }, { refresh }); } - + // ? This looks not in use anymore ? static batchExecute(query, callback) { return global.kuzzle.internalIndex.mExecute( this.collection, diff --git a/test/model/storage/apiKey.test.js b/test/model/storage/apiKey.test.js index ffcc9b876c..8e392cb047 100644 --- a/test/model/storage/apiKey.test.js +++ b/test/model/storage/apiKey.test.js @@ -116,8 +116,8 @@ describe("ApiKey", () => { describe("ApiKey.load", () => { it("should throw if the key does not belong to the provided user", async () => { - const loadStub = sinon - .stub(BaseModel, "load") + const searchStub = sinon + .stub(BaseModel, "search") .resolves({ userId: "mylehuong" }); const promise = ApiKey.load("aschen", "api-key-id"); @@ -126,7 +126,13 @@ describe("ApiKey", () => { id: "services.storage.not_found", }); - should(loadStub).be.calledWith("api-key-id"); + should(searchStub).be.calledWith({ + query: { + term: { + userId: "aschen", + }, + }, + }); }); }); From f17c37fcd3cc3949af2d0a2c5f0ee8179e110c26 Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Wed, 30 Oct 2024 12:38:00 +0100 Subject: [PATCH 04/16] tests: fix legacy test --- features/step_definitions/auth-steps.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/features/step_definitions/auth-steps.js b/features/step_definitions/auth-steps.js index 2acdd73603..541c8f288a 100644 --- a/features/step_definitions/auth-steps.js +++ b/features/step_definitions/auth-steps.js @@ -27,21 +27,20 @@ Given( should(token).not.be.undefined(); this.sdk.jwt = token; - - const { valid } = await this.sdk.auth.checkToken(); - - this.sdk.jwt = previousToken; - if (not) { - should(valid).be.false("Provided token is valid"); + should(await this.sdk.auth.checkToken()).throwError({ + id: "services.storage.not_found", + }); } else { + const { valid } = await this.sdk.auth.checkToken(); + this.sdk.jwt = previousToken; should(valid).be.true("Provided token is invalid"); } }, ); Given("I save the created API key", function () { - this.props.token = this.props.result._source.token; + this.props.token = this.props.result.token; }); Given( From f3139c11bed491441364bc5b58d359d86f14f65a Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Wed, 30 Oct 2024 12:57:51 +0100 Subject: [PATCH 05/16] fixup! tests: fix legacy test --- features/step_definitions/auth-steps.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/features/step_definitions/auth-steps.js b/features/step_definitions/auth-steps.js index 541c8f288a..9a3aa93d30 100644 --- a/features/step_definitions/auth-steps.js +++ b/features/step_definitions/auth-steps.js @@ -24,14 +24,14 @@ Given( const previousToken = this.sdk.jwt; const token = _.get(this.props, "result._source.token") || this.props.token; - should(token).not.be.undefined(); - - this.sdk.jwt = token; if (not) { should(await this.sdk.auth.checkToken()).throwError({ id: "services.storage.not_found", }); } else { + should(token).not.be.undefined(); + + this.sdk.jwt = token; const { valid } = await this.sdk.auth.checkToken(); this.sdk.jwt = previousToken; should(valid).be.true("Provided token is invalid"); From a351f968383e9a9d7cd28970332f514b7f08272a Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Thu, 31 Oct 2024 07:45:29 +0100 Subject: [PATCH 06/16] fix: remove un wanted breaking change --- features/step_definitions/auth-steps.js | 19 +++++++------- lib/core/security/tokenRepository.ts | 18 +++++++++++++- lib/model/storage/apiKey.js | 33 +++++++++---------------- test/model/storage/apiKey.test.js | 13 +++------- 4 files changed, 41 insertions(+), 42 deletions(-) diff --git a/features/step_definitions/auth-steps.js b/features/step_definitions/auth-steps.js index 9a3aa93d30..2acdd73603 100644 --- a/features/step_definitions/auth-steps.js +++ b/features/step_definitions/auth-steps.js @@ -24,23 +24,24 @@ Given( const previousToken = this.sdk.jwt; const token = _.get(this.props, "result._source.token") || this.props.token; + should(token).not.be.undefined(); + + this.sdk.jwt = token; + + const { valid } = await this.sdk.auth.checkToken(); + + this.sdk.jwt = previousToken; + if (not) { - should(await this.sdk.auth.checkToken()).throwError({ - id: "services.storage.not_found", - }); + should(valid).be.false("Provided token is valid"); } else { - should(token).not.be.undefined(); - - this.sdk.jwt = token; - const { valid } = await this.sdk.auth.checkToken(); - this.sdk.jwt = previousToken; should(valid).be.true("Provided token is invalid"); } }, ); Given("I save the created API key", function () { - this.props.token = this.props.result.token; + this.props.token = this.props.result._source.token; }); Given( diff --git a/lib/core/security/tokenRepository.ts b/lib/core/security/tokenRepository.ts index de916e9b0a..1ccea725cf 100644 --- a/lib/core/security/tokenRepository.ts +++ b/lib/core/security/tokenRepository.ts @@ -363,8 +363,24 @@ export class TokenRepository extends ObjectRepository { if (isApiKey) { const fingerprint = sha256(token); + + const userApiKeys = await ApiKey.search({ query : { + term: { + userId: decoded._id, + }, + } }); + + if (userApiKeys.length === 0) { + throw securityError.get("invalid"); + } + + const targetApiKey = userApiKeys.find((apiKey) => apiKey.fingerprint === fingerprint); + + if(!targetApiKey) { + throw securityError.get("invalid"); + } - const apiKey = await ApiKey.load(decoded._id, fingerprint); + const apiKey = await ApiKey.load(decoded._id, targetApiKey._id); const userToken = new Token({ _id: `${decoded._id}#${token}`, diff --git a/lib/model/storage/apiKey.js b/lib/model/storage/apiKey.js index e7a21bc2a7..e309ca01aa 100644 --- a/lib/model/storage/apiKey.js +++ b/lib/model/storage/apiKey.js @@ -112,7 +112,7 @@ class ApiKey extends BaseModel { }, apiKeyId || fingerprint, ); - + console.log(apiKey) await apiKey.save({ refresh, userId: creatorId }); apiKey.token = token.jwt; @@ -128,29 +128,18 @@ class ApiKey extends BaseModel { * * @returns {ApiKey} */ - static async load(userId, fingerprint) { - const query = { - term: { - userId, - }, - }; - - const apiKeys = await super.search({ query }); - if (apiKeys.length > 0) { - const apiKey = apiKeys.find((a) => a.fingerprint === fingerprint); - if (userId !== apiKey.userId) { - throw kerror.get("services", "storage", "not_found", fingerprint, { - message: `ApiKey "${fingerprint}" not found for user "${userId}".`, - }); - } - - return apiKey; + static async load(userId, id) { + const apiKey = await super.load(id); + + if (userId !== apiKey.userId) { + throw kerror.get("services", "storage", "not_found", id, { + message: `ApiKey "${id}" not found for user "${userId}".`, + }); } - throw kerror.get("services", "storage", "not_found", fingerprint, { - message: `ApiKey "${fingerprint}" not found for user "${userId}".`, - }); - } + return apiKey; + } + /** * Deletes API keys for an user * diff --git a/test/model/storage/apiKey.test.js b/test/model/storage/apiKey.test.js index 8e392cb047..4eec5a0807 100644 --- a/test/model/storage/apiKey.test.js +++ b/test/model/storage/apiKey.test.js @@ -116,23 +116,16 @@ describe("ApiKey", () => { describe("ApiKey.load", () => { it("should throw if the key does not belong to the provided user", async () => { - const searchStub = sinon - .stub(BaseModel, "search") + const loadStub = sinon + .stub(BaseModel, "load") .resolves({ userId: "mylehuong" }); const promise = ApiKey.load("aschen", "api-key-id"); - await should(promise).be.rejectedWith({ id: "services.storage.not_found", }); - should(searchStub).be.calledWith({ - query: { - term: { - userId: "aschen", - }, - }, - }); + should(loadStub).be.calledWith("api-key-id"); }); }); From 4423b70adf704729360fe34578b9b34c2b577823 Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Thu, 31 Oct 2024 07:50:13 +0100 Subject: [PATCH 07/16] docs(CONTRIBUTING): update command for functional tests and secure wait kuzzle on port 7512 --- .ci/scripts/run-test-cluster.sh | 2 +- CONTRIBUTING.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.ci/scripts/run-test-cluster.sh b/.ci/scripts/run-test-cluster.sh index cdd7a6059c..ee9ee1cae2 100755 --- a/.ci/scripts/run-test-cluster.sh +++ b/.ci/scripts/run-test-cluster.sh @@ -37,10 +37,10 @@ trap 'docker compose -f $YML_FILE logs' err docker compose -f $YML_FILE up -d -# don't wait on 7512: nginx will accept connections far before Kuzzle does KUZZLE_PORT=17510 ./bin/wait-kuzzle KUZZLE_PORT=17511 ./bin/wait-kuzzle KUZZLE_PORT=17512 ./bin/wait-kuzzle +KUZZLE_PORT=7512 ./bin/wait-kuzzle trap - err diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1e9848cf81..f028d435ba 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -123,5 +123,5 @@ npm run test:unit ### Functional tests ```bash -KUZZLE_FUNCTIONAL_TESTS="test:functional:websocket" NODE_VERSION="20" ./.ci/scripts/run-test-cluster.sh +KUZZLE_FUNCTIONAL_TESTS="test:functional:websocket" NODE_VERSION="20" ES_VERSION=8 ./.ci/scripts/run-test-cluster.sh ``` From 899d6e2962e82bc842c6145430a8996f73ba4320 Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Thu, 31 Oct 2024 07:59:15 +0100 Subject: [PATCH 08/16] chore: lint --- lib/core/security/tokenRepository.ts | 20 ++++++++++++-------- lib/model/storage/apiKey.js | 3 +-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/lib/core/security/tokenRepository.ts b/lib/core/security/tokenRepository.ts index 1ccea725cf..a52e32e0fc 100644 --- a/lib/core/security/tokenRepository.ts +++ b/lib/core/security/tokenRepository.ts @@ -363,20 +363,24 @@ export class TokenRepository extends ObjectRepository { if (isApiKey) { const fingerprint = sha256(token); - - const userApiKeys = await ApiKey.search({ query : { - term: { - userId: decoded._id, + + const userApiKeys = await ApiKey.search({ + query: { + term: { + userId: decoded._id, + }, }, - } }); + }); if (userApiKeys.length === 0) { throw securityError.get("invalid"); } - const targetApiKey = userApiKeys.find((apiKey) => apiKey.fingerprint === fingerprint); - - if(!targetApiKey) { + const targetApiKey = userApiKeys.find( + (apiKey) => apiKey.fingerprint === fingerprint, + ); + + if (!targetApiKey) { throw securityError.get("invalid"); } diff --git a/lib/model/storage/apiKey.js b/lib/model/storage/apiKey.js index e309ca01aa..707102e56e 100644 --- a/lib/model/storage/apiKey.js +++ b/lib/model/storage/apiKey.js @@ -112,7 +112,6 @@ class ApiKey extends BaseModel { }, apiKeyId || fingerprint, ); - console.log(apiKey) await apiKey.save({ refresh, userId: creatorId }); apiKey.token = token.jwt; @@ -139,7 +138,7 @@ class ApiKey extends BaseModel { return apiKey; } - + /** * Deletes API keys for an user * From 71de3c6058a114324c1b1f78221523aa71501640 Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Thu, 31 Oct 2024 08:37:20 +0100 Subject: [PATCH 09/16] refactor(verifyToken): reduce function complexity --- lib/core/security/tokenRepository.ts | 56 ++++++++++++++-------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/lib/core/security/tokenRepository.ts b/lib/core/security/tokenRepository.ts index a52e32e0fc..1635458143 100644 --- a/lib/core/security/tokenRepository.ts +++ b/lib/core/security/tokenRepository.ts @@ -362,7 +362,33 @@ export class TokenRepository extends ObjectRepository { } if (isApiKey) { - const fingerprint = sha256(token); + return this._verifyApiKey(decoded, token) + } + + let userToken; + + try { + userToken = await this.loadForUser(decoded._id, token); + } catch (err) { + if (err instanceof UnauthorizedError) { + throw err; + } + throw securityError.getFrom(err, "verification_error", err.message); + } + + if (userToken === null) { + throw securityError.get("invalid"); + } + + if (userToken.singleUse) { + await this.expire(userToken); + } + + return userToken; + } + + async _verifyApiKey(decoded, token: string) { + const fingerprint = sha256(token); const userApiKeys = await ApiKey.search({ query: { @@ -372,11 +398,7 @@ export class TokenRepository extends ObjectRepository { }, }); - if (userApiKeys.length === 0) { - throw securityError.get("invalid"); - } - - const targetApiKey = userApiKeys.find( + const targetApiKey = userApiKeys?.find( (apiKey) => apiKey.fingerprint === fingerprint, ); @@ -395,28 +417,6 @@ export class TokenRepository extends ObjectRepository { }); return userToken; - } - - let userToken; - - try { - userToken = await this.loadForUser(decoded._id, token); - } catch (err) { - if (err instanceof UnauthorizedError) { - throw err; - } - throw securityError.getFrom(err, "verification_error", err.message); - } - - if (userToken === null) { - throw securityError.get("invalid"); - } - - if (userToken.singleUse) { - await this.expire(userToken); - } - - return userToken; } removeTokenPrefix(token: string) { From 166250d6c933bbc6ef3908043f9d476728e82213 Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Thu, 31 Oct 2024 09:10:35 +0100 Subject: [PATCH 10/16] fixup! refactor(verifyToken): reduce function complexity --- lib/core/security/tokenRepository.ts | 44 ++++++++++++++-------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/lib/core/security/tokenRepository.ts b/lib/core/security/tokenRepository.ts index 1635458143..b2da60fe4c 100644 --- a/lib/core/security/tokenRepository.ts +++ b/lib/core/security/tokenRepository.ts @@ -362,7 +362,7 @@ export class TokenRepository extends ObjectRepository { } if (isApiKey) { - return this._verifyApiKey(decoded, token) + return this._verifyApiKey(decoded, token); } let userToken; @@ -390,33 +390,33 @@ export class TokenRepository extends ObjectRepository { async _verifyApiKey(decoded, token: string) { const fingerprint = sha256(token); - const userApiKeys = await ApiKey.search({ - query: { - term: { - userId: decoded._id, - }, + const userApiKeys = await ApiKey.search({ + query: { + term: { + userId: decoded._id, }, - }); + }, + }); - const targetApiKey = userApiKeys?.find( - (apiKey) => apiKey.fingerprint === fingerprint, - ); + const targetApiKey = userApiKeys?.find( + (apiKey) => apiKey.fingerprint === fingerprint, + ); - if (!targetApiKey) { - throw securityError.get("invalid"); - } + if (!targetApiKey) { + throw securityError.get("invalid"); + } - const apiKey = await ApiKey.load(decoded._id, targetApiKey._id); + const apiKey = await ApiKey.load(decoded._id, targetApiKey._id); - const userToken = new Token({ - _id: `${decoded._id}#${token}`, - expiresAt: apiKey.expiresAt, - jwt: token, - ttl: apiKey.ttl, - userId: decoded._id, - }); + const userToken = new Token({ + _id: `${decoded._id}#${token}`, + expiresAt: apiKey.expiresAt, + jwt: token, + ttl: apiKey.ttl, + userId: decoded._id, + }); - return userToken; + return userToken; } removeTokenPrefix(token: string) { From 798f25c2393cbf4408158201d29c7cd493c99f13 Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Thu, 31 Oct 2024 09:22:32 +0100 Subject: [PATCH 11/16] chore: auto review fixes --- doc/2/guides/getting-started/deploy-your-application/index.md | 2 +- lib/core/security/tokenRepository.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/2/guides/getting-started/deploy-your-application/index.md b/doc/2/guides/getting-started/deploy-your-application/index.md index c9da850166..88d738d0a2 100644 --- a/doc/2/guides/getting-started/deploy-your-application/index.md +++ b/doc/2/guides/getting-started/deploy-your-application/index.md @@ -51,7 +51,7 @@ A production deployment must include a reverse proxy to securize the connection ::: warning #### Production Deployment: Auth Token Secret -For every production deployment of Kuzzle, it is essential to set the kuzzle_security__authToken__secret environment variable. This ensures that the JWT secrets used for authenticating requests are generated externally and not stored in Elasticsearch. By managing the secret through an environment variable, you enhance security, prevent potential data exposure, and ensure tokens remain valid only as long as the secret remains unchanged. +For every production deployment of Kuzzle, it is essential to set the `kuzzle_security__authToken__secret` environment variable. This ensures that the JWT secrets used for authenticating requests are generated externally and not stored in Elasticsearch. By managing the secret through an environment variable, you enhance security, prevent potential data exposure, and ensure tokens remain valid only as long as the secret remains unchanged. Important: If the `kuzzle_security__authToken__secret` value is changed when Kuzzle restarts, all existing tokens will be invalidated. This ensures that only tokens signed with the current secret remain valid, adding an extra layer of security. diff --git a/lib/core/security/tokenRepository.ts b/lib/core/security/tokenRepository.ts index b2da60fe4c..05f2868c63 100644 --- a/lib/core/security/tokenRepository.ts +++ b/lib/core/security/tokenRepository.ts @@ -135,7 +135,7 @@ export class TokenRepository extends ObjectRepository { this.verifyToken(hash), ); - // ? those checks are necessary to detect JWT seed changes and delete existing token if necessary + // ? those checks are necessary to detect JWT seed changes and delete existing tokens if necessary const existingTokens = await global.kuzzle.ask( "core:cache:internal:searchKeys", "repos/kuzzle/token/*", From a4cb166b2c8216a118de13dedf1efe112cf60ba8 Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Thu, 31 Oct 2024 10:35:51 +0100 Subject: [PATCH 12/16] chore: fmauNeko review fix --- lib/kuzzle/internalIndexHandler.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/kuzzle/internalIndexHandler.js b/lib/kuzzle/internalIndexHandler.js index 72358e8378..3767eb672f 100644 --- a/lib/kuzzle/internalIndexHandler.js +++ b/lib/kuzzle/internalIndexHandler.js @@ -205,8 +205,7 @@ class InternalIndexHandler extends Store { async _initSecret() { const { authToken, jwt } = global.kuzzle.config.security; - const configSeed = - authToken && authToken.secret ? authToken.secret : jwt && jwt.secret; + const configSeed = authToken?.secret ?? jwt?.secret; let storedSeed = await this.exists("config", this._JWT_SECRET_ID); From a96e7e1434c2e765bf40cc6535d99b9381c28eb7 Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Thu, 31 Oct 2024 10:55:52 +0100 Subject: [PATCH 13/16] fix: target right ecma version in eslint --- .eslintrc.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.eslintrc.json b/.eslintrc.json index fb9dbb8b5c..caf7da7f0d 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -1,6 +1,9 @@ { "plugins": ["kuzzle"], "extends": ["plugin:kuzzle/default", "plugin:kuzzle/node"], + "parserOptions": { + "ecmaVersion": 2020 + }, "rules": { "sort-keys": "warn", "kuzzle/array-foreach": "warn" From 9dbe1b15446fd712c3f3090d54c4ccb41e8708e5 Mon Sep 17 00:00:00 2001 From: Juiced66 Date: Wed, 6 Nov 2024 07:18:01 +0100 Subject: [PATCH 14/16] chore: add log to warn user about generated seed usage --- lib/kuzzle/internalIndexHandler.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/kuzzle/internalIndexHandler.js b/lib/kuzzle/internalIndexHandler.js index 3767eb672f..c4ab0c50e6 100644 --- a/lib/kuzzle/internalIndexHandler.js +++ b/lib/kuzzle/internalIndexHandler.js @@ -209,12 +209,18 @@ class InternalIndexHandler extends Store { let storedSeed = await this.exists("config", this._JWT_SECRET_ID); - if (!configSeed && !storedSeed) { - storedSeed = crypto.randomBytes(512).toString("hex"); - await this.create( - "config", - { seed: storedSeed }, - { id: this._JWT_SECRET_ID }, + if (!configSeed) { + if (!storedSeed) { + storedSeed = crypto.randomBytes(512).toString("hex"); + await this.create( + "config", + { seed: storedSeed }, + { id: this._JWT_SECRET_ID }, + ); + } + + global.kuzzle.log.warn( + "[!] Kuzzle is using generated seed for authentication. This is suitable for development but should NEVER be use in Production. See https://docs.kuzzle.io/core/2/guides/getting-started/deploy-your-application/", ); } global.kuzzle.secret = configSeed From 073ef2ad0d4afdf0719a51d78f34b2092be22da7 Mon Sep 17 00:00:00 2001 From: Juiced66 <80784430+Juiced66@users.noreply.github.com> Date: Thu, 7 Nov 2024 12:27:54 +0100 Subject: [PATCH 15/16] Update doc/2/guides/getting-started/deploy-your-application/index.md Co-authored-by: Ricky --- .../deploy-your-application/index.md | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/doc/2/guides/getting-started/deploy-your-application/index.md b/doc/2/guides/getting-started/deploy-your-application/index.md index 88d738d0a2..b1b5c275ca 100644 --- a/doc/2/guides/getting-started/deploy-your-application/index.md +++ b/doc/2/guides/getting-started/deploy-your-application/index.md @@ -49,17 +49,25 @@ A production deployment must include a reverse proxy to securize the connection ::: ::: warning -#### Production Deployment: Auth Token Secret +# Authentication Security in Production -For every production deployment of Kuzzle, it is essential to set the `kuzzle_security__authToken__secret` environment variable. This ensures that the JWT secrets used for authenticating requests are generated externally and not stored in Elasticsearch. By managing the secret through an environment variable, you enhance security, prevent potential data exposure, and ensure tokens remain valid only as long as the secret remains unchanged. +## ⚠️ Important Security Requirement -Important: If the `kuzzle_security__authToken__secret` value is changed when Kuzzle restarts, all existing tokens will be invalidated. This ensures that only tokens signed with the current secret remain valid, adding an extra layer of security. +You must set the `kuzzle_security__authToken__secret` environment variable before deploying Kuzzle to production. This secret is used to sign and verify JSON Web Tokens (JWTs) for user authentication. -For default configuration values, you can refer to [the sample Kuzzle configuration file](https://github.com/kuzzleio/kuzzle/blob/master/.kuzzlerc.sample.jsonc). +## Why This Matters +- Prevents tokens from being stored in Elasticsearch +- Improves overall security +- Gives you direct control over token management -Note: If the secret is not set, Kuzzle will fallback to a less secure method of generating and storing the secret, which is not recommended for production environments. -::: +## Security Notes +1. **Fallback Warning**: If you don't set this variable, Kuzzle will use a less secure fallback method (not recommended for production) +2. **Token Invalidation**: Changing the secret value will immediately invalidate all existing authentication tokens +3. **User Impact**: Users will need to log in again if the secret changes +## Additional Resources +For other configuration options, see the [sample configuration file](https://github.com/kuzzleio/kuzzle/blob/master/.kuzzlerc.sample.jsonc). +::: ## Prepare our Docker Compose deployment We are going to write a `docker-compose.yml` file that describes our services. From d7214be3f7104ce171347b6b4e4d011da46add91 Mon Sep 17 00:00:00 2001 From: Juiced66 <80784430+Juiced66@users.noreply.github.com> Date: Thu, 7 Nov 2024 14:30:03 +0100 Subject: [PATCH 16/16] Update lib/kuzzle/internalIndexHandler.js Co-authored-by: Florian Maunier --- lib/kuzzle/internalIndexHandler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/kuzzle/internalIndexHandler.js b/lib/kuzzle/internalIndexHandler.js index c4ab0c50e6..1b1e52d3b0 100644 --- a/lib/kuzzle/internalIndexHandler.js +++ b/lib/kuzzle/internalIndexHandler.js @@ -220,7 +220,7 @@ class InternalIndexHandler extends Store { } global.kuzzle.log.warn( - "[!] Kuzzle is using generated seed for authentication. This is suitable for development but should NEVER be use in Production. See https://docs.kuzzle.io/core/2/guides/getting-started/deploy-your-application/", + "[!] Kuzzle is using a generated seed for authentication. This is suitable for development but should NEVER be used in production. See https://docs.kuzzle.io/core/2/guides/getting-started/deploy-your-application/", ); } global.kuzzle.secret = configSeed