Repository navigation
feat(discovery): secret zeroization, request limits, error sanitization - #510
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
2817181 to
6d1c30b
Compare
6d1c30b to
b07e567
Compare
b07e567 to
de329a4
Compare
4894f41 to
1a716aa
Compare
8fa742f to
852caed
Compare
1a716aa to
3a51071
Compare
852caed to
d1127ea
Compare
3a51071 to
a6531a6
Compare
d1127ea to
b6b94bf
Compare
a6531a6 to
6a3c48e
Compare
b6b94bf to
e22a8e5
Compare
6a3c48e to
c8443b9
Compare
e22a8e5 to
6c5d66c
Compare
c8443b9 to
0087239
Compare
6c5d66c to
bf77877
Compare
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: 3 of 43 files reviewed, 2 unresolved discussions (waiting on m-kus).
crates/discovery-core/src/test_fixtures.rs line 73 at r2 (raw file):
pub auditor_public_key: Felt, pub user_addr: Felt, pub user_private_key: Felt,
Anything else that should be marked as secret?
Code quote:
pub auditor_private_key: Felt,
pub auditor_public_key: Felt,
pub user_addr: Felt,
pub user_private_key: Felt,crates/discovery-core/src/privacy_pool/types.rs line 79 at r2 (raw file):
Felt::deserialize(d).map(SecretFelt::new) } }
Why in a separate mod?
Code quote:
pub mod secret_felt_serde {
use super::*;
use serde::{Deserializer, Serializer};
pub fn serialize<S>(secret: &SecretFelt, s: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
Felt::serialize(&secret.0, s)
}
pub fn deserialize<'de, D>(d: D) -> Result<SecretFelt, D::Error>
where
D: Deserializer<'de>,
{
Felt::deserialize(d).map(SecretFelt::new)
}
}
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware made 1 comment.
Reviewable status: 3 of 43 files reviewed, 2 unresolved discussions (waiting on m-kus).
crates/discovery-core/src/privacy_pool/types.rs line 79 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Why in a separate mod?
I.e., why not make these the default serde for SecretFelt?
m-kus
left a comment
There was a problem hiding this comment.
@m-kus made 2 comments.
Reviewable status: 3 of 43 files reviewed, 2 unresolved discussions (waiting on Yoni-Starkware).
crates/discovery-core/src/test_fixtures.rs line 73 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Anything else that should be marked as secret?
These are for tests anyways, I just made secret the ones we need - to reduce the casting boilerplate
crates/discovery-core/src/privacy_pool/types.rs line 79 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
I.e., why not make these the default serde for SecretFelt?
To avoid unintentional serialization
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware reviewed 2 files, made 1 comment, and resolved 1 discussion.
Reviewable status: 5 of 43 files reviewed, 2 unresolved discussions (waiting on m-kus).
crates/discovery-core/src/discovery/cursor.rs line 82 at r2 (raw file):
pub struct ChannelCursor { // TODO: Consider encrypting/masking channel_key in the serialized cursor // to avoid exposing it in plaintext (sensitive value).
The channel key is still exposed in the serialized cursor, right? Was this your intention? It doesn't seem so
Code quote:
// TODO: Consider encrypting/masking channel_key in the serialized cursor
// to avoid exposing it in plaintext (sensitive value).e27a5ed to
7115da6
Compare
e60326a to
1a65d84
Compare
m-kus
left a comment
There was a problem hiding this comment.
@m-kus made 1 comment.
Reviewable status: 4 of 46 files reviewed, 2 unresolved discussions (waiting on Yoni-Starkware).
crates/discovery-core/src/discovery/cursor.rs line 82 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
The channel key is still exposed in the serialized cursor, right? Was this your intention? It doesn't seem so
Resolved (likely rebasing artifact)
1a65d84 to
40f2ed6
Compare
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware made 1 comment and resolved 1 discussion.
Reviewable status: 4 of 46 files reviewed, 1 unresolved discussion (waiting on m-kus).
crates/discovery-core/src/discovery/cursor.rs line 82 at r2 (raw file):
Previously, m-kus (Michael Zaikin) wrote…
Resolved (likely rebasing artifact)
No, I mean that the serde of channel_key looks like a normal Felt serde, while your TODO says that you want to mask it in the serialized cursor.
40f2ed6 to
a63997f
Compare
7115da6 to
522935b
Compare
m-kus
left a comment
There was a problem hiding this comment.
@m-kus made 1 comment.
Reviewable status: 4 of 46 files reviewed, 1 unresolved discussion (waiting on m-kus and Yoni-Starkware).
crates/discovery-core/src/discovery/cursor.rs line 82 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
No, I mean that the serde of channel_key looks like a normal Felt serde, while your TODO says that you want to mask it in the serialized cursor.
Ah got it, yeah that's the intention - use secretfelt for sensitive keys, not adding additional app-level encryption (rely on tls)
522935b to
a18d822
Compare
a63997f to
5a1f287
Compare
a18d822 to
e41eea7
Compare
5a1f287 to
2a855f3
Compare
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware partially reviewed 42 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on m-kus).
2a855f3 to
a79525b
Compare
a79525b to
ca4b658
Compare

Implement SecretFelt for Sensitive Key Management
This PR enhances security by implementing proper handling of sensitive cryptographic material throughout the discovery service:
Adds
SecretFeltwrapper for sensitive keys with:[REDACTED]instead of key values)Copytrait to prevent accidental duplicationApplies
SecretFeltto all sensitive keys:viewing_keyin API requestschannel_keyin cursors and channel objectsImproves error sanitization:
Adds HTTP-level protections:
Updates security documentation to reflect implemented mitigations
This change is