limit the length of the request URI we write down

Author: david-crespo

nexus/src/app/audit_log.rs

@@ -20,12 +20,12 @@ use crate::context::ApiContext;
 
 /// Truncate a str to at most `max` bytes, but make sure not to cut any chars
 /// in half.
-fn safe_truncate(s: &str, max: usize) -> &str {
+fn safe_truncate(s: &str, max: usize) -> String {
     let mut end = s.len().min(max);
     while !s.is_char_boundary(end) {
         end -= 1; // back up until we hit a boundary
     }
-    &s[..end]
+    s[..end].to_string()
 }
 
 impl super::Nexus {
@@ -84,6 +84,21 @@ impl super::Nexus {
         self.audit_log_entry_init_inner(&opctx, actor, rqctx).await
     }
 
+    // A note on the handling of request URI: request.request.uri() is a
+    // http::Uri, which contains the scheme and host only if they are in the
+    // HTTP request line itself, i.e., only for HTTP/2 requests. So for HTTP/1.1
+    // requests, all we'll have is a path. We are truncating it because it can
+    // be arbitrarily long in theory, and we don't want to let people jam very
+    // long strings into the DB.
+    //
+    // We could use the authority_for_request helper defined elsewhere to pull
+    // the authority out of either the URI or the host header as appropriate
+    // and log that in a dedicated column. In that case I think we would want
+    // to log uri().path_and_query() instead of the full URI -- the only problem
+    // is that path_and_query() returns an option, so we'd need to decide what
+    // to fall back to, though in practice I don't think it's possible for it to
+    // come back as `None` because every operation we audit log has a path.
+
     async fn audit_log_entry_init_inner(
         &self,
         opctx: &OpContext,
@@ -97,12 +112,12 @@ impl super::Nexus {
             .headers()
             .get(http::header::USER_AGENT)
             .and_then(|value| value.to_str().ok())
-            .map(|s| safe_truncate(s, 255).to_string());
+            .map(|s| safe_truncate(s, 256));
 
         let entry_params = AuditLogEntryInitParams {
             request_id: rqctx.request_id.clone(),
             operation_id: rqctx.endpoint.operation_id.clone(),
-            request_uri: rqctx.request.uri().to_string(),
+            request_uri: safe_truncate(&rqctx.request.uri().to_string(), 512),
             source_ip: rqctx.request.remote_addr().ip(),
             user_agent,
             actor,

nexus/tests/integration_tests/audit_log.rs

@@ -73,10 +73,14 @@ async fn test_audit_log_list(ctx: &ControlPlaneTestContext) {
             description: "a pier".to_string(),
         },
     };
-    RequestBuilder::new(client, Method::POST, "/v1/projects")
+    let long_user_agent = "A".repeat(300);
+    let long_query_value = "B".repeat(600);
+    let long_uri =
+        format!("/v1/projects?very_long_parameter={}", long_query_value);
+    RequestBuilder::new(client, Method::POST, &long_uri)
         .body(Some(&body))
         .header(header::COOKIE, session_cookie.clone())
-        .header("User-Agent", "A pretend user agent string")
+        .header("User-Agent", &long_user_agent)
         .expect_status(Some(StatusCode::CREATED))
         .execute()
         .await
@@ -132,10 +136,13 @@ async fn test_audit_log_list(ctx: &ControlPlaneTestContext) {
         .unwrap();
 
     // third one was done with the session cookie, reflected in auth_method
-    assert_eq!(e3.request_uri, "/v1/projects");
+    assert_eq!(e3.request_uri.len(), 512);
+    assert!(
+        e3.request_uri.starts_with("/v1/projects?very_long_parameter=BBBBB")
+    );
     assert_eq!(e3.operation_id, "project_create");
     assert_eq!(e3.source_ip.to_string(), "127.0.0.1");
-    assert_eq!(e3.user_agent.as_ref().unwrap(), "A pretend user agent string");
+    assert_eq!(e3.user_agent.clone().unwrap(), "A".repeat(256));
     assert_eq!(e3.auth_method, Some("session_cookie".to_string()));
     assert!(e3.time_started >= t3 && e3.time_started <= t4);
     assert!(e3.time_completed > e3.time_started);
@@ -179,7 +186,7 @@ async fn test_audit_log_list(ctx: &ControlPlaneTestContext) {
     // when we use the next page cursor we should get e2
     let url = format!(
         "/v1/system/audit-log?page_token={}&limit=1",
-        log.next_page.unwrap()
+        log.next_page.clone().unwrap()
     );
     let log =
         objects_list_page_authz::<views::AuditLogEntry>(client, &url).await;

nexus/types/src/external_api/views.rs

@@ -1623,7 +1623,9 @@ pub struct AuditLogEntry {
 
     /// Request ID for tracing requests through the system
     pub request_id: String,
-    /// Full URL of the request
+    /// URI of the request, truncated at 500 characters. Will only include
+    /// host and scheme for HTTP/2 requests, otherwise will be a path and query
+    /// params.
     pub request_uri: String,
     /// API endpoint ID, e.g., `project_create`
     pub operation_id: String,

schema/crdb/audit-log/up03.sql

@@ -4,7 +4,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.audit_log (
     -- request IDs are UUIDs but let's give them a little extra space
     -- https://github.com/oxidecomputer/dropshot/blob/83f78e7/dropshot/src/server.rs#L743
     request_id STRING(63) NOT NULL,
-    request_uri STRING NOT NULL,
+    request_uri STRING(512) NOT NULL,
     operation_id STRING(512) NOT NULL,
     source_ip INET NOT NULL,
     -- Pulled from request header if present and truncated
@@ -17,7 +17,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.audit_log (
     -- actor kind indicating builtin user, silo user, or unauthenticated
     actor_kind omicron.public.audit_log_actor_kind NOT NULL,
     -- The name of the authn scheme used
-    auth_method STRING,
+    auth_method STRING(63),
 
     -- below are fields we can only fill in after the operation
 

schema/crdb/dbinit.sql

@@ -5788,7 +5788,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.audit_log (
     -- request IDs are UUIDs but let's give them a little extra space
     -- https://github.com/oxidecomputer/dropshot/blob/83f78e7/dropshot/src/server.rs#L743
     request_id STRING(63) NOT NULL,
-    request_uri STRING NOT NULL,
+    request_uri STRING(512) NOT NULL,
     operation_id STRING(512) NOT NULL,
     source_ip INET NOT NULL,
     -- Pulled from request header if present and truncated
@@ -5801,7 +5801,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.audit_log (
     -- actor kind indicating builtin user, silo user, or unauthenticated
     actor_kind omicron.public.audit_log_actor_kind NOT NULL,
     -- The name of the authn scheme used
-    auth_method STRING,
+    auth_method STRING(63),
 
     -- below are fields we can only fill in after the operation