WIP: eliminate leak of node secret key from KeysInterface #1362
Conversation
| /// | ||
| /// This method must return the same value each time it is called with a given `Recipient` | ||
| /// parameter. | ||
| fn get_node_secret(&self, recipient: Recipient) -> Result<SecretKey, ()>; |
There was a problem hiding this comment.
The goal is to eliminate this function.
devrandom
force-pushed
the
2022-03-protocol-encryptor
branch
from
7697e7c to
3de8c72
Compare
devrandom
force-pushed
the
2022-03-protocol-encryptor
branch
from
3de8c72 to
f7c4c6b
Compare
Codecov Report
@@ Coverage Diff @@
## main #1362 +/- ##
==========================================
- Coverage 90.65% 90.63% -0.03%
==========================================
Files 73 72 -1
Lines 40462 40539 +77
==========================================
+ Hits 36682 36741 +59
- Misses 3780 3798 +18
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hmm, maybe best to just leave the private key in PeerManager for now as a first step and just address the ChannelManager parts? That would let us drop get_node_secret from KeysInterface, move it to KeysManager, and then we can address the desire for a ful shared secret there. I'm kinda meh on the general idea of a ECDH oracle in the signer interface, I'd really rather do "ECDH+get_rho_mu_from_shared_secret" (or um or ammag) all in one go instead to narrow the interface a bit.
| /// A trait to describe an object which can get user secrets and key material. | ||
| pub trait KeysInterface { | ||
| /// A type which implements Sign which will be returned by get_channel_signer. | ||
| type Signer : Sign; | ||
|
|
||
| /// Get node secret key (aka node_id or network_key) based on the provided [`Recipient`]. | ||
| /// A shared secret producer using the node key | ||
| fn get_shared_secret_producer(&self) -> Box<dyn SharedSecretProduce>; |
There was a problem hiding this comment.
Not quite sure I understand the need for this trait here, and super not a fan of box-dyn in our interface.
There was a problem hiding this comment.
The trait replaces SecretKey in several places. It seems particularly necessary in DirectionalNoiseState, so that low-level protocol enum doesn't get exposed to the full KeysInterface.
The Box<dyn> was expedient for the draft - a generic would propagate into a lot of peer protocol structs.
|
Here are some thoughts:
|
|
Hmm, doing onion decryption inside the signer seems fine, but I'm pretty dubious of hiding the contents in it such that the |
|
That said, I'm not sure what thatwould provide that just returning the hashed shared secrets doesn't, except requiring copying more data... |
|
I'm still trying to grasp better the security gain of decrypting the onion on the signer side instead of the node. Like it could make sense for application where the onion payload is confidential content at destination of the node user herself but right now it's all processing data. Or maybe you're thinking about enforcing policies on the onion content itself before to yield it back to the local node ? |
|
The main example that comes to mind is keysend. If the preimage leaks to a node other than the final recipient, the payment can be taken by an intermediate node. |
Ugh, this just feels like a failure of the keysend design - lack of proof-of-payment sucks, yo :(. If there's no other issue with letting the Do you share the concern about upgradability? It seems kinda annoying to require a signer update to get any new onion TLV passed through, and I'd imagine c-lightning's current plugin archtecture assumes all the TLVs can be exposed. |
Sure.
Well, the signer could pass through anything it doesn't understand. That said, I don't feel strongly about this, and passing onions through the signer does have a performance impact. So the ECDH-derived-key approach sounds good to me. |
Hmm, true, of course that means new protocols default to "insecure" rather than "secure", which I guess is the key distinction. Anyway, we can revisit this later if keysend is still materially around in a few years. |
Yes though moving onion decryption on the Signer side doesn't improve security in case of Node compromise, as the Node has to load in-memory the keysend preimage to fetch payment from counterparty ? If the payment path encryption worked well, your intermediate nodes won't learn the preimage, independently of where on the final host the decryption happens, I think. Beyond, I lean to agree with Matt here, if we have payment/onion protocols in the future where it makes sense to minimize the trust towards the Node, we can rethink about it. |
|
@ariard - my comment about keysend is about the security of the sender, not the recipient. The issue is if the preimage is leaked to an intermediate node, which then takes the funds from the sender without propagating the HTLC to the intended final recipient. But anyway, I agree that it's not worth designing around keysend right now. |
Gotcha, you're correct that's a viable vector of keysend attack in case of Node compromise. That lets me think, if I compromise your Node and I don't control your payees, can I feed you malicious payment paths, where all the intermediate nodes are mine and the routing fees are inflated. I see in |
Yes, we do have a policy parameter |
|
Do you still intend to complete this, @devrandom? |
|
yes, I do |
|
Didn't realize this existed. I opened #1951 to address it. |
|
Superseded by #1951. |
No description provided.