[BUG] Single-line privateKey value works OK for (default) HTTP-Redirect, but not for HTTP-POST binding

[oliverlockwood]

I had SAML working successfully, including signing, with the default HTTP Redirect binding.

Then I made a single change, to set authnRequestBinding to HTTP-POST, and suddenly the system threw an error because of "no start line".

Error: error:0909006C:PEM routines:get_name:no start line
    at Sign.sign (node:internal/crypto/sig:131:29)
    at RSASHA256.getSignature (/opt/my-service/bin/node_modules/xml-crypto/lib/signed-xml.js:128:22)
    at SignedXml.calculateSignatureValue (/opt/my-service/bin/node_modules/xml-crypto/lib/signed-xml.js:469:32)
    at SignedXml.computeSignature (/opt/my-service/bin/node_modules/xml-crypto/lib/signed-xml.js:834:10)
    at signXml (/opt/my-service/bin/node_modules/passport-saml/lib/node-saml/xml.js:107:9)
    at signSamlPost (/opt/my-service/bin/node_modules/passport-saml/lib/node-saml/saml-post-signing.js:8:30)
    at signAuthnRequestPost (/opt/my-service/bin/node_modules/passport-saml/lib/node-saml/saml-post-signing.js:12:12)
    at SAML.generateAuthorizeRequestAsync (/opt/my-service/bin/node_modules/passport-saml/lib/node-saml/saml.js:276:74)
    at SAML.getAuthorizeFormAsync (/opt/my-service/bin/node_modules/passport-saml/lib/node-saml/saml.js:430:36)
    at login-request (/opt/my-service/bin/node_modules/passport-saml/lib/passport-saml/strategy.js:100:59)
    at MultiSamlStrategy.authenticate (/opt/my-service/bin/node_modules/passport-saml/lib/passport-saml/strategy.js:129:13)
    at /opt/my-service/bin/node_modules/passport-saml/lib/passport-saml/multiSamlStrategy.js:28:32
    at PassportSamlConfig.fetchConfig (/opt/my-service/bin/config/passport-saml-config.js:141:16)
    at async Object.getSamlOptions (/opt/my-service/bin/config/passport-saml-config.js:113:50

code: ERR_OSSL_PEM_NO_START_LINE

My passport-saml configuration (with redactions) looks like this:

{
  "callbackUrl": "http://redacted.ourhost.com/login/saml/SSO",
  "protocol": "http://",
  "entryPoint": "https://dev-redacted.oktapreview.com/app/redacted/redacted/sso/saml",
  "issuer": "urn:redacted:redacted:redacted-app",
  "cert": "MIIDpD...REDACTED...qHzQmf",
  "privateKey": "MIIEvQ...REDACTED...gUdJ/qk=",
  "signatureAlgorithm": "sha256",
  "digestAlgorithm": "sha256",
  "maxAssertionAgeMs": 2592000000,
  "authnRequestBinding": "HTTP-POST",
  "logoutUrl": "https://dev-redacted.oktapreview.com/app/redacted/redacted/slo/saml"
}

If I remove authnRequestBinding, it works fine again.
If I keep the authnRequestBinding and contrive my online private key into a multiline one, it works fine again.

I think that either:

1. this should be fixed, or
2. at very least, https://github.com/node-saml/passport-saml#security-and-signatures should be updated to say that single-line private keys cannot be used if authnRequestBinding is set to HTTP-POST.

[markstos]

Please contribute a failing test to the the test suite that confirms this bug.

[oliverkane]

Just an FYI, I just encountered this bug as well, and would like to chime in. I was going to open up an issue, but see this is here already.

Single-line keys and certs work just fine for signingCert and privateKey but do NOT work for decryptionCert and decryptionPvk

[cjbarth]

We are open to PRs to address this problem. As mentioned above, a good place to start would be a PR with a suite of failing tests. Then, others can help address the problem, should you need such help.

[djbrown78]

same issue here, single line decryptionPvk works fine with 3.2.0, but 3.2.1 returns error:0909006C:PEM routines:get_name:no start line

[markstos]

@djbrown78 We are open to PRs to address this problem. As mentioned above, a good place to start would be a PR with a suite of failing tests.

You mentioned the issue appeared for you between 3.2.0 and 3.2.1. Here's the diff between those two versions. Not much changed. v3.2.0...v3.2.1

If an issue was really introduced there, it seems likely that it was triggered by changes in the xml-encryption library. A new test for the test suite that triggers the issue would confirm for certain that the issue appeared between these versions.

[christian-hawk]

additional information:
xml-encryption replaced node-forge module with bundled crypto module in 2.0.0.

crypto module don't accept string/buffer from an "online / single line" cert, only multiline cert, including header/footer. I opened a more detailed issue here: auth0/node-xml-encryption#99

[cjbarth]

Since these problems are apparently well documented, let's get some tests so we are ready for any fix that may come along. I don't see much movement over at node-xml-encryption, but there are others that are having issues with certs: node-saml/node-saml#206

[cjbarth]

This looks like it might be related to #524

[christian-hawk]

@cjbarth given that it may be understood as an undocumented/unrealized BREAKING CHANGE from node-xml-encryption when bumped crypto's major version, it caused one of the following:

• A breaking change in passport-saml: in case passport-saml want's to follow the dependency breaking change, and stop accepting "online / single line" certs, only multiline.
• A bug in passport-saml that will keep it features without breaking old code: create a proper adapter and data transformer to use the dependency accordingly (i.e. transform "online / single line cert" into multiline cert accepted by dependency.

Also, if xml-encryption looks unmaintained, should passport-saml keep using it?

Looking forward your (and community) thoughts.

[cjbarth]

The various XML packages that node-saml uses, along with the rest of the NodeJS/NPM auth community uses are mostly unmaintained. It would be nice if we could get enough developer support behind xml-crypto and xmo-encryption to get them properly maintained, but most people just want to use the code and move on. (Open source, doesn't mean free, it means the cost is contributing developer hours to the project instead of an outright payment, and most employers neglect this.)

You might have a look at node-saml/xml-crypto#267 to see some discussion around this issue.

[markstos]

@christian-hawk I think it's fair to treat this as a bug in passport-saml. We can adapt the cert format to match the format expected by our dependency.

[markstos]

Also, maybe we should look again at using Github Sponsors or some other mechanism to get some financial support behind the maintenance of these modules important for web security.

[christian-hawk]

@christian-hawk I think it's fair to treat this as a bug in passport-saml. We can adapt the cert format to match the format expected by our dependency.

@markstos this fix should be implemented in node-saml repo, correct?

[cjbarth]

@markstos this fix should be implemented in node-saml repo, correct?

I'd actually like to see just a test added to make sure that our dependent libraries keep doing what they should. If they break something, then we can take it up with them, and we'll know as soon as we try to take the broken dependently and can therefore avoid it. I don't want to take on more code than necessary since we then have to maintain it.

[markstos]

Good point @cjbarth about keeping our surface area smaller.