From c9b82008deac98dee17e062da2447ed3052785c4 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 4 Aug 2017 10:37:47 +1000 Subject: [PATCH 1/7] Fixes to secrets init --- config.js | 22 ++++++++++------------ secrets.js | 6 +++++- services/jwt.js | 13 ++++++++++++- services/passport.js | 15 ++++++++++----- services/tokens.js | 4 +++- 5 files changed, 40 insertions(+), 20 deletions(-) diff --git a/config.js b/config.js index 02b40681d..16353caaa 100644 --- a/config.js +++ b/config.js @@ -130,20 +130,18 @@ if (process.env.NODE_ENV === 'test' && !CONFIG.ROOT_URL) { if (CONFIG.JWT_SECRETS) { CONFIG.JWT_SECRETS = JSON.parse(CONFIG.JWT_SECRETS); -} - -if (process.env.NODE_ENV === 'test' && !CONFIG.JWT_SECRET) { - CONFIG.JWT_SECRET = 'keyboard cat'; } else if (!CONFIG.JWT_SECRET) { - throw new Error( - 'TALK_JWT_SECRET must be provided in the environment to sign/verify tokens' - ); -} + if (process.env.NODE_ENV === 'test') { + if (!CONFIG.JWT_ALG.startsWith('HS')) { + throw new Error('Providing a asymmetric signing/verfying algorithm without a corresponding secret is not permitted'); + } -// If this is not employing a HMAC based signing method, then we need to turn -// the secret into a buffer. -if (!CONFIG.JWT_ALG.startsWith('HS')) { - CONFIG.JWT_SECRET = Buffer.from(CONFIG.JWT_SECRET); + CONFIG.JWT_SECRET = 'keyboard cat'; + } else { + throw new Error( + 'TALK_JWT_SECRET must be provided in the environment to sign/verify tokens' + ); + } } //------------------------------------------------------------------------------ diff --git a/secrets.js b/secrets.js index 14b5fc922..b7044fa09 100644 --- a/secrets.js +++ b/secrets.js @@ -22,6 +22,10 @@ if (JWT_SECRETS) { throw new Error('when multiple keys are specified, kid\'s must be specified'); } + if (typeof secret.kid !== 'string' || secret.kid.length === 0) { + throw new Error('kid must be a unique string'); + } + // HMAC secrets do not have public/private keys. if (JWT_ALG.startsWith('HS')) { return new jwt.SharedSecret(secret, JWT_ALG); @@ -34,7 +38,7 @@ if (JWT_SECRETS) { return new jwt.AsymmetricSecret(secret, JWT_ALG); })); - debug(`loaded ${JWT_SECRET.length} ${JWT_ALG.startsWith('HS') ? 'shared' : 'asymmetric'} secrets`); + debug(`loaded ${JWT_SECRETS.length} ${JWT_ALG.startsWith('HS') ? 'shared' : 'asymmetric'} secrets`); } else if (JWT_SECRET) { if (JWT_ALG.startsWith('HS')) { module.exports.jwt = new jwt.SharedSecret({ diff --git a/services/jwt.js b/services/jwt.js index c5f69c2fd..d930971cb 100644 --- a/services/jwt.js +++ b/services/jwt.js @@ -1,4 +1,5 @@ const jwt = require('jsonwebtoken'); +const uniq = require('lodash/uniq'); /** * MultiSecret will take many secrets and provide a unified interface for @@ -6,6 +7,12 @@ const jwt = require('jsonwebtoken'); */ class MultiSecret { constructor(secrets) { + this.kids = secrets.map(({kid}) => kid); + + if (uniq(this.kids).length !== secrets.length) { + throw new Error('Duplicate kid\'s cannot be used to construct a MultiSecret'); + } + this.secrets = secrets; } @@ -86,7 +93,11 @@ class Secret { /** * SharedSecret is the HMAC based secret that's used for signing/verifying. */ -function SharedSecret({kid = undefined, secret}, algorithm) { +function SharedSecret({kid = undefined, secret = null}, algorithm) { + if (secret === null || secret.length === 0) { + throw new Error('Secret cannot have a zero length'); + } + return new Secret({ kid, signingKey: secret, diff --git a/services/passport.js b/services/passport.js index 542fc9471..8b5b3879b 100644 --- a/services/passport.js +++ b/services/passport.js @@ -195,7 +195,9 @@ const CheckBlacklisted = async (jwt) => { } // It wasn't a PAT! Check to see if it is valid anyways. - return checkGeneralTokenBlacklist(jwt); + await checkGeneralTokenBlacklist(jwt); + + return null; }; const JwtStrategy = require('passport-jwt').Strategy; @@ -257,11 +259,14 @@ passport.use(new JwtStrategy({ try { // Check to see if the token has been revoked - await CheckBlacklisted(jwt); + let user = await CheckBlacklisted(jwt); - // Try to get the user from the database or crack it from the token and - // plugin integrations. - let user = await UsersService.findOrCreateByIDToken(jwt.sub, {token, jwt}); + if (user === null) { + + // Try to get the user from the database or crack it from the token and + // plugin integrations. + user = await UsersService.findOrCreateByIDToken(jwt.sub, {token, jwt}); + } // Attach the JWT to the request. req.jwt = jwt; diff --git a/services/tokens.js b/services/tokens.js index 8184d47b5..9c9af34a4 100644 --- a/services/tokens.js +++ b/services/tokens.js @@ -93,7 +93,7 @@ module.exports = class TokenService { // Find the user. let user = await UserModel.findOne({ id: userID - }).select('tokens'); + }); if (!user || !user.tokens) { throw new errors.ErrAuthentication('user does not exist'); } @@ -108,6 +108,8 @@ module.exports = class TokenService { if (!token.active) { throw new errors.ErrAuthentication('token is not active'); } + + return user; } /** From 5c8130becec58b287b582587d72188c4127298fb Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 4 Aug 2017 11:01:09 +1000 Subject: [PATCH 2/7] Added guard for private key length --- services/jwt.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/jwt.js b/services/jwt.js index d930971cb..e38b1120b 100644 --- a/services/jwt.js +++ b/services/jwt.js @@ -112,7 +112,7 @@ function SharedSecret({kid = undefined, secret = null}, algorithm) { */ function AsymmetricSecret({kid = undefined, private: privateKey, public: publicKey}, algorithm) { publicKey = Buffer.from(publicKey.replace(/\\n/g, '\n')); - privateKey = privateKey ? Buffer.from(privateKey.replace(/\\n/g, '\n')) : null; + privateKey = privateKey && privateKey.length > 0 ? Buffer.from(privateKey.replace(/\\n/g, '\n')) : null; return new Secret({ kid, From f92754a7001c4d77044fb568052b9bef28b6d18b Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 4 Aug 2017 13:54:42 +1000 Subject: [PATCH 3/7] updated docs related to secrets --- docs/_docs/02-02-secrets.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/_docs/02-02-secrets.md b/docs/_docs/02-02-secrets.md index a4e343570..c974c350f 100644 --- a/docs/_docs/02-02-secrets.md +++ b/docs/_docs/02-02-secrets.md @@ -60,6 +60,11 @@ must have their newlines replaced with `\\n`, this is to ensure that the newlines are preserved after JSON decoding. Not doing so will result in parsing errors. +To assist with this process, we have developed a tool that can generate new +certificates that match our required format: [coralcert](https://github.com/coralproject/coralcert). +This tool can generate RSA and ECDSA certificates, check it's [README](https://github.com/coralproject/coralcert) +for more details. + ## Authentication Types Talk also supports two methods of providing authenticationd details. From 8cdf289b13b70ae85ac24811d21f716c3f9e5aaf Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 4 Aug 2017 15:49:37 +1000 Subject: [PATCH 4/7] exposed more controls on jwt and added docs --- config.js | 21 +++++++++++++++++++++ docs/_docs/02-01-configuration.md | 21 +++++++++++++++++---- services/passport.js | 22 ++++++++++++++-------- services/tokens.js | 7 +++++-- 4 files changed, 57 insertions(+), 14 deletions(-) diff --git a/config.js b/config.js index 16353caaa..282b05ea9 100644 --- a/config.js +++ b/config.js @@ -35,11 +35,22 @@ const CONFIG = { // cleared when the user is logged out. JWT_CLEAR_COOKIE_LOGOUT: process.env.TALK_JWT_CLEAR_COOKIE_LOGOUT ? process.env.TALK_JWT_CLEAR_COOKIE_LOGOUT !== 'FALSE' : true, + // JWT_DISABLE_AUDIENCE when TRUE will disable the issuer claim (iss) from tokens. + JWT_DISABLE_AUDIENCE: process.env.TALK_JWT_DISABLE_AUDIENCE === 'TRUE', + // JWT_AUDIENCE is the value for the audience claim for the tokens that will be // verified when decoding. If `JWT_AUDIENCE` is not in the environment, then it // will default to `talk`. JWT_AUDIENCE: process.env.TALK_JWT_AUDIENCE || 'talk', + // JWT_DISABLE_ISSUER when TRUE will disable the issuer claim (iss) from tokens. + JWT_DISABLE_ISSUER: process.env.TALK_JWT_DISABLE_ISSUER === 'TRUE', + + // JWT_USER_ID_CLAIM is the claim which stores the user's id. This may be a deep + // object delimited using dot notation. Example `user.id` would store it like: + // {user: {id}} on the claims object. (Default `sub`) + JWT_USER_ID_CLAIM: process.env.TALK_JWT_USER_ID_CLAIM || 'sub', + // JWT_ISSUER is the value for the issuer for the tokens that will be verified // when decoding. If `JWT_ISSUER` is not in the environment, then it will try // `TALK_ROOT_URL`, otherwise, it will be undefined. @@ -144,6 +155,16 @@ if (CONFIG.JWT_SECRETS) { } } +// Disable the audience claim if requested. +if (CONFIG.JWT_DISABLE_AUDIENCE) { + CONFIG.JWT_AUDIENCE = undefined; +} + +// Disable the issuer claim if requested. +if (CONFIG.JWT_DISABLE_ISSUER) { + CONFIG.JWT_ISSUER = undefined; +} + //------------------------------------------------------------------------------ // External database url's //------------------------------------------------------------------------------ diff --git a/docs/_docs/02-01-configuration.md b/docs/_docs/02-01-configuration.md index d2e990375..475823b4c 100644 --- a/docs/_docs/02-01-configuration.md +++ b/docs/_docs/02-01-configuration.md @@ -77,15 +77,28 @@ The following are configuration shared with every type of secret used. tokens. (Default `process.env.TALK_ROOT_URL`) - `TALK_JWT_AUDIENCE` (_optional_) - the audience (`aud`) claim for login JWT tokens. (Default `talk`) + +**You must also specify secrets as either the `TALK_JWT_SECRET` or the `TALK_JWT_SECRETS` +variable. Refer to the [Secrets Documentation]({{ "/docs/running/secrets/" | absolute_url }}) +on the contents of those variables.** + +#### Advanced + +These are advanced settings for fine tuning the auth integration, and +is not needed in most situations. + - `TALK_JWT_COOKIE_NAME` (_optional_) - the name of the cookie to extract the JWT from (Default `authorization`) - `TALK_JWT_CLEAR_COOKIE_LOGOUT` (_optional_) - when `FALSE`, Talk will not clear the cookie with name `TALK_JWT_COOKIE_NAME` when logging out (Default `TRUE`) - -**You must also specify secrets as either the `TALK_JWT_SECRET` or the `TALK_JWT_SECRETS` -variable. Refer to the [Secrets Documentation]({{ "/docs/running/secrets/" | absolute_url }}) -on the contents of those variables.** +- `TALK_JWT_DISABLE_AUDIENCE` (_optional_) - when `TRUE`, Talk will not verify or sign JWT's + with an audience (`aud`) claim, even if the `TALK_JWT_AUDIENCE` config is set. (Default `FALSE`) +- `TALK_JWT_DISABLE_ISSUER` (_optional_) - when `TRUE`, Talk will not verify or sign JWT's + with an issuer (`iss`) claim, even if the `TALK_JWT_ISSUER` config is set. (Default `FALSE`) +- `TALK_JWT_USER_ID_CLAIM` (_optional_) - specify the claim using dot notation for where the + user id should be stored/read to/from. Example `user.id` would store it like: `{user: {id}}` + on the claims object. (Default `sub`) ### Email diff --git a/services/passport.js b/services/passport.js index 8b5b3879b..409bb2279 100644 --- a/services/passport.js +++ b/services/passport.js @@ -1,4 +1,5 @@ const passport = require('passport'); +const {set, get} = require('lodash'); const UsersService = require('./users'); const SettingsService = require('./settings'); const TokensService = require('./tokens'); @@ -23,21 +24,26 @@ const { RECAPTCHA_SECRET, RECAPTCHA_ENABLED, JWT_COOKIE_NAME, - JWT_CLEAR_COOKIE_LOGOUT + JWT_CLEAR_COOKIE_LOGOUT, + JWT_USER_ID_CLAIM, } = require('../config'); const { - jwt: JWT_SECRET + jwt } = require('../secrets'); // GenerateToken will sign a token to include all the authorization information // needed for the front end. const GenerateToken = (user) => { - return JWT_SECRET.sign({}, { + const claims = {}; + + // Set the user id. + set(claims, JWT_USER_ID_CLAIM, user.id); + + return jwt.sign(claims, { jwtid: uuid.v4(), expiresIn: JWT_EXPIRY, issuer: JWT_ISSUER, - subject: user.id, audience: JWT_AUDIENCE, algorithm: JWT_ALG }); @@ -191,7 +197,7 @@ const CheckBlacklisted = async (jwt) => { // Check to see if this is a PAT. if (jwt.pat) { - return TokensService.validate(jwt.sub, jwt.jti); + return TokensService.validate(get(jwt, JWT_USER_ID_CLAIM), jwt.jti); } // It wasn't a PAT! Check to see if it is valid anyways. @@ -216,7 +222,7 @@ let cookieExtractor = function(req) { // Override the JwtVerifier method on the JwtStrategy so we can pack the // original token into the payload. JwtStrategy.JwtVerifier = (token, secretOrKey, options, callback) => { - return JWT_SECRET.verify(token, options, (err, jwt) => { + return jwt.verify(token, options, (err, jwt) => { if (err) { return callback(err); } @@ -238,7 +244,7 @@ passport.use(new JwtStrategy({ // Use the secret passed in which is loaded from the environment. This can be // a certificate (loaded) or a HMAC key. - secretOrKey: JWT_SECRET, + secretOrKey: jwt, // Verify the issuer. issuer: JWT_ISSUER, @@ -265,7 +271,7 @@ passport.use(new JwtStrategy({ // Try to get the user from the database or crack it from the token and // plugin integrations. - user = await UsersService.findOrCreateByIDToken(jwt.sub, {token, jwt}); + user = await UsersService.findOrCreateByIDToken(get(jwt, JWT_USER_ID_CLAIM), {token, jwt}); } // Attach the JWT to the request. diff --git a/services/tokens.js b/services/tokens.js index 9c9af34a4..bedd5b272 100644 --- a/services/tokens.js +++ b/services/tokens.js @@ -1,10 +1,12 @@ const errors = require('../errors'); const UserModel = require('../models/user'); const uuid = require('uuid'); +const {set, get} = require('lodash'); const { JWT_ISSUER, - JWT_AUDIENCE + JWT_AUDIENCE, + JWT_USER_ID_CLAIM, } = require('../config'); const { @@ -30,10 +32,11 @@ module.exports = class TokenService { jti: uuid.v4(), iss: JWT_ISSUER, aud: JWT_AUDIENCE, - sub: userID, pat: true }; + set(payload, JWT_USER_ID_CLAIM, userID); + // Sign the payload. const jwt = JWT_SECRET.sign(payload, {}); From 02a75c590f0b455882076abec6787506f630edbd Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 4 Aug 2017 15:54:14 +1000 Subject: [PATCH 5/7] lint fix --- services/tokens.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/tokens.js b/services/tokens.js index bedd5b272..2df33c1ca 100644 --- a/services/tokens.js +++ b/services/tokens.js @@ -1,7 +1,7 @@ const errors = require('../errors'); const UserModel = require('../models/user'); const uuid = require('uuid'); -const {set, get} = require('lodash'); +const {set} = require('lodash'); const { JWT_ISSUER, From 190e93c8d78ca5e1707f8a83bac30d58725362ee Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 4 Aug 2017 15:57:44 +1000 Subject: [PATCH 6/7] fixed comment --- config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.js b/config.js index 282b05ea9..4472af26e 100644 --- a/config.js +++ b/config.js @@ -35,7 +35,7 @@ const CONFIG = { // cleared when the user is logged out. JWT_CLEAR_COOKIE_LOGOUT: process.env.TALK_JWT_CLEAR_COOKIE_LOGOUT ? process.env.TALK_JWT_CLEAR_COOKIE_LOGOUT !== 'FALSE' : true, - // JWT_DISABLE_AUDIENCE when TRUE will disable the issuer claim (iss) from tokens. + // JWT_DISABLE_AUDIENCE when TRUE will disable the audience claim (aud) from tokens. JWT_DISABLE_AUDIENCE: process.env.TALK_JWT_DISABLE_AUDIENCE === 'TRUE', // JWT_AUDIENCE is the value for the audience claim for the tokens that will be From a39b7237e692ea3bc0a17860b16c17e03ba9c53f Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 4 Aug 2017 16:10:08 +1000 Subject: [PATCH 7/7] updates to docs --- docs/_docs/02-01-configuration.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/_docs/02-01-configuration.md b/docs/_docs/02-01-configuration.md index 475823b4c..74bd8972e 100644 --- a/docs/_docs/02-01-configuration.md +++ b/docs/_docs/02-01-configuration.md @@ -100,6 +100,21 @@ is not needed in most situations. user id should be stored/read to/from. Example `user.id` would store it like: `{user: {id}}` on the claims object. (Default `sub`) +When integrating with an external authentication system, the following JWT claims +will be used: + +```js +{ + "jti": "", // *required* unique id used for blacklisting + "aud": TALK_JWT_AUDIENCE, // *optional* if TALK_JWT_DISABLE_AUDIENCE === 'TRUE', *required* otherwise + "iss": TALK_JWT_ISSUER, // *optional* if TALK_JWT_DISABLE_ISSUER === 'TRUE', *required* otherwise + + [TALK_JWT_USER_ID_CLAIM]: "", // *required* the id of the user + // Note, if TALK_JWT_USER_ID_CLAIM contains '.', it will be used to deliniate an object, for example + // `user.id` would store it like: `{user: {id}}` +} +``` + ### Email - `TALK_SMTP_EMAIL` (*required for email*) - the address to send emails from