Repository navigation
feat(envelope): add ModelMessageEnvelope with cryptographic signing [OMN-1898] - #478
Conversation
…OMN-1898] ## Summary Implement runtime-gateway-centric message envelope with Ed25519 signatures and Blake3 payload hashing for secure inter-service communication. ## Key Components ### Crypto Utilities - Blake3 hasher for deterministic payload hashing - Ed25519 signer for envelope authentication - FileKeyProvider for runtime public key management ### Envelope Models - ModelMessageEnvelope[T]: Generic signed envelope - ModelEnvelopeSignature: Signature metadata - ModelEmitterIdentity: Component identity for observability ### Protocol - ProtocolKeyProvider: Interface for key storage backends ## Design Decisions - Signature bound to runtime_id (gateway), not emitter_identity - emitter_identity is optional observability metadata - Identity mismatch validation: emitter_identity.env must match realm - Canonical JSON serialization for deterministic hashing ## Test Coverage - 63 tests for crypto utilities and envelope models - Full sign → serialize → verify roundtrip tests - Identity mismatch rejection tests - Protocol compliance tests
📝 WalkthroughWalkthroughAdds Blake3 hashing, Ed25519 signing, a file-backed key provider and ProtocolKeyProvider, envelope models (emitter identity, signature, generic message envelope) with signing/verification, new envelope-related error codes, public re-exports, and comprehensive unit tests. Dependency Changes
Sequence DiagramsequenceDiagram
actor App
participant Envelope as ModelMessageEnvelope
participant Hasher as Blake3Hasher
participant Signer as Ed25519Signer
participant Provider as FileKeyProvider
participant Verifier as SignatureVerifier
App->>Envelope: create_signed(payload, realm, runtime_id, keypair, ...)
Envelope->>Hasher: hash_canonical_json(payload)
Hasher-->>Envelope: payload_hash
Envelope->>Signer: sign(private_key, signed_content)
Signer-->>Envelope: signature
Envelope-->>App: signed ModelMessageEnvelope
App->>Envelope: verify_signature(key_provider)
Envelope->>Provider: get_public_key(runtime_id)
Provider-->>Envelope: public_key / None
Envelope->>Verifier: verify(public_key, signed_content, signature)
Verifier-->>Envelope: True / False
Envelope-->>App: verification result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
- Narrow exception handling in verify() and verify_base64() to specific crypto exceptions (ValueError, TypeError, InvalidSignature) instead of bare Exception catch to avoid masking system errors - Set explicit file permissions (0o600) when saving keys to disk in FileKeyProvider to prevent unauthorized access - Add prominent Security Warning in ModelMessageEnvelope docstring about emitter_identity being untrusted metadata not covered by signature Review iteration: 1/10
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 44: The blake3 dependency line uses the PEP 508 parentheses style
("blake3 (>=1.0.8,<2.0.0)"), which is inconsistent with the other dependencies;
update that entry to the normalized, modern format without parentheses (e.g.,
"blake3>=1.0.8,<2.0.0") so it matches the rest of the dependency syntax in
pyproject.toml and retains the same version constraints.
🧹 Nitpick comments (12)
src/omnibase_core/crypto/file_key_provider.py (2)
1-17: Consider renaming file to follow naming conventions.The file is named
file_key_provider.py, but the coding guidelines require files insrc/omnibase_core/**/*.pyto follow specific naming prefixes (e.g.,service_*,util_*,protocol_*). Consider renaming toservice_file_key_provider.pyorutil_file_key_provider.py.As per coding guidelines: "File names must follow the naming convention based on directory:
cli_*,constants_*,container_*,context_*,contract_*,decorator_*,enum_*,error_*orexception_*,factory_*,node_*,mixin_*,model_*,protocol_*,runtime_*,service_*,util_*,validator_*orchecker_*"
71-73: Consider logging when key file is invalid.When the key file has invalid JSON or format, the code silently resets to an empty state. While this is reasonable for development/testing, consider adding a warning log to help diagnose configuration issues.
except (json.JSONDecodeError, KeyError, ValueError): - # Invalid file format - start fresh + # Invalid file format - start fresh + # Consider: logging.warning("Invalid key file format at %s, starting fresh", self._key_file) self._keys = {}src/omnibase_core/crypto/ed25519_signer.py (1)
1-12: Consider renaming file to follow naming conventions.The file is named
ed25519_signer.py, but the coding guidelines require specific prefixes. Consider renaming toutil_ed25519_signer.py.As per coding guidelines: "File names must follow the naming convention based on directory."
tests/unit/crypto/test_blake3_hasher.py (1)
10-11: Missing@pytest.mark.unitmarker on test classes.Per coding guidelines, test functions should use pytest markers like
@pytest.mark.unit. Consider adding the marker to each test class or method.♻️ Proposed fix to add pytest markers
+@pytest.mark.unit class TestHashBytes: """Tests for hash_bytes function."""+@pytest.mark.unit class TestHashCanonicalJson: """Tests for hash_canonical_json function."""As per coding guidelines:
tests/**/*.py: Use@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow,@pytest.mark.performance,@pytest.mark.memory_intensive,@pytest.mark.isolatedmarkers on test functions.src/omnibase_core/models/envelope/model_envelope_signature.py (1)
54-59: Consider adding hex character validation forpayload_hash.The
payload_hashfield validates length but not that the content is valid hexadecimal. While invalid hex will fail during signature verification, early validation provides clearer error messages.♻️ Optional: Add hex pattern validation
payload_hash: str = Field( ..., min_length=64, max_length=64, + pattern=r"^[0-9a-fA-F]{64}$", description="Blake3 hash of the payload (64 hex characters).", )tests/unit/crypto/test_ed25519_signer.py (1)
17-18: Missing@pytest.mark.unitmarkers on test classes.Per coding guidelines, test functions should use pytest markers. Consider adding
@pytest.mark.unitto each test class.♻️ Proposed fix to add pytest markers
+@pytest.mark.unit class TestGenerateKeypair: """Tests for generate_keypair function."""Apply similarly to
TestEd25519KeyPair,TestSignAndVerify, andTestBase64Functions.As per coding guidelines:
tests/**/*.py: Use@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow,@pytest.mark.performance,@pytest.mark.memory_intensive,@pytest.mark.isolatedmarkers on test functions.tests/unit/crypto/test_file_key_provider.py (1)
26-73: Add@pytest.mark.unitmarker to test classes.Per coding guidelines, test functions should use appropriate markers (
@pytest.mark.unit,@pytest.mark.integration, etc.).♻️ Proposed fix
+@pytest.mark.unit class TestFileKeyProviderBasics: """Basic functionality tests."""Apply similarly to
TestFileKeyProviderPersistence,TestFileKeyProviderRemoval, andTestFileKeyProviderProtocolCompliance.As per coding guidelines:
tests/**/*.py: Use@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow,@pytest.mark.performance,@pytest.mark.memory_intensive,@pytest.mark.isolatedmarkers on test functions.tests/unit/models/envelope/test_model_message_envelope.py (2)
31-42: Add type annotations to fixtures and@pytest.mark.unitmarkers to test classes.The fixtures are missing return type annotations, and test classes are missing the required markers.
♻️ Proposed fix for fixtures
`@pytest.fixture` -def keypair(): +def keypair() -> Ed25519KeyPair: """Generate a keypair for testing.""" return generate_keypair() `@pytest.fixture` -def key_provider(tmp_path, keypair): +def key_provider(tmp_path: Path, keypair: Ed25519KeyPair) -> FileKeyProvider: """Create a key provider with a registered key.""" provider = FileKeyProvider(tmp_path / "keys.json") provider.register_key("runtime-dev-001", keypair.public_key_bytes) return providerAdd required imports:
from pathlib import Path from omnibase_core.crypto.ed25519_signer import Ed25519KeyPairAs per coding guidelines: Use
@pytest.mark.unitmarkers on test functions.
69-75: Consider using specific exception types inpytest.raises.Using
pytest.raises(Exception)is broad. Pydantic frozen models raiseValidationErroron attribute mutation attempts.♻️ Proposed fix
+from pydantic import ValidationError + ... def test_identity_is_frozen(self) -> None: """Identity model is immutable.""" identity = ModelEmitterIdentity(...) - with pytest.raises(Exception): # ValidationError for frozen model + with pytest.raises(ValidationError): identity.env = "prod" # type: ignore[misc] def test_identity_requires_all_fields(self) -> None: """All fields are required.""" - with pytest.raises(Exception): + with pytest.raises(ValidationError): ModelEmitterIdentity(env="dev") # type: ignore[call-arg]src/omnibase_core/models/envelope/model_message_envelope.py (3)
87-87: Consider addingfrozen=Trueto make the envelope immutable.Per coding guidelines, immutable value models should use
ConfigDict(frozen=True, extra="forbid", from_attributes=True). Since signed envelopes should not be modified after creation (cryptographic integrity), immutability is appropriate here.♻️ Proposed fix
- model_config = ConfigDict(extra="forbid", from_attributes=True) + model_config = ConfigDict(frozen=True, extra="forbid", from_attributes=True)As per coding guidelines:
src/omnibase_core/models/**/*.py: Pydantic immutable value models must useConfigDict(frozen=True, extra="forbid", from_attributes=True).
172-194: Consider extracting shared signing content logic to reduce duplication.The signing content dictionary construction is duplicated between
_get_signing_content(Lines 179-194) andcreate_signed(Lines 287-302). Both build the same structure with identical keys.One approach: extract a static helper method that both can call:
♻️ Suggested refactor
`@staticmethod` def _build_signing_dict( realm: str, runtime_id: str, bus_id: str, trace_id: UUID, emitted_at: datetime, payload_hash: str, causality_id: UUID | None = None, tenant_id: str | None = None, ) -> dict[str, str]: signing_dict: dict[str, str] = { "realm": realm, "runtime_id": runtime_id, "bus_id": bus_id, "trace_id": str(trace_id), "emitted_at": emitted_at.isoformat(), "payload_hash": payload_hash, } if causality_id: signing_dict["causality_id"] = str(causality_id) if tenant_id: signing_dict["tenant_id"] = tenant_id return signing_dictThen both methods can use this helper.
Also applies to: 286-302
276-284: Document or reconsider the fallback payload handling.When payload is neither a
BaseModelnordict, it's wrapped as{"value": payload}(Line 283). This implicit transformation affects thepayload_hashand could be surprising. For example, a string payload"hello"becomes{"value": "hello"}for hashing purposes.Consider either:
- Documenting this behavior explicitly in the docstring
- Raising an error for unsupported payload types to make behavior explicit
♻️ Option: Make unsupported types explicit
else: - # For other types, attempt JSON serialization - payload_dict = {"value": payload} + # Reject unsupported payload types to ensure deterministic hashing + raise TypeError( + f"Payload must be a BaseModel or dict, got {type(payload).__name__}" + )
- Use umask to set restrictive permissions (0o077) atomically before creating the key file, preventing a race condition window where keys could be read between write_text() and chmod() - Restore original umask after file creation in finally block Review iteration: 2/10
5d4c3cd to
e9bc7ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/omnibase_core/enums/enum_core_error_code.py`:
- Line 129: Add the missing ENVELOPE_IDENTITY_MISMATCH enum entry into both the
CORE_ERROR_CODE_TO_EXIT_CODE mapping and the descriptions dict returned by
get_core_error_description: update CORE_ERROR_CODE_TO_EXIT_CODE to explicitly
map ENVELOPE_IDENTITY_MISMATCH (from enum ENVELOPE_IDENTITY_MISMATCH) to the
appropriate ExitCode (use ExitCode.ERROR if that is the intended default) and
add a human-readable message for "ENVELOPE_IDENTITY_MISMATCH" into the
descriptions dict inside get_core_error_description (so calls to
get_core_error_description(ENVELOPE_IDENTITY_MISMATCH) return a meaningful
string instead of "Unknown error").
In `@src/omnibase_core/models/envelope/model_message_envelope.py`:
- Around line 204-219: derive_consumer_group currently returns a fallback string
f"{self.realm}.{self.runtime_id}.{purpose}" which is structurally different from
ModelEmitterIdentity.derive_consumer_group() (which yields
"{env}.{service}.{node_name}.{purpose}.{version}"); update derive_consumer_group
to produce a consistent five-segment format or explicitly document the
asymmetry. Specifically, in derive_consumer_group (and its fallback branch when
emitter_identity is None) either add missing components (e.g.,
env/service/node_name/version placeholders derived from available attributes or
a default version) so the fallback matches
"{env}.{service}.{node_name}.{purpose}.{version}", or add a clear docstring
comment noting the format difference; reference derive_consumer_group,
emitter_identity, and ModelEmitterIdentity.derive_consumer_group when making the
change.
🧹 Nitpick comments (6)
tests/unit/models/envelope/test_model_envelope_signature.py (1)
12-19: Consider adding pytest markers to test classes/functions.The coding guidelines specify using markers like
@pytest.mark.uniton test functions. Consider adding markers to enable selective test execution:`@pytest.mark.unit` class TestEnumSignatureAlgorithm:As per coding guidelines: "Use
@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow,@pytest.mark.performance,@pytest.mark.memory_intensive,@pytest.mark.isolatedmarkers on test functions".src/omnibase_core/models/envelope/model_emitter_identity.py (1)
95-103: Consider using proper Pydantic type for validator info parameter.The
infoparameter is typed asobject, but Pydantic providesFieldValidationInfofor field validators. Using the proper type improves IDE support and type safety:♻️ Suggested improvement
+from pydantic import BaseModel, ConfigDict, Field, field_validator, FieldValidationInfo -from pydantic import BaseModel, ConfigDict, Field, field_validator `@field_validator`("env", "service", "node_name", "version") `@classmethod` - def validate_no_dots(cls, v: str, info: object) -> str: + def validate_no_dots(cls, v: str, info: FieldValidationInfo) -> str: """Validate that identity components don't contain dots (used as separator).""" if "." in v: - field_name = getattr(info, "field_name", "field") + field_name = info.field_name msg = f"{field_name} cannot contain dots (used as consumer group separator)" raise ValueError(msg) return vtests/unit/crypto/test_ed25519_signer.py (1)
97-100: Consider hoisting repeated imports to module level.The
EncodingandPublicFormatimports are repeated in multiple test methods (lines 97-100, 125-128, 146-149, 170-173, 229-232). Moving them to module level improves readability:♻️ Suggested improvement
from cryptography.hazmat.primitives.asymmetric.ed25519 import ( Ed25519PrivateKey, Ed25519PublicKey, ) +from cryptography.hazmat.primitives.serialization import ( + Encoding, + PublicFormat, +)Then remove the inline imports from each test method.
src/omnibase_core/crypto/ed25519_signer.py (1)
1-16: File naming convention.The file is named
ed25519_signer.py, but coding guidelines require files insrc/omnibase_core/to follow naming conventions likeutil_*,service_*, etc. Consider renaming toutil_ed25519_signer.pyfor consistency.As per coding guidelines: "File names must follow the naming convention based on directory:
cli_*,constants_*,container_*,context_*,contract_*,decorator_*,enum_*,error_*orexception_*,factory_*,node_*,mixin_*,model_*,protocol_*,runtime_*,service_*,util_*,validator_*orchecker_*"src/omnibase_core/models/envelope/model_message_envelope.py (2)
191-200: Consider consistency in error handling for signer mismatch.The
validate_signature_signer_matches_runtimevalidator raisesValueError, whilevalidate_identity_realm_matchraisesModelOnexError. Both are security-related envelope validations. Consider whether the signer mismatch should also useModelOnexErrorwith a dedicated error code (e.g.,ENVELOPE_SIGNATURE_SIGNER_MISMATCH) for consistency and richer error context.That said, using
ValueErrorhere is acceptable per coding guidelines for boundary validation, so this is optional.
254-254: Misleading parameter name:signature_bytesis actually hex string.The parameter
signature_bytes: stris passed directly toModelEnvelopeSignature.signature, which per the relevant snippet expects "Ed25519 signature as hex string". The namesignature_bytessuggests raw bytes, but it's actually a hex-encoded string.Consider renaming to
signature_hexfor clarity and consistency withpayload_hash(which is also hex but named appropriately).♻️ Proposed fix
def create( cls, *, realm: str, runtime_id: str, # string-id-ok: gateway identifier bus_id: str, # string-id-ok: external system identifier trace_id: UUID, payload: T, payload_hash: str, - signature_bytes: str, + signature_hex: str, tenant_id: str | None = None, # string-id-ok: external tenant identifier causality_id: UUID | None = None, emitter_identity: ModelEmitterIdentity | None = None, emitted_at: datetime | None = None, ) -> "ModelMessageEnvelope[T]":And update the usage on line 293:
signature = ModelEnvelopeSignature( algorithm=EnumSignatureAlgorithm.ED25519, signer=runtime_id, payload_hash=payload_hash, - signature=signature_bytes, + signature=signature_hex, )
| def derive_consumer_group(self, purpose: str) -> str: | ||
| """ | ||
| Derive a consumer group ID for this envelope. | ||
|
|
||
| Uses emitter_identity if present, otherwise falls back to runtime_id. | ||
|
|
||
| Args: | ||
| purpose: The purpose suffix (e.g., 'consume', 'handle') | ||
|
|
||
| Returns: | ||
| Consumer group ID string | ||
| """ | ||
| if self.emitter_identity is not None: | ||
| return self.emitter_identity.derive_consumer_group(purpose) | ||
| # Fallback to runtime_id-based group | ||
| return f"{self.realm}.{self.runtime_id}.{purpose}" |
There was a problem hiding this comment.
Consumer group format asymmetry.
The fallback format {realm}.{runtime_id}.{purpose} differs structurally from ModelEmitterIdentity.derive_consumer_group() which produces {env}.{service}.{node_name}.{purpose}.{version}. This inconsistency (3 vs 5 dot-separated segments) could cause issues if downstream consumers expect a consistent format.
Consider documenting this asymmetry or adding a version component to the fallback format for consistency.
🤖 Prompt for AI Agents
In `@src/omnibase_core/models/envelope/model_message_envelope.py` around lines 204
- 219, derive_consumer_group currently returns a fallback string
f"{self.realm}.{self.runtime_id}.{purpose}" which is structurally different from
ModelEmitterIdentity.derive_consumer_group() (which yields
"{env}.{service}.{node_name}.{purpose}.{version}"); update derive_consumer_group
to produce a consistent five-segment format or explicitly document the
asymmetry. Specifically, in derive_consumer_group (and its fallback branch when
emitter_identity is None) either add missing components (e.g.,
env/service/node_name/version placeholders derived from available attributes or
a default version) so the fallback matches
"{env}.{service}.{node_name}.{purpose}.{version}", or add a clear docstring
comment noting the format difference; reference derive_consumer_group,
emitter_identity, and ModelEmitterIdentity.derive_consumer_group when making the
change.
- Fix directory permissions bug: set umask before mkdir, not just file write - Add key length validation on load to reject corrupt keys early - Add payload integrity verification to verify_signature() method - Add thread safety tests for FileKeyProvider concurrent access - Add tampered envelope detection tests to verify integrity protection Review iteration: 1/10
- Add field_validator for emitted_at to reject naive datetimes - Add tests for timezone-aware datetime validation Review iteration: 2/10
e9bc7ab to
4dbc4d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/omnibase_core/crypto/blake3_hasher.py`:
- Around line 1-18: Rename the module file from blake3_hasher.py to
util_blake3_hasher.py to follow the util_* prefix convention, then update all
imports that reference the old module name (e.g., any "from
omnibase_core.crypto.blake3_hasher import ..." or "import
omnibase_core.crypto.blake3_hasher" usages) to point to
omnibase_core.crypto.util_blake3_hasher; also update any package __init__.py
exports, tests, and CI references that import or reference blake3_hasher so they
import util_blake3_hasher instead and ensure the module-level contents
(docstring and imports like blake3 and json) remain unchanged.
In `@src/omnibase_core/crypto/ed25519_signer.py`:
- Around line 1-12: Rename the module file from ed25519_signer.py to
util_ed25519_signer.py and update all imports that reference ed25519_signer to
the new module name (e.g., change "from omnibase_core.crypto.ed25519_signer
import ..." to "from omnibase_core.crypto.util_ed25519_signer import ...");
search the repo for occurrences of the old module name and update them, and if
the package __init__.py in omnibase_core.crypto exports or imports
ed25519_signer, update those export/import lines to util_ed25519_signer as well
so the module prefix convention and all references (including tests and docs)
remain consistent.
In `@src/omnibase_core/models/envelope/model_message_envelope.py`:
- Around line 194-216: The signing-dict construction is duplicated between
_get_signing_content and create_signed causing risk of divergence; add a single
shared helper (e.g., a `@staticmethod` named _build_signing_dict) that accepts
realm, runtime_id, bus_id, trace_id, emitted_at, payload_hash, optional
causality_id and tenant_id and returns the canonical dict (with trace_id and
causality_id stringified and emitted_at.isoformat()); then update
_get_signing_content to call _build_signing_dict and json.dumps that result, and
update create_signed to call _build_signing_dict when constructing the bytes to
sign so both places use the same logic.
- Around line 218-226: Extract the duplicate payload-to-dict logic from
_compute_payload_hash() and create_signed() into a new private helper
_serialize_payload(self) that returns a canonical dict (use
payload.model_dump(mode="json") for BaseModel, passthrough for dict, else
{"value": payload}); update _compute_payload_hash() to call _serialize_payload()
and pass its result to hash_canonical_json(), and update create_signed() to
reuse _serialize_payload() instead of repeating the branching so both methods
share the exact serialization path.
🧹 Nitpick comments (5)
tests/unit/crypto/test_ed25519_signer.py (1)
17-41: Missing@pytest.mark.unitmarkers on test classes.Per coding guidelines, test functions/classes should use appropriate markers (
@pytest.mark.unit,@pytest.mark.integration, etc.). Add the@pytest.mark.unitmarker to each test class.♻️ Proposed fix
+@pytest.mark.unit class TestGenerateKeypair: """Tests for generate_keypair function."""Apply similarly to
TestEd25519KeyPair,TestSignAndVerify, andTestBase64Functions.tests/unit/crypto/test_file_key_provider.py (1)
26-73: Missing@pytest.mark.unitmarkers on test classes.Per coding guidelines, test classes should use the
@pytest.mark.unitmarker. Apply to all test classes in this file.♻️ Proposed fix
+@pytest.mark.unit class TestFileKeyProviderBasics: """Basic functionality tests."""Apply similarly to all other test classes.
src/omnibase_core/crypto/file_key_provider.py (1)
28-42: Consider renaming to follow naming convention.Per coding guidelines, file names must follow the naming convention with prefixes like
service_*,util_*, etc. Consider renaming toservice_file_key_provider.pysince this is a concrete service implementation ofProtocolKeyProvider.src/omnibase_core/models/envelope/model_message_envelope.py (2)
94-95: Consider addingfrozen=Truefor immutability.Per coding guidelines, Pydantic immutable value models should use
ConfigDict(frozen=True, extra="forbid", from_attributes=True). Since signed envelopes should not be modified after creation (which would invalidate the signature), consider making this model frozen.If mutability is intentional for pre-signing construction, document this design decision.
348-355: Broad exception catch is acceptable here.Catching generic
Exceptionand wrapping inModelOnexErrorprovides useful context for callers. Consider narrowing toValueErrorif you want to be more specific about expected failure modes fromsign_base64.
| """ | ||
| Blake3 hashing utilities for envelope payload verification. | ||
|
|
||
| Blake3 is chosen over SHA-256 for: | ||
| - Faster performance (especially on modern CPUs with SIMD) | ||
| - Tree-hashing support for parallel processing | ||
| - Designed for cryptographic security from the ground up | ||
|
|
||
| The canonical JSON serialization ensures deterministic hashing | ||
| regardless of key ordering in dictionaries. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
|
|
||
| import blake3 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename module to match required file prefix.
blake3_hasher.py doesn’t follow the required prefix conventions for src/omnibase_core/**/*.py. Consider renaming to something like util_blake3_hasher.py and updating imports accordingly.
As per coding guidelines, File names must follow the naming convention based on directory: cli_*, constants_*, container_*, context_*, contract_*, decorator_*, enum_*, error_* or exception_*, factory_*, node_*, mixin_*, model_*, protocol_*, runtime_*, service_*, util_*, validator_* or checker_*.
🤖 Prompt for AI Agents
In `@src/omnibase_core/crypto/blake3_hasher.py` around lines 1 - 18, Rename the
module file from blake3_hasher.py to util_blake3_hasher.py to follow the util_*
prefix convention, then update all imports that reference the old module name
(e.g., any "from omnibase_core.crypto.blake3_hasher import ..." or "import
omnibase_core.crypto.blake3_hasher" usages) to point to
omnibase_core.crypto.util_blake3_hasher; also update any package __init__.py
exports, tests, and CI references that import or reference blake3_hasher so they
import util_blake3_hasher instead and ensure the module-level contents
(docstring and imports like blake3 and json) remain unchanged.
| """ | ||
| Ed25519 signing utilities for envelope authentication. | ||
|
|
||
| Ed25519 is chosen for envelope signing because: | ||
| - Fast key generation and signing | ||
| - Small keys (32 bytes) and signatures (64 bytes) | ||
| - Deterministic signatures (no random nonce needed) | ||
| - Strong security guarantees | ||
|
|
||
| The cryptography library is used as it's already a dependency | ||
| and provides a well-audited Ed25519 implementation. | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename module to match required file prefix.
ed25519_signer.py doesn’t follow the required prefix conventions for src/omnibase_core/**/*.py. Consider renaming to something like util_ed25519_signer.py (or equivalent) and updating imports accordingly.
As per coding guidelines, File names must follow the naming convention based on directory: cli_*, constants_*, container_*, context_*, contract_*, decorator_*, enum_*, error_* or exception_*, factory_*, node_*, mixin_*, model_*, protocol_*, runtime_*, service_*, util_*, validator_* or checker_*.
🤖 Prompt for AI Agents
In `@src/omnibase_core/crypto/ed25519_signer.py` around lines 1 - 12, Rename the
module file from ed25519_signer.py to util_ed25519_signer.py and update all
imports that reference ed25519_signer to the new module name (e.g., change "from
omnibase_core.crypto.ed25519_signer import ..." to "from
omnibase_core.crypto.util_ed25519_signer import ..."); search the repo for
occurrences of the old module name and update them, and if the package
__init__.py in omnibase_core.crypto exports or imports ed25519_signer, update
those export/import lines to util_ed25519_signer as well so the module prefix
convention and all references (including tests and docs) remain consistent.
…ing consistency - Fixed: file_key_provider.py - Add permission validation on load and explicit chmod after write - Fixed: model_message_envelope.py - Wrap payload hash computation in ModelOnexError for consistency - Added: ENVELOPE_PAYLOAD_SERIALIZATION_FAILED error code for serialization failures Review iteration: 1/10
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_core/crypto/file_key_provider.py`:
- Around line 108-124: The code sets process-wide umask via os.umask(0o077)
around the write, which is unsafe in multithreaded contexts; instead remove the
umask change and explicitly set permissions on both the parent directory and the
file after creation: create the parent directory via
self._key_file.parent.mkdir(...), then call
self._key_file.parent.chmod(self._MAX_ALLOWED_MODE) to ensure directory
permissions, write the file (preferably via an atomic temp-write+rename
pattern), and finally call self._key_file.chmod(self._MAX_ALLOWED_MODE) as
already present; update the block that currently uses os.umask,
self._key_file.parent.mkdir, self._key_file.write_text and self._key_file.chmod
to perform explicit chmods and atomic write without changing global umask.
🧹 Nitpick comments (3)
src/omnibase_core/models/envelope/model_message_envelope.py (1)
95-95: Consider addingfrozen=Trueto prevent post-creation mutation.Since the envelope contains a cryptographic signature over its contents, mutating any signed field after creation would silently invalidate the signature. Making the model frozen would prevent accidental mutation and align with the "immutable value model" guideline.
♻️ Proposed change
- model_config = ConfigDict(extra="forbid", from_attributes=True) + model_config = ConfigDict(frozen=True, extra="forbid", from_attributes=True)As per coding guidelines: "Pydantic immutable value models must use
ConfigDict(frozen=True, extra="forbid", from_attributes=True)"src/omnibase_core/crypto/file_key_provider.py (2)
1-25: File naming convention violation.Per coding guidelines, files must follow the naming convention with prefixes like
service_*,util_*,protocol_*, etc. Consider renaming toservice_file_key_provider.pyorutil_file_key_provider.py.Additionally,
loggingis imported inside_load_keys(line 74). Move it to module-level imports for clarity and to avoid repeated import overhead on reload.import base64 import json +import logging import os import threading from pathlib import Path
72-93: Consider more precise permission check and logging invalid keys.
The numeric comparison
file_mode > self._MAX_ALLOWED_MODEwill flag0o700(owner rwx) as insecure even though no group/other access exists. A bitwise check is more semantically correct:Silently skipping invalid-length keys (line 93) could hide data corruption. A warning log aids operational visibility.
♻️ Suggested improvements
- file_mode = self._key_file.stat().st_mode & 0o777 - if file_mode > self._MAX_ALLOWED_MODE: - import logging - + file_mode = self._key_file.stat().st_mode & 0o777 + # Check if group or other permissions are set + if file_mode & 0o077: logging.warning( - "Key file %s has insecure permissions %o (should be <= %o). " + "Key file %s has insecure permissions %o (group/other access detected). " "This may allow unauthorized key modification.", self._key_file, file_mode, - self._MAX_ALLOWED_MODE, )if len(key_bytes) == self.ED25519_PUBLIC_KEY_LENGTH: loaded_keys[runtime_id] = key_bytes - # Skip invalid-length keys silently (corrupt data) + else: + logging.warning( + "Skipping key for runtime %s: invalid length %d (expected %d)", + runtime_id, + len(key_bytes), + self.ED25519_PUBLIC_KEY_LENGTH, + )
Major fixes: - Rename crypto modules to follow naming convention (crypto_* prefix) - Deduplicate signing content and payload hash logic in ModelMessageEnvelope Minor fixes: - Normalize blake3 dependency syntax in pyproject.toml - Add @pytest.mark.unit markers to all test classes - Add type annotations to test fixtures Nitpick fixes: - Add frozen=True to ModelMessageEnvelope for immutability - Add hex character validation for payload_hash field - Replace generic pytest.raises(Exception) with specific ValidationError
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/omnibase_core/crypto/crypto_file_key_provider.py`:
- Around line 72-82: The current numeric comparison of file_mode against
self._MAX_ALLOWED_MODE misses cases; change the check in the block that computes
file_mode (using self._key_file.stat().st_mode) to a bitwise test for disallowed
group/other bits instead of `file_mode > self._MAX_ALLOWED_MODE`. For example,
replace the `file_mode > self._MAX_ALLOWED_MODE` condition with a test that
fails when any group/other permission bits are set (e.g., `if file_mode &
~self._MAX_ALLOWED_MODE:`) or explicitly test `stat.S_IRWXG | stat.S_IRWXO` so
that secure owner-only modes like 0o700 pass and any group/other bits (like
0o060) trigger the warning; keep the same logging block using self._key_file and
file_mode.
In `@src/omnibase_core/models/envelope/model_message_envelope.py`:
- Around line 304-311: The verify_signature method currently calls
self._compute_payload_hash() which can raise TypeError/ValueError for
non-serializable payloads; wrap the call to _compute_payload_hash() in a
try/except that catches TypeError and ValueError and returns False to keep
verification fail-closed, but do not catch other exceptions (so missing-key
behavior remains unchanged). After the try/except, continue comparing
computed_hash to self.signature.payload_hash and then call
_get_signing_content() and verify_base64(public_key, signing_content,
self.signature.signature) as before.
- Around line 171-192: Add a post-validation that enforces signature.signer
equals the envelope runtime_id: implement a `@model_validator`(mode="after")
method (e.g., validate_signature_signer_matches_runtime_id) on
ModelMessageEnvelope that checks if self.signature is not None and
self.signature.signer != self.runtime_id and, if so, raises ModelOnexError (use
a new or existing error code like
EnumCoreErrorCode.ENVELOPE_SIGNATURE_SIGNER_MISMATCH) with a message and context
including "signature_signer" and "runtime_id"; this prevents tampered signer
metadata from being accepted.
In `@tests/unit/models/envelope/test_model_message_envelope.py`:
- Around line 71-77: The type-ignore comments in this test (the inline ignore on
the assignment to identity.env and the call to ModelEmitterIdentity in
test_identity_requires_all_fields) need concrete ticket references and brief
explanations; update each `# type: ignore[...]` to include the most specific
ignore code (e.g., arg-type, misc) and append a NOTE with a Linear ticket (e.g.,
NOTE(LINEAR-1234):) plus a one-line rationale (what the type checker is
incorrectly flagging and why the ignore is safe). Apply the same change to the
other test ignores referenced in the review (the similar ignores around the
identity manipulation/assertions and any ignores in the ModelEmitterIdentity
tests), ensuring you keep the specific ignore kind and add the `#
NOTE(LINEAR-XXXX): short reason` for each occurrence.
🧹 Nitpick comments (3)
tests/unit/crypto/test_crypto_file_key_provider.py (1)
114-114: Move inline imports to module level.
base64(lines 114, 230) andthreading(line 187) are imported inside test methods. For consistency and readability, consider moving these to the top of the file alongside the other imports.♻️ Suggested import section
import json +import base64 +import threading from pathlib import Path import pytestAlso applies to: 187-187, 230-230
src/omnibase_core/crypto/crypto_file_key_provider.py (2)
74-74: Moveloggingimport to module level.The lazy import inside the method is non-standard. Since
loggingis a standard library module with negligible import cost, it should be imported at the module level for consistency.♻️ Suggested fix
Add to module-level imports:
import loggingThen remove line 74.
116-118: Non-atomic write could expose partial file content to concurrent readers.
write_textdirectly overwrites the file, which means external readers (including other FileKeyProvider instances callingreload()) could observe partial/corrupt content during the write window.For a development/testing provider this is likely acceptable given the docstring recommends production alternatives. If durability matters, consider write-to-temp-then-rename:
♻️ Atomic write pattern
+import tempfile + def _save_keys(self) -> None: """Save keys to the file.""" with self._lock: keys_data = { runtime_id: base64.urlsafe_b64encode(key).decode("ascii") for runtime_id, key in self._keys.items() } data: dict[str, dict[str, str]] = {"keys": keys_data} old_umask = os.umask(0o077) try: self._key_file.parent.mkdir(parents=True, exist_ok=True) - self._key_file.write_text( - json.dumps(data, indent=2, sort_keys=True), encoding="utf-8" - ) + # Write to temp file then atomic rename + fd, tmp_path = tempfile.mkstemp( + dir=self._key_file.parent, suffix=".tmp" + ) + try: + os.write(fd, json.dumps(data, indent=2, sort_keys=True).encode("utf-8")) + os.close(fd) + os.replace(tmp_path, self._key_file) + except Exception: + os.unlink(tmp_path) + raise self._key_file.chmod(self._MAX_ALLOWED_MODE) finally: os.umask(old_umask)
| # Step 1: Verify payload integrity - hash must match stored hash | ||
| computed_hash = self._compute_payload_hash() | ||
| if computed_hash != self.signature.payload_hash: | ||
| return False | ||
|
|
||
| # Step 2: Verify signature authenticity | ||
| signing_content = self._get_signing_content() | ||
| return verify_base64(public_key, signing_content, self.signature.signature) |
There was a problem hiding this comment.
verify_signature can raise on non-serializable payloads.
_compute_payload_hash() can throw TypeError/ValueError, but the method contract says it returns bool unless the key is missing. Catch and return False to keep verification fail-closed without unexpected exceptions.
🔧 Proposed fix
- computed_hash = self._compute_payload_hash()
- if computed_hash != self.signature.payload_hash:
- return False
+ try:
+ computed_hash = self._compute_payload_hash()
+ except (TypeError, ValueError):
+ return False
+ if computed_hash != self.signature.payload_hash:
+ return False🤖 Prompt for AI Agents
In `@src/omnibase_core/models/envelope/model_message_envelope.py` around lines 304
- 311, The verify_signature method currently calls self._compute_payload_hash()
which can raise TypeError/ValueError for non-serializable payloads; wrap the
call to _compute_payload_hash() in a try/except that catches TypeError and
ValueError and returns False to keep verification fail-closed, but do not catch
other exceptions (so missing-key behavior remains unchanged). After the
try/except, continue comparing computed_hash to self.signature.payload_hash and
then call _get_signing_content() and verify_base64(public_key, signing_content,
self.signature.signature) as before.
…back Major: - Add signer validation (signature.signer == runtime_id) with new error code - Handle non-serializable payloads in verify_signature() with clear errors - Implement thread-safe atomic file writes in crypto_file_key_provider - Add 13 missing enum error code mappings with runtime validation Minor: - Add blake3 to Poetry dependencies section for consistency - Add precise permission checks with bitmask validation - Add logging for invalid key files with remediation advice - Add type annotations to 14 test fixture parameters - Add NOTE explanations to 4 type ignore comments Nitpick: - Document fallback payload handling strategy in docstrings - Verify file naming conventions are compliant - Confirm all imports at module level
- [major] FileKeyProvider now raises ModelOnexError on insecure file permissions instead of just logging warnings (fail-closed security) - [minor] Add 12 new tests for file permission validation scenarios - [minor] Fix misleading comment about frozen model in test Security fix ensures keys won't load from files with group/other access bits or special permission bits (setuid/setgid/sticky). Review iteration: 1/10
1 participant
Summary
Implement runtime-gateway-centric message envelope with Ed25519 signatures and Blake3 payload hashing for secure inter-service communication.
Ticket: OMN-1898
Key Components
Crypto Utilities (
src/omnibase_core/crypto/)Envelope Models (
src/omnibase_core/models/envelope/)Protocol (
src/omnibase_core/protocols/crypto/)Design Decisions
Signature bound to runtime_id (gateway), not emitter_identity
Identity mismatch validation
emitter_identity.envMUST equalrealmCanonical JSON serialization
Fail-closed security model
ENVELOPE_KEY_NOT_FOUNDerror raised for missing keysNew Error Codes (341-345)
ENVELOPE_IDENTITY_MISMATCH: emitter_identity.env doesn't match realmENVELOPE_SIGNATURE_INVALID: Signature verification failedENVELOPE_KEY_NOT_FOUND: Public key not found for runtime_idENVELOPE_SIGNING_FAILED: Signing operation failedENVELOPE_VALIDATION_FAILED: General validation failureTest Coverage
Test Plan
Summary by CodeRabbit
New Features
Tests
Chores