Reverse OTAA dev-nonce and mic validations.

The dev-nonce validation updates the used dev-nonces in the database.
Therefore, we should first check the MIC. This also removes the
ctx.save_device_keys step, and puts the join-nonce increment in the
device_keys::validate_incr_... function, reducing the SQL queries by one.

chirpstack/src/storage/device_keys.rs

@@ -158,7 +158,7 @@ pub async fn reset_nonces(dev_eui: &EUI64) -> Result<DeviceKeys, Error> {
     Ok(dk)
 }
 
-pub async fn validate_and_store_dev_nonce(
+pub async fn validate_incr_join_and_store_dev_nonce(
     dev_eui: &EUI64,
     dev_nonce: i32,
 ) -> Result<DeviceKeys, Error> {
@@ -178,11 +178,13 @@ pub async fn validate_and_store_dev_nonce(
                 }
 
                 dk.dev_nonces.push(Some(dev_nonce));
+                dk.join_nonce += 1;
 
                 diesel::update(device_keys::dsl::device_keys.find(&dev_eui))
                     .set((
                         device_keys::updated_at.eq(Utc::now()),
                         device_keys::dev_nonces.eq(&dk.dev_nonces),
+                        device_keys::join_nonce.eq(&dk.join_nonce),
                     ))
                     .get_result(c)
                     .map_err(|e| Error::from_diesel(e, dev_eui.to_string()))
@@ -191,7 +193,7 @@ pub async fn validate_and_store_dev_nonce(
     })
     .await??;
 
-    info!(dev_eui = %dev_eui, dev_nonce = dev_nonce, "Device-nonce validated and stored");
+    info!(dev_eui = %dev_eui, dev_nonce = dev_nonce, "Device-nonce validated, join-nonce incremented and stored");
     Ok(dk)
 }
 

chirpstack/src/uplink/join.rs

@@ -129,10 +129,9 @@ impl JoinRequest {
             ctx.get_join_accept_from_js().await?;
         } else {
             // Using internal keys
-            ctx.validate_dev_nonce_and_get_device_keys().await?;
             ctx.validate_mic().await?;
+            ctx.validate_dev_nonce_and_get_device_keys().await?;
             ctx.construct_join_accept_and_set_keys()?;
-            ctx.save_device_keys().await?;
         }
         ctx.log_uplink_meta().await?;
         ctx.create_device_session().await?;
@@ -185,10 +184,9 @@ impl JoinRequest {
             ctx.get_join_accept_from_js().await?;
         } else {
             // Using internal keys
-            ctx.validate_dev_nonce_and_get_device_keys().await?;
             ctx.validate_mic().await?;
+            ctx.validate_dev_nonce_and_get_device_keys().await?;
             ctx.construct_join_accept_and_set_keys()?;
-            ctx.save_device_keys().await?;
         }
         ctx.create_device_session().await?;
         ctx.flush_device_queue().await?;
@@ -392,66 +390,9 @@ impl JoinRequest {
         Ok(())
     }
 
-    async fn validate_dev_nonce_and_get_device_keys(&mut self) -> Result<()> {
-        trace!("Validate dev-nonce and get device-keys");
-        let dev = self.device.as_ref().unwrap();
-        let app = self.application.as_ref().unwrap();
-        let join_request = self.join_request.as_ref().unwrap();
-
-        self.device_keys = Some(
-            match device_keys::validate_and_store_dev_nonce(
-                &dev.dev_eui,
-                join_request.dev_nonce as i32,
-            )
-            .await
-            {
-                Ok(v) => v,
-                Err(v) => match v {
-                    StorageError::InvalidDevNonce => {
-                        integration::log_event(
-                            app.id,
-                            &dev.variables,
-                            &integration_pb::LogEvent {
-                                time: Some(Utc::now().into()),
-                                device_info: self.device_info.clone(),
-                                level: integration_pb::LogLevel::Error.into(),
-                                code: integration_pb::LogCode::Otaa.into(),
-                                description: "DevNonce has already been used".into(),
-                                context: [(
-                                    "deduplication_id".to_string(),
-                                    self.uplink_frame_set.uplink_set_id.to_string(),
-                                )]
-                                .iter()
-                                .cloned()
-                                .collect(),
-                            },
-                        )
-                        .await;
-
-                        metrics::save(
-                            &format!("device:{}", dev.dev_eui),
-                            &metrics::Record {
-                                time: Local::now(),
-                                kind: metrics::Kind::ABSOLUTE,
-                                metrics: [("error_OTAA".into(), 1f64)].iter().cloned().collect(),
-                            },
-                        )
-                        .await?;
-
-                        return Err(v.into());
-                    }
-                    _ => {
-                        return Err(v.into());
-                    }
-                },
-            },
-        );
-
-        Ok(())
-    }
-
     async fn validate_mic(&self) -> Result<()> {
-        let device_keys = self.device_keys.as_ref().unwrap();
+        let join_request = self.join_request.as_ref().unwrap();
+        let device_keys = device_keys::get(&join_request.dev_eui).await?;
 
         if let Some(relay_ctx) = self.relay_context.as_ref() {
             if relay_ctx
@@ -508,6 +449,64 @@ impl JoinRequest {
         Err(anyhow!("Invalid MIC"))
     }
 
+    async fn validate_dev_nonce_and_get_device_keys(&mut self) -> Result<()> {
+        trace!("Validate dev-nonce and get device-keys");
+        let dev = self.device.as_ref().unwrap();
+        let app = self.application.as_ref().unwrap();
+        let join_request = self.join_request.as_ref().unwrap();
+
+        self.device_keys = Some(
+            match device_keys::validate_incr_join_and_store_dev_nonce(
+                &dev.dev_eui,
+                join_request.dev_nonce as i32,
+            )
+            .await
+            {
+                Ok(v) => v,
+                Err(v) => match v {
+                    StorageError::InvalidDevNonce => {
+                        integration::log_event(
+                            app.id,
+                            &dev.variables,
+                            &integration_pb::LogEvent {
+                                time: Some(Utc::now().into()),
+                                device_info: self.device_info.clone(),
+                                level: integration_pb::LogLevel::Error.into(),
+                                code: integration_pb::LogCode::Otaa.into(),
+                                description: "DevNonce has already been used".into(),
+                                context: [(
+                                    "deduplication_id".to_string(),
+                                    self.uplink_frame_set.uplink_set_id.to_string(),
+                                )]
+                                .iter()
+                                .cloned()
+                                .collect(),
+                            },
+                        )
+                        .await;
+
+                        metrics::save(
+                            &format!("device:{}", dev.dev_eui),
+                            &metrics::Record {
+                                time: Local::now(),
+                                kind: metrics::Kind::ABSOLUTE,
+                                metrics: [("error_OTAA".into(), 1f64)].iter().cloned().collect(),
+                            },
+                        )
+                        .await?;
+
+                        return Err(v.into());
+                    }
+                    _ => {
+                        return Err(v.into());
+                    }
+                },
+            },
+        );
+
+        Ok(())
+    }
+
     fn get_random_dev_addr(&mut self) -> Result<()> {
         self.dev_addr = Some(get_random_dev_addr());
         Ok(())
@@ -610,7 +609,9 @@ impl JoinRequest {
         let join_request = self.join_request.as_ref().unwrap();
 
         let dk = self.device_keys.as_mut().unwrap();
-        if dk.join_nonce == (1 << 24) - 1 {
+
+        let join_nonce = dk.join_nonce - 1; // this was incremented on validation
+        if join_nonce == (1 << 24) - 1 {
             return Err(anyhow!("Join-nonce overflow"));
         }
 
@@ -629,7 +630,7 @@ impl JoinRequest {
                 major: Major::LoRaWANR1,
             },
             payload: Payload::JoinAccept(JoinAcceptPayload {
-                join_nonce: dk.join_nonce as u32,
+                join_nonce: join_nonce as u32,
                 home_netid: conf.network.net_id,
                 devaddr: self.dev_addr.unwrap(),
                 dl_settings: DLSettings {
@@ -671,7 +672,7 @@ impl JoinRequest {
             &device_keys.nwk_key,
             &conf.network.net_id,
             &join_request.join_eui,
-            device_keys.join_nonce as u32,
+            join_nonce as u32,
             join_request.dev_nonce,
         )?);
 
@@ -681,7 +682,7 @@ impl JoinRequest {
                 &device_keys.nwk_key,
                 &conf.network.net_id,
                 &join_request.join_eui,
-                device_keys.join_nonce as u32,
+                join_nonce as u32,
                 join_request.dev_nonce,
             )?,
             false => keys::get_f_nwk_s_int_key(
@@ -689,7 +690,7 @@ impl JoinRequest {
                 &device_keys.nwk_key,
                 &conf.network.net_id,
                 &join_request.join_eui,
-                device_keys.join_nonce as u32,
+                join_nonce as u32,
                 join_request.dev_nonce,
             )?,
         });
@@ -700,7 +701,7 @@ impl JoinRequest {
                 &device_keys.nwk_key,
                 &conf.network.net_id,
                 &join_request.join_eui,
-                device_keys.join_nonce as u32,
+                join_nonce as u32,
                 join_request.dev_nonce,
             )?,
             false => keys::get_f_nwk_s_int_key(
@@ -708,7 +709,7 @@ impl JoinRequest {
                 &device_keys.nwk_key,
                 &conf.network.net_id,
                 &join_request.join_eui,
-                device_keys.join_nonce as u32,
+                join_nonce as u32,
                 join_request.dev_nonce,
             )?,
         });
@@ -721,7 +722,7 @@ impl JoinRequest {
                     &device_keys.app_key,
                     &conf.network.net_id,
                     &join_request.join_eui,
-                    device_keys.join_nonce as u32,
+                    join_nonce as u32,
                     join_request.dev_nonce,
                 )?,
                 false => keys::get_app_s_key(
@@ -729,7 +730,7 @@ impl JoinRequest {
                     &device_keys.nwk_key,
                     &conf.network.net_id,
                     &join_request.join_eui,
-                    device_keys.join_nonce as u32,
+                    join_nonce as u32,
                     join_request.dev_nonce,
                 )?,
             }
@@ -872,15 +873,6 @@ impl JoinRequest {
         Ok(())
     }
 
-    async fn save_device_keys(&mut self) -> Result<()> {
-        trace!("Updating device-keys");
-        let mut dk = self.device_keys.as_mut().unwrap();
-        dk.join_nonce += 1;
-
-        *dk = device_keys::update(dk.clone()).await?;
-        Ok(())
-    }
-
     async fn start_downlink_join_accept_flow(&self) -> Result<()> {
         trace!("Starting downlink join-accept flow");
         downlink::join::JoinAccept::handle(

chirpstack/src/uplink/join_sns.rs

@@ -93,10 +93,9 @@ impl JoinRequest {
             ctx.get_join_accept_from_js().await?;
         } else {
             // Using internal keys
-            ctx.validate_dev_nonce_and_get_device_keys().await?;
             ctx.validate_mic().await?;
+            ctx.validate_dev_nonce_and_get_device_keys().await?;
             ctx.construct_join_accept_and_set_keys()?;
-            ctx.save_device_keys().await?;
         }
         ctx.log_uplink_meta().await?;
         ctx.create_device_session().await?;
@@ -289,6 +288,57 @@ impl JoinRequest {
         Ok(())
     }
 
+    async fn validate_mic(&self) -> Result<()> {
+        let join_request = self.join_request.as_ref().unwrap();
+        let device_keys = device_keys::get(&join_request.dev_eui).await?;
+
+        if self
+            .uplink_frame_set
+            .phy_payload
+            .validate_join_request_mic(&device_keys.nwk_key)?
+        {
+            return Ok(());
+        }
+
+        let app = self.application.as_ref().unwrap();
+        let dev = self.device.as_ref().unwrap();
+
+        integration::log_event(
+            app.id,
+            &dev.variables,
+            &integration_pb::LogEvent {
+                time: Some(Utc::now().into()),
+                device_info: self.device_info.clone(),
+                level: integration_pb::LogLevel::Error.into(),
+                code: integration_pb::LogCode::UplinkMic.into(),
+                description: "MIC of join-request is invalid, make sure keys are correct".into(),
+                context: [(
+                    "deduplication_id".to_string(),
+                    self.uplink_frame_set.uplink_set_id.to_string(),
+                )]
+                .iter()
+                .cloned()
+                .collect(),
+            },
+        )
+        .await;
+
+        metrics::save(
+            &format!("device:{}", dev.dev_eui),
+            &metrics::Record {
+                time: Local::now(),
+                kind: metrics::Kind::ABSOLUTE,
+                metrics: [("error_UPLINK_MIC".into(), 1f64)]
+                    .iter()
+                    .cloned()
+                    .collect(),
+            },
+        )
+        .await?;
+
+        Err(anyhow!("Invalid MIC"))
+    }
+
     async fn validate_dev_nonce_and_get_device_keys(&mut self) -> Result<()> {
         trace!("Validate dev-nonce and get device-keys");
         let dev = self.device.as_ref().unwrap();
@@ -296,7 +346,7 @@ impl JoinRequest {
         let join_request = self.join_request.as_ref().unwrap();
 
         self.device_keys = Some(
-            match device_keys::validate_and_store_dev_nonce(
+            match device_keys::validate_incr_join_and_store_dev_nonce(
                 &dev.dev_eui,
                 join_request.dev_nonce as i32,
             )
@@ -347,55 +397,6 @@ impl JoinRequest {
         Ok(())
     }
 
-    async fn validate_mic(&self) -> Result<()> {
-        let device_keys = self.device_keys.as_ref().unwrap();
-        if self
-            .uplink_frame_set
-            .phy_payload
-            .validate_join_request_mic(&device_keys.nwk_key)?
-        {
-            return Ok(());
-        }
-
-        let app = self.application.as_ref().unwrap();
-        let dev = self.device.as_ref().unwrap();
-
-        integration::log_event(
-            app.id,
-            &dev.variables,
-            &integration_pb::LogEvent {
-                time: Some(Utc::now().into()),
-                device_info: self.device_info.clone(),
-                level: integration_pb::LogLevel::Error.into(),
-                code: integration_pb::LogCode::UplinkMic.into(),
-                description: "MIC of join-request is invalid, make sure keys are correct".into(),
-                context: [(
-                    "deduplication_id".to_string(),
-                    self.uplink_frame_set.uplink_set_id.to_string(),
-                )]
-                .iter()
-                .cloned()
-                .collect(),
-            },
-        )
-        .await;
-
-        metrics::save(
-            &format!("device:{}", dev.dev_eui),
-            &metrics::Record {
-                time: Local::now(),
-                kind: metrics::Kind::ABSOLUTE,
-                metrics: [("error_UPLINK_MIC".into(), 1f64)]
-                    .iter()
-                    .cloned()
-                    .collect(),
-            },
-        )
-        .await?;
-
-        Err(anyhow!("Invalid MIC"))
-    }
-
     fn construct_join_accept_and_set_keys(&mut self) -> Result<()> {
         trace!("Constructing JoinAccept payload");
 
@@ -405,7 +406,9 @@ impl JoinRequest {
         let join_request = self.join_request.as_ref().unwrap();
 
         let dk = self.device_keys.as_mut().unwrap();
-        if dk.join_nonce == (1 << 24) - 1 {
+
+        let join_nonce = dk.join_nonce - 1; // this was incremented on validation
+        if join_nonce == (1 << 24) - 1 {
             return Err(anyhow!("Join-nonce overflow"));
         }
 
@@ -424,7 +427,7 @@ impl JoinRequest {
                 major: lrwn::Major::LoRaWANR1,
             },
             payload: lrwn::Payload::JoinAccept(lrwn::JoinAcceptPayload {
-                join_nonce: dk.join_nonce as u32,
+                join_nonce: join_nonce as u32,
                 home_netid: conf.network.net_id,
                 devaddr: self.dev_addr.unwrap(),
                 dl_settings: lrwn::DLSettings {
@@ -466,7 +469,7 @@ impl JoinRequest {
             &device_keys.nwk_key,
             &conf.network.net_id,
             &join_request.join_eui,
-            device_keys.join_nonce as u32,
+            join_nonce as u32,
             join_request.dev_nonce,
         )?);
 
@@ -476,7 +479,7 @@ impl JoinRequest {
                 &device_keys.nwk_key,
                 &conf.network.net_id,
                 &join_request.join_eui,
-                device_keys.join_nonce as u32,
+                join_nonce as u32,
                 join_request.dev_nonce,
             )?,
             false => keys::get_f_nwk_s_int_key(
@@ -484,7 +487,7 @@ impl JoinRequest {
                 &device_keys.nwk_key,
                 &conf.network.net_id,
                 &join_request.join_eui,
-                device_keys.join_nonce as u32,
+                join_nonce as u32,
                 join_request.dev_nonce,
             )?,
         });
@@ -495,7 +498,7 @@ impl JoinRequest {
                 &device_keys.nwk_key,
                 &conf.network.net_id,
                 &join_request.join_eui,
-                device_keys.join_nonce as u32,
+                join_nonce as u32,
                 join_request.dev_nonce,
             )?,
             false => keys::get_f_nwk_s_int_key(
@@ -503,7 +506,7 @@ impl JoinRequest {
                 &device_keys.nwk_key,
                 &conf.network.net_id,
                 &join_request.join_eui,
-                device_keys.join_nonce as u32,
+                join_nonce as u32,
                 join_request.dev_nonce,
             )?,
         });
@@ -516,7 +519,7 @@ impl JoinRequest {
                     &device_keys.app_key,
                     &conf.network.net_id,
                     &join_request.join_eui,
-                    device_keys.join_nonce as u32,
+                    join_nonce as u32,
                     join_request.dev_nonce,
                 )?,
                 false => keys::get_app_s_key(
@@ -524,7 +527,7 @@ impl JoinRequest {
                     &device_keys.nwk_key,
                     &conf.network.net_id,
                     &join_request.join_eui,
-                    device_keys.join_nonce as u32,
+                    join_nonce as u32,
                     join_request.dev_nonce,
                 )?,
             }
@@ -534,15 +537,6 @@ impl JoinRequest {
         Ok(())
     }
 
-    async fn save_device_keys(&mut self) -> Result<()> {
-        trace!("Updating device-keys");
-        let mut dk = self.device_keys.as_mut().unwrap();
-        dk.join_nonce += 1;
-
-        *dk = device_keys::update(dk.clone()).await?;
-        Ok(())
-    }
-
     async fn log_uplink_meta(&self) -> Result<()> {
         trace!("Logging uplink meta");