Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 31 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdded two documentation pages: Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Contract
participant TEE_Worker
participant Chain
Note over Client,Contract: Bootstrap / Project Init (per-message.md flow)
Client->>Contract: register project / request init
Contract->>TEE_Worker: request intermediate/key derivation
TEE_Worker->>TEE_Worker: derive intermediate (hidden) + per-project child pubkey
TEE_Worker-->>Contract: return per-project child public key (only)
Contract->>Chain: publish per-project child public key (optional)
Client->>Contract: submit encrypted secret / task
Contract->>TEE_Worker: provide ciphertext + metadata
TEE_Worker->>TEE_Worker: decrypt/derive app key inside TEE, run WASM, produce proof
TEE_Worker-->>Contract: submit proof
Contract->>Chain: submit/verify proof on-chain
sequenceDiagram
participant App
participant Contract
participant TEE_Worker
participant Chain
Note over App,Contract: CKD API flow (docs/v2.md)
App->>Contract: call public_key(domain_id?)
Contract->>TEE_Worker: request MPC master pubkey / domain handling
TEE_Worker-->>Contract: return BLS12-381 G1 point (compressed)
App->>Contract: request_app_private_key(path, app_public_key, domain_id) + 1 yoctoNEAR
Contract->>TEE_Worker: forward request, app_public_key, path
TEE_Worker->>TEE_Worker: compute derived secret, optionally encrypt via HMAC seed or X25519 ECDHE
TEE_Worker-->>Contract: return wrapped secret or confirmation
Contract->>App: deliver response
Note over App,Chain: App uses derived material locally to form final app private key
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/message.md (1)
84-84: Make the open question a hard decision gate.Good callout, but this should be framed as a blocking criterion (“do not ship until proven non-reconstructible from published child public keys”) with a concrete verification method and acceptance criteria.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/message.md` at line 84, Change the "Open question" note about the isolation guarantee and the intermediate public key into a blocking decision: replace the phrase "Open question" and passive wording with an explicit gate such as "Must not ship" and a requirement line stating "Do not ship until it is proven that the intermediate public key cannot be reconstructed from published child public keys." Add a concrete verification method and acceptance criteria: describe the validation test (e.g., formal proof or cryptanalysis attempt demonstrating non-reconstructibility), required artifacts (test results, cryptographic proof, or external audit), and the condition for acceptance (evidence that reconstruction probability is negligible or audit signed off). Reference the terms "isolation guarantee" and "intermediate public key" so reviewers can locate and validate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/message.md`:
- Around line 47-48: Update the docs where it says the worker "hands the child
private key to the WASM" to include a short security subsection enumerating
required trust-boundary safeguards: mandate only trusted/attested WASM runtimes,
enforce non-exportability of keys (no host calls that return key material),
require memory zeroization after use, set short key lifetimes/rotation policies,
and explicitly prohibit/mitigate exfiltration channels (network, logging, host
APIs); apply the same safeguards to the other occurrence of this design
described nearby so both places contain the minimal protective requirements.
- Line 1: Replace the absolute claim that "Each derivation is fully isolated"
with a conditional statement that specifies the isolation depends on the
derivation scheme (e.g., "Under hardened derivation schemes, knowledge of a
derived private key does not reveal the master key or other hardened child
keys"); explicitly add a caveat referencing the non-hardened derivation failure
mode described later (lines 45–46) by saying non-hardened derivation can permit
key recovery under certain compromised conditions, so implementations must not
assume isolation for non-hardened paths.
---
Nitpick comments:
In `@docs/message.md`:
- Line 84: Change the "Open question" note about the isolation guarantee and the
intermediate public key into a blocking decision: replace the phrase "Open
question" and passive wording with an explicit gate such as "Must not ship" and
a requirement line stating "Do not ship until it is proven that the intermediate
public key cannot be reconstructed from published child public keys." Add a
concrete verification method and acceptance criteria: describe the validation
test (e.g., formal proof or cryptanalysis attempt demonstrating
non-reconstructibility), required artifacts (test results, cryptographic proof,
or external audit), and the condition for acceptance (evidence that
reconstruction probability is negligible or audit signed off). Reference the
terms "isolation guarantee" and "intermediate public key" so reviewers can
locate and validate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| @@ -0,0 +1,87 @@ | |||
| In the MPC contract, knowing a derived private key for one derivation path tells you nothing about any other derived key or the master private key. Each derivation is fully isolated. | |||
There was a problem hiding this comment.
Security claim is too absolute and internally inconsistent.
Line 1 says derivation is “fully isolated,” but Lines 45–46 explicitly describe a non-hardened derivation failure mode where key recovery is possible under certain conditions. Please rewrite Line 1 to be conditional on the derivation scheme (e.g., hardened-only assumptions) to avoid unsafe implementation assumptions.
Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/message.md` at line 1, Replace the absolute claim that "Each derivation
is fully isolated" with a conditional statement that specifies the isolation
depends on the derivation scheme (e.g., "Under hardened derivation schemes,
knowledge of a derived private key does not reveal the master key or other
hardened child keys"); explicitly add a caveat referencing the non-hardened
derivation failure mode described later (lines 45–46) by saying non-hardened
derivation can permit key recovery under certain compromised conditions, so
implementations must not assume isolation for non-hardened paths.
Comment on lines
+47
to
+48
| **Note:** This also removes the need for the worker to manage secret decryption on behalf of WASM. Instead of injecting decrypted secrets as environment variables, the worker simply hands the child private key to the WASM and lets it handle everything. | ||
|
|
There was a problem hiding this comment.
Injecting child private keys into WASM needs explicit trust-boundary constraints.
This design hands long-lived key material to WASM directly. Please document minimum safeguards (trusted/attested WASM only, non-exportability policy, memory zeroization, key lifetime, and explicit prohibition of exfiltration paths), otherwise this can negate the isolation benefits.
Also applies to: 79-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/message.md` around lines 47 - 48, Update the docs where it says the
worker "hands the child private key to the WASM" to include a short security
subsection enumerating required trust-boundary safeguards: mandate only
trusted/attested WASM runtimes, enforce non-exportability of keys (no host calls
that return key material), require memory zeroization after use, set short key
lifetimes/rotation policies, and explicitly prohibit/mitigate exfiltration
channels (network, logging, host APIs); apply the same safeguards to the other
occurrence of this design described nearby so both places contain the minimal
protective requirements.
pityjllk
force-pushed
the
review/outlayer
branch
2 times, most recently
from
084a050 to
433b5c8
Compare
pityjllk
force-pushed
the
review/outlayer
branch
from
433b5c8 to
fb3b439
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/v2.md (1)
19-19: Add language identifiers to fenced code blocks.Markdown fences should specify a language (
text,rust, etc.) to satisfy linting and improve readability.Also applies to: 26-26, 45-45, 53-53, 72-72, 82-82, 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/v2.md` at line 19, Several fenced code blocks in the document are missing language identifiers; locate each triple-backtick fence (```) and add an appropriate language tag (e.g., text, rust, bash) after the opening fence so every fenced block is like ```rust or ```text to satisfy linting and improve readability; update all occurrences of ``` in the file to include the correct language label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/v2.md`:
- Line 60: The proposed X25519 construction currently derives a long-lived key
directly from HMAC-SHA256(master_secret) which is underspecified and unsafe;
update the design to insert a KDF step (e.g., HKDF-SHA256) with explicit
per-domain/per-project/per-version salt and info labels (use distinct info
strings like "x25519-key|project:<project_id>|version:<v>"), define key
lifetimes and rotation rules (max age, rotation trigger, and how to re-encrypt
persistent data), and describe how the TEE worker derives the ephemeral X25519
keypair from HKDF output (seed -> X25519 scalar) while keeping
predecessor_id-based request acceptance separate; ensure you reference the
original symbols (HMAC-SHA256(master_secret), X25519, predecessor_id, TEE
worker) when documenting the KDF inputs, outputs, and lifecycle so implementers
can locate and apply the change.
- Line 7: The sentence in docs/v2.md contains multiple typos and inconsistent
terminology that reduce clarity and precision for a security design: locate the
occurrences of "entites", "view only", "symetric", "Outalyer", "secretes",
"beeing", "parend" and the line with "TEE", "CKD" and "app_private_key" and
correct spelling and phrasing (e.g., "entities", "view-only", "symmetric",
"Outlayer" if that is the intended term or replace with the correct component
name, "secrets", "being", "parent") and make terminology consistent across the
file (also fix the other occurrences referenced at lines 25, 57, 67, 79) so the
sentence reads precisely (for example: "In the case of TEE, we only depend on
CKD. The benefit of CKD is that entities running in a TEE can access the same
app_private_key and derive the same keypair from it."). Ensure consistent
hyphenation and camel/underscore use for symbols like app_private_key and
key-pair throughout the document.
---
Nitpick comments:
In `@docs/v2.md`:
- Line 19: Several fenced code blocks in the document are missing language
identifiers; locate each triple-backtick fence (```) and add an appropriate
language tag (e.g., text, rust, bash) after the opening fence so every fenced
block is like ```rust or ```text to satisfy linting and improve readability;
update all occurrences of ``` in the file to include the correct language label.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1825880-45b9-446c-a330-7ff0283de5be
📒 Files selected for processing (2)
docs/message.mddocs/v2.md
✅ Files skipped from review due to trivial changes (1)
- docs/message.md
| - Chain signatures — users submit a message to the contract, and the MPC network collaboratively produces a threshold ECDSA or EdDSA signature without any single node holding the complete private key. | ||
| - Confidential key derivation (CKD) — users submit an ephemeral public key and a derivation path, and the MPC network produces an application-specific private key encrypted so that only the requester can unwrap it. The derived key is a BLS signature over BLS12-381, with the property that anyone can verify it was honestly derived from the network's master key without the master key ever being revealed. | ||
|
|
||
| In the case of TEE, we only depend on CKD. The benefit of CKD is that we can have entites running in TEE that will have access to the same `app_private_key` and possibly derive the same key-pair from it. |
There was a problem hiding this comment.
Fix documentation typos and wording for precision.
There are multiple spelling/wording issues that reduce clarity in a security-sensitive design doc (entites, view only, symetric, Outalyer, secretes, beeing, parend). Please correct these and keep terminology consistent.
Also applies to: 25-25, 57-57, 67-67, 79-79
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: Ensure spelling is correct
Context: ... The benefit of CKD is that we can have entites running in TEE that will have access to...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/v2.md` at line 7, The sentence in docs/v2.md contains multiple typos and
inconsistent terminology that reduce clarity and precision for a security
design: locate the occurrences of "entites", "view only", "symetric",
"Outalyer", "secretes", "beeing", "parend" and the line with "TEE", "CKD" and
"app_private_key" and correct spelling and phrasing (e.g., "entities",
"view-only", "symmetric", "Outlayer" if that is the intended term or replace
with the correct component name, "secrets", "being", "parent") and make
terminology consistent across the file (also fix the other occurrences
referenced at lines 25, 57, 67, 79) so the sentence reads precisely (for
example: "In the case of TEE, we only depend on CKD. The benefit of CKD is that
entities running in a TEE can access the same app_private_key and derive the
same keypair from it."). Ensure consistent hyphenation and camel/underscore use
for symbols like app_private_key and key-pair throughout the document.
| Now project owner needs to encrypt secrets and provide them confidentially to TEE worker. In origin outlayer implementation it was used symetric encryption. Basically Outalyer was enforcing particular encryption key for the secretes derived from `app_private_key` and `project_id`. That means that it wasn't possible to rotate the secrets, and that there had to be a way to confidentially provide a symmetric encryption key to the project owner. | ||
|
|
||
|
|
||
| That could be solved with a better approach namely [ECDHE](https://en.wikipedia.org/wiki/Elliptic-curve_Diffie%E2%80%93Hellman). Basically the TEE worker could generate X25519 public key from `HMAC-SHA256(master_secret)`. And anyone could then provide secrets encrypted using an ephemeral keypair to the contract. Then the project owner knowing that public key (probably persisted on chain) can simply provide new secrets and TEE can accept an encrypted request based on `predecessor_id`. |
There was a problem hiding this comment.
Clarify and harden the X25519 key-derivation construction.
Deriving a long-lived X25519 key directly from HMAC-SHA256(master_secret) is underspecified and risky (cross-context key reuse, unclear domain separation/rotation semantics). Please specify a KDF step (e.g., HKDF with explicit salt/info per domain/project/version) and key lifecycle/rotation rules before adopting this scheme.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/v2.md` at line 60, The proposed X25519 construction currently derives a
long-lived key directly from HMAC-SHA256(master_secret) which is underspecified
and unsafe; update the design to insert a KDF step (e.g., HKDF-SHA256) with
explicit per-domain/per-project/per-version salt and info labels (use distinct
info strings like "x25519-key|project:<project_id>|version:<v>"), define key
lifetimes and rotation rules (max age, rotation trigger, and how to re-encrypt
persistent data), and describe how the TEE worker derives the ephemeral X25519
keypair from HKDF output (seed -> X25519 scalar) while keeping
predecessor_id-based request acceptance separate; ensure you reference the
original symbols (HMAC-SHA256(master_secret), X25519, predecessor_id, TEE
worker) when documenting the KDF inputs, outputs, and lifecycle so implementers
can locate and apply the change.
b145080 to
d247f04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/v2.md (2)
60-61:⚠️ Potential issue | 🟠 MajorHarden X25519 derivation spec with explicit KDF/domain separation/rotation.
Line 60 still describes deriving a long-lived X25519 key directly from
HMAC-SHA256(master_secret). Please specify HKDF inputs (salt/infowith domain+project+version labels), key lifetime/rotation, and re-encryption behavior to avoid cross-context reuse and ambiguous lifecycle semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/v2.md` around lines 60 - 61, Clarify and harden the X25519 derivation: replace the current direct derivation HMAC-SHA256(master_secret) with an explicit HKDF call and document its inputs (use a per-key salt and an info string that encodes domain, project_id, and version labels), define key lifetime/rotation policy (e.g., TTL or version bump semantics tied to the version label and rotation triggers), specify when and how re-encryption occurs (e.g., on rotation re-encrypt stored secrets with a new ephemeral X25519 derived key and retain previous keys for grace period), and update references to X25519, HKDF, TEE worker, and predecessor_id to ensure no cross-context reuse by including domain/project/version in both salt/info and persisted public key metadata.
7-7:⚠️ Potential issue | 🟡 MinorFix remaining terminology/typos in security-critical wording.
Line 7/57/67/79 still contain spelling and wording issues (
entites,symetric,secretes,beeing,parend, etc.), which reduces precision in a design doc.Suggested doc patch
-In the case of TEE, we only depend on CKD. The benefit of CKD is that we can have entites running in TEE that will have access to the same `app_private_key` and possibly derive the same key-pair from it. +In the case of TEE, we only depend on CKD. The benefit of CKD is that entities running in a TEE can access the same `app_private_key` and derive the same key pair from it. -Now project owner needs to encrypt secrets and provide them confidentially to TEE worker. In origin outlayer implementation it was used symetric encryption. Basically Outalyer was enforcing particular encryption key for the secretes derived from `app_private_key` and `project_id`. +Now the project owner needs to encrypt secrets and provide them confidentially to the TEE worker. In the original Outlayer implementation, symmetric encryption was used. Outlayer enforced a specific encryption key for secrets derived from `app_private_key` and `project_id`. -- in case of child private key beeing exposed only that single project credentials should be exposed(B) +- in case of child private key being exposed, only that single project's credentials should be exposed (B) -Allows for deriving public key without revealing the private key. BUT **by having access to child_private_key and pub_parent anyone can recover parent private key**. For this reason its mainly used in wallets. Where single entity owns all parend and child private keys. +Allows deriving a public key without revealing the private key. BUT **with access to child_private_key and pub_parent, anyone can recover the parent private key**. For this reason, it is mainly used in wallets, where a single entity owns all parent and child private keys.Also applies to: 57-57, 67-67, 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/v2.md` at line 7, Fix the spelling and wording issues in docs/v2.md by correcting typos and tightening security-critical terminology: change "entites" to "entities", "symetric" to "symmetric", "secretes" to "secrets", "beeing" to "being", "parend" to "parent", and revise the phrasing around TEE/CKD to read e.g. "In the case of TEE, we depend solely on CKD" and "entities running in a TEE will have access to the same app_private_key and may derive the same key-pair from it" to improve precision; apply the same corrections at the other occurrences mentioned (lines ~57, ~67, ~79).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/v2.md`:
- Line 19: Several fenced code blocks in the markdown are missing language
identifiers (plain ```), causing markdownlint MD040 warnings; update each bare
triple-backtick block to include the appropriate language tag (e.g., ```text,
```rust, ```bash) consistently across the file so the blocks render and lint
correctly—search for the plain ``` fences in docs/v2.md and replace them with
the matching language-specific fence for the snippet content.
---
Duplicate comments:
In `@docs/v2.md`:
- Around line 60-61: Clarify and harden the X25519 derivation: replace the
current direct derivation HMAC-SHA256(master_secret) with an explicit HKDF call
and document its inputs (use a per-key salt and an info string that encodes
domain, project_id, and version labels), define key lifetime/rotation policy
(e.g., TTL or version bump semantics tied to the version label and rotation
triggers), specify when and how re-encryption occurs (e.g., on rotation
re-encrypt stored secrets with a new ephemeral X25519 derived key and retain
previous keys for grace period), and update references to X25519, HKDF, TEE
worker, and predecessor_id to ensure no cross-context reuse by including
domain/project/version in both salt/info and persisted public key metadata.
- Line 7: Fix the spelling and wording issues in docs/v2.md by correcting typos
and tightening security-critical terminology: change "entites" to "entities",
"symetric" to "symmetric", "secretes" to "secrets", "beeing" to "being",
"parend" to "parent", and revise the phrasing around TEE/CKD to read e.g. "In
the case of TEE, we depend solely on CKD" and "entities running in a TEE will
have access to the same app_private_key and may derive the same key-pair from
it" to improve precision; apply the same corrections at the other occurrences
mentioned (lines ~57, ~67, ~79).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| - AppPublicKeyPV(pk1, pk2) — G1 + G2 pair, publicly verifiable variant | ||
| **NOTE** In both cases the only thing that can be verified is that the `app_private_key` was correctly derived from the master private key for a particular derivation path based on `env::predecessor_account_id` and `derivation_path`. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (MD040).
These fences are missing language tags and will keep markdownlint warnings active. Please annotate them (text, rust, etc.) consistently.
Also applies to: 26-26, 45-45, 53-53, 72-72, 82-82, 91-91
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/v2.md` at line 19, Several fenced code blocks in the markdown are
missing language identifiers (plain ```), causing markdownlint MD040 warnings;
update each bare triple-backtick block to include the appropriate language tag
(e.g., ```text, ```rust, ```bash) consistently across the file so the blocks
render and lint correctly—search for the plain ``` fences in docs/v2.md and
replace them with the matching language-specific fence for the snippet content.
d247f04 to
a19dd5d
Compare
pityjllk
force-pushed
the
review/outlayer
branch
from
a19dd5d to
dfe7307
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit