Cryptoki etas integration - #49
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
80k lines. Sorry, but I'm not able to review that. Is it possible to create smaller, readable PR's? |
|
Kindly check the PR again and resolve the merge conflicts. |
4d92606 to
cc51b96
Compare
|
test_pkcs11_provider fails when compiled with USE_RUST_PKCS11 ("C_InitToken failed: 160"). ./bazel-bin/tests/provider_test/test_pkcs11_provider [----------] Global test environment tear-down |
4e95a25 to
0f08964
Compare
|
The created documentation from the pull request is available at: docu-html |
ab068a8 to
09800f7
Compare
ChansAlive
left a comment
There was a problem hiding this comment.
Review done on the daemon changes as part of integrating the rust pkcs11 provider
| ":use_rust_pkcs11": [ | ||
| "//score/cryptoki:cryptoki_cdylib", | ||
| "//score/cryptoki:cryptoki_headers", | ||
| "//third_party/openssl", |
There was a problem hiding this comment.
I believe this would any way be a transitive dependency when depending on cryptoki_cdylib right?
|
|
||
| #include <cryptoki.h> | ||
| #include <pkcs11.h> | ||
| #ifdef USE_RUST_PKCS11 | ||
| #include <pkcs11.h> | ||
| #else | ||
| #include <cryptoki.h> | ||
| #endif |
There was a problem hiding this comment.
I think it was unnecessary to include the cryptoki.h header in the first place.
You may ignore this comment, I will raise a separate fix later.
| #ifdef USE_RUST_PKCS11 | ||
| { | ||
| CK_ULONG slot_count{0U}; | ||
| CK_FUNCTION_LIST* const fl = m_module->GetFunctionList(); | ||
| CK_RV rv = fl->C_GetSlotList(CK_TRUE, nullptr, &slot_count); | ||
| if ((rv == CKR_OK) && (slot_count > 0U)) | ||
| { | ||
| std::vector<CK_SLOT_ID> slots(slot_count); | ||
| rv = fl->C_GetSlotList(CK_TRUE, slots.data(), &slot_count); | ||
| if ((rv == CKR_OK) && !slots.empty()) | ||
| { | ||
| m_config.slotId = slots.front(); | ||
| score::mw::log::LogWarn() << "[PKCS#11] Warning: token label autodetect failed for '" | ||
| << m_config.tokenLabel | ||
| << "'. Falling back to first present slot id=" << m_config.slotId; | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
What would be the rationale behind this callback?
The intention would be to use the correct token represented by the label (plus Model).
The overall idea of identifying the correct provider would be based on the provider name which is then mapped to the token label in the config to match with the pkcs11 token. If this is not honored, how can we ensure that we are communicating with the correct token when multiple tokens are present in the pkcs11 module?
There was a problem hiding this comment.
yes @ChansAlive, I agree with you. I removed this.
| #ifdef USE_RUST_PKCS11 | ||
| { | ||
| // Hack to modify slot entries since GetSlotEntries returns const | ||
| auto& slot_entries = const_cast<std::vector<score::crypto::daemon::config::KeyConfig::KeySlotEntry>&>( | ||
| config.GetKeyConfig().GetSlotEntries() | ||
| ); | ||
| for (auto& slot : slot_entries) | ||
| { | ||
| for (auto& provider_name : slot.provider_names) | ||
| { | ||
| if (provider_name == "SOFTHSM") | ||
| { | ||
| provider_name = "SCORE_CRYPTO_PROVIDER"; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
I think this is not the right way to use the keyslots for the rust pkcs11 provider. Infact there are three better alternatives
- Use a common provider name for both softhsm and cryptoki. You can then change the pkcs11_token_config.cpp
- Second option is to switch the integration_test_config.json (https://github.com/eclipse-score/inc_security_crypto/blob/main/tests/test_vectors/config/integration_test_config.json) in the BUILD file to use the modified keyslot entries.
- Add new keyslot entries in the existing json. Adapt the test to use the new keyslot resource identifier when rust pkcs11 is selected.
There was a problem hiding this comment.
Thanks @ChansAlive for your feedback, yes I agree with you. refactoring is ready now for generic provider name
| } | ||
| auto& km = km_result.value(); | ||
|
|
||
| auto slot_result = ctx->ResolveResource("HMAC_SHA256_IntegrationTestKey_SoftHSM", ResourceType::kKeySlot); |
There was a problem hiding this comment.
Since we want to choose either softHSM or cryptoki, it may be better to rename the keyslot resourceId to have a generic name. Else it is bit confusing. The change requires updating the config json.
There was a problem hiding this comment.
Are these patch files really required to be merged?
There was a problem hiding this comment.
Hi @ChansAlive, Yes, these patches are required to compile successfully:
- openssl_sys patch: Bypasses host filesystem scanning (which is blocked by Bazel's secure sandbox) to link directly to our built-in //third_party/openssl library.
- Other patches: Fix outdated Rust syntax and Cargo workspace README paths so S-CORE compiles cleanly on modern stable toolchains
There was a problem hiding this comment.
Hi @ShoroukRamzy ,
Thanks for the response.
- Currently, if I understand correctly, the patch hardcodes the version to 3.4 and we already are using 3.6.1, shouldn't we check which version of openssl we are using and adapt the patch to maintain consistency?
- Regarding the score_logging.patch, if this is some change that should ideally be present in the respective module, we should not be patching it here right? Probably if we are using a third_party exclusively used sources, it may be fine. But from the middleware perspective, considering the modules could be shared by multiple modules, how do we expect this patching to work?
Ideally we should try to reduce patches as this might cause issues in the reference integration.
cc: @OliverHeilwagen
There was a problem hiding this comment.
Hi @ChansAlive,
Thank you for your feedback. Regarding the first point, Let me clarify some points:
-
OpenSSL (C++) vs openssl-sys (Rust): S-CORE compiles OpenSSL 3.6.1 (the actual C++ binary), but our Rust bindings crate openssl-sys has no 3.6.x or ossl360 flags in its source code; its highest defined 3.x flag is ossl350.
-
Why we patch (Bazel Sandbox): The unpatched crate’s script scans host paths (like /usr/include), which is blocked inside Bazel's secure sandbox. Our patch simply bypasses this scan to prevent a compile-time crash.
-
Backward Compatibility: If the unpatched script could scan our 3.6.1 dynamically, it would automatically fall back to ossl350. OpenSSL strictly guarantees ABI backward compatibility, making this 3.5.0 compiled subset 100% compatible and safe to run against S-CORE's compiled 3.6.1 binary at runtime.
To maintain perfect consistency, we have updated the patch to output ossl350 and version number 30500000 (3.5.0 fallback).
There was a problem hiding this comment.
@ShoroukRamzy,
I agree with the reason for patching the openssl-sys crate. Please check if all other patches are necessary.
The open point I have is on how to maintain consistency with openssl-sys crate patch and the openssl what we are building automatically. Else we might change (upgrade or downgrade) the openssl version without upgrading the patch.
It would be great if we have a possibility to patch with the maximum supported version of openssl (either of what is supported by crate or what we build) and throw necessary error in case of incompatibility.
Thanks.
There was a problem hiding this comment.
@ChansAlive,
I removed all other patches, I tested without them and they are not needed
There was a problem hiding this comment.
@ChansAlive, Regarding the openssl point, I fully agree with it and I updated the patch to maintain this consistency. Thanks!
There was a problem hiding this comment.
@ChansAlive, Regarding the openssl point, I fully agree with it and I updated the patch to maintain this consistency. Thanks!
@ChansAlive, Done
09800f7 to
2811e99
Compare
ce954fc to
ad1619a
Compare
ad1619a to
d029311
Compare
d029311 to
7918321
Compare
7918321 to
23a7102
Compare
23a7102 to
1711631
Compare
Refactor the openssl_sys_build_rs.patch build script to dynamically parse the compiled OpenSSL version number from S-CORE's opensslv.h header inside the sandboxed build environment.
95cc948 to
8e1dc3d
Compare
Update openssl_sys_build_rs.patch to emit the ossl350 configuration flags and version number 30500000. This provides the correct backward-compatible fallback configuration for S-CORE's compiled OpenSSL 3.6.1.
| + println!("cargo:rustc-cfg=openssl"); | ||
| + | ||
| + // 1. Declare S-CORE's compiled OpenSSL version and the highest supported crate version | ||
| + const S_CORE_OPENSSL_VERSION: &str = "3.6.1"; |
There was a problem hiding this comment.
@ShoroukRamzy Please create a follow up work item, to explore how we can keep this in sync with the actual used openssl version.
Later it should be possible that reference_integration is able to overwrite the openssl version, doing the manual sync will then only be valid in the security_crypto module scope.
| # ******************************************************************************* | ||
| git_override( | ||
| module_name = "rules_rust", | ||
| commit = "2b171a7376e69cb1207eced1f66904ce4ae0c819", |
There was a problem hiding this comment.
What is the difference to "0.68.2-score" ?
Is this a upcomming score version?
| # ******************************************************************************* | ||
| # Valeo - Cryptoki | ||
| # ******************************************************************************* | ||
| git_override( |
There was a problem hiding this comment.
Please move the rust toolchain specific parts to section within this file.
# *******************************************************************************
# Rust toolchains
# *******************************************************************************
|
|
||
| # s-core baselibs | ||
| bazel_dep(name = "score_baselibs", version = "0.2.7") | ||
| bazel_dep(name = "score_baselibs_rust", version = "0.1.2") |
There was a problem hiding this comment.
score_baselibs_rust is deprecated and got merged into score_baselibs with version 0.2.9 please create a follow up issue to bump the version and drop the score_baselibs_rust dependency.
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
|
|
||
| load("@rules_shell//shell:sh_binary.bzl", "sh_binary") |
There was a problem hiding this comment.
please check if this is really needed.
| load("@rules_shell//shell:sh_binary.bzl", "sh_binary") |
| source_dir = ".", | ||
| ) | ||
|
|
||
| package(default_visibility = ["//visibility:public"]) |
There was a problem hiding this comment.
Please apply target specific and keep the default private, also for other BUILD files accordingly.
| # ------------------------------------------------------------------------------- | ||
| # Different toolchain configuration for x86_64-qnx | ||
| # ------------------------------------------------------------------------------- | ||
| build:target_config_1 --config=x86_64-qnx | ||
| build:target_config_1 --extra_toolchains=@score_qcc_x86_64_toolchain//:x86_64-qnx-sdp_8.0.0 | ||
| build:target_config_1 --extra_toolchains=@score_toolchains_rust//toolchains/ferrocene:ferrocene_x86_64_unknown_linux_gnu | ||
| build:target_config_1 --extra_toolchains=@score_toolchains_rust//toolchains/ferrocene:ferrocene_x86_64_pc_nto_qnx800 | ||
|
|
There was a problem hiding this comment.
With the updated bazelrc from main these changes should not longer be necessary.
The build should work with --config=x86_64-qnx.
| # ------------------------------------------------------------------------------- | |
| # Different toolchain configuration for x86_64-qnx | |
| # ------------------------------------------------------------------------------- | |
| build:target_config_1 --config=x86_64-qnx | |
| build:target_config_1 --extra_toolchains=@score_qcc_x86_64_toolchain//:x86_64-qnx-sdp_8.0.0 | |
| build:target_config_1 --extra_toolchains=@score_toolchains_rust//toolchains/ferrocene:ferrocene_x86_64_unknown_linux_gnu | |
| build:target_config_1 --extra_toolchains=@score_toolchains_rust//toolchains/ferrocene:ferrocene_x86_64_pc_nto_qnx800 |
| test:aarch64-qnx --test_lang_filters=cc,rust | ||
| test:aarch64-qnx --test_timeout=180,900,2700,10800 # Increase default test timeout by factor 3 | ||
|
|
||
| # ------------------------------------------------------------------------------- |
There was a problem hiding this comment.
Same point as above the aarch64-qnx config should already be valid.
| # ignore Bazel's own output symlinks | ||
| bazel-inc_security_crypto | ||
| bazel-bin | ||
| bazel-out | ||
| bazel-testlogs |
There was a problem hiding this comment.
Shouldn't this be default bazel behaviour?
| crate = use_extension("@rules_rust//crate_universe:extensions.bzl", "crate", isolate = True) | ||
| crate.from_cargo( |
There was a problem hiding this comment.
Direct create_universe usage in modules seems invalid looking at:
https://github.com/eclipse-score/score-crates
Which raises the question how we handle the openssl dependency in future.
It seems to make more and more sense to move it into a separate repository.
Key Changes:
Integrated the cryptoki module natively into the score_crypto.
Moved the cryptoki provider to score/crypto/daemon/provider/pkcs11/cryptoki.
Updated all Bazel build configurations, Cargo dependencies, and lockfiles to correctly map to the new path.
Verified that the daemon, integration tests, and multi-provider demos build and pass successfully with the new provider location.
Use a single source for openssl (ETAS provided version) found under third_party/openssl
Integrate SCORE Logger into Cryptoki code
closes Integrate PKCS11 (Cryptoki) to SCORE #29 (children tasks PKCS11 (Cryptoki) Integration with SCORE #31 and Integration of PKCS11 (Cryptoki) With ETAS Architecture #32)
closes Have only one version of OpenSSL on target #30 (child task Selecting One OpenSSL Version for the Security Code #33)
Your Reviews will be appreciated @ChansAlive @PandaeDo @masc2023 @schreibwsag @OliverHeilwagen