introduce libvault as the certificate manager & refactor rks server #200
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces libvault as a certificate manager by implementing comprehensive authentication and credential management functionality. The changes include a complete AppRole authentication backend, token storage management, expiration handling, and HTTP metrics capabilities.
Key Changes
- Implementation of AppRole authentication backend with login, role management, and secret ID handling
- Addition of comprehensive token storage and management system with encryption and expiration
- Introduction of HTTP metrics collection and system monitoring capabilities
Reviewed Changes
Copilot reviewed 97 out of 168 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| project/libvault/src/modules/credential/approle/path_login.rs | Implements AppRole login authentication path with role validation and secret ID verification |
| project/libvault/src/modules/credential/approle/mod.rs | Core AppRole module with backend setup, role management, and extensive test coverage |
| project/libvault/src/modules/auth/token_store.rs | Token storage implementation with creation, lookup, revocation, and renewal capabilities |
| project/libvault/src/modules/auth/mod.rs | Authentication module manager with mount/unmount functionality and auth backend registry |
| project/libvault/src/modules/auth/expiration.rs | Lease expiration management with priority queue for tracking and automatic cleanup |
| project/libvault/src/module_manager.rs | Module lifecycle management system for organizing RustyVault components |
| project/libvault/src/metrics/*.rs | HTTP and system metrics collection using Prometheus with middleware integration |
| project/libvault/src/logical/*.rs | Core logical framework with backend traits, request/response handling, and field validation |
Comments suppressed due to low confidence (1)
project/libvault/src/modules/credential/approle/path_login.rs:1
- Corrected spelling from 'n token entry' to 'a token entry'.
use std::{collections::HashMap, mem, sync::Arc, time::SystemTime};
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mon3stera
force-pushed
the
dev
branch
2 times, most recently
from
6aa7b10 to
6fddf18
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 98 out of 178 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mon3stera
changed the title
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]> refactor(libvault): remove unused modules Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]> refactor(libvault): remove unused modules Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]> refactor(libvault): use tracing rather than env_logger Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]> refactor(libvault): refactor docker build and remove unused docs Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]> refactor(libvault): remove unused modules Signed-off-by: mon3stera <[email protected]> fix(rks): remove docker dir Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]> fix(libvault): restore kv module because of other modules Signed-off-by: mon3stera <[email protected]> # Conflicts: # project/Cargo.lock
Signed-off-by: mon3stera <[email protected]> � Conflicts: � project/rkl/Cargo.toml
Signed-off-by: mon3stera <[email protected]>
…ate tls Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 73 out of 218 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]> fix: truly fix buck2 build Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
mon3stera
changed the title
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
|
|
||
| pub fn recover_secret(shares: Vec<Vec<u8>>) -> Option<Vec<u8>> { | ||
| if shares.len() < 2 { | ||
| println!("Less than two parts cannot be used to reconstruct the secret"); |
There was a problem hiding this comment.
Replace println! with proper logging using the log crate (e.g., log::error!) for consistency with the rest of the codebase.
| if ciphertext[0] != 0 | ||
| || ciphertext[1] != 0 | ||
| || ciphertext[2] != 0 | ||
| || ciphertext[3] != KEY_EPOCH |
There was a problem hiding this comment.
[nitpick] Consider extracting the epoch validation logic into a separate helper function to improve readability and reusability.
| if self.segment_wildcard_paths.is_empty() { | ||
| return None; | ||
| } | ||
|
|
There was a problem hiding this comment.
Duplicate condition check at lines 320 and 324. Remove the redundant second check.
| if self.segment_wildcard_paths.is_empty() { | |
| return None; | |
| } |
| .unwrap() | ||
| .clone(); | ||
|
|
||
| info.clone() |
There was a problem hiding this comment.
Unnecessary clone at the end of function. Return info directly instead of info.clone().
| info.clone() | |
| info |
Signed-off-by: mon3stera <[email protected]>
Signed-off-by: mon3stera <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 73 out of 211 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
project/libvault/src/modules/policy/policy_store.rs:1
- Avoid using
unwrap()in library code. This should return a Result instead, as per the function signature. Replace with the?operator.
//! The `policy_store.rs` file manages the storage and retrieval of security policies. It provides
project/libvault/src/router.rs:1
- Duplicate check for
segment_wildcard_paths.is_empty(). Remove one of these redundant checks (lines 320-322 or 324-326).
//! The `libvault::router` module contains the functions that are used to do the routing work.
| .unwrap() | ||
| .clone(); | ||
|
|
||
| info.clone() |
There was a problem hiding this comment.
Unnecessary clone operation. The function already returns a cloned value from info.clone() on line 1147, so this additional clone is redundant. Simply return info instead.
| info.clone() | |
| info |
| match key_type { | ||
| "rsa" => { | ||
| if key_bits == 0 { | ||
| key_bits = 2048; | ||
| } | ||
|
|
||
| if key_bits != 2048 && key_bits != 3072 && key_bits != 4096 { | ||
| return Err(RvError::ErrPkiKeyBitsInvalid); | ||
| } | ||
| } | ||
| "ec" => { | ||
| if key_bits == 0 { | ||
| key_bits = 256; | ||
| } | ||
|
|
||
| if key_bits != 224 && key_bits != 256 && key_bits != 384 && key_bits != 512 { | ||
| return Err(RvError::ErrPkiKeyBitsInvalid); | ||
| } | ||
| } | ||
| #[cfg(feature = "crypto_adaptor_tongsuo")] | ||
| "sm2" => { | ||
| if key_bits == 0 { | ||
| key_bits = 256; | ||
| } | ||
|
|
||
| if key_bits != 256 { | ||
| return Err(RvError::ErrPkiKeyBitsInvalid); | ||
| } | ||
| } | ||
| _ => { | ||
| return Err(RvError::ErrPkiKeyTypeInvalid); | ||
| } | ||
| } |
There was a problem hiding this comment.
This key validation logic is duplicated from util.rs (lines 32-62). Extract this into a shared validation function to avoid code duplication and ensure consistency.
2 participants
servermodule into 7 different smaller modules. Altough these small functions cannot be reused, it will make code clearer and more readable.OnceLockso that we needn't to pass them around everywhere, which is ok because it will never be modified.It is a large update and refactor. Therefore, more reviews are needed.