-
Notifications
You must be signed in to change notification settings - Fork 681
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
Add SSL CA bundle option #1024
Add SSL CA bundle option #1024
Conversation
src/server.js
Outdated
key: fs.readFileSync(keyPath), | ||
cert: fs.readFileSync(certPath) | ||
}, app); | ||
if (config.https.ca.length && fs.existsSync(caPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic doesn't seem right.
I'm assuming you mean (config.https.ca.length && !fs.existsSync(caPath))
Also, you'd probably want to do "caPath" rather than "config.https.ca", yeah?
src/server.js
Outdated
log.error("Path to SSL ca bundle is invalid. Stopping server..."); | ||
process.exit(); | ||
} | ||
if (config.https.ca.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be dreadful to restructure this to do:
server = server.createServer({
key: fs.readFileSync(keyPath),
cert: fs.readFileSync(certPath),
ca: caPath ? fs.readFileSync(caPath) : undefined
}, app);
That should? work exactly the same (test it), but it also removes the duplicate code, and looks much nicer, imo.
Changed as requested, can test it on nightime. |
For my case, its working. |
src/server.js
Outdated
@@ -53,9 +54,14 @@ module.exports = function() { | |||
log.error("Path to SSL certificate is invalid. Stopping server..."); | |||
process.exit(); | |||
} | |||
if (config.https.ca.length && !fs.existsSync(caPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change config.https.ca.length
to caPath.length
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Idea is to check if configuration parameter is wrongly set. That means parameter exists and file is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caPath is just config.https.ca with normalised "~" is it not? So what's the need to use config.https.ca? That's extra characters. If config.https.ca is "", then caPath will be "".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, its easier to understand what is config.https.ca
then Helper.expandHome(config.https.ca)
. Speciel when same style is applied on config.https.key
and config.https.certificate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those would be changed as well (not as part of your pr). You created the variable, so it should be used here. It's confusing not using it because it leads one to wonder why you aren't using it, and the only reason is because that's what we do with the others.
Changed, I could change other ssl file paths too, so they use same style. |
src/server.js
Outdated
@@ -53,9 +54,14 @@ module.exports = function() { | |||
log.error("Path to SSL certificate is invalid. Stopping server..."); | |||
process.exit(); | |||
} | |||
if (caPath.length && !fs.existsSync(caPath)) { | |||
log.error("Path to SSL ca bundle is invalid. Stopping server..."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CA bundle is not required to run TLS, don't kill the server if there isn't one provided. This whole if
isn't even required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the point of this is just that if you specify a ca, but it doesn't exist...that's an error.
If you don't specify a ca, then it'll go through fine. But we don't want to pass through nonsense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh whoops, yeah, that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, good addition, thanks for this.
Squash the commits and then 👍 |
Also @metsjeesus, mind rebasing with |
Thanks! |
Add SSL CA bundle option
Hey @metsjeesus, we have sticker packs for our contributors now! |
Letsencrypt serts give me errors on firefox.
ERR_CERT_AUTHORITY_INVALID
, to fix it, it needs a CA bundle file. Currently, lounge does not accept this, so i made one.