Wireguard Review 2021-03-12 #1

nickray · 2021-03-12T13:26:27Z

No description provided.

…rial into wireguard

nickray · 2021-03-12T13:30:04Z

src/cli.rs

+    let mut arr : [u8; 32] = [0;32];
+    let chars = hex::decode( s);
+
+    for (index, c) in chars.unwrap().iter().enumerate()
+    {
+        arr[index] = c.clone();
+    }
+
+   return Ok(arr.clone())
+}


Try getting acquainted with "standard Rust traits", mainly From/Into and their fallible variants TryFrom/TryInto.

Also combinators on Result, such as map/map_err, and the ? operator.

The key to readable/idiomatic Rust code (in my opinion) is to separate data conversion (such as this) from program logic.

E.g. here something like:

let decoded = hex::decode(s) use core::conver::TryFrom; let arr: [u8; 32] = decoded.try_int().map_err(|_| anyhow::anyhow!("String too long"))?; Ok(arr)

You can also always run cargo clean; cargo clippy to get hints from the compiler. E.g. bytes are Copy, so these clones are redundant.

nickray · 2021-03-12T13:36:37Z

src/cli.rs

+                        let pin = match command.value_of("PIN") {
+                            Some(s) => {s},
+                            None => {return Err(anyhow::anyhow!("Could not parse pin"));}
+
+                        };
+                       return Ok(WgCommand::Unlock(Unlock {


Just stylistic, but similar to the hex_string_to32_char_array below, combinators compactify this kind of thing:

let pin = command.value_of("PIN").ok_or_else(|| anyhow::anyhow!("Could not parse pin"))?.to_string(); return Ok(WgCommand::Unlock(Unlock { pin }))

Note that return is usually only used for explicit early error returns, if you can setup the code that the main Result of the function is "at then end", you can leave out the return keyword.

nickray · 2021-03-12T13:37:22Z

src/main.rs

+    //setup wireguard
+    let trussed_platform_wg = platform::init_platform(state_file.clone());
+    let mut trussed_service_wg = trussed::service::Service::new(trussed_platform_wg);
+    let client_id_wg = "wireguard";


Could use wg here, as this client ID goes in all filenames on the flash. Premature optimization probably xD

nickray · 2021-03-12T13:39:50Z

src/main.rs

+    match wg_command {
+       wireguard::WgCommand::Unlock(unlock) => 
+        {


A pattern I use here is bringing all the enum variants in scope:

use wireguard::WgCommand::*; match command { Unlock(unlock) => (...)

Also, generally it's a C-ism to use prefixes like wg_ or Wg. When the WgCommand is already in the wireguard module, it's clear that wireguard::Command refers to a WireGuard command. If you want to import without the module path prefix, you can do that at call site: use wireguard::Command as WgCommand.

nickray · 2021-03-12T13:40:22Z

src/main.rs

+        {
+
+        }


I'd put a todo!("handle Command::UpdateKeypair") or similar here, instead of an empty match.

nickray · 2021-03-12T13:42:15Z

src/wireguard.rs

+Perhaps not interesting for NPX: 
+
+    set time                                     - No RTC available on the STM32, that was capable of incrementing a day/month/year. 
+                                                  Restricted to time exclusively. Solution: transmit the current epoch and extract the date.
+
+    get time                                     - get the device time. Mostly to check, whether a new timestamp is needed.


Not sure I understand this. I'd never rely on an RTC if this is about a timestamp that's relevant to the protocol, this should come from a desktop/client (which hopefully has some kind of NTP set up).

nickray · 2021-03-12T13:43:02Z

src/wireguard.rs

+
+no permissions / unknown user : 
+
+     Authenticate                    params: PIN/pass         - Unlocks the device to use it. 


Just FYI, I am planning to add a "global PIN" to Trussed, so apps don't necessarily have to do their own PIN handling. Depending on the app, they of course may choose to do so all the same.

nickray · 2021-03-12T13:44:48Z

src/wireguard.rs

+ #[allow(missing_docs)]
+ pub struct GetAead 
+ {
+     pub pubkey: [u8;SIZE_PUBKEY],


Ideally this is a newtype PublicKey([u8; SIZE_PUBKEY]), with appropriate TryFrom/TryInto implementations (possibly also normal constructors inherent to the struct, not coming from traits). The idea being that by making the inner array not pub, the constructor/converter can enforce validity of the data (e.g., the bag-of-bytes is a valid point on the curve).

nickray · 2021-03-12T13:45:58Z

src/wireguard.rs

+ {
+     pub privkey : [u8; SIZE_PRIVKEY],
+     pub pubkey : [u8; SIZE_PUBKEY],
+     pub label : String


Probably not wrong to start using heapless::String<CAPACITY> soon-ish, as you won't have alloc::String on the device.

nickray · 2021-03-12T13:46:51Z

src/wireguard.rs

+impl core::fmt::Display for AEAD {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "{:x?}", self.0)
+    }
+}


This seems more like a Debug impl to me than a Display (which is intended for final / user-facing representation).

nickray · 2021-03-12T13:47:54Z

src/wireguard.rs

+
+impl core::fmt::Display for KeyResponse {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "{:x?} w/ UID: {} pubkey : {:x?}", self.key_info.id, self.key_info.label, self.key_info.pubkey)


Again, seems more Debug than Display. In any case, might be useful to look at https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.debug_struct if you're not aware.

nickray · 2021-03-12T13:49:22Z

src/wireguard.rs

+
+    pub fn unlock(&mut self, parameters: &Unlock) -> Result<()> {
+
+        print!("Unlock called: {:?}", parameters);


I'd suggest using our delog library for this. Possibly needs out-of-band discussion to explain how to use. The point is, it will let you have debug!, info! etc. calls in the code that get completely compiled out, unless explicitly opted in from the runner.

nickray · 2021-03-12T13:49:48Z

src/wireguard.rs

+            return KeyResponse
+        */
+
+        Ok(KeyResponse{ key_info : KeyInfo{ pubkey : [0;32], id : 0, label : String::from("A key label!")}})


There should definitely be some kind of check on the pubkey here.

nickray

Some feedback.

movcmpret added 9 commits February 8, 2021 13:22

wg init

590673f

wireguard scaffold

2f72118

merge main

bcf9484

some updates

2c84a11

wg init

3fc6962

wireguard scaffold

fdb8519

some updates

fd4b236

Merge branch 'wireguard' of github.com:movcmpret/trussed-totp-pc-tuto…

39c8a75

…rial into wireguard

pseudo code unlock

50a1b12

nickray commented Mar 12, 2021

View reviewed changes

nickray changed the title ~~Wireguard WIP~~ Wireguard Review 2021-03-12 Mar 12, 2021

movcmpret added 2 commits April 12, 2021 15:02

key storage

7044b11

removed warnings

c58fa9f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wireguard Review 2021-03-12 #1

Wireguard Review 2021-03-12 #1

nickray commented Mar 12, 2021

nickray Mar 12, 2021 •

edited

Loading

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray Mar 12, 2021

nickray left a comment


		no permissions / unknown user :

		Authenticate params: PIN/pass - Unlocks the device to use it.


		pub fn unlock(&mut self, parameters: &Unlock) -> Result<()> {

		print!("Unlock called: {:?}", parameters);

Wireguard Review 2021-03-12 #1

Are you sure you want to change the base?

Wireguard Review 2021-03-12 #1

Conversation

nickray commented Mar 12, 2021

nickray Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickray left a comment

Choose a reason for hiding this comment

nickray Mar 12, 2021 •

edited

Loading