I used to think that next Nettle release should be focused on performance and add a few more features, no incompatible changes.
But since the gcm changes breaks gnutls (fix in https://gitlab.com/gnutls/gnutls/commit/176aa191380e5d63a981f08f537deffdd78f...), best way to reduce damage might be to bump the nettle soname. And if we do that, there are a couple of other things that could be done at the same time. Some changes to consider:
1. Remove the the symbols nettle_hashes and nettle_secp_384r1 and friends from the interface. They should be renamed with leading underscore. Important to be able to merge ed448 work without breaking the abi again.
2. Delete the old aes_* interface, in favor of aes128_, aes192_* and aes256_*. This could help aesni performance, where it might be nice with separately completely unrolled code for each key size.
3. Change struct nettle_aead to be more message-oriented.
4. Hide undocumented and internal symbols (the ones with leading underscore) more. Move declarations to internal uninstalled headers, possible tweak linker script to not expose at all in the shared library.
5. Revamp hmac and underlying hash functions with a separate state struct. Probably low priority, but it is a bit silly that, e.g., hmac_sha512_ctx includes three 128-byte large block buffers.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
- Delete the old aes_* interface, in favor of aes128_, aes192_* and aes256_*.
I've now made a branch for this, delete-old-aes.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
- Delete the old aes_* interface, in favor of aes128_, aes192_* and aes256_*.
I've now made a branch for this, delete-old-aes.
And it seems building gnutls with this branch fails, see https://gitlab.com/gnutls/nettle/-/jobs/53760965
aes-padlock.c: In function 'padlock_aes_cipher_setkey': aes-padlock.c:65:17: error: storage size of 'nc' isn't known struct aes_ctx nc; ^~
It's great to have that ci job set up.
Regards, /Niels
On Thu, 2018-02-22 at 07:54 +0100, Niels Möller wrote:
nisse@lysator.liu.se (Niels Möller) writes:
- Delete the old aes_* interface, in favor of aes128_, aes192_*
and aes256_*.
I've now made a branch for this, delete-old-aes.
And it seems building gnutls with this branch fails, see https://gitlab.com/gnutls/nettle/-/jobs/53760965
aes-padlock.c: In function 'padlock_aes_cipher_setkey': aes-padlock.c:65:17: error: storage size of 'nc' isn't known struct aes_ctx nc; ^~
It's great to have that ci job set up.
Thanks for bringing that up. I have a quick fix for that, although I no longer have such systems for checking. I dropped AES-192 accelerated support as part of that patch as well. https://gitlab.com/gnutls/gnutls/merge_requests/602
How widely used are these macros? Searching debian code: https://codesearch.debian.net/search?q=aes_set_encrypt_key&page=1&pe...
seems to show gnutls (in fips140 drbg code), stoken, qemu, rdup, filezilla, pike, cmake, uanytun, haskell-bindings-nettle, libarchive, anytun, and mosh.
That seems to be quite a popular API and removing it would break those projects. Why not keep it as backwards compatible and mark it as deprecated with a macro (copied from gnutls):
#ifdef __GNUC__ # define _GNUTLS_GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)
# if _GNUTLS_GCC_VERSION >= 30100 # define _GNUTLS_GCC_ATTR_DEPRECATED __attribute__ ((__deprecated__)) # endif #endif
#ifndef _GNUTLS_GCC_ATTR_DEPRECATED #define _GNUTLS_GCC_ATTR_DEPRECATED #endif ?
regards, Nikos
Hello,
2018-02-22 13:41 GMT+03:00 Nikos Mavrogiannopoulos nmav@redhat.com:
On Thu, 2018-02-22 at 07:54 +0100, Niels Möller wrote:
nisse@lysator.liu.se (Niels Möller) writes:
Thanks for bringing that up. I have a quick fix for that, although I no longer have such systems for checking.
Do you need one? Maybe we can buy you smth. from eBay if it's not too costly?
seems to show gnutls (in fips140 drbg code), stoken, qemu, rdup, filezilla, pike, cmake, uanytun, haskell-bindings-nettle, libarchive, anytun, and mosh.
That seems to be quite a popular API and removing it would break those projects. Why not keep it as backwards compatible and mark it as deprecated with a macro (copied from gnutls):
Dropping API is always painfull. Maybe it can be verbally deprecated now and removed before 4.0 in future?
It is not possible to deprecate a macro. Deprecation attributes are handled by compiler, while macros are resolved by preprocessor.
#ifdef __GNUC__ # define _GNUTLS_GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)
# if _GNUTLS_GCC_VERSION >= 30100 # define _GNUTLS_GCC_ATTR_DEPRECATED __attribute__ ((__deprecated__)) # endif #endif
On Thu, 2018-02-22 at 13:51 +0300, Dmitry Eremin-Solenikov wrote:
Hello,
2018-02-22 13:41 GMT+03:00 Nikos Mavrogiannopoulos nmav@redhat.com:
On Thu, 2018-02-22 at 07:54 +0100, Niels Möller wrote:
nisse@lysator.liu.se (Niels Möller) writes:
Thanks for bringing that up. I have a quick fix for that, although I no longer have such systems for checking.
Do you need one? Maybe we can buy you smth. from eBay if it's not too costly?
The hw cost is not really a blocker. Maintenance costs are, such as, installing everything and making sure that system is online and part of the gnutls CI, and when something breaks (disks etc) replace it. I've tried it before, but I gave up on that approach. If there is a cloud provider which provides Via cpus we can most likely include it in gnutls' CI. Otherwise I think it would be too time consuming.
regards, Nikos
Nikos Mavrogiannopoulos nmav@redhat.com writes:
How widely used are these macros? Searching debian code: https://codesearch.debian.net/search?q=aes_set_encrypt_key&page=1&pe...
seems to show gnutls (in fips140 drbg code), stoken, qemu, rdup, filezilla, pike, cmake, uanytun, haskell-bindings-nettle, libarchive, anytun, and mosh.
If we want to keep it, and still make it possible to replace x86_64/aesni/aes-encrypt-internal.asm with three x86_64/aesni/aesKEYSIZE-encrypt.asm, we could do that by redefining aes_ctx as something like
struct aes_ctx { unsigned keysize; union { struct aes128_ctx aes128; struct aes192_ctx aes192; struct aes256_ctx aes256; } u; };
and write each aes_* function as a switch on the keysize. Since we already have the tests, that should be fairly cheap in terms of maintenance.
Deprecation was announced with nettle-3.0, 3.5 years ago:
The old interface, with struct aes_ctx and struct camellia_ctx, is kept for backwards compatibility, but might be removed in later versions. (NEWS file)
but I guess noone saw any urgent need to update old code to the new interfaces. (We never do, do we?).
Regards, /Niels
Nikos Mavrogiannopoulos nmav@redhat.com writes:
#ifdef __GNUC__ # define _GNUTLS_GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)
# if _GNUTLS_GCC_VERSION >= 30100 # define _GNUTLS_GCC_ATTR_DEPRECATED __attribute__ ((__deprecated__)) # endif #endif
That means that __attribute__ ((__deprecated__)) was introduced in gcc-3.1 ? Is that documented somewhere? I find no mention in https://gcc.gnu.org/gcc-3.1/changes.html.
Regards, /Niels
On Mon, Mar 12, 2018 at 2:49 PM, Niels Möller nisse@lysator.liu.se wrote:
Nikos Mavrogiannopoulos nmav@redhat.com writes:
#ifdef __GNUC__ # define _GNUTLS_GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)
# if _GNUTLS_GCC_VERSION >= 30100 # define _GNUTLS_GCC_ATTR_DEPRECATED __attribute__ ((__deprecated__)) # endif #endif
That means that __attribute__ ((__deprecated__)) was introduced in gcc-3.1 ? Is that documented somewhere? I find no mention in https://gcc.gnu.org/gcc-3.1/changes.html.
I believe the versions for '__attribute__((deprecated (msg)))' are:
* GCC 4.5 * LLVM Clang 2.8 * Apple Clang 4.2
There is a version without 'msg' that has been around since the GCC 3 days. But I can't find it either. If the 'msg' version is not available you are usually safe to fall back to the non-msg version with GCC. I've never seen the non-msg version break a compile.
Jeff
On Mon, Mar 12, 2018 at 2:56 PM, Jeffrey Walton noloader@gmail.com wrote:
On Mon, Mar 12, 2018 at 2:49 PM, Niels Möller nisse@lysator.liu.se wrote:
Nikos Mavrogiannopoulos nmav@redhat.com writes:
#ifdef __GNUC__ # define _GNUTLS_GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)
# if _GNUTLS_GCC_VERSION >= 30100 # define _GNUTLS_GCC_ATTR_DEPRECATED __attribute__ ((__deprecated__)) # endif #endif
That means that __attribute__ ((__deprecated__)) was introduced in gcc-3.1 ? Is that documented somewhere? I find no mention in https://gcc.gnu.org/gcc-3.1/changes.html.
I believe the versions for '__attribute__((deprecated (msg)))' are:
* GCC 4.5 * LLVM Clang 2.8 * Apple Clang 4.2There is a version without 'msg' that has been around since the GCC 3 days....
Here it is. It first surfaces in the GCC 3.1.1 manual: https://gcc.gnu.org/onlinedocs/gcc-3.1.1/gcc/Function-Attributes.html#Functi...
It is missing from the 2.95 and 3.0.4 manuals.
Jeff
nisse@lysator.liu.se (Niels Möller) writes:
- Remove the the symbols nettle_hashes and nettle_secp_384r1 and friends from the interface. They should be renamed with leading underscore. Important to be able to merge ed448 work without breaking the abi again.
I've made this change, and pushed to a branch rename-data-symbols. Might break gnutls, if it hasn't yet been updated to use functions nettle_get_secp_256r1 and friends.
Regards, /Niels
On Sat, 2018-03-17 at 17:30 +0100, Niels Möller wrote:
nisse@lysator.liu.se (Niels Möller) writes:
- Remove the the symbols nettle_hashes and nettle_secp_384r1 and
friends from the interface. They should be renamed with leading underscore. Important to be able to merge ed448 work without breaking the abi again.
I've made this change, and pushed to a branch rename-data-symbols. Might break gnutls, if it hasn't yet been updated to use functions nettle_get_secp_256r1 and friends.
It has but the detection is done through major and minor lib version. Are they updated? (btw. I'd really recommend providing easy to use macros for version detection)
However what is the goal with this rename? As far as I see the _nettle symbols are also exported from the library and thus the new symbols are as well. Wouldn't it make sense to remove them from the map file as well, and only export symbols starting with nettle_*?
regards, Nikos
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
It has but the detection is done through major and minor lib version. Are they updated?
Not yet. I could add a version number bump on that branch, and see if the tests then pass. I generally recommend configure checks for features used rather than version checks, whenever practical.
(btw. I'd really recommend providing easy to use macros for version detection)
I know... As I see it, that could be motivated by convenience primarily for projects not using autoconf.
However what is the goal with this rename?
To be able to change the size in later releases, without *subtle* breakages of applications using the old and documented api.
Wouldn't it make sense to remove them from the map file as well, and only export symbols starting with nettle_*?
I'm considing it, but it's not trivial. A related option is to move declarations into internal, uninstalled headers.
First, symbols have underscores for a few different reasons; the symbols I'm renaming here have it only because size is private and it may leak into the abi with some flavors of linking. Otherwise, they're well documented and unlikely to see any incompatible changes.
Other symbols, e.g., _nettle_sha256_compress and _nettle_umac_nh, are internal interfaces which might change if nettle gets new or different optimizations.
And there are different kinds of possible uses:
0. From within the nettle library.
1. From test and benchmarking executables in Nettle.
2. From user's experimental code, which don't care much about api or abi compatibilities with other versions.
3. Statically linked binaries, e.g, on embedded systems, might access _nettle_secp_256r1 directly to avoid an extra level of indirection and runtime lookup, with no real problem.
4. Plain applications relying on both api and abi stability.
We need to allow (1) (at least for some of the _nettle_* symbols). We need to strongly discourage (4). For (2) and (3), it would be nice to be liberal, but it might be fine to simply export more symbols in the static library.
Removing declarations from internal header files would allow (1) and (2), and strongly discourage all other use (if anyone adds their own declarations to be able to call internal functions in a library they are using, they ought to understand they're in unsupported territory).
What would it take to hide all _nettle symbols in libnettle.se? Just delete the _nettle_* line in libnettle.map.in, like
--- a/libnettle.map.in +++ b/libnettle.map.in @@ -9,7 +9,6 @@ NETTLE_@LIBNETTLE_MAJOR@ { global: nettle_*; - _nettle_*;
local: *;
Regards, /Niels
On Sun, 2018-03-18 at 16:59 +0100, Niels Möller wrote:
Wouldn't it make sense to remove them from the map file as well, and only export symbols starting with nettle_*?
I'm considing it, but it's not trivial. A related option is to move declarations into internal, uninstalled headers.
That would be sufficient. In gnutls I group these internal symbols outside the main version used for symbols. That is:
https://gitlab.com/gnutls/gnutls/blob/master/lib/libgnutls.map#L1244
These are not present in any header, and tests use the internal headers to see them. Another approach could be to put them into a version which has variable (per release name). That would allow linking with them, but will always break software that uses them.
First, symbols have underscores for a few different reasons; the symbols I'm renaming here have it only because size is private and it may leak into the abi with some flavors of linking. Otherwise, they're well documented and unlikely to see any incompatible changes.
Other symbols, e.g., _nettle_sha256_compress and _nettle_umac_nh, are internal interfaces which might change if nettle gets new or different optimizations.
We can list the ones that are unlike to see incompatible changes and are useful in applications explicitly, and move any others to either local, or to a private version.
And there are different kinds of possible uses:
From within the nettle library.
From test and benchmarking executables in Nettle.
From user's experimental code, which don't care much about api or
abi compatibilities with other versions.
Statically linked binaries, e.g, on embedded systems, might access _nettle_secp_256r1 directly to avoid an extra level of indirection and runtime lookup, with no real problem.
Plain applications relying on both api and abi stability.
We need to allow (1) (at least for some of the _nettle_* symbols). We need to strongly discourage (4). For (2) and (3), it would be nice to be liberal, but it might be fine to simply export more symbols in the static library.
Removing declarations from internal header files would allow (1) and (2), and strongly discourage all other use (if anyone adds their own declarations to be able to call internal functions in a library they are using, they ought to understand they're in unsupported territory).
What would it take to hide all _nettle symbols in libnettle.se? Just delete the _nettle_* line in libnettle.map.in
Yes.
regards, Nikos
Nikos Mavrogiannopoulos nmav@redhat.com writes:
Another approach could be to put them into a version which has variable (per release name). That would allow linking with them, but will always break software that uses them.
I think I like this. Should be easy to implement, document and understand.
A version like
NETTLE_INTERNAL_@PACKAGE_VERSION@
in libnettle.map.in would do, I guess? By definition, this gets a new value for every release, and we don't need to care about structure of the version string (major, minor, patchlevel, rcX, whatever).
I guess I'd need to read up a bit on how linker scripts work.
Other symbols, e.g., _nettle_sha256_compress and _nettle_umac_nh, are internal interfaces which might change if nettle gets new or different optimizations.
We can list the ones that are unlike to see incompatible changes and are useful in applications explicitly, and move any others to either local, or to a private version.
Right, we can decide case by case if those symbols should be in installed headers (I think that's ok for the hash *_compress functions), and if so, whether or not they should be document beyond header comments.
Regards, /Niels
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
It has but the detection is done through major and minor lib version. Are they updated?
Hmm, I'm looking at
https://gitlab.com/gnutls/gnutls/blob/master/lib/nettle/pk.c, line 1066:
#if NETTLE_MAJOR_VERSION < 3 || (NETTLE_MAJOR_VERSION == 3 && NETTLE_MINOR_VERSION < 4)
That should use the old symbols only prior to nettle-3.4, which seems correct, since the new accessors were introduced in nettle-3.4. But the rename-data-symbols branch also carries version 3.4, so the failure
https://gitlab.com/gnutls/nettle/-/jobs/57981115
is a bit strange. Maybe it's missing an #include <nettle/version.h> ?
It would be good to always compile with -Wundef, but that might break other checks.
Regards, /Niels
On Mon, 2018-03-19 at 23:08 +0100, Niels Möller wrote:
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
It has but the detection is done through major and minor lib version. Are they updated?
Hmm, I'm looking at
https://gitlab.com/gnutls/gnutls/blob/master/lib/nettle/pk.c, line 1066:
#if NETTLE_MAJOR_VERSION < 3 || (NETTLE_MAJOR_VERSION == 3 && NETTLE_MINOR_VERSION < 4)
That should use the old symbols only prior to nettle-3.4, which seems correct, since the new accessors were introduced in nettle-3.4. But the rename-data-symbols branch also carries version 3.4, so the failure
https://gitlab.com/gnutls/nettle/-/jobs/57981115
is a bit strange. Maybe it's missing an #include <nettle/version.h> ?
That's very strange. bignum.h already includes version.h, so that should have been ok. I've also explicitly added it and re-run with the same results: https://gitlab.com/gnutls/nettle/-/jobs/58340605
What is even more strange is that gnutls in fedora is already using the new functions, and we got two bugs about it already: https://bugzilla.redhat.com/show_bug.cgi?id=1556890 https://bugzilla.redhat.com/show_bug.cgi?id=1549190
regards, Nikos
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
is a bit strange. Maybe it's missing an #include <nettle/version.h> ?
That's very strange. bignum.h already includes version.h, so that should have been ok. I've also explicitly added it and re-run with the same results: https://gitlab.com/gnutls/nettle/-/jobs/58340605
I'll try reproducing locally.
I hade a quick look at the config.log for clues. I noticed that the test
if test "$enable_non_suiteb" = "yes";then dnl nettle_secp_192r1 is not really a function AC_CHECK_LIB(hogweed, nettle_secp_192r1, enable_non_suiteb=yes, enable_non_suiteb=no, [$HOGWEED_LIBS])
fails, but that's an unrelated problem.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
is a bit strange. Maybe it's missing an #include <nettle/version.h> ?
That's very strange. bignum.h already includes version.h, so that should have been ok. I've also explicitly added it and re-run with the same results: https://gitlab.com/gnutls/nettle/-/jobs/58340605
I'll try reproducing locally.
It turns out it was a typo, with below fix, compilation works fine:
--- a/lib/nettle/pk.c +++ b/lib/nettle/pk.c @@ -1062,7 +1062,7 @@ _wrap_nettle_pk_verify(gnutls_pk_algorithm_t algo, return ret; }
-#if NETTLE_MAJOR_VERSION < 3 || (NETTLE_MAJOR_VERSION == 3 && NETTLE_MINOR_VERSION < 4) +#if NETTLE_VERSION_MAJOR < 3 || (NETTLE_VERSION_MAJOR == 3 && NETTLE_VERSION_MINOR < 4) # ifdef ENABLE_NON_SUITEB_CURVES # define nettle_get_secp_192r1() &nettle_secp_192r1 # define nettle_get_secp_224r1() &nettle_secp_224r1
The compilation flag -Wundef would have caught this.
Regards, /Niels
On Sun, 2018-03-25 at 10:37 +0200, Niels Möller wrote:
nisse@lysator.liu.se (Niels Möller) writes:
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
is a bit strange. Maybe it's missing an #include <nettle/version.h> ?
That's very strange. bignum.h already includes version.h, so that should have been ok. I've also explicitly added it and re-run with the same results: https://gitlab.com/gnutls/nettle/-/jobs/58340605
I'll try reproducing locally.
It turns out it was a typo, with below fix, compilation works fine:
--- a/lib/nettle/pk.c +++ b/lib/nettle/pk.c @@ -1062,7 +1062,7 @@ _wrap_nettle_pk_verify(gnutls_pk_algorithm_t algo, return ret; }
-#if NETTLE_MAJOR_VERSION < 3 || (NETTLE_MAJOR_VERSION == 3 && NETTLE_MINOR_VERSION < 4) +#if NETTLE_VERSION_MAJOR < 3 || (NETTLE_VERSION_MAJOR == 3 && NETTLE_VERSION_MINOR < 4) # ifdef ENABLE_NON_SUITEB_CURVES # define nettle_get_secp_192r1() &nettle_secp_192r1 # define nettle_get_secp_224r1() &nettle_secp_224r1
Thank you. I've committed a fix at: https://gitlab.com/gnutls/gnutls/merge_requests/614
The compilation flag -Wundef would have caught this.
I have never tried it, but it looks easy to fail on legal scenarios. If for example we compile with any version of nettle 3.1 or later (3.1 doesn't have version.h), that macro would make it fail. Extrapolating it to more dependencies would make a valid run extremely likely to fail compilation.
regards, Nikos
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
Thank you. I've committed a fix at: https://gitlab.com/gnutls/gnutls/merge_requests/614
Good. And you'll trigger a new test run when it's in?
The compilation flag -Wundef would have caught this.
I have never tried it, but it looks easy to fail on legal scenarios. If for example we compile with any version of nettle 3.1 or later (3.1 doesn't have version.h),
I'm not sure how hard it would be to migrate to building with -Wundef (I haven't yet tried it in Nettle or any of my own projects). But it's of course still possible check if a preprocessor symbol is defined, one just has to use #ifdef FOO or #if defined(FOO) where appropriate.
I think it's very rare that the implicit expansion of undefined symbols to 0 (in the context of the preprocessor evaluating the value of an #if) really is important or desired.
A potential problem is the conventions around autoconf AC_DEFINE, where symbols are usually either undefined or defined with the value 1. So with -Wundef, I think one would need to use
#ifdef HAVE_FOO
rahter than
#if HAVE_FOO
(unless there's some recent features to change config.h convention, to always define the symbols, to 0 or 1 depending on corresponding test).
Regards, /Niels
On Sun, 2018-03-25 at 21:22 +0200, Niels Möller wrote:
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
Thank you. I've committed a fix at: https://gitlab.com/gnutls/gnutls/merge_requests/614
Good. And you'll trigger a new test run when it's in?
Yes. It is at: https://gitlab.com/gnutls/nettle/-/jobs/59417145
and seems to pass. Thank you.
regards, Nikos
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
Yes. It is at: https://gitlab.com/gnutls/nettle/-/jobs/59417145
and seems to pass. Thank you.
Good!
Change now merged to master-updates.
Regards, /Niels
nettle-bugs@lists.lysator.liu.se
-
Dmitry Eremin-Solenikov -
Jeffrey Walton -
Nikos Mavrogiannopoulos -
Nikos Mavrogiannopoulos -
nisse＠lysator.liu.se