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

Add Native SSL Support #1027

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mdconaway
Copy link

@mdconaway mdconaway commented May 18, 2022

Closes #1024
-Add native SSL support
-Add deprecation warning for missing host value (no behavior change)
-Add self-signed certificate factory
-Add certificate file loaders
-Add prettier formatter for server code npm run formatserver
-Formatted server code
-Update README

@saubyk
Copy link
Collaborator

saubyk commented May 23, 2022

Hi @mdconaway this is a great PR, thanks for all the effort you have put on it so far. Having SSL support natively (as an option) is a great addition to RTL. Although, there is an issue with self signed certificates for some browsers e.g. Chrome where the app will not work at all (I am not considering that a big deal though).

Nonetheless, we can merge the change as long as it is an optional configuration, which the user can enable and we keep it OFF by default. As we need to keep in mind that there are vendor integrations for RTL, where https access is enabled via reverse proxy setup and the default should always be ssl as OFF or we will end up breaking such integrations.

As far as the review of the PR is concerned, I would request to limit the changes to SSL only and roll back the rest e.g. the formatted server code and prettier formatter etc. This would enable us to do a thorough review of the relevant changes only.

Looking forward to review and work on this PR with you. Thanks.

@mdconaway
Copy link
Author

Yeah, I can do that.

In this pull request the default value for SSL is definitely false. Regarding Chrome, I am currently able to use this RTL SSL feature branch with self signed certificates in the browser pretty much anywhere on my LAN, a user just has to click "proceed" through the warning messages. Ideally, users would generate their own CA and certs for their LAN/WAN and use the other config options for this enhancement like key, cert, and ca. (These options are absolute file paths to find the key files) Using actual key files also allows x509 client certificates to be requested to have a fully verified SSL tunnel.

This pull request also adds a deprecation warning on server boot if a user has not added a "host" value to their config. You guys can leave the warning as long as you'd like, and someday eventually change the host binding default when you feel sufficient time has passed. This is mainly a security related warning, but does not change any default configs or behavior. I added this deprecation warning because it occurs approximately where the ssl certs are loaded into the server.

While working on this pull I didn't see any linting script/config to check the server/backend code. I could easily include the addition of prettier as just a package.json update, then you guys can run npm run formatserver on your end in order to trust the server linting / formatting. Essentially the server formatter setup is just 2 package.json lines. The prettier changes are fully confined to package.json.

-Add native SSL support
-Add deprecation warning for missing host value
-Add self-signed certificate factory
-Add prettier formatter to package.json
-Update README
@mdconaway mdconaway force-pushed the feature-security-options branch from a86d720 to 021f4e0 Compare May 24, 2022 00:03
@mdconaway
Copy link
Author

Just force pushed an update that matches the conversation above. Let me know if that is sufficient!

@ShahanaFarooqui
Copy link
Collaborator

@saubyk The PR is blocked due to your decision on adding this support.

BND0027

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security by Default / SSL Options
4 participants