Shared storage infrastructure and provider capability routing for certificate management - #160
ChansAlive wants to merge 18 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 |
|
The created documentation from the pull request is available at: docu-html |
d8c0760 to
3806c91
Compare
a425c7b to
60c98d8
Compare
a0b9c2c to
186e256
Compare
03e9354 to
9d4bc98
Compare
9d4bc98 to
cee8b32
Compare
|
|
||
| load("@rules_cc//cc:defs.bzl", "cc_library") | ||
|
|
||
| cc_library( |
There was a problem hiding this comment.
cert_types.hpp is pure enums. i_ocsp_request_export.hpp is a polymorphic interface and bundling them under one target named cert_types:
Forces every consumer of the enums (lightweight, stable) to also depend on the interface (heavier, unstable/inactive), inflating the rebuild graph for unrelated changes.
split into two cc_library targets (cert_types for enums, i_ocsp_request_export for the interface)
| /// @brief Removes the CRL stored in a certificate slot. | ||
| /// @param cert_slot Handle to the slot whose CRL should be removed (type = kCertSlot) | ||
| /// @return std::monostate on success, error if no CRL is present or access is denied | ||
| virtual score::Result<std::monostate> DeleteCrl(const CryptoResourceId& cert_slot) = 0; |
There was a problem hiding this comment.
DeleteExpiredCrls of batch expired CRLs method is removed without replacement
| /// @param issuer_cert Handle to the issuer certificate (type = kCertSlot or kCertificate) | ||
| /// @param persist When true, store permanently to the issuer slot (kCertSlot only) | ||
| /// @return std::monostate on success, error if validation fails or access is denied | ||
| virtual score::Result<std::monostate> ImportCrl(score::cpp::span<const uint8_t> crl_data, |
There was a problem hiding this comment.
When you call a function through a base-class pointer/reference, C++ decides which default value to use by looking at the declared type of the variable, not the actual object it points to example persist = false
example:
`class Base
{
public:
virtual void Foo(bool flag = false) = 0;
};
class Derived : public Base
{
public:
void Foo(bool flag = true) override { /* ... */ } // legal, but dangerous
};
Derived d;
Base& b = d;
d.Foo(); // uses Derived's default is flag = true
b.Foo(); // uses Base's default is flag = false (SAME object, DIFFERENT behavior!)`
| /// @param cert CryptoResourceId of the certificate to save (type = kCertificate or kCertSlot) | ||
| /// @param target_slot Handle to the target slot (type = kCertSlot) | ||
| /// @param with_crl When true, propagate the associated CRL to the destination slot | ||
| virtual score::Result<std::monostate> SaveCertificate(const CryptoResourceId& cert, |
There was a problem hiding this comment.
Managing one certificate" (parse/save/export) and "curating the trust anchor set" (add/remove/enable/disable members) are two different concerns, we need to split the interfaces into the smaller chucks. we shall follow the Single Responsibility Principle
| /// @brief References an imported CRL for revocation checking. | ||
| /// @param crl Handle to a previously imported CRL | ||
| /// @return std::monostate on success, error if handle is invalid | ||
| virtual score::Result<std::monostate> SetCrl(const CryptoResourceId& crl) = 0; |
There was a problem hiding this comment.
SetCrl() was deleted, so you can no longer tell the system exactly which revocation list to use — it now guesses automatically, with no documentation on how, and no way to check afterward which one it actually used.
| /// | ||
| /// Each entry corresponds to a certificate slot that currently holds a certificate. | ||
| /// Empty slots (not yet populated) are omitted. | ||
| virtual const std::vector<MemberInfo>& GetMembers() const noexcept = 0; |
There was a problem hiding this comment.
GetMembers() returns a snapshot that can go stale. if a compromised trust anchor is disabled or removed elsewhere while your code still holds an older snapshot, it will keep reporting that anchor as trusted/enabled. There's no version marker or refresh method to detect this, so code should always re-fetch a fresh snapshot before using it for any actual trust decision, not just for display/logging.
| } | ||
| const std::string key = Trim(trimmed.substr(0U, eq_pos)); | ||
| const std::string value = Trim(trimmed.substr(eq_pos + 1U)); | ||
| descriptor.sections[current_section][key] = value; |
There was a problem hiding this comment.
Duplicate keys are silently overwritten. for example [key_slots]
slot_1 = /secure/path/a.key
slot_1 = /tmp/attacker_path.key
| std::unordered_map<std::string, std::unordered_map<std::string, std::string>> sections; | ||
|
|
||
| /// @brief Get a value from a section, returning default_val if absent. | ||
| [[nodiscard]] const std::string& Get(const std::string& section, |
There was a problem hiding this comment.
When the section/key isn't found, this returns a reference to the default_val parameter. That's only safe if the caller passes a named object that outlives the call. The most natural calling pattern — passing a string literal as the default. for example const std::string& path = descriptor.Get("key_slots", "slot_1", "/default/path");
// path is dangling here — the literal's temporary was destroyed
| bool FileExists(const std::string& path) | ||
| { | ||
| score::filesystem::StandardFilesystem fs{}; | ||
| const auto result = fs.IsRegularFile(score::filesystem::Path{path}); |
There was a problem hiding this comment.
How to handle the permissions related issues, actual file present but no access
| else if (required_capability != common::ProviderCapability::kNone && | ||
| requested_provider_type == common::CryptoProviderType::DEFAULT) | ||
| { | ||
| provider = m_provider_manager->GetProviderForCapability(required_capability); |
There was a problem hiding this comment.
In the else branch the provider capablity is not checked, so it will fail on create handler with unsupported algo
99970fa to
51f6130
Compare
51f6130 to
a01368f
Compare
Foundational groundwork for the certificate management daemon component.
Closes #162