sync up various TODOs with issues

common/src/api/external/mod.rs

@@ -480,9 +480,6 @@ impl JsonSchema for RoleName {
 /// can fail (if the value is larger than i64::MAX).  We provide all of these for
 /// consumers' convenience.
 // TODO-cleanup This could benefit from a more complete implementation.
-// TODO-correctness RFD 4 requires that this be a multiple of 256 MiB.  We'll
-// need to write a validator for that.
-// /
 //
 // The maximum byte count of i64::MAX comes from the fact that this is stored in
 // the database as an i64.  Constraining it here ensures that we can't fail to

common/src/sql/dbinit.sql

@@ -1788,7 +1788,6 @@ CREATE TABLE omicron.public.device_auth_request (
 );
 
 -- Access tokens granted in response to successful device authorization flows.
--- TODO-security: expire tokens.
 CREATE TABLE omicron.public.device_access_token (
     token STRING(40) PRIMARY KEY,
     client_id UUID NOT NULL,

nexus/db-model/src/device_auth.rs

@@ -75,7 +75,6 @@ fn generate_token() -> String {
 /// suggested in RFC 8628 §6.1 (User Code Recommendations); q.v. also for
 /// a discussion of entropy requirements. On input, use codes should be
 /// uppercased, and characters not in this alphabet should be stripped.
-// TODO-security: user code tries should be rate-limited
 const USER_CODE_ALPHABET: [char; 20] = [
     'B', 'C', 'D', 'F', 'G', 'H', 'J', 'K', 'L', 'M', 'N', 'P', 'Q', 'R', 'S',
     'T', 'V', 'W', 'X', 'Z',

nexus/src/app/device_auth.rs

@@ -118,7 +118,6 @@ impl super::Nexus {
                 message: "device authorization request expired".to_string(),
             })
         } else {
-            // TODO-security: set an expiration time for the valid token.
             self.db_datastore
                 .device_access_token_create(
                     opctx,

nexus/src/app/mod.rs

@@ -63,7 +63,7 @@ pub(crate) const MAX_DISKS_PER_INSTANCE: u32 = 8;
 
 pub(crate) const MAX_NICS_PER_INSTANCE: usize = 8;
 
-// TODO-completness: Support multiple external IPs
+// TODO-completeness: Support multiple external IPs
 pub(crate) const MAX_EXTERNAL_IPS_PER_INSTANCE: usize = 1;
 
 pub(crate) struct ExternalServers {

nexus/src/app/sagas/instance_migrate.rs

@@ -194,7 +194,7 @@ async fn sim_instance_migrate(
     }
     let source_nat = SourceNatConfig::from(snat_ip.into_iter().next().unwrap());
 
-    // The TODOs below are tracked in
+    // The TODO items below are tracked in
     //   https://github.com/oxidecomputer/omicron/issues/1783
     let instance_hardware = InstanceHardware {
         runtime: runtime.into(),

nexus/src/app/silo.rs

@@ -517,7 +517,7 @@ impl super::Nexus {
         // silo_user_password_verify().
         // TODO-security There may still be some vulnerability to timing attack
         // here, in that we'll do one fewer database lookup if a user does not
-        // exist.
+        // exist.  Rate limiting might help.  See omicron#2184.
         let fetch_user = self
             .datastore()
             .silo_user_fetch_by_external_id(

nexus/src/bin/nexus.rs

@@ -5,13 +5,10 @@
 //! Executable program to run Nexus, the heart of the control plane
 
 // TODO
-// - TCP and HTTP KeepAlive parameters
 // - Server hostname
 // - Disable signals?
-// - Analogs for actix client_timeout (request timeout), client_shutdown (client
-//   shutdown timeout), server backlog, number of workers, max connections per
-//   worker, max connect-in-progress sockets, shutdown_timeout (server shutdown
-//   timeout)
+// - General networking and runtime tuning for availability and security: see
+//   omicron#2184, omicron#2414.
 
 use clap::Parser;
 use omicron_common::cmd::fatal;

nexus/src/db/datastore/role.rs

@@ -236,8 +236,6 @@ impl DataStore {
     where
         T: authz::ApiResourceWithRolesType + AuthorizedResource + Clone,
     {
-        // TODO-security We should carefully review what permissions are
-        // required for modifying the policy of a resource.
         opctx.authorize(authz::Action::ModifyPolicy, authz_resource).await?;
 
         let authz_resource = authz_resource.clone();

nexus/src/db/fixed_data/silo_user.rs

@@ -13,7 +13,7 @@ lazy_static! {
     /// local development
     // TODO-security Once we have a way to bootstrap the initial Silo with the
     // initial privileged user, this user should be created in the test suite,
-    // not automatically at Nexus startup.
+    // not automatically at Nexus startup.  See omicron#2305.
     pub static ref USER_TEST_PRIVILEGED: db::model::SiloUser =
         db::model::SiloUser::new(
             *db::fixed_data::silo::SILO_ID,
@@ -47,7 +47,7 @@ lazy_static! {
     /// Test user that's granted no privileges, used for automated testing
     // TODO-security Once we have a way to bootstrap the initial Silo with the
     // initial privileged user, this user should be created in the test suite,
-    // not automatically at Nexus startup.
+    // not automatically at Nexus startup.  See omicron#2305.
     pub static ref USER_TEST_UNPRIVILEGED: db::model::SiloUser =
         db::model::SiloUser::new(
             *db::fixed_data::silo::SILO_ID,

nexus/src/db/queries/external_ip.rs

@@ -1023,7 +1023,7 @@ mod tests {
         .unwrap();
         context.initialize_ip_pool("default", range).await;
 
-        // TODO-completess: Implementing Iterator for IpRange would be nice.
+        // TODO-completeness: Implementing Iterator for IpRange would be nice.
         let addresses = [
             Ipv4Addr::new(10, 0, 0, 1),
             Ipv4Addr::new(10, 0, 0, 2),

nexus/src/db/saga_recovery.rs

@@ -123,7 +123,7 @@ where
             // it has a larger log that requires more queries).  To avoid one
             // bad saga ruining the rest, we should try to recover the rest
             // before we go back to one that's failed.
-            // TODO-debug want visibility into "abandoned" sagas
+            // TODO-debugging want visibility into "abandoned" sagas
             let saga_id: steno::SagaId = saga.id.into();
             recover_saga(
                 &opctx,

nexus/src/db/sec_store.rs

@@ -61,7 +61,7 @@ impl steno::SecStore for CockroachDbSecStore {
         let our_event = db::saga_types::SagaNodeEvent::new(event, self.sec_id);
 
         // TODO-robustness This should be wrapped with a retry loop rather than
-        // unwrapping the result.
+        // unwrapping the result.  See omicron#2416.
         self.datastore.saga_create_event(&our_event).await.unwrap();
     }
 
@@ -75,7 +75,7 @@ impl steno::SecStore for CockroachDbSecStore {
         );
 
         // TODO-robustness This should be wrapped with a retry loop rather than
-        // unwrapping the result.
+        // unwrapping the result.  See omicron#2416.
         self.datastore
             .saga_update_state(id, update, self.sec_id, Generation::new())
             .await

nexus/src/external_api/console_api.rs

@@ -142,7 +142,7 @@ pub async fn login_spoof(
 // TODO If the user does not have this information it's unclear what should
 // happen.  If they know the silo name they are trying to log into, they could
 // `GET /system/silos/{silo_name}/identity_providers` in order to list available
-// identity providers. If not, TODO.
+// identity providers.
 //
 // Once the appropriate login URL is created, the user's browser is redirected:
 //

nexus/tests/integration_tests/instances.rs

@@ -1094,7 +1094,7 @@ async fn test_instance_with_new_custom_network_interfaces(
     .await
     .expect("Failed to create custom VPC Subnet");
 
-    // TODO-testing: We'd like to assert things about this VPC Subnet we just
+    // TODO-coverage: We'd like to assert things about this VPC Subnet we just
     // created, but the `vpc_subnets_post` endpoint in Nexus currently returns
     // the "private" `omicron_nexus::db::model::VpcSubnet` type. That should be
     // converted to return the public `omicron_common::external` type, which is
@@ -1192,7 +1192,7 @@ async fn test_instance_with_new_custom_network_interfaces(
     );
     let if1 = &interfaces1.all_items[0];
 
-    // TODO-testing: Add this test once the `VpcSubnet` type can be
+    // TODO-coverage: Add this test once the `VpcSubnet` type can be
     // deserialized.
     // assert_eq!(if1.subnet_id, non_default_vpc_subnet.id);
 

nexus/tests/integration_tests/silos.rs

@@ -115,7 +115,7 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) {
     )
     .await;
 
-    // TODO-coverage, TODO-security: Add test for Silo-local session
+    // TODO-coverage  TODO-security Add test for Silo-local session
     // when we can use users in another Silo.
 
     let authn_opctx = nexus.opctx_external_authn();

nexus/types/src/external_api/params.rs

@@ -409,7 +409,7 @@ impl TryFrom<String> for Password {
             .map_err(|e| format!("unsupported password: {:#}", e))?;
         // TODO-security If we want to apply password policy rules, this seems
         // like the place.  We presumably want to also document them in the
-        // OpenAPI schema below.
+        // OpenAPI schema below.  See omicron#2307.
         Ok(Password(value, inner))
     }
 }
@@ -437,7 +437,7 @@ impl JsonSchema for Password {
                     "A password used to authenticate a user".to_string(),
                 ),
                 // TODO-doc If we apply password strength rules, they should
-                // presumably be documented here.
+                // presumably be documented here.  See omicron#2307.
                 description: Some(
                     "Passwords may be subject to additional constraints."
                         .to_string(),
@@ -770,7 +770,7 @@ pub struct NetworkInterfaceUpdate {
     /// Note that this can only be used to select a new primary interface for an
     /// instance. Requests to change the primary interface into a secondary will
     /// return an error.
-    // TODO-completeness TODO-docs: When we get there, this should note that a
+    // TODO-completeness TODO-doc When we get there, this should note that a
     // change in the primary interface will result in changes to the DNS records
     // for the instance, though not the name.
     #[serde(default)]

nexus/types/src/external_api/views.rs

@@ -416,7 +416,6 @@ pub struct DeviceAuthResponse {
 }
 
 /// Successful access token grant. See RFC 6749 §5.1.
-/// TODO-security: `expires_in`, `refresh_token`, etc.
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
 pub struct DeviceAccessTokenGrant {
     /// The access token issued to the client.

sled-agent/src/opte/illumos/port_manager.rs

@@ -181,8 +181,8 @@ impl PortManager {
         external_ips: Option<Vec<IpAddr>>,
         firewall_rules: &[VpcFirewallRule],
     ) -> Result<(Port, PortTicket), Error> {
-        // TODO-completess: Remove IPv4 restrictions once OPTE supports virtual
-        // IPv6 networks.
+        // TODO-completeness: Remove IPv4 restrictions once OPTE supports
+        // virtual IPv6 networks.
         let private_ip = match nic.ip {
             IpAddr::V4(ip) => Ok(oxide_vpc::api::Ipv4Addr::from(ip)),
             IpAddr::V6(_) => Err(opte_ioctl::Error::InvalidArgument(

sled-agent/src/opte/non_illumos/port_manager.rs

@@ -114,8 +114,8 @@ impl PortManager {
         external_ips: Option<Vec<IpAddr>>,
         _firewall_rules: &[VpcFirewallRule],
     ) -> Result<(Port, PortTicket), Error> {
-        // TODO-completess: Remove IPv4 restrictions once OPTE supports virtual
-        // IPv6 networks.
+        // TODO-completeness: Remove IPv4 restrictions once OPTE supports
+        // virtual IPv6 networks.
         let _ = match nic.ip {
             IpAddr::V4(ip) => Ok(ip),
             IpAddr::V6(_) => Err(Error::InvalidArgument(String::from(

sled-agent/src/sim/collection.rs

@@ -243,7 +243,7 @@ impl<S: Simulatable + 'static> SimCollection<S> {
 
         // If the object came to rest destroyed, complete any async cleanup
         // needed now.
-        // TODO-debug It would be nice to have visibility into objects that
+        // TODO-debugging It would be nice to have visibility into objects that
         // are cleaning up in case we have to debug resource leaks here.
         // TODO-correctness Is it a problem that nobody waits on the background
         // task?  If we did it here, we'd deadlock, since we're invoked from the