diff --git a/Cargo.lock b/Cargo.lock index bdbea7ed53..23ffbc1e8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2952,8 +2952,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/THIRD_PARTY_NOTICES.md b/THIRD_PARTY_NOTICES.md index b01c00e4f0..c60a995272 100644 --- a/THIRD_PARTY_NOTICES.md +++ b/THIRD_PARTY_NOTICES.md @@ -478,6 +478,20 @@ Distributed under the following license(s): * MIT +## const-random + +Distributed under the following license(s): + +* MIT +* Apache-2.0 + +## const-random-macro + +Distributed under the following license(s): + +* MIT +* Apache-2.0 + ## const_format Distributed under the following license(s): @@ -588,6 +602,12 @@ Distributed under the following license(s): * MIT * Apache-2.0 +## crunchy + +Distributed under the following license(s): + +* MIT + ## crypto-common Distributed under the following license(s): @@ -714,6 +734,13 @@ Distributed under the following license(s): * MIT * Apache-2.0 +## dlv-list + +Distributed under the following license(s): + +* MIT +* Apache-2.0 + ## downcast Distributed under the following license(s): @@ -1677,6 +1704,12 @@ Distributed under the following license(s): * MIT +## ordered-multimap + +Distributed under the following license(s): + +* MIT + ## parking Distributed under the following license(s): @@ -2028,6 +2061,12 @@ Distributed under the following license(s): * MIT * Apache-2.0 +## rust-ini + +Distributed under the following license(s): + +* MIT + ## rust_decimal Distributed under the following license(s): @@ -2455,6 +2494,12 @@ Distributed under the following license(s): * MIT * Apache-2.0 +## tiny-keccak + +Distributed under the following license(s): + +* CC0-1.0 + ## tinystr Distributed under the following license(s): 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, }, } };