security: obey opts.iss, issue warning by default

2021-06-15 17:03:18 -06:00 · 2021-06-15 17:03:18 -06:00 · 8ec2d98645
parent 9141962456
commit 8ec2d98645
5 changed files with 101 additions and 58 deletions
--- 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)
--- 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, {
--- 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]) {
--- 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",
--- 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 <solderjs@gmail.com> (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 <coolaj86@gmail.com> (https://coolaj86.com/)",
+    "license": "MPL-2.0"
 }
-