IEEE802.15.4 RxClient Encrypted Receive #3940
Conversation
|
There is no good reason to do this. This is an artifact of our current stack / trait design. Having a new trait as you suggested would fix this.
Crazy to hear this popped up in TinyOS too...
I am very much in favor of this. The only complicating factor (which I think we can work around) is the fact that the driver for all userspace processes using 15.4 is one |
|
I updated the I have included below diagrams of the prior design and the new design to help in understanding these changes. Prior Design: Proposed Design: |
|
I think this is a reasonable approach. |
|
@bradjc if others are in favor of these changes and once we merge this, I'll go ahead and update the tock book documentation we have on the network stack to incorporate the new trait. |
| @@ -135,3 +137,13 @@ pub trait RxClient { | |||
| /// - `data_len`: Length of the data payload | |||
| fn receive<'a>(&self, buf: &'a [u8], header: Header<'a>, data_offset: usize, data_len: usize); | |||
| } | |||
|
|
|||
| pub trait RawRxClient { | |||
There was a problem hiding this comment.
A comment here explaining that the fields here will match the explanation of RxClient would be nice. Also the comment should explain what differs from a normal RxClient (i.e., what "raw" actually means here)
There was a problem hiding this comment.
@brghena I just pushed changes adding these comments. (good catch on the missing documentation)
While I was adding this, I also softened the language to "potentially" including extra bytes for the PHY layer in the buf argument comment. This is to reflect #3933 removing the PSDU offset passed to RxClients that are "higher" in the stack.
There was a problem hiding this comment.
I don't really want to block the critical path to working demo, but I'm a bit confused on semantics here.
My instinct from the naming is that ReceiveRaw would get every packet, and Receive would only get 'processed' (in this context, unencrypted) packets.
If understand what happens around https://github.com/tock/tock/pull/3940/files#diff-85e0d54f818678f55fd9b21778b576635e2fdfb6e2fe633fd3e1fdc312dcbaa3R492 correctly, it seems that what happens is every packet will flow up either Receive or ReceiveRaw. In that sense, are these better named ReceiveDecrypted and ReceiveEncrypted?
Perhaps more importantly, are there layers above this that realistically want both? Wouldn't the layer above either be expected packets in the clear or expecting to encrypt? i.e., should the rx_client be a union enum for one of Receive[De/En]crypted?
Hmm.. maybe answering my own question, is the issue that link/security establishment for something capable of doing encryption needs access to both?
Even in that case, I still get hung up on something called ReceiveRaw not simply giving every packet to its client. Could we stick with the receiver enum, but the ReceiveRaw trait include a bool/flag which indicates whether the packet is encrypted? (re-read thread re: Brad's comment on type-checked enforcement) trait have two functions, receive_encrypted and receive_decrypted?
I agree with this.
This I'm not sure about. Is a packet that was never encrypted "decrypted"? I would say no. I end up at: we need something that works for now, and what the interfaces are seems like it should be a more holistic analysis. Is it a problem to just pass every packet to receive raw? |
|
Circling back to @ppannuto and @bradjc comments now... If I am understanding your concerns correctly, the issue is naming something Would perhaps a better way of doing this be to change |
|
Why not just pass everything to raw? |
|
So we would then have two traits: |
|
I think it is common to have two paths in a networking stack "raw", which on the RX side just passes everything, and on the TX side just sends packets without worrying about headers/structure, and the conventional path, which parses and checks headers. I think adding a raw interface, without any if statements (ie checking for encryption), makes this an easy change. |
|
@bradjc this sounds very reasonable to me but I am getting a little lost with our next steps to implement this. Would moving
In actuality, the only location the receive paths (raw / standard) diverge is in At the end of the day and after a while thinking about all of this, it seems that no one solution will perfectly fit into the existing stack design. The original stack was designed around having a generic |
|
I changed the naming of the trait to a verbose name. I think this should provide more clarity as to what this trait is (and isn't) used for. @ppannuto does this resolve your earlier concerns with "raw" being in the trait name? |
There was a problem hiding this comment.
I think in an ideal world, we'd pull raw out of all the function names too, but there's a point where it's just becoming busy-work for an ultimately temporary change to the networking stack.
I'm in favor of merging this and moving on, with the understanding that the more real re-write of the networking stack interfaces is on the short-term, post-tutorial priority list
|
@ppannuto I quickly went through and removed the function names and comments with raw. It wasn't too much work and will hopefully avoid any confusion between now and the rewrite. |
|
@tyler-potyondy It looks like this is back in your court due to a conflict. |
There now are 2 potential RxClients a MacDevice implements. With this design, a mac device can decide if it wishes to receive raw or parsed (i.e. decrypted) packets. Furthermore, a mac device can subscribe to both clients to receive both raw and parsed packets.
This implements the needed changes for passing received packets to correct clients and subsequently to the userprocess.
Using the verbose name `SecuredFrameNoDecryptRxClient`. This should be removed with the redesign.
|
Rebased and should be good to go. I also updated the |
Pull Request Overview
This pull request fixes the bug described in #3923. This provides a minimal change to the
RxClientreceivemethod to allow theRxClientto know if a packet remains encrypted and subsequently filter the packet if needed. Additional context for these changes can be found in the linked issue.Testing Strategy
This pull request was tested using the libtock-c openthread implementation.
TODO or Help Wanted
N/A
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.