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

Conversation

mnr73
Copy link

@mnr73 mnr73 commented Dec 9, 2024

Added the ability to specify the SSL key type (ECDSA or RSA) for each site in the Nginx Proxy Manager. This enhancement is particularly useful for environments with IoT devices that have limitations with specific key types, such as RSA-only support. The implementation includes:

  • Backend support for storing and validating the ssl_key_type field.
  • Swagger schema updated to validate the new input.
  • Frontend update to allow users to select the SSL key type via a dropdown menu.

This feature ensures greater flexibility and compatibility in managing SSL certificates for diverse setups.

#3354

mnr73 added 9 commits December 9, 2024 11:27
Added the ability to specify the SSL key type (ECDSA or RSA) for each site in the Nginx Proxy Manager. This enhancement is particularly useful for environments with IoT devices that have limitations with specific key types, such as RSA-only support. The implementation includes:

- Backend support for storing and validating the `ssl_key_type` field.
- Swagger schema updated to validate the new input.
- Frontend update to allow users to select the SSL key type via a dropdown menu.

This feature ensures greater flexibility and compatibility in managing SSL certificates for diverse setups.
This reverts commit 95a94a4.
fix ci test error
@mnr73
Copy link
Author

mnr73 commented Dec 12, 2024

it's work truly :)
image
image

@jc21
Copy link
Member

jc21 commented Dec 16, 2024

Great work. Do you happen to have any automated commands that might test the feature? ie:

  1. I request a ECDSA cert and run a console command to check it given I only have the HTTPS URL
  2. I request a RSA cert and run a console command to check it given I only have the HTTPS URL

@mnr73
Copy link
Author

mnr73 commented Dec 17, 2024

Thanks. Yes I think could be test with nmap but I work on Diffie-Hellman cipher support and another problem that we had for IOT devices. after that I add some automated test for this.

Please hold on while I complete these.

@mnr73
Copy link
Author

mnr73 commented Dec 22, 2024

This pull request introduces features specifically designed to address common challenges with Embedded and IoT devices. The following updates have been made:

  • Encryption Key Selection: Added the ability to choose between RSA and ECDSA encryption keys for each domain to enhance flexibility and security.
  • Extended Cipher Suites: Included additional cipher suites to support a broader range of IoT devices.
  • Support for Diffie-Hellman Parameters: Integrated cipher suites for stronger and more secure communications.
  • Default Server Selection: Added the capability to designate a default server from existing hosts for simpler configuration and better management.

related issues

@mnr73
Copy link
Author

mnr73 commented Dec 22, 2024

the result supported ciphers

PORT    STATE SERVICE
443/tcp open  https
| ssl-enum-ciphers:
|   TLSv1.2:
|     ciphers:
|       TLS_DHE_RSA_WITH_AES_128_CBC_SHA (dh 2048) - A
|       TLS_DHE_RSA_WITH_AES_128_CBC_SHA256 (dh 2048) - A
|       TLS_DHE_RSA_WITH_AES_128_CCM (dh 2048) - A
|       TLS_DHE_RSA_WITH_AES_128_CCM_8 (dh 2048) - A
|       TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 (dh 2048) - A
|       TLS_DHE_RSA_WITH_AES_256_CBC_SHA (dh 2048) - A
|       TLS_DHE_RSA_WITH_AES_256_CBC_SHA256 (dh 2048) - A
|       TLS_DHE_RSA_WITH_AES_256_CCM (dh 2048) - A
|       TLS_DHE_RSA_WITH_AES_256_CCM_8 (dh 2048) - A
|       TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 (dh 2048) - A
|       TLS_DHE_RSA_WITH_ARIA_128_GCM_SHA256 (dh 2048) - A
|       TLS_DHE_RSA_WITH_ARIA_256_GCM_SHA384 (dh 2048) - A
|       TLS_DHE_RSA_WITH_CAMELLIA_128_CBC_SHA (dh 2048) - A
|       TLS_DHE_RSA_WITH_CAMELLIA_128_CBC_SHA256 (dh 2048) - A
|       TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA (dh 2048) - A
|       TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA256 (dh 2048) - A
|       TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (dh 2048) - A
|       TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_ARIA_128_GCM_SHA256 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_ARIA_256_GCM_SHA384 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_CAMELLIA_128_CBC_SHA256 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_CAMELLIA_256_CBC_SHA384 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (secp256r1) - A
|       TLS_RSA_WITH_AES_128_CBC_SHA (rsa 2048) - A
|       TLS_RSA_WITH_AES_128_CBC_SHA256 (rsa 2048) - A
|       TLS_RSA_WITH_AES_128_CCM (rsa 2048) - A
|       TLS_RSA_WITH_AES_128_CCM_8 (rsa 2048) - A
|       TLS_RSA_WITH_AES_128_GCM_SHA256 (rsa 2048) - A
|       TLS_RSA_WITH_AES_256_CBC_SHA (rsa 2048) - A
|       TLS_RSA_WITH_AES_256_CBC_SHA256 (rsa 2048) - A
|       TLS_RSA_WITH_AES_256_CCM (rsa 2048) - A
|       TLS_RSA_WITH_AES_256_CCM_8 (rsa 2048) - A
|       TLS_RSA_WITH_AES_256_GCM_SHA384 (rsa 2048) - A
|       TLS_RSA_WITH_ARIA_128_GCM_SHA256 (rsa 2048) - A
|       TLS_RSA_WITH_ARIA_256_GCM_SHA384 (rsa 2048) - A
|       TLS_RSA_WITH_CAMELLIA_128_CBC_SHA (rsa 2048) - A
|       TLS_RSA_WITH_CAMELLIA_128_CBC_SHA256 (rsa 2048) - A
|       TLS_RSA_WITH_CAMELLIA_256_CBC_SHA (rsa 2048) - A
|       TLS_RSA_WITH_CAMELLIA_256_CBC_SHA256 (rsa 2048) - A
|     compressors:
|       NULL
|     cipher preference: client
|_  least strength: A

Nmap done: 1 IP address (1 host up) scanned in 33.33 seconds

@mnr73
Copy link
Author

mnr73 commented Dec 23, 2024

Great work. Do you happen to have any automated commands that might test the feature? ie:

  1. I request a ECDSA cert and run a console command to check it given I only have the HTTPS URL
  2. I request a RSA cert and run a console command to check it given I only have the HTTPS URL

with this command we can see what is the SSL key type. rsa or ecdsa

nmap --script ssl-cert -p 443 example.com

or can use this command with openssl

openssl s_client -connect example.com:443 </dev/null 2>/dev/null | openssl x509 -noout -text | grep "Public Key Algorithm"

@jc21 jc21 added the requires-verification Waiting for one or more people to confirm the fix label Dec 29, 2024
@jc21
Copy link
Member

jc21 commented Dec 29, 2024

Nice. I'll look at adding this in the next release, after the one I'm about to do today.

frontend/js/app/nginx/proxy/form.ejs Show resolved Hide resolved
@@ -0,0 +1,13 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

This can be achieved using the S6 init scripts instead of adding another layer of initialization. However this might not be required here at all...

Can this file /etc/ssl/certs/dhparam.pem be generated at build time instead of run time?

Copy link
Author

Choose a reason for hiding this comment

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

if generate this file in build time. it's be same for all user that use this and i think this is a security problem.

Copy link
Member

Choose a reason for hiding this comment

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

ok maybe, but why would it be different for all users when they are all using the same docker image?

Copy link
Author

Choose a reason for hiding this comment

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

The DH parameter file is used for secure key exchange, and having the same file for all users can compromise security. It’s recommended to generate a unique file per instance to ensure the security of each user’s connection.


/**
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.

backend/internal/nginx.js Outdated Show resolved Hide resolved
backend/schema/components/certificate-object.json Outdated Show resolved Hide resolved
backend/schema/components/proxy-host-object.json Outdated Show resolved Hide resolved
@mnr73
Copy link
Author

mnr73 commented Jan 4, 2025

I think there is a problem in auto test. i test in local every thing was Ok. also revert all commit but it fail again.

@nginxproxymanagerci
Copy link

Docker Image for build 21 is available on
DockerHub
as nginxproxymanager/nginx-proxy-manager-dev:pr-4218

Note: ensure you backup your NPM instance before testing this image! Especially if there are database changes
Note: this is a different docker image namespace than the official image

@mnr73 mnr73 requested a review from jc21 January 8, 2025 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement requires-verification Waiting for one or more people to confirm the fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants