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

Ignore premature termination in GnuTLS. #189

Merged
merged 3 commits into from
Dec 27, 2017

Conversation

acolomb
Copy link
Contributor

@acolomb acolomb commented Dec 26, 2017

While testing with the update API server from dynu.com, the received response was never shown in the debug log. After adding some more logging to ssl_read() in gnutls.c, it turns out that the server is prematurely closing the connection without a proper TLS shutdown message, which is buggy behaviour.

Googling around, I found some similar reports for other software, where the solution on the client-side is to just ignore the GNUTLS_E_PREMATURE_TERMINATION error, just as OpenSSL does by itself. I'm not aware of the exact security implications, but in this case it fixed my update errors with dynu.com.

The log message should also be implemented in openssl.c, but I couldn't yet get my head around their error reporting API, and have not even compiled with OpenSSL for testing.

When receiving a reply using GnuTLS, gnutls_record_recv() may return
GNUTLS_E_PREMATURE_TERMINATION when the server closes the TCP
connection without a prior TLS shutdown alert. That probably indicates
a bug in the server's TLS handling.

Assuming the error is not really fatal, and because OpenSSL seems to
ignore it as well, check for and ignore this error code in the GnuTLS
implementation.

Also adds some log messages for GnuTLS connection failures.
@troglobit
Copy link
Owner

Nice catch! However, Travis-CI fails to build due to what seems to be caused by too old GnuTLS.

gnutls.c:322:24: error: ‘GNUTLS_E_PREMATURE_TERMINATION’ undeclared (first use in this function)
  if (ret < 0 && ret != GNUTLS_E_PREMATURE_TERMINATION) {
                        ^
gnutls.c:322:24: note: each undeclared identifier is reported only once for each function it appears in
make[1]: *** [inadyn-gnutls.o] Error 1
make[1]: *** Waiting for unfinished jobs....

Could you refactor your code to handle this, please?

@acolomb
Copy link
Contributor Author

acolomb commented Dec 26, 2017

I don't get why Travis fails on this. GNUTLS_E_PREMATURE_TERMINATION has been declared in gnutls/gnutls.h since at least version 2.99, and Ubuntu trusty has version 3.2.11-2ubuntu1.1. I even checked the built package for trusty on launchpad, the macro definition is definitely there.

Any other idea what may be going wrong here?

@acolomb
Copy link
Contributor Author

acolomb commented Dec 26, 2017

@troglobit
Copy link
Owner

I think I've found it ➡️ travis-ci/travis-ci#5659 -- they're still using the default gnutls26 package.

OK, we should update .travis.yml to install libgnutls28-dev and update configure.ac to PKG_CHECK_MODULES([GnuTLS], [gnutls >= 2.8]) ... to enfore a slightly newer version. I would greatly appreciate if you could do this on your branch!

The GNUTLS_E_PREMATURE_TERMINATION error code appeared just before the
GnuTLS 3.0 release, according to their changelog.  Add a version
requirement in configure.ac and update Travis CI script to pull in the
libgnutls28-dev package, which should be at version 3.2.11 in Ubuntu
trusty.
@acolomb
Copy link
Contributor Author

acolomb commented Dec 27, 2017

Actually it needs gnutls >= 3.0, as gnutls28 is just the Debian package's soname. The old trusty version was already above 2.12 according to pkgconfig. Seeing that the 2.99 changelog first mentions the new constant, I suppose 3.0 was the first suitable stable release.

Now does this propagate automatically to the Debian build scripts' build dependencies?

@acolomb
Copy link
Contributor Author

acolomb commented Dec 27, 2017

Apparently it does not. Should we specify gnutls-dev (>= 3.0) or libgnutls28-dev then?

@troglobit
Copy link
Owner

Commit f9d9718 seems to have done the trick, good job! OK, gnutls >= 3.0 seems like a good point to start from, even though I'm sure some RedHat/CentOS user will soon report build problems ... ;-)

Nope, to fix the Debian build the debian/control file needs to be updated to gnutls-dev (>= 2.8), or something.

@troglobit
Copy link
Owner

(That reminds me, I need to add a build pass to Travis to generate .deb files, just for testing.)

After Travis builds fine with the regular toolchain, make sure the
Debian package build will also succeed by including the right GnuTLS
version as build dependency.
@acolomb
Copy link
Contributor Author

acolomb commented Dec 27, 2017

As far as I can see, the Debian packaging should be okay now even with Travis. While you're at it, care to provide premade DEBs in a PPA or in GitHub releases? ;-)

@troglobit
Copy link
Owner

Great stuff! Just checked and libgnutls28-dev seems to be available in all relevant releases :-)

Heh, yeah I've been considering providing ready made debs, even started messing around with my own repo. But I'll definitely consider uploading .debs to GitHub at least, not just for Inadyn but for libconfuse as well :)

Merging ...

@troglobit troglobit merged commit 9521387 into troglobit:master Dec 27, 2017
@acolomb acolomb deleted the gnutls-premature-termination branch December 27, 2017 16:11
@acolomb
Copy link
Contributor Author

acolomb commented Dec 27, 2017

Thanks, nice working with you. Sorry for not updating the ChangeLog, but I'm never sure what version number to put in there for my changes.

@troglobit
Copy link
Owner

No, thank you! Always a pleasure working with professionals 😃

No worries, I hadn't updated the configure.ac with the version for the next release cycle, and it's almost always up to the maintainer to handle such things anyway :)

@acolomb
Copy link
Contributor Author

acolomb commented Dec 27, 2017

Funny thing, when I saw you're now maintaining libconfuse, it reminded me of a project I started almost eleven years ago where I used just that for my config file handling. It was a small background daemon to play sound samples upon Linux input events, for really silent keyboards to make cool sounds. Configurable for each key or mouse button... Never released it as open source though, and never even used it much myself.

Anyhow, it's a steep learning curve to get up to speed with Autotools, Git, socket programming and friends :-)

@troglobit
Copy link
Owner

libConfuse really is an old dog, but very useful! I've just done some cleanup work and maintenance releases, glad you liked it but most of the work was done by @martinh so all thanks should go to him :)

It sure is steep, but I must say you've managed well! Most people don't care enough to read up on details such as why sth doesn't build on old releases, details in Debian packaging, etc.

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.

2 participants