search

uptane / aktualizr

C++ Uptane Client
Mozilla Public License 2.0
15 stars 15 forks source link

Disable broken p11 tests untill someone can sort them out. #63

Closed pattivacek closed 2 years ago

cajun-rat commented 2 years ago

+1 for doing something about these failing tests.

What does everyone think about going further and ripping out the PKCS11 feature altogether? It has been pretty expensive to carry (extra CI steps, complications to the build process, etc), and given that no-one has fixed the P11 tests I'm suspect that the none of the people who are actively contributing to Aktualizr need it for their use case. That means that either:

Either way, deleting seems like a reasonable option. If there are active users, then please shout!

pattivacek commented 2 years ago

What does everyone think about going further and ripping out the PKCS11 feature altogether?

I've had this thought in the back of my mind as well. I am not interested in maintaining the PKCS#11 feature, and I'm not aware of anyone actively using it. I like the idea, but it requires someone to be willing to actively support it. I tried to debug this recently and I couldn't make sense of it. I suspect something upstream changed, probably in the PKCS#11 library or softhsm.

@mike-sul @doanac @ricardosalveti are y'all still using this feature? I seem to recall seeing some code for it in aktualizr-lite.

mike-sul commented 2 years ago

are y'all still using this feature?

Yes, are. Our tests that use the pkcs11/HSM functionality passes, they are rather integration tests though.

pattivacek commented 2 years ago

Yes, are. Our tests that use the pkcs11/HSM functionality passes, they are rather integration tests though.

Would you be willing to support the related code and tests in aktualizr? Or are they not actually necessary for your needs?

mike-sul commented 2 years ago

Would you be willing to support the related code and tests in aktualizr?

Yes, I/we/Fio will be supporting it.

I'll start working on fixing these failing tests this week, I'll be slow in delivering it though because of my current circumstances (need to arrange life of my family in the new country).

mike-sul commented 2 years ago

@pattivacek What would be the best/correct way to reproduce this issue? Which the given CI tests used to fail at the pkcs11 related tests? Is it Coverage on Ubuntu Bionic?

FYI: I couldn't reproduce it in the context of docker/Dockerfile.ubuntu.focal container and the following build params cmake -DBUILD_P11=1 -DPKCS11_ENGINE_PATH=/usr/lib/x86_64-linux-gnu/engines-1.1/libpkcs11.so -DTEST_PKCS11_MODULE_PATH=/usr/lib/softhsm/libsofthsm2.so ..

pattivacek commented 2 years ago

What would be the best/correct way to reproduce this issue? Which the given CI tests used to fail at the pkcs11 related tests? Is it Coverage on Ubuntu Bionic?

It was failing on every CI run for several months now, so you can look at any recent CI action and see. Here's an example. It is indeed the Coverage on Ubuntu Bionic job. I don't know how best to reproduce it, but I can reproduce it locally, both on my local machine and in docker. I just more or less follow the same steps as CI.

I couldn't reproduce it in the context of docker/Dockerfile.ubuntu.focal container and the following build params cmake -DBUILD_P11=1 -DPKCS11_ENGINE_PATH=/usr/lib/x86_64-linux-gnu/engines-1.1/libpkcs11.so -DTEST_PKCS11_MODULE_PATH=/usr/lib/softhsm/libsofthsm2.so

Can I assume you re-enabled the tests I disabled? Otherwise I can't explain that.

mike-sul commented 2 years ago

Thanks @pattivacek, that's exactly what I was looking for.