[#174710289] Better SAML Response validation #49

BurnedMarshal · 2020-09-25T15:41:51Z

Improve the SAML validation inside getPreValidateResponse:

SAML Response can have only one Response element
SAML Response can have only one Assertion element
Any validation Error on a SAML Response must be sent to appInsights
Send an event if some IDP issuer sends an unsigned Response

Some unit tests were added for the previous scenarios.

pagopa-github-bot · 2020-09-25T15:42:50Z

	Warnings
⚠️	This PR changes a total of 430 LOCs, that is more than a reasonable size of 250. Consider splitting the pull request into smaller ones.

Affected stories

⚙️ #174710289: Ottimizzazioni sicurezza login SPID

Generated by 🚫 dangerJS against 7e7ff8e

codecov-commenter · 2020-09-25T15:43:14Z

Codecov Report

Merging #49 into master will increase coverage by 0.75%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   65.05%   65.81%   +0.75%     
==========================================
  Files          11       11              
  Lines         724      740      +16     
  Branches      120      122       +2     
==========================================
+ Hits          471      487      +16     
  Misses        252      252              
  Partials        1        1

Impacted Files	Coverage Δ
src/index.ts	`59.42% <ø> (ø)`
src/strategy/spid.ts	`36.95% <100.00%> (ø)`
src/utils/saml.ts	`57.75% <100.00%> (+1.76%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5da249b...7e7ff8e. Read the comment docs.

gunzip · 2020-09-25T15:51:35Z

src/strategy/spid.ts

@@ -60,7 +60,7 @@ export class SpidStrategy extends SamlStrategy {
    super(options, verify);
    if (!options.requestIdExpirationPeriodMs) {
      // 1 hour
-      options.requestIdExpirationPeriodMs = 3600 * 1000;
+      options.requestIdExpirationPeriodMs = 3600 * 1000; // TODO: Change the default value or set it into io-backend?


that would be better, probably keeping a sane default here

Fixed in 3198ea4

gunzip · 2020-09-25T15:53:32Z

src/utils/saml.ts

+          fromOption(new Error("Assertion element must be present"))(
+            fromNullable(
+              _1.Response.getElementsByTagNameNS(
+                SAML_NAMESPACE.ASSERTION,
+                "Assertion"
+              ).item(0)
+            )


isn't this implied by the previous check?

This code is only for type check. We need a cast like .item(0) as Element to avoid the fromNullable

can you add this as a comment?

I've changed the predicate to discriminate 2 different errors:

multiple Responses or Assertions

missing Response or Assertion

Fixed in a729df3

gunzip · 2020-09-25T15:54:31Z

src/utils/saml.ts

+        const signatureOfResponseCount = (xpath.select(
+          "count(//*[local-name(.)='Response']/*[local-name(.)='Signature'])",
+          doc


can we use DOM parser and remove xpath dependency instead?

Yes, but the code will be a little more complex

const signatureOfResponseCount = _.Response.getElementsByTagNameNS(SAML_NAMESPACE.XMLDSIG, "Signature") .length - _.Assertion.getElementsByTagNameNS( SAML_NAMESPACE.XMLDSIG, "Signature" ).length;

yes that's right, we must chose one between the two approaches and try to not mix them.
(if I'd had to start again I'd rather have used https://www.npmjs.com/package/xml2js instead of xpath / dom :-)

Fixed in a729df3

gunzip · 2020-09-25T15:55:25Z

src/utils/saml.ts

+      error => {
+        if (telemetryClient) {
+          telemetryClient.trackEvent({
+            name: "backend.login.prevalidate",


Suggested change

name: "backend.login.prevalidate",

name: "spid.error.generic",

Fixed in a729df3

gunzip · 2020-09-25T15:56:11Z

src/utils/saml.ts

+        ) as unknown) as number;
+        if (telemetryClient && signatureOfResponseCount === 0) {
+          telemetryClient.trackEvent({
+            name: "backend.login.prevalidate",


Suggested change

name: "backend.login.prevalidate",

name: "spid.error.signature",

Fixed in a729df3

gunzip · 2020-09-25T15:57:06Z

src/index.ts

-        getPreValidateResponse(serviceProviderConfig.strictResponseValidation),
+        getPreValidateResponse(
+          serviceProviderConfig.strictResponseValidation,
+          appConfig.applicationInsights


can we take the AI client from the backend? (so we will share the configuration)

Yes, the appConfig comes from the backend (provided inside the withSpid method)

I've thought about this a little bit. Since this may be a general purpose library used by other projects as well (there're no parts tied to the IO platform), it's better to do not introduce any deps to AI (which is Azure specific). That would be better to pass some track({eventName, type: "error"|"event", data: any }) method from the backend that uses its own instance of AI.

Fixed in a729df3

…issing elements

gunzip · 2020-09-29T16:55:54Z

src/utils/saml.ts

+            type: "INFO"
+          });
+        }
+        return callback(null, true, _.InResponseTo);


signatureOfResponseCount === 0 is considered a successulf case?

Yes, the Signature of the Response element is optional (link), we just log that we have received a SAML response without a signature.

why do we check response#signature - assertion#signature ? can you add some comments?

Fixed with a comment in 7e7ff8e

gunzip · 2020-09-30T13:14:16Z

src/strategy/spid.ts

@@ -59,8 +59,8 @@ export class SpidStrategy extends SamlStrategy {
  ) {
    super(options, verify);
    if (!options.requestIdExpirationPeriodMs) {
-      // 1 hour
-      options.requestIdExpirationPeriodMs = 3600 * 1000;
+      // 5 minutes


after a quick chat with @pp-ps we agreed to put this to 15 minutes

Fixed in 7e7ff8e

gunzip · 2020-09-30T13:18:13Z

src/utils/saml.ts

+            type: "INFO"
+          });
+        }
+        return callback(null, true, _.InResponseTo);


why do we check response#signature - assertion#signature ? can you add some comments?

…ments

[#174710289] Multiple Responses, multiple Assertions, events for traking

a49eaeb

[#174710289] Fix linter

b0939cd

BurnedMarshal requested a review from gunzip September 25, 2020 15:48

gunzip reviewed Sep 25, 2020

View reviewed changes

BurnedMarshal added 2 commits September 28, 2020 12:25

[#174710289] Change default Request expiration from 1h to 5 minutes

3198ea4

[#174710289] Agnostic event tracker, remove xpath, better loggin on m…

a729df3

…issing elements

BurnedMarshal marked this pull request as ready for review September 28, 2020 10:32

BurnedMarshal requested a review from cloudify as a code owner September 28, 2020 10:32

BurnedMarshal requested a review from gunzip September 28, 2020 10:32

[#174710289] rename mockEventHandler to mockEventTracker

c0cc6f7

BurnedMarshal mentioned this pull request Sep 28, 2020

[#174710289] Add SPID/CIE events tracking pagopa/io-backend#715

Merged

gunzip reviewed Sep 29, 2020

View reviewed changes

gunzip reviewed Sep 30, 2020

View reviewed changes

[#174710289] Change default request expiration to 15 minutes, add com…

7e7ff8e

…ments

gunzip merged commit 17c84ff into master Sep 30, 2020

gunzip deleted the 174710289-better-assertion-validation branch September 30, 2020 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#174710289] Better SAML Response validation #49

[#174710289] Better SAML Response validation #49

BurnedMarshal commented Sep 25, 2020

pagopa-github-bot commented Sep 25, 2020 •

edited

Loading

codecov-commenter commented Sep 25, 2020 •

edited

Loading

gunzip Sep 25, 2020

BurnedMarshal Sep 28, 2020

gunzip Sep 25, 2020

BurnedMarshal Sep 25, 2020

gunzip Sep 25, 2020

BurnedMarshal Sep 28, 2020

gunzip Sep 25, 2020

BurnedMarshal Sep 25, 2020

gunzip Sep 25, 2020 •

edited

Loading

BurnedMarshal Sep 28, 2020

gunzip Sep 25, 2020

BurnedMarshal Sep 28, 2020

gunzip Sep 25, 2020

BurnedMarshal Sep 28, 2020

gunzip Sep 25, 2020

BurnedMarshal Sep 25, 2020

gunzip Sep 25, 2020 •

edited

Loading

BurnedMarshal Sep 28, 2020

gunzip Sep 29, 2020

BurnedMarshal Sep 30, 2020

gunzip Sep 30, 2020

BurnedMarshal Sep 30, 2020

gunzip Sep 30, 2020

BurnedMarshal Sep 30, 2020

gunzip Sep 30, 2020

	name: "backend.login.prevalidate",
	name: "spid.error.generic",

	name: "backend.login.prevalidate",
	name: "spid.error.signature",

[#174710289] Better SAML Response validation #49

[#174710289] Better SAML Response validation #49

Conversation

BurnedMarshal commented Sep 25, 2020

pagopa-github-bot commented Sep 25, 2020 • edited Loading

Affected stories

codecov-commenter commented Sep 25, 2020 • edited Loading

Codecov Report

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

gunzip Sep 25, 2020 • edited Loading

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

gunzip Sep 25, 2020 • edited Loading

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

pagopa-github-bot commented Sep 25, 2020 •

edited

Loading

codecov-commenter commented Sep 25, 2020 •

edited

Loading

gunzip Sep 25, 2020 •

edited

Loading

gunzip Sep 25, 2020 •

edited

Loading