[PM-19896]: reduce panic risk in cnight-observation genesis construction#1466
[PM-19896]: reduce panic risk in cnight-observation genesis construction#1466m2ux wants to merge 4 commits into
Conversation
Draft PR for: [Node] triage audit findings reduce panic risk in inherent and genesis construction Jira: https://shielded.atlassian.net/browse/PM-19896 GitHub Issue: shieldedtech/shielded-security-engineering#365 Residual scope: four expect() calls in BuildGenesisConfig::build() at pallets/cnight-observation/src/lib.rs:307,316,323,333 Signed-off-by: Mike Clay <mike.clay@shielded.io>
|
I am not sure if we can make significant improvement on that, be design |
Replace the four `expect(...)` calls in `pallet-cnight-observation`'s `BuildGenesisConfig::build()` with `unwrap_or_else(|_| panic!(...))` whose messages name the chain-spec field path, the supplied byte length, and the maximum permitted length expressed via a named constant. An operator reading a startup-failure log can now locate and correct the offending field without inspecting the source. Introduce two named bound constants in `midnight-primitives-cnight-observation` so the panic messages and the bounded-vector storage type parameters share a single source of truth: - `CNIGHT_POLICY_ID_LENGTH = 28` (Cardano native-asset policy ID width) - `CARDANO_ASSET_NAME_MAX_LENGTH = 32` (Cardano native-asset name CDDL cap) Update `MainChainAuthTokenAssetName` and `CNightIdentifier` storage types to use the new constants. `ConstU32<28>` and `ConstU32<CNIGHT_POLICY_ID_LENGTH>` produce identical monomorphisations, so the Substrate metadata, storage layout, and wire format are unchanged. Add three unit tests in `pallets/cnight-observation/tests/tests.rs` that drive `BuildGenesisConfig::build` through `build_storage()` with an over-length value for `mapping_validator_address`, `auth_token_asset_name`, and `cnight_asset_name`, capture the panic via `std::panic::catch_unwind`, and assert the panic message contains both the dotted chain-spec field path and the cap-constant name. `cnight_policy_id` is not covered by a unit test because its source type is `[u8; 28]` and the panic site is unreachable from chain-spec deserialization (serde rejects mismatched lengths first); the defence-in-depth panic remains so a future change to the source type does not silently lose the diagnostic. Expose `runtime_genesis_with_cnight_addresses` and `midnight_test_config` helpers in the cnight-observation mock crate so the new tests reuse the existing `MidnightConfig` initializer rather than duplicating it. Behaviour for valid chain specs is byte-identical: `try_into` still produces the `BoundedVec` and `set` is called as before. The only observable change is the panic string for over-length input. Closes audit finding for the four cnight-observation genesis sites. JIRA: PM-19896 Signed-off-by: Mike Clay <mike.clay@shielded.io>
yes |
Adds the GitHub issue reference required by the .github changes-check workflow (regex 'github.com/.../issues/[0-9]+'). Issue: shieldedtech/shielded-security-engineering#365 Signed-off-by: Mike Clay <mike.clay@shielded.io>
m2ux
changed the title
| // (serde rejects mismatched lengths before reaching genesis build). The | ||
| // diagnostic panic is retained so a future change to the source type does | ||
| // not silently lose the operator-facing failure detail. | ||
| cnight_policy_id_bytes.try_into().unwrap_or_else(|_| { |
There was a problem hiding this comment.
It's a shame that we store BoundedVec instead of [u8; 28]. This is a lot of defensive code that doesn't cover error happening in the other direction: when serialization allows to provide too short bytes.
There was a problem hiding this comment.
I've kept the storage shape this round — switching CNightAddresses to BoundedVec directly is broader (it changes the chain-spec JSON encoding for operators who currently pass hex / ASCII), so it's deferred to a follow-up issue per your summary suggestion. On the too-short direction: zero-length asset names are valid per Cardano CDDL (asset_name = bytes .size (0..32)); for cnight_policy_id the source type is [u8; 28], so serde rejects mismatched lengths before genesis runs.
| .to_vec() | ||
| .try_into() | ||
| .expect("Mapping Validator address longer than expected"), | ||
| mapping_validator_bytes.try_into().unwrap_or_else(|_| { |
There was a problem hiding this comment.
there exists bound function on BoundedVec, so having that may allow us to stay without constants defined for number of elements.
Also TryFrom/TryInto error type is just the source vec that was provided, so we have an option to check the length of the source collection, which may allow us to provide a generic function for it
There was a problem hiding this comment.
Done in e0f64900: the four call sites now go through bounded_or_panic_with_field_path<S: Get<u32>>(field_path, bytes). The helper uses BoundedVec::<u8, S>::bound() to discover the cap and reads the source length from the TryInto error path. The named-constant parentheticals are gone; messages name the numeric bound directly (e.g., maximum 108).
| .expect("Mapping Validator address longer than expected"), | ||
| mapping_validator_bytes.try_into().unwrap_or_else(|_| { | ||
| panic!( | ||
| "genesis: cnight_observation.addresses.mapping_validator_address \ |
There was a problem hiding this comment.
I think that the first segment depends on the name of the pallet in the runtime. IOW: this is too early to know it.
In case of midnight-runtime this name is not cnight_observation but cNightObservation.
There was a problem hiding this comment.
Confirmed and fixed in e0f64900: the panic now prints cNightObservation.config.addresses.<field> (lowerCamelCase, with the .config segment from GenesisConfig<T>::config: CNightGenesis). Verified against runtime/src/lib.rs:913, res/perfnet/chain-spec-abridged.json:207-214, and scripts/genesis/genesis-construction.sh:540.
LGLO
left a comment
LGLO
left a comment
There was a problem hiding this comment.
TBH I would prefer passing BoundedVecs in CNightAddresses. Now we have some parsing there and some parsing in genesis build.
… bounded_or_panic helper Round-2 review feedback on PR #1466 (commit 6bd0113) flagged two issues with the genesis-build panic diagnostics introduced in 8b3b897: - LGLO (R3): the chain-spec field-path strings used the snake_case pallet segment `cnight_observation.addresses.<field>` and omitted the `config.` segment. The runtime declares the pallet as `CNightObservation` and the Substrate v2 #[runtime::*] form renders it as `cNightObservation` in RuntimeGenesisConfig JSON; the GenesisConfig<T>'s Rust `config` field serialises as `config`. Operator-facing chain specs under res/ use `cNightObservation.config.addresses.<field>`. Correct the four panic messages and the corresponding test substrings. - Klapeyron (R2): four near-duplicate `unwrap_or_else(|_| panic!(...))` blocks can be replaced with a single helper that reads the bound off the destination type via `BoundedVec::<u8, S>::bound()`, removing the duplicate plumbing and ensuring the message and the storage type cannot drift apart on a future bound change. Add `pub fn bounded_or_panic_with_field_path<S: Get<u32>>(...)` and route all four genesis-build BoundedVec conversions through it. The named bound constants `CARDANO_BECH32_ADDRESS_MAX_LENGTH`, `CNIGHT_POLICY_ID_LENGTH`, `CARDANO_ASSET_NAME_MAX_LENGTH` remain in primitives — they are still used as `ConstU32<…>` parameters at storage type definitions and helper call sites — but they no longer appear in panic prose. Tests assert `maximum <N>` (the numeric bound returned by `bound()`) instead. Update the change file with a Future-hardening footnote noting the deferred CNightAddresses → BoundedVec refactor (LGLO R4), and add two helper-level unit tests pinning the helper's panic shape (field path, supplied length, `maximum N`) and its happy path independent of the pallet call sites. Round-2 plan: §11–§19 of the work-package plan. JIRA: https://shielded.atlassian.net/browse/PM-19896 Signed-off-by: Mike Clay <mike.clay@shielded.io>
|
@LGLO re your summary suggestion to type |
3 participants
Summary
Reduce panic risk in
cnight-observationgenesis construction by replacing the four remainingexpect()calls inBuildGenesisConfig::build()with diagnostic panics whose messages name the chain-spec field path, the supplied byte length, and the maximum permitted length. The Substrate fail-fast genesis convention (build()returns(); node halts on invalid genesis) is preserved — only the diagnostic prose changes.🎫 Ticket 📐 Engineering
GitHub Issue: shieldedtech/shielded-security-engineering#365
Motivation
External audit flagged three panic sites in the
cnight-observationpallet (formerlynative-token-observation):get_data_from_inherent_data— already remediated upstream via PM-21799 / PR [PM-21799] (fix): replace panic with error handling in get_data_from_inherent_data #1234.set_mapping_validator_contract_address— already remediated via the existingtry_into().map_err(...)pattern.BuildGenesisConfig::build()— residual scope for this work package.The four remaining
expect()calls inbuild()panic with four-word messages ("Mapping Validator address longer than expected", "Policy ID too long", "Asset name too long", "Auth Token asset name longer than expected") that don't say which field was wrong, what length was supplied, or what the cap was. Replacing them with descriptive errors that name the offending field and the maximum permitted length turns a noisy crash into a one-line, self-explanatory diagnostic, shortening operational recovery and closing the audit finding.Planned Changes
Round 1 (landed —
8b3b8976,6bd01131). Named length constants inprimitives/cnight-observation; fourexpect()→unwrap_or_else(|_| panic!(...))inBuildGenesisConfig::build()naming the field, supplied length, and cap; three panic-shape tests; mock helpers; change fragment.Round 2 (landed —
e0f64900). Correct the field path in panic messages to the runtime's actual chain-spec key (cNightObservation.config.addresses.<field>); replace the four near-duplicate panic blocks with a single generic helperbounded_or_panic_with_field_path<S: Get<u32>>(field_path, bytes)that reads the cap viaBoundedVec::<u8, S>::bound(); update three round-1 tests; add two helper-level tests.Out of scope: sibling pallets' genesis builds;
serde_valid::Validateat the genesis boundary; typingCNightAddressesfields asBoundedVecdirectly (deferred — captured as follow-up issue per LGLO's suggestion); asset-name minimum-length check (CDDL allows zero-length); storage migrations; runtime metadata regeneration.Detailed plan:
05-work-package-plan.md. Test plan:05-test-plan.md.📌 Submission Checklist
🔱 Fork Strategy
🗹 TODO before merging