diff --git a/docs/sonic-configdb-integration.md b/docs/sonic-configdb-integration.md index 6dc35db..882f818 100644 --- a/docs/sonic-configdb-integration.md +++ b/docs/sonic-configdb-integration.md @@ -51,7 +51,7 @@ TACPLUS|global src_intf "Management0" # default source interface TACPLUS_SERVER|192.0.2.10 - priority "1" # lower = preferred (failover order) + priority "64" # higher = preferred; range is 1..64 tcp_port "49" timeout "10" # overrides TACPLUS|global.timeout passkey "..." # overrides TACPLUS|global.passkey @@ -59,8 +59,10 @@ TACPLUS_SERVER|192.0.2.10 Each `TACPLUS_SERVER` row becomes one `TacacsPlusServer` in the YANG configuration. Per-server fields fall back to the matching `TACPLUS|global` -field when absent. The synthesized YANG `name` for each server is -`sonic-server--
`. +field when absent. SONiC `priority` values are in the range `1..64`; higher +numbers are preferred and therefore appear earlier in the daemon's failover +order. The synthesized YANG `name` for each server is +`sonic-server-
`. ### Forward-compatible extension keys @@ -68,16 +70,16 @@ Operators and SONiC schema maintainers can experiment with TLS-aware fields ahead of upstream ConfigDB schema work by adding the following keys to `TACPLUS_SERVER|`: -| Key | YANG field | Notes | -|---------------------|---------------------|------------------------------------------------------------------------------------------------| -| `use_tls` | `server-authentication: {}` | Accepts the same boolean forms as `sni_enabled`; also supported on `TACPLUS|global` as a default | -| `domain_name` | `domain-name` | Used as SNI hostname | -| `sni_enabled` | `sni-enabled` | `true`/`false`/`yes`/`no`/`1`/`0` | -| `single_connection` | `single-connection` | Boolean | -| `vrf_name` | `vrf-instance` | VRF name for outbound traffic | -| `src_ip` | `source-ip` | Mutually exclusive with `src_intf` | -| `src_intf` | `source-interface` | Falls back to the global TACPLUS row | -| `server_type` | `server-type` | Defaults to `all`; tokens accept `authentication`, `authorization`, `accounting`, or `all` | +| Key | YANG field | Notes | +|---------------------|-------------------------------|----------------------------------------------------------------------------------------------------| +| `use_tls` | `server-authentication: {}` | Accepts the same boolean forms as `sni_enabled`; also supported on the global TACPLUS row | +| `domain_name` | `domain-name` | Used as SNI hostname | +| `sni_enabled` | `sni-enabled` | `true`/`false`/`yes`/`no`/`1`/`0` | +| `single_connection` | `single-connection` | Boolean | +| `vrf_name` | `vrf-instance` | VRF name for outbound traffic | +| `src_ip` | `source-ip` | Mutually exclusive with `src_intf` | +| `src_intf` | `source-interface` | Falls back to the global TACPLUS row | +| `server_type` | `server-type` | Defaults to `all`; tokens accept `authentication`, `authorization`, `accounting`, or `all` | Unknown fields are logged at `warn` level and ignored, so legacy operator annotations on TACPLUS rows do not break the agent. @@ -202,7 +204,7 @@ done redis-cli -n 4 HSET 'TACPLUS|global' \ timeout 5 passkey shared-secret auth_type pap src_intf Management0 redis-cli -n 4 HSET 'TACPLUS_SERVER|192.0.2.10' \ - priority 1 tcp_port 49 timeout 10 passkey server-secret \ + priority 64 tcp_port 49 timeout 10 passkey server-secret \ domain_name tacacs-a.example.test sni_enabled true single_connection true ``` @@ -220,7 +222,7 @@ the expected delta: ```bash redis-cli -n 4 HSET 'TACPLUS_SERVER|192.0.2.20' \ - priority 2 tcp_port 49 passkey backup-secret + priority 32 tcp_port 49 passkey backup-secret redis-cli -n 4 HSET 'TACPLUS_SERVER|192.0.2.10' timeout 20 redis-cli -n 4 DEL 'TACPLUS_SERVER|192.0.2.20' redis-cli -n 4 HSET 'TACPLUS|global' timeout 7 diff --git a/executables/tacacsrs_agentd/README.md b/executables/tacacsrs_agentd/README.md index b9616b2..6d7d550 100644 --- a/executables/tacacsrs_agentd/README.md +++ b/executables/tacacsrs_agentd/README.md @@ -18,6 +18,19 @@ this, the daemon removes upstream entries whose address and port parse or resolv to the local TCP proxy endpoint, including loopback IPv4, loopback IPv6, and hostnames such as `localhost`. +If one or more local proxy endpoint rows are removed this way, the raw proxy +uses the highest-priority matching row's resolved shared secret for downstream +TACACS+ obfuscation. This includes secrets inherited from global configuration. +If that highest-priority local proxy row has no shared secret, local clients are +expected to send unobfuscated TACACS+ packets to the proxy even when lower- +priority local rows or real upstream servers have shared secrets. + +Outside SONiC mode, use `--proxy-shared-secret` with `--proxy-endpoint` when +raw TACACS+ proxy clients send obfuscated packets to the proxy. If the flag is +omitted, local proxy clients are expected to send unobfuscated packets. A +filtered local proxy endpoint row takes precedence over this CLI fallback if +both are present. + ## TLS Client Certificates and Keys Use `--client-certificate` and `--client-key` with `--use-tls` when the upstream TACACS+ server requires the daemon to present a TLS client identity. diff --git a/executables/tacacsrs_agentd/src/cli.rs b/executables/tacacsrs_agentd/src/cli.rs index 61b3305..b8c329b 100644 --- a/executables/tacacsrs_agentd/src/cli.rs +++ b/executables/tacacsrs_agentd/src/cli.rs @@ -84,6 +84,10 @@ pub(crate) struct Cli { #[arg(long)] pub(crate) proxy_endpoint: Option, + /// Shared secret expected from raw TACACS+ proxy clients. + #[arg(long, value_name = "SECRET", requires = "proxy_endpoint", conflicts_with = "sonic")] + pub(crate) proxy_shared_secret: Option, + /// Runtime service mode. Defaults to client-api, or both when --proxy-endpoint is set. #[arg(long, value_enum)] pub(crate) service_mode: Option, diff --git a/executables/tacacsrs_agentd/src/config_filter.rs b/executables/tacacsrs_agentd/src/config_filter.rs index c847134..6458191 100644 --- a/executables/tacacsrs_agentd/src/config_filter.rs +++ b/executables/tacacsrs_agentd/src/config_filter.rs @@ -1,47 +1,92 @@ //! Runtime TACACS+ configuration filters composed by the daemon entry point. +use std::collections::HashSet; use std::net::{IpAddr, SocketAddr}; use std::sync::Arc; use futures_util::future::{BoxFuture, FutureExt}; -use tacacsrs_agent::EnabledServices; +use tacacsrs_agent::{EnabledServices, ProxyDownstreamObfuscation}; use tacacsrs_agent_client::IpcEndpoint; use tacacsrs_config::{TacacsPlus, TacacsPlusServer}; +/// Filtered daemon configuration plus local proxy-only metadata derived from it. +pub(crate) struct FilteredTacacsPlus { + /// TACACS+ configuration that should be applied to upstream runtime state. + pub(crate) tacacs_plus: TacacsPlus, + /// Obfuscation policy expected on downstream raw TACACS+ proxy traffic. + pub(crate) proxy_downstream_obfuscation: ProxyDownstreamObfuscation, +} + /// Filters a validated TACACS+ configuration before it is applied to runtime state. pub(crate) trait TacacsPlusFilter: Send + Sync { /// Return the configuration snapshot that should be applied to the daemon. - fn filter(&self, tacacs_plus: TacacsPlus) -> BoxFuture<'_, TacacsPlus>; + fn filter(&self, tacacs_plus: TacacsPlus) -> BoxFuture<'_, anyhow::Result>; } /// Filter implementation that preserves every server unchanged. #[derive(Debug, Default)] -pub(crate) struct NoopTacacsPlusFilter; +pub(crate) struct NoopTacacsPlusFilter { + proxy_downstream_obfuscation: ProxyDownstreamObfuscation, +} + +impl NoopTacacsPlusFilter { + fn new(proxy_downstream_obfuscation: ProxyDownstreamObfuscation) -> Self { + Self { + proxy_downstream_obfuscation, + } + } +} impl TacacsPlusFilter for NoopTacacsPlusFilter { - fn filter(&self, tacacs_plus: TacacsPlus) -> BoxFuture<'_, TacacsPlus> { - async move { tacacs_plus }.boxed() + fn filter(&self, tacacs_plus: TacacsPlus) -> BoxFuture<'_, anyhow::Result> { + let proxy_downstream_obfuscation = self.proxy_downstream_obfuscation.clone(); + async move { + Ok(FilteredTacacsPlus { + tacacs_plus, + proxy_downstream_obfuscation, + }) + } + .boxed() } } /// Filter that removes upstream servers targeting the local TCP proxy endpoint. -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone)] pub(crate) struct ProxySelfLoopFilter { proxy_endpoint: SocketAddr, + fallback_proxy_downstream_obfuscation: ProxyDownstreamObfuscation, } impl ProxySelfLoopFilter { /// Create a filter for the local TCP proxy endpoint. + #[cfg(test)] + #[must_use] + pub(crate) fn new(proxy_endpoint: SocketAddr) -> Self { + Self::new_with_fallback(proxy_endpoint, ProxyDownstreamObfuscation::Unobfuscated) + } + #[must_use] - pub(crate) const fn new(proxy_endpoint: SocketAddr) -> Self { - Self { proxy_endpoint } + pub(crate) fn new_with_fallback( + proxy_endpoint: SocketAddr, + fallback_proxy_downstream_obfuscation: ProxyDownstreamObfuscation, + ) -> Self { + Self { + proxy_endpoint, + fallback_proxy_downstream_obfuscation, + } } - async fn filter_proxy_self_loop_servers(&self, mut tacacs_plus: TacacsPlus) -> TacacsPlus { + async fn filter_proxy_self_loop_servers( + &self, + mut tacacs_plus: TacacsPlus, + ) -> anyhow::Result { let original_server_count = tacacs_plus.server.len(); - let mut retained = Vec::with_capacity(original_server_count); - for server in tacacs_plus.server { - if upstream_server_targets_proxy_endpoint(&server, self.proxy_endpoint).await { + let enumerated_servers = tacacsrs_config::enumerate_servers(&tacacs_plus)?; + let mut filtered_names = HashSet::new(); + let mut selected_proxy_server: Option = None; + + for server in &enumerated_servers { + if upstream_server_targets_proxy_endpoint(server, self.proxy_endpoint).await { log::warn!( "Ignoring upstream TACACS+ server '{}' at {}:{} because it resolves to the local TACACS+ proxy endpoint {}; keeping it would proxy client traffic back into this daemon", server.name, @@ -49,13 +94,31 @@ impl ProxySelfLoopFilter { server.port, self.proxy_endpoint, ); - } else { - retained.push(server); + filtered_names.insert(server.name.clone()); + if let Some(selected) = selected_proxy_server.as_ref() { + if selected.shared_secret != server.shared_secret { + log::warn!( + "Multiple upstream TACACS+ server rows resolve to local proxy endpoint {}; using the highest-priority row '{}' for downstream proxy obfuscation settings", + self.proxy_endpoint, + selected.name, + ); + } + } else { + selected_proxy_server = Some(server.clone()); + } } } - let filtered_count = original_server_count.saturating_sub(retained.len()); - tacacs_plus.server = retained; + let proxy_downstream_obfuscation = selected_proxy_server.as_ref().map_or_else( + || self.fallback_proxy_downstream_obfuscation.clone(), + |server| proxy_downstream_obfuscation_from_secret(server.shared_secret.clone()), + ); + + tacacs_plus + .server + .retain(|server| !filtered_names.contains(&server.name)); + + let filtered_count = original_server_count.saturating_sub(tacacs_plus.server.len()); if filtered_count > 0 { log::info!( @@ -69,13 +132,29 @@ impl ProxySelfLoopFilter { self.proxy_endpoint, ); } + if let Some(server) = selected_proxy_server.as_ref() { + if server.shared_secret.is_some() { + log::info!( + "Using shared secret from highest-priority filtered local proxy endpoint row '{}' for downstream TACACS+ proxy obfuscation", + server.name, + ); + } else { + log::info!( + "Highest-priority filtered local proxy endpoint row '{}' has no shared secret; expecting unobfuscated downstream TACACS+ proxy traffic", + server.name, + ); + } + } - tacacs_plus + Ok(FilteredTacacsPlus { + tacacs_plus, + proxy_downstream_obfuscation, + }) } } impl TacacsPlusFilter for ProxySelfLoopFilter { - fn filter(&self, tacacs_plus: TacacsPlus) -> BoxFuture<'_, TacacsPlus> { + fn filter(&self, tacacs_plus: TacacsPlus) -> BoxFuture<'_, anyhow::Result> { async move { self.filter_proxy_self_loop_servers(tacacs_plus).await }.boxed() } } @@ -85,17 +164,30 @@ impl TacacsPlusFilter for ProxySelfLoopFilter { pub(crate) fn config_filter_from_runtime_options( enabled_services: EnabledServices, proxy_endpoint: Option<&IpcEndpoint>, + proxy_shared_secret: Option, ) -> Arc { + let proxy_downstream_obfuscation = proxy_shared_secret + .map_or(ProxyDownstreamObfuscation::Unobfuscated, ProxyDownstreamObfuscation::SharedSecret); + if !enabled_services.tacacs_proxy() { - return Arc::new(NoopTacacsPlusFilter); + return Arc::new(NoopTacacsPlusFilter::new(proxy_downstream_obfuscation)); } match proxy_endpoint { - Some(IpcEndpoint::Tcp(address)) => Arc::new(ProxySelfLoopFilter::new(*address)), - _ => Arc::new(NoopTacacsPlusFilter), + Some(IpcEndpoint::Tcp(address)) => { + Arc::new(ProxySelfLoopFilter::new_with_fallback(*address, proxy_downstream_obfuscation)) + } + _ => Arc::new(NoopTacacsPlusFilter::new(proxy_downstream_obfuscation)), } } +fn proxy_downstream_obfuscation_from_secret( + shared_secret: Option, +) -> ProxyDownstreamObfuscation { + shared_secret + .map_or(ProxyDownstreamObfuscation::Unobfuscated, ProxyDownstreamObfuscation::SharedSecret) +} + async fn upstream_server_targets_proxy_endpoint( server: &TacacsPlusServer, proxy_endpoint: SocketAddr, @@ -128,31 +220,58 @@ mod tests { config_filter_from_runtime_options, NoopTacacsPlusFilter, ProxySelfLoopFilter, TacacsPlusFilter, }; - use tacacsrs_agent::EnabledServices; + use tacacsrs_agent::{EnabledServices, ProxyDownstreamObfuscation}; use tacacsrs_agent_client::IpcEndpoint; - use tacacsrs_config::{TacacsPlus, TacacsPlusBuilder, TacacsPlusServerBuilder}; use tacacsrs_config::TacacsPlusServerType; + use tacacsrs_config::{ + TacacsPlus, TacacsPlusBuilder, TacacsPlusServerBuilder, ValidationOptions, + ValidationRelaxation, + }; fn test_config_from_host_ports(addresses: &[(&str, u16)]) -> TacacsPlus { + test_config_from_host_ports_and_secrets( + &addresses + .iter() + .map(|(address, port)| (*address, *port, Some("test-secret"))) + .collect::>(), + ) + } + + fn test_config_from_host_ports_and_secrets( + addresses: &[(&str, u16, Option<&str>)], + ) -> TacacsPlus { addresses .iter() .enumerate() - .map(|(index, (address, port))| { - TacacsPlusServerBuilder::new( + .map(|(index, (address, port, shared_secret))| { + let builder = TacacsPlusServerBuilder::new( format!("server-{index}"), TacacsPlusServerType::AUTHENTICATION | TacacsPlusServerType::AUTHORIZATION | TacacsPlusServerType::ACCOUNTING, (*address).to_owned(), *port, - ) - .with_shared_secret("test-secret".to_owned()) + ); + if let Some(shared_secret) = shared_secret { + builder.with_shared_secret((*shared_secret).to_owned()) + } else { + builder + } }) .fold(TacacsPlusBuilder::new(), TacacsPlusBuilder::with_server_builder) - .build() + .build_with_options( + &ValidationOptions::new() + .with_relaxation(ValidationRelaxation::AllowPlainTcpWithoutSharedSecret), + ) .expect("test config should be valid") } + fn proxy_obfuscation(secret: Option<&str>) -> ProxyDownstreamObfuscation { + secret.map_or(ProxyDownstreamObfuscation::Unobfuscated, |secret| { + ProxyDownstreamObfuscation::SharedSecret(secret.to_owned()) + }) + } + #[tokio::test] async fn runtime_options_select_proxy_filter_for_enabled_tcp_proxy() { let endpoint = Some( @@ -161,13 +280,17 @@ mod tests { .expect("TCP endpoint should parse"), ); let config = test_config_from_host_ports(&[("127.0.0.1", 9050), ("192.0.2.20", 49)]); - let filter = - config_filter_from_runtime_options(EnabledServices::TACACS_PROXY, endpoint.as_ref()); + let filter = config_filter_from_runtime_options( + EnabledServices::TACACS_PROXY, + endpoint.as_ref(), + None, + ); - let filtered = filter.filter(config).await; + let filtered = filter.filter(config).await.expect("filter should succeed"); - assert_eq!(filtered.server.len(), 1); - assert_eq!(filtered.server[0].address, "192.0.2.20"); + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!(filtered.tacacs_plus.server[0].address, "192.0.2.20"); + assert_eq!(filtered.proxy_downstream_obfuscation, proxy_obfuscation(Some("test-secret"))); } #[tokio::test] @@ -178,13 +301,17 @@ mod tests { .expect("TCP endpoint should parse"), ); let config = test_config_from_host_ports(&[("127.0.0.1", 9050)]); - let filter = - config_filter_from_runtime_options(EnabledServices::CLIENT_API, endpoint.as_ref()); + let filter = config_filter_from_runtime_options( + EnabledServices::CLIENT_API, + endpoint.as_ref(), + None, + ); - let filtered = filter.filter(config).await; + let filtered = filter.filter(config).await.expect("filter should succeed"); - assert_eq!(filtered.server.len(), 1); - assert_eq!(filtered.server[0].address, "127.0.0.1"); + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!(filtered.tacacs_plus.server[0].address, "127.0.0.1"); + assert_eq!(filtered.proxy_downstream_obfuscation, ProxyDownstreamObfuscation::Unobfuscated); } #[cfg(unix)] @@ -196,23 +323,31 @@ mod tests { .expect("Unix endpoint should parse"), ); let config = test_config_from_host_ports(&[("127.0.0.1", 9050)]); - let filter = - config_filter_from_runtime_options(EnabledServices::TACACS_PROXY, endpoint.as_ref()); + let filter = config_filter_from_runtime_options( + EnabledServices::TACACS_PROXY, + endpoint.as_ref(), + None, + ); - let filtered = filter.filter(config).await; + let filtered = filter.filter(config).await.expect("filter should succeed"); - assert_eq!(filtered.server.len(), 1); - assert_eq!(filtered.server[0].address, "127.0.0.1"); + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!(filtered.tacacs_plus.server[0].address, "127.0.0.1"); + assert_eq!(filtered.proxy_downstream_obfuscation, ProxyDownstreamObfuscation::Unobfuscated); } #[tokio::test] async fn noop_filter_preserves_all_servers() { let config = test_config_from_host_ports(&[("127.0.0.1", 9050)]); - let filtered = NoopTacacsPlusFilter.filter(config).await; + let filtered = NoopTacacsPlusFilter::default() + .filter(config) + .await + .expect("filter should succeed"); - assert_eq!(filtered.server.len(), 1); - assert_eq!(filtered.server[0].address, "127.0.0.1"); + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!(filtered.tacacs_plus.server[0].address, "127.0.0.1"); + assert_eq!(filtered.proxy_downstream_obfuscation, ProxyDownstreamObfuscation::Unobfuscated); } #[tokio::test] @@ -221,10 +356,11 @@ mod tests { let filter = ProxySelfLoopFilter::new("127.0.0.1:9050".parse().expect("socket should parse")); - let filtered = filter.filter(config).await; + let filtered = filter.filter(config).await.expect("filter should succeed"); - assert_eq!(filtered.server.len(), 1); - assert_eq!(filtered.server[0].address, "192.0.2.20"); + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!(filtered.tacacs_plus.server[0].address, "192.0.2.20"); + assert_eq!(filtered.proxy_downstream_obfuscation, proxy_obfuscation(Some("test-secret"))); } #[tokio::test] @@ -232,10 +368,11 @@ mod tests { let config = test_config_from_host_ports(&[("::1", 9050), ("192.0.2.20", 49)]); let filter = ProxySelfLoopFilter::new("[::1]:9050".parse().expect("socket should parse")); - let filtered = filter.filter(config).await; + let filtered = filter.filter(config).await.expect("filter should succeed"); - assert_eq!(filtered.server.len(), 1); - assert_eq!(filtered.server[0].address, "192.0.2.20"); + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!(filtered.tacacs_plus.server[0].address, "192.0.2.20"); + assert_eq!(filtered.proxy_downstream_obfuscation, proxy_obfuscation(Some("test-secret"))); } #[tokio::test] @@ -248,10 +385,11 @@ mod tests { let config = test_config_from_host_ports(&[("localhost", 9050), ("192.0.2.20", 49)]); let filter = ProxySelfLoopFilter::new(proxy_endpoint); - let filtered = filter.filter(config).await; + let filtered = filter.filter(config).await.expect("filter should succeed"); - assert_eq!(filtered.server.len(), 1); - assert_eq!(filtered.server[0].address, "192.0.2.20"); + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!(filtered.tacacs_plus.server[0].address, "192.0.2.20"); + assert_eq!(filtered.proxy_downstream_obfuscation, proxy_obfuscation(Some("test-secret"))); } #[tokio::test] @@ -260,9 +398,10 @@ mod tests { let filter = ProxySelfLoopFilter::new("127.0.0.1:9050".parse().expect("socket should parse")); - let filtered = filter.filter(config).await; + let filtered = filter.filter(config).await.expect("filter should succeed"); - assert_eq!(filtered.server.len(), 2); + assert_eq!(filtered.tacacs_plus.server.len(), 2); + assert_eq!(filtered.proxy_downstream_obfuscation, ProxyDownstreamObfuscation::Unobfuscated); } #[tokio::test] @@ -271,9 +410,90 @@ mod tests { let filter = ProxySelfLoopFilter::new("127.0.0.1:9050".parse().expect("socket should parse")); - let filtered = filter.filter(config).await; + let filtered = filter.filter(config).await.expect("filter should succeed"); + + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!(filtered.tacacs_plus.server[0].address, "192.0.2.20"); + assert_eq!(filtered.proxy_downstream_obfuscation, ProxyDownstreamObfuscation::Unobfuscated); + } + + #[tokio::test] + async fn runtime_options_use_cli_proxy_secret_when_no_local_proxy_row_matches() { + let endpoint = Some( + "127.0.0.1:9050" + .parse::() + .expect("TCP endpoint should parse"), + ); + let config = test_config_from_host_ports(&[("192.0.2.20", 49)]); + let filter = config_filter_from_runtime_options( + EnabledServices::TACACS_PROXY, + endpoint.as_ref(), + Some("cli-proxy-secret".to_owned()), + ); + + let filtered = filter.filter(config).await.expect("filter should succeed"); + + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!( + filtered.proxy_downstream_obfuscation, + proxy_obfuscation(Some("cli-proxy-secret")) + ); + } + + #[tokio::test] + async fn proxy_filter_prefers_matching_local_row_secret_over_cli_proxy_secret() { + let config = test_config_from_host_ports_and_secrets(&[ + ("127.0.0.1", 9050, Some("local-row-secret")), + ("192.0.2.20", 49, Some("upstream-secret")), + ]); + let filter = ProxySelfLoopFilter::new_with_fallback( + "127.0.0.1:9050".parse().expect("socket should parse"), + proxy_obfuscation(Some("cli-proxy-secret")), + ); + + let filtered = filter.filter(config).await.expect("filter should succeed"); + + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!( + filtered.proxy_downstream_obfuscation, + proxy_obfuscation(Some("local-row-secret")) + ); + } + + #[tokio::test] + async fn proxy_filter_uses_highest_priority_matching_proxy_secret() { + let config = test_config_from_host_ports_and_secrets(&[ + ("127.0.0.1", 9050, Some("highest-priority-secret")), + ("localhost", 9050, Some("lower-priority-secret")), + ("192.0.2.20", 49, Some("upstream-secret")), + ]); + let filter = + ProxySelfLoopFilter::new("127.0.0.1:9050".parse().expect("socket should parse")); + + let filtered = filter.filter(config).await.expect("filter should succeed"); + + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!(filtered.tacacs_plus.server[0].address, "192.0.2.20"); + assert_eq!( + filtered.proxy_downstream_obfuscation, + proxy_obfuscation(Some("highest-priority-secret")) + ); + } + + #[tokio::test] + async fn proxy_filter_does_not_use_lower_priority_secret_when_highest_has_none() { + let config = test_config_from_host_ports_and_secrets(&[ + ("127.0.0.1", 9050, None), + ("localhost", 9050, Some("lower-priority-secret")), + ("192.0.2.20", 49, Some("upstream-secret")), + ]); + let filter = + ProxySelfLoopFilter::new("127.0.0.1:9050".parse().expect("socket should parse")); + + let filtered = filter.filter(config).await.expect("filter should succeed"); - assert_eq!(filtered.server.len(), 1); - assert_eq!(filtered.server[0].address, "192.0.2.20"); + assert_eq!(filtered.tacacs_plus.server.len(), 1); + assert_eq!(filtered.tacacs_plus.server[0].address, "192.0.2.20"); + assert_eq!(filtered.proxy_downstream_obfuscation, proxy_obfuscation(None)); } } diff --git a/executables/tacacsrs_agentd/src/main.rs b/executables/tacacsrs_agentd/src/main.rs index 740e985..615dfac 100644 --- a/executables/tacacsrs_agentd/src/main.rs +++ b/executables/tacacsrs_agentd/src/main.rs @@ -178,8 +178,13 @@ async fn apply_config_change( change.delta.modified_servers, change.delta.root_metadata_changed, ); - let config = config_filter.filter((*change.config).clone()).await; - service.reload_tacacs_plus(config).await + let filtered = config_filter.filter((*change.config).clone()).await?; + service + .reload_tacacs_plus_with_proxy_downstream_obfuscation( + filtered.tacacs_plus, + filtered.proxy_downstream_obfuscation, + ) + .await } /// Spawn a background task that consumes [`ConfigDatastore::subscribe`] @@ -271,8 +276,11 @@ async fn main() -> anyhow::Result<()> { "--proxy-endpoint requires --service-mode tacacs-proxy or --service-mode both" ); } - let config_filter = - config_filter_from_runtime_options(enabled_services, proxy_endpoint.as_ref()); + let config_filter = config_filter_from_runtime_options( + enabled_services, + proxy_endpoint.as_ref(), + cli.proxy_shared_secret.clone(), + ); let datastore = build_datastore(&cli); let tacacs_plus = { @@ -280,8 +288,10 @@ async fn main() -> anyhow::Result<()> { format!("Failed to load configuration from datastore '{}'", datastore.label()) })?; log::info!("Initial configuration loaded from datastore '{}'", datastore.label()); - config_filter.filter(initial).await + config_filter.filter(initial).await? }; + let proxy_downstream_obfuscation = tacacs_plus.proxy_downstream_obfuscation; + let tacacs_plus = tacacs_plus.tacacs_plus; log::info!( "Upstream servers: {} configured, probe interval: {}s", @@ -302,6 +312,7 @@ async fn main() -> anyhow::Result<()> { enabled_services, endpoint, proxy_endpoint, + proxy_downstream_obfuscation, tacacs_plus, preferred_probe_interval: Duration::from_secs(cli.preferred_probe_interval_seconds), #[cfg(unix)] @@ -333,7 +344,9 @@ mod tests { use super::{Cli, apply_config_change, cli_datastore_input_from_cli, enabled_services_from_cli}; use crate::config_filter::{NoopTacacsPlusFilter, ProxySelfLoopFilter}; - use tacacsrs_agent::{EnabledServices, ServiceConfig, TacacsClientService}; + use tacacsrs_agent::{ + EnabledServices, ProxyDownstreamObfuscation, ServiceConfig, TacacsClientService, + }; use tacacsrs_agent_client::IpcEndpoint; use tacacsrs_config::PskDheKeSupportedGroup; use tacacsrs_config::crypto_types::PrivateKeyFormat; @@ -391,6 +404,7 @@ mod tests { enabled_services: EnabledServices::CLIENT_API, endpoint: IpcEndpoint::default_local(), proxy_endpoint: None, + proxy_downstream_obfuscation: ProxyDownstreamObfuscation::default(), tacacs_plus: config, preferred_probe_interval: Duration::from_secs(1), #[cfg(unix)] @@ -526,6 +540,33 @@ mod tests { assert_eq!(enabled_services_from_cli(&cli), EnabledServices::TACACS_PROXY); } + #[test] + fn proxy_shared_secret_requires_proxy_endpoint() { + let result = Cli::try_parse_from([ + "tacacsrs-agentd", + "--server-addr", + "192.0.2.20:49", + "--proxy-shared-secret", + "proxy-secret", + ]); + + assert!(result.is_err()); + } + + #[test] + fn proxy_shared_secret_conflicts_with_sonic_config_source() { + let result = Cli::try_parse_from([ + "tacacsrs-agentd", + "--sonic", + "--proxy-endpoint", + "127.0.0.1:9050", + "--proxy-shared-secret", + "proxy-secret", + ]); + + assert!(result.is_err()); + } + #[tokio::test] async fn apply_config_change_reloads_service_without_restart() { let initial = test_config(&["192.0.2.10:49"]); @@ -538,7 +579,7 @@ mod tests { config: Arc::new(updated), }; - apply_config_change("test", change, &service, &NoopTacacsPlusFilter) + apply_config_change("test", change, &service, &NoopTacacsPlusFilter::default()) .await .unwrap(); diff --git a/libraries/tacacsrs_agent/src/config.rs b/libraries/tacacsrs_agent/src/config.rs index e3ad978..769270d 100644 --- a/libraries/tacacsrs_agent/src/config.rs +++ b/libraries/tacacsrs_agent/src/config.rs @@ -70,6 +70,18 @@ impl EnabledServices { } } +/// Downstream obfuscation policy for raw TACACS+ proxy clients. +#[derive(Debug, Clone, Default, Eq, PartialEq)] +pub enum ProxyDownstreamObfuscation { + /// Expect downstream proxy clients to send unobfuscated TACACS+ packets. + #[default] + Unobfuscated, + + /// Expect downstream proxy clients to use this shared secret for TACACS+ + /// message obfuscation. + SharedSecret(String), +} + /// Configuration for the long-lived TACACS+ client service process. /// /// The service consumes this once at startup. Validation that requires cross- @@ -104,6 +116,15 @@ pub struct ServiceConfig { /// one upstream TACACS+ session. TCP proxy endpoints must be loopback-only. pub proxy_endpoint: Option, + /// Obfuscation policy expected from raw TACACS+ proxy clients. + /// + /// This is intentionally separate from upstream server shared secrets: + /// the proxy has to deobfuscate and reobfuscate packets when rewriting + /// TACACS+ session IDs across the downstream/upstream boundary, so the + /// downstream client-facing choice is simply whether clients send + /// unobfuscated packets or packets obfuscated with a local proxy secret. + pub proxy_downstream_obfuscation: ProxyDownstreamObfuscation, + /// Root TACACS+ configuration including upstream servers and any shared /// credential bundles. /// diff --git a/libraries/tacacsrs_agent/src/lib.rs b/libraries/tacacsrs_agent/src/lib.rs index 1b1031f..5db7212 100644 --- a/libraries/tacacsrs_agent/src/lib.rs +++ b/libraries/tacacsrs_agent/src/lib.rs @@ -20,5 +20,5 @@ pub mod upstream; #[cfg(test)] mod test_support; -pub use config::{EnabledServices, ServiceConfig}; +pub use config::{EnabledServices, ProxyDownstreamObfuscation, ServiceConfig}; pub use runtime::TacacsClientService; diff --git a/libraries/tacacsrs_agent/src/runtime/client_service.rs b/libraries/tacacsrs_agent/src/runtime/client_service.rs index a49aa1f..8b5332e 100644 --- a/libraries/tacacsrs_agent/src/runtime/client_service.rs +++ b/libraries/tacacsrs_agent/src/runtime/client_service.rs @@ -23,10 +23,11 @@ use std::sync::Arc; use tacacsrs_agent_client::IpcEndpoint; use tacacsrs_config::TacacsPlus; +use tokio::sync::RwLock; use tokio::task::JoinSet; use super::{RequestTracker, enumerate_supported_servers}; -use crate::config::ServiceConfig; +use crate::config::{ProxyDownstreamObfuscation, ServiceConfig}; use crate::services::client_api::ClientApiService; use crate::services::tacacs_proxy::TacacsProxyService; use crate::services::ListenerOptions; @@ -61,6 +62,8 @@ pub struct TacacsClientService { config: ServiceConfig, /// Shared failover state used by all IPC client handlers. state: Arc, + /// Shared downstream obfuscation policy for newly accepted raw proxy clients. + proxy_downstream_obfuscation: Arc>, /// Shared request lifecycle tracker used for graceful shutdown draining. request_tracker: Arc, } @@ -85,11 +88,14 @@ impl TacacsClientService { }); let state = Arc::new(UpstreamManager::new(servers, connector, config.preferred_probe_interval)); + let proxy_downstream_obfuscation = + Arc::new(RwLock::new(config.proxy_downstream_obfuscation.clone())); let request_tracker = Arc::new(RequestTracker::default()); Ok(Self { config, state, + proxy_downstream_obfuscation, request_tracker, }) } @@ -107,11 +113,14 @@ impl TacacsClientService { let state = Arc::new(UpstreamManager::new(servers, connector, config.preferred_probe_interval)); + let proxy_downstream_obfuscation = + Arc::new(RwLock::new(config.proxy_downstream_obfuscation.clone())); let request_tracker = Arc::new(RequestTracker::default()); Ok(Self { config, state, + proxy_downstream_obfuscation, request_tracker, }) } @@ -123,10 +132,32 @@ impl TacacsClientService { /// Returns an error if credential-reference resolution fails. The existing /// runtime state is left unchanged on error. pub async fn reload_tacacs_plus(&self, tacacs_plus: TacacsPlus) -> anyhow::Result<()> { + let proxy_downstream_obfuscation = self.proxy_downstream_obfuscation.read().await.clone(); + self.reload_tacacs_plus_with_proxy_downstream_obfuscation( + tacacs_plus, + proxy_downstream_obfuscation, + ) + .await + } + + /// Applies a TACACS+ snapshot plus raw proxy downstream obfuscation policy to the running service. + /// + /// # Errors + /// + /// Returns an error if credential-reference resolution fails. The existing + /// runtime state is left unchanged on error. + pub async fn reload_tacacs_plus_with_proxy_downstream_obfuscation( + &self, + tacacs_plus: TacacsPlus, + proxy_downstream_obfuscation: ProxyDownstreamObfuscation, + ) -> anyhow::Result<()> { let mut reload_config = self.config.clone(); reload_config.tacacs_plus = tacacs_plus; + reload_config.proxy_downstream_obfuscation = proxy_downstream_obfuscation.clone(); let servers = enumerate_supported_servers(&reload_config)?; - self.state.reload_servers(servers).await + self.state.reload_servers(servers).await?; + *self.proxy_downstream_obfuscation.write().await = proxy_downstream_obfuscation; + Ok(()) } /// Returns the current number of accounting-capable upstream servers. @@ -185,8 +216,11 @@ impl TacacsClientService { } if self.config.enabled_services.tacacs_proxy() { - let service = - TacacsProxyService::new(Arc::clone(&self.state), Arc::clone(&self.request_tracker)); + let service = TacacsProxyService::new( + Arc::clone(&self.state), + Arc::clone(&self.proxy_downstream_obfuscation), + Arc::clone(&self.request_tracker), + ); let endpoint = proxy_endpoint(&self.config)?.clone(); tasks.spawn(async move { service.serve(&endpoint, listener_options).await }); service_count += 1; @@ -251,7 +285,7 @@ mod tests { }; use tonic::Request; - use crate::config::EnabledServices; + use crate::config::{EnabledServices, ProxyDownstreamObfuscation}; use super::TacacsClientService; use crate::runtime::RequestTracker; use crate::runtime::REQUIRED_SERVER_TYPES; @@ -299,6 +333,7 @@ mod tests { enabled_services: EnabledServices::CLIENT_API, endpoint, proxy_endpoint: None, + proxy_downstream_obfuscation: ProxyDownstreamObfuscation::default(), tacacs_plus, preferred_probe_interval: Duration::from_millis(50), socket_mode: 0o660, diff --git a/libraries/tacacsrs_agent/src/services/tacacs_proxy/service.rs b/libraries/tacacsrs_agent/src/services/tacacs_proxy/service.rs index a0bfaba..00fd8e7 100644 --- a/libraries/tacacsrs_agent/src/services/tacacs_proxy/service.rs +++ b/libraries/tacacsrs_agent/src/services/tacacs_proxy/service.rs @@ -4,9 +4,11 @@ use std::sync::Arc; use tacacsrs_agent_client::IpcEndpoint; use tokio::io::{AsyncRead, AsyncWrite}; +use tokio::sync::RwLock; use super::listener; use super::upstream_bridge::UpstreamBridge; +use crate::config::ProxyDownstreamObfuscation; use crate::runtime::{RequestGuard, RequestTracker}; use crate::services::ListenerOptions; use crate::upstream::manager::UpstreamManager; @@ -22,11 +24,12 @@ impl TacacsProxyService { /// Creates a raw TACACS+ proxy service over shared runtime state. pub(crate) fn new( upstream_manager: Arc, + downstream_obfuscation: Arc>, request_tracker: Arc, ) -> Self { Self { request_tracker, - upstream_bridge: UpstreamBridge::new(upstream_manager), + upstream_bridge: UpstreamBridge::new(upstream_manager, downstream_obfuscation), } } diff --git a/libraries/tacacsrs_agent/src/services/tacacs_proxy/upstream_bridge/mod.rs b/libraries/tacacsrs_agent/src/services/tacacs_proxy/upstream_bridge/mod.rs index cfa3c68..0827d86 100644 --- a/libraries/tacacsrs_agent/src/services/tacacs_proxy/upstream_bridge/mod.rs +++ b/libraries/tacacsrs_agent/src/services/tacacs_proxy/upstream_bridge/mod.rs @@ -4,16 +4,17 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Context; -use tacacsrs_config::TacacsPlusServer; use tacacsrs_flow_abstractions::client_session_flow_io::ClientSessionFlowIoTrait; use tacacsrs_messages::packet::PacketTrait; use tacacsrs_networking::PacketWriter; use tokio::io::{AsyncRead, AsyncWrite}; +use tokio::sync::RwLock; use self::error::ProxyConnectionError; use self::packet_io::{read_downstream_packet, read_upstream_packet, write_downstream_packet}; use self::reply_action::{ReplyAction, reply_action}; use self::session_mapping::rewrite_session_id; +use crate::config::ProxyDownstreamObfuscation; use crate::runtime::RequestGuard; use crate::upstream::manager::{BoundServer, UpstreamManager}; @@ -26,12 +27,19 @@ mod session_mapping; #[derive(Clone)] pub(super) struct UpstreamBridge { upstream_manager: Arc, + downstream_obfuscation: Arc>, } impl UpstreamBridge { /// Creates a raw proxy upstream bridge over the shared upstream manager. - pub(super) fn new(upstream_manager: Arc) -> Self { - Self { upstream_manager } + pub(super) fn new( + upstream_manager: Arc, + downstream_obfuscation: Arc>, + ) -> Self { + Self { + upstream_manager, + downstream_obfuscation, + } } pub(super) async fn handle_connection( @@ -60,7 +68,8 @@ impl UpstreamBridge { bound_server.index, ); - let result = proxy_bound_connection(stream, &bound_server).await; + let downstream_obfuscation = self.downstream_obfuscation.read().await.clone(); + let result = proxy_bound_connection(stream, &bound_server, downstream_obfuscation).await; match result { Ok(()) => Ok(()), @@ -78,16 +87,13 @@ impl UpstreamBridge { async fn proxy_bound_connection( stream: Stream, bound_server: &BoundServer, + downstream_obfuscation: ProxyDownstreamObfuscation, ) -> Result<(), ProxyConnectionError> where Stream: AsyncRead + AsyncWrite + Unpin + Send, { - let server = bound_server.server(); let timeout = bound_server.timeout_duration(); - let obfuscation_key = server - .shared_secret - .as_ref() - .map(|secret| secret.as_bytes().to_vec()); + let obfuscation_key = downstream_obfuscation_key(&downstream_obfuscation); let upstream_session = match bound_server.connection.create_raw_session().await { Ok(upstream_session) => upstream_session, Err(error) => { @@ -97,12 +103,21 @@ where } }; - proxy_connection_with_session(stream, server, timeout, obfuscation_key, &upstream_session).await + proxy_connection_with_session(stream, timeout, obfuscation_key, &upstream_session).await +} + +fn downstream_obfuscation_key( + downstream_obfuscation: &ProxyDownstreamObfuscation, +) -> Option> { + match downstream_obfuscation { + ProxyDownstreamObfuscation::Unobfuscated => None, + ProxyDownstreamObfuscation::SharedSecret(shared_secret) => Some(shared_secret.as_str()), + } + .map(|secret| secret.as_bytes().to_vec()) } async fn proxy_connection_with_session( mut stream: Stream, - server: &TacacsPlusServer, timeout: Duration, obfuscation_key: Option>, upstream_session: &Session, @@ -111,9 +126,15 @@ where Stream: AsyncRead + AsyncWrite + Unpin + Send, Session: ClientSessionFlowIoTrait + Sync + ?Sized, { - let writer = PacketWriter::new(obfuscation_key); - let result = - proxy_connection_loop(&mut stream, server, timeout, &writer, upstream_session).await; + let writer = PacketWriter::new(obfuscation_key.clone()); + let result = proxy_connection_loop( + &mut stream, + timeout, + obfuscation_key.as_deref(), + &writer, + upstream_session, + ) + .await; upstream_session.complete().await; result @@ -121,8 +142,8 @@ where async fn proxy_connection_loop( stream: &mut Stream, - server: &TacacsPlusServer, timeout: Duration, + downstream_obfuscation_key: Option<&[u8]>, writer: &PacketWriter, upstream_session: &Session, ) -> Result<(), ProxyConnectionError> @@ -134,16 +155,11 @@ where let upstream_session_id = upstream_session.session_id(); loop { - let downstream_packet = match read_downstream_packet( - stream, - timeout, - server.shared_secret.as_ref().map(String::as_bytes), - ) - .await - { - Ok(packet) => packet, - Err(error) => return Err(error), - }; + let downstream_packet = + match read_downstream_packet(stream, timeout, downstream_obfuscation_key).await { + Ok(packet) => packet, + Err(error) => return Err(error), + }; let downstream_reply_obfuscation = downstream_packet.reply_obfuscation; let downstream_packet = downstream_packet.packet; @@ -206,7 +222,6 @@ mod tests { use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; use std::time::Duration; - use tacacsrs_config::TacacsPlusServer; use tacacsrs_flow_abstractions::client_session_flow_io::ClientSessionFlowIoTrait; use tacacsrs_messages::accounting::reply::AccountingReply; use tacacsrs_messages::authentication::reply::AuthenticationReply; @@ -328,31 +343,20 @@ mod tests { .unwrap() } - fn test_server() -> TacacsPlusServer { - TacacsPlusServer { - name: "server".to_owned(), - server_type: tacacsrs_config::TacacsPlusServerType::AUTHENTICATION - | tacacsrs_config::TacacsPlusServerType::AUTHORIZATION - | tacacsrs_config::TacacsPlusServerType::ACCOUNTING, - address: "127.0.0.1".to_owned(), - port: 49, - shared_secret: None, - timeout: 5, - single_connection: false, - domain_name: None, - sni_enabled: None, - client_identity: None, - server_authentication: None, - source_ip: None, - source_interface: None, - vrf_instance: None, - } + #[test] + fn downstream_obfuscation_uses_no_key_for_unobfuscated_clients() { + assert_eq!(downstream_obfuscation_key(&ProxyDownstreamObfuscation::Unobfuscated), None); } - fn test_server_with_secret(secret: &str) -> TacacsPlusServer { - let mut server = test_server(); - server.shared_secret = Some(secret.to_owned()); - server + #[test] + fn downstream_obfuscation_uses_configured_proxy_secret() { + assert_eq!( + downstream_obfuscation_key(&ProxyDownstreamObfuscation::SharedSecret( + "local-proxy-secret".to_owned(), + ),) + .as_deref(), + Some(b"local-proxy-secret".as_slice()) + ); } #[tokio::test] @@ -372,15 +376,9 @@ mod tests { write_packet(&mut client_stream, &request).await; - proxy_connection_with_session( - proxy_stream, - &test_server(), - Duration::from_secs(1), - None, - &fake_session, - ) - .await - .expect("proxy connection should complete"); + proxy_connection_with_session(proxy_stream, Duration::from_secs(1), None, &fake_session) + .await + .expect("proxy connection should complete"); let (received_len, received_session_id, received_body) = { let received_packets = fake_session.received_packets.lock().await; @@ -420,7 +418,6 @@ mod tests { proxy_connection_with_session( proxy_stream, - &test_server_with_secret(secret), Duration::from_secs(1), Some(secret.as_bytes().to_vec()), &fake_session, @@ -456,7 +453,6 @@ mod tests { proxy_connection_with_session( proxy_stream, - &test_server_with_secret(secret), Duration::from_secs(1), Some(secret.as_bytes().to_vec()), &fake_session, @@ -513,7 +509,6 @@ mod tests { let result = proxy_connection_with_session( proxy_stream, - &test_server(), Duration::from_secs(1), None, &fake_session, diff --git a/libraries/tacacsrs_sonic/README.md b/libraries/tacacsrs_sonic/README.md index 88d533e..4152e29 100644 --- a/libraries/tacacsrs_sonic/README.md +++ b/libraries/tacacsrs_sonic/README.md @@ -71,6 +71,9 @@ From the repository root, the full local smoke test can be run with Podman: The manual equivalent is: +SONiC TACACS+ server priorities are in the range `1..64`; higher values are +preferred and are placed earlier in the daemon failover order. + ```bash docker run --rm -p 6379:6379 redis redis-cli -n 4 CONFIG SET notify-keyspace-events KEA @@ -78,7 +81,7 @@ redis-cli -n 4 CONFIG SET notify-keyspace-events KEA redis-cli -n 4 HSET 'TACPLUS|global' \ timeout 5 auth_type pap src_intf Management0 redis-cli -n 4 HSET 'TACPLUS_SERVER|192.0.2.10' \ - priority 1 tcp_port 49 timeout 10 \ + priority 64 tcp_port 49 timeout 10 \ use_tls true domain_name tacacs-a.example.test \ sni_enabled true single_connection true @@ -92,7 +95,7 @@ computed delta: ```bash redis-cli -n 4 HSET 'TACPLUS_SERVER|192.0.2.20' \ - priority 2 tcp_port 49 + priority 32 tcp_port 49 redis-cli -n 4 HSET 'TACPLUS_SERVER|192.0.2.10' timeout 20 redis-cli -n 4 DEL 'TACPLUS_SERVER|192.0.2.20' ``` diff --git a/libraries/tacacsrs_sonic/src/mapping.rs b/libraries/tacacsrs_sonic/src/mapping.rs index bef7bd1..3dab6e5 100644 --- a/libraries/tacacsrs_sonic/src/mapping.rs +++ b/libraries/tacacsrs_sonic/src/mapping.rs @@ -15,7 +15,7 @@ //! passkey "optional-shared-secret" //! src_intf "Management0" //! -//! TACPLUS_SERVER|192.0.2.10 priority "1" +//! TACPLUS_SERVER|192.0.2.10 priority "64" //! tcp_port "49" //! timeout "10" //! use_tls "true" @@ -79,10 +79,10 @@ impl SonicTacacsTables { /// Translate a SONiC ConfigDB snapshot into the YANG `TacacsPlus` root. /// /// Each `TACPLUS_SERVER|` row becomes one -/// [`tacacsrs_config::TacacsPlusServer`]. Servers are ordered by ascending -/// `priority` (with stable address-based tiebreaking) so that the daemon's -/// failover semantics — index 0 is preferred — line up with SONiC's -/// administrator intent. +/// [`tacacsrs_config::TacacsPlusServer`]. Servers are ordered by descending +/// `priority` in SONiC's `1..64` range (with stable address-based +/// tiebreaking) so that the daemon's failover semantics — index 0 is preferred +/// — line up with SONiC's administrator intent. /// /// Per-row fields fall back to the matching `TACPLUS|global` field when the /// per-server value is absent (this matches SONiC's `pam_tacplus` behavior). @@ -92,9 +92,9 @@ impl SonicTacacsTables { /// /// # Errors /// -/// Returns an error if a row has an invalid numeric value -/// (priority/port/timeout), or if the resulting non-empty configuration fails -/// validation. +/// Returns an error if a row has an invalid numeric value (priority, port, or +/// timeout), if priority is outside SONiC's `1..64` range, or if the resulting +/// non-empty configuration fails validation. pub fn map_sonic_tables_to_tacacs_plus(tables: &SonicTacacsTables) -> anyhow::Result { if tables.servers.is_empty() { return Ok(TacacsPlus { @@ -112,11 +112,11 @@ pub fn map_sonic_tables_to_tacacs_plus(tables: &SonicTacacsTables) -> anyhow::Re .map(|(address, fields)| SonicServerRow::from_hash(address, fields)) .collect::>>()?; - // SONiC convention: lower priority number = higher preference. Ties broken - // by lexicographic address so the order is deterministic in tests/logs. + // SONiC convention: priority is 1..64, and higher numbers are preferred. + // Ties are broken by lexicographic address for deterministic tests/logs. rows.sort_by(|a, b| { - a.priority - .cmp(&b.priority) + b.priority + .cmp(&a.priority) .then_with(|| a.address.cmp(&b.address)) }); @@ -183,7 +183,7 @@ impl SonicGlobal { #[derive(Debug, Clone)] struct SonicServerRow { address: String, - priority: i64, + priority: u8, tcp_port: u16, timeout: Option, passkey: Option, @@ -201,7 +201,7 @@ impl SonicServerRow { fn from_hash(address: &str, hash: &SonicHash) -> anyhow::Result { let mut row = Self { address: address.to_string(), - priority: i64::MAX, + priority: 1, tcp_port: DEFAULT_TACACS_TCP_PORT, timeout: None, passkey: None, @@ -218,9 +218,8 @@ impl SonicServerRow { for (key, value) in hash { match key.as_str() { "priority" => { - row.priority = value.parse::().with_context(|| { - format!("TACPLUS_SERVER|{address}.priority='{value}' is not an integer") - })?; + row.priority = parse_priority(value) + .with_context(|| format!("TACPLUS_SERVER|{address}.priority='{value}'"))?; } "tcp_port" => { row.tcp_port = value.parse::().with_context(|| { @@ -383,6 +382,16 @@ fn parse_timeout(value: &str) -> anyhow::Result { .with_context(|| format!("expected timeout in seconds, got '{value}'")) } +fn parse_priority(value: &str) -> anyhow::Result { + let priority = value + .parse::() + .with_context(|| format!("expected priority in range 1..64, got '{value}'"))?; + if !(1..=64).contains(&priority) { + bail!("expected priority in range 1..64, got '{value}'"); + } + Ok(priority) +} + fn parse_server_type(value: &str) -> anyhow::Result { let mut bits = TacacsPlusServerType::empty(); for token in value.split(|c: char| c.is_whitespace() || c == ',' || c == '|') { @@ -430,15 +439,15 @@ mod tests { fn maps_global_defaults_into_each_server() { let cfg = map_sonic_tables_to_tacacs_plus(&tables()).expect("mapping succeeds"); assert_eq!(cfg.server.len(), 2); - // Lower priority -> higher preference (index 0). - assert_eq!(cfg.server[0].address, "192.0.2.10"); + // Higher priority -> higher preference (index 0). + assert_eq!(cfg.server[0].address, "192.0.2.20"); assert_eq!(cfg.server[0].port, 49); assert_eq!(cfg.server[0].timeout, 7); - assert_eq!(cfg.server[0].shared_secret.as_deref(), Some("default-secret")); + assert_eq!(cfg.server[0].shared_secret.as_deref(), Some("per-server-secret")); - // Per-server passkey overrides global. - assert_eq!(cfg.server[1].address, "192.0.2.20"); - assert_eq!(cfg.server[1].shared_secret.as_deref(), Some("per-server-secret")); + // Per-server passkey absent -> falls back to global. + assert_eq!(cfg.server[1].address, "192.0.2.10"); + assert_eq!(cfg.server[1].shared_secret.as_deref(), Some("default-secret")); // Per-server tcp_port absent -> default 49. assert_eq!(cfg.server[1].port, DEFAULT_TACACS_TCP_PORT); // Per-server timeout absent -> falls back to global. @@ -466,11 +475,10 @@ mod tests { #[test] fn invalid_priority_is_an_error() { let mut servers = BTreeMap::new(); - servers - .insert("192.0.2.99".to_string(), h(&[("priority", "not-a-number"), ("passkey", "x")])); + servers.insert("192.0.2.99".to_string(), h(&[("priority", "70"), ("passkey", "x")])); let tables = SonicTacacsTables::new(SonicHash::new(), servers); let err = map_sonic_tables_to_tacacs_plus(&tables).unwrap_err(); - assert!(format!("{err:#}").contains("priority")); + assert!(format!("{err:#}").contains("1..64")); } #[test] @@ -580,7 +588,7 @@ mod tests { .iter() .map(|s| s.address.clone()) .collect::>(); - assert_eq!(order, vec!["192.0.2.10", "192.0.2.30", "192.0.2.20"]); + assert_eq!(order, vec!["192.0.2.20", "192.0.2.10", "192.0.2.30"]); } #[test] @@ -595,7 +603,7 @@ mod tests { .expect("previous mapping succeeds"); let mut new_servers = BTreeMap::new(); - new_servers.insert("192.0.2.10".to_string(), h(&[("priority", "1"), ("passkey", "x")])); + new_servers.insert("192.0.2.10".to_string(), h(&[("priority", "10"), ("passkey", "x")])); new_servers.insert("192.0.2.20".to_string(), h(&[("priority", "5"), ("passkey", "x")])); let new = map_sonic_tables_to_tacacs_plus(&SonicTacacsTables::new(SonicHash::new(), new_servers))