Support direct use of SSL on the HTTP auth/websocket #176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mofirouz
reviewed
Mar 21, 2018
| if len(sockConfig.SSLCertificate) > 0 && len(sockConfig.SSLPrivateKey) > 0 { | ||
| cer, err := tls.LoadX509KeyPair(sockConfig.SSLCertificate, sockConfig.SSLPrivateKey) | ||
| if err != nil { | ||
| a.logger.Error("Loading SSL certs failed", zap.Error(err)) |
There was a problem hiding this comment.
We'll need to make this into FATAL to ensure that the server doesn't start with invalid config values, aka fail fast, as this could make developers confused as to why SSL certs were given but unused. I'll make this code change on master.
There was a problem hiding this comment.
Ah OK, thanks. I wasn’t sure what the expected error level was and erred on the side of not stopping the startup but that makes sense.
mofirouz
approved these changes
Mar 21, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Presently Nakama requires the use of a separate SSL terminator such as a load balancer. This adds additional complexity, resource usage and cost - if you set up Load Balancer on Google Cloud Platform to do the job, it actually costs more than a small instance to run. You can set up Nginx locally to do it, but this is another moving part which needs configuring & maintaining, uses system resources, and which can potentially fail.
Small devs using Nakama in limited scenarios may want to keep things small and simple, taking the risk of running a single node cheaply (they couldn't run multiple nodes without Enterprise anyway) while still protecting user data by using SSL (you can use self-signed certificates securely, see related PR for Unity client heroiclabs/nakama-unity#60).
This PR allows you to configure an ssl cert/key combo for the Nakama server itself, meaning you can use the server directly instead of via an intermediary. This is clearly not as robust as using 3rd party proxy services to help with DDoS protection etc, but a dev running the server on a single node is already making the trade-off of resilience for simplicity / cost, so it's a valid usage scenario for some.
I'm prepared that this PR might not be accepted since it's not a recommended configuration, but @mofirouz encouraged me to submit it in case it's of use to someone anyway.