Require cert for every strategy #548

cjbarth · 2021-02-26T23:20:52Z

Description

This addresses the issue that a SAML strategy could be constructed without a certificate, which would allow an insecure situation. The changes needed to force a cert were quite significant. Types had to be adjusted to accommodate these changes. Additionally, changes were made to tests to get them to fail with the correct error. Our tests were in a situation such that once a cert was required the tests would fail as expected, however, the error message that was shown in no way indicated what the reason for the failure actually was. The tests were updated only to get them to actually show the failure saying that "cert is required". This should yield a much better debugging experience going forward.

While the initial version of this PR has many tests failing, I wanted to get some feedback on the approach taken before I fix the tests by adding in a cert. I intend to add in the certs next week to complete this.

This PR also includes the test conditions that were mentioned in #523.

Checklist:

Issue Addressed: [Closes Missing cert configuration option disables signature validation silenty/without huge warning messages #523]
Link to SAML spec: [N/A]
Tests included? [Yes]
Documentation updated? [Yes]

cjbarth · 2021-02-26T23:22:52Z

@markstos, @gugu, @srd90, @HendrikJan: Did you want to have a look at this before I fix all the tests to include a cert?

markstos · 2021-02-26T23:33:32Z

@cjbarth Seems like a good path to me. Thanks for all the work.

s/manditory/mandatory/g

HendrikJan · 2021-02-27T08:06:13Z

@cjbarth your changes look like real improvements, please proceed! I'll gladly update #536 after this has landed.

src/passport-saml/multiSamlStrategy.ts

gugu · 2021-02-27T18:32:53Z

Are you sure change of casing needed? It will start to silently ignore these parameters in existing JS code, which will lead to unexpected behavior changing in module users' code

cjbarth · 2021-02-28T02:33:20Z

It isn't needed, but it does unify our casing. We had a mix of capitalizing acronyms entirely and only capitalizing the first letter. I have followed the .Net convention here (which is very close to TypeScript in most ways): https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions?redirectedfrom=MSDN. You might also see this short, reasonable post: https://www.approxion.com/capital-offenses-how-to-handle-abbreviations-in-camelcase/

Thus, in a variable name "SAML" is "Saml" and "RAC" is "Rac". There are already many things in this version that are breaking changes so it seems like a good time to do this. If there are other things of this nature, I don't mind a branch off the 2.x tag and putting in a few more deprecation warnings for things we know are breaking in this code.

src/passport-saml/saml.ts

srd90 · 2021-02-28T20:21:59Z

Did you want to have a look at this...

@cjbarth Due to my limited typescript/javascript knowledge and quite large changes in this pull request (which include from my point of view ts/js specific "magic") I'm unable to see just by browsing the code whether cert is now truly mandatory and that there isn't any execution path which would ignore signature checking (also if cert is array of certificates with null/empty values)

src/passport-saml/saml.ts

HendrikJan · 2021-03-01T08:59:02Z

I'm missing time and experience to fully review these changes.
The changes look well thought through to me and I learn new things while reading it.

cjbarth · 2021-03-01T22:58:46Z

@srd90 I appreciate your feedback. I updated the code in my latest commit to do better checking of the cert property to check for each member of an array. This function is the only one that reads this.options.cert. Basically, there is no code path that will allow a cert to go unused. In fact, the SAML class can't even be constructed without specifying a cert.

I will add more test to exercise the two new errors that this certsToCheck function throws in a future commit.

A lot of tests are fixed. There is one test that wasn't hermetically sealed. There are other tests that are now failing because they aren't signed, so providing a fake cert doesn't help them pass.

cjbarth

I've reviewed my own code and I think we're good to go on this. Of course, feedback is welcome.

I expect that there is more work that can be done to strengthen the types and to add tests to make sure that we have better coverage, especially of the code that throws exceptions. However, at this point, I believe this will cover cert being required. Of course, this is a breaking change.

I will create a follow-up PR to remove support for privateCert, since it is deprecated anyway.

src/passport-saml/saml.ts

src/passport-saml/multiSamlStrategy.ts

cjbarth · 2021-03-15T23:58:26Z

@gugu Are there any more changes you'd like to suggest on this? @HendrikJan or @srd90 Do you have any thoughts on this?

gugu · 2021-03-16T09:18:36Z

No, that's all

forty

I made a few generic comments, I don't know the code base too well yet, so it's hard for me to comment on the code organization.

One thing I noticed it that this PR contains quite a lot of lint fixes (which is great, for example '!= => !==) and caml case fixes (RAC => Rac) which is also nice, but I would strongly suggest to make a dedicated PR so it can be merged quickly, and then it's easier for everyone to focus on the actual changes (sorry, yet another comment that's a bit generic, that's all I have for now ;) )

forty · 2021-03-16T08:33:35Z

src/passport-saml/inmemory-cache-provider.ts

-    this.options = options as CacheProviderOptions;
+    this.options = {
+      ...options,
+      keyExpirationPeriodMs: options.keyExpirationPeriodMs,


It's not clear to me what this line does

It allows us to preserve all incoming options, while simultaneously converting the option from partial<CacheProviderOptions> to CacheProviderOptions.

I see. I would find it clearer with

this.options = { keyExpirationPeriodMs: 28800000; // 8 hours ...options };

I've made an adjustment to clarify this code block. Please have a look at this change.

src/passport-saml/types.ts

forty · 2021-03-16T08:45:56Z

src/passport-saml/types.ts

+/**
+ * These options are availble for configuring a SAML strategy
+ */
+export type SamlConfig = Partial<SamlOptions> & MandatorySamlOptions;


this type lost the StrategyOptions part, is it intended?

I see that you as re-adding the type in the constructor here https://github.com/node-saml/passport-saml/pull/548/files#diff-f795057e9c7334402f4693eabcd8786ff52e2f791bf985c10d9acc98a57682aeR23
On the other hand for mutlisaml, you do include it in the type https://github.com/node-saml/passport-saml/pull/548/files#diff-c2e9d28b65918a1bdc8038b9c8dd097a2d455436b6a8855017aaf28ecda216a9R185

I feel it's not consistent. Also, I'm pretty sure that having the StrategyOptions type in the SamlConfig would avoid to have to many an inelegant explicit cast here https://github.com/node-saml/passport-saml/pull/548/files#diff-cb165d3df3a1dace61ca699c892c1408f9d89ebbe253c470a842b46f11e9150cR28

The trick is that SamlConfig requires a cert, and MultiSamlConfig does not. So, we can't pass a MultiSamlConfig to the Strategy ctor. What is more, getSamlOptions() must return an object can can be used in the Stragegy ctor, so it must include a cert, whereas we don't need a cert to construct MultiSamlStrategy.

Having said all that, your original comment holds. Done.

forty · 2021-03-16T08:56:23Z

src/passport-saml/multiSamlStrategy.ts

  VerifyWithoutRequest,
  VerifyWithRequest,
 } from "./types";

 class MultiSamlStrategy extends SamlStrategy {
-  _options: MultiSamlConfig;
+  static newSamlProviderOnConstruct = false;


I would find it more elegant to have it as a protected instance field. The current way forces an explicit cast, which should always be avoided IMO.

And normally I would agree with you, but we can't have an instance field before the call to super, and we need this information before doing that. I did however, mark it readonly.

See "src/passport-saml/multiSamlStrategy.ts:23:3 - error TS2376: A 'super' call must be the first statement in the constructor when a class contains initialized properties, parameter properties, or private identifiers."

forty · 2021-03-16T09:19:12Z

src/passport-saml/saml.ts

+         * - better:  Assertion context must be stronger than all contexts in the list
+         */
+        ctorOptions.RacComparison = ctorOptions.RacComparison ?? "exact";
+        if (!["exact", "minimum", "maximum", "better"].includes(ctorOptions.RacComparison)) {


Why do the check in an IIFE ? I would find it clearer to have it after the object initialization

The idea was to keep all the logic right together so there wouldn't be two places, even in the same ctor, to look for how a property ends up the way it is. However, I can see how IIFE would actually mutate the incoming obejct and make things slower. Done.

forty · 2021-03-16T09:20:18Z

src/passport-saml/saml.ts

-      options.RACComparison = "exact";
-    }
+    const options = {
+      ...ctorOptions,


I'm not sure I understand everything that's going on.
Why not simply have something like:

const DEFAULTS = { passive: false, disableRequestedAuthnContext: false, // all the default values } const options = {...DEFAULTS, ...ctorOptions};

Note that this can still cause problems with the user explicitly passes an option to null or undefined (like new SAML({passive: undefined}) ) so some kind of cleanup might be needed beforehand (for JS users - TS users should not have this issue thanks to typing)

Something like

// remove all the undefined, and the null for non nullable options. Object.keys(ctorOptions).forEach((key) => { if(ctorOptions[key] === undefined) delete ctorOptions[key]; if(ctorOptions[key] === null && key !== 'identifierFormat' && key !== 'authnContext') delete ctorOptions[key]; })

Enumerating all the properties and their default with ?? as you did works too, but in this case I would not add the ...ctorOptions at the beginning, which IMO is confusing (and maybe just explicit all properties that don't have any default, if any)

Putting in ...ctorOptions like is done is so that all properties don't have to be explicitly specified, only ones where there are some defaults or special handling.

Generally, I see what you are after here, but I feel like this logic might be easier to follow than the hoops you describe. It also makes it easier to extend in one place instead of having to go to multiple places. ("I have to remember that if null is a valid value, to go to this other block to explicitly allow that.")

forty · 2021-03-16T10:26:04Z

Just a thought: I think that rather than having MutliSaml being a special case of SingleSaml, you should actually have SingleSaml be a special case of MutliSaml (so reverse everything compared to how it's currently, have the main implementation be MultiSaml)

That makes much more sense, since after all SingleSaml is just MutliSaml with n = 1 (so you could have getSamlOptions be a trivial function in this case, that alway return the same config)

cjbarth · 2021-03-16T11:11:46Z

@forty Thank you very much for your review; I've incorporated several changes because of it. Please see my other comments on your review.

As for the MultiSaml and Saml being the way it is, I'm sure you can imagine which one came first and why it is the way it is :) I am not opposed to reversing things, and would welcome a PR to help with that. I think that is out of scope for this PR though. We want to get this landed so we can stop holding up some other PRs.

cjbarth · 2021-03-18T14:09:23Z

I'm going to land this tomorrow barring any additional feedback so that we can unblock a bunch of other PRs giving others a chance to work with the updated master branch over the weekend should they so choose.

crossan007 · 2021-11-16T14:43:34Z

src/passport-saml/multiSamlStrategy.ts

-      options.requestIdExpirationPeriodMs = 28800000; // 8 hours
-    }
-
-    if (!options.cacheProvider) {


It seems like this breaks the default InMemoryCacheProvider for MultiSamlStrategy added via #350 (discussed in #334)

Require cert for every strategy

ff2f062

cjbarth mentioned this pull request Feb 26, 2021

Add WantAssertionsSigned #536

Merged

cjbarth added 2 commits February 26, 2021 18:37

Fix spelling mistake

4080a51

Fix complication error

4e7a874

HendrikJan reviewed Feb 27, 2021

View reviewed changes

src/passport-saml/multiSamlStrategy.ts Outdated Show resolved Hide resolved

Improve comment

1c169db

srd90 reviewed Feb 28, 2021

View reviewed changes

src/passport-saml/saml.ts Outdated Show resolved Hide resolved

Removed unneeded non-null assertion

c97a599

HendrikJan reviewed Mar 1, 2021

View reviewed changes

src/passport-saml/saml.ts Show resolved Hide resolved

HendrikJan reviewed Mar 1, 2021

View reviewed changes

src/passport-saml/saml.ts Show resolved Hide resolved

Improve cert checking

dd72c77

cjbarth added 4 commits March 1, 2021 18:06

Clean up logic for identifierFormat

b7a8c5b

Fix lots of tests

020f97d

A lot of tests are fixed. There is one test that wasn't hermetically sealed. There are other tests that are now failing because they aren't signed, so providing a fake cert doesn't help them pass.

Fixed all the tests

5a136f8

Clean up lots of TypeScript forced types

3e1386e

cjbarth commented Mar 3, 2021

View reviewed changes

cjbarth marked this pull request as ready for review March 3, 2021 23:11

gugu suggested changes Mar 9, 2021

View reviewed changes

src/passport-saml/saml.ts Outdated Show resolved Hide resolved

src/passport-saml/multiSamlStrategy.ts Outdated Show resolved Hide resolved

cjbarth added 2 commits March 10, 2021 12:01

Remove need for "fake cert"

ef38d58

Stop lazy-loading initialization options

3ccd30f

cjbarth requested a review from gugu March 10, 2021 22:25

cjbarth mentioned this pull request Mar 16, 2021

Improve the typing of the Strategy class hierarchy. #554

Merged

forty reviewed Mar 16, 2021

View reviewed changes

Rename RacComparison to racComparison

6dea742

cjbarth added 4 commits March 16, 2021 06:44

Add StratgeyOptions to SamlConfig type

9fcc972

Make newSamlProviderOnConstruct readonly

161c444

Check racComparison after initialization, not during

6642d6a

Lint

5ca517e

Simply settings defaults for cache provider

060e2d7

gugu approved these changes Mar 16, 2021

View reviewed changes

cjbarth added the semver-major This change requires at least a semver-major version bump label Mar 18, 2021

cjbarth merged commit 224f25f into node-saml:master Mar 19, 2021

cjbarth deleted the manditory-cert branch March 19, 2021 13:44

cjbarth mentioned this pull request May 10, 2021

Changelog #587

Merged

srd90 mentioned this pull request Jun 6, 2021

consider upgrading passport-saml to 3.x GluuFederation/gluu-passport#288

Closed

tunderdomb mentioned this pull request Jun 24, 2021

restore BC for authnContext config #615

Closed

backstage-service mentioned this pull request Sep 10, 2021

Version Packages backstage/backstage#7123

Merged

cjbarth mentioned this pull request Sep 30, 2021

[BUG] Wrong readme example for authnContext for ADFS #642

Closed

crossan007 reviewed Nov 16, 2021

View reviewed changes

srd90 mentioned this pull request Feb 3, 2022

README file should indicate signature checking is critical (not just a good idea) #199

Closed

srd90 mentioned this pull request Mar 30, 2022

[BUG] InResponseTo not valid for all requests #682

Closed

srd90 mentioned this pull request Feb 5, 2023

this.options.authnContext.forEach is not a function OpenCTI-Platform/opencti#2791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require cert for every strategy #548

Require cert for every strategy #548

cjbarth commented Feb 26, 2021 •

edited

Loading

cjbarth commented Feb 26, 2021

markstos commented Feb 26, 2021

HendrikJan commented Feb 27, 2021

gugu commented Feb 27, 2021

cjbarth commented Feb 28, 2021

srd90 commented Feb 28, 2021

HendrikJan commented Mar 1, 2021

cjbarth commented Mar 1, 2021

cjbarth left a comment

cjbarth commented Mar 15, 2021

gugu commented Mar 16, 2021

forty left a comment

forty Mar 16, 2021

cjbarth Mar 16, 2021

forty Mar 16, 2021

cjbarth Mar 16, 2021

forty Mar 16, 2021

forty Mar 16, 2021

cjbarth Mar 16, 2021

forty Mar 16, 2021

cjbarth Mar 16, 2021

forty Mar 16, 2021

cjbarth Mar 16, 2021

forty Mar 16, 2021

cjbarth Mar 16, 2021

forty commented Mar 16, 2021

cjbarth commented Mar 16, 2021

cjbarth commented Mar 18, 2021

crossan007 Nov 16, 2021

Require cert for every strategy #548

Require cert for every strategy #548

Conversation

cjbarth commented Feb 26, 2021 • edited Loading

Description

Checklist:

cjbarth commented Feb 26, 2021

markstos commented Feb 26, 2021

HendrikJan commented Feb 27, 2021

gugu commented Feb 27, 2021

cjbarth commented Feb 28, 2021

srd90 commented Feb 28, 2021

HendrikJan commented Mar 1, 2021

cjbarth commented Mar 1, 2021

cjbarth left a comment

Choose a reason for hiding this comment

cjbarth commented Mar 15, 2021

gugu commented Mar 16, 2021

forty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forty commented Mar 16, 2021

cjbarth commented Mar 16, 2021

cjbarth commented Mar 18, 2021

Choose a reason for hiding this comment

cjbarth commented Feb 26, 2021 •

edited

Loading