Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uncatchable Exception Thrown when bad privateCert used #230

Closed
chriswininger opened this issue Aug 22, 2017 · 7 comments
Closed

Uncatchable Exception Thrown when bad privateCert used #230

chriswininger opened this issue Aug 22, 2017 · 7 comments

Comments

@chriswininger
Copy link
Contributor

When the authenticate method is used with an incorrect value passed for privateCert the call to self.signRequest(samlMessage); throws the exception error:0906D06C:PEM routines:PEM_read_bio:no start line.

When this happens rather than triggering a 500 error response the server crashes. Here is a simple app which shows this

const express = require('express');
const app = express();
const passport = require('passport');
const SAMLStrategy = require('passport-saml').Strategy;

const options = {
    "path": "/login/callback",
    "entryPoint": "https://www.example.com",
    "callbackUrl": "https://example.com/login/callback",
    "protocol": "https://",
    "privateCert": "-----BEGIN CERTIFICATE-----\n"+
	"8mvhvrcCOiJ3mjgKNN1F31jOBJuZNmq0U7n9v+Z+3NfyU/0E9jkrnFvm5ks+p8kl\n" +
	"BjuBk9RAkazsU9l02XMS/VxOOIifxKC7R9bDtx0hjolYxgqxPIO5s4rmjj0rLzvo\n" +
	"vQTTTx/tB5e+hbdx922QSeTjP4DO4ms6cIexcH+ZEUOJ3wXiHToJW83SXLRtwPI9\n" +
	"JbWKeS9nWPnzcedbDNZkGtohW5vf32BHuvLsWcl6eFXRSkdX/7+rgpXmDRB7caQ+\n" +
	"2SXVY7ORily7LTKg1cFmuKHDzKTGFIp5/GU6dwIDAQABAoIBAArgFQ+Uk4UN4diY\n" +
	"gJWCAaQlTVmP0UEHZQt/NmJrc9ZVduuhOP0hH6gF53nREHz5UQb4nXB2Ksa3MtYD\n" +
	"Z1vhJcu/T7pvmib4q+Ij6oAmlyeL/xwVY3IUURMxX3tCdPItlk4PEFELKeqQOiIS\n" +
	"7B0DYxWfJbMle3c95w5ruYEr2A+fHCKVSlDpg7uPd9VQ6t7bGMZZvc9tDSC1qPXQ\n" +
	"Gd/WOMXxi+t/TpyVZ6tOcEekQzAMLmWElUUPx3TJ0ur0Zl2LZ7IvQEXXias4lUHV\n" +
	"fnH3akDCMmdhlJSVqUfplrh85zAOh6fLloZagphj/Kpgfw1TZ+njSDYqSLYE0NZ1\n" +
	"j+83feECgYEA2aNGgbc+t6QLrJJ63l9Mz541lVV3IUAxZ5ACqOnMkQVuLoa5IMwM\n" +
	"oENIo38ptfHQqjQ9x8/tEINFqOHnQuOJ/+1xP9f0Me+0clRDCqjGYqNYgmakKyD7\n" +
	"vey/q6kwHk679RVGiI1p+HdoA+CbEKWHJiRxE0RhAA3G3wGAq7kpJocCgYEAxp4/\n" +
	"tCft+eHVRivspfDN//axc2TR6qWP9E1ueGvbiXPXv0Puag0W9cER/df/s5jW4Rqg\n" +
	"CE8649HPUZ0FJT+YaeKgu2Sw9SMcGl4/uyHzg7KnXIeYyQZJPqQkKyXmIix8cw3+\n" +
	"HBGRtwX5nOy0DgFdaMiH0F08peNI9QHKKTBoWJECgYEAyymJ1ekzWMaAR1Zt8EvS\n" +
	"LjWoG4EuthFwjRZ4BSpLVk1Vb4VAKAeS+cAVfNpmG3xip6Ag0/ebe0CvtFk9QsmZ\n" +
	"txj2EP0M7div/9H8y2SF3OpS41fhhIlDtyXcPuivDHu/Jaf4sdwgwlrk9EmlN0Lu\n" +
	"CIMYMz4vtpclwGNss+EjMt0CgYEAqepD0Vm/iuCaVhfJsgSaFvnywSdlNfpBdtyv\n" +
	"PzH2dFa4IZZ55hwgoklznNgmlnyQh68BbVpqpO+fDtDnz//h4ePRYb84a96Hcj9j\n" +
	"AjJ/YxF5f/04xfEsw/wkPQ2FHYM1TDCSTWzyXcMs0gTl3H1qbfPvzF+XPMt+ZKwN\n" +
	"SMNy4SECgYB3ig6t+XVfNkw8oBOh0Gx37XKbmImXsA8ucDAX9KUbMIvD03XCEf34\n" +
	"jF3SNJh0SmHoT62vc+cJqPxMDP6E7Q1nZxsEyaAkKr2H4dSM4SlRm0VB+bS+jXsz\n" +
	"PCiRGSm8eupuxfix05LMMreo4mC7e3Ir4JhdCsXxAMZIvbNyXcvUMA==\n" +
	"-----END CERTIFICATE-----\n",
   };

const strategy = new SAMLStrategy(options, function(profile, done) {});
passport.use('saml', strategy);
app.get('/test', passport.authenticate('saml', { failureRedirect: '/', failureFlash: true }), function(req, res) {
	console.log('!!! done');
	res.redirect('/');
});

app.listen(3000);
console.log('listening on port: 3000');

It would be nice to handle this more gracefully. Let me know what you think. Thanks much.

@chriswininger
Copy link
Contributor Author

chriswininger commented Aug 22, 2017

I went ahead and made a pull request (#231) with a simple change that will catch the error and pass it back via the callback instead of throwing the exception. I also added a test with the commit which shows the issue. If this is something you can use then please feel free to pull it in. If there's anything I can do to make the commit better let me know. Thanks again.

@ilmatic
Copy link

ilmatic commented Sep 6, 2017

@chriswininger Having issues with privateCert, mine looks pretty much like the one in your example above

const PRIVATE_CERT = `-----BEGIN CERTIFICATE-----
abcd
efgh
etcetc
-----END CERTIFICATE-----
`;

What is it about this format that makes it a bad private cert? It appears to have a start line, why is it throwing the PEM_read_bio:no start line error?

@cburatto
Copy link

I just had the issue and it seemed that adding the BEGIN/END lines would solve it; however, since I was running a lot of tests, it could really have been an issue with the certificate itself.

It is important to accept this pull if it is working because this issue really crashes the server.

@markstos
Copy link
Contributor

For future reference, if you are having trouble figuring out the correct private key format, you can use this little script to test:

  "use strict";                                                                                                      
  var fs = require("fs")                                                                                                                                                                                                         
                                                                                                                                                                                                                                 
  var SAML = require("passport-saml").SAML;                                                                                                                                                                                      
                                                                                                                                                                                                                                 
  var saml = new SAML({                                                                                                                                                                                                          
      privateCert : fs.readFileSync("./tmp.pem")                                                                                                                                                                                 
  })                                                                                                                                                                                                                             
                                                                                                                                                                                                                            
  var message = { SAMLResponse: "BOOM" };                                                                                                                                                                                        
  saml.signRequest(message)                                                                                                                                                                                                      
console.log(message.Signature)    

Using that, I determined that the private key must start with -----BEGIN PRIVATE KEY----- on it's own line. (And -----END PRIVATE KEY----- on it's own line at the end for good measure.)

It doesn't work to put BEGIN PRIVATE KEY on the same line as the rest of the cert, and didn't work to omit it and didn't work to use BEGIN CERTIFICATE instead.

The certificate itself can be on a single line, or presumably will also work with linebreaks.

This behavior is inconsistent with other parts of passport-saml which accept certs on a single line without the header/footer. The need for the headers here is not currently documented-- I'll work on that.

It also be nice if passport-saml validated the cert formats as soon as they were loaded for faster feedback that something is wrong.

@markstos
Copy link
Contributor

Resolving, since #231 was merged.

cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
…-string-signing

* commit 'da829fc0216ed961ea7cb8a6234df65a60f51114':
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  Drop support for Node versions < 4.
  v0.20.2
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
…url-params

* commit 'da829fc0216ed961ea7cb8a6234df65a60f51114':
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  Drop support for Node versions < 4.
  v0.20.2
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
* master: (51 commits)
  start to use the debug module.
  v0.34.0: release
  internal: provide unique failure messages for invalid signatures. Fixes node-saml#146
  package.json: bump version to 0.33.0
  docs: mention that disableRequestAuthnContext helps with AD FS
  New Feature: allow customizing the name of the strategy.
  bump version to v0.32.1
  README: link to where our Changes are documented.
  Audience validation
  README: fix typo `s/ADSF/ADFS/`
  jshint: fix jshint violation.
  v0.31.0 release
  README: update link description for ADFS docs.
  Upd: Mention ADFS 2016 with NameIDFormatError. (node-saml#242)
  Support multiple and dynamic signing certificates (node-saml#218)
  v0.30.0
  Ignore .tern-port files
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  ...

# Conflicts:
#	README.md
#	test/samlTests.js
#	test/tests.js
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
* master: (51 commits)
  start to use the debug module.
  v0.34.0: release
  internal: provide unique failure messages for invalid signatures. Fixes node-saml#146
  package.json: bump version to 0.33.0
  docs: mention that disableRequestAuthnContext helps with AD FS
  New Feature: allow customizing the name of the strategy.
  bump version to v0.32.1
  README: link to where our Changes are documented.
  Audience validation
  README: fix typo `s/ADSF/ADFS/`
  jshint: fix jshint violation.
  v0.31.0 release
  README: update link description for ADFS docs.
  Upd: Mention ADFS 2016 with NameIDFormatError. (node-saml#242)
  Support multiple and dynamic signing certificates (node-saml#218)
  v0.30.0
  Ignore .tern-port files
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  ...

# Conflicts:
#	README.md
#	test/samlTests.js
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 11, 2018
…-authn-context

* commit 'da829fc0216ed961ea7cb8a6234df65a60f51114':
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  Drop support for Node versions < 4.
  v0.20.2
@ZoharLiran
Copy link

ZoharLiran commented Jun 24, 2019

I'm seeing a similar issue when my cert has the lines
-----BEGIN CERTIFICATE-----
-----END CERTIFICATE-----
I get the following error:
Error: error:0906D06C:PEM routines:PEM_read_bio:no start line

when I replace CERTIFICATE with PRIVATE KEY to get the lines:
-----BEGIN PRIVATE KEY-----
-----END PRIVATE KEY-----

I get the error:
Error: error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag

using the snippet you posted @markstos

any ideas?

@markstos
Copy link
Contributor

@ZoharLiran Are you using the most recent version of passport-saml? Check your cert with other certificate checking tools to confirm it valid and in the right format. This does not appear to be a bug in passport-saml, so I'm marking this as closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants