OIDC proxy implementation #1383

OtterleyW · 2020-12-02T08:10:01Z

In this PR, we add helper functions for using FTW as OIDC proxy. These new helpers are added in the api-util/idToken.js file. This also introduces two now server-side endpoints for serving discovery document and jwks uri.

server/api/auth/createUserWithIdp.js

server/api/auth/loginWithIdp.js

Gnito · 2020-12-02T11:35:55Z

server/apiRouter.js

+// loginWithIdp endpoint in Flex API to authenticate user to Flex
+router.get('/auth/linkedin/callback', authenticateLinkedinCallback);
+
+router.get('/.well-known/openid-configuration', openIdConfiguration);


It would be good to have some comment or even link somewhere, where one can read more about these routes that this setup requires.

server/api-util/idToken.js

Gnito · 2020-12-02T12:03:40Z

server/api/auth/linkedin.js

+        defaultConfirm,
+      };
+
+      console.log('userData:', userData);


Does this contain something that should not be saved to server logs in production?

Gnito · 2020-12-02T12:10:32Z

server/api/auth/linkedin.js

+// to log in the user to Flex with the data from Linkedin
+exports.authenticateLinkedinCallback = (req, res, next) => {
+  passport.authenticate('linkedin', function(err, user) {
+    loginWithIdp(err, user, req, res, 'client-id', 'linkedin');


I'm quite confused about how this 'client-id' string works.
In loginWithIdp.js, there's CLIENT_ID constant and clientId param and they are not the same. There probably should be some kind of renaming made to these client id variables (and values?).

My initial idea was that we are not going to keep the LinkedIn implementation in FTW so I cut some corners when I did testing. So this is one of those parts. But if we want to keep this in the repo, then this of course needs to be fixed.

Anyway, this 'client-id' is just a hard-coded test value. This 'client-id' the same thing you should have in process.env.CUSTOM_OIDC_CLIENT_ID and it's the value that should match the client id you have in Console. Most likely that environment variable is named to match the identity provider you are using (LinkedIn, Instagram, your own system etc.).

Makes sense 👍

How about the renaming of things in loginWithIdp.js file? (And sorry, this should have been addressed already in previous PRs.) In that file, there is CLIENT_ID referring to Flex client id and "clientId" referring to IDP client id - and then code like clientId: CLIENT_ID, - so, I'd imagine that this could be pretty confusing for someone who tries to understand what happens under the hood.

This is a really good point and I think renaming that parameter makes sense. I added that change to this PR now.

lyyder

👍

server/apiRouter.js

lyyder · 2020-12-03T22:52:36Z

server/api-util/idToken.js

+  const jwt = new SignJWT({
+    given_name: profile.firstName,
+    family_name: profile.lastName,
+    email: profile.email,
+    email_verified: profile.emailVerified,
+  })
+    .setProtectedHeader({ alg: 'RS256' })
+    .setIssuedAt()
+    .setIssuer(issuerUrl)
+    .setSubject(userId)
+    .setAudience(clientId)
+    .setExpirationTime('1h')
+    .sign(privateKey);


Change request: Did we have the kid value in the token header? I might have mentioned that it's not compulsory but maybe we could add it by default to the ID token. Even though it's one more random value for the user to decide, enforcing the kid value in the token and in JWK could be useful as key caching in the Flex API relies heavily on it.

Yes, we had but then I removed it to test if it works without it (and it did). I didn't want to leave it there with any hardcoded value. But I think it makes sense to have it in the example so it there again. Now the key id is also read from environment variables so it can be basically any string user decides. I guess this is ok?

lyyder · 2020-12-03T22:53:03Z

server/api-util/idToken.js

+// api/.well-known/jwks.json endpoint as stated in discovery document
+exports.jwksUri = (req, res) => {
+  fromKeyLike(crypto.createPublicKey(rsaPublicKey)).then(jwkPublicKey => {
+    res.json({ keys: [{ alg: 'RS256', use: 'sig', ...jwkPublicKey }] });


Same here about he kid attribute.

lyyder

👍

server/api-util/idToken.js

lyyder · 2020-12-14T15:19:31Z

server/api-util/idToken.js

+  const jwkKeys = keys.map(key => {
+    return fromKeyLike(crypto.createPublicKey(key.rsaPublicKey));
+  });
+
+  Promise.all(jwkKeys).then(resolvedJwkKeys => {
+    const jwkKeys = resolvedJwkKeys.map((resolvedKey, i) => {
+      const originalKey = keys[i];
+      return { alg: signingAlg, kid: originalKey.keyId, use: 'sig', ...resolvedKey };
+    });
+    res.json({ keys: jwkKeys });


Could the index access be avoided if this was was written somehow like this:

const jwkKeys = keys.map(key => { return fromKeyLike(crypto.createPublicKey(key.rsaPublicKey)) // reads alg from key .then(res => { return {alg: key.alg, kid: key.keyId, ...res}; }); }); Promise.all(jwkKeys).then(resolvedJwkKeys => { res.json({ keys: resolvedJwkKeys }); });

Thanks for the example! I tried to make it work like this yesterday but I might have missed something and the index access was the only way I managed to generate the list correctly. However, now it works like this and I agree that this is a better approach.

…s confusing