From 8ec2d98645a1c9f5efd0980b2a37c7226f203f7d Mon Sep 17 00:00:00 2001 From: AJ ONeal Date: Tue, 15 Jun 2021 17:03:18 -0600 Subject: [PATCH] security: obey opts.iss, issue warning by default --- README.md | 13 ++++----- keyfetch-test.js | 33 ++++++++++++++++------- keyfetch.js | 24 ++++++++++++++--- package-lock.json | 20 ++++++++++---- package.json | 69 ++++++++++++++++++++++++----------------------- 5 files changed, 101 insertions(+), 58 deletions(-) diff --git a/README.md b/README.md index a7aec2b..aeb9c80 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# keyfetch +# [keyfetch](https://git.rootprojects.org/root/keyfetch.js) Lightweight support for fetching JWKs. @@ -178,15 +178,16 @@ If your authorization `claims` can be expressed as exact string matches, you can ```js keyfetch.jwt.verify(jwt, { - strategy: 'oidc' -, issuers: [ 'https://example.com/' ] -, claims: { role: 'admin', sub: 'abc', group: 'xyz' } + strategy: 'oidc', + issuers: [ 'https://example.com/' ], + //iss: 'https://example.com/', + claims: { role: 'admin', sub: 'abc', group: 'xyz' } }).then(function (verified) { - ``` - `strategy` may be `oidc` (default) , `auth0`, or a direct JWKs url. -- `issuers` must be a list of https urls (though http is allowed for things like Docker swarm) +- `issuers` must be a list of https urls (though http is allowed for things like Docker swarm), or '\*' +- `iss` is like `issuers`, but only one - `claims` is an object with arbitrary keys (i.e. everything except for the standard `iat`, `exp`, `jti`, etc) - `exp` may be set to `false` if you're validating on your own (i.e. allowing time drift leeway) - `jwks` can be used to specify a list of allowed public key rather than fetching them (i.e. for offline unit tests) diff --git a/keyfetch-test.js b/keyfetch-test.js index 7247a5d..2d8cf8a 100644 --- a/keyfetch-test.js +++ b/keyfetch-test.js @@ -25,29 +25,32 @@ keyfetch }); /*global Promise*/ -var keypairs = require("keypairs.js"); +var keypairs = require("keypairs"); keypairs.generate().then(function (pair) { + var iss = "https://example.com/"; return Promise.all([ keypairs .signJwt({ jwk: pair.private, - iss: "https://example.com/", + iss: iss, sub: "mikey", exp: "1h" }) .then(function (jwt) { return Promise.all([ - keyfetch.jwt.verify(jwt, { jwk: pair.public }).then(function (verified) { + keyfetch.jwt.verify(jwt, { jwk: pair.public, iss: "*" }).then(function (verified) { if (!(verified.claims && verified.claims.exp)) { throw new Error("malformed decoded token"); } }), - keyfetch.jwt.verify(keyfetch.jwt.decode(jwt), { jwk: pair.public }).then(function (verified) { - if (!(verified.claims && verified.claims.exp)) { - throw new Error("malformed decoded token"); - } - }), - keyfetch.jwt.verify(jwt, { jwks: [pair.public] }), + keyfetch.jwt + .verify(keyfetch.jwt.decode(jwt), { jwk: pair.public, iss: iss }) + .then(function (verified) { + if (!(verified.claims && verified.claims.exp)) { + throw new Error("malformed decoded token"); + } + }), + keyfetch.jwt.verify(jwt, { jwks: [pair.public], issuers: [iss] }), keyfetch.jwt.verify(jwt, { jwk: pair.public, issuers: ["https://example.com/"] @@ -117,8 +120,18 @@ keypairs.generate().then(function (pair) { exp: "1h" }) .then(function (jwt) { + var warned = false; + console.warn = function () { + warned = true; + }; return Promise.all([ - keyfetch.jwt.verify(jwt, { jwk: pair.public }), + // test that the old behavior of defaulting to '*' still works + keyfetch.jwt.verify(jwt, { jwk: pair.public }).then(function () { + if (!warned) { + throw e("should have issued security warning about allow all by default"); + } + }), + keyfetch.jwt.verify(jwt, { jwk: pair.public, issuers: ["*"] }), keyfetch.jwt.verify(jwt).then(e("should have an issuer")).catch(throwIfNotExpected), keyfetch.jwt .verify(jwt, { diff --git a/keyfetch.js b/keyfetch.js index 7b3d764..704f4d5 100644 --- a/keyfetch.js +++ b/keyfetch.js @@ -3,7 +3,7 @@ var keyfetch = module.exports; var promisify = require("util").promisify; -var requestAsync = promisify(require("@coolaj86/urequest")); +var requestAsync = promisify(require("@root/request")); var Rasha = require("rasha"); var Eckles = require("eckles"); var mincache = 1 * 60 * 60; @@ -197,6 +197,10 @@ keyfetch._setCache = function (iss, cacheable) { }; function normalizeIss(iss) { + if (!iss) { + throw new Error("'iss' is not defined"); + } + // We definitely don't want false negatives stemming // from https://example.com vs https://example.com/ // We also don't want to allow insecure issuers @@ -232,7 +236,21 @@ keyfetch.jwt.verify = function (jwt, opts) { var exp; var nbf; var active; - var issuers = opts.issuers || ["*"]; + var issuers = opts.issuers || []; + var err; + if (opts.iss) { + issuers.push(opts.iss); + } + if (opts.claims && opts.claims.iss) { + issuers.push(opts.claims.iss); + } + if (!issuers.length) { + err = new Error( + "\n[keyfetch.js] Security Warning:\nNeither of opts.issuers nor opts.iss were provided. The default behavior of setting opts.issuers = ['*'] by default - which allows any or no issuers - is deprecated and will throw an error instead in the next major version.\n" + ); + console.warn(err); + issuers.push("*"); + } var claims = opts.claims || {}; if (!jwt || "string" === typeof jwt) { try { @@ -249,7 +267,7 @@ keyfetch.jwt.verify = function (jwt, opts) { if (!issuers.some(isTrustedIssuer(decoded.claims.iss))) { throw new Error("token was issued by an untrusted issuer: '" + decoded.claims.iss + "'"); } - // TODO verify claims also? + // Note claims.iss validates more strictly than opts.issuers (requires exact match) if ( !Object.keys(claims).every(function (key) { if (claims[key] === decoded.claims[key]) { diff --git a/package-lock.json b/package-lock.json index 1a7bd50..52a7ab0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,19 +1,29 @@ { "name": "keyfetch", - "version": "1.1.8", + "version": "1.2.1", "lockfileVersion": 1, "requires": true, "dependencies": { - "@coolaj86/urequest": { - "version": "1.3.7", - "resolved": "https://registry.npmjs.org/@coolaj86/urequest/-/urequest-1.3.7.tgz", - "integrity": "sha512-PPrVYra9aWvZjSCKl/x1pJ9ZpXda1652oJrPBYy5rQumJJMkmTBN3ux+sK2xAUwVvv2wnewDlaQaHLxLwSHnIA==" + "@root/request": { + "version": "1.7.0", + "resolved": "https://registry.npmjs.org/@root/request/-/request-1.7.0.tgz", + "integrity": "sha512-lre7XVeEwszgyrayWWb/kRn5fuJfa+n0Nh+rflM9E+EpC28yIYA+FPm/OL1uhzp3TxhQM0HFN4FE2RDIPGlnmg==" }, "eckles": { "version": "1.4.1", "resolved": "https://registry.npmjs.org/eckles/-/eckles-1.4.1.tgz", "integrity": "sha512-auWyk/k8oSkVHaD4RxkPadKsLUcIwKgr/h8F7UZEueFDBO7BsE4y+H6IMUDbfqKIFPg/9MxV6KcBdJCmVVcxSA==" }, + "keypairs": { + "version": "1.2.14", + "resolved": "https://registry.npmjs.org/keypairs/-/keypairs-1.2.14.tgz", + "integrity": "sha512-ZoZfZMygyB0QcjSlz7Rh6wT2CJasYEHBPETtmHZEfxuJd7bnsOG5AdtPZqHZBT+hoHvuWCp/4y8VmvTvH0Y9uA==", + "dev": true, + "requires": { + "eckles": "^1.4.1", + "rasha": "^1.2.4" + } + }, "rasha": { "version": "1.2.4", "resolved": "https://registry.npmjs.org/rasha/-/rasha-1.2.4.tgz", diff --git a/package.json b/package.json index 4172505..953a44b 100644 --- a/package.json +++ b/package.json @@ -1,36 +1,37 @@ { - "name": "keyfetch", - "version": "1.2.1", - "description": "Lightweight support for fetching JWKs.", - "homepage": "https://git.coolaj86.com/coolaj86/keyfetch.js", - "main": "keyfetch.js", - "files": [], - "dependencies": { - "@coolaj86/urequest": "^1.3.7", - "eckles": "^1.4.1", - "rasha": "^1.2.4" - }, - "devDependencies": {}, - "scripts": { - "test": "node keyfetch-test.js" - }, - "repository": { - "type": "git", - "url": "https://git.coolaj86.com/coolaj86/keyfetch.js.git" - }, - "keywords": [ - "jwks", - "jwk", - "jwt", - "auth0", - "pem", - "RSA", - "EC", - "ECDSA", - "OIDC", - "well-known" - ], - "author": "AJ ONeal (https://coolaj86.com/)", - "license": "MPL-2.0" + "name": "keyfetch", + "version": "1.2.1", + "description": "Lightweight support for fetching JWKs.", + "homepage": "https://git.rootprojects.org/root/keyfetch.js", + "main": "keyfetch.js", + "files": [], + "dependencies": { + "@root/request": "^1.7.0", + "eckles": "^1.4.1", + "rasha": "^1.2.4" + }, + "devDependencies": { + "keypairs": "^1.2.14" + }, + "scripts": { + "test": "node keyfetch-test.js" + }, + "repository": { + "type": "git", + "url": "https://git.rootprojects.org/root/keyfetch.js.git" + }, + "keywords": [ + "jwks", + "jwk", + "jwt", + "auth0", + "pem", + "RSA", + "EC", + "ECDSA", + "OIDC", + "well-known" + ], + "author": "AJ ONeal (https://coolaj86.com/)", + "license": "MPL-2.0" } -