Conversation
Covers IC-specific security patterns: access control, anonymous principal rejection, TOCTOU/reentrancy, pre_upgrade traps, cycles monitoring, controller backup, fetchRootKey risks, admin guards, secret storage, storage exhaustion, callback trap inconsistency, and unbounded wait calls. Both Motoko and Rust implementations. Reviewed against IC interface specification, DFINITY security best practices, and recent audit findings. Adds @dfinity/security as CODEOWNER. Based on the draft from #51, with corrections: - inspect_message: specify boundary node bypass, not generic node - freezing_threshold: default is already 30 days, canister is uninstalled not deleted - fetchRootKey: not deprecated, required for local dev - Added pitfalls: pre_upgrade trap, storage exhaustion, callback trap after state mutation, unbounded wait calls - Links to official security best practices documentation
- Rust: caller() -> msg_caller(), api::call::{accept_message, method_name}
-> api::{accept_message, msg_method_name} (removed in ic-cdk 0.19)
- Motoko: Array.append -> Array.concat (renamed in base->core migration)
- Add missing Error import note to saga pattern
- Remove deploy boilerplate, consolidate into Verify It Works
- Add cross-referencing guidance to CONTRIBUTING.md
venkkatesh-sekar
left a comment
venkkatesh-sekar
left a comment
There was a problem hiding this comment.
@marc0olo I took a first pass and left feedback. But the security team is also doing some parallel work here https://github.com/dfinity-lab/security-skills, so maybe we can unify the effort.
@venkkatesh-sekar thanks a lot. I didn't know about the effort of the security team. I think unifying the overall effort would be good. what I definitely like in your repo is the test harness. ideally we would have evaluations for all skills to ensure their effectiveness. this is currently completely missing on this repo. I am wondering about the best approach to unify and whether it makes sense to have this proposed skill here in the repo. did you have the chance to read #52 and follow-up comments? do you have any suggestion / recommendation how to structure our skills and how to incorporate your security skills? |
|
Thanks for the thorough review @venkkatesh-sekar — all 9 comments are addressed in the latest push. Regarding unification with
A single broad Ideally the security team co-owns |
Address all 9 review comments from venkkatesh-sekar: - Use persistent actor instead of stable keyword (Motoko) - Add init() method with race-window warning - Add removeAdmin for admin revocation - Rewrite Rust access control to use CDK guard pattern - Move compensation into finally/Drop cleanup context - Add call_on_cleanup mention for callback traps - Use replica-signed queries terminology Additional improvements: - Rewrite description and intro for agent consumption - Remove external links (agents cannot follow URLs) - Replace How It Works point 4 with state-rollback-on-trap - Add CallerGuard (per-caller locking) for both languages - Add CompensationGuard with Drop for trap-safe rollback - Use Call::bounded_wait with correct ic-cdk 0.19 API - Remove Verify It Works section (not actionable by agents) - Add placeholder comments for application-specific functions
|
@marc0olo is this freshly generated? I see a lot of differences from the previous review and it would be difficult for me do a full re review again. |
|
@venkkatesh-sekar it looks like a complete rewrite, but it's not. however, I provided some additional changes/removals which were independent of your review. I am not sure if a full re-review is needed, but I think it would be good. And sorry for applying additional changes in this PR. Would have been better to come up with a follow-up PR. Changes targeting your review:
Additional changes:
|
Keep both canister-security (new) and wallet-integration (renamed from oisy-wallet-signer on main).
2 participants
Summary
canister-securityskill covering 12 IC-specific security pitfalls with links to official DFINITY security best practices@dfinity/securityas CODEOWNER forskills/canister-security/canister-securityrather than inlining general security patternsRelationship to #57: Complementary. #57 adds multi-canister-specific security content (reentrancy in factory patterns, production readiness). This skill covers general IC security fundamentals that apply to all canisters. Both should coexist.