feat(credential-manager): Scaffold credential manager service with base implementation. #1650
Conversation
…match table name used in Python intialization scripts.
WalkthroughA new credential manager microservice component is introduced to the workspace. The implementation includes configuration management, MySQL-backed persistence, HTTP API endpoints for credential operations (list/create/get/delete), health checks, token placeholder routes, JWT utilities, error handling with type-mapped service errors, and structured audit logging with tracing integration. Changes
Sequence DiagramssequenceDiagram
participant CLI as CLI
participant Main as main.rs
participant Tracer as Tracing
participant Config as AppConfig
participant Service as CredentialManagerService
participant DB as MySqlPool
participant Router as Axum Router
participant Listener as TcpListener
CLI->>Main: Start with config path
Main->>Tracer: init_tracing()
Main->>Config: load from YAML file
Config-->>Main: AppConfig
Main->>DB: MySqlConnectOptions → Pool
DB-->>Main: Connected pool
Main->>Service: new(config)
Service->>DB: Test connection
DB-->>Service: OK
Service-->>Main: CredentialManagerService
Main->>Router: build_router(service)
Router-->>Main: Router with routes
Main->>Listener: bind(socket_addr)
Listener-->>Main: TcpListener
Main->>Router: serve(listener)
Router->>Main: Listening on address (logged)
note over Router: Server ready
sequenceDiagram
participant Client as HTTP Client
participant Router as Axum Router
participant Handler as Route Handler
participant Service as CredentialManagerService
participant DB as Database
participant Audit as Audit Log
Client->>Router: POST /credentials (CreateCredentialRequest)
Router->>Handler: create_credential(request)
Handler->>Service: create_credential(request)
Service->>Service: validate()
alt Validation fails
Service-->>Handler: ServiceError::Validation
Handler->>Audit: log_event(failure)
else Validation succeeds
Service->>DB: insert_credential()
alt Insert succeeds
DB-->>Service: CredentialMetadata
Service->>Audit: log_event(success) with id/name
Service-->>Handler: CredentialMetadata
else Duplicate key (1062)
DB-->>Service: ServiceError::Conflict
Handler->>Audit: log_event(failure)
Service-->>Handler: ServiceError::Conflict
else Other DB error
DB-->>Service: ServiceError::Database
Handler->>Audit: log_event(failure)
end
end
Handler->>Router: ApiError or JSON response
Router-->>Client: HTTP 200 (JSON) or 4xx/5xx (ApiError)
sequenceDiagram
participant Client as HTTP Client
participant Router as Axum Router
participant Handler as Route Handler
participant Service as CredentialManagerService
participant DB as Database
participant Audit as Audit Log
Client->>Router: GET /credentials/:id
Router->>Handler: get_credential(id)
Handler->>Service: get_credential(id)
Service->>DB: sqlx SELECT by id
alt Row found
DB-->>Service: CredentialMetadataRow
Service->>Service: Convert to CredentialMetadata
Service->>Audit: log_event(success) with id/name
Service-->>Handler: CredentialMetadata
Handler-->>Client: HTTP 200 (JSON)
else Row not found
DB-->>Service: ServiceError::NotFound
Service->>Audit: log_event(failure)
Service-->>Handler: ServiceError::NotFound
Handler-->>Client: HTTP 404 (ApiError)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.toml(1 hunks)components/clp-credential-manager/Cargo.toml(1 hunks)components/clp-credential-manager/src/audit.rs(1 hunks)components/clp-credential-manager/src/config.rs(1 hunks)components/clp-credential-manager/src/db/mod.rs(1 hunks)components/clp-credential-manager/src/db/operations.rs(1 hunks)components/clp-credential-manager/src/db/schema.rs(1 hunks)components/clp-credential-manager/src/error.rs(1 hunks)components/clp-credential-manager/src/main.rs(1 hunks)components/clp-credential-manager/src/models/credential.rs(1 hunks)components/clp-credential-manager/src/models/mod.rs(1 hunks)components/clp-credential-manager/src/models/token.rs(1 hunks)components/clp-credential-manager/src/routes/credentials.rs(1 hunks)components/clp-credential-manager/src/routes/health.rs(1 hunks)components/clp-credential-manager/src/routes/mod.rs(1 hunks)components/clp-credential-manager/src/routes/tokens.rs(1 hunks)components/clp-credential-manager/src/service.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
components/clp-credential-manager/Cargo.tomlCargo.tomlcomponents/clp-credential-manager/src/main.rs
📚 Learning: 2025-10-22T21:01:31.391Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:01:31.391Z
Learning: In y-scope/clp, for the clp-rust-checks workflow (GitHub Actions) and the Taskfile target deps:lock:check-rust, we should verify Rust Cargo.lock is in sync with Cargo.toml using a non-mutating method (e.g., cargo metadata --locked / cargo check --locked) to keep CI deterministic and avoid updating dependencies during validation.
Applied to files:
components/clp-credential-manager/Cargo.toml
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
Applied to files:
components/clp-credential-manager/src/config.rscomponents/clp-credential-manager/src/main.rs
🧬 Code graph analysis (10)
components/clp-credential-manager/src/routes/mod.rs (3)
components/clp-credential-manager/src/routes/credentials.rs (1)
router(17-27)components/clp-credential-manager/src/routes/health.rs (1)
router(6-8)components/clp-credential-manager/src/routes/tokens.rs (1)
router(7-11)
components/clp-credential-manager/src/routes/tokens.rs (2)
components/clp-credential-manager/src/routes/credentials.rs (1)
router(17-27)components/clp-credential-manager/src/routes/health.rs (1)
router(6-8)
components/clp-credential-manager/src/routes/health.rs (2)
components/clp-credential-manager/src/routes/credentials.rs (1)
router(17-27)components/clp-credential-manager/src/routes/tokens.rs (1)
router(7-11)
components/clp-credential-manager/src/routes/credentials.rs (3)
components/clp-credential-manager/src/error.rs (3)
new(70-75)from(44-58)from(84-103)components/clp-credential-manager/src/service.rs (6)
new(36-53)new(184-199)list_credentials(71-86)create_credential(142-172)get_credential(89-106)delete_credential(109-133)components/clp-credential-manager/src/db/operations.rs (4)
list_credentials(11-23)create_credential(63-70)get_credential(26-44)delete_credential(47-55)
components/clp-credential-manager/src/models/credential.rs (2)
components/clp-credential-manager/src/error.rs (1)
fmt(124-126)components/clp-credential-manager/src/db/operations.rs (2)
sqlx(18-18)sqlx(33-33)
components/clp-credential-manager/src/service.rs (3)
components/clp-credential-manager/src/db/operations.rs (6)
sqlx(18-18)sqlx(33-33)list_credentials(11-23)get_credential(26-44)delete_credential(47-55)create_credential(63-70)components/clp-credential-manager/src/audit.rs (4)
success(42-53)log_event(71-93)failure(56-67)as_str(19-24)components/clp-credential-manager/src/models/credential.rs (1)
as_str(21-27)
components/clp-credential-manager/src/db/operations.rs (4)
components/clp-credential-manager/src/routes/credentials.rs (4)
list_credentials(31-36)get_credential(53-59)delete_credential(63-72)create_credential(40-49)components/clp-credential-manager/src/service.rs (4)
list_credentials(71-86)get_credential(89-106)delete_credential(109-133)create_credential(142-172)components/clp-credential-manager/src/models/credential.rs (2)
try_from(33-42)try_from(167-180)components/clp-credential-manager/src/error.rs (3)
db_err(50-50)from(44-58)from(84-103)
components/clp-credential-manager/src/db/mod.rs (2)
components/clp-credential-manager/src/db/operations.rs (4)
create_credential(63-70)delete_credential(47-55)get_credential(26-44)list_credentials(11-23)components/clp-credential-manager/src/service.rs (4)
create_credential(142-172)delete_credential(109-133)get_credential(89-106)list_credentials(71-86)
components/clp-credential-manager/src/error.rs (2)
components/clp-credential-manager/src/db/operations.rs (3)
sqlx(18-18)sqlx(33-33)db_err(118-118)components/clp-credential-manager/src/service.rs (2)
new(36-53)new(184-199)
components/clp-credential-manager/src/main.rs (3)
components/clp-credential-manager/src/routes/mod.rs (1)
build_router(14-20)components/clp-credential-manager/src/config.rs (1)
from_file(37-41)components/clp-credential-manager/src/service.rs (2)
new(36-53)new(184-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: package-image
- GitHub Check: rust-checks
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build (macos-15)
🔇 Additional comments (10)
components/clp-credential-manager/Cargo.toml (2)
1-28: Verify Cargo.lock is updated to maintain build determinism.Per repository guidelines for y-scope/clp, Cargo.lock must remain synchronized with Cargo.toml changes to ensure deterministic CI builds. Verify that this PR includes an updated Cargo.lock file reflecting the new dependency set.
Based on learnings
4-4: Fix invalid Rust edition—2024 does not exist.Rust currently supports editions 2015, 2018, and 2021. The edition must be set to "2021".
-edition = "2024" +edition = "2021"⛔ Skipped due to learnings
Learnt from: LinZhihao-723 Repo: y-scope/clp PR: 1448 File: components/clp-rust-utils/Cargo.toml:4-4 Timestamp: 2025-10-20T22:20:45.733Z Learning: Rust edition 2024 is a valid edition value in Cargo.toml files. It was released with Rust 1.85 on February 20, 2025.Learnt from: junhaoliao Repo: y-scope/clp PR: 0 File: :0-0 Timestamp: 2025-10-22T21:01:31.391Z Learning: In y-scope/clp, for the clp-rust-checks workflow (GitHub Actions) and the Taskfile target deps:lock:check-rust, we should verify Rust Cargo.lock is in sync with Cargo.toml using a non-mutating method (e.g., cargo metadata --locked / cargo check --locked) to keep CI deterministic and avoid updating dependencies during validation.Learnt from: junhaoliao Repo: y-scope/clp PR: 0 File: :0-0 Timestamp: 2025-10-22T21:02:31.113Z Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).Cargo.toml (1)
4-6: Workspace membership registration looks good.The new credential manager crate is correctly added to the workspace members list in alphabetical order, and the trailing comma maintains proper formatting.
components/clp-credential-manager/src/db/mod.rs (1)
1-4: Public db facade looks clean and cohesiveRe-exporting the core CRUD operations at the
dbmodule level gives a simple, well-scoped API for the service layer without leaking internal schema/operations details. No issues from a design or correctness perspective.components/clp-credential-manager/src/config.rs (1)
17-41: Config loading and defaulting are consistent with the rest of the service
AppConfig::from_filecleanly lifts IO/YAML failures intoServiceError,ServerConfigprovides sane defaults and a typedSocketAddr, and database/JWT configs are explicit with reasonable defaults for ports, pool size, and TTL. This should integrate smoothly with the service bootstrap paths you’ve wired inmain.rs/service.rs.Also applies to: 44-64, 83-94, 104-114
components/clp-credential-manager/src/routes/health.rs (1)
5-13: Health endpoint is simple and sufficient for readiness checksThe
/healthrouter andhealth_checkhandler are minimal, deterministic, and don’t depend on shared state or the DB, which is ideal for a basic liveness/readiness probe. No changes needed here.components/clp-credential-manager/src/routes/mod.rs (1)
1-20: Router composition and shared AppState look goodMerging the sub-routers and applying
TraceLayer::new_for_http()at the top level is a clean, idiomatic way to expose the service surface with sharedAppState.components/clp-credential-manager/src/models/mod.rs (1)
1-5: Model module wiring is cohesiveRe-exporting the credential and token types here gives a tidy public surface for consumers; this layout is clear and maintainable.
components/clp-credential-manager/src/models/token.rs (1)
1-23: TokenSubject modelling and helpers look appropriateThe tagged enum with snake_case variants, plus the
jobandserviceconvenience constructors, gives a clear and robust way to represent token subjects for jobs vs services.components/clp-credential-manager/src/routes/credentials.rs (1)
29-72: Concern Verified: CredentialMetadata safely excludes secret materialThe
CredentialMetadatastruct correctly contains metadata only—id, name, type, description, durations, flags, and timestamps. No sensitive fields (keys, tokens, passwords) are present. Service layer methods (list_credentials,get_credential,create_credential) consistently returnCredentialMetadata, and theTryFromconversion implementation maps only non-sensitive fields. The struct is explicitly documented as public-facing, and the design properly isolates credential metadata from any internal secret values.
| serde_yaml = "0.9.34" | ||
| sqlx = { version = "0.8.6", features = ["runtime-tokio", "mysql", "macros", "chrono"] } | ||
| thiserror = "2.0.17" | ||
| tokio = { version = "1.48.0", features = ["full"] } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace overly broad tokio "full" feature with explicitly needed features.
The "full" feature includes all runtime features (signal handling, process spawning, fs operations, net utilities, etc.), which increases compilation time and binary size. For a credential manager service using HTTP, async tasks, and time utilities, a more conservative set is preferable.
-tokio = { version = "1.48.0", features = ["full"] }
+tokio = { version = "1.48.0", features = ["rt-multi-thread", "macros", "time", "sync"] }If additional tokio features are required based on the actual service implementation, adjust accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tokio = { version = "1.48.0", features = ["full"] } | |
| tokio = { version = "1.48.0", features = ["rt-multi-thread", "macros", "time", "sync"] } |
🤖 Prompt for AI Agents
In components/clp-credential-manager/Cargo.toml around line 18, replace the
overly broad tokio feature = ["full"] with an explicit, minimal set required by
this service (e.g., runtime, macros and time utilities for async tasks and
timers, plus any IO/net/sync features used by your HTTP client/server); update
the features list to include only what the credential manager actually needs
(for example rt-multi-thread, macros, time and any of net/io-util/sync needed),
and run a build to add any missing features iteratively until compilation
succeeds.
| /// Emits the provided audit event to the tracing ecosystem using the `audit` target. | ||
| pub fn log_event(event: AuditEvent<'_>) { | ||
| let subject = event | ||
| .subject | ||
| .and_then(|subject| serde_json::to_value(subject).ok()) | ||
| .unwrap_or(Value::Null); | ||
| let source_ip = event | ||
| .source_ip | ||
| .map(|addr| addr.ip().to_string()) | ||
| .unwrap_or_default(); | ||
| let payload = json!({ | ||
| "component": COMPONENT_NAME, | ||
| "event_type": event.event_type, | ||
| "status": event.status.as_str(), | ||
| "credential_id": event.credential_id, | ||
| "credential_name": event.credential_name, | ||
| "subject": subject, | ||
| "source_ip": if source_ip.is_empty() { Value::Null } else { Value::String(source_ip) }, | ||
| "error": event.error, | ||
| "metadata": event.metadata.unwrap_or(Value::Null), | ||
| }); | ||
|
|
||
| info!(target = "audit", %payload); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid double‑encoding JSON in audit logs for easier downstream processing
Given that the global tracing setup uses JSON formatting, logging a serde_json::Value via info!(target = "audit", %payload) will produce a JSON log whose payload field is itself a JSON string. That’s workable, but downstream consumers need to parse JSON twice. You may want instead to log the fields as structured attributes (e.g., info!(target = "audit", component = COMPONENT_NAME, event_type = event.event_type, status = event.status.as_str(), credential_id = event.credential_id, ...)) so the tracing backend emits a single-layer JSON object.
🤖 Prompt for AI Agents
In components/clp-credential-manager/src/audit.rs around lines 70–93, the
current code builds a serde_json::Value payload and logs it with info!(target =
"audit", %payload), which double‑encodes JSON under the global JSON tracing
formatter; instead, log structured fields directly so the tracing layer emits a
single JSON object. Replace the single %payload argument with named fields
(component, event_type, status, credential_id, credential_name, subject,
source_ip, error, metadata), converting Option/IpAddr to strings or
serde_json::Value as needed (use Value::Null for missing subject/metadata and an
empty string or null for missing source_ip), and pass them as structured key =
value pairs to info!(target = "audit", ...). Ensure no one call formats the
entire payload as a string.
| /// Returns all credential metadata sorted alphabetically by name. | ||
| pub async fn list_credentials(pool: &MySqlPool) -> ServiceResult<Vec<CredentialMetadata>> { | ||
| let query = format!( | ||
| "SELECT {columns} FROM {table} ORDER BY name", | ||
| columns = AWS_CREDENTIAL_METADATA_COLUMNS, | ||
| table = AWS_CREDENTIALS_TABLE | ||
| ); | ||
|
|
||
| let rows = sqlx::query_as::<_, CredentialMetadataRow>(&query) | ||
| .fetch_all(pool) | ||
| .await?; | ||
|
|
||
| rows.into_iter().map(CredentialMetadata::try_from).collect() | ||
| } | ||
|
|
||
| /// Fetches a single credential row by ID. | ||
| pub async fn get_credential(pool: &MySqlPool, id: i64) -> ServiceResult<CredentialMetadata> { | ||
| let query = format!( | ||
| "SELECT {columns} FROM {table} WHERE id = ?", | ||
| columns = AWS_CREDENTIAL_METADATA_COLUMNS, | ||
| table = AWS_CREDENTIALS_TABLE | ||
| ); | ||
|
|
||
| let row = sqlx::query_as::<_, CredentialMetadataRow>(&query) | ||
| .bind(id) | ||
| .fetch_optional(pool) | ||
| .await?; | ||
|
|
||
| match row { | ||
| Some(row) => CredentialMetadata::try_from(row), | ||
| None => Err(ServiceError::NotFound(format!( | ||
| "credential with id `{id}` was not found" | ||
| ))), | ||
| } | ||
| } | ||
|
|
||
| /// Deletes the credential row and returns the affected row count. | ||
| pub async fn delete_credential(pool: &MySqlPool, id: i64) -> ServiceResult<u64> { | ||
| let query = format!( | ||
| "DELETE FROM {table} WHERE id = ?", | ||
| table = AWS_CREDENTIALS_TABLE | ||
| ); | ||
|
|
||
| let result = sqlx::query(&query).bind(id).execute(pool).await?; | ||
| Ok(result.rows_affected()) | ||
| } | ||
|
|
||
| /// Persists a new credential then fetches the stored metadata. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// * Propagates validation/persistence failures from [`insert_credential`]. | ||
| /// * Propagates errors from [`get_credential`] when reloading the inserted row. | ||
| pub async fn create_credential( | ||
| pool: &MySqlPool, | ||
| request: &CreateCredentialRequest, | ||
| ) -> ServiceResult<CredentialMetadata> { | ||
| let result = insert_credential(pool, request).await?; | ||
| let inserted_id = result.last_insert_id() as i64; | ||
| get_credential(pool, inserted_id).await | ||
| } | ||
|
|
||
| /// Raw insert helper that maps low-level driver errors into [`ServiceError`] variants. | ||
| async fn insert_credential( | ||
| pool: &MySqlPool, | ||
| request: &CreateCredentialRequest, | ||
| ) -> ServiceResult<MySqlQueryResult> { | ||
| let query = format!( | ||
| "INSERT INTO {table} (name, credential_type, access_key_id, secret_access_key, role_arn, \ | ||
| external_id, description, default_session_duration_seconds, transient, created_by) \ | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", | ||
| table = AWS_CREDENTIALS_TABLE | ||
| ); | ||
|
|
||
| let name = request.name.as_str(); | ||
| let credential_type = request.credential_type.as_str(); | ||
| let access_key_id = request.access_key_id.as_deref(); | ||
| let secret_access_key = request | ||
| .secret_access_key | ||
| .as_ref() | ||
| .map(|secret| secret.expose_secret().to_owned()); | ||
| let role_arn = request.role_arn.as_deref(); | ||
| let external_id = request.external_id.as_deref(); | ||
| let description = request.description.as_deref(); | ||
| let default_duration = request.default_session_duration_seconds.unwrap_or(3600); | ||
| let transient = request.transient.unwrap_or(false); | ||
| let created_by = request.created_by.as_deref(); | ||
|
|
||
| sqlx::query(&query) | ||
| .bind(name) | ||
| .bind(credential_type) | ||
| .bind(access_key_id) | ||
| .bind(secret_access_key.as_deref()) | ||
| .bind(role_arn) | ||
| .bind(external_id) | ||
| .bind(description) | ||
| .bind(default_duration) | ||
| .bind(transient) | ||
| .bind(created_by) | ||
| .execute(pool) | ||
| .await | ||
| .map_err(|err| map_insert_error(err, name)) | ||
| } | ||
|
|
||
| /// Converts insert failures into domain-specific conflicts when possible. | ||
| fn map_insert_error(err: sqlx::Error, name: &str) -> ServiceError { | ||
| let is_duplicate = err | ||
| .as_database_error() | ||
| .and_then(|db_err| db_err.try_downcast_ref::<sqlx::mysql::MySqlDatabaseError>()) | ||
| .is_some_and(|mysql_err| mysql_err.number() == 1062); | ||
|
|
||
| if is_duplicate { | ||
| return ServiceError::Conflict(format!("credential with name `{}` already exists", name)); | ||
| } | ||
|
|
||
| ServiceError::from(err) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Metadata‑only queries and insert mapping look good; consider static SQL for compile‑time checks
The operations correctly:
- Restrict SELECTs to metadata columns (no secrets are read back).
- Use bound parameters for all user input, avoiding SQL injection.
- Map duplicate‑name insert failures to a domain‑level
Conflict.
One improvement to consider is replacing the format!‑built SQL strings with static query strings and sqlx macros (e.g., query_as!), which would give you compile‑time validation of column sets and types and avoid string allocation at runtime. That’s especially helpful as the schema evolves.
| /// Canonical table name for the unified AWS credentials store. | ||
| pub const AWS_CREDENTIALS_TABLE: &str = "clp_aws_credentials"; | ||
|
|
||
| /// Columns selected by metadata queries to avoid leaking secrets. | ||
| pub const AWS_CREDENTIAL_METADATA_COLUMNS: &str = "id, name, credential_type, description, \n \ | ||
| default_session_duration_seconds, transient, \ | ||
| created_at, updated_at, last_used_at"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Column list constant embeds a newline/indent; harmless but slightly surprising
The AWS_CREDENTIAL_METADATA_COLUMNS string literal includes a \n and indentation spaces, which is fine for SQL (whitespace is ignored) but might be unexpected if this constant is ever reused in a non-SQL context. Consider collapsing this to a single-line literal or using concat! for readability and to avoid embedded formatting artefacts.
🤖 Prompt for AI Agents
In components/clp-credential-manager/src/db/schema.rs around lines 1 to 7, the
AWS_CREDENTIAL_METADATA_COLUMNS constant embeds a `\n` and indentation spaces
which can introduce unexpected whitespace if reused; replace the
multi-line/escaped-newline string with a single-line literal containing the
comma-separated column names (or use concat! to build a single-line string) so
the constant contains no embedded newlines or leading spaces, preserving the
same column list but collapsing it into a clean single-line value for
readability and reuse.
| impl From<sqlx::Error> for ServiceError { | ||
| fn from(err: sqlx::Error) -> Self { | ||
| if matches!(err, sqlx::Error::RowNotFound) { | ||
| return ServiceError::NotFound("requested record was not found".to_owned()); | ||
| } | ||
|
|
||
| if let Some(db_err) = err.as_database_error() { | ||
| if let Some(mysql_err) = db_err.try_downcast_ref::<sqlx::mysql::MySqlDatabaseError>() { | ||
| if mysql_err.number() == 1062 { | ||
| return ServiceError::Conflict(mysql_err.message().to_owned()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ServiceError::Database(err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid leaking low-level backend errors to API clients; consider more user-facing messages and statuses
Right now ServiceError::Conflict and ServiceError::Database (and the Jwt/Io/Yaml variants) are turned into ApiError with source.to_string() (or mysql_err.message()) exposed directly to callers. For a credential service, this can leak internal schema details (constraint/index names, SQL text, filesystem paths, JWT library wording, etc.) and couples the public API tightly to backend implementations.
It may be safer to:
- Normalise DB conflicts and generic DB errors into short, user-facing strings, and keep the original
sqlx::Erroronly in logs/tracing. - Map common JWT failures (invalid/expired token) to
401 UNAUTHORIZED(or400 BAD_REQUEST) with a generic message instead of500.
For example:
impl From<ServiceError> for ApiError {
fn from(err: ServiceError) -> Self {
match err {
- ServiceError::Validation(msg) => ApiError::new(StatusCode::BAD_REQUEST, msg),
- ServiceError::Conflict(msg) => ApiError::new(StatusCode::CONFLICT, msg),
- ServiceError::NotFound(msg) => ApiError::new(StatusCode::NOT_FOUND, msg),
- ServiceError::Config(msg) => ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, msg),
- ServiceError::Database(source) => {
- ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, source.to_string())
- }
- ServiceError::Jwt(source) => {
- ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, source.to_string())
- }
- ServiceError::Io(source) => {
- ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, source.to_string())
- }
- ServiceError::Yaml(source) => {
- ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, source.to_string())
- }
+ ServiceError::Validation(msg) => ApiError::new(StatusCode::BAD_REQUEST, msg),
+ ServiceError::Conflict(_msg) => {
+ ApiError::new(StatusCode::CONFLICT, "resource already exists")
+ }
+ ServiceError::NotFound(msg) => ApiError::new(StatusCode::NOT_FOUND, msg),
+ ServiceError::Config(_msg) => {
+ ApiError::internal("service configuration error")
+ }
+ ServiceError::Database(_source) => {
+ ApiError::internal("database operation failed")
+ }
+ ServiceError::Jwt(_source) => {
+ ApiError::new(StatusCode::UNAUTHORIZED, "invalid or expired token")
+ }
+ ServiceError::Io(_source) => ApiError::internal("internal I/O error"),
+ ServiceError::Yaml(_source) => ApiError::internal("configuration parse error"),
}
}
}Similarly, you could stop propagating the raw MySQL error message for code 1062 in the From<sqlx::Error> impl and instead use a generic conflict message or have the higher-level operation craft a domain-specific one.
Also applies to: 83-102
🏁 Script executed:
#!/bin/bash
# Locate and examine the ApiError implementation and From<ServiceError> conversion
rg -n "struct ApiError|impl From<ServiceError>" components/clp-credential-manager/src/error.rs -A 30Length of output: 1937
🏁 Script executed:
#!/bin/bash
# Search for ApiError definition in the entire module
fd -e rs -x rg -l "struct ApiError|impl ApiError" {} \; | head -5Length of output: 48
🏁 Script executed:
#!/bin/bash
# Examine the full error.rs file to understand the complete error handling structure
cat components/clp-credential-manager/src/error.rsLength of output: 3708
🏁 Script executed:
#!/bin/bash
# Find endpoints/handlers that use ServiceError or ApiError
rg -n "ServiceError|ApiError" components/clp-credential-manager/src/ --type rs -B 2 | grep -E "(fn |pub async|ServiceError|ApiError)" | head -40Length of output: 26
🏁 Script executed:
#!/bin/bash
# Check if there are any HTTP handlers returning ServiceError
fd -e rs -path "*/src/*" | xargs grep -l "handler\|route\|endpoint" | head -5Length of output: 168
Sanitize backend error messages before exposing to API clients; map JWT errors to 401/400
The current implementation leaks low-level backend details to API callers, creating an information disclosure vulnerability:
- Lines 90–91 (
ServiceError::Database): Rawsqlx::Errormessages expose database schema details, query structure, or connection issues. - Line 87 (
ServiceError::Conflict): Raw MySQL error from line 50 exposes constraint/index names (e.g., "Duplicate entry for unique key 'email'"). - Lines 93–94 (
ServiceError::Jwt): JWT library errors with 500 status leak token parsing internals; should return 401 or 400 with a generic message. - Lines 96–99 (
ServiceError::Io,Yaml): File paths, YAML parse locations, and system-level details exposed via.to_string().
Replace raw error messages with generic, user-facing strings; log original errors server-side:
impl From<ServiceError> for ApiError {
fn from(err: ServiceError) -> Self {
match err {
ServiceError::Validation(msg) => ApiError::new(StatusCode::BAD_REQUEST, msg),
- ServiceError::Conflict(msg) => ApiError::new(StatusCode::CONFLICT, msg),
+ ServiceError::Conflict(_) => {
+ ApiError::new(StatusCode::CONFLICT, "resource already exists")
+ }
ServiceError::NotFound(msg) => ApiError::new(StatusCode::NOT_FOUND, msg),
ServiceError::Config(_msg) => {
- ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, msg)
+ ApiError::internal("service configuration error")
}
ServiceError::Database(_source) => {
- ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, source.to_string())
+ ApiError::internal("database operation failed")
}
ServiceError::Jwt(_source) => {
- ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, source.to_string())
+ ApiError::new(StatusCode::UNAUTHORIZED, "invalid or expired token")
}
- ServiceError::Io(_source) => {
- ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, source.to_string())
- }
+ ServiceError::Io(_) => ApiError::internal("internal I/O error"),
- ServiceError::Yaml(_source) => {
- ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, source.to_string())
- }
+ ServiceError::Yaml(_) => ApiError::internal("configuration parse error"),
}
}
}Also update line 50 to use a generic conflict message instead of propagating mysql_err.message().
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/clp-credential-manager/src/error.rs around lines 43–58 (and
related ServiceError variants referenced in the review), the code currently
returns raw backend error strings to API clients; change it to return generic,
user-facing messages and log the original error server-side. Specifically:
replace returning ServiceError::Database(err) with a generic
ServiceError::Database("internal database error".to_owned()) while logging the
original err (e.g., log::error!("database error: {:?}", err)); replace the MySQL
duplicate-key branch to return ServiceError::Conflict("resource
conflict".to_owned()) and log mysql_err; ensure ServiceError::Jwt maps token
validation/parsing failures to appropriate client-level errors (401 Unauthorized
for invalid/missing token or 400 Bad Request for malformed token) using generic
messages like "invalid authentication token" and log the detailed jwt error;
likewise replace ServiceError::Io and ServiceError::Yaml to return generic
messages ("internal IO error", "invalid configuration") and log the underlying
errors; keep logs detailed but never expose underlying messages in the returned
ServiceError strings.
| use axum::{ | ||
| Json, | ||
| Router, | ||
| extract::{Path, State}, | ||
| http::StatusCode, | ||
| routing::get, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Missing post import will break compilation
The router uses post(create_credential) but post is not imported from axum::routing, so this module will not compile.
-use axum::{
- Json,
- Router,
- extract::{Path, State},
- http::StatusCode,
- routing::get,
-};
+use axum::{
+ Json,
+ Router,
+ extract::{Path, State},
+ http::StatusCode,
+ routing::{get, post},
+};🏁 Script executed:
#!/bin/bash
# Verify the actual contents of credentials.rs and check for post import and usage
cat components/clp-credential-manager/src/routes/credentials.rsLength of output: 1938
🏁 Script executed:
#!/bin/bash
# Search for post( usage in the credentials.rs file
rg -n "post\s*\(" components/clp-credential-manager/src/routes/credentials.rsLength of output: 61
Missing post and delete imports will break compilation
The router uses .post(create_credential) and .delete(delete_credential) but neither post nor delete are imported from axum::routing, causing compilation to fail.
use axum::{
Json,
Router,
extract::{Path, State},
http::StatusCode,
- routing::get,
+ routing::{delete, get, post},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use axum::{ | |
| Json, | |
| Router, | |
| extract::{Path, State}, | |
| http::StatusCode, | |
| routing::get, | |
| }; | |
| use axum::{ | |
| Json, | |
| Router, | |
| extract::{Path, State}, | |
| http::StatusCode, | |
| routing::{delete, get, post}, | |
| }; |
🤖 Prompt for AI Agents
In components/clp-credential-manager/src/routes/credentials.rs around lines 1 to
7, the router uses .post(create_credential) and .delete(delete_credential) but
only get is imported from axum::routing; add the missing routing imports so
compilation succeeds: update the use axum::routing import to include post and
delete (e.g., import routing::{get, post, delete}) or add separate imports for
post and delete so the router functions resolve.
| /// Registers CRUD endpoints for long-term credentials. | ||
| pub fn router() -> Router<AppState> { | ||
| Router::new() | ||
| .route( | ||
| "/credentials", | ||
| get(list_credentials).post(create_credential), | ||
| ) | ||
| .route( | ||
| "/credentials/:id", | ||
| get(get_credential).delete(delete_credential), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Router shape is sensible; consider 201 for resource creation
The CRUD paths and method mapping look good. For POST /credentials, you may want to return 201 Created (and possibly a Location header) instead of 200 OK to better reflect that a new resource was created.
🤖 Prompt for AI Agents
In components/clp-credential-manager/src/routes/credentials.rs around lines 16
to 27, the POST /credentials route currently leads to a 200 OK response; update
the create_credential handler to return 201 Created and set a Location header
pointing to the new credential resource (e.g. /credentials/{id}) when creation
succeeds, and ensure any testing/response types are adjusted accordingly so
callers receive the status and header instead of 200.
| use axum::{Json, Router, http::StatusCode, routing::post}; | ||
| use serde::Serialize; | ||
|
|
||
| use super::AppState; | ||
|
|
||
| /// Temporary placeholder router until token issuance/exchange is implemented. | ||
| pub fn router() -> Router<AppState> { | ||
| Router::new() | ||
| .route("/credentials/issue-token", post(not_implemented)) | ||
| .route("/credentials/exchange-token", post(not_implemented)) | ||
| } | ||
|
|
||
| #[derive(Serialize)] | ||
| struct PlaceholderResponse { | ||
| message: &'static str, | ||
| } | ||
|
|
||
| /// Returns a 501 response indicating the endpoint is still under construction. | ||
| async fn not_implemented() -> (StatusCode, Json<PlaceholderResponse>) { | ||
| ( | ||
| StatusCode::NOT_IMPLEMENTED, | ||
| Json(PlaceholderResponse { | ||
| message: "token endpoints are not implemented yet", | ||
| }), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Placeholder token routes are fine; consider future response-shape alignment
Using 501 with a small JSON payload is reasonable for a scaffold. Once the real token logic is added, it would be good to align these endpoints with the service’s standard JSON envelope / error type so clients see a consistent response shape across all routes.
🤖 Prompt for AI Agents
In components/clp-credential-manager/src/routes/tokens.rs around lines 1 to 26,
the placeholder endpoints currently return a bare JSON message which will break
client expectations; change the response to use the service’s standard JSON
envelope/error type (import the shared response or error struct used elsewhere
in the project), adapt PlaceholderResponse to that type (populate the usual
fields such as message, error code or details as required by the envelope), keep
returning StatusCode::NOT_IMPLEMENTED, and update the function signature and
Json wrapper to use the shared type so the endpoints match the application's
consistent response shape.
| pub async fn new(config: &AppConfig) -> ServiceResult<Self> { | ||
| let db_config = &config.database; | ||
| let options = sqlx::mysql::MySqlConnectOptions::new() | ||
| .host(&db_config.host) | ||
| .port(db_config.port) | ||
| .database(&db_config.name) | ||
| .username(&db_config.user) | ||
| .password(db_config.password.expose_secret()); | ||
|
|
||
| let pool = MySqlPoolOptions::new() | ||
| .max_connections(db_config.max_connections) | ||
| .connect_with(options) | ||
| .await?; | ||
|
|
||
| let jwt = JwtManager::new(&config.jwt)?; | ||
|
|
||
| Ok(Self { db_pool: pool, jwt }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DB pool setup is fine; optional hardening via explicit timeouts
The database pool initialisation is straightforward and matches the config structure. For resilience, you may eventually want to surface connect_timeout / acquire_timeout (and related options) from MySqlPoolOptions into your config so a misbehaving database doesn’t block startup or request handling longer than intended.
🤖 Prompt for AI Agents
In components/clp-credential-manager/src/service.rs around lines 36 to 53, the
DB pool is created without any timeout or lifetime settings; update the code and
config to accept/connect pool timeouts and lifetimes (e.g. connect_timeout,
acquire_timeout, max_lifetime, idle_timeout) and apply them to MySqlPoolOptions
when building the pool. Add corresponding optional Duration or integer fields to
AppConfig.database (with sensible defaults), convert them to std::time::Duration
(e.g. Duration::from_secs) and call the matching MySqlPoolOptions methods
(connect_timeout, acquire_timeout, max_lifetime, idle_timeout) before
connect_with so the service won’t block indefinitely on DB operations.
| /// Fetches every credential metadata row and records the attempt in audit logs. | ||
| pub async fn list_credentials(&self) -> ServiceResult<Vec<CredentialMetadata>> { | ||
| match db::list_credentials(self.db_pool()).await { | ||
| Ok(credentials) => { | ||
| let mut event = AuditEvent::success("credential_list"); | ||
| event.metadata = Some(json!({ "count": credentials.len() })); | ||
| audit::log_event(event); | ||
| Ok(credentials) | ||
| } | ||
| Err(err) => { | ||
| let error_message = err.to_string(); | ||
| let event = AuditEvent::failure("credential_list", &error_message); | ||
| audit::log_event(event); | ||
| Err(err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Retrieves a single credential and enriches audit records with identifiers. | ||
| pub async fn get_credential(&self, id: i64) -> ServiceResult<CredentialMetadata> { | ||
| match db::get_credential(self.db_pool(), id).await { | ||
| Ok(credential) => { | ||
| let mut event = AuditEvent::success("credential_get"); | ||
| event.credential_id = Some(credential.id); | ||
| event.credential_name = Some(&credential.name); | ||
| audit::log_event(event); | ||
| Ok(credential) | ||
| } | ||
| Err(err) => { | ||
| let error_message = err.to_string(); | ||
| let mut event = AuditEvent::failure("credential_get", &error_message); | ||
| event.credential_id = Some(id); | ||
| audit::log_event(event); | ||
| Err(err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Attempts to delete a credential and distinguishes between success and missing rows. | ||
| pub async fn delete_credential(&self, id: i64) -> ServiceResult<()> { | ||
| let rows = db::delete_credential(self.db_pool(), id).await; | ||
| match rows { | ||
| Ok(rows_affected) if rows_affected > 0 => { | ||
| let mut event = AuditEvent::success("credential_delete"); | ||
| event.credential_id = Some(id); | ||
| audit::log_event(event); | ||
| Ok(()) | ||
| } | ||
| Ok(_) => { | ||
| let error_message = format!("credential with id `{id}` was not found"); | ||
| let mut event = AuditEvent::failure("credential_delete", &error_message); | ||
| event.credential_id = Some(id); | ||
| audit::log_event(event); | ||
| Err(ServiceError::NotFound(error_message)) | ||
| } | ||
| Err(err) => { | ||
| let error_message = err.to_string(); | ||
| let mut event = AuditEvent::failure("credential_delete", &error_message); | ||
| event.credential_id = Some(id); | ||
| audit::log_event(event); | ||
| Err(err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Validates, persists, and audits a new credential request. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// * Returns [`ServiceError::Validation`] when the input fails | ||
| /// [`CreateCredentialRequest::validate`]. | ||
| /// * Propagates persistence failures from [`db::create_credential`]. | ||
| pub async fn create_credential( | ||
| &self, | ||
| request: CreateCredentialRequest, | ||
| ) -> ServiceResult<CredentialMetadata> { | ||
| if let Err(err) = request.validate() { | ||
| let error_message = err.to_string(); | ||
| let event = AuditEvent::failure("credential_create", &error_message); | ||
| audit::log_event(event); | ||
| return Err(err); | ||
| } | ||
|
|
||
| match db::create_credential(self.db_pool(), &request).await { | ||
| Ok(created) => { | ||
| let mut event = AuditEvent::success("credential_create"); | ||
| event.credential_id = Some(created.id); | ||
| event.credential_name = Some(&created.name); | ||
| event.metadata = Some(json!({ | ||
| "credential_type": created.credential_type.as_str(), | ||
| "transient": created.transient, | ||
| })); | ||
| audit::log_event(event); | ||
| Ok(created) | ||
| } | ||
| Err(err) => { | ||
| let error_message = err.to_string(); | ||
| let event = AuditEvent::failure("credential_create", &error_message); | ||
| audit::log_event(event); | ||
| Err(err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Audit usage is consistent; ensure error strings don’t leak sensitive or low‑level details
The CRUD methods cleanly integrate with the audit helper and avoid logging secrets, which is good. The failure paths, however, funnel err.to_string() directly into AuditEvent::failure, which means whatever Display for ServiceError (and any wrapped causes) emits will end up in audit logs. It’s worth double‑checking that these messages are suitably high‑level and do not expose SQL snippets, connection details, or other sensitive internals; if they do, consider mapping errors to coarser reason codes or sanitised messages before logging.
🤖 Prompt for AI Agents
components/clp-credential-manager/src/service.rs lines 70-172: the review warns
that calling err.to_string() and stuffing that into AuditEvent::failure may leak
sensitive/low-level details; update all failure branches to map ServiceError
(and underlying errors) to a sanitized high-level message or reason code before
passing to AuditEvent::failure (e.g., match on error variants and produce short
labels like "validation_failed", "not_found", "database_error", or include only
the public error message for validation errors), use that sanitized_message for
the audit event while still returning the original err to the caller so behavior
is unchanged.
1 participant
Description
This PR adds the initial implementation for the
clp-credential-managerservice.Implemented:
clp-credential-managercrate with Axum server bootstrap, JSON logging, SQLx MySQL pool wiring, and shared service layer that wraps audit logging plus credential metadata CRUD.db::operationsplus models/validation for IAM user and IAM role credentials, returning metadata only so secrets never leave storage, and expose REST handlers for GET/POST/credentialsand GET/DELETE/credentials/:id.TODO:
credential-manageoperations.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.