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

Check if SSL key and certificate files exist #673

Merged
merged 1 commit into from
Oct 8, 2016

Conversation

toXel
Copy link
Contributor

@toXel toXel commented Oct 4, 2016

It's a first attempt. Please suggest changes.
This fixes #416.

key: fs.readFileSync(keyPath),
cert: fs.readFileSync(certPath)
}, app).listen(config.port, config.host);
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer two separate messages for each of the files. And do a negative check, then you dont need an else.

@xPaw
Copy link
Member

xPaw commented Oct 5, 2016

Just wondering, does it explode if the file exists but it's not valid? If so might want to add an extra check for that.

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Oct 5, 2016
@xPaw xPaw added this to the 2.1.0 milestone Oct 5, 2016
@toXel
Copy link
Contributor Author

toXel commented Oct 5, 2016

Good idea. I'll look into this.

@astorije
Copy link
Member

astorije commented Oct 6, 2016

@xPaw, in #416, @toXel asked if we preferred an if condition or a try/catch block. Do you have an opinion on this? I don't really have one myself.

@toXel
Copy link
Contributor Author

toXel commented Oct 6, 2016

@xPaw yes it crashes if the files are not valid:

_tls_common.js:68
      c.context.setCert(options.cert);
                ^

Error: error:0906D06C:PEM routines:PEM_read_bio:no start line

I could add a try/catch block around

server = server.createServer({
    key: fs.readFileSync(keyPath),
    cert: fs.readFileSync(certPath)
}, app).listen(config.port, config.host);

to catch it and stop the server with a better message.

But then if the server couldn't be created because of some other error, it will give the user a wrong error message...

Copy link
Member

@xPaw xPaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me, let it crash on invalid certs, unless node has a function to validate them (which I doubt)

@toXel
Copy link
Contributor Author

toXel commented Oct 7, 2016

@xPaw Nope I haven't found a function for it. There's a pem npm module with a readCertificateInfo function. But I think this will also crash on invalid files.

@xPaw xPaw self-assigned this Oct 7, 2016
@@ -36,9 +36,19 @@ module.exports = function() {
server = server.createServer(app).listen(config.port, config.host);
} else {
server = require("spdy");
let keyPath = Helper.expandHome(config.https.key);
let certPath = Helper.expandHome(config.https.certificate);
Copy link
Member

@astorije astorije Oct 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change these from let to const please?

@astorije
Copy link
Member

astorije commented Oct 8, 2016

After addressing my comment, can you squash your PR as well please? :)

@toXel toXel force-pushed the toxel/fix-missing-keys branch from 0fee528 to 5b6f5d5 Compare October 8, 2016 12:56
@toXel
Copy link
Contributor Author

toXel commented Oct 8, 2016

Done :)

Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @toXel, thanks!

@astorije astorije merged commit 45ff1c0 into thelounge:master Oct 8, 2016
@toXel toXel deleted the toxel/fix-missing-keys branch October 8, 2016 20:54
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Check if SSL key and certificate files exist
@xPaw xPaw removed their assignment Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check if ssl key files exist instead of crashing with a cryptic error
3 participants