Skip to content

Commit bc07eb0

Browse files
inicklesinickles
authored
Add protections for password handling in Nexus (#8093)
This wraps uses of user passwords in `secrecy` types that: 1. make an effort to not accidentally expose passwords (via Debug, serialization, etc.) 2. zeroize memory when a password is no longer used. The Nexus external API parameter type for passwords kept a local String copy of user passwords in addition to the internal password type, the former of which was used only for testing purposes. This removes that, ensuring that passwords in Nexus are always wrapped in `secrecy`, but it has the unfortunate requirement now that test code needs to use slightly different parameter types than those exposed by the external Nexus API. I've tried to make it clear where that happens in code and why in the code comments, but happy to take suggestions on different approaches.
1 parent 6a56f00 commit bc07eb0

File tree

16 files changed

+162
-117
lines changed

16 files changed

+162
-117
lines changed

‎Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎nexus/test-utils/src/resource_helpers.rs

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -394,19 +394,53 @@ pub async fn create_silo(
394394
.await
395395
}
396396

397+
/// This contains slight variations of Nexus external API types that are to be
398+
/// used for testing purposes only. These new types are needed because we want
399+
/// special protections for how passwords are handled in the Nexus server,
400+
/// including protections against user passwords accidentally becoming
401+
/// serialized.
402+
pub mod test_params {
403+
/// Testing only version of the the create-time parameters for a `User`
404+
#[derive(Clone, serde::Serialize)]
405+
pub struct UserCreate {
406+
/// username used to log in
407+
pub external_id: super::UserId,
408+
/// how to set the user's login password
409+
pub password: UserPassword,
410+
}
411+
412+
/// Testing only version of parameters for setting a user's password
413+
#[derive(Clone, serde::Serialize)]
414+
#[serde(rename_all = "snake_case")]
415+
#[serde(tag = "mode", content = "value")]
416+
pub enum UserPassword {
417+
/// Sets the user's password to the provided value
418+
Password(String),
419+
/// Invalidates any current password (disabling password authentication)
420+
LoginDisallowed,
421+
}
422+
423+
/// Testing only credentials for local user login
424+
#[derive(Clone, serde::Serialize)]
425+
pub struct UsernamePasswordCredentials {
426+
pub username: super::UserId,
427+
pub password: String,
428+
}
429+
}
430+
397431
pub async fn create_local_user(
398432
client: &ClientTestContext,
399433
silo: &views::Silo,
400434
username: &UserId,
401-
password: params::UserPassword,
435+
password: test_params::UserPassword,
402436
) -> User {
403437
let silo_name = &silo.identity.name;
404438
let url =
405439
format!("/v1/system/identity-providers/local/users?silo={}", silo_name);
406440
object_create(
407441
client,
408442
&url,
409-
&params::UserCreate { external_id: username.to_owned(), password },
443+
&test_params::UserCreate { external_id: username.to_owned(), password },
410444
)
411445
.await
412446
}

‎nexus/tests/integration_tests/authz.rs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
//! Tests for authz policy not covered in the set of unauthorized tests
66
77
use nexus_test_utils::http_testing::{AuthnMode, NexusRequest, RequestBuilder};
8+
use nexus_test_utils::resource_helpers::test_params;
89
use nexus_test_utils_macros::nexus_test;
910

1011
use nexus_types::external_api::params;
@@ -38,15 +39,15 @@ async fn test_cannot_read_others_ssh_keys(cptestctx: &ControlPlaneTestContext) {
3839
client,
3940
&silo,
4041
&"user1".parse().unwrap(),
41-
params::UserPassword::LoginDisallowed,
42+
test_params::UserPassword::LoginDisallowed,
4243
)
4344
.await
4445
.id;
4546
let user2 = create_local_user(
4647
client,
4748
&silo,
4849
&"user2".parse().unwrap(),
49-
params::UserPassword::LoginDisallowed,
50+
test_params::UserPassword::LoginDisallowed,
5051
)
5152
.await
5253
.id;
@@ -144,7 +145,7 @@ async fn test_list_silo_users_for_unpriv(cptestctx: &ControlPlaneTestContext) {
144145
client,
145146
&silo,
146147
&"unpriv".parse().unwrap(),
147-
params::UserPassword::LoginDisallowed,
148+
test_params::UserPassword::LoginDisallowed,
148149
)
149150
.await
150151
.id;
@@ -162,7 +163,7 @@ async fn test_list_silo_users_for_unpriv(cptestctx: &ControlPlaneTestContext) {
162163
client,
163164
&silo,
164165
&"otheruser".parse().unwrap(),
165-
params::UserPassword::LoginDisallowed,
166+
test_params::UserPassword::LoginDisallowed,
166167
)
167168
.await;
168169

@@ -200,7 +201,7 @@ async fn test_list_silo_idps_for_unpriv(cptestctx: &ControlPlaneTestContext) {
200201
client,
201202
&silo,
202203
&"unpriv".parse().unwrap(),
203-
params::UserPassword::LoginDisallowed,
204+
test_params::UserPassword::LoginDisallowed,
204205
)
205206
.await
206207
.id;
@@ -236,7 +237,7 @@ async fn test_session_me_for_unpriv(cptestctx: &ControlPlaneTestContext) {
236237
client,
237238
&silo,
238239
&"unpriv".parse().unwrap(),
239-
params::UserPassword::LoginDisallowed,
240+
test_params::UserPassword::LoginDisallowed,
240241
)
241242
.await
242243
.id;
@@ -266,7 +267,7 @@ async fn test_silo_read_for_unpriv(cptestctx: &ControlPlaneTestContext) {
266267
client,
267268
&silo,
268269
&"unpriv".parse().unwrap(),
269-
params::UserPassword::LoginDisallowed,
270+
test_params::UserPassword::LoginDisallowed,
270271
)
271272
.await
272273
.id;

‎nexus/tests/integration_tests/console_api.rs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,15 @@ use nexus_db_queries::db::identity::{Asset, Resource};
1919
use nexus_test_utils::http_testing::{
2020
AuthnMode, NexusRequest, RequestBuilder, TestResponse,
2121
};
22+
use nexus_test_utils::resource_helpers::test_params;
2223
use nexus_test_utils::resource_helpers::{
2324
create_silo, grant_iam, object_create,
2425
};
2526
use nexus_test_utils::{
2627
TEST_SUITE_PASSWORD, load_test_config, test_setup_with_config,
2728
};
2829
use nexus_test_utils_macros::nexus_test;
29-
use nexus_types::external_api::params::{
30-
self, ProjectCreate, UsernamePasswordCredentials,
31-
};
30+
use nexus_types::external_api::params::{self, ProjectCreate};
3231
use nexus_types::external_api::shared::{SiloIdentityMode, SiloRole};
3332
use nexus_types::external_api::{shared, views};
3433
use omicron_common::api::external::IdentityMetadataCreateParams;
@@ -878,9 +877,9 @@ async fn log_in_and_extract_token(
878877
) -> String {
879878
let testctx = &cptestctx.external_client;
880879
let url = format!("/v1/login/{}/local", cptestctx.silo_name);
881-
let credentials = UsernamePasswordCredentials {
880+
let credentials = test_params::UsernamePasswordCredentials {
882881
username: cptestctx.user_name.as_ref().parse().unwrap(),
883-
password: TEST_SUITE_PASSWORD.parse().unwrap(),
882+
password: TEST_SUITE_PASSWORD.to_string(),
884883
};
885884
let login = RequestBuilder::new(&testctx, Method::POST, &url)
886885
.body(Some(&credentials))

‎nexus/tests/integration_tests/endpoints.rs

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ use nexus_test_utils::PHYSICAL_DISK_UUID;
1818
use nexus_test_utils::RACK_UUID;
1919
use nexus_test_utils::SLED_AGENT_UUID;
2020
use nexus_test_utils::SWITCH_UUID;
21+
use nexus_test_utils::resource_helpers::test_params;
2122
use nexus_types::external_api::params;
2223
use nexus_types::external_api::shared;
2324
use nexus_types::external_api::shared::IpRange;
@@ -1140,10 +1141,10 @@ pub static DEMO_TIMESERIES_QUERY: LazyLock<params::TimeseriesQuery> =
11401141
});
11411142

11421143
// Users
1143-
pub static DEMO_USER_CREATE: LazyLock<params::UserCreate> =
1144-
LazyLock::new(|| params::UserCreate {
1144+
pub static DEMO_USER_CREATE: LazyLock<test_params::UserCreate> =
1145+
LazyLock::new(|| test_params::UserCreate {
11451146
external_id: UserId::from_str("dummy-user").unwrap(),
1146-
password: params::UserPassword::LoginDisallowed,
1147+
password: test_params::UserPassword::LoginDisallowed,
11471148
});
11481149

11491150
// Allowlist for user-facing services.
@@ -1689,8 +1690,10 @@ pub static VERIFY_ENDPOINTS: LazyLock<Vec<VerifyEndpoint>> =
16891690
visibility: Visibility::Public,
16901691
unprivileged_access: UnprivilegedAccess::ReadOnly,
16911692
allowed_methods: vec![AllowedMethod::Post(
1692-
serde_json::to_value(params::UserPassword::LoginDisallowed)
1693-
.unwrap(),
1693+
serde_json::to_value(
1694+
test_params::UserPassword::LoginDisallowed,
1695+
)
1696+
.unwrap(),
16941697
)],
16951698
},
16961699
/* Projects */

‎nexus/tests/integration_tests/external_ips.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ use nexus_test_utils::resource_helpers::object_delete;
3434
use nexus_test_utils::resource_helpers::object_delete_error;
3535
use nexus_test_utils::resource_helpers::object_get;
3636
use nexus_test_utils::resource_helpers::object_put;
37+
use nexus_test_utils::resource_helpers::test_params;
3738
use nexus_test_utils_macros::nexus_test;
3839
use nexus_types::external_api::params;
3940
use nexus_types::external_api::shared;
@@ -297,7 +298,7 @@ async fn test_floating_ip_create_non_admin(
297298
client,
298299
&silo,
299300
&"user".parse().unwrap(),
300-
params::UserPassword::LoginDisallowed,
301+
test_params::UserPassword::LoginDisallowed,
301302
)
302303
.await;
303304

‎nexus/tests/integration_tests/instances.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ use nexus_test_utils::resource_helpers::object_get;
3737
use nexus_test_utils::resource_helpers::object_put;
3838
use nexus_test_utils::resource_helpers::object_put_error;
3939
use nexus_test_utils::resource_helpers::objects_list_page_authz;
40+
use nexus_test_utils::resource_helpers::test_params;
4041
use nexus_test_utils::wait_for_producer;
4142
use nexus_types::external_api::params::SshKeyCreate;
4243
use nexus_types::external_api::shared::IpKind;
@@ -6593,7 +6594,7 @@ async fn test_instance_create_in_silo(cptestctx: &ControlPlaneTestContext) {
65936594
client,
65946595
&silo,
65956596
&"unpriv".parse().unwrap(),
6596-
params::UserPassword::LoginDisallowed,
6597+
test_params::UserPassword::LoginDisallowed,
65976598
)
65986599
.await
65996600
.id;

‎nexus/tests/integration_tests/internet_gateway.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ use nexus_test_utils::{
1313
create_local_user, create_project, create_route, create_router,
1414
create_vpc, delete_internet_gateway, detach_ip_address_from_igw,
1515
detach_ip_pool_from_igw, link_ip_pool, objects_list_page_authz,
16+
test_params,
1617
},
1718
};
1819
use nexus_test_utils_macros::nexus_test;
@@ -271,7 +272,7 @@ async fn test_igw_ip_pool_attach_silo_user(ctx: &ControlPlaneTestContext) {
271272
c,
272273
&silo,
273274
&"user".parse().unwrap(),
274-
params::UserPassword::LoginDisallowed,
275+
test_params::UserPassword::LoginDisallowed,
275276
)
276277
.await;
277278

‎nexus/tests/integration_tests/ip_pools.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ use nexus_test_utils::resource_helpers::object_get_error;
3636
use nexus_test_utils::resource_helpers::object_put;
3737
use nexus_test_utils::resource_helpers::object_put_error;
3838
use nexus_test_utils::resource_helpers::objects_list_page_authz;
39+
use nexus_test_utils::resource_helpers::test_params;
3940
use nexus_test_utils_macros::nexus_test;
4041
use nexus_types::external_api::params;
4142
use nexus_types::external_api::params::IpPoolCreate;
@@ -1183,7 +1184,7 @@ async fn test_ip_pool_list_in_silo(cptestctx: &ControlPlaneTestContext) {
11831184
client,
11841185
&silo,
11851186
&"user".parse().unwrap(),
1186-
params::UserPassword::LoginDisallowed,
1187+
test_params::UserPassword::LoginDisallowed,
11871188
)
11881189
.await;
11891190

0 commit comments

Comments
 (0)