Feature: Enable self-signed certificates#1973
Feature: Enable self-signed certificates#1973blitztide wants to merge 2 commits intonovnc:masterfrom
Conversation
This will use OpenSSL and generate a new certificate, then start the server in SSL-Only mode
|
Nice idea, I'll take a look! |
| echo " Default: 6080 (on all interfaces)" | ||
| echo " --vnc VNC_HOST:PORT VNC server host:port proxy target" | ||
| echo " Default: localhost:5900" | ||
| echo " --self-sign hostname Generate self-signed certificates for hostname" |
There was a problem hiding this comment.
Wouldn't it be better to not force users to provide the hostname argument here? If they care enough, they will probably generate a more detailed certificate.
There was a problem hiding this comment.
What would you suggest for the default value?
There was a problem hiding this comment.
I was using it to point to something behind a reverse proxy, for testing, so hostname is pretty critical for this.
Once fully set up, then a proper SSL would be generated.
There was a problem hiding this comment.
I would suggest something equally generic to the other properties like CN=noVNC-host or perhaps CN=${HOSTNAME} if that environment variable is set.
I was using it to point to something behind a reverse proxy, for testing, so hostname is pretty critical for this.
Once fully set up, then a proper SSL would be generated.
Why is CN in the certificate important in this case?
|
Tested changes, ensuring argument compatibility |
| if [ $? != 0 ]; then | ||
| echo "Unable to find OpenSSL, please ensure you have OpenSSL installed and available in \$PATH" | ||
| exit 1 | ||
| fi | ||
| # Check that the file doesn't already exist | ||
| if [ -f $(pwd)/self.pem ]; then | ||
| read -p "$(pwd)/self.pem aleady exists, overwrite? (Y/N) " overwrite | ||
| if [ "$overwrite" != "Y" ]; then | ||
| echo "Not overwriting $(pwd)/self.pem" | ||
| exit 1 | ||
| fi | ||
| fi | ||
| echo "Generating Certificate for: ${SELF_SIGN}" | ||
| openssl req -x509 -newkey rsa:4096 -keyout key.pem -out self.pem -sha256 -days 3650 -nodes -subj "/C=XX/ST=NoVNC/L=NoVNC/O=NoVNC/OU=NoVNC/CN=${SELF_SIGN}" | ||
|
|
There was a problem hiding this comment.
Wouldn't it be more convenient if --self-sign only created a certificate if one was not found and echoed some info otherwise, Skip generating self-signed certificate. $(pwd)/self.pem and $(pwd)/key.pem already exists ? This way you could just manually delete the old files if you want to regenerate a cert.
Right now, if I e.g. run novnc_proxy --self-sign as a part of a larger script, I would have to manage the input to read on each subsequent run.
I know this maybe reverts parts of what you added here; sorry about that.
There was a problem hiding this comment.
--self-signed is more likely to be used in a test scenario.
If you are running scripts to spawn something, then certificate management will also be used in the higher level script.
There was a problem hiding this comment.
I hear you, I still think forcing users to make a choice every time they run with --self-sign adds unnecessary inconvenience. It's more likely that they will just want to continue where they left off on subsequent runs.
| fi | ||
| fi | ||
| echo "Generating Certificate for: ${SELF_SIGN}" | ||
| openssl req -x509 -newkey rsa:4096 -keyout key.pem -out self.pem -sha256 -days 3650 -nodes -subj "/C=XX/ST=NoVNC/L=NoVNC/O=NoVNC/OU=NoVNC/CN=${SELF_SIGN}" |
There was a problem hiding this comment.
key.pem should be called self.key since key.pem might as well be the key for a valid certificate.
| exit 1 | ||
| fi | ||
| # Check that the file doesn't already exist | ||
| if [ -f $(pwd)/self.pem ]; then |
There was a problem hiding this comment.
You should probably check for the existence of all files that are generated by your OpenSSL command.
There was a problem hiding this comment.
It's also allowed to set -keyout self.pem so that both cert and pubkey end up in the same file. Would probably simplify existence checks a bit.
2 participants
This will use OpenSSL and generate a new certificate, then start the server in SSL-Only mode