-
Notifications
You must be signed in to change notification settings - Fork 61
Conversation
if(TEST_PKCS11_MODULE_PATH) | ||
add_definitions(-DTEST_PKCS11_MODULE_PATH="${TEST_PKCS11_MODULE_PATH}" | ||
-DTEST_PKCS11_ENGINE_PATH="${TEST_PKCS11_ENGINE_PATH}") | ||
endif(TEST_PKCS11_MODULE_PATH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I reading this right that you've basically moved these definitions to only get set in the source files that need them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the intent.
I recommend running the oe-selftests on this before merging since it seems likely to affect them. Looks good in general, though! |
src/libaktualizr/crypto/p11engine.cc
Outdated
static const std::array<boost::filesystem::path, 3> engine_system_paths = { | ||
"/usr/lib/engines-1.1/pkcs11.so", | ||
"/usr/lib/engines/pkcs11.so", | ||
"/usr/lib/x86_64-linux-gnu/engines-1.1/pkcs11.so" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x86_64-linux-gnu? This is not generic across architectures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a small sample of possible locations I've seen (in this case, Ubuntu Bionic x86-64).
The recommended way should be to define PKCS11_ENGINE_PATH
, this is only a helpful fallback. But maybe this is more harmful than useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem is that if you want to cover other common locations this list will end up growing quite a bit as it depends on the distro and architecture used (e.g. bionic armhf/arm64/ppc64 would all have a different linux-gnu path).
Shouldn't necessarily be a problem, but having PKCS11_ENGINE_PATH is probably cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, that's why the list is not intended to be exhaustive, just be helpful for the common desktop testing scenario.
I will at least add some comments or maybe even do that through CMake in a safer way.
Codecov Report
@@ Coverage Diff @@
## master #1471 +/- ##
==========================================
- Coverage 80.6% 80.44% -0.17%
==========================================
Files 184 184
Lines 11082 11083 +1
==========================================
- Hits 8933 8916 -17
- Misses 2149 2167 +18
Continue to review full report at Codecov.
|
I've made some changes to remove the engine detection at runtime. |
Signed-off-by: Laurent Bonnans <[email protected]>
And change the name to not have "TEST" in it, as it is not test specific Signed-off-by: Laurent Bonnans <[email protected]>
Signed-off-by: Laurent Bonnans <[email protected]>
Used in one ancient test Signed-off-by: Laurent Bonnans <[email protected]>
A configure time mechanism is kept for convenience in development environments. It needs to be passed explicitely in cross-compiling (yocto) situations. Signed-off-by: Laurent Bonnans <[email protected]>
Found after trying the real thing.