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

feat: add support for selecting SSL key type (ECDSA/RSA) #4218

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
e6ec74c
feat: add support for selecting SSL key type (ECDSA/RSA)
mnr73 Dec 9, 2024
8e9e033
fix indent: tab to space
mnr73 Dec 9, 2024
891877a
fix ssl key-type certificate
mnr73 Dec 11, 2024
2723de2
add ssl_ecdh_curve for more compatibility
mnr73 Dec 11, 2024
5e7b69c
add update cipher suites
mnr73 Dec 11, 2024
95a94a4
add elliptic-curve
mnr73 Dec 11, 2024
111fc28
Revert "add elliptic-curve"
mnr73 Dec 11, 2024
04b3608
remove elliptic-curve from certbot command options
mnr73 Dec 11, 2024
cb79556
add ssl_key_type in swagger
mnr73 Dec 12, 2024
eb5c51a
add support more cipher suites
mnr73 Dec 12, 2024
2e45444
change ssl_ciphers for more compatibility
mnr73 Dec 12, 2024
5ba7363
fix ssl cipher bug
mnr73 Dec 13, 2024
f386f6b
remove elliptic-curve
mnr73 Dec 13, 2024
32e0784
support more cipher suites
mnr73 Dec 21, 2024
f68c1b7
add Diffie-Hellman Parameters to cipher suites
mnr73 Dec 21, 2024
1353937
fix copy address
mnr73 Dec 21, 2024
04636b7
add feature: set default server
mnr73 Dec 21, 2024
5dc78df
fix messages indent: convert to space
mnr73 Dec 22, 2024
c6d884d
fix indent
mnr73 Dec 22, 2024
ad36fb5
show select ssl key type just for create new ssl
mnr73 Jan 1, 2025
65f971f
add migration names and combine ssl key migrations
mnr73 Jan 1, 2025
a121cb1
remove unnecessary whitespace
mnr73 Jan 1, 2025
d3a5fac
make ssl_key_type optional
mnr73 Jan 1, 2025
2cab405
Merge branch 'fix-bugs' into develop
mnr73 Jan 1, 2025
101afa0
remove default_server from certificate object
mnr73 Jan 3, 2025
408eab8
remove unesessary default values
mnr73 Jan 4, 2025
c135880
Revert "remove default_server from certificate object"
mnr73 Jan 4, 2025
f34cb59
Revert "remove unesessary default values"
mnr73 Jan 4, 2025
3856b6b
remove default server from certificate object
mnr73 Jan 8, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion backend/internal/certificate.js
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,7 @@ const internalCertificate = {
return internalCertificate.create(access, {
provider: 'letsencrypt',
domain_names: data.domain_names,
ssl_key_type: data.ssl_key_type,
meta: data.meta
});
},
Expand Down Expand Up @@ -832,6 +833,7 @@ const internalCertificate = {

const cmd = `${certbotCommand} certonly ` +
`--config '${letsencryptConfig}' ` +
`--key-type '${certificate.ssl_key_type}' ` +
'--work-dir "/tmp/letsencrypt-lib" ' +
'--logs-dir "/tmp/letsencrypt-log" ' +
`--cert-name "npm-${certificate.id}" ` +
Expand Down Expand Up @@ -873,6 +875,7 @@ const internalCertificate = {

let mainCmd = certbotCommand + ' certonly ' +
`--config '${letsencryptConfig}' ` +
`--key-type '${certificate.ssl_key_type}' ` +
'--work-dir "/tmp/letsencrypt-lib" ' +
'--logs-dir "/tmp/letsencrypt-log" ' +
`--cert-name 'npm-${certificate.id}' ` +
Expand Down Expand Up @@ -969,6 +972,7 @@ const internalCertificate = {

const cmd = certbotCommand + ' renew --force-renewal ' +
`--config '${letsencryptConfig}' ` +
`--key-type '${certificate.ssl_key_type}' ` +
'--work-dir "/tmp/letsencrypt-lib" ' +
'--logs-dir "/tmp/letsencrypt-log" ' +
`--cert-name 'npm-${certificate.id}' ` +
Expand Down Expand Up @@ -1002,6 +1006,7 @@ const internalCertificate = {

let mainCmd = certbotCommand + ' renew --force-renewal ' +
`--config "${letsencryptConfig}" ` +
`--key-type '${certificate.ssl_key_type}' ` +
'--work-dir "/tmp/letsencrypt-lib" ' +
'--logs-dir "/tmp/letsencrypt-log" ' +
`--cert-name 'npm-${certificate.id}' ` +
Expand Down Expand Up @@ -1032,9 +1037,10 @@ const internalCertificate = {
*/
revokeLetsEncryptSsl: (certificate, throw_errors) => {
logger.info('Revoking Let\'sEncrypt certificates for Cert #' + certificate.id + ': ' + certificate.domain_names.join(', '));

const mainCmd = certbotCommand + ' revoke ' +
`--config '${letsencryptConfig}' ` +
`--key-type '${certificate.ssl_key_type}' ` +
'--work-dir "/tmp/letsencrypt-lib" ' +
'--logs-dir "/tmp/letsencrypt-log" ' +
`--cert-path '/etc/letsencrypt/live/npm-${certificate.id}/fullchain.pem' ` +
Expand Down
26 changes: 25 additions & 1 deletion backend/internal/host.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,32 @@ const internalHost = {
}

return response;
}
},

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default server thing doesn't work. Here's some thoughts:

  • The file ./backend/templates/default.conf sets the default site already, so turning it on for any host always causes an error and makes it "Offline" even when passing your checkDefaultServerNotExist test below
  • The UI for this feature probably requires more review; having a switch on every host form isn't a great experience especially when trying to find out which host has it on already, as the error message doesn't tell you. I think the best place is to amend the existing Default Site setting form to allow selecting from one of the existing hosts as the default.
  • Remove it from this PR; if you still want to do it, create a new PR and keep this one focussed on the SSL certificate type

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that when I try to connect to the server with an IoT device, the connection fails. After some research, I found this command:

openssl s_client -connect :443

However, this command returns no response.

When I add a default server to one of the Nginx host configurations, everything works correctly. The above command returns a response, and the IoT device can connect to all the hosts configured in Nginx Proxy Manager.

so i add this feature and its work without any problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Well it was implemented by another contributor a long time ago, that the default HTTPS host returns a bad cipher/ssl cert or something like that. There was a very good reason for that at the time.

The default-site config doesn't apply to HTTPS though, since any certificate assigned to that would always be invalid for a catch-all domain.

Is there no other way you can fetch the ciphers?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the certificate for a default server would always be invalid. However, I haven't found any other solution. Even when I manually configured Nginx (before switching to Nginx Proxy Manager), I spent a week troubleshooting this issue. Without setting a default server in one of the configurations, IoT devices simply cannot connect.

I believe this issue might be related to how SNI is handled.

* Internal use only, checks to see if the there is another default server record
*
* @param {String} hostname
* @param {String} [ignore_type] 'proxy', 'redirection', 'dead'
* @param {Integer} [ignore_id] Must be supplied if type was also supplied
* @returns {Promise}
*/
checkDefaultServerNotExist: function (hostname) {
let promises = proxyHostModel
.query()
.where('default_server', true)
.andWhere('domain_names', 'not like', '%' + hostname + '%');


return Promise.resolve(promises)
.then((promises_results) => {
if (promises_results.length > 0){
return false;
}
return true;
});

}
};

module.exports = internalHost;
2 changes: 1 addition & 1 deletion backend/internal/nginx.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ const internalNginx = {
// Prevent modifying the original object:
let host = JSON.parse(JSON.stringify(host_row));
const nice_host_type = internalNginx.getFileFriendlyHostType(host_type);

if (config.debug()) {
logger.info('Generating ' + nice_host_type + ' Config:', JSON.stringify(host, null, 2));
}
Expand Down
33 changes: 33 additions & 0 deletions backend/internal/proxy-host.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,22 @@ const internalProxyHost = {
});
});
})
.then(() => {
// Get a list of the domain names and check each of them against default records
if (data.default_server){
if (data.domain_names.length > 1) {
throw new error.ValidationError('Default server cant be set for multiple domain!');
}

return internalHost
.checkDefaultServerNotExist(data.domain_names[0])
.then((result) => {
if (!result){
throw new error.ValidationError('One default server already exists');
}
});
}
})
.then(() => {
// At this point the domains should have been checked
data.owner_user_id = access.token.getUserId(1);
Expand Down Expand Up @@ -140,6 +156,22 @@ const internalProxyHost = {
});
}
})
.then(() => {
// Get a list of the domain names and check each of them against default records
if (data.default_server){
if (data.domain_names.length > 1) {
throw new error.ValidationError('Default server cant be set for multiple domain!');
}

return internalHost
.checkDefaultServerNotExist(data.domain_names[0])
.then((result) => {
if (!result){
throw new error.ValidationError('One default server already exists');
}
});
}
})
.then(() => {
return internalProxyHost.get(access, {id: data.id});
})
Expand All @@ -152,6 +184,7 @@ const internalProxyHost = {
if (create_certificate) {
return internalCertificate.createQuickCertificate(access, {
domain_names: data.domain_names || row.domain_names,
ssl_key_type: data.ssl_key_type || row.ssl_key_type,
meta: _.assign({}, row.meta, data.meta)
})
.then((cert) => {
Expand Down
39 changes: 39 additions & 0 deletions backend/migrations/20241209062244_ssl_key_type.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
const migrate_name = 'identifier_for_migrate';
const logger = require('../logger').migrate;

/**
* Migrate
*
* @see http://knexjs.org/#Schema
*
* @param {Object} knex
* @param {Promise} Promise
* @returns {Promise}
*/
exports.up = function (knex) {

logger.info(`[${migrate_name}] Migrating Up...`);

return knex.schema.alterTable('proxy_host', (table) => {
table.enum('ssl_key_type', ['ecdsa', 'rsa']).defaultTo('ecdsa').notNullable();
}).then(() => {
logger.info(`[${migrate_name}] Column 'ssl_key_type' added to table 'proxy_host'`);
});
};

/**
* Undo Migrate
*
* @param {Object} knex
* @param {Promise} Promise
* @returns {Promise}
*/
exports.down = function (knex) {
logger.info(`[${migrate_name}] Migrating Down...`);

return knex.schema.alterTable('proxy_host', (table) => {
table.dropColumn('ssl_key_type');
}).then(() => {
logger.info(`[${migrate_name}] Column 'ssl_key_type' removed from table 'proxy_host'`);
});
};
39 changes: 39 additions & 0 deletions backend/migrations/20241211081223_ssl_key_type_in_proxy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
const migrate_name = 'identifier_for_migrate';
const logger = require('../logger').migrate;

/**
* Migrate
*
* @see http://knexjs.org/#Schema
*
* @param {Object} knex
* @param {Promise} Promise
* @returns {Promise}
*/
exports.up = function (knex) {

logger.info(`[${migrate_name}] Migrating Up...`);

return knex.schema.alterTable('certificate', (table) => {
table.enum('ssl_key_type', ['ecdsa', 'rsa']).defaultTo('ecdsa').notNullable();
}).then(() => {
logger.info(`[${migrate_name}] Column 'ssl_key_type' added to table 'proxy_host'`);
});
};

/**
* Undo Migrate
*
* @param {Object} knex
* @param {Promise} Promise
* @returns {Promise}
*/
exports.down = function (knex) {
logger.info(`[${migrate_name}] Migrating Down...`);

return knex.schema.alterTable('certificate', (table) => {
table.dropColumn('ssl_key_type');
}).then(() => {
logger.info(`[${migrate_name}] Column 'ssl_key_type' removed from table 'proxy_host'`);
});
};
40 changes: 40 additions & 0 deletions backend/migrations/20241221201400_default_server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
const migrate_name = 'identifier_for_migrate';
const logger = require('../logger').migrate;

/**
* Migrate Up
*
* @param {Object} knex
* @param {Promise} Promise
* @returns {Promise}
*/
exports.up = function (knex) {
logger.info(`[${migrate_name}] Migrating Up...`);

// Add default_server column to proxy_host table
return knex.schema.table('proxy_host', (table) => {
table.boolean('default_server').notNullable().defaultTo(false);
})
.then(() => {
logger.info(`[${migrate_name}] Column 'default_server' added to 'proxy_host' table`);
});
};

/**
* Migrate Down
*
* @param {Object} knex
* @param {Promise} Promise
* @returns {Promise}
*/
exports.down = function (knex) {
logger.info(`[${migrate_name}] Migrating Down...`);

// Remove default_server column from proxy_host table
return knex.schema.table('proxy_host', (table) => {
table.dropColumn('default_server');
})
.then(() => {
logger.info(`[${migrate_name}] Column 'default_server' removed from 'proxy_host' table`);
});
};
1 change: 1 addition & 0 deletions backend/models/proxy_host.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const boolFields = [
'enabled',
'hsts_enabled',
'hsts_subdomains',
'default_server',
];

class ProxyHost extends Model {
Expand Down
9 changes: 9 additions & 0 deletions backend/schema/components/certificate-object.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@
"owner": {
"$ref": "./user-object.json"
},
"ssl_key_type": {
"type": "string",
"enum": ["ecdsa", "rsa"],
"description": "Type of SSL key (either ecdsa or rsa)"
},
"default_server": {
"type": "boolean",
"description": "Defines if the server is the default for unmatched requests"
},
"meta": {
"type": "object",
"additionalProperties": false,
Expand Down
11 changes: 11 additions & 0 deletions backend/schema/components/proxy-host-object.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
"locations",
"hsts_enabled",
"hsts_subdomains",
"ssl_key_type",
"default_server",
"certificate"
],
"additionalProperties": false,
Expand Down Expand Up @@ -149,6 +151,15 @@
"$ref": "./access-list-object.json"
}
]
},
"ssl_key_type": {
"type": "string",
"enum": ["ecdsa", "rsa"],
"description": "Type of SSL key (either ecdsa or rsa)"
},
"default_server": {
"type": "boolean",
"description": "Defines if the server is the default for unmatched requests"
}
}
}
6 changes: 6 additions & 0 deletions backend/schema/paths/nginx/proxy-hosts/hostID/put.json
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@
},
"locations": {
"$ref": "../../../../components/proxy-host-object.json#/properties/locations"
},
"ssl_key_type": {
"$ref": "../../../../components/proxy-host-object.json#/properties/ssl_key_type"
},
"default_server": {
"$ref": "../../../../components/proxy-host-object.json#/properties/default_server"
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions backend/schema/paths/nginx/proxy-hosts/post.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@
},
"locations": {
"$ref": "../../../components/proxy-host-object.json#/properties/locations"
},
"ssl_key_type": {
"$ref": "../../../components/proxy-host-object.json#/properties/ssl_key_type"
},
"default_server": {
"$ref": "../../../components/proxy-host-object.json#/properties/default_server"
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions backend/templates/_listen.conf
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
listen 80;
listen 80{% if default_server == true %} default_server{% endif %};
{% if ipv6 -%}
listen [::]:80;
listen [::]:80{% if default_server == true %} default_server{% endif %};
{% else -%}
#listen [::]:80;
{% endif %}
{% if certificate -%}
listen 443 ssl;
listen 443 ssl{% if default_server == true %} default_server{% endif %};
{% if ipv6 -%}
listen [::]:443 ssl;
listen [::]:443 ssl{% if default_server == true %} default_server{% endif %};
{% else -%}
#listen [::]:443;
{% endif %}
Expand Down
4 changes: 3 additions & 1 deletion docker/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,11 @@ COPY --from=testca /home/step/certs/root_ca.crt /etc/ssl/certs/NginxProxyManager
# Remove frontend service not required for prod, dev nginx config as well
RUN rm -rf /etc/s6-overlay/s6-rc.d/user/contents.d/frontend /etc/nginx/conf.d/dev.conf \
&& chmod 644 /etc/logrotate.d/nginx-proxy-manager
COPY docker/start-container /usr/local/bin/start-container
RUN chmod +x /usr/local/bin/start-container

VOLUME [ "/data" ]
ENTRYPOINT [ "/init" ]
ENTRYPOINT [ "start-container" ]

LABEL org.label-schema.schema-version="1.0" \
org.label-schema.license="MIT" \
Expand Down
5 changes: 4 additions & 1 deletion docker/dev/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,8 @@ RUN rm -f /etc/nginx/conf.d/production.conf \
COPY --from=pebbleca /test/certs/pebble.minica.pem /etc/ssl/certs/pebble.minica.pem
COPY --from=testca /home/step/certs/root_ca.crt /etc/ssl/certs/NginxProxyManager.crt

COPY start-container /usr/local/bin/start-container
RUN chmod +x /usr/local/bin/start-container

EXPOSE 80 81 443
ENTRYPOINT [ "/init" ]
ENTRYPOINT [ "start-container" ]
2 changes: 0 additions & 2 deletions docker/dev/letsencrypt.ini
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
text = True
non-interactive = True
webroot-path = /data/letsencrypt-acme-challenge
key-type = ecdsa
elliptic-curve = secp384r1
preferred-chain = ISRG Root X1
server =
Loading