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

Support ECDSA certificates on server side and show parameters in dialog #1483

Merged
merged 2 commits into from Sep 25, 2021

Conversation

domosekai
Copy link
Contributor

This PR lifts restrictions on certificate bits for server-side certificates, so that administrators can freely choose the type of certificates. This enables the use of ECDSA certificates.
Since ECDSA certificates (e.g. from Let's Encrypt) are super easy to get and increasingly preferred by many, I believe the change is important.

Ideally, we should make an aligned change in certificate handling for both client and server side. But since client side (e.g. certificate-based authentication) is more involved as we need to handle sign / verify functions, I am currently not changing that. Comments are welcome.

The certificate dialog in Windows UI won't display the ECC bits for the time being. This can also be improved later.

Fix #1034

@domosekai
Copy link
Contributor Author

Tested with CI artifacts.
Windows UI now shows the type along with the bits, such as "ECC 256 bits" or "RSA 2048 bits".
@davidebeatrici Could you please take a look at my handling of unicode string in WinUI.c? I am really unsure how it can be done gracefully.

@chipitsine
Copy link
Member

despite we bound to openssl-1.1.1 and higher, it might be built without ECDSA support, see openssl/openssl@3b6aa36

I suggest to add corresponding check in CMake (for example, https://github.com/SoftEtherVPN/SoftEtherVPN/blob/master/src/Mayaqua/CMakeLists.txt#L26 ) that openssl is built using ECDSA

@domosekai
Copy link
Contributor Author

I am not sure if it's needed as we are not going to change the default key from RSA. The user is responsible to make sure the OpenSSL and the default cipher supports ECDSA. For example if the default cipher is AES128-SHA it won't work.

@chipitsine
Copy link
Member

Users sometimes pick openssl from distribution. It would be good to check during cmake that openssl supports ecdsa.

However, I agree it is not critical. We'll fail during build anyway

@domosekai
Copy link
Contributor Author

domosekai commented Sep 20, 2021

Another way is adding the check in the check command. That way we can also check if the default cipher is compatible with the certificate.

@chipitsine
Copy link
Member

please ignore my suggestion. seems, "OPENSSL_NO_ECDSA" is gone from OpenSSL now.

should we merge and give it a go ?

@domosekai
Copy link
Contributor Author

I think it's ready to merge. Just the string process in WinUi.c feels a bit clumsy.

@domosekai
Copy link
Contributor Author

Ideally the certificate type should be on a separate line. I haven't figured out how to do that.

@domosekai
Copy link
Contributor Author

I revamped the certificate dialog. Now it shows key type and parameters.
image

@domosekai domosekai marked this pull request as ready for review September 24, 2021 09:26
@domosekai domosekai changed the title Allow ECDSA certificates on server side Support ECDSA certificates on server side and show parameters in dialog Sep 24, 2021
@chipitsine
Copy link
Member

let us merge and see.

I tried to build myself using our "build for windows" guide. it is indeed broken, pity that nobody except @domosekai complains :(
also, I investigated binaries using https://github.com/DynamoRIO/drmemory , there are few findings, but they to be there for long time. no need to block merging

@domosekai domosekai merged commit 3a2d588 into SoftEtherVPN:master Sep 25, 2021
@domosekai domosekai deleted the ecc branch September 28, 2021 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Request] Client and Server Manager support ECDSA certificates
2 participants