From 5ea89bdbc08674661a5f31d92d3360d3e72665da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Felipe=20=C3=81lvarez?= Date: Thu, 26 Mar 2026 09:27:52 +0100 Subject: [PATCH 1/8] feat(k8s): register system identities through cli command --- Tiltfile | 4 +- .../src/bin/main_agent_control_k8s_cli.rs | 15 +- .../src/bin/main_agent_control_onhost_cli.rs | 2 +- .../src/cli/common/system_identity.rs | 10 + agent-control/src/cli/k8s.rs | 1 + agent-control/src/cli/k8s/system_identity.rs | 410 ++++++++++++++++++ 6 files changed, 439 insertions(+), 3 deletions(-) create mode 100644 agent-control/src/cli/k8s/system_identity.rs diff --git a/Tiltfile b/Tiltfile index 431b9dc989..539847fb42 100644 --- a/Tiltfile +++ b/Tiltfile @@ -120,6 +120,7 @@ ac_flags = [ '--set=agentControlDeployment.chartValues.config.cdRemoteUpdate=' + enable_cd_remote_update, '--version=>=0.0.0-beta', '--set=agentControlDeployment.chartValues.image.imagePullPolicy=Always', + '--set=agentControlDeployment.chartValues.toolkitImage.imagePullPolicy=Always', '--values=' + sa_chart_values_file, ] @@ -193,7 +194,8 @@ helm_resource( flags=ac_flags, image_deps=['tilt.local/agent-control-dev', 'tilt.local/agent-control-cli-dev'], image_keys=[('agentControlDeployment.chartValues.image.registry', 'agentControlDeployment.chartValues.image.repository', 'agentControlDeployment.chartValues.image.tag'), - [('toolkitImage.registry', 'toolkitImage.repository', 'toolkitImage.tag')]], + [('agentControlDeployment.chartValues.toolkitImage.registry', 'agentControlDeployment.chartValues.toolkitImage.repository', 'agentControlDeployment.chartValues.toolkitImage.tag'), + ('toolkitImage.registry', 'toolkitImage.repository', 'toolkitImage.tag')]], resource_deps=ac_chart_deps ) diff --git a/agent-control/src/bin/main_agent_control_k8s_cli.rs b/agent-control/src/bin/main_agent_control_k8s_cli.rs index 2ed0f15daa..5c5723767c 100644 --- a/agent-control/src/bin/main_agent_control_k8s_cli.rs +++ b/agent-control/src/bin/main_agent_control_k8s_cli.rs @@ -1,8 +1,10 @@ -use clap::{Parser, Subcommand}; +use clap::error::ErrorKind; +use clap::{CommandFactory, Parser, Subcommand}; use newrelic_agent_control::cli::common::{error::CliError, logs}; use newrelic_agent_control::cli::k8s::install::agent_control::InstallAgentControl; use newrelic_agent_control::cli::k8s::install::flux::InstallFlux; use newrelic_agent_control::cli::k8s::install::{InstallData, apply_resources}; +use newrelic_agent_control::cli::k8s::system_identity::{self, register_system_identity}; use newrelic_agent_control::cli::k8s::uninstall::agent_control::{ AgentControlUninstallData, uninstall_agent_control, }; @@ -41,6 +43,9 @@ enum Operations { /// Remove the resources created to handled the Continuous Deployment utility #[clap(name = "remove-cd-resources")] RemoveCDResources(FluxUninstallData), + + /// Registers the System Identity to be used in Agent Control for authentication. + RegisterSystemIdentity(system_identity::Args), } fn main() -> ExitCode { @@ -67,6 +72,14 @@ fn main() -> ExitCode { Operations::RemoveCDResources(cd_data) => { remove_flux_crs(&cli.namespace, &cd_data.release_name) } + Operations::RegisterSystemIdentity(args) => match args.validate() { + Ok(spec) => register_system_identity(&cli.namespace, spec), + Err(err) => { + let mut cmd = Cli::command(); + cmd.error(ErrorKind::ArgumentConflict, err.to_string()) + .exit() + } + }, }; match result { diff --git a/agent-control/src/bin/main_agent_control_onhost_cli.rs b/agent-control/src/bin/main_agent_control_onhost_cli.rs index 67bee46fb2..58d839c6f3 100644 --- a/agent-control/src/bin/main_agent_control_onhost_cli.rs +++ b/agent-control/src/bin/main_agent_control_onhost_cli.rs @@ -37,7 +37,7 @@ fn main() -> ExitCode { let result = match cli.command { Commands::GenerateConfig(args) => match args.validate() { - Ok(inputs) => config_gen::generate(inputs), + Ok(params) => config_gen::generate(params), Err(err) => { let mut cmd = Cli::command(); cmd.error(ErrorKind::ArgumentConflict, err.to_string()) diff --git a/agent-control/src/cli/common/system_identity.rs b/agent-control/src/cli/common/system_identity.rs index 43b62c3553..db63344814 100644 --- a/agent-control/src/cli/common/system_identity.rs +++ b/agent-control/src/cli/common/system_identity.rs @@ -38,6 +38,16 @@ pub struct Identity { } /// Arguments required to provide or generate a system identity. +/// +/// **Design note:** modelling the different provisioning methods as subcommands would be +/// a more natural fit for clap (each subcommand carrying only its own required fields), +/// but it was deliberately avoided. These arguments are shared across several external +/// installation tools — the Linux recipe, the Windows installation script, and the +/// Kubernetes installation job — none of which can easily branch on a subcommand name. +/// A flat, optional-field struct means the logic for selecting the provisioning method +/// (reuse an existing identity, generate a new one, obtain a bearer token via client +/// credentials, or accept a pre-obtained token) lives here rather than being duplicated +/// in each installer. #[derive(Debug, Default, clap::Args)] pub struct SystemIdentityArgs { /// Path where the private key is stored or will be written. diff --git a/agent-control/src/cli/k8s.rs b/agent-control/src/cli/k8s.rs index 8380c475b9..1aeb0b046e 100644 --- a/agent-control/src/cli/k8s.rs +++ b/agent-control/src/cli/k8s.rs @@ -1,4 +1,5 @@ pub mod errors; pub mod install; +pub mod system_identity; pub mod uninstall; pub mod utils; diff --git a/agent-control/src/cli/k8s/system_identity.rs b/agent-control/src/cli/k8s/system_identity.rs new file mode 100644 index 0000000000..d699f5208b --- /dev/null +++ b/agent-control/src/cli/k8s/system_identity.rs @@ -0,0 +1,410 @@ +use fs::file::{LocalFile, reader::FileReader}; +use k8s_openapi::{Resource as _, api::core::v1::Secret}; +use kube::api::{DynamicObject, ObjectMeta, TypeMeta}; +use serde_json::json; +use tracing::info; + +#[cfg_attr(test, mockall_double::double)] +use crate::k8s::client::SyncK8sClient; +use crate::{ + agent_control::agent_id::AgentID, + cli::{ + common::{ + error::CliError, + proxy_config::ProxyConfig, + region::{Region, region_parser}, + system_identity::{Identity, SystemIdentityArgs, SystemIdentitySpec, provide_identity}, + }, + k8s::{errors::K8sCliError, utils::try_new_k8s_client}, + }, + k8s::{client::K8sObjectKey, labels::Labels}, +}; + +const CLIENT_ID_SECRET_KEY: &str = "CLIENT_ID"; +const PRIVATE_KEY_SECRET_KEY: &str = "private_key"; + +/// Registers a System Identity for Agent Control configuration +#[derive(Debug, clap::Parser)] +pub struct Args { + /// Identity Secret name + #[arg(long, required = true)] + secret_name: String, + + /// New Relic region + #[arg(long, value_parser = region_parser(), required = true)] + region: Region, + + /// Identity configuration + #[command(flatten)] + identity: SystemIdentityArgs, + + /// Proxy configuration + #[command(flatten)] + proxy_config: Option, +} + +/// Data required to register a System Identity, information from [Args] after validation. +#[derive(Debug)] +pub struct IdentityRegistrationSpec { + secret_name: String, + region: Region, + identity: SystemIdentitySpec, + proxy_config: Option, +} + +impl Args { + /// Performs additional args validation (not covered by clap's arguments) + pub fn validate(self) -> Result { + if let Some(proxy_config) = self.proxy_config.clone() + && let Err(err) = crate::http::config::ProxyConfig::try_from(proxy_config) + { + return Err(format!("invalid proxy configuration: {err}")); + } + Ok(IdentityRegistrationSpec { + secret_name: self.secret_name, + region: self.region, + identity: self.identity.validate()?, + proxy_config: self.proxy_config, + }) + } +} + +/// Registers a System Identity as defined in the provided spec. +pub fn register_system_identity( + namespace: &str, + spec: IdentityRegistrationSpec, +) -> Result<(), K8sCliError> { + let k8s_client = try_new_k8s_client()?; + provide_system_identity_secret(namespace, spec, &k8s_client, provide_identity) +} + +/// Helper function to implement [register_system_identity] while allowing the usage of mocks for the k8s client +/// and `provide_identity_fn`. +/// The function `provide_identity_fn` is expected to return an [Identity] and store the corresponding private +/// key in the filesystem. +fn provide_system_identity_secret( + namespace: &str, + spec: IdentityRegistrationSpec, + k8s_client: &SyncK8sClient, + provide_identity_fn: F, +) -> Result<(), K8sCliError> +where + F: Fn(&SystemIdentitySpec, Region, Option) -> Result, +{ + let secret_object_key = K8sObjectKey { + name: &spec.secret_name, + namespace, + }; + + info!( + secret_name = spec.secret_name, %namespace, + "Checking if the System Identity secret already exists..." + ); + if secret_already_exists(secret_object_key, k8s_client)? { + info!("System Identity already exists, all setup."); + return Ok(()); + } + + info!("Secret is not present, creating system identity"); + let identity = + provide_identity_fn(&spec.identity, spec.region, spec.proxy_config).map_err(|err| { + K8sCliError::Generic(format!("failure registering the System Identity: {err}")) + })?; + + let private_key = LocalFile + .read(identity.private_key_path.as_path()) + .map_err(|err| { + K8sCliError::Generic(format!( + "failure reading System Identity generated private-key: {err}" + )) + })?; + + let secret_content = json!({"stringData": { + CLIENT_ID_SECRET_KEY: identity.client_id, + PRIVATE_KEY_SECRET_KEY: private_key + }}); + + let secret = secret_dynamic_object(secret_object_key, secret_content); + + k8s_client.apply_dynamic_object(&secret).map_err(|err| { + K8sCliError::ApplyResource(format!( + "failure creating the System Identity secret: {err}" + )) + })?; + + info!( + secret_name = spec.secret_name, %namespace, + "System identity successfully stored" + ); + + Ok(()) +} + +fn secret_already_exists( + object_key: K8sObjectKey<'_>, + k8s_client: &SyncK8sClient, +) -> Result { + k8s_client + .get_dynamic_object(&secret_type_meta(), object_key) + .map(|s| s.is_some()) + .map_err(|err| { + K8sCliError::GetResource(format!( + "failed to check if the system identity secret exists: {err}" + )) + }) +} + +/// Builds the secret to store the System Identity data with the provided data. It includes the [Labels] corresponding +/// to the agent [AgentID::AgentControl]. +fn secret_dynamic_object(object_key: K8sObjectKey<'_>, data: serde_json::Value) -> DynamicObject { + let labels = Labels::new(&AgentID::AgentControl); + DynamicObject { + types: Some(secret_type_meta()), + metadata: ObjectMeta { + name: Some(object_key.name.to_string()), + namespace: Some(object_key.namespace.to_string()), + labels: Some(labels.get()), + ..Default::default() + }, + data, + } +} + +fn secret_type_meta() -> TypeMeta { + TypeMeta { + api_version: Secret::API_VERSION.to_string(), + kind: Secret::KIND.to_string(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::cli::common::system_identity::ProvisioningMethod; + use crate::k8s::client::MockSyncK8sClient; + use assert_matches::assert_matches; + use clap::{CommandFactory, FromArgMatches}; + use rstest::rstest; + use std::{env::current_dir, path::PathBuf, sync::Arc}; + use tempfile::TempDir; + + impl Default for IdentityRegistrationSpec { + fn default() -> Self { + IdentityRegistrationSpec { + secret_name: "test-secret".to_string(), + region: Region::US, + identity: SystemIdentitySpec { + method: ProvisioningMethod::ExistingIdentity { + auth_client_id: "test-client-id".to_string(), + }, + private_key_path: PathBuf::from("/test/key"), + }, + proxy_config: None, + } + } + } + + #[rstest] + #[case::token_based_identity( + || String::from("--secret-name s --region us --auth-private-key-path /some/path --auth-parent-token TOKEN --auth-parent-client-id id --organization-id org-id") + )] + #[case::client_secret_based_identity( + || String::from("--secret-name s --region us --auth-private-key-path /some/path --auth-parent-client-secret SECRET --auth-parent-client-id id --organization-id org-id") + )] + fn test_args_validation(#[case] args: fn() -> String) { + let cmd = Args::command().no_binary_name(true); + let matches = cmd + .try_get_matches_from(args().split_ascii_whitespace()) + .expect("arguments should be valid"); + let args = Args::from_arg_matches(&matches).expect("should create the struct back"); + assert!(args.validate().is_ok()); + } + + #[rstest] + #[case::missing_private_key_path( + || String::from("--secret-name s --region us --auth-client-id some-id") + )] + #[case::no_identity_method( + || format!("--secret-name s --region us --auth-private-key-path {}", current_dir().unwrap().display()) + )] + #[case::missing_org_id_with_token( + || String::from("--secret-name s --region us --auth-private-key-path /p --auth-parent-token TOKEN --auth-parent-client-id id") + )] + #[case::missing_parent_client_id_with_secret( + || String::from("--secret-name s --region us --auth-private-key-path /p --auth-parent-client-secret SECRET --organization-id org-id") + )] + #[case::missing_org_id_with_secret( + || String::from("--secret-name s --region us --auth-private-key-path /p --auth-parent-client-secret SECRET --auth-parent-client-id id") + )] + #[case::invalid_proxy_config( + || String::from("--secret-name s --region us --auth-private-key-path /p --auth-parent-client-secret SECRET --auth-parent-client-id id --organization-id org-id --proxy-url https::/invalid") + )] + fn test_args_validation_errors(#[case] args: fn() -> String) { + let cmd = Args::command().no_binary_name(true); + let matches = cmd + .try_get_matches_from(args().split_ascii_whitespace()) + .expect("arguments should be valid"); + let args = Args::from_arg_matches(&matches).expect("should create the struct back"); + assert_matches!(args.validate(), Err(_)); + } + + fn empty_dynamic_object() -> Arc { + Arc::new(DynamicObject { + types: None, + metadata: Default::default(), + data: serde_json::Value::Null, + }) + } + + fn mock_secret_not_found() -> MockSyncK8sClient { + let mut mock_client = MockSyncK8sClient::new(); + mock_client + .expect_get_dynamic_object() + .once() + .returning(|_, _| Ok(None)); + mock_client + } + + #[test] + fn test_secret_already_exists_skips_creation() { + let mut mock_client = MockSyncK8sClient::new(); + + mock_client + .expect_get_dynamic_object() + .once() + .returning(|_, _| Ok(Some(empty_dynamic_object()))); + + mock_client.expect_apply_dynamic_object().never(); + + provide_system_identity_secret( + "test-namespace", + IdentityRegistrationSpec::default(), + &mock_client, + |_, _, _| panic!("identity provider should not be called"), + ) + .expect("system identity should be provided successfully"); + } + + #[test] + fn test_creates_secret_when_not_present() { + let tempdir = TempDir::new().unwrap(); + let key_path = tempdir.path().join("private-key"); + std::fs::write(&key_path, "test-private-key-content").unwrap(); + + let mut mock_client = MockSyncK8sClient::new(); + + mock_client + .expect_get_dynamic_object() + .once() + .returning(|_, _| Ok(None)); + + mock_client + .expect_apply_dynamic_object() + .once() + .withf(|obj| { + obj.metadata.name.as_deref() == Some("test-secret") + && obj.metadata.namespace.as_deref() == Some("test-namespace") + && obj.data["stringData"][CLIENT_ID_SECRET_KEY].as_str() + == Some("new-client-id") + && obj.data["stringData"][PRIVATE_KEY_SECRET_KEY].as_str() + == Some("test-private-key-content") + }) + .returning(|_| Ok(())); + + provide_system_identity_secret( + "test-namespace", + IdentityRegistrationSpec::default(), + &mock_client, + move |_, _, _| { + Ok(Identity { + client_id: "new-client-id".to_string(), + private_key_path: key_path.clone(), + }) + }, + ) + .expect("system identity should be provided successfully"); + } + + #[test] + fn test_get_dynamic_object_error_returns_error() { + let mut mock_client = MockSyncK8sClient::new(); + mock_client + .expect_get_dynamic_object() + .once() + .returning(|_, _| { + Err(crate::k8s::error::K8sError::Generic( + "k8s error".to_string(), + )) + }); + + let result = provide_system_identity_secret( + "test-namespace", + IdentityRegistrationSpec::default(), + &mock_client, + |_, _, _| panic!("should not be called"), + ); + assert_matches!(result, Err(K8sCliError::GetResource(_))); + } + + #[test] + fn test_provide_identity_error_returns_error() { + let mock_client = mock_secret_not_found(); + + let result = provide_system_identity_secret( + "test-namespace", + IdentityRegistrationSpec::default(), + &mock_client, + |_, _, _| Err(CliError::Command("identity failure".to_string())), + ); + assert_matches!(result, Err(K8sCliError::Generic(_))); + } + + #[test] + fn test_error_reading_private_key_returns_error() { + let mock_client = mock_secret_not_found(); + + let result = provide_system_identity_secret( + "test-namespace", + IdentityRegistrationSpec::default(), + &mock_client, + |_, _, _| { + Ok(Identity { + client_id: "id".to_string(), + private_key_path: PathBuf::from("/does/not/exist/key.pem"), + }) + }, + ); + assert_matches!(result, Err(K8sCliError::Generic(_))); + } + + #[test] + fn test_apply_dynamic_object_error_returns_error() { + let tempdir = TempDir::new().unwrap(); + let key_path = tempdir.path().join("private-key"); + std::fs::write(&key_path, "key-content").unwrap(); + + let mut mock_client = mock_secret_not_found(); + mock_client + .expect_apply_dynamic_object() + .once() + .returning(|_| { + Err(crate::k8s::error::K8sError::Generic( + "apply failed".to_string(), + )) + }); + + let key_path_clone = key_path.clone(); + let result = provide_system_identity_secret( + "test-namespace", + IdentityRegistrationSpec::default(), + &mock_client, + move |_, _, _| { + Ok(Identity { + client_id: "id".to_string(), + private_key_path: key_path_clone.clone(), + }) + }, + ); + assert_matches!(result, Err(K8sCliError::ApplyResource(_))); + } +} From 55ca30a5a3d32839ee9e943e4f8c5aa8f818a553 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Felipe=20=C3=81lvarez?= Date: Tue, 31 Mar 2026 17:22:18 +0200 Subject: [PATCH 2/8] chore: update nr-auth --- Cargo.lock | 81 +++++++++---------- agent-control/Cargo.toml | 2 +- .../src/cli/common/system_identity.rs | 25 ++---- 3 files changed, 47 insertions(+), 61 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e82790b817..0d8e459bd1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -49,7 +49,7 @@ dependencies = [ "mime", "percent-encoding", "pin-project-lite", - "rand 0.9.2", + "rand 0.9.4", "sha1", "smallvec", "tokio", @@ -725,7 +725,7 @@ checksum = "6f8d983286843e49675a4b7a2d174efe136dc93a18d69130dd18198a6c167601" dependencies = [ "cfg-if", "cpufeatures 0.3.0", - "rand_core 0.10.0", + "rand_core 0.10.1", ] [[package]] @@ -1506,7 +1506,7 @@ dependencies = [ "dummy", "either", "http 1.4.0", - "rand 0.10.0", + "rand 0.10.1", "url-escape", ] @@ -1754,7 +1754,7 @@ dependencies = [ "js-sys", "libc", "r-efi 6.0.0", - "rand_core 0.10.0", + "rand_core 0.10.1", "wasip2", "wasip3", "wasm-bindgen", @@ -2079,9 +2079,9 @@ dependencies = [ [[package]] name = "hyper-rustls" -version = "0.27.7" +version = "0.27.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e3c93eb611681b207e1fe55d5a71ecf91572ec8a6705cdb6857f7d8d5242cf58" +checksum = "c2b52f86d1d4bc0d6b4e6826d960b1b333217e07d36b882dca570a5e1c48895b" dependencies = [ "http 1.4.0", "hyper", @@ -2089,7 +2089,6 @@ dependencies = [ "log", "rustls", "rustls-native-certs", - "rustls-pki-types", "tokio", "tokio-rustls", "tower-service", @@ -2416,9 +2415,9 @@ dependencies = [ [[package]] name = "js-sys" -version = "0.3.94" +version = "0.3.95" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2e04e2ef80ce82e13552136fabeef8a5ed1f985a96805761cbb9a2c34e7664d9" +checksum = "2964e92d1d9dc3364cae4d718d93f227e3abb088e747d92e0395bfdedf1c12ca" dependencies = [ "cfg-if", "futures-util", @@ -2636,9 +2635,9 @@ checksum = "2c4a545a15244c7d945065b5d392b2d2d7f21526fba56ce51467b06ed445e8f7" [[package]] name = "libc" -version = "0.2.184" +version = "0.2.185" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "48f5d2a454e16a5ea0f4ced81bd44e4cfc7bd3a507b61887c99fd3538b28e4af" +checksum = "52ff2c0fe9bc6cb6b14a0592c2ff4fa9ceb83eea9db979b0487cd054946a2b8f" [[package]] name = "libflate" @@ -2958,8 +2957,8 @@ checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be" [[package]] name = "nr-auth" -version = "0.2.0" -source = "git+https://github.com/newrelic/newrelic-auth-rs.git?tag=0.2.0#ba56af17de237a6e80138f52ff3d9ce73262fd06" +version = "0.4.0" +source = "git+https://github.com/newrelic/newrelic-auth-rs.git?branch=main#50ce9271a004756a3a3f91cf0df79e18b77b64ca" dependencies = [ "base64 0.22.1", "chrono", @@ -3226,7 +3225,7 @@ dependencies = [ "futures-util", "opentelemetry", "percent-encoding", - "rand 0.9.2", + "rand 0.9.4", "thiserror 2.0.18", ] @@ -3401,9 +3400,9 @@ checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" [[package]] name = "pkg-config" -version = "0.3.32" +version = "0.3.33" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7edddbd0b52d732b21ad9a5fab5c704c14cd949e5e9a1ec5929a24fded1b904c" +checksum = "19f132c84eca552bf34cab8ec81f1c1dcc229b811638f9d283dceabe58c5569e" [[package]] name = "plain" @@ -3626,7 +3625,7 @@ dependencies = [ "bytes", "getrandom 0.3.4", "lru-slab", - "rand 0.9.2", + "rand 0.9.4", "ring", "rustc-hash 2.1.2", "rustls", @@ -3675,9 +3674,9 @@ checksum = "f8dcc9c7d52a811697d2151c701e0d08956f92b0e24136cf4cf27b57a6a0d9bf" [[package]] name = "rand" -version = "0.9.2" +version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6db2770f06117d490610c7488547d543617b21bfa07796d7a12f6f1bd53850d1" +checksum = "44c5af06bb1b7d3216d91932aed5265164bf384dc89cd6ba05cf59a35f5f76ea" dependencies = [ "rand_chacha", "rand_core 0.9.5", @@ -3685,13 +3684,13 @@ dependencies = [ [[package]] name = "rand" -version = "0.10.0" +version = "0.10.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bc266eb313df6c5c09c1c7b1fbe2510961e5bcd3add930c1e31f7ed9da0feff8" +checksum = "d2e8e8bcc7961af1fdac401278c6a831614941f6164ee3bf4ce61b7edb162207" dependencies = [ "chacha20", "getrandom 0.4.2", - "rand_core 0.10.0", + "rand_core 0.10.1", ] [[package]] @@ -3724,9 +3723,9 @@ dependencies = [ [[package]] name = "rand_core" -version = "0.10.0" +version = "0.10.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0c8d0fd677905edcbeedbf2edb6494d676f0e98d54d5cf9bda0b061cb8fb8aba" +checksum = "63b8176103e19a2643978565ca18b50549f6101881c443590420e4dc998a3c69" [[package]] name = "rcgen" @@ -4059,9 +4058,9 @@ dependencies = [ [[package]] name = "rustls" -version = "0.23.37" +version = "0.23.38" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "758025cb5fccfd3bc2fd74708fd4682be41d99e5dff73c377c0646c6012c73a4" +checksum = "69f9466fb2c14ea04357e91413efb882e2a6d4a406e625449bc0a5d360d53a21" dependencies = [ "aws-lc-rs", "log", @@ -4124,9 +4123,9 @@ checksum = "f87165f0995f63a9fbeea62b64d10b4d9d8e78ec6d7d51fb2125fda7bb36788f" [[package]] name = "rustls-webpki" -version = "0.103.10" +version = "0.103.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "df33b2b81ac578cabaf06b89b0631153a3f416b0a886e8a7a1707fb51abbd1ef" +checksum = "20a6af516fea4b20eccceaf166e8aa666ac996208e8a644ce3ef5aa783bc7cd4" dependencies = [ "aws-lc-rs", "ring", @@ -5153,7 +5152,7 @@ dependencies = [ "http 1.4.0", "httparse", "log", - "rand 0.9.2", + "rand 0.9.4", "sha1", "thiserror 2.0.18", "utf-8", @@ -5358,9 +5357,9 @@ dependencies = [ [[package]] name = "wasm-bindgen" -version = "0.2.117" +version = "0.2.118" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0551fc1bb415591e3372d0bc4780db7e587d84e2a7e79da121051c5c4b89d0b0" +checksum = "0bf938a0bacb0469e83c1e148908bd7d5a6010354cf4fb73279b7447422e3a89" dependencies = [ "cfg-if", "once_cell", @@ -5371,9 +5370,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-futures" -version = "0.4.67" +version = "0.4.68" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "03623de6905b7206edd0a75f69f747f134b7f0a2323392d664448bf2d3c5d87e" +checksum = "f371d383f2fb139252e0bfac3b81b265689bf45b6874af544ffa4c975ac1ebf8" dependencies = [ "js-sys", "wasm-bindgen", @@ -5381,9 +5380,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro" -version = "0.2.117" +version = "0.2.118" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7fbdf9a35adf44786aecd5ff89b4563a90325f9da0923236f6104e603c7e86be" +checksum = "eeff24f84126c0ec2db7a449f0c2ec963c6a49efe0698c4242929da037ca28ed" dependencies = [ "quote", "wasm-bindgen-macro-support", @@ -5391,9 +5390,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro-support" -version = "0.2.117" +version = "0.2.118" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dca9693ef2bab6d4e6707234500350d8dad079eb508dca05530c85dc3a529ff2" +checksum = "9d08065faf983b2b80a79fd87d8254c409281cf7de75fc4b773019824196c904" dependencies = [ "bumpalo", "proc-macro2", @@ -5404,9 +5403,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-shared" -version = "0.2.117" +version = "0.2.118" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "39129a682a6d2d841b6c429d0c51e5cb0ed1a03829d8b3d1e69a011e62cb3d3b" +checksum = "5fd04d9e306f1907bd13c6361b5c6bfc7b3b3c095ed3f8a9246390f8dbdee129" dependencies = [ "unicode-ident", ] @@ -5460,9 +5459,9 @@ dependencies = [ [[package]] name = "web-sys" -version = "0.3.94" +version = "0.3.95" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cd70027e39b12f0849461e08ffc50b9cd7688d942c1c8e3c7b22273236b4dd0a" +checksum = "4f2dfbb17949fa2088e5d39408c48368947b86f7834484e87b73de55bc14d97d" dependencies = [ "js-sys", "wasm-bindgen", diff --git a/agent-control/Cargo.toml b/agent-control/Cargo.toml index 8656313998..415a8b6bf0 100644 --- a/agent-control/Cargo.toml +++ b/agent-control/Cargo.toml @@ -32,7 +32,7 @@ semver = { workspace = true, features = ["serde"] } chrono = { workspace = true } base64 = { version = "0.22.1" } # New Relic dependencies (external repos) -nr-auth = { git = "https://github.com/newrelic/newrelic-auth-rs.git", tag = "0.2.0" } +nr-auth = { git = "https://github.com/newrelic/newrelic-auth-rs.git", branch = "main" } # TODO: revert back to tag when ready opamp-client = { git = "https://github.com/newrelic/newrelic-opamp-rs.git", tag = "0.0.36" } # local dependencies fs = { path = "../fs" } diff --git a/agent-control/src/cli/common/system_identity.rs b/agent-control/src/cli/common/system_identity.rs index db63344814..e803ca439b 100644 --- a/agent-control/src/cli/common/system_identity.rs +++ b/agent-control/src/cli/common/system_identity.rs @@ -15,11 +15,8 @@ use nr_auth::{ }, system_identity::{ generator::L2SystemIdentityGenerator, - iam_client::http::HttpIAMClient, - input_data::{ - SystemIdentityCreationMetadata, SystemIdentityInput, environment::NewRelicEnvironment, - output_platform::OutputPlatform, - }, + iam_client::http::{HttpIAMClient, IAMAuthCredential}, + input_data::{SystemIdentityCreationMetadata, environment::NewRelicEnvironment}, }, token::{Token, TokenType}, token_retriever::TokenRetrieverWithCache, @@ -85,7 +82,6 @@ pub enum ProvisioningMethod { }, ParentToken { token: String, - parent_client_id: String, organization_id: String, }, ParentSecret { @@ -139,7 +135,6 @@ impl SystemIdentityArgs { self.require_org_and_parent_id("token based")?; return Ok(ProvisioningMethod::ParentToken { token: self.auth_parent_token, - parent_client_id: self.auth_parent_client_id, organization_id: self.organization_id, }); } @@ -200,7 +195,6 @@ fn build_identity( } ProvisioningMethod::ParentToken { token, - parent_client_id, organization_id, } => { let token = Token::new(token.to_string(), TokenType::Bearer, Default::default()); @@ -209,7 +203,6 @@ fn build_identity( token, private_key_path.clone(), organization_id.clone(), - parent_client_id.clone(), environment, http_client, ) @@ -230,7 +223,6 @@ fn build_identity( token, private_key_path.clone(), organization_id.clone(), - parent_client_id.clone(), environment, http_client, ) @@ -282,20 +274,13 @@ fn build_identity_from_token( token: Token, private_key_path: PathBuf, organization_id: String, - parent_client_id: String, environment: NewRelicEnvironment, http_client: HttpClient, ) -> Result { - let output_platform = OutputPlatform::LocalPrivateKeyPath(private_key_path.clone()); - let system_identity_creation_metadata = SystemIdentityCreationMetadata { - system_identity_input: SystemIdentityInput { - organization_id: organization_id.clone(), - client_id: parent_client_id.clone(), - }, + organization_id: organization_id.clone(), name: None, environment, - output_platform, }; let iam_client = HttpIAMClient::new(http_client, system_identity_creation_metadata.to_owned()); @@ -309,8 +294,10 @@ fn build_identity_from_token( key_creator, }; + let auth_credential = IAMAuthCredential::BearerToken(token.access_token().to_string()); + let result = system_identity_generator - .generate(&token) + .generate(&auth_credential) .map_err(|err| CliError::Command(format!("error generating the system identity: {err}")))?; info!( From b0d31c5c20f697b376c745e57181cb1fc19ce21b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Felipe=20=C3=81lvarez?= Date: Wed, 1 Apr 2026 14:25:37 +0200 Subject: [PATCH 3/8] chore(cli): avoid persisting private key on k8s --- .../src/cli/common/system_identity.rs | 118 ++++++++---------- agent-control/src/cli/k8s/system_identity.rs | 116 ++++++++--------- agent-control/src/cli/on_host/config_gen.rs | 38 ++++-- 3 files changed, 135 insertions(+), 137 deletions(-) diff --git a/agent-control/src/cli/common/system_identity.rs b/agent-control/src/cli/common/system_identity.rs index e803ca439b..e2104411de 100644 --- a/agent-control/src/cli/common/system_identity.rs +++ b/agent-control/src/cli/common/system_identity.rs @@ -9,10 +9,7 @@ use nr_auth::{ TokenRetriever, authenticator::HttpAuthenticator, http::{client::HttpClient, config::HttpConfig}, - key::{ - creator::KeyType, - local::{KeyPairGeneratorLocalConfig, LocalCreator}, - }, + key::creator::Creator, system_identity::{ generator::L2SystemIdentityGenerator, iam_client::http::{HttpIAMClient, IAMAuthCredential}, @@ -28,12 +25,6 @@ use crate::cli::common::{error::CliError, proxy_config::ProxyConfig, region::Reg const DEFAULT_TIMEOUT: Duration = Duration::from_secs(3); const DEFAULT_RETRIES: u8 = 3; -/// Represents a key-based identity to be used in Agent Control configuration. -pub struct Identity { - pub client_id: String, - pub private_key_path: PathBuf, -} - /// Arguments required to provide or generate a system identity. /// /// **Design note:** modelling the different provisioning methods as subcommands would be @@ -164,14 +155,15 @@ impl SystemIdentityArgs { } } -/// Provides a key-based identity considering the supplied args. +/// Provides a key-based identity considering the supplied args. It returns the corresponding **client_id** as a String. pub fn provide_identity( identity_input: &SystemIdentitySpec, region: Region, proxy_config: Option, -) -> Result { + key_creator: impl Creator, +) -> Result { let environment = NewRelicEnvironment::from(region); - build_identity(identity_input, environment, proxy_config) + build_identity(identity_input, environment, proxy_config, key_creator) } /// Helper to allow injecting testing urls when building the identity. @@ -179,19 +171,12 @@ fn build_identity( identity_input: &SystemIdentitySpec, environment: NewRelicEnvironment, proxy_config: Option, -) -> Result { - let SystemIdentitySpec { - private_key_path, - method, - } = identity_input; - - match method { + key_creator: impl Creator, +) -> Result { + match &identity_input.method { ProvisioningMethod::ExistingIdentity { auth_client_id } => { info!("Using provided System Identity"); - Ok(Identity { - client_id: auth_client_id.to_string(), - private_key_path: private_key_path.clone(), - }) + Ok(auth_client_id.to_string()) } ProvisioningMethod::ParentToken { token, @@ -201,10 +186,10 @@ fn build_identity( let http_client = http_client(proxy_config)?; build_identity_from_token( token, - private_key_path.clone(), organization_id.clone(), environment, http_client, + key_creator, ) } ProvisioningMethod::ParentSecret { @@ -221,10 +206,10 @@ fn build_identity( )?; build_identity_from_token( token, - private_key_path.clone(), organization_id.clone(), environment, http_client, + key_creator, ) } } @@ -272,11 +257,11 @@ fn get_auth_token( fn build_identity_from_token( token: Token, - private_key_path: PathBuf, organization_id: String, environment: NewRelicEnvironment, http_client: HttpClient, -) -> Result { + key_creator: impl Creator, +) -> Result { let system_identity_creation_metadata = SystemIdentityCreationMetadata { organization_id: organization_id.clone(), name: None, @@ -284,11 +269,6 @@ fn build_identity_from_token( }; let iam_client = HttpIAMClient::new(http_client, system_identity_creation_metadata.to_owned()); - let key_creator = LocalCreator::from(KeyPairGeneratorLocalConfig { - key_type: KeyType::Rsa4096, - file_path: private_key_path.clone(), - }); - let system_identity_generator = L2SystemIdentityGenerator { iam_client, key_creator, @@ -300,15 +280,7 @@ fn build_identity_from_token( .generate(&auth_credential) .map_err(|err| CliError::Command(format!("error generating the system identity: {err}")))?; - info!( - private_key_path = %private_key_path.to_string_lossy(), - "System Identity successfully generated" - ); - - Ok(Identity { - client_id: result.client_id, - private_key_path, - }) + Ok(result.client_id) } /// Builds the proxy config for nr-auth from the AC's system proxy @@ -336,12 +308,38 @@ pub mod tests { use assert_matches::assert_matches; use http::header::AUTHORIZATION; use httpmock::{Method::POST, MockServer}; + use mockall::mock; + use nr_auth::key::creator::PublicKeyPem; use rstest::rstest; use std::fs; use tempfile::TempDir; + use thiserror::Error; use super::*; + #[derive(Debug, Error)] + #[error("{0}")] + pub struct TestCreatorError(String); + + mock! { + pub Creator {} + impl Creator for Creator { + type Error = TestCreatorError; + fn create(&self) -> Result; + } + } + impl MockCreator { + fn expecting_create_ok() -> Self { + let mut creator = Self::new(); + creator + .expect_create() + .once() + .returning(|| Ok(PublicKeyPem::default())); + + creator + } + } + #[rstest] #[case::existing_identity(|| SystemIdentityArgs { auth_private_key_path: Some(std::env::current_dir().unwrap()), @@ -430,10 +428,12 @@ pub mod tests { }; let identity_data = identity_args.validate().expect("validation should succeed"); - let identity = - build_identity(&identity_data, environment, None).expect("no error expected"); - assert_eq!(identity.client_id, "provided_client_id".to_string()); - assert_eq!(identity.private_key_path, auth_private_key_path); + let mut creator_mock = MockCreator::new(); + creator_mock.expect_create().never(); + + let client_id = build_identity(&identity_data, environment, None, creator_mock) + .expect("no error expected"); + assert_eq!(client_id, "provided_client_id".to_string()); } #[test] @@ -469,18 +469,13 @@ pub mod tests { .expect("url should be valid"), }; + let creator_mock = MockCreator::expecting_create_ok(); let identity_data = identity_args.validate().expect("validation should succeed"); - let identity = - build_identity(&identity_data, environment, None).expect("no error expected"); + let client_id = build_identity(&identity_data, environment, None, creator_mock) + .expect("no error expected"); identity_mock.assert_calls(1); - assert_eq!(identity.client_id, "created_client_id".to_string()); - assert_eq!(identity.private_key_path, auth_private_key_path); - assert!( - fs::read_to_string(&auth_private_key_path) - .unwrap() - .contains("BEGIN PRIVATE KEY"), - ); + assert_eq!(client_id, "created_client_id".to_string()); } #[test] @@ -524,19 +519,14 @@ pub mod tests { .expect("url should be valid"), }; + let creator_mock = MockCreator::expecting_create_ok(); let identity_data = identity_args.validate().expect("validation should succeed"); - let identity = - build_identity(&identity_data, environment, None).expect("no error expected"); + let client_id = build_identity(&identity_data, environment, None, creator_mock) + .expect("no error expected"); identity_mock.assert_calls(1); token_mock.assert_calls(1); - assert_eq!(identity.client_id, "created_client_id".to_string()); - assert_eq!(identity.private_key_path, auth_private_key_path); - assert!( - fs::read_to_string(&auth_private_key_path) - .unwrap() - .contains("BEGIN PRIVATE KEY"), - ); + assert_eq!(client_id, "created_client_id".to_string()); } fn token_body(token: &str) -> String { diff --git a/agent-control/src/cli/k8s/system_identity.rs b/agent-control/src/cli/k8s/system_identity.rs index d699f5208b..21aa43b902 100644 --- a/agent-control/src/cli/k8s/system_identity.rs +++ b/agent-control/src/cli/k8s/system_identity.rs @@ -1,6 +1,11 @@ -use fs::file::{LocalFile, reader::FileReader}; +use std::convert::Infallible; + use k8s_openapi::{Resource as _, api::core::v1::Secret}; use kube::api::{DynamicObject, ObjectMeta, TypeMeta}; +use nr_auth::key::{ + creator::{Creator, KeyPair, KeyType, PublicKeyPem}, + rsa::rsa, +}; use serde_json::json; use tracing::info; @@ -13,7 +18,7 @@ use crate::{ error::CliError, proxy_config::ProxyConfig, region::{Region, region_parser}, - system_identity::{Identity, SystemIdentityArgs, SystemIdentitySpec, provide_identity}, + system_identity::{SystemIdentityArgs, SystemIdentitySpec, provide_identity}, }, k8s::{errors::K8sCliError, utils::try_new_k8s_client}, }, @@ -80,8 +85,8 @@ pub fn register_system_identity( /// Helper function to implement [register_system_identity] while allowing the usage of mocks for the k8s client /// and `provide_identity_fn`. -/// The function `provide_identity_fn` is expected to return an [Identity] and store the corresponding private -/// key in the filesystem. +/// The function `provide_identity_fn` is expected to return the client_id of the registered System Identity. +/// The generated private key is stored in a Kubernetes Secret alongside the client_id. fn provide_system_identity_secret( namespace: &str, spec: IdentityRegistrationSpec, @@ -89,7 +94,12 @@ fn provide_system_identity_secret( provide_identity_fn: F, ) -> Result<(), K8sCliError> where - F: Fn(&SystemIdentitySpec, Region, Option) -> Result, + F: Fn( + &SystemIdentitySpec, + Region, + Option, + PublicKeyHolder, + ) -> Result, { let secret_object_key = K8sObjectKey { name: &spec.secret_name, @@ -104,23 +114,31 @@ where info!("System Identity already exists, all setup."); return Ok(()); } - info!("Secret is not present, creating system identity"); - let identity = - provide_identity_fn(&spec.identity, spec.region, spec.proxy_config).map_err(|err| { - K8sCliError::Generic(format!("failure registering the System Identity: {err}")) - })?; - let private_key = LocalFile - .read(identity.private_key_path.as_path()) + let KeyPair { + private_key, + public_key, + } = rsa(&KeyType::Rsa4096).map_err(|err| { + K8sCliError::Generic(format!( + "failure building key-pair for System Identity: {err}" + )) + })?; + let pk_holder = PublicKeyHolder { public_key }; + + let client_id = provide_identity_fn(&spec.identity, spec.region, spec.proxy_config, pk_holder) .map_err(|err| { - K8sCliError::Generic(format!( - "failure reading System Identity generated private-key: {err}" - )) + K8sCliError::Generic(format!("failure registering the System Identity: {err}")) })?; + let private_key = String::from_utf8(private_key).map_err(|err| { + K8sCliError::Generic(format!( + "failure decoding System Identity private-key: {err}" + )) + })?; + let secret_content = json!({"stringData": { - CLIENT_ID_SECRET_KEY: identity.client_id, + CLIENT_ID_SECRET_KEY: client_id, PRIVATE_KEY_SECRET_KEY: private_key }}); @@ -177,6 +195,19 @@ fn secret_type_meta() -> TypeMeta { } } +/// Helper struct to hold a public key an use it as [Creator] for System Identity provisioning. +struct PublicKeyHolder { + public_key: PublicKeyPem, +} + +impl Creator for PublicKeyHolder { + type Error = Infallible; + + fn create(&self) -> Result { + Ok(self.public_key.clone()) + } +} + #[cfg(test)] mod tests { use super::*; @@ -186,7 +217,6 @@ mod tests { use clap::{CommandFactory, FromArgMatches}; use rstest::rstest; use std::{env::current_dir, path::PathBuf, sync::Arc}; - use tempfile::TempDir; impl Default for IdentityRegistrationSpec { fn default() -> Self { @@ -280,17 +310,13 @@ mod tests { "test-namespace", IdentityRegistrationSpec::default(), &mock_client, - |_, _, _| panic!("identity provider should not be called"), + |_, _, _, _| panic!("identity provider should not be called"), ) .expect("system identity should be provided successfully"); } #[test] fn test_creates_secret_when_not_present() { - let tempdir = TempDir::new().unwrap(); - let key_path = tempdir.path().join("private-key"); - std::fs::write(&key_path, "test-private-key-content").unwrap(); - let mut mock_client = MockSyncK8sClient::new(); mock_client @@ -306,8 +332,9 @@ mod tests { && obj.metadata.namespace.as_deref() == Some("test-namespace") && obj.data["stringData"][CLIENT_ID_SECRET_KEY].as_str() == Some("new-client-id") - && obj.data["stringData"][PRIVATE_KEY_SECRET_KEY].as_str() - == Some("test-private-key-content") + && obj.data["stringData"][PRIVATE_KEY_SECRET_KEY] + .as_str() + .is_some_and(|k| k.contains("-----BEGIN PRIVATE KEY-----")) }) .returning(|_| Ok(())); @@ -315,12 +342,7 @@ mod tests { "test-namespace", IdentityRegistrationSpec::default(), &mock_client, - move |_, _, _| { - Ok(Identity { - client_id: "new-client-id".to_string(), - private_key_path: key_path.clone(), - }) - }, + move |_, _, _, _| Ok("new-client-id".to_string()), ) .expect("system identity should be provided successfully"); } @@ -341,7 +363,7 @@ mod tests { "test-namespace", IdentityRegistrationSpec::default(), &mock_client, - |_, _, _| panic!("should not be called"), + |_, _, _, _| panic!("should not be called"), ); assert_matches!(result, Err(K8sCliError::GetResource(_))); } @@ -354,35 +376,13 @@ mod tests { "test-namespace", IdentityRegistrationSpec::default(), &mock_client, - |_, _, _| Err(CliError::Command("identity failure".to_string())), - ); - assert_matches!(result, Err(K8sCliError::Generic(_))); - } - - #[test] - fn test_error_reading_private_key_returns_error() { - let mock_client = mock_secret_not_found(); - - let result = provide_system_identity_secret( - "test-namespace", - IdentityRegistrationSpec::default(), - &mock_client, - |_, _, _| { - Ok(Identity { - client_id: "id".to_string(), - private_key_path: PathBuf::from("/does/not/exist/key.pem"), - }) - }, + |_, _, _, _| Err(CliError::Command("identity failure".to_string())), ); assert_matches!(result, Err(K8sCliError::Generic(_))); } #[test] fn test_apply_dynamic_object_error_returns_error() { - let tempdir = TempDir::new().unwrap(); - let key_path = tempdir.path().join("private-key"); - std::fs::write(&key_path, "key-content").unwrap(); - let mut mock_client = mock_secret_not_found(); mock_client .expect_apply_dynamic_object() @@ -393,17 +393,11 @@ mod tests { )) }); - let key_path_clone = key_path.clone(); let result = provide_system_identity_secret( "test-namespace", IdentityRegistrationSpec::default(), &mock_client, - move |_, _, _| { - Ok(Identity { - client_id: "id".to_string(), - private_key_path: key_path_clone.clone(), - }) - }, + move |_, _, _, _| Ok("id".to_string()), ); assert_matches!(result, Err(K8sCliError::ApplyResource(_))); } diff --git a/agent-control/src/cli/on_host/config_gen.rs b/agent-control/src/cli/on_host/config_gen.rs index d3f1052bdc..0a4a082f39 100644 --- a/agent-control/src/cli/on_host/config_gen.rs +++ b/agent-control/src/cli/on_host/config_gen.rs @@ -4,13 +4,17 @@ use crate::cli::{ error::CliError, proxy_config::ProxyConfig, region::{Region, region_parser}, - system_identity::{Identity, SystemIdentityArgs, SystemIdentitySpec, provide_identity}, + system_identity::{SystemIdentityArgs, SystemIdentitySpec, provide_identity}, }, on_host::config_gen::config::{ AuthConfig, Config, FleetControl, LogConfig, Server, SignatureValidation, }, }; use fs::file::{LocalFile, writer::FileWriter}; +use nr_auth::key::{ + creator::KeyType, + local::{KeyPairGeneratorLocalConfig, LocalCreator}, +}; use std::{collections::HashMap, path::PathBuf}; use tracing::info; @@ -187,15 +191,27 @@ fn generate_config_and_system_identity( provide_identity_fn: F, ) -> Result where - F: Fn(&SystemIdentitySpec, Region, Option) -> Result, + F: Fn( + &SystemIdentitySpec, + Region, + Option, + LocalCreator, + ) -> Result, { let fleet_control = match ¶ms.fleet { FleetParams::FleetDisabled => None, FleetParams::FleetEnabled { fleet_id, identity } => { - let Identity { - client_id, - private_key_path, - } = provide_identity_fn(identity, params.region, params.proxy_config.clone())?; + let key_creator = LocalCreator::from(KeyPairGeneratorLocalConfig { + key_type: KeyType::Rsa4096, + file_path: identity.private_key_path.clone(), + }); + + let client_id = provide_identity_fn( + identity, + params.region, + params.proxy_config.clone(), + key_creator, + )?; Some(FleetControl { endpoint: params.region.opamp_endpoint().to_string(), @@ -207,7 +223,7 @@ where token_url: params.region.token_renewal_endpoint().to_string(), client_id, provider: "local".to_string(), - private_key_path: private_key_path.to_string_lossy().to_string(), + private_key_path: identity.private_key_path.to_string_lossy().to_string(), }, }) } @@ -387,11 +403,9 @@ mod tests { _: &SystemIdentitySpec, _: Region, _: Option, - ) -> Result { - Ok(Identity { - client_id: "test-client-id".to_string(), - private_key_path: PathBuf::from("/path/to/private/key"), - }) + _: LocalCreator, + ) -> Result { + Ok("test-client-id".to_string()) } fn create_test_args( From 92aca062658404226bfdcdb6e86c5430de10293c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Felipe=20=C3=81lvarez?= Date: Wed, 1 Apr 2026 15:53:39 +0200 Subject: [PATCH 4/8] chore: improve name and avoid type-meta repetition --- agent-control/src/agent_control/config.rs | 12 ++++++++---- agent-control/src/cli/k8s/system_identity.rs | 14 +++----------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/agent-control/src/agent_control/config.rs b/agent-control/src/agent_control/config.rs index 735a9a4af5..8813a29155 100644 --- a/agent-control/src/agent_control/config.rs +++ b/agent-control/src/agent_control/config.rs @@ -391,6 +391,13 @@ pub fn deployment_type_meta() -> TypeMeta { } } +pub fn secret_type_meta() -> TypeMeta { + TypeMeta { + api_version: "v1".to_string(), + kind: "Secret".to_string(), + } +} + pub fn default_group_version_kinds() -> Vec { // In flux health check we are currently supporting just a single helm_release_type_meta // Each time we support a new version we should decide if and how to support retrieving its health @@ -400,10 +407,7 @@ pub fn default_group_version_kinds() -> Vec { instrumentation_v1beta3_type_meta(), // This allows Secrets created as dynamic objects to be cleaned up by the GC // This should not be needed anymore whenever the GC detection logic doesn't rely on this list. - TypeMeta { - api_version: "v1".to_string(), - kind: "Secret".to_string(), - }, + secret_type_meta(), helmrepository_type_meta(), helmrelease_v2_type_meta(), ] diff --git a/agent-control/src/cli/k8s/system_identity.rs b/agent-control/src/cli/k8s/system_identity.rs index 21aa43b902..6d4d60740a 100644 --- a/agent-control/src/cli/k8s/system_identity.rs +++ b/agent-control/src/cli/k8s/system_identity.rs @@ -1,7 +1,6 @@ use std::convert::Infallible; -use k8s_openapi::{Resource as _, api::core::v1::Secret}; -use kube::api::{DynamicObject, ObjectMeta, TypeMeta}; +use kube::api::{DynamicObject, ObjectMeta}; use nr_auth::key::{ creator::{Creator, KeyPair, KeyType, PublicKeyPem}, rsa::rsa, @@ -12,7 +11,7 @@ use tracing::info; #[cfg_attr(test, mockall_double::double)] use crate::k8s::client::SyncK8sClient; use crate::{ - agent_control::agent_id::AgentID, + agent_control::{agent_id::AgentID, config::secret_type_meta}, cli::{ common::{ error::CliError, @@ -167,7 +166,7 @@ fn secret_already_exists( .map(|s| s.is_some()) .map_err(|err| { K8sCliError::GetResource(format!( - "failed to check if the system identity secret exists: {err}" + "failure checking if the system identity secret exists: {err}" )) }) } @@ -188,13 +187,6 @@ fn secret_dynamic_object(object_key: K8sObjectKey<'_>, data: serde_json::Value) } } -fn secret_type_meta() -> TypeMeta { - TypeMeta { - api_version: Secret::API_VERSION.to_string(), - kind: Secret::KIND.to_string(), - } -} - /// Helper struct to hold a public key an use it as [Creator] for System Identity provisioning. struct PublicKeyHolder { public_key: PublicKeyPem, From 2a6a5891e0c5b90ad56668c44b08c191458a2a2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Felipe=20=C3=81lvarez?= Date: Wed, 1 Apr 2026 16:05:14 +0200 Subject: [PATCH 5/8] fix: typo --- agent-control/src/cli/on_host/config_gen.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/agent-control/src/cli/on_host/config_gen.rs b/agent-control/src/cli/on_host/config_gen.rs index 0a4a082f39..5fe5e1661a 100644 --- a/agent-control/src/cli/on_host/config_gen.rs +++ b/agent-control/src/cli/on_host/config_gen.rs @@ -463,7 +463,7 @@ fleet_control: token_url: https://system-identity-oauth.service.newrelic.com/oauth2/token client_id: test-client-id provider: local - private_key_path: /path/to/private/key + private_key_path: /path/to/key "#; @@ -477,7 +477,7 @@ fleet_control: token_url: https://system-identity-oauth.service.newrelic.com/oauth2/token client_id: test-client-id provider: local - private_key_path: /path/to/private/key + private_key_path: /path/to/key "#; @@ -491,7 +491,7 @@ fleet_control: token_url: https://system-identity-oauth.staging-service.newrelic.com/oauth2/token client_id: test-client-id provider: local - private_key_path: /path/to/private/key + private_key_path: /path/to/key "#; From a66566c34b0e26fd411efa952a0b5d59c2f572b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Felipe=20=C3=81lvarez?= Date: Thu, 2 Apr 2026 17:06:52 +0200 Subject: [PATCH 6/8] chore: bump newrelic-auth-rs --- Cargo.lock | 4 ++-- agent-control/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0d8e459bd1..0b0f4752e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2957,8 +2957,8 @@ checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be" [[package]] name = "nr-auth" -version = "0.4.0" -source = "git+https://github.com/newrelic/newrelic-auth-rs.git?branch=main#50ce9271a004756a3a3f91cf0df79e18b77b64ca" +version = "0.3.0" +source = "git+https://github.com/newrelic/newrelic-auth-rs.git?tag=0.3.0#b4773eb96fbead5071e3b9acd792461ed7c8cf81" dependencies = [ "base64 0.22.1", "chrono", diff --git a/agent-control/Cargo.toml b/agent-control/Cargo.toml index 415a8b6bf0..e0e737427b 100644 --- a/agent-control/Cargo.toml +++ b/agent-control/Cargo.toml @@ -32,7 +32,7 @@ semver = { workspace = true, features = ["serde"] } chrono = { workspace = true } base64 = { version = "0.22.1" } # New Relic dependencies (external repos) -nr-auth = { git = "https://github.com/newrelic/newrelic-auth-rs.git", branch = "main" } # TODO: revert back to tag when ready +nr-auth = { git = "https://github.com/newrelic/newrelic-auth-rs.git", tag = "0.3.0" } opamp-client = { git = "https://github.com/newrelic/newrelic-opamp-rs.git", tag = "0.0.36" } # local dependencies fs = { path = "../fs" } From 81bbdfe089568f82aadb77e4b8699fd3953b2c11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Felipe=20=C3=81lvarez?= Date: Thu, 2 Apr 2026 17:12:50 +0200 Subject: [PATCH 7/8] fix: typo --- agent-control/src/cli/common/system_identity.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent-control/src/cli/common/system_identity.rs b/agent-control/src/cli/common/system_identity.rs index e2104411de..476e7190e1 100644 --- a/agent-control/src/cli/common/system_identity.rs +++ b/agent-control/src/cli/common/system_identity.rs @@ -270,7 +270,7 @@ fn build_identity_from_token( let iam_client = HttpIAMClient::new(http_client, system_identity_creation_metadata.to_owned()); let system_identity_generator = L2SystemIdentityGenerator { - iam_client, + iam_client: &iam_client, key_creator, }; From 05c81d07a252d239fa332eac783942be3145012d Mon Sep 17 00:00:00 2001 From: Christian Date: Mon, 13 Apr 2026 17:36:12 +0200 Subject: [PATCH 8/8] chore: bump newrelic-auth-rs (#2399) --- Cargo.lock | 4 +- agent-control/Cargo.toml | 2 +- .../src/cli/common/system_identity.rs | 257 ++++-------------- agent-control/src/cli/k8s/system_identity.rs | 114 ++++---- agent-control/src/cli/on_host/config_gen.rs | 184 ++++++++++--- 5 files changed, 245 insertions(+), 316 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0b0f4752e9..a7a129194c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2957,8 +2957,8 @@ checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be" [[package]] name = "nr-auth" -version = "0.3.0" -source = "git+https://github.com/newrelic/newrelic-auth-rs.git?tag=0.3.0#b4773eb96fbead5071e3b9acd792461ed7c8cf81" +version = "0.4.0" +source = "git+https://github.com/newrelic/newrelic-auth-rs.git?tag=0.4.0#2d936f25f6950dbec6de2ab39d3b1654711862af" dependencies = [ "base64 0.22.1", "chrono", diff --git a/agent-control/Cargo.toml b/agent-control/Cargo.toml index e0e737427b..2ae930a2e8 100644 --- a/agent-control/Cargo.toml +++ b/agent-control/Cargo.toml @@ -32,7 +32,7 @@ semver = { workspace = true, features = ["serde"] } chrono = { workspace = true } base64 = { version = "0.22.1" } # New Relic dependencies (external repos) -nr-auth = { git = "https://github.com/newrelic/newrelic-auth-rs.git", tag = "0.3.0" } +nr-auth = { git = "https://github.com/newrelic/newrelic-auth-rs.git", tag = "0.4.0" } opamp-client = { git = "https://github.com/newrelic/newrelic-opamp-rs.git", tag = "0.0.36" } # local dependencies fs = { path = "../fs" } diff --git a/agent-control/src/cli/common/system_identity.rs b/agent-control/src/cli/common/system_identity.rs index 476e7190e1..383c75ba77 100644 --- a/agent-control/src/cli/common/system_identity.rs +++ b/agent-control/src/cli/common/system_identity.rs @@ -1,18 +1,15 @@ //! This module provides the functions to handle identity creation when setting up Agent Control. //! -use std::{ - path::{Path, PathBuf}, - time::Duration, -}; +use std::{path::PathBuf, time::Duration}; use nr_auth::{ TokenRetriever, authenticator::HttpAuthenticator, http::{client::HttpClient, config::HttpConfig}, - key::creator::Creator, + key::generation::PublicKeyPem, system_identity::{ - generator::L2SystemIdentityGenerator, iam_client::http::{HttpIAMClient, IAMAuthCredential}, + identity_creator::L2IdentityCreator, input_data::{SystemIdentityCreationMetadata, environment::NewRelicEnvironment}, }, token::{Token, TokenType}, @@ -25,7 +22,7 @@ use crate::cli::common::{error::CliError, proxy_config::ProxyConfig, region::Reg const DEFAULT_TIMEOUT: Duration = Duration::from_secs(3); const DEFAULT_RETRIES: u8 = 3; -/// Arguments required to provide or generate a system identity. +/// Arguments required to provision system identity. /// /// **Design note:** modelling the different provisioning methods as subcommands would be /// a more natural fit for clap (each subcommand carrying only its own required fields), @@ -37,16 +34,7 @@ const DEFAULT_RETRIES: u8 = 3; /// credentials, or accept a pre-obtained token) lives here rather than being duplicated /// in each installer. #[derive(Debug, Default, clap::Args)] -pub struct SystemIdentityArgs { - /// Path where the private key is stored or will be written. - #[arg(long)] - pub auth_private_key_path: Option, - - /// Client ID of an already-provisioned system identity. When non-empty, no identity - /// generation is performed. - #[arg(long, default_value_t)] - pub auth_client_id: String, - +pub struct ProvisionIdentityArgs { /// Client ID of the parent system identity, used to obtain a token or as metadata /// during identity generation. #[arg(long, default_value_t)] @@ -66,11 +54,9 @@ pub struct SystemIdentityArgs { pub organization_id: String, } +/// Defines the supported provisioning methods for System Identities #[derive(Debug)] -pub enum ProvisioningMethod { - ExistingIdentity { - auth_client_id: String, - }, +pub enum ParentAuthMethod { ParentToken { token: String, organization_id: String, @@ -82,49 +68,11 @@ pub enum ProvisioningMethod { }, } -/// Valid data to create a SystemIdentity, represent [SystemIdentityArgs] after validation. -#[derive(Debug)] -pub struct SystemIdentitySpec { - pub method: ProvisioningMethod, - pub private_key_path: PathBuf, -} - -impl SystemIdentityArgs { - /// Performs additional args validation (not covered by clap's arguments) and returns [SystemIdentityData] if - /// validation was Ok. - pub fn validate(self) -> Result { - let private_key_path = self - .auth_private_key_path - .as_ref() - .ok_or_else(|| { - "'auth_private_key_path' needs to be set to register System Identity".to_string() - })? - .clone(); - - let method = self.resolve_provisioning_method(&private_key_path)?; - - Ok(SystemIdentitySpec { - method, - private_key_path, - }) - } - - fn resolve_provisioning_method(self, key_path: &Path) -> Result { - if !self.auth_client_id.is_empty() { - if !key_path.exists() { - return Err( - "when 'auth_client_id' is provided the 'auth_private_key_path' must also be provided and exist" - .to_string(), - ); - } - return Ok(ProvisioningMethod::ExistingIdentity { - auth_client_id: self.auth_client_id, - }); - } - +impl ProvisionIdentityArgs { + pub fn validate(self) -> Result { if !self.auth_parent_token.is_empty() { self.require_org_and_parent_id("token based")?; - return Ok(ProvisioningMethod::ParentToken { + return Ok(ParentAuthMethod::ParentToken { token: self.auth_parent_token, organization_id: self.organization_id, }); @@ -132,15 +80,14 @@ impl SystemIdentityArgs { if !self.auth_parent_client_secret.is_empty() { self.require_org_and_parent_id("client-secret based")?; - return Ok(ProvisioningMethod::ParentSecret { + return Ok(ParentAuthMethod::ParentSecret { secret: self.auth_parent_client_secret, parent_client_id: self.auth_parent_client_id, organization_id: self.organization_id, }); } - Err( - "either 'auth_client_id', 'auth_parent_token' or 'auth_parent_secret' should be set to register System Identity" + "either 'auth_parent_token' or 'auth_parent_secret' should be set to create a System Identity" .to_string(), ) } @@ -155,30 +102,26 @@ impl SystemIdentityArgs { } } -/// Provides a key-based identity considering the supplied args. It returns the corresponding **client_id** as a String. -pub fn provide_identity( - identity_input: &SystemIdentitySpec, +/// Creates a key-based identity considering the supplied args. It returns the corresponding **client_id** as a String. +pub fn create_identity( + parent_auth_method: &ParentAuthMethod, region: Region, proxy_config: Option, - key_creator: impl Creator, + pub_key: PublicKeyPem, ) -> Result { let environment = NewRelicEnvironment::from(region); - build_identity(identity_input, environment, proxy_config, key_creator) + build_identity(parent_auth_method, environment, proxy_config, pub_key) } /// Helper to allow injecting testing urls when building the identity. fn build_identity( - identity_input: &SystemIdentitySpec, + parent_auth_method: &ParentAuthMethod, environment: NewRelicEnvironment, proxy_config: Option, - key_creator: impl Creator, + pub_key: PublicKeyPem, ) -> Result { - match &identity_input.method { - ProvisioningMethod::ExistingIdentity { auth_client_id } => { - info!("Using provided System Identity"); - Ok(auth_client_id.to_string()) - } - ProvisioningMethod::ParentToken { + match parent_auth_method { + ParentAuthMethod::ParentToken { token, organization_id, } => { @@ -189,10 +132,10 @@ fn build_identity( organization_id.clone(), environment, http_client, - key_creator, + pub_key, ) } - ProvisioningMethod::ParentSecret { + ParentAuthMethod::ParentSecret { secret, parent_client_id, organization_id, @@ -209,7 +152,7 @@ fn build_identity( organization_id.clone(), environment, http_client, - key_creator, + pub_key, ) } } @@ -241,12 +184,9 @@ fn get_auth_token( let authenticator = HttpAuthenticator::new(http_client.clone(), environment.token_renewal_endpoint()); - let token_retriever = TokenRetrieverWithCache::new_with_secret( - parent_client_id.clone(), - authenticator, - secret.into(), - ) - .with_retries(DEFAULT_RETRIES); + let token_retriever = + TokenRetrieverWithCache::new_with_secret(parent_client_id, authenticator, secret.into()) + .with_retries(DEFAULT_RETRIES); token_retriever.retrieve().map_err(|err| { CliError::Command(format!( @@ -260,7 +200,7 @@ fn build_identity_from_token( organization_id: String, environment: NewRelicEnvironment, http_client: HttpClient, - key_creator: impl Creator, + pub_key: PublicKeyPem, ) -> Result { let system_identity_creation_metadata = SystemIdentityCreationMetadata { organization_id: organization_id.clone(), @@ -269,15 +209,10 @@ fn build_identity_from_token( }; let iam_client = HttpIAMClient::new(http_client, system_identity_creation_metadata.to_owned()); - let system_identity_generator = L2SystemIdentityGenerator { - iam_client: &iam_client, - key_creator, - }; - - let auth_credential = IAMAuthCredential::BearerToken(token.access_token().to_string()); + let auth_credentials = IAMAuthCredential::BearerToken(token.access_token().to_string()); - let result = system_identity_generator - .generate(&auth_credential) + let result = iam_client + .create_l2_system_identity(&auth_credentials, &pub_key) .map_err(|err| CliError::Command(format!("error generating the system identity: {err}")))?; Ok(result.client_id) @@ -308,145 +243,60 @@ pub mod tests { use assert_matches::assert_matches; use http::header::AUTHORIZATION; use httpmock::{Method::POST, MockServer}; - use mockall::mock; - use nr_auth::key::creator::PublicKeyPem; use rstest::rstest; - use std::fs; - use tempfile::TempDir; - use thiserror::Error; use super::*; - #[derive(Debug, Error)] - #[error("{0}")] - pub struct TestCreatorError(String); - - mock! { - pub Creator {} - impl Creator for Creator { - type Error = TestCreatorError; - fn create(&self) -> Result; - } - } - impl MockCreator { - fn expecting_create_ok() -> Self { - let mut creator = Self::new(); - creator - .expect_create() - .once() - .returning(|| Ok(PublicKeyPem::default())); - - creator - } - } - #[rstest] - #[case::existing_identity(|| SystemIdentityArgs { - auth_private_key_path: Some(std::env::current_dir().unwrap()), - auth_client_id: "some-client-id".to_string(), - ..Default::default() - })] - #[case::parent_token(|| SystemIdentityArgs { - auth_private_key_path: Some(PathBuf::from("/some/path")), + #[case::parent_token(ProvisionIdentityArgs { auth_parent_token: "TOKEN".to_string(), auth_parent_client_id: "parent-id".to_string(), organization_id: "org-id".to_string(), ..Default::default() })] - #[case::parent_secret(|| SystemIdentityArgs { - auth_private_key_path: Some(PathBuf::from("/some/path")), + #[case::parent_secret(ProvisionIdentityArgs { auth_parent_client_secret: "SECRET".to_string(), auth_parent_client_id: "parent-id".to_string(), organization_id: "org-id".to_string(), ..Default::default() })] - fn test_validate(#[case] make_args: fn() -> SystemIdentityArgs) { - assert_matches!(make_args().validate(), Ok(_)); + fn test_validate(#[case] make_args: ProvisionIdentityArgs) { + assert_matches!(make_args.validate(), Ok(_)); } #[rstest] - #[case::missing_private_key_path(|| SystemIdentityArgs { - auth_client_id: "some-client-id".to_string(), - ..Default::default() - })] - #[case::nonexistent_key_with_client_id(|| SystemIdentityArgs { - auth_private_key_path: Some(PathBuf::from("/does/not/exist")), - auth_client_id: "some-client-id".to_string(), - ..Default::default() - })] - #[case::token_missing_org_id(|| SystemIdentityArgs { - auth_private_key_path: Some(PathBuf::from("/some/path")), + #[case::token_missing_org_id(|| ProvisionIdentityArgs { auth_parent_token: "TOKEN".to_string(), auth_parent_client_id: "parent-id".to_string(), ..Default::default() })] - #[case::token_missing_parent_client_id(|| SystemIdentityArgs { - auth_private_key_path: Some(PathBuf::from("/some/path")), + #[case::token_missing_parent_client_id(|| ProvisionIdentityArgs { auth_parent_token: "TOKEN".to_string(), organization_id: "org-id".to_string(), ..Default::default() })] - #[case::secret_missing_org_id(|| SystemIdentityArgs { - auth_private_key_path: Some(PathBuf::from("/some/path")), + #[case::secret_missing_org_id(|| ProvisionIdentityArgs { auth_parent_client_secret: "SECRET".to_string(), auth_parent_client_id: "parent-id".to_string(), ..Default::default() })] - #[case::secret_missing_parent_client_id(|| SystemIdentityArgs { - auth_private_key_path: Some(PathBuf::from("/some/path")), + #[case::secret_missing_parent_client_id(|| ProvisionIdentityArgs { auth_parent_client_secret: "SECRET".to_string(), organization_id: "org-id".to_string(), ..Default::default() })] - #[case::no_method_provided(|| SystemIdentityArgs { - auth_private_key_path: Some(PathBuf::from("/some/path")), - ..Default::default() - })] - fn test_validate_errors(#[case] make_args: fn() -> SystemIdentityArgs) { + #[case::no_method_provided(|| ProvisionIdentityArgs::default())] + fn test_validate_errors(#[case] make_args: fn() -> ProvisionIdentityArgs) { assert_matches!(make_args().validate(), Err(_)); } - #[test] - fn test_build_identity_already_provided() { - let tempdir = TempDir::new().unwrap(); - let auth_private_key_path = tempdir.path().join("private-key"); - // Expect no request because the identity was already provided - let environment = NewRelicEnvironment::Custom { - token_renewal_endpoint: "https://should-not-call.this" - .parse() - .expect("url should be valid"), - system_identity_creation_uri: "https://should-not-call.this" - .parse() - .expect("url should be valid"), - }; - // Key file must exist when using ExistingIdentity - fs::write(&auth_private_key_path, "").unwrap(); - let identity_args = SystemIdentityArgs { - auth_private_key_path: Some(auth_private_key_path.clone()), - auth_client_id: "provided_client_id".to_string(), - ..Default::default() - }; - let identity_data = identity_args.validate().expect("validation should succeed"); - - let mut creator_mock = MockCreator::new(); - creator_mock.expect_create().never(); - - let client_id = build_identity(&identity_data, environment, None, creator_mock) - .expect("no error expected"); - assert_eq!(client_id, "provided_client_id".to_string()); - } - #[test] fn test_build_identity_with_token() { - let tempdir = TempDir::new().unwrap(); - let auth_private_key_path = tempdir.path().join("private-key"); - let server = MockServer::start(); - let identity_args = SystemIdentityArgs { + let identity_args = ProvisionIdentityArgs { auth_parent_client_id: "parent-client-id".to_string(), auth_parent_token: "TOKEN".to_string(), - auth_private_key_path: Some(auth_private_key_path.clone()), organization_id: "test-org-id".to_string(), ..Default::default() }; @@ -469,26 +319,23 @@ pub mod tests { .expect("url should be valid"), }; - let creator_mock = MockCreator::expecting_create_ok(); - let identity_data = identity_args.validate().expect("validation should succeed"); - let client_id = build_identity(&identity_data, environment, None, creator_mock) + let method = identity_args.validate().expect("validation should succeed"); + let pub_key = b"mock-pub-key"; + + let client_id = build_identity(&method, environment, None, pub_key.to_vec()) .expect("no error expected"); + assert_eq!(client_id, "created_client_id".to_string()); identity_mock.assert_calls(1); - assert_eq!(client_id, "created_client_id".to_string()); } #[test] fn test_build_identity_client_secret() { - let tempdir = TempDir::new().unwrap(); - let auth_private_key_path = tempdir.path().join("private-key"); - let server = MockServer::start(); - let identity_args = SystemIdentityArgs { + let identity_args = ProvisionIdentityArgs { auth_parent_client_id: "parent-client-id".to_string(), auth_parent_client_secret: "client-secret-value".to_string(), - auth_private_key_path: Some(auth_private_key_path.clone()), organization_id: "test-org-id".to_string(), ..Default::default() }; @@ -519,14 +366,14 @@ pub mod tests { .expect("url should be valid"), }; - let creator_mock = MockCreator::expecting_create_ok(); - let identity_data = identity_args.validate().expect("validation should succeed"); - let client_id = build_identity(&identity_data, environment, None, creator_mock) + let method = identity_args.validate().expect("validation should succeed"); + let pub_key = b"mock-public-key"; + let client_id = build_identity(&method, environment, None, pub_key.to_vec()) .expect("no error expected"); + assert_eq!(client_id, "created_client_id".to_string()); identity_mock.assert_calls(1); token_mock.assert_calls(1); - assert_eq!(client_id, "created_client_id".to_string()); } fn token_body(token: &str) -> String { diff --git a/agent-control/src/cli/k8s/system_identity.rs b/agent-control/src/cli/k8s/system_identity.rs index 6d4d60740a..477bca41a2 100644 --- a/agent-control/src/cli/k8s/system_identity.rs +++ b/agent-control/src/cli/k8s/system_identity.rs @@ -1,8 +1,6 @@ -use std::convert::Infallible; - use kube::api::{DynamicObject, ObjectMeta}; use nr_auth::key::{ - creator::{Creator, KeyPair, KeyType, PublicKeyPem}, + generation::{KeyPair, KeyType, PublicKeyPem}, rsa::rsa, }; use serde_json::json; @@ -17,7 +15,7 @@ use crate::{ error::CliError, proxy_config::ProxyConfig, region::{Region, region_parser}, - system_identity::{SystemIdentityArgs, SystemIdentitySpec, provide_identity}, + system_identity::{ParentAuthMethod, ProvisionIdentityArgs, create_identity}, }, k8s::{errors::K8sCliError, utils::try_new_k8s_client}, }, @@ -40,7 +38,7 @@ pub struct Args { /// Identity configuration #[command(flatten)] - identity: SystemIdentityArgs, + identity: ProvisionIdentityArgs, /// Proxy configuration #[command(flatten)] @@ -52,7 +50,7 @@ pub struct Args { pub struct IdentityRegistrationSpec { secret_name: String, region: Region, - identity: SystemIdentitySpec, + identity_provisioning_method: ParentAuthMethod, proxy_config: Option, } @@ -67,7 +65,7 @@ impl Args { Ok(IdentityRegistrationSpec { secret_name: self.secret_name, region: self.region, - identity: self.identity.validate()?, + identity_provisioning_method: self.identity.validate()?, proxy_config: self.proxy_config, }) } @@ -79,7 +77,7 @@ pub fn register_system_identity( spec: IdentityRegistrationSpec, ) -> Result<(), K8sCliError> { let k8s_client = try_new_k8s_client()?; - provide_system_identity_secret(namespace, spec, &k8s_client, provide_identity) + provide_system_identity_secret(namespace, spec, &k8s_client, create_identity) } /// Helper function to implement [register_system_identity] while allowing the usage of mocks for the k8s client @@ -90,15 +88,10 @@ fn provide_system_identity_secret( namespace: &str, spec: IdentityRegistrationSpec, k8s_client: &SyncK8sClient, - provide_identity_fn: F, + create_identity: F, ) -> Result<(), K8sCliError> where - F: Fn( - &SystemIdentitySpec, - Region, - Option, - PublicKeyHolder, - ) -> Result, + F: Fn(&ParentAuthMethod, Region, Option, PublicKeyPem) -> Result, { let secret_object_key = K8sObjectKey { name: &spec.secret_name, @@ -123,12 +116,16 @@ where "failure building key-pair for System Identity: {err}" )) })?; - let pk_holder = PublicKeyHolder { public_key }; - let client_id = provide_identity_fn(&spec.identity, spec.region, spec.proxy_config, pk_holder) - .map_err(|err| { - K8sCliError::Generic(format!("failure registering the System Identity: {err}")) - })?; + let client_id = create_identity( + &spec.identity_provisioning_method, + spec.region, + spec.proxy_config, + public_key, + ) + .map_err(|err| { + K8sCliError::Generic(format!("failure registering the System Identity: {err}")) + })?; let private_key = String::from_utf8(private_key).map_err(|err| { K8sCliError::Generic(format!( @@ -187,39 +184,25 @@ fn secret_dynamic_object(object_key: K8sObjectKey<'_>, data: serde_json::Value) } } -/// Helper struct to hold a public key an use it as [Creator] for System Identity provisioning. -struct PublicKeyHolder { - public_key: PublicKeyPem, -} - -impl Creator for PublicKeyHolder { - type Error = Infallible; - - fn create(&self) -> Result { - Ok(self.public_key.clone()) - } -} - #[cfg(test)] mod tests { use super::*; - use crate::cli::common::system_identity::ProvisioningMethod; + use crate::cli::common::system_identity::ParentAuthMethod; use crate::k8s::client::MockSyncK8sClient; use assert_matches::assert_matches; use clap::{CommandFactory, FromArgMatches}; use rstest::rstest; - use std::{env::current_dir, path::PathBuf, sync::Arc}; + use std::sync::Arc; impl Default for IdentityRegistrationSpec { fn default() -> Self { IdentityRegistrationSpec { secret_name: "test-secret".to_string(), region: Region::US, - identity: SystemIdentitySpec { - method: ProvisioningMethod::ExistingIdentity { - auth_client_id: "test-client-id".to_string(), - }, - private_key_path: PathBuf::from("/test/key"), + identity_provisioning_method: ParentAuthMethod::ParentSecret { + secret: "secret".to_string(), + parent_client_id: "parent_client_id".to_string(), + organization_id: "org_id".to_string(), }, proxy_config: None, } @@ -227,44 +210,39 @@ mod tests { } #[rstest] - #[case::token_based_identity( - || String::from("--secret-name s --region us --auth-private-key-path /some/path --auth-parent-token TOKEN --auth-parent-client-id id --organization-id org-id") - )] - #[case::client_secret_based_identity( - || String::from("--secret-name s --region us --auth-private-key-path /some/path --auth-parent-client-secret SECRET --auth-parent-client-id id --organization-id org-id") - )] - fn test_args_validation(#[case] args: fn() -> String) { + #[case::token_based_identity(String::from( + "--secret-name s --region us --auth-parent-token TOKEN --auth-parent-client-id id --organization-id org-id" + ))] + #[case::client_secret_based_identity(String::from( + "--secret-name s --region us --auth-parent-client-secret SECRET --auth-parent-client-id id --organization-id org-id" + ))] + fn test_args_validation(#[case] args: String) { let cmd = Args::command().no_binary_name(true); let matches = cmd - .try_get_matches_from(args().split_ascii_whitespace()) + .try_get_matches_from(args.split_ascii_whitespace()) .expect("arguments should be valid"); let args = Args::from_arg_matches(&matches).expect("should create the struct back"); assert!(args.validate().is_ok()); } #[rstest] - #[case::missing_private_key_path( - || String::from("--secret-name s --region us --auth-client-id some-id") - )] - #[case::no_identity_method( - || format!("--secret-name s --region us --auth-private-key-path {}", current_dir().unwrap().display()) - )] - #[case::missing_org_id_with_token( - || String::from("--secret-name s --region us --auth-private-key-path /p --auth-parent-token TOKEN --auth-parent-client-id id") - )] - #[case::missing_parent_client_id_with_secret( - || String::from("--secret-name s --region us --auth-private-key-path /p --auth-parent-client-secret SECRET --organization-id org-id") - )] - #[case::missing_org_id_with_secret( - || String::from("--secret-name s --region us --auth-private-key-path /p --auth-parent-client-secret SECRET --auth-parent-client-id id") - )] - #[case::invalid_proxy_config( - || String::from("--secret-name s --region us --auth-private-key-path /p --auth-parent-client-secret SECRET --auth-parent-client-id id --organization-id org-id --proxy-url https::/invalid") - )] - fn test_args_validation_errors(#[case] args: fn() -> String) { + #[case::no_identity_method(String::from("--secret-name s --region us"))] + #[case::missing_org_id_with_token(String::from( + "--secret-name s --region us --auth-parent-token TOKEN --auth-parent-client-id id" + ))] + #[case::missing_parent_client_id_with_secret(String::from( + "--secret-name s --region us --auth-parent-client-secret SECRET --organization-id org-id" + ))] + #[case::missing_org_id_with_secret(String::from( + "--secret-name s --region us --auth-parent-client-secret SECRET --auth-parent-client-id id" + ))] + #[case::invalid_proxy_config(String::from( + "--secret-name s --region us --auth-parent-client-secret SECRET --auth-parent-client-id id --organization-id org-id --proxy-url https::/invalid" + ))] + fn test_args_validation_errors(#[case] args: String) { let cmd = Args::command().no_binary_name(true); let matches = cmd - .try_get_matches_from(args().split_ascii_whitespace()) + .try_get_matches_from(args.split_ascii_whitespace()) .expect("arguments should be valid"); let args = Args::from_arg_matches(&matches).expect("should create the struct back"); assert_matches!(args.validate(), Err(_)); diff --git a/agent-control/src/cli/on_host/config_gen.rs b/agent-control/src/cli/on_host/config_gen.rs index 5fe5e1661a..a05de5f3bd 100644 --- a/agent-control/src/cli/on_host/config_gen.rs +++ b/agent-control/src/cli/on_host/config_gen.rs @@ -4,7 +4,7 @@ use crate::cli::{ error::CliError, proxy_config::ProxyConfig, region::{Region, region_parser}, - system_identity::{SystemIdentityArgs, SystemIdentitySpec, provide_identity}, + system_identity::{ParentAuthMethod, ProvisionIdentityArgs, create_identity}, }, on_host::config_gen::config::{ AuthConfig, Config, FleetControl, LogConfig, Server, SignatureValidation, @@ -12,10 +12,13 @@ use crate::cli::{ }; use fs::file::{LocalFile, writer::FileWriter}; use nr_auth::key::{ - creator::KeyType, - local::{KeyPairGeneratorLocalConfig, LocalCreator}, + generation::{KeyType, PublicKeyPem}, + local::{LocalKeyPairGenerator, LocalKeyPairGeneratorConfig}, +}; +use std::{ + collections::HashMap, + path::{Path, PathBuf}, }; -use std::{collections::HashMap, path::PathBuf}; use tracing::info; pub mod config; @@ -42,9 +45,18 @@ pub struct Args { #[arg(long, default_value_t)] fleet_id: String, + /// Path where the private key is stored or will be written. + #[arg(long)] + auth_private_key_path: Option, + + /// Client ID of an already-provisioned system identity. When non-empty, no identity + /// generation is performed. + #[arg(long, default_value_t)] + auth_client_id: String, + /// Identity configuration #[command(flatten)] - identity: SystemIdentityArgs, + identity: ProvisionIdentityArgs, /// Proxy configuration #[command(flatten)] @@ -59,6 +71,24 @@ pub struct Args { env_vars_file_path: Option, } +/// Valid data to create a SystemIdentity, represent [SystemIdentityArgs] after validation. +#[derive(Debug)] +pub struct SystemIdentitySpec { + /// Data to get or create the System Identity to be used by Agent Control + system_identity_data: SystemIdentityData, + /// Path where the corresponding private key needs to be read from or written to + private_key_path: PathBuf, +} + +/// Defines whether a SystemIdentity already exists or needs to be provisioned +#[derive(Debug)] +pub enum SystemIdentityData { + /// The Identity already exists + Existing { auth_client_id: String }, + /// The identity needs to be provisioned + Provision(ParentAuthMethod), +} + /// Represents fleet parameters to generate configuration depending of it its enabled or not. #[derive(Debug)] pub enum FleetParams { @@ -90,9 +120,25 @@ impl Args { if self.fleet_id.is_empty() { return Err(String::from("'fleet_id' should be set when enabling fleet")); } + let private_key_path = self + .auth_private_key_path + .as_ref() + .ok_or_else(|| { + "'auth_private_key_path' needs to be set to register System Identity" + .to_string() + })? + .clone(); + FleetParams::FleetEnabled { fleet_id: self.fleet_id, - identity: self.identity.validate()?, + identity: SystemIdentitySpec { + system_identity_data: Self::identity_data( + &private_key_path, + self.auth_client_id, + self.identity, + )?, + private_key_path, + }, } }; if let Some(proxy_config) = self.proxy_config.clone() @@ -109,6 +155,24 @@ impl Args { fleet: fleet_inputs, }) } + + /// Helper to build [SystemIdentityData] from args + fn identity_data( + private_key_path: &Path, + auth_client_id: String, + identity_args: ProvisionIdentityArgs, + ) -> Result { + if !auth_client_id.is_empty() { + if !private_key_path.exists() { + return Err( + "when 'auth_client_id' is provided, 'auth_private_key_path' must exist" + .to_string(), + ); + } + return Ok(SystemIdentityData::Existing { auth_client_id }); + } + Ok(SystemIdentityData::Provision(identity_args.validate()?)) + } } /// Generates: @@ -125,7 +189,7 @@ pub fn generate(params: Params) -> Result<(), CliError> { fn write_config_and_generate_system_identity(params: &Params) -> Result<(), CliError> { info!("Generating Agent Control configuration"); - let yaml = generate_config_and_system_identity(params, provide_identity)?; + let yaml = generate_config_and_system_identity(params, create_identity)?; LocalFile.write(¶ms.output_path, yaml).map_err(|err| { CliError::Command(format!( @@ -188,30 +252,39 @@ fn generate_env_var_config(params: &Params) -> Result { /// Generates the configuration according to args using the provided function to generate the identity. fn generate_config_and_system_identity( params: &Params, - provide_identity_fn: F, + create_identity: F, ) -> Result where - F: Fn( - &SystemIdentitySpec, - Region, - Option, - LocalCreator, - ) -> Result, + F: Fn(&ParentAuthMethod, Region, Option, PublicKeyPem) -> Result, { let fleet_control = match ¶ms.fleet { FleetParams::FleetDisabled => None, - FleetParams::FleetEnabled { fleet_id, identity } => { - let key_creator = LocalCreator::from(KeyPairGeneratorLocalConfig { - key_type: KeyType::Rsa4096, - file_path: identity.private_key_path.clone(), - }); - - let client_id = provide_identity_fn( - identity, - params.region, - params.proxy_config.clone(), - key_creator, - )?; + FleetParams::FleetEnabled { + fleet_id, + identity: identity_spec, + } => { + let client_id = match &identity_spec.system_identity_data { + SystemIdentityData::Existing { auth_client_id } => auth_client_id.to_string(), + SystemIdentityData::Provision(parent_auth_method) => { + let pub_key = LocalKeyPairGenerator::from(LocalKeyPairGeneratorConfig { + key_type: KeyType::Rsa4096, + file_path: identity_spec.private_key_path.clone(), + }) + .generate() + .map_err(|err| { + CliError::Command(format!( + "could not generate System Identity's key-pair; {err}" + )) + })?; + + create_identity( + parent_auth_method, + params.region, + params.proxy_config.clone(), + pub_key, + )? + } + }; Some(FleetControl { endpoint: params.region.opamp_endpoint().to_string(), @@ -223,7 +296,7 @@ where token_url: params.region.token_renewal_endpoint().to_string(), client_id, provider: "local".to_string(), - private_key_path: identity.private_key_path.to_string_lossy().to_string(), + private_key_path: identity_spec.private_key_path.to_string_lossy().to_string(), }, }) } @@ -257,11 +330,12 @@ fn default_log_config() -> Option { mod tests { use super::*; use crate::agent_control::config::AgentControlConfig; - use crate::cli::common::system_identity::ProvisioningMethod; + use crate::cli::common::system_identity::ParentAuthMethod; use assert_matches::assert_matches; use clap::{CommandFactory, FromArgMatches}; use rstest::rstest; use std::env::current_dir; + use tempfile::tempdir; impl Default for Params { fn default() -> Self { @@ -280,9 +354,6 @@ mod tests { #[case::fleet_disabled( || String::from("--fleet-disabled --output-path /some/path --region us") )] - #[case::identity_already_provided( - || format!("--output-path /some/path --region us --fleet-id some-id --auth-private-key-path {} --auth-client-id some-client-id", pwd()) - )] #[case::token_based_identity( || format!("--output-path /some/path --region us --fleet-id some-id --auth-private-key-path {} --auth-parent-token TOKEN --auth-parent-client-id id --organization-id org-id", pwd()) )] @@ -298,6 +369,27 @@ mod tests { assert_matches!(args.validate(), Ok(_)); } + #[test] + fn test_identity_already_provided() { + let args_definition = format!( + "--output-path /some/path --region us --fleet-id some-id --auth-private-key-path {} --auth-client-id some-client-id", + pwd() + ); + let cmd = Args::command().no_binary_name(true); + let matches = cmd + .try_get_matches_from(args_definition.split_ascii_whitespace()) + .expect("arguments should be valid"); + let args = Args::from_arg_matches(&matches).expect("should create the struct back"); + assert_matches!(args.validate(), Ok(params) => { + assert_matches!(params.fleet, FleetParams::FleetEnabled { fleet_id, identity } => { + assert_eq!(fleet_id, "some-id".to_string()); + assert_matches!(identity.system_identity_data, SystemIdentityData::Existing { auth_client_id } => { + assert_eq!(auth_client_id, "some-client-id".to_string()); + }) + }) + }) + } + #[rstest] #[case::missing_identity_creation_method( || format!("--output-path /some/path --region us --auth-private-key-path {}", pwd()) @@ -345,7 +437,16 @@ mod tests { #[case] proxy_config: Option, #[case] expected: String, ) { - let args = create_test_args(fleet_enabled, region, proxy_config); + let tmp = tempdir().unwrap(); + let private_key_path = tmp.path().join("private_key"); + let args = create_test_args( + fleet_enabled, + region, + proxy_config, + private_key_path.clone(), + ); + // Replacing hardcoded path in expectations because the private-key-path is dynamic + let expected = expected.replace("/path/to/key", &private_key_path.to_string_lossy()); let yaml = generate_config_and_system_identity(&args, identity_provider_mock) .expect("result expected to be OK"); @@ -400,10 +501,10 @@ mod tests { } fn identity_provider_mock( - _: &SystemIdentitySpec, + _: &ParentAuthMethod, _: Region, _: Option, - _: LocalCreator, + _: PublicKeyPem, ) -> Result { Ok("test-client-id".to_string()) } @@ -412,6 +513,7 @@ mod tests { fleet_disabled: bool, region: Region, proxy_config: Option, + private_key_path: PathBuf, ) -> Params { let fleet = if fleet_disabled { FleetParams::FleetDisabled @@ -419,12 +521,14 @@ mod tests { FleetParams::FleetEnabled { fleet_id: "test-fleet-id".to_string(), identity: SystemIdentitySpec { - method: ProvisioningMethod::ParentSecret { - secret: "parent-client-secret".to_string(), - parent_client_id: "parent-client-id".to_string(), - organization_id: "test-org-id".to_string(), - }, - private_key_path: PathBuf::from("/path/to/key"), + system_identity_data: SystemIdentityData::Provision( + ParentAuthMethod::ParentSecret { + secret: "parent-client-secret".to_string(), + parent_client_id: "parent-client-id".to_string(), + organization_id: "test-org-id".to_string(), + }, + ), + private_key_path, }, } };