Compatibility with cookie-session

[patrick-m-m]

I'm looking at using this with a GraphQL service that we're hoping can remain completely stateless. We included the cookie-session package to put the authenticated OIDC tokens into the client cookies. There is an unfortunate interaction where it seems this passport strategy is setting some secrets in the session.

{
  "regularSessionKey": "regularSessionValue",
  "openidconnect:openid-connect": {
    "state": {
      "handle": "1ds+QByb7tO+p2zkV50WaqA4",
      "issuer": "",
      "authorizationURL": "",
      "tokenURL": "",
      "userInfoURL": "",
      "clientID": "",
      "clientSecret": "",
      "callbackURL": "",
      "params": {
        "response_type": "code",
        "client_id": "",
        "redirect_uri": "http://localhost:8080/oauth/callback",
        "scope": "openid extra",
        "state": "1ds+QByb7tO+p2zkV50WaqA4"
      }
    }
  }
}

Why does the strategy need to store so much in the session? All of the state keys except params.scope, handle, params.state (which are the same) are passed in when I construct new Strategy().

Since sending the secret with every client cookie is not an option, should that be interpreted as a clear indication that the passport-openidconnect requires a server-side session store? If so, could this be documented somewhere?

[ShayDavidson]

@patrick-m-m hey patrick, have you found a workaroound for this since last year? :) just run into the same issue

[georgwittberger]

I've encountered the same issue using this Passport strategy in conjunction with koa-passport and koa-session which also stores the session in a browser cookie by default.

I've tried stripping out the clientSecret property of the state object in the session but then the authentication fails because the strategy relies on these parameters in the state. See https://github.com/jaredhanson/passport-openidconnect/blob/52d6874d1031dd9a1ed642871dffaba44050608b/lib/strategy.js#L415

The function Strategy.prototype._getOAuth2Client is invoked with the state obtained from the store which is the SessionStateStore by default. But this is unnecessary because the function could very well access instance variables of the strategy. For example:

function Strategy(options, verify) {
  // ...
  this._clientID = options.clientID;
  this._clientSecret = options.clientSecret;
}

Strategy.prototype._getOAuth2Client = function (config) {
  return new OAuth2(this._clientID, this._clientSecret,
                    '', config.authorizationURL, config.tokenURL);
}

Until this has been fixed this must be considered a severe security risk when used with a cookie-based session store.

[GitProdEnv]

I agree, that this is a dangerous situation when you implement this by just sending down cookies to the client.

Until this has been fixed this must be considered a severe security risk when used with a cookie-based session store.

The docs should warn you about sensitive information leakage, that the client secret and client id will be passed into the storage implementation by its second arg (meta)

SessionStore.prototype.store = function(req, meta, callback) {

But, If you write your own implementation, you are responsible for security issues. This library is very secure, if you don't implement your own store and continue to use the default session store.

There are two options currently available to make it stateless via an HTTP Cookie approach:

• Make sure that you encrypt sensitive information, before sending down the cookie to the client (e.g. client_secret, client_id), this prevents sensitive information leakage
• Store the discovery properties as instance properties, such that the cookie only contains the state, nonce properties, etc... . I don't see any reason, why the data has to be stored in a session cookie, since this is static data (unless you are using the "dynamic" approach). The manual "setup" approach works fine with this proposed procedure, but I don't know if this is a solution for the dynamic approach (I didn't look too much into this, and you could just opt for the first bullet point in this case), but you shouldn't use the dynamic setup in most of the cases anyway.