[PM-19896]: reduce panic risk in cnight-observation genesis construction#1466
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>
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 |
Culprit of my comment was parsing in two places, we could replace with and/or with (BTW validate pattern above allows for total 9+1+108=118 ascii chars, but runtime allows only 108 bytes) => again problem with having this logic in two places. Anyway, I don't see a bug introduced in this PR. I think that if we touch this code (and add substantial number of lines), we can do it better. |
…ke/pm-19896-cnight-genesis-panic-hardening Signed-off-by: Mike Clay <mike.clay@shielded.io>
Round-3 fix-cycle response to external review on commit `e0f64900`. Klapeyron (2026-05-13) observed that the round-1+2 solution shipped roughly 300 LOC of helper, mock helpers, constants, comments, and tests to enrich four fail-fast panic messages — a surface disproportionate to the underlying audit finding. This commit accepts the critique and shrinks the change. Changes: - Remove `pub fn bounded_or_panic_with_field_path<S: Get<u32>>(...)` from the pallet. The four genesis-build call sites now use inline `unwrap_or_else(|v: Vec<u8>| panic!(...))` blocks, reading the supplied byte length from the `TryFrom<Vec<u8>>` error path and the cap from the destination `BoundedVec` type via `bound()`. The dotted chain-spec field-path strings introduced in round 2 to satisfy LGLO R3 are preserved verbatim (`cNightObservation.config.addresses.<field>`). - Revert the two mock-crate helpers (`runtime_genesis_with_cnight_addresses`, `midnight_test_config`) and the corresponding `mock/Cargo.toml` dep entries. The `midnight` field literal is inlined back into `new_test_ext` exactly as it was pre-PR. The mock crate is now byte-identical to the PR base. - Reduce the five round-1+2 tests to one representative regression guard (`build_panics_with_field_path_and_bound_when_mapping_validator_address_too_long`). The retained test exercises the genesis-build path directly via `GenesisConfig::<Test>::build()` on a fresh `TestExternalities`, without the removed mock helpers. The four call sites use a byte-identical message template, so a regression to the format would affect all four simultaneously. - Slim the change-file prose to one paragraph; drop the round-2 "future hardening" footnote (LGLO R4 — `CNightAddresses → BoundedVec` refactor — is a separate follow-up issue). Round-3 net delta vs PR base (`19f8b0f9`): +112 / -21 across 4 files, down from round-2's +288 / -46 across 7 files. Behaviour for valid chain specs is unchanged; the operator-facing panic strings are unchanged for over-length input. Round-3 plan: §20–§28 of the work-package plan. JIRA: https://shielded.atlassian.net/browse/PM-19896 Signed-off-by: Mike Clay <mike.clay@shielded.io>
|
The "parsing in two places" framing makes sense — a serde-side Round 3 ( |
…ke/pm-19896-cnight-genesis-panic-hardening Signed-off-by: Mike Clay <mike.clay@shielded.io>
Signed-off-by: Mike Clay <mike.clay@shielded.io>
|
Blocked on Docker login issue |
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.Round 3 (landed —
134b4da8, on top of merge15e310b3). Addresses Klapeyron's overcomplication critique one0f64900: remove the helper and inline the four panics, restore the mock crate to its pre-PR shape, reduce the five panic-message tests to one representative regression guard onmapping_validator_address, slim the change file. Net diff vs PR base shrinks from+288/−46to+112/−21(≈176 LOC reduction). Panic strings, field paths, and the operator-facing contract are unchanged.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