diff --git a/end-to-end-tests/src/bin/bootstrap.rs b/end-to-end-tests/src/bin/bootstrap.rs index 97ad4384f7f..aa2c928346a 100644 --- a/end-to-end-tests/src/bin/bootstrap.rs +++ b/end-to-end-tests/src/bin/bootstrap.rs @@ -115,7 +115,7 @@ async fn main() -> Result<()> { deserialize_byte_stream( ctx.client .device_auth_request() - .body(DeviceAuthRequest { client_id }) + .body(DeviceAuthRequest { client_id, ttl_seconds: None }) .send() .await?, ) diff --git a/nexus/db-model/src/device_auth.rs b/nexus/db-model/src/device_auth.rs index 76c6b6ac37a..e681d8d01e6 100644 --- a/nexus/db-model/src/device_auth.rs +++ b/nexus/db-model/src/device_auth.rs @@ -13,8 +13,10 @@ use chrono::{DateTime, Duration, Utc}; use nexus_types::external_api::views; use omicron_uuid_kinds::{AccessTokenKind, GenericUuid, TypedUuid}; use rand::{Rng, RngCore, SeedableRng, distributions::Slice, rngs::StdRng}; +use std::num::NonZeroU32; use uuid::Uuid; +use crate::SqlU32; use crate::typed_uuid::DbTypedUuid; /// Default timeout in seconds for client to authenticate for a token request. @@ -32,6 +34,9 @@ pub struct DeviceAuthRequest { pub user_code: String, pub time_created: DateTime, pub time_expires: DateTime, + + /// TTL requested by the user + pub token_ttl_seconds: Option, } impl DeviceAuthRequest { @@ -98,7 +103,10 @@ fn generate_user_code() -> String { } impl DeviceAuthRequest { - pub fn new(client_id: Uuid) -> Self { + pub fn new( + client_id: Uuid, + requested_ttl_seconds: Option, + ) -> Self { let now = Utc::now(); Self { client_id, @@ -107,6 +115,8 @@ impl DeviceAuthRequest { time_created: now, time_expires: now + Duration::seconds(CLIENT_AUTHENTICATION_TIMEOUT), + token_ttl_seconds: requested_ttl_seconds + .map(|ttl| ttl.get().into()), } } diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 73d946bfeb1..689d0948c16 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: Version = Version::new(146, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(147, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock> = LazyLock::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(147, "device-auth-request-ttl"), KnownVersion::new(146, "silo-settings-token-expiration"), KnownVersion::new(145, "token-and-session-ids"), KnownVersion::new(144, "inventory-omicron-sled-config"), diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 8312f085850..42f0cf191c3 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -1360,6 +1360,7 @@ table! { device_code -> Text, time_created -> Timestamptz, time_expires -> Timestamptz, + token_ttl_seconds -> Nullable, } } diff --git a/nexus/src/app/device_auth.rs b/nexus/src/app/device_auth.rs index c7fe08f3f2f..668e363572c 100644 --- a/nexus/src/app/device_auth.rs +++ b/nexus/src/app/device_auth.rs @@ -53,7 +53,7 @@ use nexus_db_queries::context::OpContext; use nexus_db_queries::db::model::{DeviceAccessToken, DeviceAuthRequest}; use anyhow::anyhow; -use nexus_types::external_api::params::DeviceAccessTokenRequest; +use nexus_types::external_api::params; use nexus_types::external_api::views; use omicron_common::api::external::{ CreateResult, DataPageParams, Error, ListResultVec, @@ -77,13 +77,17 @@ impl super::Nexus { pub(crate) async fn device_auth_request_create( &self, opctx: &OpContext, - client_id: Uuid, + params: params::DeviceAuthRequest, ) -> CreateResult { // TODO-correctness: the `user_code` generated for a new request // is used as a primary key, but may potentially collide with an // existing outstanding request. So we should retry some (small) // number of times if inserting the new request fails. - let auth_request = DeviceAuthRequest::new(client_id); + + // Note that we cannot validate the TTL here against the silo max + // because we do not know what silo we're talking about until verify + let auth_request = + DeviceAuthRequest::new(params.client_id, params.ttl_seconds); self.db_datastore.device_auth_request_create(opctx, auth_request).await } @@ -115,17 +119,30 @@ impl super::Nexus { .silo_auth_settings_view(opctx, &authz_silo) .await?; - // Create an access token record. + let silo_max_ttl = silo_auth_settings.device_token_max_ttl_seconds; + let requested_ttl = db_request.token_ttl_seconds; + + // Validate the requested TTL against the silo's max TTL + if let (Some(requested), Some(max)) = (requested_ttl, silo_max_ttl) { + if requested > max.0.into() { + return Err(Error::invalid_request(&format!( + "Requested TTL {} seconds exceeds maximum \ + allowed TTL for this silo of {} seconds", + requested, max + ))); + } + } + + let time_expires = requested_ttl + .or(silo_max_ttl) + .map(|ttl| Utc::now() + Duration::seconds(ttl.0.into())); + let token = DeviceAccessToken::new( db_request.client_id, db_request.device_code, db_request.time_created, silo_user_id, - // Token gets the max TTL for the silo (if there is one) until we - // build a way for the user to ask for a different TTL - silo_auth_settings - .device_token_max_ttl_seconds - .map(|ttl| Utc::now() + Duration::seconds(ttl.0.into())), + time_expires, ); if db_request.time_expires < Utc::now() { @@ -224,7 +241,7 @@ impl super::Nexus { pub(crate) async fn device_access_token( &self, opctx: &OpContext, - params: DeviceAccessTokenRequest, + params: params::DeviceAccessTokenRequest, ) -> Result, HttpError> { // RFC 8628 ยง3.4 if params.grant_type != "urn:ietf:params:oauth:grant-type:device_code" { diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 69366a4674a..fd23b7e8b35 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7856,9 +7856,8 @@ impl NexusExternalApi for NexusExternalApiImpl { } }; - let model = nexus - .device_auth_request_create(&opctx, params.client_id) - .await?; + let model = + nexus.device_auth_request_create(&opctx, params).await?; nexus.build_oauth_response( StatusCode::OK, &model.into_response(rqctx.server.using_tls(), host), diff --git a/nexus/tests/integration_tests/device_auth.rs b/nexus/tests/integration_tests/device_auth.rs index 110624f6239..138cd2a572b 100644 --- a/nexus/tests/integration_tests/device_auth.rs +++ b/nexus/tests/integration_tests/device_auth.rs @@ -5,8 +5,8 @@ use std::num::NonZeroU32; use chrono::Utc; -use dropshot::ResultsPage; use dropshot::test_util::ClientTestContext; +use dropshot::{HttpErrorResponseBody, ResultsPage}; use nexus_auth::authn::USER_TEST_UNPRIVILEGED; use nexus_db_queries::db::fixed_data::silo::DEFAULT_SILO; use nexus_db_queries::db::identity::{Asset, Resource}; @@ -55,7 +55,9 @@ async fn test_device_auth_flow(cptestctx: &ControlPlaneTestContext) { .expect("failed to reject device auth start without client_id"); let client_id = Uuid::new_v4(); - let authn_params = DeviceAuthRequest { client_id }; + // note that this exercises ttl_seconds being omitted from the body because + // it's URL encoded, so None means it's omitted + let authn_params = DeviceAuthRequest { client_id, ttl_seconds: None }; // Using a JSON encoded body fails. RequestBuilder::new(testctx, Method::POST, "/device/auth") @@ -241,7 +243,7 @@ async fn test_device_auth_flow(cptestctx: &ControlPlaneTestContext) { /// as a string async fn get_device_token(testctx: &ClientTestContext) -> String { let client_id = Uuid::new_v4(); - let authn_params = DeviceAuthRequest { client_id }; + let authn_params = DeviceAuthRequest { client_id, ttl_seconds: None }; // Start a device authentication flow let auth_response: DeviceAuthResponse = @@ -431,6 +433,184 @@ async fn test_device_token_expiration(cptestctx: &ControlPlaneTestContext) { assert_eq!(settings.device_token_max_ttl_seconds, None); } +// lets me stick whatever I want in this thing to be URL-encoded +#[derive(serde::Serialize)] +struct BadAuthReq { + client_id: String, + ttl_seconds: String, +} + +/// Test that 0 and negative values for ttl_seconds give immediate 400s +#[nexus_test] +async fn test_device_token_request_ttl_invalid( + cptestctx: &ControlPlaneTestContext, +) { + let testctx = &cptestctx.external_client; + + let auth_response = NexusRequest::new( + RequestBuilder::new(testctx, Method::POST, "/device/auth") + .allow_non_dropshot_errors() + .body_urlencoded(Some(&BadAuthReq { + client_id: Uuid::new_v4().to_string(), + ttl_seconds: "0".to_string(), + })) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .execute() + // .execute_and_parse_unwrap::() + .await + .expect("expected an Ok(TestResponse)"); + + let error_body: serde_json::Value = + serde_json::from_slice(&auth_response.body).unwrap(); + assert_eq!( + error_body.get("message").unwrap().to_string(), + "\"unable to parse URL-encoded body: ttl_seconds: \ + invalid value: integer `0`, expected a nonzero u32\"" + ); + + let auth_response = NexusRequest::new( + RequestBuilder::new(testctx, Method::POST, "/device/auth") + .allow_non_dropshot_errors() + .body_urlencoded(Some(&BadAuthReq { + client_id: Uuid::new_v4().to_string(), + ttl_seconds: "-3".to_string(), + })) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .execute() + // .execute_and_parse_unwrap::() + .await + .expect("expected an Ok(TestResponse)"); + + let error_body: serde_json::Value = + serde_json::from_slice(&auth_response.body).unwrap(); + assert_eq!( + error_body.get("message").unwrap().to_string(), + "\"unable to parse URL-encoded body: ttl_seconds: \ + invalid digit found in string\"" + ); +} + +#[nexus_test] +async fn test_device_token_request_ttl(cptestctx: &ControlPlaneTestContext) { + let testctx = &cptestctx.external_client; + + // Set silo max TTL to 10 seconds + let settings = params::SiloAuthSettingsUpdate { + device_token_max_ttl_seconds: NonZeroU32::new(10).into(), + }; + let _: views::SiloAuthSettings = + object_put(testctx, "/v1/auth-settings", &settings).await; + + // Request TTL above the max should fail at verification time + let invalid_ttl = DeviceAuthRequest { + client_id: Uuid::new_v4(), + ttl_seconds: NonZeroU32::new(20), // Above the 10 second max + }; + + let auth_response = NexusRequest::new( + RequestBuilder::new(testctx, Method::POST, "/device/auth") + .body_urlencoded(Some(&invalid_ttl)) + .expect_status(Some(StatusCode::OK)), + ) + .execute_and_parse_unwrap::() + .await; + + let confirm_params = + DeviceAuthVerify { user_code: auth_response.user_code }; + + // Confirmation fails because requested TTL exceeds max + let confirm_error = NexusRequest::new( + RequestBuilder::new(testctx, Method::POST, "/device/confirm") + .body(Some(&confirm_params)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute_and_parse_unwrap::() + .await; + + // Check that the error message mentions TTL + assert_eq!(confirm_error.error_code, Some("InvalidRequest".to_string())); + assert_eq!( + confirm_error.message, + "Requested TTL 20 seconds exceeds maximum allowed TTL \ + for this silo of 10 seconds" + ); + + // Request TTL below the max should succeed and be used + let valid_ttl = DeviceAuthRequest { + client_id: Uuid::new_v4(), + ttl_seconds: NonZeroU32::new(3), // Below the 10 second max + }; + + let auth_response = NexusRequest::new( + RequestBuilder::new(testctx, Method::POST, "/device/auth") + .body_urlencoded(Some(&valid_ttl)) + .expect_status(Some(StatusCode::OK)), + ) + .execute_and_parse_unwrap::() + .await; + + let device_code = auth_response.device_code; + let user_code = auth_response.user_code; + let confirm_params = DeviceAuthVerify { user_code }; + + // this time will be pretty close to the now() used on the server when + // calculating expiration time + let t0 = Utc::now(); + + // Confirmation should succeed + NexusRequest::new( + RequestBuilder::new(testctx, Method::POST, "/device/confirm") + .body(Some(&confirm_params)) + .expect_status(Some(StatusCode::NO_CONTENT)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to confirm"); + + let token_params = DeviceAccessTokenRequest { + grant_type: "urn:ietf:params:oauth:grant-type:device_code".to_string(), + device_code, + client_id: valid_ttl.client_id, + }; + + // Get the token + let token_grant = NexusRequest::new( + RequestBuilder::new(testctx, Method::POST, "/device/token") + .allow_non_dropshot_errors() + .body_urlencoded(Some(&token_params)) + .expect_status(Some(StatusCode::OK)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute_and_parse_unwrap::() + .await; + + // Verify the token has roughly the correct expiration time. One second + // threshold is sufficient to confirm it's not getting the silo max of 10 + // seconds. Locally, I saw diffs as low as 14ms. + let tokens = get_tokens_priv(testctx).await; + let time_expires = tokens[0].time_expires.unwrap(); + let expected_expires = t0 + Duration::from_secs(3); + let diff_ms = (time_expires - expected_expires).num_milliseconds().abs(); + assert!(diff_ms <= 1000, "time diff was {diff_ms} ms. should be near zero"); + + // Token should work initially + project_list(&testctx, &token_grant.access_token, StatusCode::OK) + .await + .expect("token should work initially"); + + // Wait for token to expire + sleep(Duration::from_secs(4)).await; + + // Token is expired + project_list(&testctx, &token_grant.access_token, StatusCode::UNAUTHORIZED) + .await + .expect("token should be expired"); +} + async fn get_tokens_priv( testctx: &ClientTestContext, ) -> Vec { diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 782d4d7f7f6..813afdc7af9 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -2467,6 +2467,9 @@ impl TryFrom for RelativeUri { #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct DeviceAuthRequest { pub client_id: Uuid, + /// Optional lifetime for the access token in seconds. If not specified, the + /// silo's max TTL will be used (if set). + pub ttl_seconds: Option, } #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] diff --git a/openapi/nexus.json b/openapi/nexus.json index 03d5ed9a5bf..1f86f1d33d8 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -16676,6 +16676,13 @@ "client_id": { "type": "string", "format": "uuid" + }, + "ttl_seconds": { + "nullable": true, + "description": "Optional lifetime for the access token in seconds. If not specified, the silo's max TTL will be used (if set).", + "type": "integer", + "format": "uint32", + "minimum": 1 } }, "required": [ diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index c26eeb026e9..71b15502677 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2811,7 +2811,9 @@ CREATE TABLE IF NOT EXISTS omicron.public.device_auth_request ( client_id UUID NOT NULL, device_code STRING(40) NOT NULL, time_created TIMESTAMPTZ NOT NULL, - time_expires TIMESTAMPTZ NOT NULL + time_expires TIMESTAMPTZ NOT NULL, + -- requested TTL for the token in seconds (if specified by the user) + token_ttl_seconds INT8 CHECK (token_ttl_seconds > 0) ); -- Access tokens granted in response to successful device authorization flows. @@ -5690,7 +5692,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '146.0.0', NULL) + (TRUE, NOW(), NOW(), '147.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/device-auth-request-ttl/up01.sql b/schema/crdb/device-auth-request-ttl/up01.sql new file mode 100644 index 00000000000..b6981077785 --- /dev/null +++ b/schema/crdb/device-auth-request-ttl/up01.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.device_auth_request + ADD COLUMN IF NOT EXISTS token_ttl_seconds INT8 CHECK (token_ttl_seconds > 0);