Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

feat(CA Bundle option) #1189

Closed
wants to merge 2 commits into from
Closed

Conversation

siteshackinc
Copy link

Added CA Bundle option for SSL Certs

@lirantal lirantal added this to the 0.5.0 milestone Feb 5, 2016
@lirantal lirantal self-assigned this Feb 5, 2016
@lirantal
Copy link
Member

lirantal commented Feb 5, 2016

HI @siteshackinc

Why is the CA bundle required? what happens if it's not there? which is the case now.

A couple of comments on my side:

  1. Is the file /config/sslcerts/cabundle.crt supposed to be created by the ssl generation scripts or should we be adding it as part of the repo?
  2. you need to update your commit messages (see the CONTRIBUTING.md file to see how to style them) and also squash your commits to 1

Thanks for contributing to the project.

@siteshackinc
Copy link
Author

If a person buys a Comodo SSL certificate for example, not all browsers will validate the crt, which is why they provide a CABundle.crt or mulple CA's to add to create a CABundle.crt. Adding the CABundle that a Certificate Authority provides (like Comodo), fixes that issue. I bought a Comodo cert, this was required to have it work correctly. So I thought I would share.

@codydaig
Copy link
Member

codydaig commented Feb 6, 2016

I'm not opposed to having it in there as a comment. But, the self-signed certs do not come with a bundle file, and since our instructions and scripts allow for self-assigned, this would break.

@siteshackinc
Copy link
Author

How would this break? I would love to hear about it. As the cabundle.crt wont break anything its just an added root ca file that accompanies some ssl providers certificates.

@siteshackinc
Copy link
Author

Tell you what codydaig, create a blank cabundle.crt file. Enable those options in the proper configs. create a self signed cert. Now show me how this breaks...

@lirantal
Copy link
Member

@siteshackinc what we mean is that if we try to just merge this PR it will break because we are telling the server to look for the cabundle.crt file but it doesn't really exist.

This will be the error that the user will see:

    Error: ENOENT, no such file or directory 'config/sslcerts/cabundle.crt'

I do however see the value in this PR and if we can put an empty file then I'm all up for it.
So can you please fix this PR so that:

  1. you will add an empty file at config/sslcert/cabundle.crt
  2. squash all of your commits to just a single commit (and make sure you follow the commit message guideline per the CONTRIBUTING.md)

@jloveland
Copy link
Contributor

What if you add a line in the generate-ssl-certs.sh to create an empty config/sslcerts/cabundle.crt?
https://github.com/meanjs/mean/blob/master/scripts/generate-ssl-certs.sh#L16

something like touch ./config/sslcerts/cabundle.crt

@ilanbiala
Copy link
Member

@siteshackinc @lirantal let's try to wrap this up. Can we get a consensus on the implementation and do a final review before merging? @siteshackinc make sure there is only 1 commit and it follows the commit message guidelines before we review so we can just merge it in right away if it looks good.

@lirantal
Copy link
Member

@siteshackinc let's add it to config/sslcerts/cabundle.crt as @jloveland suggested and squash the commits to 1 for this to be merged in.

@ilanbiala
Copy link
Member

@siteshackinc can you make the changes @lirantal requested ^^?

@lirantal
Copy link
Member

@ilanbiala @siteshackinc my PR for applying this fix is here: #1342

@lirantal lirantal closed this May 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants