Invalid cert tests break with Node >= 8.7.0 about passport-wsfed-saml2 HOT 6 CLOSED

I've investigated a bit. It's indeed the change in error message generation.

Currently the error message catch in saml.js:94 is:

Error: error:0D07209B:asn1 encoding routines:ASN1_get_object:too long
at Verify.verify (crypto.js:381:23)
at RSASHA256.verifySignature (D:\SC\GitHub\passport-wsfed-saml2\node_modules\xml-crypto\lib\signed-xml.js:138:24)
at SignedXml.validateSignatureValue (D:\SC\GitHub\passport-wsfed-saml2\node_modules\xml-crypto\lib\signed-xml.js:281:20)
at SignedXml.checkSignature (D:\SC\GitHub\passport-wsfed-saml2\node_modules\xml-crypto\lib\signed-xml.js:269:13)
at SAML.validateSignature (D:\SC\GitHub\passport-wsfed-saml2\lib\passport-wsfed-saml2\saml.js:93:17)
at SAML.validateSamlAssertion (D:\SC\GitHub\passport-wsfed-saml2\lib\passport-wsfed-saml2\saml.js:308:8)
at Context. (D:\SC\GitHub\passport-wsfed-saml2\test\saml20.tests.js:193:35)
at Test.Runnable.run (D:\SC\GitHub\passport-wsfed-saml2\node_modules\mocha\lib\runnable.js:196:15)
at Runner.runTest (D:\SC\GitHub\passport-wsfed-saml2\node_modules\mocha\lib\runner.js:344:10)
at D:\SC\GitHub\passport-wsfed-saml2\node_modules\mocha\lib\runner.js:390:12
at next (D:\SC\GitHub\passport-wsfed-saml2\node_modules\mocha\lib\runner.js:270:14)
at D:\SC\GitHub\passport-wsfed-saml2\node_modules\mocha\lib\runner.js:279:7
at next (D:\SC\GitHub\passport-wsfed-saml2\node_modules\mocha\lib\runner.js:227:23)
at Immediate. (D:\SC\GitHub\passport-wsfed-saml2\node_modules\mocha\lib\runner.js:247:5)
at runCallback (timers.js:794:20)
at tryOnImmediate (timers.js:752:5)
at processImmediate [as _immediateCallback] (timers.js:729:5)
opensslErrorStack:
[ 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib',
'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error',
'error:0D068066:asn1 encoding routines:ASN1_CHECK_TLEN:bad object header' ] }

Crypto.js file which throws the error is part of Node.js. In 8.11.1 there's following test related to the issue:

assert.throws(function() {
  // The correct header inside `test_bad_rsa_privkey.pem` should have been
  // -----BEGIN PRIVATE KEY----- and -----END PRIVATE KEY-----
  // instead of
  // -----BEGIN RSA PRIVATE KEY----- and -----END RSA PRIVATE KEY-----
  // It is generated in this way:
  //   $ openssl genrsa -out mykey.pem 512;
  //   $ openssl pkcs8 -topk8 -inform PEM -outform PEM -in mykey.pem \
  //     -out private_key.pem -nocrypt;
  //   Then open private_key.pem and change its header and footer.
  const sha1_privateKey = fixtures.readSync('test_bad_rsa_privkey.pem',
                                            'ascii');
  // this would inject errors onto OpenSSL's error stack
  crypto.createSign('sha1').sign(sha1_privateKey);
}, (err) => {
  // Throws crypto error, so there is an opensslErrorStack property.
  // The openSSL stack should have content.
  if ((err instanceof Error) &&
      /asn1 encoding routines:[^:]*:wrong tag/.test(err) &&
      err.opensslErrorStack !== undefined &&
      Array.isArray(err.opensslErrorStack) &&
      err.opensslErrorStack.length > 0) {
    return true;
  }
});

As we can see we have an array of errors thrown by OpenSSL embedded into exception.

opensslErrorStack:
[ 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib',
'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error',
'error:0D068066:asn1 encoding routines:ASN1_CHECK_TLEN:bad object header' ] }

I'm going to provide fix for this soon.

from passport-wsfed-saml2.

Ironically this test expects a failure but it's just not getting the right failure message. This might just be a change to how the underlying crypto library generates the message...

from passport-wsfed-saml2.

Current suspect is this commit in the Node repo: nodejs/node@1ed0c7706f

from passport-wsfed-saml2.

@mikeops Actually it might not be just issue of test, but the real bug. At company I work for we use passport-wsfed-saml2 strategy in the project. It has calculated the same thumbprint for two different certificates.

A bit off-topic question - is this strategy maintained? Should we contribute on our own or there's a chance it will be investigated by the community.

from passport-wsfed-saml2.

I'm going to provide fix for this soon.

@kalkusa Awesome for looking into this - thank you!

If you can reach out to @machuga when you do it'll be faster to get it merged. (He's a member of the team that's actively supporting this library.)

Thanks again.

from passport-wsfed-saml2.

FYI these are the breaking changes related to crypto node6 -> node8: https://github.com/nodejs/wiki-archive/blob/master/Breaking-changes-between-v6-LTS-and-v8-LTS.md#crypto

from passport-wsfed-saml2.