Skip to content

Manually exercise audit logged endpoints not in VERIFY_ENDPOINTS#10256

Merged
david-crespo merged 4 commits intomainfrom
audit-test-refactor
Apr 14, 2026
Merged

Manually exercise audit logged endpoints not in VERIFY_ENDPOINTS#10256
david-crespo merged 4 commits intomainfrom
audit-test-refactor

Conversation

@david-crespo
Copy link
Copy Markdown
Contributor

@david-crespo david-crespo commented Apr 9, 2026

  • Manually exercise all endpoints in the audit log coverage test that were previously left out
    • We weren't missing any audit logging, we were only missing the test checking that we have the audit logging
  • Convert text file snapshotting the list of un-audited endpoints to a Rust array of tuples to we can put comments in there explaining why things are in the list. In 9c42800 I built in the ability to put comments right in the text file, but I realized there's no reason it needs to be a text file in the first place. The only advantage was that it's nice to be able to pull up that file by name, but you can find the list in the audit log integration test file anyway.

Sensitivity checks

Had the robot manually trigger various failure scenarios to make sure the tests catch them and the error messages give clear instructions about what to do.

  1. Endpoint missing audit logging: Removed audit_and_time from project_create. Test flags it with instructions to add audit logging or add it to the allowlist.
  2. Untested endpoint not in allowlist: Commented out the check_manual call for logout and removed it from allowed_unaudited. Caught via untested_mutating. (This is the bug the last commit fixes.)
  3. Stale allowlist entry: Added bogus_endpoint to allowed_unaudited. Test panics with "Stale allowed_unaudited entry ... remove it from the list."
  4. Allowlist entry with wrong path: Changed logout path to /v1/logoutt. Test fails with "allowed_unaudited entry for logout has wrong method/path."

Background

@inickles asked me about login_local and login_saml why they're listed as uncovered even though they have audit logging.

Mutating endpoints without audit logging:
disk_bulk_write_import (post "/v1/disks/{disk}/bulk-write")
system_timeseries_query (post "/v1/system/timeseries/query")
timeseries_query (post "/v1/timeseries/query")
Mutating endpoints not tested (not in VERIFY_ENDPOINTS):
device_access_token (post "/device/token")
device_auth_confirm (post "/device/confirm")
device_auth_request (post "/device/auth")
login_local (post "/v1/login/{silo_name}/local")
login_saml (post "/login/{silo_name}/saml/{provider_name}")
logout (post "/v1/logout")

The reason is when we test audit log coverage, we piggyback on top of the list of endpoints that is used for verifying authorization in order to have a nice list of valid request bodies for each endpoint. login_local and login_saml aren't in there because they are technically unauthenticated in the usual sense — the contents of the request body are what authenticates the endpoint.

/// List of endpoints to be verified
pub static VERIFY_ENDPOINTS: LazyLock<Vec<VerifyEndpoint>> = LazyLock::new(
|| {
vec![
// Global IAM policy
VerifyEndpoint {
url: &SYSTEM_POLICY_URL,
visibility: Visibility::Public,
unprivileged_access: UnprivilegedAccess::None,
allowed_methods: vec![
AllowedMethod::Get,
AllowedMethod::Put(
serde_json::to_value(&policy::Policy::<
policy::FleetRole,
> {
role_assignments: vec![],
})
.unwrap(),
),
],
},
// IP Pools top-level endpoint
VerifyEndpoint {
url: &DEMO_IP_POOLS_URL,
visibility: Visibility::Public,
unprivileged_access: UnprivilegedAccess::None,
allowed_methods: vec![
AllowedMethod::Get,
AllowedMethod::Post(
serde_json::to_value(&*DEMO_IP_POOL_CREATE).unwrap(),
),
],
},

But it's a short list of endpoints that we're failing to exercise, so we can call them by hand in the test.

@david-crespo david-crespo requested a review from inickles April 9, 2026 21:46
Move the uncovered-audit-log-endpoints.txt and audited-get-endpoints.txt
snapshot files into hardcoded BTreeMaps in the test code. Comments
explaining each exception are now regular Rust comments. Add staleness
detection for allowlist entries that no longer apply.
@david-crespo david-crespo force-pushed the audit-test-refactor branch from d59444b to db2061c Compare April 9, 2026 22:02
client_id: uuid::Uuid::nil(),
})),
)
.await;
Copy link
Copy Markdown
Contributor Author

@david-crespo david-crespo Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a look at why each of the above is not in VERIFY_ENDPOINTS — a couple of of them could actually move there, but mostly they're not in there because it's noisy to change the test to accommodate these weird request bodies.

  • login_local: JSON body, could move
  • login_saml: UntypedBody, not JSON
  • logout: no body
  • device_auth_request: URL-encoded body
  • device_auth_confirm: JSON body, could move
  • device_access_token: URL-encoded body

@david-crespo david-crespo merged commit b5a9aab into main Apr 14, 2026
16 checks passed
@david-crespo david-crespo deleted the audit-test-refactor branch April 14, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant