feat: allow custom mechanisms for handling pairwise identifiers

BREAKING CHANGE: the configuration option `pairwiseSalt` is replaced with `pairwiseIdentifier` async helper function. This allows for different means of generating the pairwise identifier to be implemented, such as the ones mentioned in [Core 1.0](https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.8.1)
panva · Sep 26, 2018 · 57ce6d7 · 57ce6d7
1 parent bb4269f
commit 57ce6d7
Show file tree

Hide file tree

Showing 20 changed files with 253 additions and 123 deletions.
diff --git a/docs/configuration.md b/docs/configuration.md
@@ -77,7 +77,7 @@ is a good starting point to get an idea of what you should provide.
   - [interactionUrl](#interactionurl)
   - [introspectionEndpointAuthMethods](#introspectionendpointauthmethods)
   - [logoutSource](#logoutsource)
-  - [pairwiseSalt](#pairwisesalt)
+  - [pairwiseIdentifier](#pairwiseidentifier)
   - [postLogoutRedirectUri](#postlogoutredirecturi)
   - [prompts](#prompts)
   - [refreshTokenRotation](#refreshtokenrotation)
@@ -1094,7 +1094,7 @@ false
   <br>
 
 
-The User-Agent must allow access to the provider cookies from a third-party context when the OP frame is embedded. Oidc-provider checks if this is enabled using a [CDN hosted](https://rawgit.com/) [iframe][third-party-cookies-git]. It is recommended to host these helper pages on your own (on a different domain from the one you host oidc-provider on). Once hosted, set the `features.sessionManagement.thirdPartyCheckUrl` to an absolute URL for the start page. See [this][third-party-cookies-so] for more info. Note: This is still just a best-effort solution and is in no way bulletproof. Currently there's no better way to check if access to third party cookies has been blocked or the cookies are just missing. (ITP2.0 Storage Access API is also not an option) Configure `features.sessionManagement` as an object like so:
+The User-Agent must allow access to the provider cookies from a third-party context when the OP frame is embedded. Oidc-provider checks if this is enabled using a [CDN hosted](https://rawgit.com/) [iframe][third-party-cookies-git]. It is recommended to host these helper pages on your own (on a different domain from the one you host oidc-provider on). Once hosted, set the `features.sessionManagement.thirdPartyCheckUrl` to an absolute URL for the start page. See [this][third-party-cookies-so] for more info. Note: This is still just a best-effort solution and is in no way bulletproof. Currently there's no better way to check if access to third party cookies has been blocked or the cookies are just missing. (Safari's ITP 2.0 Storage Access API also cannot be used) Configure `features.sessionManagement` as an object like so:
 
 
 ```js
@@ -1670,17 +1670,29 @@ async logoutSource(ctx, form) {
 </details>
 
 
-### pairwiseSalt
+### pairwiseIdentifier
 
-Salt used by OP when resolving pairwise ID Token and Userinfo sub claim value  
+Function used by the OP when resolving pairwise ID Token and Userinfo sub claim values. See [Core 1.0](https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.8.1)  
 
-_**affects**_: ID Token and Userinfo sub claim values  
+_**affects**_: pairwise ID Token and Userinfo sub claim values  
+_**recommendation**_: Since this might be called several times in one request with the same arguments consider using memoization or otherwise caching the result based on account and client ids.  
+<details>
+  <summary><em><strong>default value</strong></em> (Click to expand)</summary>
+  <br>
 
-_**default value**_:
 ```js
-''
+async pairwiseIdentifier(accountId, client) {
+  return crypto.createHash('sha256')
+    .update(client.sectorIdentifier)
+    .update(accountId)
+    .update(os.hostname()) // put your own unique salt here, or implement other mechanism
+    .digest('hex');
+}
 ```
 
+</details>
+
+
 ### postLogoutRedirectUri
 
 URL to which the OP redirects the User-Agent when no post_logout_redirect_uri is provided by the RP  

diff --git a/docs/update-configuration.js b/docs/update-configuration.js
@@ -210,7 +210,8 @@ const props = [
               line.match(/^(\s+)\S+/);
               fixIndent = RegExp.$1.length - 2;
             }
-            if (line.includes('changeme')) return undefined;
+            if (line.includes('shouldChange')) return undefined;
+            if (line.includes('mustChange')) return undefined;
             if (line.startsWith(' ')) line = line.slice(fixIndent);
             line = line.replace(/ \/\/ eslint-disable.+/, '');
             line = line.replace(/ \/\/ TODO.+/, '');

diff --git a/example/support/configuration.js b/example/support/configuration.js
@@ -1,3 +1,5 @@
+const crypto = require('crypto');
+
 const pkg = require('../../package.json');
 
 module.exports.provider = {
@@ -48,7 +50,13 @@ module.exports.provider = {
     AccessToken: 'jwt',
   },
   subjectTypes: ['public', 'pairwise'],
-  pairwiseSalt: 'da1c442b365b563dfc121f285a11eedee5bbff7110d55c88',
+  pairwiseIdentifier(accountId, { sectorIdentifier }) {
+    return crypto.createHash('sha256')
+      .update(sectorIdentifier)
+      .update(accountId)
+      .update('da1c442b365b563dfc121f285a11eedee5bbff7110d55c88')
+      .digest('hex');
+  },
   interactionUrl: function interactionUrl(ctx, interaction) { // eslint-disable-line no-unused-vars
     return `/interaction/${ctx.oidc.uuid}`;
   },

diff --git a/lib/actions/authorization/interactions.js b/lib/actions/authorization/interactions.js
@@ -7,7 +7,7 @@ const { InvalidRequest } = require('../../helpers/errors');
 const instance = require('../../helpers/weak_cache');
 
 module.exports = (provider, resumeRouteName) => {
-  const interactionCheck = instance(provider).configuration('interactionCheck');
+  const { pairwiseIdentifier, interactionCheck } = instance(provider).configuration();
 
   const interactionChecks = [
     // no account id was found in the session info
@@ -50,9 +50,12 @@ module.exports = (provider, resumeRouteName) => {
     },
 
     // session subject value differs from the one requested
-    (ctx) => {
+    async (ctx) => {
       if (_.has(ctx.oidc.claims, 'id_token.sub.value')) {
-        const subject = provider.Claims.sub(ctx.oidc.session.accountId(), ctx.oidc.client);
+        let subject = ctx.oidc.session.accountId();
+        if (ctx.oidc.client.sectorIdentifier) {
+          subject = await pairwiseIdentifier(subject, ctx.oidc.client);
+        }
         if (ctx.oidc.claims.id_token.sub.value !== subject) {
           return {
             error: 'login_required',
@@ -121,8 +124,12 @@ module.exports = (provider, resumeRouteName) => {
       const hint = ctx.oidc.params.id_token_hint;
       if (hint !== undefined) {
         const { client } = ctx.oidc;
-        const actualSub = provider.Claims.sub(ctx.oidc.session.accountId(), client);
         const { IdToken } = provider;
+        let actualSub = ctx.oidc.session.accountId();
+
+        if (ctx.oidc.client.sectorIdentifier) {
+          actualSub = await pairwiseIdentifier(actualSub, ctx.oidc.client);
+        }
 
         const decoded = await IdToken.validate(hint, client).catch((err) => {
           throw new InvalidRequest(`could not validate id_token_hint (${err.message})`);

diff --git a/lib/actions/introspection.js b/lib/actions/introspection.js
@@ -18,7 +18,7 @@ const JWT = 'application/jwt';
 
 module.exports = function introspectionAction(provider) {
   const configuration = instance(provider).configuration();
-  const { features: { jwtIntrospection } } = configuration;
+  const { pairwiseIdentifier, features: { jwtIntrospection } } = configuration;
   const parseBody = bodyParser('application/x-www-form-urlencoded');
   const buildParams = getParams(PARAM_LIST);
   const { grantTypeHandlers } = instance(provider);
@@ -163,13 +163,14 @@ module.exports = function introspectionAction(provider) {
         return;
       }
 
+      ctx.body.sub = token.accountId;
       if (token.clientId !== ctx.oidc.client.clientId) {
-        ctx.body.sub = provider.Claims.sub(
-          token.accountId,
-          await Client.find(token.clientId),
-        );
-      } else {
-        ctx.body.sub = provider.Claims.sub(token.accountId, ctx.oidc.client);
+        const client = await Client.find(token.clientId);
+        if (client.sectorIdentifier) {
+          ctx.body.sub = await pairwiseIdentifier(ctx.body.sub, client);
+        }
+      } else if (ctx.oidc.client.sectorIdentifier) {
+        ctx.body.sub = await pairwiseIdentifier(ctx.body.sub, ctx.oidc.client);
       }
 
       Object.assign(ctx.body, {

diff --git a/lib/actions/userinfo.js b/lib/actions/userinfo.js
@@ -127,7 +127,7 @@ module.exports = function userinfoAction(provider) {
         mask.mask(claims);
         mask.rejected(rejected);
 
-        ctx.body = mask.result();
+        ctx.body = await mask.result();
       }
 
       debug('uuid=%s content-type=%s response=%o', ctx.oidc.uuid, ctx.type, ctx.body);

diff --git a/lib/helpers/attention.js b/lib/helpers/attention.js
@@ -1,4 +1,5 @@
 const SET_BOLD_YELLOW_TEXT = '\x1b[33;1m';
+const SET_BOLD_RED_TEXT = '\x1b[31;1m';
 const RESET_ALL_ATTRIBUTES = '\x1b[0m';
 
 function colorizeStdout(str) {
@@ -10,7 +11,7 @@ function colorizeStdout(str) {
 
 function colorizeStderr(str) {
   if (process.stderr.isTTY) {
-    return `${SET_BOLD_YELLOW_TEXT}${str}${RESET_ALL_ATTRIBUTES}`;
+    return `${SET_BOLD_RED_TEXT}${str}${RESET_ALL_ATTRIBUTES}`;
   }
   return str;
 }

diff --git a/lib/helpers/claims.js b/lib/helpers/claims.js
@@ -1,13 +1,12 @@
 const assert = require('assert');
-const crypto = require('crypto');
 
 const _ = require('lodash');
 
 const instance = require('./weak_cache');
 
 module.exports = function getClaims(provider) {
   const {
-    pairwiseSalt, claims: claimConfig, claimsSupported, dynamicScopes,
+    claims: claimConfig, claimsSupported, dynamicScopes, pairwiseIdentifier,
   } = instance(provider).configuration();
 
   return class Claims {
@@ -22,22 +21,6 @@ module.exports = function getClaims(provider) {
       this.filter = {};
     }
 
-    static sub(accountId, client) {
-      assert(client instanceof provider.Client, 'second argument must be a Client instance');
-
-      if (!accountId) return undefined;
-
-      if (client.sectorIdentifier) {
-        return crypto.createHash('sha256')
-          .update(client.sectorIdentifier)
-          .update(accountId)
-          .update(pairwiseSalt)
-          .digest('hex');
-      }
-
-      return accountId;
-    }
-
     scope(value = '') {
       assert(!Object.keys(this.filter).length, 'scope cannot be assigned after mask has been set');
       value.split(' ').forEach((scope) => {
@@ -63,7 +46,7 @@ module.exports = function getClaims(provider) {
       });
     }
 
-    result() {
+    async result() {
       const { available } = this;
       const include = Object.entries(this.filter)
         .map(([key, value]) => {
@@ -91,7 +74,7 @@ module.exports = function getClaims(provider) {
       }
 
       if (this.client.sectorIdentifier && claims.sub) {
-        claims.sub = this.constructor.sub(claims.sub, this.client);
+        claims.sub = await pairwiseIdentifier(claims.sub, this.client);
       }
 
       return claims;

diff --git a/lib/helpers/configuration.js b/lib/helpers/configuration.js
@@ -53,10 +53,6 @@ class Configuration {
       });
     }
 
-    if (this.subjectTypes.includes('pairwise') && !this.pairwiseSalt) {
-      throw new Error('pairwiseSalt must be configured when pairwise subjectType is to be supported');
-    }
-
     if (!this.features.sessionManagement) {
       if (this.features.backchannelLogout) {
         throw new Error('backchannelLogout is only available in conjuction with sessionManagement');

diff --git a/lib/helpers/defaults.js b/lib/helpers/defaults.js
@@ -1,5 +1,8 @@
 /* eslint-disable max-len */
 
+const crypto = require('crypto');
+const os = require('os');
+
 const LRU = require('lru-cache');
 
 const attention = require('./attention');
@@ -8,10 +11,16 @@ const epochTime = require('./epoch_time');
 const cache = new LRU(100);
 
 const warned = new Set();
-function changeme(name, msg) {
+function shouldChange(name, msg) {
+  if (!warned.has(name)) {
+    warned.add(name);
+    attention.info(`default helper ${name} called, you SHOULD change it in order to ${msg}.`);
+  }
+}
+function mustChange(name, msg) {
   if (!warned.has(name)) {
     warned.add(name);
-    attention.info(`default helper ${name} called, you should probably change it in order to ${msg}.`);
+    attention.warn(`default helper ${name} called, you MUST change it in order to ${msg}.`);
   }
 }
 
@@ -633,13 +642,23 @@ const DEFAULTS = {
 
 
   /*
-   * pairwiseSalt
+   * pairwiseIdentifier
    *
-   * description: Salt used by OP when resolving pairwise ID Token and Userinfo sub claim value
-   * affects: ID Token and Userinfo sub claim values
+   * description: Function used by the OP when resolving pairwise ID Token and Userinfo sub claim
+   *   values. See [Core 1.0](https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.8.1)
+   * affects: pairwise ID Token and Userinfo sub claim values
+   * recommendation: Since this might be called several times in one request with the same arguments
+   *   consider using memoization or otherwise caching the result based on account and client
+   *   ids.
    */
-  // TODO: pairwise helper instead, might do salting, might do random generation and mapping
-  pairwiseSalt: '',
+  async pairwiseIdentifier(accountId, client) {
+    mustChange('pairwiseIdentifier', 'provide an implementation for pairwise identifiers, the default one uses `os.hostname()` as salt and is therefore not fit for anything else than development');
+    return crypto.createHash('sha256')
+      .update(client.sectorIdentifier)
+      .update(accountId)
+      .update(os.hostname()) // put your own unique salt here, or implement other mechanism
+      .digest('hex');
+  },
 
 
   /*
@@ -737,7 +756,7 @@ const DEFAULTS = {
    * affects: session management
    */
   async postLogoutRedirectUri(ctx) { // eslint-disable-line no-unused-vars
-    changeme('postLogoutRedirectUri', 'specify where to redirect the user after logout without post_logout_redirect_uri specified or validated');
+    shouldChange('postLogoutRedirectUri', 'specify where to redirect the user after logout without post_logout_redirect_uri specified or validated');
     return ctx.origin;
   },
 
@@ -753,7 +772,7 @@ const DEFAULTS = {
     // @param ctx - koa request context
     // @param form - form source (id="op.logoutForm") to be embedded in the page and submitted by
     //   the End-User
-    changeme('logoutSource', 'customize the look of the logout page');
+    shouldChange('logoutSource', 'customize the look of the logout page');
     ctx.body = `<!DOCTYPE html>
 <head>
   <meta charset="utf-8">
@@ -806,7 +825,7 @@ const DEFAULTS = {
     //   rendered, i.e. does not include internal error messages
     // @param err - error object with an optional userCode property passed when the form is being
     //   re-rendered due to code missing/invalid/expired
-    changeme('userCodeInputSource', 'customize the look of the user code input page');
+    shouldChange('userCodeInputSource', 'customize the look of the user code input page');
     let msg;
     if (err && (err.userCode || err.name === 'NoCodeError')) {
       msg = '<p class="red">The code you entered is incorrect. Try again</p>';
@@ -849,7 +868,7 @@ const DEFAULTS = {
     // @param form - form source (id="op.deviceConfirmForm") to be embedded in the page and
     //   submitted by the End-User.
     // @param deviceInfo - device information from the device_authorization_endpoint call
-    changeme('userCodeConfirmSource', 'customize the look of the user code confirmation page');
+    shouldChange('userCodeConfirmSource', 'customize the look of the user code confirmation page');
     const {
       clientId, clientName, clientUri, logoUri, policyUri, tosUri, // eslint-disable-line no-unused-vars, max-len
     } = ctx.oidc.client;
@@ -891,7 +910,7 @@ const DEFAULTS = {
    */
   async deviceFlowSuccess(ctx) {
     // @param ctx - koa request context
-    changeme('deviceFlowSuccess', 'customize the look of the device code success page');
+    shouldChange('deviceFlowSuccess', 'customize the look of the device code success page');
     const {
       clientId, clientName, clientUri, initiateLoginUri, logoUri, policyUri, tosUri, // eslint-disable-line no-unused-vars, max-len
     } = ctx.oidc.client;
@@ -925,7 +944,7 @@ const DEFAULTS = {
    */
   // TODO: check escaping of client entered url values
   async frontchannelLogoutPendingSource(ctx, frames, postLogoutRedirectUri, timeout) {
-    changeme('frontchannelLogoutPendingSource', 'customize the front-channel logout pending page');
+    shouldChange('frontchannelLogoutPendingSource', 'customize the front-channel logout pending page');
     ctx.body = `<!DOCTYPE html>
 <head>
   <meta charset="utf-8">
@@ -966,7 +985,7 @@ const DEFAULTS = {
    *   private_key_jwt are used
    */
   async uniqueness(ctx, jti, expiresAt) {
-    changeme('uniqueness', 'to have the values unique-checked across processes');
+    mustChange('uniqueness', 'to have the values unique-checked across processes');
     if (cache.get(jti)) return false;
 
     cache.set(jti, true, (expiresAt - epochTime()) * 1000);
@@ -982,7 +1001,7 @@ const DEFAULTS = {
    * affects: presentation of errors encountered during End-User flows
    */
   async renderError(ctx, out, error) { // eslint-disable-line no-unused-vars
-    changeme('renderError', 'customize the look of the error page');
+    shouldChange('renderError', 'customize the look of the error page');
     ctx.type = 'html';
     ctx.body = `<!DOCTYPE html>
 <head>
@@ -1012,7 +1031,7 @@ const DEFAULTS = {
    * affects: authorization interactions
    */
   async interactionUrl(ctx, interaction) { // eslint-disable-line no-unused-vars
-    changeme('interactionUrl', 'to specify where the user interactions should take place');
+    shouldChange('interactionUrl', 'to specify where the user interactions should take place');
     return `/interaction/${ctx.oidc.uuid}`;
   },
 
@@ -1027,7 +1046,7 @@ const DEFAULTS = {
    * affects: authorization interactions
    */
   async interactionCheck(ctx) {
-    changeme('interactionCheck', 'to define the policy for requiring End-User interactions');
+    shouldChange('interactionCheck', 'to define the policy for requiring End-User interactions');
     if (!ctx.oidc.session.sidFor(ctx.oidc.client.clientId)) {
       return {
         error: 'consent_required',
@@ -1105,7 +1124,7 @@ const DEFAULTS = {
     // @param sub {string} - account identifier (subject)
     // @param token - is a reference to the token used for which a given account is being loaded,
     //   is undefined in scenarios where claims are returned from authorization endpoint
-    changeme('findById', 'to use your own account model');
+    mustChange('findById', 'to use your own account model');
     return {
       accountId: sub, // TODO: sub property in the future
       // @param use {string} - can either be "id_token" or "userinfo", depending on