Introduce ReceiveAuthKey #3917
Introduce ReceiveAuthKey #3917
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
cc @TheBlueMatt |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3917 +/- ##
==========================================
+ Coverage 88.84% 88.85% +0.01%
==========================================
Files 166 166
Lines 119487 119651 +164
Branches 119487 119651 +164
==========================================
+ Hits 106155 106321 +166
+ Misses 11008 11003 -5
- Partials 2324 2327 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
Oops, needs rebase. |
|
Updated from pr3917.03 to pr3917.04 (diff): Changes:
|
Haha, just a direct consequence of standing on the shoulders of giants. Thanks so much for the amazing |
There was a problem hiding this comment.
Looks like this needs rebase now, sorry about that. Once you do that and address #3917 (comment) I think this should be about ready to go.
| #[cfg(not(fuzzing))] | ||
| let mut mac = Poly1305::new(&mac_key[..32]); | ||
| #[cfg(fuzzing)] | ||
| let mut mac = Poly1305::new(&key); |
There was a problem hiding this comment.
I find this a bit confusing. Seems we can avoid it by updating the full_stack fuzzer tests to use zeroed-out MACs. This fixes the test_gossip_exchange_breakage test, similar changes would be needed in test_no_existing_test_breakage.
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 1d16b5c4a..db9d05f68 100644
--- a/fuzz/src/full_stack.rs
+++ b/fuzz/src/full_stack.rs
@@ -1579,41 +1579,40 @@ fn gossip_exchange_seed() -> Vec<u8> {
// inbound read from peer id 0 of len 50
ext_from_hex("030032", &mut test);
// noise act two (0||pubkey||mac)
- ext_from_hex("00 030000000000000000000000000000000000000000000000000000000000000002 03000000000000000000000000000000", &mut test);
+ ext_from_hex("00 030000000000000000000000000000000000000000000000000000000000000002 00000000000000000000000000000000", &mut test);
// inbound read from peer id 0 of len 18
ext_from_hex("030012", &mut test);
// message header indicating message length 16
- ext_from_hex("0010 03000000000000000000000000000000", &mut test);
+ ext_from_hex("0010 00000000000000000000000000000000", &mut test);
// inbound read from peer id 0 of len 32
ext_from_hex("030020", &mut test);
// init message (type 16) with static_remotekey required, no channel_type/anchors/taproot, and other bits optional and mac
- ext_from_hex("0010 00021aaa 0008aaa20aaa2a0a9aaa 03000000000000000000000000000000", &mut test);
+ ext_from_hex("0010 00021aaa 0008aaa20aaa2a0a9aaa 00000000000000000000000000000000", &mut test);
// new inbound connection with id 1
ext_from_hex("01", &mut test);
// inbound read from peer id 1 of len 50
ext_from_hex("030132", &mut test);
// inbound noise act 1
- ext_from_hex("0003000000000000000000000000000000000000000000000000000000000000000703000000000000000000000000000000", &mut test);
- // inbound read from peer id 1 of len 66
+ ext_from_hex("0003000000000000000000000000000000000000000000000000000000000000000700000000000000000000000000000000", &mut test); // inbound read from peer id 1 of len 66
ext_from_hex("030142", &mut test);
// inbound noise act 3
- ext_from_hex("000302000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003000000000000000000000000000000", &mut test);
+ ext_from_hex("000302000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", &mut test);
// inbound read from peer id 1 of len 18
ext_from_hex("030112", &mut test);
// message header indicating message length 16
- ext_from_hex("0010 01000000000000000000000000000000", &mut test);
+ ext_from_hex("0010 00000000000000000000000000000000", &mut test);
// inbound read from peer id 1 of len 32
ext_from_hex("030120", &mut test);
// init message (type 16) with static_remotekey required, no channel_type/anchors/taproot, and other bits optional and mac
- ext_from_hex("0010 00021aaa 0008aaa20aaa2a0a9aaa 01000000000000000000000000000000", &mut test);
+ ext_from_hex("0010 00021aaa 0008aaa20aaa2a0a9aaa 00000000000000000000000000000000", &mut test);
// inbound read from peer id 0 of len 18
ext_from_hex("030012", &mut test);
// message header indicating message length 432
- ext_from_hex("01b0 03000000000000000000000000000000", &mut test);
+ ext_from_hex("01b0 00000000000000000000000000000000", &mut test);
// inbound read from peer id 0 of len 255
ext_from_hex("0300ff", &mut test);
// First part of channel_announcement (type 256)
@@ -1621,25 +1620,25 @@ fn gossip_exchange_seed() -> Vec<u8> {
// inbound read from peer id 0 of len 193
ext_from_hex("0300c1", &mut test);
// Last part of channel_announcement and mac
- ext_from_hex("020202 00006fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000000000000000002a030303030303030303030303030303030303030303030303030303030303030303020202020202020202020202020202020202020202020202020202020202020202030303030303030303030303030303030303030303030303030303030303030303020202020202020202020202020202020202020202020202020202020202020202 03000000000000000000000000000000", &mut test);
+ ext_from_hex("020202 00006fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000000000000000002a030303030303030303030303030303030303030303030303030303030303030303020202020202020202020202020202020202020202020202020202020202020202030303030303030303030303030303030303030303030303030303030303030303020202020202020202020202020202020202020202020202020202020202020202 00000000000000000000000000000000", &mut test);
// inbound read from peer id 0 of len 18
ext_from_hex("030012", &mut test);
// message header indicating message length 138
- ext_from_hex("008a 03000000000000000000000000000000", &mut test);
+ ext_from_hex("008a 00000000000000000000000000000000", &mut test);
// inbound read from peer id 0 of len 154
ext_from_hex("03009a", &mut test);
// channel_update (type 258) and mac
- ext_from_hex("0102 00000000000000000000000000000000000000000000000000000000000000a60303030303030303030303030303030303030303030303030303030303030303 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000 000000000000002a0000002c01000028000000000000000000000000000000000000000005f5e100 03000000000000000000000000000000", &mut test);
+ ext_from_hex("0102 00000000000000000000000000000000000000000000000000000000000000a60303030303030303030303030303030303030303030303030303030303030303 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000 000000000000002a0000002c01000028000000000000000000000000000000000000000005f5e100 00000000000000000000000000000000", &mut test);
// inbound read from peer id 0 of len 18
ext_from_hex("030012", &mut test);
// message header indicating message length 142
- ext_from_hex("008e 03000000000000000000000000000000", &mut test);
+ ext_from_hex("008e 00000000000000000000000000000000", &mut test);
// inbound read from peer id 0 of len 158
ext_from_hex("03009e", &mut test);
// node_announcement (type 257) and mac
- ext_from_hex("0101 00000000000000000000000000000000000000000000000000000000000000280303030303030303030303030303030303030303030303030303030303030303 00000000002b03030303030303030303030303030303030303030303030303030303030303030300000000000000000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test);
+ ext_from_hex("0101 00000000000000000000000000000000000000000000000000000000000000280303030303030303030303030303030303030303030303030303030303030303 00000000002b03030303030303030303030303030303030303030303030303030303030303030300000000000000000000000000000000000000000000000000000000000000000000000000 00000000000000000000000000000000", &mut test);
test
}
diff --git a/lightning/src/crypto/chacha20poly1305rfc.rs b/lightning/src/crypto/chacha20poly1305rfc.rs
index 839fad9ce..f96869051 100644
--- a/lightning/src/crypto/chacha20poly1305rfc.rs
+++ b/lightning/src/crypto/chacha20poly1305rfc.rs
@@ -41,10 +41,10 @@ impl ChaCha20Poly1305RFC {
let zero_key = [0u8; 64];
cipher.process(&zero_key, &mut mac_key);
- #[cfg(not(fuzzing))]
+ // #[cfg(not(fuzzing))]
let mut mac = Poly1305::new(&mac_key[..32]);
- #[cfg(fuzzing)]
- let mut mac = Poly1305::new(&key);
+ // #[cfg(fuzzing)]
+ // let mut mac = Poly1305::new(&key);
mac.input(aad);
ChaCha20Poly1305RFC::pad_mac_16(&mut mac, aad.len());
There was a problem hiding this comment.
I can also do this in follow-up. Not a huge deal but would prefer to either do this or at least have a doc comment on this fuzzing-gated code.
There was a problem hiding this comment.
Unnecessarily changing the full_stack_target kinda sucks cause it breaks all the existing corpus files that exist. I'd kinda rather not :/
There was a problem hiding this comment.
Hmm, I guess that also already happens whenever a new feerate call is added? The diff above is a more significant change to the hardcoded strings though. Adding a comment seems fine too.
There was a problem hiding this comment.
Yea, it does happen somewhat frequently, though this breaks all the messages too, so was kinda trying to avoid it. I don't think its critical, but I just try to avoid where possible.
There was a problem hiding this comment.
I was looking through this part of code and came across something that felt related to this, so I just wanted to bring it to attention.
I noticed that the ChaCha20Poly1305RFC constructor accepts either a 16 or 32-byte key:
assert!(key.len() == 16 || key.len() == 32);But in the fuzzing implementation of Poly1305, the constructor always asserts a 32-byte key:
impl Poly1305 {
pub fn new(key: &[u8]) -> Poly1305 {
assert_eq!(key.len(), 32);
...
}
}I'm not sure why the fuzz code is set up this way. Would love to understand the reasoning behind it.
Thanks a lot! ✨
There was a problem hiding this comment.
AFAICT we always pass in a 32-byte key, so not sure if it's necessary to allow 16-byte keys. It's a fuzzing only potential issue so don't think it's a huge concern; in non-fuzzing builds, it looks like the code works when a 16-byte key is used. I'm guessing we support 16-byte keys in ChaCha20Poly1305RFC because that complies with the RFC.
There was a problem hiding this comment.
Got it! Since it’s just a fuzzing-only quirk, we can probably address it in a follow-up if it ever turns into an issue.
Thanks so much for the clarification! ✨
There was a problem hiding this comment.
LGTM after outstanding feedback. Let me know if you don't have time for this @shaavan, I'm going to be OOO next week so would like to get this in
`Poly1305::raw_result` copies the output into a slice, for some reason allowing any length sice. This isn't a great API, so while we're here we change it to return the 16-byte tag instead.
|
Updated from pr3917.06 to pr3917.07 (diff): Changes:
|
`ChaChaPolyReadAdapter` decodes an arbitrary object and checks the poly1305 tag. In the coming commits, we'll need a variant of this which allows for an *optional* AAD in the poly1305 tag, accepting either tag as valid, but indicating to the caller whether the AAD was used. We could use the actual AAD setup in poly1305, which puts the AAD first in the MAC (and then pads it out to a multiple of 16 bytes), but since we're gonna check both with and without, its nice to instead put the AAD at the end, enabling us to only calculate most of the hash once before cloning its state and adding the AAD block. We do this by swapping the AAD and the data being MAC'd in the AAD-containing MAC check (but leaving them where they belong for the non-AAD-containing MAC check). We also add a corresponding `chachapoly_encrypt_with_swapped_aad` which allows encrypting with the new MAC format.
In the upcoming commit, we introduce the usage of a new `ReceiveAuthKey` that will be used to authenticate message contexts in the received `BlindedMessagePath`s.
|
Updated from pr3917.07 to pr3917.08 (diff): Changes:
|
| ChaChaDualPolyReader { chacha: &mut chacha, poly: &mut mac, read_len: 0, read: s }; | ||
|
|
||
| let readable: T = Readable::read(&mut chacha_stream)?; | ||
| chacha_stream.read.eat_remaining()?; |
There was a problem hiding this comment.
Oops this is wrong, this bypasses the chacha_stream which means the extra data won't be included in the MAC but should be. This is an existing bug, however, in ChaChaPolyReadAdapter, so needs fixing in both.
There was a problem hiding this comment.
Shouldn't we just error if any bytes remain, since it means the length we were given for the data was wrong?
| // Note: We introduced the `control_tlvs_authenticated` check in LDK v0.2 | ||
| // to simplify and standardize onion message authentication. | ||
| // To continue supporting offers/refunds created before v0.2, we allow | ||
| // unauthenticated control TLVs for these messages, as they were verified | ||
| // using the legacy method. | ||
| Ok(PeeledOnion::Offers(msg, Some(ctx), reply_path)) |
There was a problem hiding this comment.
I don't think we want to push the logic down into OnionMessenger, rather we should expose the authenticated bit to the message handler which can decide whether we wanted an authenticated BP or not there.
valentinewallace
force-pushed
the
3845-mac
branch
2 times, most recently
from
7e751ba to
5bb9ffb
Compare
|
Pushed some docs updates and support for legacy refunds. Diff Edit: pushed one more change adding a newline to a doc to cause a paragraph break |
valentinewallace
force-pushed
the
3845-mac
branch
from
5bb9ffb to
f1f3c2f
Compare
When we receive an onion message, we often want to make sure it was sent through a blinded path we constructed. This protects us from various deanonymization attacks where someone can send a message to every node on the network until they find us, effectively unwrapping the blinded path and identifying its recipient. We generally do so by adding authentication tags to our `MessageContext` variants. Because the contexts themselves are encrypted (and MAC'd) to us, we only have to ensure that they cannot be forged, which is trivially accomplished with a simple nonce and a MAC covering it. This logic has ended up being repeated in nearly all of our onion message handlers, and has gotten quite repetitive. Instead, here, we simply authenticate the blinded path contexts using the MAC that's already there, but tweaking it with an additional secret as the AAD in Poly1305. This prevents forgery as the secret is now required to make the MAC check pass. Ultimately this means that no one can ever build a blinded path which terminates at an LDK node that we'll accept, but over time we've come to recognize this as a useful property, rather than something to fight. Here we finally break from the spec fully in our context encryption (not just the contents thereof). This will save a bit of space in some of our `MessageContext`s, though sadly not in the blinded path we include in `Bolt12Offer`s, so they're generally not in space-sensitive blinded paths. We can apply the same logic in our blinded payment paths as well, but we do not do so here. This commit only adds the required changes to the cryptography, for now it uses a constant key of `[41; 32]`. Co-authored-by: Shashwat Vangani <[email protected]>
valentinewallace
force-pushed
the
3845-mac
branch
from
f1f3c2f to
d3120ba
Compare
|
Removed legacy refund support and some references to async payments cfg-gated code. Diff |
| @@ -3761,7 +3707,7 @@ where | |||
| let flow = OffersMessageFlow::new( | |||
| ChainHash::using_genesis_block(params.network), params.best_block, | |||
| our_network_pubkey, current_timestamp, expanded_inbound_key, | |||
| secp_ctx.clone(), message_router | |||
| node_signer.get_receive_auth_key(), secp_ctx.clone(), message_router | |||
There was a problem hiding this comment.
Weird, this method is #[rustfmt::skip]'d. Fixed.
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 8331ed280..b873644ac 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -16431,7 +16431,8 @@ where
our_network_pubkey,
highest_seen_timestamp,
expanded_inbound_key,
- args.node_signer.get_receive_auth_key(), secp_ctx.clone(),
+ args.node_signer.get_receive_auth_key(),
+ secp_ctx.clone(),
args.message_router,
)
.with_async_payments_offers_cache(async_receive_offer_cache);There was a problem hiding this comment.
Upstream it was removed, I believe, so it will fail once this lands :)
This commit replaces the hardcoded key used for authenticating the context in incoming `BlindedMessagePath`s with a dedicated `ReceiveAuthKey`. This makes the authentication mechanism explicit and configurable for the user. Changes include: - Introducing `ReceiveAuthKey` to the `NodeSigner`, used to authenticate the context at the final hop of an incoming blinded path. - Updating `BlindedMessagePath::new` to accept a `ReceiveAuthKey` as a parameter during path construction.
Now that we have introduced an alternate mechanism for authentication in the codebase, we can safely remove the now redundant (hmac, nonce) fields from the MessageContext's while maintaining the security of the onion messages.
In the previous commit, we stopped authenticating incoming onion messages via an explicit nonce and hmac encoded in the message context, and entirely switched to authenticating via the new NodeSigner::get_receive_auth_key more or less included as AAD in the ChaCha20Poly1305 de/encoding for the messages. As such, the message handler docs need updating to describe the new authentication scheme.
valentinewallace
force-pushed
the
3845-mac
branch
from
d3120ba to
93e3708
Compare
Builds on #3845
This PR builds on the work in #3845 by introducing
ReceiveAuthKey, a dedicated struct that replaces the previously hardcoded[u8; 32]used for authenticatingMessageContexts in incomingBlindedMessagePaths.It:
ReceiveAuthKeystructNodeSignerinterfaceBlindedMessagePathconstructor to accept it as a parameterThis completes the original intent of #3845 — making the authentication mechanism explicit and less byte-heavy, while preserving the same security properties.
Next steps:
(HMAC, Nonce)pair from relevantMessageContextvariants