diff --git a/pgdog/src/auth/gssapi/context.rs b/pgdog/src/auth/gssapi/context.rs index 8ec065e1..39478194 100644 --- a/pgdog/src/auth/gssapi/context.rs +++ b/pgdog/src/auth/gssapi/context.rs @@ -53,10 +53,10 @@ impl GssapiContext { // Create the desired mechanisms set let mut desired_mechs = OidSet::new() - .map_err(|e| GssapiError::LibGssapi(format!("Failed to create OidSet: {}", e)))?; + .map_err(|e| GssapiError::LibGssapi(format!("failed to create OidSet: {}", e)))?; desired_mechs .add(&GSS_MECH_KRB5) - .map_err(|e| GssapiError::LibGssapi(format!("Failed to add mechanism: {}", e)))?; + .map_err(|e| GssapiError::LibGssapi(format!("failed to add mechanism: {}", e)))?; // Acquire credentials from the cache that TicketManager populated // Pass None to use the default principal from the cache @@ -67,7 +67,7 @@ impl GssapiContext { Some(&desired_mechs), ) .map_err(|e| { - GssapiError::CredentialAcquisitionFailed(format!("Failed for {}: {}", principal, e)) + GssapiError::CredentialAcquisitionFailed(format!("failed for {}: {}", principal, e)) })?; // Parse target service principal (use KRB5_PRINCIPAL to avoid hostname canonicalization) @@ -123,7 +123,7 @@ impl GssapiContext { self.inner .source_name() .map(|name| name.to_string()) - .map_err(|e| GssapiError::ContextError(format!("Failed to get client name: {}", e))) + .map_err(|e| GssapiError::ContextError(format!("failed to get client name: {}", e))) } } diff --git a/pgdog/src/auth/gssapi/error.rs b/pgdog/src/auth/gssapi/error.rs index 8f868b31..f6643f5c 100644 --- a/pgdog/src/auth/gssapi/error.rs +++ b/pgdog/src/auth/gssapi/error.rs @@ -1,79 +1,55 @@ //! GSSAPI-specific error types -use std::fmt; use std::path::PathBuf; +use thiserror::Error; /// Result type for GSSAPI operations pub type Result = std::result::Result; /// GSSAPI-specific errors -#[derive(Debug)] +#[derive(Debug, Error)] pub enum GssapiError { /// Keytab file not found + #[error("keytab file not found: {0}")] KeytabNotFound(PathBuf), /// Invalid principal name + #[error("invalid principal: {0}")] InvalidPrincipal(String), /// Ticket has expired + #[error("kerberos ticket has expired")] TicketExpired, /// Failed to acquire credentials + #[error("failed to acquire credentials: {0}")] CredentialAcquisitionFailed(String), /// GSSAPI context error + #[error("GSSAPI context error: {0}")] ContextError(String), /// Token processing error + #[error("token processing error: {0}")] TokenError(String), /// Refresh failed + #[error("ticket refresh failed: {0}")] RefreshFailed(String), /// Internal libgssapi error + #[error("GSSAPI library error: {0}")] LibGssapi(String), /// I/O error - Io(std::io::Error), + #[error("{0}")] + Io(#[from] std::io::Error), /// Configuration error + #[error("configuration error: {0}")] Config(String), } -impl fmt::Display for GssapiError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::KeytabNotFound(path) => write!(f, "Keytab file not found: {}", path.display()), - Self::InvalidPrincipal(principal) => write!(f, "Invalid principal: {}", principal), - Self::TicketExpired => write!(f, "Kerberos ticket has expired"), - Self::CredentialAcquisitionFailed(msg) => { - write!(f, "Failed to acquire credentials: {}", msg) - } - Self::ContextError(msg) => write!(f, "GSSAPI context error: {}", msg), - Self::TokenError(msg) => write!(f, "Token processing error: {}", msg), - Self::RefreshFailed(msg) => write!(f, "Ticket refresh failed: {}", msg), - Self::LibGssapi(msg) => write!(f, "GSSAPI library error: {}", msg), - Self::Io(err) => write!(f, "I/O error: {}", err), - Self::Config(msg) => write!(f, "Configuration error: {}", msg), - } - } -} - -impl std::error::Error for GssapiError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - Self::Io(err) => Some(err), - _ => None, - } - } -} - -impl From for GssapiError { - fn from(err: std::io::Error) -> Self { - Self::Io(err) - } -} - // Convert libgssapi errors when we implement the actual functionality #[cfg(feature = "gssapi")] impl From for GssapiError { diff --git a/pgdog/src/auth/gssapi/mod.rs b/pgdog/src/auth/gssapi/mod.rs index 82055dc8..6c946a06 100644 --- a/pgdog/src/auth/gssapi/mod.rs +++ b/pgdog/src/auth/gssapi/mod.rs @@ -32,7 +32,7 @@ pub async fn handle_gssapi_auth( ); let mut server = server.lock().await; - tracing::debug!("Calling server.accept()"); + tracing::debug!("calling server.accept()"); match server.accept(&client_token)? { Some(response_token) => { // Check if authentication is complete despite having a token @@ -40,12 +40,11 @@ pub async fn handle_gssapi_auth( let principal = server .client_principal() .ok_or_else(|| { - tracing::error!("Context complete but no principal found"); - GssapiError::ContextError("No client principal found".to_string()) + GssapiError::ContextError("no client principal found".to_string()) })? .to_string(); - tracing::info!( + tracing::debug!( "Authentication complete (with final token), principal: {}", principal ); @@ -61,7 +60,7 @@ pub async fn handle_gssapi_auth( Ok(response) } else { // More negotiation needed - tracing::info!( + tracing::debug!( "server.accept returned token of {} bytes - negotiation continues", response_token.len() ); @@ -78,16 +77,13 @@ pub async fn handle_gssapi_auth( } None => { // Authentication complete - tracing::info!("server.accept returned None - authentication complete"); + tracing::debug!("server.accept returned None - authentication complete"); let principal = server .client_principal() - .ok_or_else(|| { - tracing::error!("No client principal found in completed context"); - GssapiError::ContextError("No client principal found".to_string()) - })? + .ok_or_else(|| GssapiError::ContextError("No client principal found".to_string()))? .to_string(); - tracing::info!("Successfully extracted principal: {}", principal); + tracing::debug!("successfully extracted principal: {}", principal); let response = GssapiResponse { is_complete: true, token: None, diff --git a/pgdog/src/auth/gssapi/server.rs b/pgdog/src/auth/gssapi/server.rs index 3d96d69a..9292bafc 100644 --- a/pgdog/src/auth/gssapi/server.rs +++ b/pgdog/src/auth/gssapi/server.rs @@ -2,6 +2,7 @@ use super::error::{GssapiError, Result}; use std::path::Path; +#[cfg(feature = "gssapi")] use std::sync::Arc; #[cfg(feature = "gssapi")] @@ -51,10 +52,10 @@ impl GssapiServer { // Create the desired mechanisms set let mut desired_mechs = OidSet::new() - .map_err(|e| GssapiError::LibGssapi(format!("Failed to create OidSet: {}", e)))?; + .map_err(|e| GssapiError::LibGssapi(format!("failed to create OidSet: {}", e)))?; desired_mechs .add(&GSS_MECH_KRB5) - .map_err(|e| GssapiError::LibGssapi(format!("Failed to add mechanism: {}", e)))?; + .map_err(|e| GssapiError::LibGssapi(format!("failed to add mechanism: {}", e)))?; // Acquire credentials for the specified principal Cred::acquire( @@ -65,7 +66,7 @@ impl GssapiServer { ) .map_err(|e| { GssapiError::CredentialAcquisitionFailed(format!( - "Failed to acquire credentials for {}: {}", + "failed to acquire credentials for {}: {}", principal, e )) })? @@ -73,7 +74,7 @@ impl GssapiServer { // Use default service principal Cred::acquire(None, None, CredUsage::Accept, None).map_err(|e| { GssapiError::CredentialAcquisitionFailed(format!( - "Failed to acquire default credentials: {}", + "failed to acquire default credentials: {}", e )) })? @@ -101,24 +102,24 @@ impl GssapiServer { if self.is_complete { tracing::warn!("GssapiServer::accept called but context already complete"); return Err(GssapiError::ContextError( - "Context already complete".to_string(), + "context already complete".to_string(), )); } // Create or reuse the server context let mut ctx = match self.inner.take() { Some(ctx) => { - tracing::debug!("Reusing existing server context"); + tracing::debug!("reusing existing server context"); ctx } None => { - tracing::debug!("Creating new server context"); + tracing::debug!("creating new server context"); ServerCtx::new(Some(self.credential.as_ref().clone())) } }; // Process the client token - tracing::debug!("Calling ctx.step with client token"); + tracing::debug!("calling ctx.step with client token"); match ctx.step(client_token) { Ok(Some(response)) => { // More negotiation needed @@ -133,20 +134,20 @@ impl GssapiServer { // Check if context is actually established despite returning a token if ctx.is_complete() { - tracing::warn!("Context is complete but still returned a token - this might confuse the client"); + tracing::warn!("context is complete but still returned a token - this might confuse the client"); // Mark as complete and extract the principal self.is_complete = true; match ctx.source_name() { Ok(name) => { let principal = name.to_string(); - tracing::info!( + tracing::debug!( "Extracted client principal (with token): {}", principal ); self.client_principal = Some(principal); } Err(e) => { - tracing::error!("Failed to get client principal: {}", e); + tracing::error!("failed to get client principal: {}", e); } } } @@ -156,20 +157,19 @@ impl GssapiServer { } Ok(None) => { // Context established successfully - tracing::info!("ctx.step returned None - GSSAPI context established successfully"); + tracing::debug!("ctx.step returned None - GSSAPI context established successfully"); self.is_complete = true; // Extract the client principal match ctx.source_name() { Ok(name) => { let principal = name.to_string(); - tracing::info!("Extracted client principal: {}", principal); + tracing::debug!("extracted client principal: {}", principal); self.client_principal = Some(principal); } Err(e) => { - tracing::error!("Failed to get client principal: {}", e); return Err(GssapiError::ContextError(format!( - "Failed to get client principal: {}", + "failed to get client principal: {}", e ))); } diff --git a/pgdog/src/auth/gssapi/ticket_cache.rs b/pgdog/src/auth/gssapi/ticket_cache.rs index 60b681a1..82c4d91e 100644 --- a/pgdog/src/auth/gssapi/ticket_cache.rs +++ b/pgdog/src/auth/gssapi/ticket_cache.rs @@ -3,6 +3,7 @@ use super::error::{GssapiError, Result}; use parking_lot::RwLock; use std::path::PathBuf; +#[cfg(feature = "gssapi")] use std::sync::Arc; use std::time::{Duration, Instant}; @@ -85,10 +86,10 @@ impl TicketCache { // Create the desired mechanisms set let mut desired_mechs = OidSet::new() - .map_err(|e| GssapiError::LibGssapi(format!("Failed to create OidSet: {}", e)))?; + .map_err(|e| GssapiError::LibGssapi(format!("failed to create OidSet: {}", e)))?; desired_mechs .add(&GSS_MECH_KRB5) - .map_err(|e| GssapiError::LibGssapi(format!("Failed to add mechanism: {}", e)))?; + .map_err(|e| GssapiError::LibGssapi(format!("failed to add mechanism: {}", e)))?; // Acquire credentials from the keytab let credential = Cred::acquire( @@ -99,7 +100,7 @@ impl TicketCache { ) .map_err(|e| { GssapiError::CredentialAcquisitionFailed(format!( - "Failed for {}: {}", + "failed for {}: {}", self.principal, e )) })?; diff --git a/pgdog/src/auth/gssapi/ticket_manager.rs b/pgdog/src/auth/gssapi/ticket_manager.rs index ba75f6ac..098f027f 100644 --- a/pgdog/src/auth/gssapi/ticket_manager.rs +++ b/pgdog/src/auth/gssapi/ticket_manager.rs @@ -39,7 +39,7 @@ impl TicketManager { /// Get or acquire a ticket for a server /// Returns Ok(()) when the credential cache is ready to use #[cfg(feature = "gssapi")] - pub fn get_ticket( + pub async fn get_ticket( &self, server: impl Into, keytab: impl AsRef, @@ -63,15 +63,16 @@ impl TicketManager { std::env::set_var("KRB5CCNAME", &cache_path); // Use kinit to get a ticket from the keytab into the unique cache - let output = std::process::Command::new("kinit") + let output = tokio::process::Command::new("kinit") .arg("-kt") .arg(&keytab_path) .arg(&principal) .env("KRB5CCNAME", &cache_path) .env("KRB5_CONFIG", "/opt/homebrew/etc/krb5.conf") .output() + .await .map_err(|e| { - super::error::GssapiError::LibGssapi(format!("Failed to run kinit: {}", e)) + super::error::GssapiError::LibGssapi(format!("failed to run kinit: {}", e)) })?; if !output.status.success() { @@ -99,7 +100,7 @@ impl TicketManager { /// Get or acquire a ticket for a server (mock version) #[cfg(not(feature = "gssapi"))] - pub fn get_ticket( + pub async fn get_ticket( &self, _server: impl Into, _keytab: impl AsRef, @@ -126,10 +127,10 @@ impl TicketManager { if cache.needs_refresh() { match cache.refresh() { Ok(()) => { - tracing::info!("Refreshed ticket for {}", server_clone); + tracing::info!("[gssapi] refreshed ticket for \"{}\"", server_clone); } Err(e) => { - tracing::error!("Failed to refresh ticket for {}: {}", server_clone, e); + tracing::error!("failed to refresh ticket for {}: {}", server_clone, e); // Continue trying - the old ticket might still be valid } } @@ -219,12 +220,14 @@ mod tests { assert!(Arc::ptr_eq(&manager1, &manager2)); } - #[test] - fn test_cache_management() { + #[tokio::test] + async fn test_cache_management() { let manager = TicketManager::new(); // This will fail because the keytab doesn't exist, but it tests the structure - let result = manager.get_ticket("server1:5432", "/nonexistent/keytab", "test@REALM"); + let result = manager + .get_ticket("server1:5432", "/nonexistent/keytab", "test@REALM") + .await; assert!(result.is_err()); // Even though ticket acquisition failed, the cache should not be stored diff --git a/pgdog/src/backend/server.rs b/pgdog/src/backend/server.rs index a6259fba..d1765e38 100644 --- a/pgdog/src/backend/server.rs +++ b/pgdog/src/backend/server.rs @@ -159,11 +159,9 @@ impl Server { stream.flush().await?; // Check if GSSAPI is configured for this server - let mut gssapi_context = if addr.gssapi_keytab.is_some() && addr.gssapi_principal.is_some() + let mut gssapi_context = if let (Some(keytab), Some(principal)) = + (&addr.gssapi_keytab, &addr.gssapi_principal) { - let keytab = addr.gssapi_keytab.as_ref().unwrap(); - let principal = addr.gssapi_principal.as_ref().unwrap(); - // Use configured target principal if available, otherwise fallback to default format let target = if let Some(ref target_principal) = addr.gssapi_target_principal { target_principal.clone() @@ -180,10 +178,13 @@ impl Server { // Use TicketManager to set up a credential cache for this server // This ensures we use the correct principal for each backend connection let cache_key = format!("{}:{}", addr.host, addr.port); - match TicketManager::global().get_ticket(&cache_key, keytab, principal) { + match TicketManager::global() + .get_ticket(&cache_key, keytab, principal) + .await + { Ok(()) => { debug!( - "Acquired ticket for {} using principal {}", + "acquired ticket for {} using principal {}", cache_key, principal ); @@ -191,17 +192,17 @@ impl Server { // that TicketManager set up with KRB5CCNAME match GssapiContext::new_initiator(keytab, principal, &target) { Ok(ctx) => { - debug!("Initialized GSSAPI context for {} -> {}", principal, target); + debug!("initialized GSSAPI context for {} -> {}", principal, target); Some(ctx) } Err(e) => { - warn!("Failed to initialize GSSAPI context: {}", e); + warn!("failed to initialize GSSAPI context: {}", e); None } } } Err(e) => { - warn!("Failed to acquire ticket for {}: {}", cache_key, e); + warn!("failed to acquire ticket for {}: {}", cache_key, e); None } } @@ -263,10 +264,6 @@ impl Server { } } else { // No GSSAPI configured, server requires it - error!( - "Server requires GSSAPI but no keytab configured for {}", - addr.host - ); return Err(Error::ConnectionError(Box::new( ErrorResponse::auth( &addr.user, @@ -277,7 +274,6 @@ impl Server { } Authentication::Sspi => { // SSPI is Windows-specific GSSAPI variant - error!("SSPI authentication not supported"); return Err(Error::ConnectionError(Box::new(ErrorResponse::auth( &addr.user, "SSPI authentication is not supported", @@ -294,7 +290,6 @@ impl Server { Ok(None) => { // Authentication should be complete if !ctx.is_complete() { - error!("GSSAPI negotiation incomplete but no token to send"); return Err(Error::ConnectionError(Box::new( ErrorResponse::auth( &addr.user, @@ -305,14 +300,12 @@ impl Server { // Continue to wait for Authentication::Ok } Err(e) => { - error!("GSSAPI negotiation failed: {}", e); return Err(Error::ConnectionError(Box::new( ErrorResponse::from_err(&e), ))); } } } else { - error!("Received GSSAPI continue without context"); return Err(Error::ConnectionError(Box::new(ErrorResponse::auth( &addr.user, "Received GSSAPI continue without context", diff --git a/pgdog/tests/backend_gssapi_test.rs b/pgdog/tests/backend_gssapi_test.rs index 94253c07..1ca737ca 100644 --- a/pgdog/tests/backend_gssapi_test.rs +++ b/pgdog/tests/backend_gssapi_test.rs @@ -70,23 +70,27 @@ fn test_backend_gssapi_target_principal() { assert_eq!(target, "postgres/192.168.1.1"); } -#[test] -fn test_ticket_manager_for_backend() { +#[tokio::test] +async fn test_ticket_manager_for_backend() { // Test that TicketManager can handle backend server tickets let manager = TicketManager::global(); // These will fail without real keytabs, but test the API - let ticket1 = manager.get_ticket( - "server1:5432", - "/etc/pgdog/server1.keytab", - "pgdog-server1@REALM", - ); - - let ticket2 = manager.get_ticket( - "server2:5432", - "/etc/pgdog/server2.keytab", - "pgdog-server2@REALM", - ); + let ticket1 = manager + .get_ticket( + "server1:5432", + "/etc/pgdog/server1.keytab", + "pgdog-server1@REALM", + ) + .await; + + let ticket2 = manager + .get_ticket( + "server2:5432", + "/etc/pgdog/server2.keytab", + "pgdog-server2@REALM", + ) + .await; // Both should fail without real keytabs assert!(ticket1.is_err()); diff --git a/pgdog/tests/gssapi_integration_test.rs b/pgdog/tests/gssapi_integration_test.rs index b7beca89..acc662de 100644 --- a/pgdog/tests/gssapi_integration_test.rs +++ b/pgdog/tests/gssapi_integration_test.rs @@ -7,8 +7,8 @@ use pgdog::auth::gssapi::{TicketCache, TicketManager}; use std::path::PathBuf; /// Test that TicketCache can acquire a credential from a keytab -#[test] -fn test_ticket_cache_acquires_credential() { +#[tokio::test] +async fn test_ticket_cache_acquires_credential() { // This test MUST FAIL initially because TicketCache doesn't exist yet let keytab_path = PathBuf::from("/etc/pgdog/test.keytab"); let principal = "test@EXAMPLE.COM"; @@ -24,24 +24,28 @@ fn test_ticket_cache_acquires_credential() { } /// Test that TicketManager maintains per-server caches -#[test] -fn test_ticket_manager_per_server_cache() { +#[tokio::test] +async fn test_ticket_manager_per_server_cache() { // This test MUST FAIL initially because TicketManager doesn't exist yet let manager = TicketManager::new(); // Get ticket for server1 - let ticket1 = manager.get_ticket( - "server1:5432", - "/etc/pgdog/keytab1.keytab", - "principal1@REALM", - ); + let ticket1 = manager + .get_ticket( + "server1:5432", + "/etc/pgdog/keytab1.keytab", + "principal1@REALM", + ) + .await; // Get ticket for server2 - let ticket2 = manager.get_ticket( - "server2:5432", - "/etc/pgdog/keytab2.keytab", - "principal2@REALM", - ); + let ticket2 = manager + .get_ticket( + "server2:5432", + "/etc/pgdog/keytab2.keytab", + "principal2@REALM", + ) + .await; assert!(ticket1.is_ok(), "Failed to get ticket for server1"); assert!(ticket2.is_ok(), "Failed to get ticket for server2"); @@ -84,15 +88,17 @@ async fn test_gssapi_frontend_authentication() { } /// Test ticket refresh mechanism -#[test] -fn test_ticket_refresh() { +#[tokio::test] +async fn test_ticket_refresh() { // This test demonstrates ticket refresh use std::time::Duration; let manager = TicketManager::new(); manager.set_refresh_interval(Duration::from_secs(1)); // Short interval for testing - let ticket = manager.get_ticket("server:5432", "/etc/pgdog/test.keytab", "test@REALM"); + let ticket = manager + .get_ticket("server:5432", "/etc/pgdog/test.keytab", "test@REALM") + .await; assert!(ticket.is_ok()); let initial_refresh_time = manager.get_last_refresh("server:5432");