Planning for passport-saml 2.0 release

[markstos]

Now that passport-saml 1.1 has been released, let's look forward.

These are my hopes for a passport-saml 2.0 release:

Compatibility

• Drop support for Node.js < 8. async/await and other newer JavaScript idioms are fair game to use now.
• From "Q" promises to native promises (and possibly bluebird?)

Project Organization

• Split library into two: A core SAML library and a wrapper specific to passport.
  • Potentially have different owners for the two projects.
  • Core library would strive for compatibility while making heavy use of async functions instead of callback-style code.
  • The current tests.js is 2,300 lines long. As part of this work, the test file should be split into several files.
• Move home from bergie account to a "Github Organization"
• Add at least one more committer / project manager
• Convert test runner from Mocha to Jest. Jest tests can run in parallel by default for faster development iterations Tests could still be written in Mocha-style
• Add .editorconfig
• Add code linting to the test suite.

Features

• As before, new features will be considered if they improve spec-compliace and contributed with a PR that references the related parts of the spec and includes automated tests and documentation as well as code. This is a community-maintained project. No one is hanging out waiting to write features for other people.

Collaboration

I'm a passport-saml user who ended up the maintainer. I have limited time to help with the project but would like to see this popular security-related module be well-maintained. I'm favor of significant updates to modernize the library, but will look to others to make big contributions to make it happen.

CC: @josecolella @stavros-wb @mlunoe @osan15 @cjbarth @andkrist @siren @LoneRifle @Archinowsk @VilleMiekkoja

[josecolella]

@markstos One thing I'd like to throw out there is conversion of the code base to typescript. Don't know how you feel about this. It would bring enforcement of types, scalability, and more maintainable codebase.

[cjbarth]

@josecolella I'd actually be all for that, and it would be easy enough to start migrating that direction by simply adding a compilation step and renaming a few files since all .js is valid .ts. The good news is that we already have fairly comprehensive types from DefinitelyTyped that we can use. However, we'd have to rely on DefinitelyTyped for up-stream types since those projects aren't native TypeScript (Passport, Express, etc.). Thus, I'm not sure if that is the direction the community would be most comfortable with.

I'm also in favor of adding structure to the tests, even getting some code-coverage reports going. I'm not sure that the speed of our tests running is really a problem, so I'm not convinced of the need for work to go in that direction. However, splitting the library seems to be a top-requested feature, so that would be nice to see.

I'm divided on the wholesale migration to async/await. I see how it would clean up the code, but it also can, if not careful, enforce a pattern on the consumer that isn't common in Node. Node currently uses the Node-back pattern, which is very different from the async/await pattern. Internally, we can use it, even writing new code with it, or leveraging it when we refactor to two modules, but I think the interface should still follow the Node-back pattern.

What I really think we need is a .editorconfig and some linting so that we can enforce some code style. Right now things are a little all over the place. Should we move to JavaScript Standard Style? That would be quick and easy and would be the same that Node uses.

[josecolella]

@cjbarth Regarding tests, I'd be very much in favor of moving to Jest, where you get coverage reports out of the box. My two cents on async/await is that it allows for code to be tested much easier. @markstos I'd also like to add @toddbaert on this. We both use this library extensively, and I'd think he'd be a good contributor to the initiatives that you'd stated.
We could start with creating a github organization

[toddbaert]

@cjbarth I agree regarding maintaining the existing API "node-back" style - I wonder if adding an async API in addition could be a possible solution, many libs have handled this conundrum this way, whether in a separate package or the same.

[cjbarth]

That could probably work @toddbaert . We would just have to detect if there is a function as the last argument and, if not, return a Promise, which is awaitable.

[markstos]

Re: TypeScript

I'm open to that. My company is adopting TypeScript in our major project. As a popular, security related module, I want to be careful about our choices there. There have been projects that have been compromised with changes hidden into minified code that weren't in the main code. My only concern is that we can provide assurance that the generated code matches the source code.

async/await vs node-back

I think @toddbaert solution supporting both callbacks and Promises is fine. That also provides backwards compatibility for us. The popular Mongoose library uses this approach. Also, the popular aws-sdk uses this approach.

.editorconfig and linting

Great ideas. I added them to the top level list. I don't have strong feelings which style we use, just any standard style. JavaScript Standard Style sounds as good as any to me.

Jest

Moving to Jest doesn't have to mean a big change in how tests are written. We can use a jest runner for mocha tests.

Moving forward

It sounds like there is general alignment on the direction. Soon we'll be looking for volunteers to help implement the updates!

[cjbarth]

The only reason I mention JavaScript Standard Style is because it is dead simple. If anyone has a suggestion for something else, I'm open to hear it.

[cjbarth]

Does anyone see an advantage of moving away from a dependency on a Promise library (Q) and just using native promises?

[toddbaert]

@cjbarth 2c: Personally, I strongly prefer native promises.

[markstos]

@cjbarth I had forgotten about the dependency on q. I've added replacing our promise library to the roadmap. I would prefer native promises where possible and bluebird where we need extra features. For example, bluebird has a helper method to help a function accept a callback or return a promise, asCallback().

We should survey what compatibility issues there might be with the change (if any), but I think we should proceed with a change here.

[sibelius]

to split this package in 2, we can use yarn workspaces, so we can easily manage them

is there someone working on this?

we can move to yarn workspaces first with only one package (the current package), and then start extracting the core logic

[markstos]

No one is working on this that I'm aware of.

…

On Wed, May 29, 2019, 3:56 PM Sibelius Seraphini ***@***.***> wrote: to split this package in 2, we can use yarn workspaces, so we can easily manage them is there someone working on this? we can move to yarn workspaces first with only one package (the current package), and then start extracting the core logic — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#373>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAGJZIEXWLIGA5P2DQ256TPX3UXLANCNFSM4HMQCRHA> .

[ssbrewster]

Decoupling the decryption from validatePostResponse would be great as part of this for e.g. extracting the Issuer from an encrypted message in a MultiSamlStrategy's getSamlOptions method (arguably it could be done before a v2.0).

[ssbrewster]

I've just realised that this lib does not support a SAML Response that has the entire message encrypted as opposed to only the assertion. It checks for the presence of an encrypted assertion when validating the POST response https://github.com/bergie/passport-saml/blob/5cdf341ae2a081eb995ab59aec0c81d732ca2758/lib/passport-saml/saml.js#L616-L617 otherwise it looks like it throws an Invalid signature: No response found error https://github.com/bergie/passport-saml/blob/5cdf341ae2a081eb995ab59aec0c81d732ca2758/lib/passport-saml/saml.js#L709-L712

It looks like this is being worked on #345

[sibelius]

I've moved the project to a monorepo structure here #383

so we can have saml-core and passport-saml and maybe more related packages if we needed

I can also setup eslint, lint-staged, commitlint, prettier and so on to improve commit messages and code in general

I can also move tests from mocha to jest

Can you add me as a contributor?

I think moving this package to typescript would be great, we can use babel to ship builded version on npm

I'm interested in the split in 2 packages

[markstos]

@sibelius I can't add Github contributors here-- I am not the project owner. That's @bergie and he has been unresponsive for months over a number of a communications. Plan B is to fork the repo. Unfortunately without @bergie's blessing, we can't bring the issues with us, but the issues and PRs will at least remain available, so we can copy over interesting ones.

I recommend making a new Github Organization/Team account named "node-saml" or "saml-core". Within it, well have two projects (but I guess one repo?), "saml-core" for the library and "passport-saml" for the wrapper.

Having a Team/Org with multiple managers will be a better long-term fit for a community maintained project.

It appears I do have full admin writes on the NPM package and can add and remove other maintainers there. 👍 I might ask some of the other inactive admins if they still want publish access for the NPM repo. It increases risk for a security-related package to have inactive accounts with ability to publish new versions!

Since you've got momentum on this, you are welcome to set up the organization/team account yourself and invite me to it if you'd like.

[cjbarth]

Thankfully @bergie is still active on GitHub, so if we got all set up for a move to a new home, there is a possibility that he might click one button for us. Here's hoping!

[DbCrWk]

Y'all this package is great, and I'd be happy to help with updating parts of this codebase.

From an end-users perspective, my two cents on some of these issues:

• Native promises are the way to go, especially coupled with the async / await syntax
• I continually have the TypeScript vs. Flow debate when writing new code, and I currently prefer Flow, for a variety of reasons. That being said, I think having types in general would be nice. Type safety catches many, many issues and a library like this would be relatively easy to introduces types into.
• I think splitting out the code into more discrete components would be great, both for testing purposes, but also for usage. It would be nice to have some of the internal functions exposed as a part of a core SAML API, and then have a wrapper library for passport.

[markstos]

@DbCrWk Thanks for the input!

I don't have much exposure to Flow. My shop already uses TypeScript via Angular is introducing TypeScript on the backend as well. It would definitely be easier for me personally to deal with more TypeScript.

[ricardosaracino]

So i need to use multi-saml and aes256-cbc assertion encryption ... passport-saml-encrypted does not support multi ... i would be willing to make some changes as i would need to fork this at the very least anyways.

.. this doesnt seem to complicated.. is there a reason this is not in the lib to being with?

https://github.com/lmarkus/passport-saml-encrypted/blob/4ed3ed4950f0d8701882881dc192f5fc5e585602/lib/decryptSAML.js#L41

[cjbarth]

@ricardosaracino Please start a new thread for this matter as your comment doesn't specifically relate to v2 of this library.

[markstos]

@ricardosaracino We strive to spec-compliant. Assuming aes256-cbc is in the spec, we are interested to support it unless there's a strong reason not to.

[ricardosaracino]

If you are interested i am trying to meet this standard... the relevant information is on page 33

Implementations MUST support the block encryption algorithms identified by the following URIs in conjunction with the use of XML Encryption [XMLEnc]:
• http://www.w3.org/2001/04/xmlenc#tripledes-cbc
• http://www.w3.org/2001/04/xmlenc#aes128-cbc
• http://www.w3.org/2001/04/xmlenc#aes256-cbc

https://canada-ca.github.io/CATS-STAE/archive/CATS_V2_0_Deployment_Profile_Final_r8_2_en.pdf

[markstos]

The links to the related standard are helpful. A PR to support more algorithms supported by the standard are welcome. "@" mention me in the PR.

[markstos]

Thanks to @walokra for helping address two of items on the 2.0 TODO list. Reviewed, approved and merged.

[mightypenguin]

Can we also support configuration using metadata? (dynamically loaded via URL)
See Ruby Saml module for an example:
https://github.com/onelogin/ruby-saml#metadata-based-configuration

This is especially useful for me as the cert updates then happen automatically as soon as they are pushed to the updated IDP XML.

[markstos]

@mightypenguin That's a welcome feature and one that my company has implemented in our code base. For a version to be included in passport-saml, I'd like a solution that's resistant to the remote URLs not being constantly available. For example, it would include a cache mechanism to fall back on and possibly a retry algorithm to recover from temporary outages. Open an Issue or PR if you want to work on it further. It's unlikely someone else is going to work on it for you.

[zoellner]

What's the latest on splitting the repos? I'd be interested in that since I have a project that could make use of the saml lib but has no need for the passport pieces.
If the general idea is this I can do that next week:

1. start an org
2. fork this repo
3. remove all the passport related code
4. publish new repo to npm
5. add PR to this repo to require the new lib-repo

What I'd need is some guidance on the tests. I assume those would need to be split into two as well.

[markstos]

@zoellner Sorry for the delay. I see you've started on the work already here: https://github.com/zoellner/node-saml/commits/master

There hasn't been much progress. I'm afraid I've been a bit of a bottleneck as one of the only admins of this repo.The original author is not active and I'm just another library user who stepped up to help and was granted commit access.

However, I don't have time to keep up with the issue traffic in this repo. It really needs a team to manage, but the team needs to careful and trusted since it's an important authentication library that would make a good target for attackers to sneak vulnerabilities into.

How's your library coming along?

[zoellner]

Totally understand. The library is coming along well. For now I've done some refactoring but not functional changes. I'm keeping v1 backwards compatible, so that could be a drop in replacement for the embedded one in this repo.

I do plan to eventually reorganize things a bit into a v2 (and convert more callbacks to async functions) at which point passport-saml would need a tiny wrapper or change the calls to the library a bit.

I've gotten more familiar with all the test in there as well and think I'll be able to organize them a bit better (XML or signature related/SAML request related).

I'll report back here when I think it's at a point where it should have maintainers from here review it. Happy to move it into an org as well if at that point people think it would be valuable for this repo.

[markstos]

I'm interested in breaking changes to convert callback APIs to async/promise APIs as well, so don't hold back on those.

I don't use the library with passport myself anyway.

[ghost]

The 1.4.0 update included breaking changes. The major rev should have been bumped today. This broke my build, it's going to break others as well.

[markstos]

@aloswald I'm open to that, but say more about what broke your build? All of our tests passed.

[markstos]

@aloswald Also please try 1.4.2. Please open a new issue to report an unintentional breaking change.

[ghost]

@markstos Will do, looking into it so once I have my issue fixed I will let you know (hopefully it helps someone else if they have issues as well).

[markstos]

@aloswald Sorry to break your build. Since there's a security fix, I wanted to err on the side of not bumping the version so people would be more likely to upgrade and receive the fix. (The security issue is in the xml-cryptop dependency. I'm not sure if this project was even exposed to the vulnerability.)

[cjbarth]

@aloswald For sure we want to add a test to cover the break you encountered. Once we can validate that we did break something, then we'll update the documentation and cut a major release. Thank you for your patience and cooperation.

[markstos]

I'm release 2.0 today and most of these goals have been met, so I'm closing this. We can open a new Roadmap / Future release issue for future big picture planning.