Repository navigation
Certificate Management Daemon Component - #197
ChansAlive wants to merge 27 commits into
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
Documentation preview for this pull request is available at: |
a2f8104 to
b8ef4f7
Compare
b8ef4f7 to
0dce3dc
Compare
0dce3dc to
ad202e4
Compare
ad202e4 to
ceead4e
Compare
ceead4e to
61322e9
Compare
61322e9 to
57f0cb1
Compare
57f0cb1 to
0f31145
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Missing production wiring and configuration parsing make the subsystem unreachable, while integrity, persistence, and concurrency defects remain unresolved.
Review effort: Balanced
Findings: 6
Open (10)
Certificate service is not wired into daemon startup · New Per-client caches are accessed without synchronization · New Per-slot handler operations are not serialized · New Required certificate hash is never validated · New Trust-store lazy loading and cache updates race · New Certificate configuration schema and parser are incomplete · New Failed certificate slot resolution is silently ignored · New CRL size limit is not enforced during writes · New Fallback certificate path is not persisted · New Legacy file certificate configuration is unsupported · New
What changed in this PR
Implements the certificate-management subsystem for issue #163, including certificate slots, trust stores, CRLs, provider integration, documentation, tests, and test vectors.
Changes:
- Adds certificate lifecycle, trust-store, policy, and DataManager components.
- Adds OpenSSL certificate/CRL parsing and validation support.
- Adds architecture documentation and extensive certificate test assets.
| File | Description |
|---|---|
docs/features/security_crypto/architecture/chklst_arc_inspection.rst |
Moves the architecture checklist. |
docs/features/security_crypto/architecture/index.rst |
Updates feature architecture documentation. |
docs/features/security_crypto/index.rst |
Adds the security feature index. |
docs/features/security_crypto/safety_analysis/aou_requirements.rst |
Adds safety assumptions template. |
docs/features/security_crypto/safety_analysis/dfa.rst |
Adds DFA documentation. |
docs/features/security_crypto/safety_analysis/fmea.rst |
Adds FMEA documentation. |
docs/features/security_crypto/safety_planning/index.rst |
Adds safety-planning documentation. |
docs/features/security_crypto/security_analysis/aou_requirements.rst |
Adds security assumptions template. |
docs/features/security_crypto/security_planning/index.rst |
Adds security-planning documentation. |
index.rst |
Links the security documentation. |
score/crypto/docs/architecture/daemon_architecture.rst |
Documents daemon certificate components. |
score/crypto/docs/architecture/index.rst |
Updates architecture navigation. |
score/crypto/docs/architecture/key_management_architecture.rst |
Updates key-management architecture references. |
score/crypto/docs/index.rst |
Updates crypto documentation navigation. |
score/crypto/src/api/types/certificate.hpp |
Adds public certificate types. |
score/crypto/src/daemon/cert_management/BUILD |
Defines certificate-management targets. |
score/crypto/src/daemon/cert_management/cert_management_module.cpp |
Implements module construction. |
score/crypto/src/daemon/cert_management/cert_management_module.hpp |
Declares the module interface. |
score/crypto/src/daemon/cert_management/core/cert_entry.hpp |
Defines certificate registry entries. |
score/crypto/src/daemon/cert_management/core/cert_management_service.cpp |
Implements certificate operations. |
score/crypto/src/daemon/cert_management/core/cert_management_service.hpp |
Declares the certificate service. |
score/crypto/src/daemon/cert_management/core/cert_registry.cpp |
Implements certificate registration. |
score/crypto/src/daemon/cert_management/core/cert_registry.hpp |
Declares the certificate registry. |
score/crypto/src/daemon/cert_management/core/cert_registry_test.cpp |
Tests certificate registration. |
score/crypto/src/daemon/cert_management/docs/architecture/cert_management_static.puml |
Adds the static architecture diagram. |
score/crypto/src/daemon/cert_management/docs/architecture/component_architecture.rst |
Documents component architecture. |
score/crypto/src/daemon/cert_management/docs/architecture/design_decisions.rst |
Records design decisions. |
score/crypto/src/daemon/cert_management/docs/architecture/index.rst |
Adds architecture navigation. |
score/crypto/src/daemon/cert_management/docs/detailed_design/cert_management_dynamic.puml |
Adds dynamic behavior diagrams. |
score/crypto/src/daemon/cert_management/docs/detailed_design/detailed_design.rst |
Documents detailed design. |
score/crypto/src/daemon/cert_management/docs/detailed_design/index.rst |
Adds detailed-design navigation. |
score/crypto/src/daemon/cert_management/docs/index.rst |
Adds component documentation index. |
score/crypto/src/daemon/cert_management/docs/requirements/index.rst |
Adds requirements navigation. |
score/crypto/src/daemon/cert_management/docs/requirements/requirements.rst |
Adds component requirements. |
score/crypto/src/daemon/cert_management/interfaces/access_policy.hpp |
Defines access policies. |
score/crypto/src/daemon/cert_management/interfaces/cert_object.hpp |
Defines certificate objects. |
score/crypto/src/daemon/cert_management/interfaces/cert_slot_config.hpp |
Defines slot configuration. |
score/crypto/src/daemon/cert_management/interfaces/cert_types.hpp |
Defines internal certificate types. |
score/crypto/src/daemon/cert_management/interfaces/i_cert_slot_catalog.hpp |
Defines the slot-catalog interface. |
score/crypto/src/daemon/cert_management/interfaces/i_cert_slot_handler.cpp |
Implements handler helpers. |
score/crypto/src/daemon/cert_management/interfaces/i_cert_slot_handler.hpp |
Defines the slot-handler interface. |
score/crypto/src/daemon/cert_management/interfaces/i_trust_store_handler.hpp |
Defines the trust-store interface. |
score/crypto/src/daemon/cert_management/interfaces/trust_store_config.hpp |
Defines trust-store configuration. |
score/crypto/src/daemon/cert_management/nodes/cert_data_node.hpp |
Adds certificate data nodes. |
score/crypto/src/daemon/cert_management/nodes/cert_slot_data_node.hpp |
Adds slot data nodes. |
score/crypto/src/daemon/cert_management/nodes/trust_store_data_node.hpp |
Adds trust-store data nodes. |
score/crypto/src/daemon/cert_management/policy/access_policy_enforcer.cpp |
Implements access checks. |
score/crypto/src/daemon/cert_management/policy/access_policy_enforcer.hpp |
Declares policy enforcement. |
score/crypto/src/daemon/cert_management/policy/access_policy_enforcer_test.cpp |
Tests access policies. |
score/crypto/src/daemon/cert_management/query/cert_object_response_builder.cpp |
Builds certificate IPC responses. |
score/crypto/src/daemon/cert_management/query/cert_object_response_builder.hpp |
Declares response construction. |
score/crypto/src/daemon/cert_management/query/cert_object_response_builder_test.cpp |
Tests response construction. |
score/crypto/src/daemon/cert_management/slot/cert_slot_manager.cpp |
Implements slot lifecycle management. |
score/crypto/src/daemon/cert_management/slot/cert_slot_manager.hpp |
Declares the slot manager. |
score/crypto/src/daemon/cert_management/slot/config_driven_slot_catalog.cpp |
Implements configured slot discovery. |
score/crypto/src/daemon/cert_management/slot/config_driven_slot_catalog.hpp |
Declares the slot catalog. |
score/crypto/src/daemon/cert_management/slot/crl_handler.cpp |
Implements CRL storage. |
score/crypto/src/daemon/cert_management/slot/crl_handler.hpp |
Declares CRL handling. |
score/crypto/src/daemon/cert_management/slot/deployment_loader.cpp |
Implements deployment loading. |
score/crypto/src/daemon/cert_management/slot/deployment_loader.hpp |
Declares deployment loading. |
score/crypto/src/daemon/cert_management/slot/deployment_writer.cpp |
Implements deployment persistence. |
score/crypto/src/daemon/cert_management/slot/deployment_writer.hpp |
Declares deployment persistence. |
score/crypto/src/daemon/cert_management/slot/file_backed_slot_handler.cpp |
Implements file-backed slots. |
score/crypto/src/daemon/cert_management/slot/file_backed_slot_handler.hpp |
Declares file-backed slots. |
score/crypto/src/daemon/cert_management/slot/file_backed_slot_handler_test.cpp |
Tests file-backed slots. |
score/crypto/src/daemon/cert_management/slot/slot_handler_factory.cpp |
Selects slot backends. |
score/crypto/src/daemon/cert_management/slot/slot_handler_factory.hpp |
Declares backend selection. |
score/crypto/src/daemon/cert_management/slot/slot_handler_factory_test.cpp |
Tests backend selection. |
score/crypto/src/daemon/cert_management/slot/slot_registry.cpp |
Implements slot registration. |
score/crypto/src/daemon/cert_management/slot/slot_registry.hpp |
Declares the slot registry. |
score/crypto/src/daemon/cert_management/tests/BUILD |
Defines component test targets. |
score/crypto/src/daemon/cert_management/tests/integration/test_certificate_management_integration.cpp |
Adds integration coverage. |
score/crypto/src/daemon/cert_management/tests/service/test_cert_management_service.cpp |
Tests service operations. |
score/crypto/src/daemon/cert_management/tests/slot/file_backed_slot_handler_test.cpp |
Adds slot-handler scenarios. |
score/crypto/src/daemon/cert_management/tests/test_environment.hpp |
Provides shared test setup. |
score/crypto/src/daemon/cert_management/tests/truststore/test_trust_store_manager.cpp |
Tests trust-store management. |
score/crypto/src/daemon/cert_management/truststore/config_driven_trust_store_catalog.cpp |
Implements trust-store discovery. |
score/crypto/src/daemon/cert_management/truststore/config_driven_trust_store_catalog.hpp |
Declares trust-store discovery. |
score/crypto/src/daemon/cert_management/truststore/trust_store_handler.cpp |
Implements trust-store caching. |
score/crypto/src/daemon/cert_management/truststore/trust_store_handler.hpp |
Declares trust-store handling. |
score/crypto/src/daemon/cert_management/truststore/trust_store_manager.cpp |
Implements trust-store lifecycle. |
score/crypto/src/daemon/cert_management/truststore/trust_store_manager.hpp |
Declares the trust-store manager. |
score/crypto/src/daemon/common/daemon_error.hpp |
Extends daemon errors. |
score/crypto/src/daemon/common/storage/kv/kv_deployment_writer.cpp |
Extends KV persistence. |
score/crypto/src/daemon/common/types.hpp |
Adds shared certificate types. |
score/crypto/src/daemon/config/inc/config.hpp |
Adds certificate configuration models. |
score/crypto/src/daemon/data_manager/data_node.hpp |
Extends data-node support. |
score/crypto/src/daemon/data_manager/docs/index.rst |
Updates DataManager documentation. |
score/crypto/src/daemon/provider/cert_management/BUILD |
Defines provider interfaces. |
score/crypto/src/daemon/provider/cert_management/cert_management_operations.hpp |
Defines provider operation IDs. |
score/crypto/src/daemon/provider/cert_management/cert_types.hpp |
Defines provider certificate types. |
score/crypto/src/daemon/provider/cert_management/i_cert_parser.hpp |
Defines parser capabilities. |
score/crypto/src/daemon/provider/i_provider.hpp |
Exposes certificate parsing. |
score/crypto/src/daemon/provider/score_provider/openssl/BUILD |
Builds OpenSSL certificate support. |
score/crypto/src/daemon/provider/score_provider/openssl/cert_management/openssl_cert_parser.cpp |
Implements OpenSSL parsing. |
score/crypto/src/daemon/provider/score_provider/openssl/cert_management/openssl_cert_parser.hpp |
Declares OpenSSL parsing. |
score/crypto/src/daemon/provider/score_provider/openssl/detail/openssl_cert_utils.cpp |
Implements OpenSSL utilities. |
score/crypto/src/daemon/provider/score_provider/openssl/detail/openssl_cert_utils.hpp |
Declares OpenSSL utilities. |
score/crypto/src/daemon/provider/score_provider/openssl/provider_openssl.cpp |
Registers certificate capabilities. |
score/crypto/src/daemon/provider/score_provider/openssl/provider_openssl.hpp |
Extends the OpenSSL provider. |
score/crypto/src/daemon/provider/tests/cert_management/BUILD |
Defines provider parser tests. |
score/crypto/src/daemon/provider/tests/cert_management/test_openssl_cert_parser.cpp |
Tests OpenSSL certificate parsing. |
score/tests/test_vectors/certificate/BUILD |
Packages certificate vectors. |
score/tests/test_vectors/certificate/README.md |
Documents vector generation. |
score/tests/test_vectors/certificate/algorithm_variety/ec_p256.pem |
Adds a P-256 certificate. |
score/tests/test_vectors/certificate/algorithm_variety/ec_p256_slot.kv |
Configures the P-256 slot. |
score/tests/test_vectors/certificate/algorithm_variety/ec_p384.pem |
Adds a P-384 certificate. |
score/tests/test_vectors/certificate/algorithm_variety/ec_p384_slot.kv |
Configures the P-384 slot. |
score/tests/test_vectors/certificate/algorithm_variety/ec_p521.pem |
Adds a P-521 certificate. |
score/tests/test_vectors/certificate/algorithm_variety/ec_p521_slot.kv |
Configures the P-521 slot. |
score/tests/test_vectors/certificate/algorithm_variety/ed25519.pem |
Adds an Ed25519 certificate. |
score/tests/test_vectors/certificate/algorithm_variety/ed25519_slot.kv |
Configures the Ed25519 slot. |
score/tests/test_vectors/certificate/algorithm_variety/ed448.pem |
Adds an Ed448 certificate. |
score/tests/test_vectors/certificate/algorithm_variety/ed448_slot.kv |
Configures the Ed448 slot. |
score/tests/test_vectors/certificate/algorithm_variety/manifest.json |
Defines algorithm vectors. |
score/tests/test_vectors/certificate/algorithm_variety/ml_dsa_44.pem |
Adds an ML-DSA-44 certificate. |
score/tests/test_vectors/certificate/algorithm_variety/ml_dsa_44_slot.kv |
Configures the ML-DSA-44 slot. |
score/tests/test_vectors/certificate/algorithm_variety/ml_dsa_65.pem |
Adds an ML-DSA-65 certificate. |
score/tests/test_vectors/certificate/algorithm_variety/ml_dsa_65_slot.kv |
Configures the ML-DSA-65 slot. |
score/tests/test_vectors/certificate/algorithm_variety/ml_dsa_87.pem |
Adds an ML-DSA-87 certificate. |
score/tests/test_vectors/certificate/algorithm_variety/ml_dsa_87_slot.kv |
Configures the ML-DSA-87 slot. |
score/tests/test_vectors/certificate/algorithm_variety/private/.gitignore |
Excludes generated private keys. |
score/tests/test_vectors/certificate/algorithm_variety/rsa_3072.pem |
Adds an RSA-3072 certificate. |
score/tests/test_vectors/certificate/algorithm_variety/rsa_3072_slot.kv |
Configures the RSA-3072 slot. |
score/tests/test_vectors/certificate/algorithm_variety/rsa_4096.pem |
Adds an RSA-4096 certificate. |
score/tests/test_vectors/certificate/algorithm_variety/rsa_4096_slot.kv |
Configures the RSA-4096 slot. |
score/tests/test_vectors/certificate/basic/certificate.crl.der |
Adds a DER CRL vector. |
score/tests/test_vectors/certificate/basic/certificate.crl.pem |
Adds a PEM CRL vector. |
score/tests/test_vectors/certificate/basic/certificate.pem |
Adds the basic CA certificate. |
score/tests/test_vectors/certificate/basic/certificate_leaf.chain.pem |
Adds a certificate chain. |
score/tests/test_vectors/certificate/basic/certificate_leaf.der |
Adds a DER leaf certificate. |
score/tests/test_vectors/certificate/basic/certificate_leaf.pem |
Adds a PEM leaf certificate. |
score/tests/test_vectors/certificate/basic/certificate_leaf_slot.kv |
Configures the leaf slot. |
score/tests/test_vectors/certificate/basic/certificate_slot.kv |
Configures the basic slot. |
score/tests/test_vectors/certificate/basic/certificate_updated.pem |
Adds a rotated certificate. |
score/tests/test_vectors/certificate/basic/certificate_updated_slot.kv |
Configures the rotated slot. |
score/tests/test_vectors/certificate/basic/manifest.json |
Defines basic PKI vectors. |
score/tests/test_vectors/certificate/basic/private/.gitignore |
Excludes generated private keys. |
score/tests/test_vectors/certificate/basic/trust_store.kv |
Adds trust-store state. |
score/tests/test_vectors/certificate/generate_certificates.py |
Generates certificate artifacts. |
score/tests/test_vectors/certificate/pki_chain/manifest.json |
Defines chain and OCSP vectors. |
score/tests/test_vectors/certificate/pki_chain/ocsp_signer.ext.conf |
Configures OCSP signing. |
tools/coverage/BUILD |
Extends certificate coverage scope. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| CertManagementModule::Sptr CertManagementModule::Create(data_manager::IDataManager::Sptr data_manager, | ||
| provider::ProviderManager::Sptr provider_manager, | ||
| const config::CertificateConfig& config) |
There was a problem hiding this comment.
This is not yet in scope of this PR. The further PR will wire the cert management component in daemon.
| if (!handler) | ||
| return score::crypto::make_unexpected(Error::kInternalError); | ||
|
|
||
| auto cert_obj = handler->LoadCertificate(cfg); |
| { | ||
| const auto hash = descriptor->Get("certificate", "cert_hash"); | ||
| if (hash.empty()) | ||
| return score::crypto::make_unexpected(Error::kInvalidArgument); | ||
| // The descriptor hash is validated by the certificate verification layer. |
| if (!m_loaded) | ||
| { | ||
| if (m_loader) | ||
| m_loader(*this); | ||
| m_loaded = true; |
| /// @brief Add a certificate slot definition (called by parser). | ||
| void AddSlotEntry(CertSlotEntry entry) | ||
| { | ||
| m_slot_entries.push_back(std::move(entry)); |
| auto nid_res = service.ResolveCertSlot(slot, client_id); | ||
| if (!nid_res.has_value()) | ||
| continue; // slot not resolvable — omit silently rather than failing | ||
| slot_node_ids.push_back(static_cast<std::uint64_t>(nid_res.value())); |
There was a problem hiding this comment.
The refactor introduced Expected<> as return, please wire the error as in BuildTrustStoreMemberObjectResponse.
This would also allow to push the slots.size() as first parameter and then the node_ids.
| if (data.empty()) | ||
| return score::crypto::make_unexpected(Error::kInvalidArgument); |
| auto path = descriptor->Get("certificate", "cert_path"); | ||
| if (path.empty()) | ||
| path = slot.deployment_path + ".pem"; |
| if (slot.storage_backend == "DEFAULT") | ||
| { | ||
| return std::make_shared<FileBackedSlotHandler>(std::move(cert_parser)); | ||
| } |
OliverHeilwagen
left a comment
There was a problem hiding this comment.
review snapshot 121/143
- truststore code review is pending.
| /// @brief Build a CertObject's chain metadata into an IPC response payload. | ||
| /// | ||
| /// Wire layout (15 params, indices 0–14): | ||
| /// [0] subject (OwnedString), [1] issuer (OwnedString), |
There was a problem hiding this comment.
Please improve layouting (each parameter in one line) if necessary disable clang-tidy for the description.
This also applies to the other method descriptions.
| /// The subject and issuer DNs are the only fields of unbounded size (X.509 | ||
| /// does not cap Name length). Returns kResponseTooLarge instead of an | ||
| /// oversized payload if the built response exceeds | ||
| /// common::kMaxResponsePayloadBytes. |
| // ============================================================================ | ||
| // Response payload budget | ||
| // ============================================================================ | ||
|
|
There was a problem hiding this comment.
Please add a todo to move it later to score/crypto/src/ipc, doing this now makes no sense as the IPC switch to lola is going on in parallel.
@se-lo: fyi
| { | ||
| CertRegistry registry; |
There was a problem hiding this comment.
| { | |
| CertRegistry registry; | |
| { | |
| RecordProperty("PartiallyVerifies", "comp_req__crypto_cert_management__parse"); | |
| RecordProperty("TestType", "interface-test"); | |
| RecordProperty("DerivationTechnique", "requirements-analysis"); | |
| RecordProperty("Description", | |
| "..."); | |
| CertRegistry registry; |
Other test cases were we demonstrate registering or retreiving of CertObjects can be linked similar
| { | ||
| CertSlotConfig slot; |
There was a problem hiding this comment.
| { | |
| CertSlotConfig slot; | |
| { | |
| RecordProperty("PartiallyVerifies", "comp_req__crypto_cert_mgmt__access_control"); | |
| RecordProperty("TestType", "interface-test"); | |
| RecordProperty("DerivationTechnique", "requirements-analysis"); | |
| RecordProperty("Description", | |
| "...."); | |
| CertSlotConfig slot; |
Please link test which contribute to client based authorization similar.
| using Sptr = std::shared_ptr<ICertParser>; | ||
| virtual ~ICertParser() = default; | ||
|
|
||
| /// Parse a single DER/PEM certificate into a neutral CertObject. |
There was a problem hiding this comment.
Please add missing param and return value description for all methods.
| const std::uint8_t* crl_data{nullptr}; | ||
| std::size_t crl_data_size{0}; |
| /// Chain-verification inputs. All certificates are provider-neutral CertObjects; | ||
| /// the service resolves client CryptoResourceIds to CertObjects before calling | ||
| /// the provider, so the provider surface never sees provider-bound cert handles. | ||
| struct VerificationRequest |
There was a problem hiding this comment.
Please add RevcoationCoveragePolicy and VerificationEvidenceMode.
|
|
||
| TEST(OpenSslCertParserTest, ParsesPemCertificateAndExtractsMetadata) | ||
| { | ||
| const auto pem = ReadCertificateVector(); |
There was a problem hiding this comment.
Please add PartiallyVerifies link to comp_req__crypto_cert_management__parse
|
|
||
| cc_test( | ||
| name = "test_openssl_cert_parser", | ||
| srcs = ["test_openssl_cert_parser.cpp"], |
There was a problem hiding this comment.
Please rename and move next to openssl_cert_parser.cpp, the same name can be used with suffix _test.cpp.
| }; | ||
|
|
||
| /// Effective status of a trust store member as seen at snapshot time. | ||
| enum class MemberStatus : uint8_t |
There was a problem hiding this comment.
looks for me a mixture of status and reason, like kEnabled is status and kFingerprintMismatch is a failure reasons ?
There was a problem hiding this comment.
MemberStatus describes the member’s effective availability in a trust-store snapshot, not the outcome of an operation. kFingerprintMismatch and kAwaitingAcknowledgement both mean the member is unavailable as an anchor; kDisabled represents an explicit administrative disablement. For conditional-external members, the more specific acceptance state is reported when applicable
Adapted the doc to bring clarity
| [[nodiscard]] score::crypto::FormatType GetSessionCrlFormat() const | ||
| { | ||
| const std::lock_guard<std::mutex> lock(m_ref_mutex); | ||
| return m_session_crl_format; |
There was a problem hiding this comment.
it lloks like always a DER format even though crl attached is absent
There was a problem hiding this comment.
changed to std::optional
|
|
||
| [[nodiscard]] data_manager::ClientId GetOwner() const noexcept | ||
| { | ||
| return m_owner_client_id; |
There was a problem hiding this comment.
/// @PARAM object Owning cert object (must not be nullptr).
but null pointer is not checked here. this can hand out a null pointer.
There was a problem hiding this comment.
This is not a claim by the class but a pre condition for the users.
Currently this is ensured by the users within the cert management component before creating the cert entry.
| private: | ||
| CertObject::Sptr m_object; | ||
| CertSlotHandle m_slot_handle; | ||
| data_manager::ClientId m_owner_client_id{0U}; |
There was a problem hiding this comment.
Suppose if someone forgets to pass the owner, then 0 will become default owner we shall make this as a mandatory.
There was a problem hiding this comment.
Have made the owner mandatory.
| score::crypto::Expected<std::monostate, Error> CertManagementService::ReleaseCert(data_manager::ClientId client_id, | ||
| data_manager::DataNodeId node_id) | ||
| { | ||
| if (!m_data_manager->deleteNode(client_id, node_id)) |
There was a problem hiding this comment.
m_cert_slot_node_cache or m_trust_store_node_cache is not refreshed with deleted node.
There was a problem hiding this comment.
This function deletes the certdata node which is completely different from the node types held by the slot node cache and the trust node cache. Hence there is no refresh required.
| if (entry_res.has_value()) | ||
| { | ||
| auto& entry = *entry_res.value(); | ||
| auto session_crl = entry.GetSessionCrl(); |
There was a problem hiding this comment.
For session CRLs, GetSessionCrl(), GetSessionCrlFormat() and GetSessionCrlMetadata() are three separate locked reads. concurrent attach can mix the bytes of one CRL with the format or metadata of another so we need one snapshot
| struct ResolvedCertSlot | ||
| { | ||
| CertSlotHandle handle; | ||
| const CertSlotConfig* config{nullptr}; |
There was a problem hiding this comment.
ResolvedCertSlot::config is exposed as a non-owning raw pointer (const CertSlotConfig*), but the ownership and lifetime guarantees are not documented. It is unclear whether the pointed-to configuration remains valid across registry updates, cache invalidation, or client cleanup operations.
There was a problem hiding this comment.
A successful ResolveSlotForOperation() always resolves the config; otherwise it returns an error. reference_wrapper expresses that the config is required and non-null without copying it. The reference is non-owning and remains valid while the startup-populated, immutable registry lives.
| } | ||
|
|
||
| /// RFC 4514 canonical string representation of the Issuer Distinguished Name. | ||
| [[nodiscard]] std::string_view GetIssuer() const noexcept |
There was a problem hiding this comment.
can this be a issue auto subject =
Load(id).value()->GetSubject(); whaere Load(id) returns temp shared pointer then temporary shared_ptr,,GetSubject(),returns string_view,temporary shared_ptr destroyed,CertEntry destroyed ,string inside destroyed then subject points to freed memory and That's a dangling string_view ??
There was a problem hiding this comment.
It is possible to have the above scenario. But I guess that shall not be an intended usage scenario.
I have added documentation regarding the usage.
| } | ||
|
|
||
| /// Format of the bytes returned by GetRawBytes() (kPem or kDer). | ||
| [[nodiscard]] score::crypto::FormatType GetFormat() const noexcept |
There was a problem hiding this comment.
the validation of headers is not done . Hope it would be good we do validat ethe headers passed/
There was a problem hiding this comment.
If you are intending to parse the contructed bytes to determine the format, I would try to avoid it as it creates additional unnecessary dependancy to the cert parser in the cert object.
Such a check is already done at the cert parser before creating the cert object by verifying the parsing of the cert data with the format specified. So if it is not complaint, it would have already resulted in error.
Moreover, I have also added check to return error during parsing incase of stale remaining bytes are present as part of the pointed buffer after parsing a single certificate.
| namespace score::crypto::daemon::cert_management | ||
| { | ||
|
|
||
| enum class TrustStoreMemberKind : uint8_t |
There was a problem hiding this comment.
missing documentations
| /// | ||
| /// Used by the deployment layer to persist mutable trust-store state, | ||
| /// including exclusive membership and conditional acceptance state. | ||
| std::string deployment_path; |
There was a problem hiding this comment.
If this is null then no enable/disable and accepted-fingerprint
|
|
||
| ~CertDataNode() override | ||
| { | ||
| if (m_cert_entry != nullptr && m_unregister_callback) |
There was a problem hiding this comment.
what if m_unregister_callback is null ? this will not unregister the client.
| if (result.has_value()) | ||
| InvalidateCertObjectCache(slot); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
I would suggest to invoke TrustStoreManager::NotifySlotChanged from here instead of relying on Executor implementation discipline.
The same finding applies to ClearSlot.
I would also expect a similar mechanism for ImportCrl and DeleteCrl, i would have expected e.g. TrustStoreManager::NotifySlotCrlChanged which then invokes NotifyCrlUpdate on the affected TrustStoreHandler.
| /// Fan out NotifyUpdate() to all trust stores referencing the given cert slot. | ||
| /// | ||
| /// Called after a successful StoreCertificate to keep trust store anchors current. | ||
| void NotifySlotCertChanged(CertSlotHandle slot_handle); |
There was a problem hiding this comment.
Ideally this should be handled internally in CertManagmentService and should not require discipline from callee.
Refer to comment in CertSlotManager::StoreCertificate.
There was a problem hiding this comment.
I agree.
I have added the slot mutation APIs to the cert management service and service will then orchestrate the mutation with slot manager and later trust store update.
I won't go with a direct trust store interaction from the slot manager as it adds direct dependency from slot manager to trust store. Orchestration will be done by the cert management service.
Since it is internally handled I will also move the API as private helper
| // Loader lambda is called by TrustStoreHandler::EnsureLoaded() on the first | ||
| // GetAnchors() — never at startup. 'this' is safe: the handler is owned by | ||
| // TrustStoreManager and never outlives it. | ||
| auto handler = std::make_shared<TrustStoreHandler>(TrustStoreHandle{id}, [this, id](TrustStoreHandler& h) { | ||
| this->LoadAnchorsIntoHandler(id, h); | ||
| }); |
There was a problem hiding this comment.
This statement seems not correct TrustStoreDataNode also provides the interface ITrustStoreHandler::Sptr GetStore(), suggesting we cannot guarantee that a Hanlder may outlive the TrustStoreManager, hence we should do the capture with a weap_ptr instead.
| /// Avoids repeated disk reads when multiple clients open the same slot cert. | ||
| /// Entries are evicted automatically when no CertEntry holds a strong ref, | ||
| /// and explicitly invalidated on StoreCertificate / ClearSlot. | ||
| std::unordered_map<uint32_t, std::weak_ptr<CertObject>> m_cert_object_cache; |
There was a problem hiding this comment.
What is the reason not to have Crl cache for a Slot?
If multiple truststore reference the same Slot the Crl will be fetched multiple times from storage instead of beeing cached.
| if (slot) | ||
| m_slot_memberships[slot->index].push_back(id); |
There was a problem hiding this comment.
Members which were present in the configuration but are not discoverable via ResolveSlotInternal are silently dropped.
This is the case for multiple methods including LoadAnchorsIntoHandler
Only management and inspection APIs which are directly referencing a slot will fail.
There should be a defined startup policy for this case, i would currently assume the TrustStore should be marked as invalid.
Usage should then lead to an early failure rather then silently dropping of a member.
A similar case exist for the persisted state. We should consider differnt cases:
- Bootstrap: first-run provisioning confirms path is absent and builds initial state from config (may long term be handled differently)
- Normal startup: file must exist and be parsable, sections should be present, otherwise TrustStore has become invalid (repair required)
- Validate state: every member should have at least a valid enable value, otherwise TrustStore has become invalid (repar required. Unclear if configuration updates need to be considered e.g. adding of an additional member slot)
| Atomic File Writes via Temp-File and Rename | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| .. dec_rec:: Atomic File Writes via Temp-File and Rename |
There was a problem hiding this comment.
Please move the decision record to the respective subcomponent.
| .. needextend:: "c.this_doc()" | ||
| :+tags: cert_management |
There was a problem hiding this comment.
What do we need this for?
| class CertificateConfig | ||
| { | ||
| public: | ||
| enum class TrustStoreMemberKind : uint8_t |
There was a problem hiding this comment.
This type is defined twice:
- trust_store_config.hpp
- this file.
| * consumed by TrustStoreManager during initialisation. | ||
| */ | ||
| class CertificateConfig | ||
| { |
There was a problem hiding this comment.
Please add missing doxygen description for types and enum values.
| /// @brief Absolute path to the KV deployment descriptor file. | ||
| std::string deployment_path; | ||
| /// @brief Format of the deployment descriptor (default "kv"). | ||
| std::string deployment_format{"kv"}; |
There was a problem hiding this comment.
Please abstract the description as with a different backend these may have different meaning.
The current description can act as an example how it is interpereted.
| { | ||
| // Trust-store reads expose certificates, not private key material. The | ||
| // application-resource mapping has already selected the store for UID. | ||
| static_cast<void>(store); |
There was a problem hiding this comment.
use the [[maybe_unused]].
| out.push_back(static_cast<std::uint64_t>(slot_node_ids.size())); | ||
| for (const auto node_id : slot_node_ids) | ||
| out.push_back(node_id); | ||
| return CheckBudget(std::move(out)); |
There was a problem hiding this comment.
in this function params as an lvalue which mean this can be expensive copy. may be return std::move(params) in CheckBudget can reduce this.
| auto cached = cache_it->second.lock(); | ||
| if (cached) | ||
| return cached; | ||
| m_cert_object_cache.erase(cache_it); |
There was a problem hiding this comment.
a read can overwrite the cache with a stale cert if a write happens at the same time, because the mutex is released during the disk load and writers invalidate before the read finishes inserting. can lead to stale cert serving untill next write
| const auto cert_path = descriptor->Get("certificate", "cert_path"); | ||
| if (!cert_path.empty()) | ||
| { | ||
| const auto dot = cert_path.rfind('.'); |
There was a problem hiding this comment.
does it make sense to only look for . after the last path separator (find the basename first, then rfind('.') within it), or always append .crl to the whole cert_path. otherwise may happen the below
Example: cert_path = "/etc/my.certs/leaf" → the last . is in the directory my.certs, so the result is "/etc/my" + ".crl" = /etc/my.crl — a wrong path outside the intended directory
| path = slot.deployment_path + ".crl"; | ||
| } | ||
| } | ||
| auto result = file_io::WriteFile(path, data); |
There was a problem hiding this comment.
what happen in case the below sequence occurs ? WriteFile(path, data) → succeeds (CRL bytes are on disk).
SaveDescriptor(...) → fails (descriptor is not updated).
Result: the CRL file exists, but the descriptor doesn't point to it / doesn't have its metadata. The two disagree, StoreCrl returns an error, and the leftover file is not cleaned up.
| descriptor->Set("crl", "crl_next_update", std::to_string(metadata->next_update)); | ||
| descriptor->Set("crl", "crl_number", std::to_string(metadata->crl_number)); | ||
| } | ||
| return SaveDescriptor(slot, *descriptor); |
There was a problem hiding this comment.
nowhere fsync is doen on each io operations. so this can be problem as well.
| { | ||
| if (!score::crypto::daemon::common::storage::IsDeploymentPathSafe(path)) | ||
| { | ||
| score::mw::log::LogError() << kLogPrefix << "Unsafe deployment path rejected: " << path; |
There was a problem hiding this comment.
Logging the rejected path exposes attacker-controlled input, which may leak filesystem details. Consider removing the path from the log message or sanitizing it before logging. May be pen test can bug this later ?
| { | ||
| if (!score::crypto::daemon::common::storage::IsDeploymentPathSafe(path)) | ||
| { | ||
| score::mw::log::LogError() << kLogPrefix << "Unsafe deployment path rejected: " << path; |
There was a problem hiding this comment.
similar dont log the path. it give or exposes the file system
- Remove unused certificate-service injection from IProvider and OpenSSL. - Add an optional parser-provider name to CertificateConfig. Let SlotHandlerFactory own parser selection and caching, with optional fail-fast validation for explicit pins. - Remove unused provider certificate types and their BUILD dependency; update tests and architecture docs.
| } | ||
|
|
||
| score::crypto::Expected<CertObject::Sptr, Error> BuildObject(X509* certificate, | ||
| const std::uint8_t* bytes, |
There was a problem hiding this comment.
Missing the individual subject fields (e.g., CN, O, OU, C, L, ST, E) parsing from certs and this is needed.
|
|
||
| score::crypto::Expected<CertObject::Sptr, Error> BuildObject(X509* certificate, | ||
| const std::uint8_t* bytes, | ||
| std::size_t size, |
There was a problem hiding this comment.
Missing public key extract.
| return result; | ||
| } | ||
|
|
||
| score::crypto::Expected<std::monostate, Error> CertSlotManager::ClearSlot(CertSlotHandle slot, |
There was a problem hiding this comment.
deletion of a certificate from certificate storage using either its Distinguished Name (DN) or Common Name (CN) not present
| if (fingerprint.size() != expected_sha256_fingerprint.size() || | ||
| !std::equal(fingerprint.begin(), fingerprint.end(), expected_sha256_fingerprint.begin())) | ||
| return score::crypto::make_unexpected(Error::kInvalidOperation); | ||
| m_slot_cert_cache[slot.index] = cert; |
There was a problem hiding this comment.
If I understand correctly, the new state is published to the cache, and TrustStoreHandler before PersistState() succeeds. If persistence fails, this method returns an error even though the certificate is already trusted by the running process; after restart, the old state is restored.
| if (has_crl) | ||
| { | ||
| const auto stored = | ||
| resolved->handler->StoreCrl(*resolved->cfg, crl_bytes, crl_format, std::move(crl_metadata)); |
There was a problem hiding this comment.
StoreCrl() failures are silently ignored on the existing-member upsert path. The method continues by enabling the member, persists the state, and returns success, so the caller cannot tell that the requested revocation data was not installed. Am I understanding correctly?


Implements the certificate management subsystem of the crypto daemon.
closes #163