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

Add support for openssl-1.1.0 #504

Merged
merged 7 commits into from Mar 15, 2019
Merged

Add support for openssl-1.1.0 #504

merged 7 commits into from Mar 15, 2019

Conversation

arjendekorte
Copy link
Contributor

Tumbleweed (openSUSE) is switching to OpenSSL-1.1.0 shortly. Apply changes to server & client code and AutoConf magic to make this happen.

@clepple clepple mentioned this pull request Nov 28, 2017
@arjendekorte
Copy link
Contributor Author

I amended this pull request a couple of times, to add proper support for protocols > TLSv1.

The OpenSSL code was developed against openssl < 1.0.1. At that time, specifying TLSv1_client_method and TLSv1_server_method, was enough to disable the use of SSLv2 and SSLv3 and automatically the highest (being the only remaining one) was used. With TLSv1.3 just around the corned and in some industries, TLSv1 is nearing its end-of-life.

Rather than hardcoding the protocol to TLSv1, this pull requests let the client and server figure out the highest mutual understood version and use that. In order to facilitate debugging which one is actually used, I just added the reporting of the version in debug mode. I doubt it is worth reporting to the user.

@clepple
Copy link
Member

clepple commented Dec 7, 2017

Hi @arjendekorte, thanks for putting this together. I agree that the code should negotiate the newest TLS version, and it sounds like the newer OpenSSL APIs make this easier than the last time we looked at that.

If I am not mistaken, the negotiated TLS version should be visible in Wireshark (which would seem to me to be a good way to verify that no NUT traffic is in cleartext), so debug logging is probably sufficient.

At the risk of adding too much to the TODO list for 2.7.5, I think this is distinct enough from the other post-2.7.4 changes that we should be able to merge this without any trouble. I would like to do a bit of regression testing (probably this weekend) against packaged versions of NUT beforehand.

@arjendekorte
Copy link
Contributor Author

Well, I guess we never looked at supporting newer TLS versions. The last time I looked at that piece of code (Nov 29, 2010) openSSL didn't support TLSv1.1 and TLSv1.2 (these were added in openSSL 1.0.1, released Mar 14, 2012). At that time supporting everything better than SSLv3, effectively meant TLSv1 only and since then, nobody bothered to add better handshakes when they became available.

For the record, I already tested interoperability pre- and post changes. With both server and client using the changed openSSL initialization code, TLSv1.2 is agreed (for both openSSL-1.0.2 and -1.1.0). If either one is 'old', TLSv1 is used. I did not check against NSS by the way. From looking at that code, it is not clear to me if it supports anything better than TLSv1, it seems to me it might suffer from the same in this aspect.

@arjendekorte
Copy link
Contributor Author

As a side note, I'm working (low profile) on supporting the mbedTLS library as well. I'm loosely involved in OpenWRT/LEDE where this is the default SSL library. Due to constraints on size, both openSSL and NSS libraries are too bloated for a lot of hardware. This would make the mess the upsclient.c and netssl.c are already in even worse, so my proposal would be to remove the #ifdef hell we're in by splitting these in seprate modules:

net-nossl.c
net-openssl.c
net-nss.c
net-mbedtls.c

These would all expose the same functions and since there is very little common code between the different SSL libraries, picking the right one at link time makes more sense. From a maintenance point of view also preferred, since changes like the one above would only touch one SSL implementation, so the risk of accidentally breaking support for other SSL libraries will be lower.

@dmacks
Copy link

dmacks commented Feb 4, 2018

@clepple, I'm tracking openssl-1.1.0 status of fink packages; I'll tag nut as "pending". Let me know if you need help testing in the fink environment.

@jimklimov
Copy link
Member

Just in case, was it tested against different versions of openssl, to at least compile with them all? :)

Now that we have travis testing, we can add test suites that would request different ubuntu versions (12, 14, now 16 recently) and/or point to specific openssl package versions to install in the testing buildenv.

@arjendekorte
Copy link
Contributor Author

As written above, this was interoperability tested with both OpenSSL 1.0.2 and 1.1.0, the only supported versions nowadays. It should be fine with 1.0.1 though.

Copy link

@cotequeiroz cotequeiroz left a comment

Choose a reason for hiding this comment

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

I'm making some suggestions, of which moving SSL_load_error_strings() is the most important, to avoid the use of a deprecated function (you could save a few bytes of real state in openwrt by not including them).

@@ -315,22 +310,35 @@ int upscli_init(int certverify, const char *certpath,
}

#ifdef WITH_OPENSSL

SSL_load_error_strings();

Choose a reason for hiding this comment

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

This is done automatically, and was deprecated, starting with openssl 1.1.0; my suggestion would be to move it inside OPENSSL_VERSION_NUMBER < 0x10100000L

server/netssl.c Outdated Show resolved Hide resolved
@@ -57,8 +57,9 @@ if test -z "${nut_have_libopenssl_seen}"; then
AC_MSG_RESULT([${LIBS}])

dnl check if openssl is usable
AC_CHECK_HEADERS(openssl/ssl.h, [nut_have_openssl=yes], [nut_have_openssl=no], [AC_INCLUDES_DEFAULT])
AC_CHECK_FUNCS(SSL_library_init, [], [nut_have_openssl=no])

Choose a reason for hiding this comment

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

Checking for SSL_CTX_new would be enough for what we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

ssl_method = TLSv1_client_method();
ssl_ctx = SSL_CTX_new(SSLv23_client_method());
#else
OPENSSL_init_ssl(0, NULL);

Choose a reason for hiding this comment

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

This call is not necessary, as library initialization is done automatically now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

server/netssl.c Outdated
if ((ssl_method = TLSv1_server_method()) == NULL) {
ssl_ctx = SSL_CTX_new(SSLv23_server_method());
#else
OPENSSL_init_ssl(0, NULL);

Choose a reason for hiding this comment

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

Not necessary, same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

@aquette aquette merged commit 612c05e into networkupstools:master Mar 15, 2019
jimklimov pushed a commit to jimklimov/nut that referenced this pull request Apr 1, 2019
* Add support for openssl-1.1.0

* Allow TLSv1 and higher (not just TLSv1)

* Fix check for empty string

* Report TLS handshake in debug mode

* Update nut_check_libopenssl.m4

* Update upsclient.c

* Update netssl.c
@jimklimov jimklimov added enhancement SSL/NSS Issues and PRs about SSL, TLS and other crypto-related matters LEDE/*WRT embedded Linux firmwares OpenWrt/LEDE, DD-WRT, Tomato and such. Small-scale NUT. labels Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement LEDE/*WRT embedded Linux firmwares OpenWrt/LEDE, DD-WRT, Tomato and such. Small-scale NUT. SSL/NSS Issues and PRs about SSL, TLS and other crypto-related matters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants