Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
mnr73 wants to merge30 commits intoNginxProxyManager:develop
base:develop
Choose a base branch
Loading
frommnr73:develop

Conversation

mnr73
Copy link

@mnr73mnr73 commentedDec 9, 2024•
edited
Loading

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 thessl_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

bro0k, iyinchao, ggottwald, Lure5134, dannyadair, and grzywek reacted with thumbs up emojigrzywek reacted with heart emoji
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.
@mnr73
Copy link
Author

it's work truly :)
image
image

@jc21
Copy link
Member

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

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

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

the result supported ciphers

PORT    STATE SERVICE443/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: ANmap done: 1 IP address (1 host up) scanned in 33.33 seconds

@mnr73
Copy link
Author

mnr73 commentedDec 23, 2024•
edited
Loading

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"

@jc21jc21 added the requires-verificationWaiting for one or more people to confirm the fix labelDec 29, 2024
@jc21
Copy link
Member

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

mnr73, ggottwald, and Lure5134 reacted with thumbs up emoji

@@ -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 yourcheckDefaultServerNotExist 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.

@mnr73
Copy link
Author

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 22 is available on
DockerHub
asnginxproxymanager/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

Lure5134 reacted with thumbs up emoji

@localhost14
Copy link

When will this be implemented?

mtrr, localhost14, DoozyDoz, and Lure5134 reacted with thumbs up emoji

@Lure5134
Copy link

@jc21 Is there anything we can do for this pr so it gets merged?

localhost14 and dannyadair reacted with thumbs up emoji

@grzywek
Copy link

@mnr73 hello, I testednginxproxymanager/nginx-proxy-manager-dev:pr-4218

I believe to maintain consistency, option to select SSL Key Type should be also visible while creating certificate from the Certificates tab:

Screenshot-02-06-2025-20-35-43@2x

Maybe we should include the key type in the table to have a clear distinction:

Screenshot-02-06-2025-20-39-20@2x

and also here:

Screenshot-02-06-2025-20-41-19@2x

Thanks - that PR already helped me much 😊

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jc21jc21Awaiting requested review from jc21

Assignees
No one assigned
Labels
enhancementrequires-verificationWaiting for one or more people to confirm the fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@mnr73@jc21@localhost14@Lure5134@grzywek
[8]ページ先頭
©2009-2025 Movatter.jp