Support dynamic SAML configuration lookup

[stavros-wb]

No description provided.

[Destreyf]

@stavros-wb I don't have any control here, but i noticed your PR and this is something i was looking into doing myself, it'd be nice to see an example how this can be used included in the PR, maybe it'll get noticed shortly and merged in.

[stavros-wb]

sure @Destreyf, say you have a model IdP which has a field options containing exactly the same key, values you would configure statically. Then you could:

function fetchSamlOptions(req, cb) {
  var idpId = paramsFrom(req);
  IdP.find({ id: idpId }, (err, data) => {
    if (err) {
      return cb(err);
    }

    var idp = data[0];
    cb(null, idp.options);
  });
}

var strategy = new MultiSamlStrategy(
  {
    passReqToCallback: true,
    failureRedirect: '/',
    failureFlash: true,
    fetchSamlOptions
  },
  verify
);

[sp90]

@bergie Should we get this merged?

[sp90]

@stavros-wb i'm trying to use your version to test it out.

Whats the verify function for, and then I get this error SAML assertion expired

let verify = () => {
	console.log("test");
	return;
}

let fetchSamlOptions = (req, cb) => {
	process.nextTick(() => {
		var opts = {
			callbackUrl: 'https://great-rattlesnake-45.localtunnel.me/auth/saml/callback',
			entryPoint: 'https://introdus-dev.onelogin.com/trust/saml2/http-post/sso/817114',
			issuer: 'https://app.onelogin.com/saml/metadata/69cf9e71-c0ff-4049-bf7f-5c145b8ccbaa'
		};

		console.log("opts: ", opts);

		cb(null, opts);
	})
}

passport.use('saml', new MultiSamlStrategy({
		passReqToCallback: true,
		failureRedirect: '/',
		failureFlash: true,
		fetchSamlOptions: fetchSamlOptions
	},
	verify
), (profile, done) => {
	console.log("profile: ", profile);
	return done(null, profile);
});

[stavros-wb]

It's the callback to get the user profile, which you already pass as an anonymous function.

[sp90]

@stavros-wb have you tried making all this for SPA, using rest

[stavros-wb]

@sp90 I'm not sure I understand, can you please elaborate?

[sp90]

@stavros-wb i made it work, everything is fine i had some trouble understanding what happend 👍

[markstos]

To have this PR considered further, add documentation for it.

[stavros-wb]

@markstos I've added a configuration example and removed the What if I have multiple SAML providers... question from the FAQ

[markstos]

@stavros-wb Thanks for this feature.

Did you consider the alternative of creating multiple instances of passport-saml instead?

To create multiple instances of passport-saml yourself, you'll end use calling
passport.use() multiple times with a unique name for each strategy. You
either need to pass the custom strategy name as the first argument to
passport.use, or use the name option described in the docs.

Each instance needs its own callbackUrl as well it's own set of routes
to handle the outbound redirection, the inbound callback URL and displaying
the metadata.

In this solution, it looks like the inbound callback URL is shared among multiple SAML IdPs, while the the outbound redirection is dynamically generated based on the lookup. Do all IdPs get the same metadata siince the inbound callback URL is the same?

Both approaches have merit, I just want to understand the differences more.

If it's merged, I'd like the fetch prefix to be renamed to get.

There are several precedents in the code of using the get prefix: getCallbackUrl, getProtocol, getLogoutResponseUrl, getAuthorizeUrl'. There are are currently no other places in the project where we use fetch instead, so the change will improve consistency.

Thanks!

[sp90]

@markstos the reason why creating multiple instances is not durable is, let's say you have a SaaS where you have numerous organisations as customers, and their employees need to login via SAML. Then you have to restart the server and code the integration for each of them instead of just getting the SAML config from the database that matches the organisation?

So the things your noting is there something I can do to help resolve these issues, and if can you point me to where I have to modify to get this up and running? @stavros-wb @markstos 👍

[sp90]

I'm testing a configuration suggestion for dynamicly loading configs and this seems to work - please let me know if you see any major issues with this (external) solution of dynamicly loading configs 👍

var SamlStrategy = require('passport-saml').Strategy;
var Passport = require('passport').Passport;

let multiConfPassport = (req) => {
  let instance = new Passport();

  instance.use('saml', new SamlStrategy(req.passportSettings, (req, profile, done) => done(null, profile)));
  instance.serializeUser((user, done) => done(null, user));
  instance.deserializeUser((user, done) => done(null, user));

  return instance.authenticate('saml', {
    session: false
  });
}

let getConfg = (req, res, next) => {
  db.getConfig()
    .then(settings => {
      req.passportSettings = settings

      next();
    })
},

route.post('/saml/callback',
  getConfg,
  (req, res, next) => {
    multiConfPassport(req)(req, res, next);
  },
  (req, res) => {
    // Do stuff with the user match with your system
  })

route.get('/saml',
  getConfg,
  (req, res, next) => {
    multiConfPassport(req)(req, res, next);
  })

[stavros-wb]

@markstos

In this solution, it looks like the inbound callback URL is shared among multiple SAML IdPs, while the the outbound redirection is dynamically generated based on the lookup. Do all IdPs get the same metadata since the inbound callback URL is the same?

yes the endpoint is the same but you can depend on the request (a body parameter, query param or even a subdomain, common in SAAS products) to select the correct configuration.

To create multiple instances of passport-saml yourself, you'll end use calling
passport.use() multiple times with a unique name for each strategy. You
either need to pass the custom strategy name as the first argument to
passport.use, or use the name option described in the docs.

I agree with @sp90 that this would take some effort to get it right. having a persistence storage (db, distributed cache) can go long ways for when you have multiple node instances and need to make live changes

@sp90

I'm testing a configuration suggestion for dynamicly loading configs and this seems to work - please let me know if you see any major issues with this (external) solution of dynamicly loading configs

that works alright and the only thing I see missing is the logout functionality (you should also dynamically lookup the specific SAML config). If you add the code for the logout functionality your code will look very similar to this PR,

I think this is useful and this library can support it out of the box. @markstos if you wish to proceed I can go on and make the styling changes

[markstos]

@stavros-wb, I've reviewed your code again and compared it with the alternative design by @sp90.

While I appreciate keeping this library light, I think your code to add built-in support is easier for module users to use and understand, and will be simpler for us to document. Also, there is not lot of code complexity added, and it's all isolated in a new SAML strategy file. If users don't want to use this code path, they don't have to.

Bottom line: please proceed with the requested refinements. I intend to merge this. Thanks!

[stavros-wb]

@markstos applied change and rebased to master

[sp90]

@stavros-wb & @markstos should the dynamic config be a wrapper module to the main lib so not everyone needs the full lib, but only need to get the dynamic config loading wrapper if they need it?

[markstos]

@sp90, @stavros-wb, it's true this functionality doesn't need to be in the main module.

As I look now, I see that the "Multi-SAML Strategy" only uses the publicly exported "strategy" and "saml.SAML" objects, so it could be also be distributed as passport-multi-saml or passport-dynamic-saml.

While it's appealing to maintain less code and docs in the core, I think it will be more convenient for users to have this here. Here's a small suggested refinement- don't export the function from index.js, so it's not loaded if it's not needed.

From the Node.js docs:

It is possible to require specific files or sub modules distributed with a module by including a path suffix after the module name. For instance require('example-module/path/to/file') would resolve path/to/file relative to where example-module is located.

So something like this could be made to work:

const MultiSamlStrategy = require( 'passport-saml/MultiSamlStrategy' );

That would provide the convenience of a single distribution, but would not require loading the code if it's not needed. Sound good @sp90 and @stavros-wb ?

[stavros-wb]

@markstos i've removed the export from the index

[markstos]

Thanks for the update. these docs need to be updated to match as well. Hopefully we can make:

const MultiSamlStrategy = require( 'passport-saml/MultiSamlStrategy' );

And not this uglier and more confusing syntax:

const MultiSamlStrategy = require( 'passport-saml/lib/passport-saml/MultiSamlStrategy' );

This might involve moving the MultiSamlStrategy.js file, but I haven't tested.

[stavros-wb]

There 3 ways to do this AFAIK

1. export the new strategy from index. and users can use it with

const { MultiSamlStrategy } = require('passport-saml')

2. leave the strategy in lib/passport-saml

lib/
└── passport-saml
    ├── index.js
    ├── inmemory-cache-provider.js
    ├── multiSamlStrategy.js
    ├── saml.js
    └── strategy.js

and users can use it with

const MultiSamlStrategy = require('passport-saml/lib/passport-saml/multiSamlStrategy')

3. move strategy to parent directory

LICENSE
README.md
docs
└── adfs
    ├── NameIDFormatError.jpg
    ├── README.md
    └── retrieve_adfs_certificate.sh
lib
└── passport-saml
    ├── index.js
    ├── inmemory-cache-provider.js
    ├── saml.js
    └── strategy.js
multiSamlStrategy.js

and users can use it with

const MultiSamlStrategy = require('passport-saml/multiSamlStrategy')

I would prefer the first options, which I think is cleaner. I don't think adding a few more functions will be of any consequence to users that don't wish to use the feature.

@markstos how'd you like to proceed?

[markstos]

Option 3, thanks. I realize the file placement is inconsistent with the others, but it serves the purpose of creating a clean export syntax. Mark

On Fri, Aug 31, 2018 at 4:44 AM Stavros Soleas ***@***.***> wrote: There 3 ways to do this AFAIK 1. export the new strategy from index. and users can use it with const { MultiSamlStrategy } = require('passport-saml') 1. leave the strategy in lib/passport-saml lib/ └── passport-saml ├── index.js ├── inmemory-cache-provider.js ├── multiSamlStrategy.js ├── saml.js └── strategy.js and users can use it with const MultiSamlStrategy = require('passport-saml/lib/passport-saml/multiSamlStrategy') 1. move strategy to parent directory LICENSE README.md docs └── adfs ├── NameIDFormatError.jpg ├── README.md └── retrieve_adfs_certificate.sh lib └── passport-saml ├── index.js ├── inmemory-cache-provider.js ├── saml.js └── strategy.js multiSamlStrategy.js and users can use it with const MultiSamlStrategy = require('passport-saml/multiSamlStrategy') I would prefer the first options, which I think is cleaner. I don't think adding a few more functions will be of any consequence to users that don't wish to use the feature. @markstos <https://github.com/markstos> how'd you like to proceed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#276 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABk5Qhyds4pxpgIP99lErNULTKtJPt-ks5uWPd4gaJpZM4TQVB2> .

-- *Mark StosbergDirector of Systems and Security | RideAmigos <https://rideamigos.com/> | 765-277-1916 | mark@rideamigos.com <mark@rideamigos.com>*

[stavros-wb]

@markstos done