Qownnotes: Switch to botan2 from botan

[tim77]

Expected behaviour

Using modern, safe, well maintained libs.

Actual behaviour

Using old, legacy stuff.

[pbek]

Thank you for your concern. Any pull requests coming up?

[tim77]

Unfortunately can't promise this for now. We trying to package it according to Fedora guidelines and push in official repos and there some issues with this old botan happens in Rawhide.

[thmo]

For now, botan has been rebuilt without OpenSSL in Fedora 31 and Rawhide.

Nonetheless, botan v1 has gone EOL 2018-12-31 and is not supported anymore: https://botan.randombit.net/handbook/support.html

[pbek]

Back then I took botan the Botan source (one big CPP file with no dependencies) from QtCreator (see https://github.com/pbek/QOwnNotes/tree/develop/src/libraries/botan), so it should compile just fine in the future.

[pbek]

Qt Creator seems to have switched to Botan 2 with 4.8 and removed it at all in 4.9.

[thmo]

Embedding such an ancient version of a crypto lib doesn't seem to be a good idea to me:
https://botan.randombit.net/security.html

[pbek]

There seems to be no "single file" botan.cpp in https://github.com/qt-creator/qt-creator/tree/4.8/src/libs/3rdparty/botan

[pbek]

Embedding such an ancient version of a crypto lib doesn't seem to be a good idea to me.

We just need it for AES encryption and decryption.
Suggestions and pull requests are welcome.

The new solution needs to work with qmake and cmake, build on all currently supported platforms and distributions and be still able to decrypt encrypted notes from Botan 1.

[tim77]

Also cant build for s390x and ppc64le arches because of bundled botan lib.

[thmo]

I would not expect a compatibility issue regarding notes encrypted with Botan v1.

[pbek]

Also cant build for s390x and ppc64le arches because of bundled botan lib.

I think I also saw the same on the OpenBuildService...

[Waqar144]

I managed to compile the project with Botan2, however, encryption/decryption isn't working (cause it's unable to create the hash).
It's probably a build problem and I will look into it later today.
On a side note, the code required for our particular use case hasn't changed much since botan 1

[pbek]

Great, thank you for taking a look at it!

[Waqar144]

Encryption/Decryption is working now, and is able to decrypt notes from Botan 1.
I will try to decrease the filesize for this library so that we only include modules we need (It's still one big .h and .cpp file + a couple of others)
Will try to send in the PR soon and hopefully @pbek can take a look at it.

[thmo]

Are you going to embed it? If so, why?

[Waqar144]

@thmo Cause it's easier that way and more simple and straightforward. Also, I think it will be more simpler this way when it comes to cross-platform building

[thmo]

@Waqar144 From a distribution perspective, this is discouraged for various reasons (e.g. in case of a security issue, the library can be updated separately).

While I see that embedding can be easier for upstream in certain cases, please foresee an option for distro packagers to use the system-wide installed library instead (and therefore, please also refrain from patching the embedded code).

[Waqar144]

@thmo Not patching the embedded code.
Also, There is an option to use the System wide installed library. Have to test that for botan 2, it's still on botan 1. Maybe you can help with this one?

[pbek]

Thank you for the effort, @Waqar144! The windows build seems to fail currently: https://ci.appveyor.com/project/pbek/qownnotes/builds/28104602/job/was94uyoyuuxppuv

[pbek]

The macOS build also fails: https://travis-ci.org/pbek/QOwnNotes/jobs/597831831

[Waqar144]

:crying_cat_face:
Windows build is failing because Botan is configured for x64. There seems to be no way to configure it to in a way that we have a single file which is configured for both(in my understanding). Reading through Botan 1 code, I didnt find any checks for this.
One solution could be to configure Botan separately for x64 and x86 and then include the files accordingly, maybe

For macOS i don't understand much what's going on.

[pbek]

For macOS i don't understand much what's going on.

It looks like that headers are used that doesn't exist on macOS:
libraries/botan/botan.cpp:27049:12: fatal error: 'sys/auxv.h' file not found

[pbek]

Windows build is failing because Botan is configured for x64.

QON is still built for 32bit because the 64bit build process still doesn't work on AppVeyor and we also need 32bit for Windows XP support (Qt 5.7).

[Waqar144]

It looks like that headers are used that doesn't exist on macOS:
libraries/botan/botan.cpp:27049:12: fatal error: 'sys/auxv.h' file not found

Yeah, I know. I was just surprised because that part of the code shouldn't have been running even but i understand now. Diving into the code a bit more, I think this one is easy to fix. I will have to remove the #defines in botan.h and then define them in qmake according to the underlying os(which the qmake file is already doing i think).

For 32 bit support in windows.. I will configure a separate version of botan and then diff it with the 64 bit one to see the differences and maybe it will not be so hard to settle the differences and have a final single file which supports both 64 and 32 bit.

[pbek]

Awesome, I will test the pull request! (btw. no need for force pushing, we can squash merge all commits) ;)

[pbek]

And I will add some tests for encryption and decryption.

[Waqar144]

Awesome, I will test the pull request! (btw. no need for force pushing, we can squash merge all commits) ;)

Great!
I will keep that in mind!

One more thing,
https://github.com/pbek/QOwnNotes/blob/f02ce8fc6b3162affa9acdf63f57ceadb93c28dc/src/libraries/botan/botan.pri#L8
Should we bump that to 2.12?

The readme needs to be updated as well.

[pbek]

Should we bump that to 2.12?
The readme needs to be updated as well.

yes, please

[thmo]

Meanwhile, there's 2.12.1: https://botan.randombit.net/news.html#version-2-12-1-2019-10-14

[Waqar144]

Meanwhile, there's 2.12.1: https://botan.randombit.net/news.html#version-2-12-1-2019-10-14

Yes, and if you have it installed on your system, QON will compile using that so no need to worry.

Updating the embedded version to 2.12.1 right now is unnecessary in my opinion as there is no minor/major change to the modules we are using i.e., aes, kdf2, pbkdf2 etc.

[pbek]

I wonder how hard it would be to rip out all unused code from the Botan library... :smiley: Compiling Botan every time takes ages on many systems and bulks up the binary.

[Waqar144]

Haha! well i think it would require _some effort_ but it's not impossible. :thinking: :smile:

First, note down of everything we are using from botan, then note down all the dependent functions. Then we could put all of the required functionality into a separate file OR just start removing all the unnecessary functionality bit by bit until we are left with the bare minimum required to get the job done.

Btw, For now I configured botan to use the following modules:

aes rng base base32 base58 base64 pbkdf bigint block cbc cbc_mac checksum cmac codec_filt cpuid crc24 crc32 filters hash hash_id hex hkdf hmac hmac_drbg kdf kdf1 kdf1_iso18033 kdf2 md4 md5 pbkdf pbkdf1 pbkdf2 sha1 sha1_sse2 sha1_x86

Of course there are some modules in there that we are not using and I tried to minimize the build further by removing modules completely but that results in errors when using the library as there are sometimes dependent on each other and the botan docs don't say much about these inter-dependencies. The resulting file size isn't that small either. For a smaller size, doing it by hand is the only option.

Anyways, I am gonna give it a try lol

[pbek]

Haha! well i think it would require some effort but it's not impossible. thinking smile

I wonder if there is some automated tool which can do that, like "tree shaking" in JavaScript terms...

Btw, For now I configured botan to use the following modules

wow, those all are needed the things we do in src/libraries/botan/botanwrapper.cpp?

Anyways, I am gonna give it a try lol

wonderful! :+1:

[pbek]

19.10.6

• the Botan library for encrypting notes was updated to Botan 2.0
  (for #1263, thank you @Waqar144)

[pbek]

There now is a new release, could you please test it and report if it works for you?

[pbek]

@Waqar144 Botan 2 seems to fall apart on other operating systems, e.g. see: https://build.opensuse.org/package/live_build_log/home:pbek:QOwnNotes/desktop/openSUSE_13.2/i586

[pbek]

You can see the results on https://build.opensuse.org/package/show/home:pbek:QOwnNotes/desktop

[pbek]

All other architectures but x86_64 seem to fail on some distributions...

[Waqar144]

@Waqar144 Botan 2 seems to fall apart on other operating systems, e.g. see: https://build.opensuse.org/package/live_build_log/home:pbek:QOwnNotes/desktop/openSUSE_13.2/i586

So it seems... This time around it's not telling much about why it's failing. At least in this case.

Some of them are failing because of different architecture which doesn't support sse2.

Shaking the tree and shedding the dead code will fix most of these issues. Probably.

i586 is building fine on debian and fedora but failing on openSUSE.
armv7l and aarch64 are failing everywhere.

[pbek]

I restarted above mentioned build.

[thmo]

I wonder if there is some automated tool which can do that, like "tree shaking" in JavaScript terms...

Maybe @randombit (botan author) can advise.

[Waqar144]

Alright so I just read the log files and it can be fixed very simply using:

We can just comment out these lines:
https://github.com/pbek/QOwnNotes/blob/38dc7f5b798506f7ed4c7fd015b3b9cdd6415cb1/src/libraries/botan/botan.h#L101-L104

and move
https://github.com/pbek/QOwnNotes/blob/38dc7f5b798506f7ed4c7fd015b3b9cdd6415cb1/src/libraries/botan/botan.h#L92
to the botan.pri file

and wrap

#if defined(BOTAN_TARGET_CPU_IS_X86_FAMILY)
#include <emmintrin.h>

namespace Botan {
...
}

#endif

and i think that would fix it.

OR

We can just properly define stuff in qmake so that the right code is executed and include the QMAKE_CXXFLAG += -msse2 flag explicitly

[pbek]

Would the latter (msse2) work on all platforms? I doubt that.

[Waqar144]

it wouldn't, that's why we can just include it for linux on x86
msse2 is an intel only flag

[pbek]

Do you care to prepare something I can test in the next release on the open build service? 😄

[Waqar144]

Well i disabled the sse stuff and some other extra modules that weren't being used. Lets see how it goes on the open build service

[pbek]

19.10.7

• an attempt was made to make the Botan 2.12 library work on non-SSE platforms
  (for #1263, thank you @Waqar144)

[pbek]

Btw. I get a lot of warnings when I build the Botan library, is that "normal"?

[pbek]

There now is a new release, let's wait and see how it builds on https://build.opensuse.org/package/show/home:pbek:QOwnNotes/desktop

[pbek]

First results are coming in: openSUSE_Leap_42.3_Ports/aarch64 is still failing.
https://build.opensuse.org/package/live_build_log/home:pbek:QOwnNotes/desktop/openSUSE_Leap_42.3_Ports/aarch64

[   87s] ../libraries/botan/botan.cpp:7401:21: fatal error: cpuid.h: No such file or directory
[   87s]    #include <cpuid.h>

[pbek]

Same error on https://build.opensuse.org/package/live_build_log/home:pbek:QOwnNotes/desktop/openSUSE_Leap_42.2_Ports/armv7l

[Waqar144]

Btw. I get a lot of warnings when I build the Botan library, is that "normal"?

warning: this header will be made internal in the future

most of these aren't something i would worry about.

Are you getting the implicit conversion warnings? :thinking:
I dont see them on the Open Build

---

First results are coming in: openSUSE_Leap_42.3_Ports/aarch64 is still failing.
https://build.opensuse.org/package/live_build_log/home:pbek:QOwnNotes/desktop/openSUSE_Leap_42.3_Ports/aarch64

[   87s] ../libraries/botan/botan.cpp:7401:21: fatal error: cpuid.h: No such file or directory
[   87s]    #include <cpuid.h>

cupid.h not being supported by arm...
Solution could be:

    #if defined(BOTAN_TARGET_CPU_IS_X86_FAMILY)
        #include <cpuid.h>
    #endif

but hey, at least the 32 bit openSUSE is working now. :smile:

[randombit]

Hi - Botan upstream author here. There are several questions here I need to address that I will try to look at soon. But one thing to mention is that (AIUI) you are building an amalgamation and then needing to compile it on multiple CPUs. That isn't something that is well supported by the amalgamation currently. But I just added a commit which creates a "generic" CPU target which results in code that should compile everywhere, using --cpu=generic. https://github.com/randombit/botan/commit/be80cd7c69eef13cb6beb1febcf6e50d1e9bf0ad

This change would backport easily to 2.12 and probably most recent versions.

However be warned that this option disables all of the very useful optimizations that are available on x86, ARM etc like use of inline asm, SIMD execution, use of AES instructions, etc. So performance will be quite terrible (on my x86-64 machine I saw a 5x reduction in RSA performance, for example). It would be better if you can use the distro installed version which will have all the CPU specific features enabled [maybe you already do that and use the amalgamation only as a fallback, idk]

[Waqar144]

But I just added a commit which creates a "generic" CPU target which results in code that should compile everywhere, using --cpu=generic. randombit/botan@be80cd7

This change would backport easily to 2.12 and probably most recent versions.

Thanks for taking the time to reply. This sounds super useful, I will take a look at it immediately.

However be warned that this option disables all of the very useful optimizations that are available on x86, ARM etc like use of inline asm, SIMD execution, use of AES instructions, etc. So performance will be quite terrible (on my x86-64 machine I saw a 5x reduction in RSA performance, for example).

The use case here is just AES. We aren't using anything else. SSE2 and other versions plus AES-NI is already disabled(for now). It's not that I can't keep them and compile for all architectures, it's just that I don't have any ARM machines(or windows/mac) to test the code quickly. So, the performance penalty is something we'll have to live with for now.
On a side note, I just tested with a note of about 30000 lines. Encryption takes about 1 second. Decryption takes much longer.

(@pbek: Long notes like that are taking too long to load.)

It would be better if you can use the distro installed version which will have all the CPU specific features enabled [maybe you already do that and use the amalgamation only as a fallback, idk]

We do have an option to use the distro installed version and anyone can use that option by setting USE_SYSTEM_BOTAN = 1. Amalgamation is meant to be a fallback however that's not the case right now. Exactly why it's like that, only @pbek could answer. But i think it's because the lack of availability of hardware for testing.

[pbek]

Long notes like that are taking too long to load

I wonder how long it took with Botan 1

We do have an option to use the distro installed version and anyone can use that option by setting USE_SYSTEM_BOTAN = 1.

we would need to search packages for all distributions and hope for the best when building QOwnNotes :/ that's a big hustle...

that's why I used QtCreator's version of Botan back when I added it

[randombit]

I would be interested if you get further results re the slow performance. I doubt AES is the issue; even in a build ignoring the AES instructions and SIMD optimizations, AES runs north of 100 MB/s on my 9 year old T410.

[Waqar144]

I wonder how long it took with Botan 1

Well that can be tested easily, old versions are available still. But I don't think it's a Botan issue. A long note(10K lines+), without any encryption/decryption is taking around 10-15 seconds to load. On top of that, if that was the last note you were looking at before closing QON, the next time you open the app, everything freezes till the note is loaded.

I would be interested if you get further results re the slow performance. I doubt AES is the issue; even in a build ignoring the AES instructions and SIMD optimizations, AES runs north of 100 MB/s on my 9 year old T410.

@randombit
Yes, I agree, since the encryption barely takes a second.

But when decryption is applied the note has to be reloaded, and i think that's where the performance issue occurs.

Edit: The performance issue is happening in QPlainTextEdit::setPlainText(QString)

[pbek]

Regarding the cpuid.h problem on non-x86 platforms

These lines are the same with our Botan 1 version as they are now:
https://github.com/pbek/QOwnNotes/blob/567b27e412afcc64a572708dc6ca8226f87a0d5c/src/libraries/botan/botan.pri#L27-L28

But these lines differ:

https://github.com/pbek/QOwnNotes/blob/567b27e412afcc64a572708dc6ca8226f87a0d5c/src/libraries/botan/botan.cpp#L7400-L7402

previously there was:

#elif defined(BOTAN_BUILD_COMPILER_IS_GCC) && (BOTAN_GCC_VERSION >= 430)

  // Only available starting in GCC 4.3
  #include <cpuid.h>

namespace {

  /*
  * Prevent inlining to work around GCC bug 44174
  */
  void __attribute__((__noinline__)) call_gcc_cpuid(Botan::u32bit type,
                                                    Botan::u32bit out[4])
     {
     __get_cpuid(type, out, out+1, out+2, out+3);
     }

  #define CALL_CPUID call_gcc_cpuid

}

[pbek]

I changed above code back to the Botan 1 version and tried a new release-build. It's still the same, it fails to build on non-x86 platforms.

I've no idea why, any clues @randombit?

[pbek]

All arm builds failed on

• https://build.opensuse.org/package/show/home:pbek:QOwnNotes/desktop
• https://launchpad.net/~pbek/+archive/ubuntu/qownnotes/+packages
• https://code.launchpad.net/~pbek/+snap/qownnotes

[pbek]

On https://build.snapcraft.io/user/pbek/QOwnNotes we can tests commits of QON without releasing.
It also tells us that s390x and ppc64el are failing too.

[Waqar144]

@pbek just comment this line:

https://github.com/pbek/QOwnNotes/blob/567b27e412afcc64a572708dc6ca8226f87a0d5c/src/libraries/botan/botan.h#L92

and add to both these lines separately in botan.pri:

https://github.com/pbek/QOwnNotes/blob/567b27e412afcc64a572708dc6ca8226f87a0d5c/src/libraries/botan/botan.pri#L32-L34

BOTAN_TARGET_CPU_IS_X86_FAMILY

It's failing because on line
https://github.com/pbek/QOwnNotes/blob/567b27e412afcc64a572708dc6ca8226f87a0d5c/src/libraries/botan/botan.cpp#L7394
it checks whether the cpu is x86 or not and since
https://github.com/pbek/QOwnNotes/blob/567b27e412afcc64a572708dc6ca8226f87a0d5c/src/libraries/botan/botan.h#L92
says it is, it goes ahead to include cpuid.h

I think this will solve it.

[pbek]

Any PR coming up? :smile_cat:

[Waqar144]

hahah ok. I will send in a few minutes.

[pbek]

s390x and ppc64el already looking fine on https://build.snapcraft.io/user/pbek/QOwnNotes, great job @Waqar144!

[pbek]

Arm is working fine too, awesome!

[Waqar144]

Yep! I was just looking at that! Finally made it through for all platforms.
To be honest, I wasn't expecting s390x and ppc to go through

[pbek]

What's still left are all the warning: this header will be made internal in the future...

[Waqar144]

Yes, but I don't think they are going to cause any problems. They are just warnings that all this part of code will be made internal (moved into botan_internal?)
The Botan author could advise us on this.

I can remove these warnings however.

[pbek]

Yes please, best lets remove them in our build, they are just causing confusion.

[pbek]

Meanwhile another release is building. We'll see what happens on https://build.opensuse.org/package/show/home:pbek:QOwnNotes/desktop in the next minutes. :smile:

[pbek]

No wonder the last release on OBS didn't work, it was revision 666. :rofl: Now we are at 667!

[Waqar144]

Yes please, best lets remove them in our build, they are just causing confusion.

Alright, let's do that.

Fingers crossed! I am optimistic it will build this time 😉😁

[pbek]

The first arm builds are already succeeding. :tada:

[pbek]

Everything seems to be alright, awesome job, @Waqar144! All jobs ran through!

New glory would await you for example at #125, since you seem to like tricky things. :laughing:

[pbek]

And a big thank you to @randombit for Botan and your concern helping us!

[Waqar144]

Everything seems to be alright, awesome job, @Waqar144! All jobs ran through!

New glory would await you for example at #125, since you seem to like tricky things. laughing

Haha, That's what i was originally intending to work on lol.
That and the Source Code highlighting issue. That's something I really want, at least in the preview(have you thought about using QWebEngine with QWebchannel + a good markdown js library?).

[pbek]

QWebEngine

yes, I did. But there are a lot of issues with it. Starting with the inability to make it build on ApVeyor (Windows) and Travis (for macOS).

[Waqar144]

yes, I did. But there are a lot of issues with it. Starting with the inability to make it build on ApVeyor (Windows) and Travis (for macOS).

I will try that later. Maybe we can find a way around it.

Anyways, on to the spell checker for now.

[randombit]

Yes the warnings are obnoxious sorry about that! Will be fixed in next release. Tracking this in https://github.com/randombit/botan/issues/2164

[pbek]

Great, thank you for all the work!