Implement a secure key storage #329
Conversation
…mm-mediator-rs into secure-key-storage
Hermann-Core
requested review from
IngridPuppet,
Christiantyemele,
Ogenbertrand,
Blindspot22,
Ngha-Boris and
ndefokou
| .env | ||
| .env.example | ||
|
|
There was a problem hiding this comment.
.env.example is not intended to be ignored.
There was a problem hiding this comment.
I don't think that we require it anymore since it is constructed dynamically in the pipeline for the tests.
| # Stage 1: Build | ||
| FROM rust:1.80-alpine AS builder | ||
| FROM rust:1.83-alpine AS builder | ||
|
|
There was a problem hiding this comment.
Please limit your changes to the requirements of this ticket.
There was a problem hiding this comment.
One dependency that I used required a higher Rust version. So I've updated the builder image version to follow that requirement.
| #[error("decryption error")] | ||
| DecryptionError, | ||
| #[error("error deserializing to jwk")] | ||
| JwkDeserializationError, |
There was a problem hiding this comment.
Did you mean to leave this behind?
There was a problem hiding this comment.
I don't get it.
| /// Create a new key store with mocked repository and encryption backends. | ||
| /// This will be use for testing purposes. | ||
| #[cfg(any(test, feature = "test-utils"))] | ||
| pub fn with_mock_configs<T>(secrets: Vec<(String, T)>) -> Self | ||
| where | ||
| T: Serialize + DeserializeOwned + Send + Sync + 'static, | ||
| { | ||
| let mock_repository = MockSecretRepository::new(secrets); | ||
| let encryptor = NoEncryption; | ||
| Self { | ||
| repository: Arc::new(mock_repository), | ||
| encryptor: Arc::new(encryptor), | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you consider moving this logic to the tests module altogether?
| #[derive(Default)] | ||
| pub struct MockSecretRepository<T> | ||
| where | ||
| T: Serialize + DeserializeOwned, | ||
| { | ||
| secrets: RwLock<Vec<(String, T)>>, | ||
| } |
There was a problem hiding this comment.
You really mean to write mocks with locks? How necessary is that?
There was a problem hiding this comment.
I want interior mutability on the vector since the methods of the SecretRepository trait take self by reference. I could have used something cheaper like RefCell but the tests don't run for long so I think it is fine.
| None => Err(Error::msg( | ||
| ErrorKind::RepositoryFailure, | ||
| format!("Secret with kid {kid} not found"), | ||
| )), |
There was a problem hiding this comment.
Is this behavior matching the concrete repository impl? I think that was removed and not finding the resource not considered an error.
There was a problem hiding this comment.
Just checked the delete function of the Repository and you are right. Let me fix that.
But anyways it is just for the test. In real scenarios, it will not throw an error.
|
|
||
| #[tokio::test] | ||
| async fn test_keystore_flow() { | ||
| let jwk1 = secret1(); | ||
| let jwk2 = secret2(); |
There was a problem hiding this comment.
I'm surprised you aren't also testing that keys are not persisted in clear. I could not find tests at the encryptor module layer either.
There was a problem hiding this comment.
Hmm right. I only wanted to test the keystore flow. But I can also provide a mock encryptor.
| /// * [`CircuitBreaker::retries(self, max_retries: usize)`]: Sets the maximum number of consecutive failures allowed before the circuit opens. | ||
| /// A value of 0 means the circuit will open on the first failure. | ||
| /// * [`half_open_max_failures(self, max_retries: usize)`](half_open_max_failures): Sets the maximum number of attempts in half-open state | ||
| /// * [`CircuitBreaker::half_open_max_failures(self, max_retries: usize)`]: Sets the maximum number of attempts in half-open state | ||
| /// before reopening the circuit. | ||
| /// * [`reset_timeout(self, reset_timeout: Duration)`](timeout): Sets the duration the circuit remains open after tripping. | ||
| /// * [`CircuitBreaker::reset_timeout(self, reset_timeout: Duration)`]: Sets the duration the circuit remains open after tripping. | ||
| /// After this timeout, the circuit transitions to the half-open state. | ||
| /// * [`exponential_backoff(self, initial_delay: Duration)`](exponential_backoff): Configures an exponential backoff strategy for retries. | ||
| /// * [`CircuitBreaker::exponential_backoff(self, initial_delay: Duration)`]: Configures an exponential backoff strategy for retries. | ||
| /// The delay between retries increases exponentially. This overrides any previously set backoff. | ||
| /// * [`constant_backoff(self, delay: Duration)`](constant_backoff): Configures a constant backoff strategy for retries. | ||
| /// * [`CircuitBreaker::constant_backoff(self, delay: Duration)`]: Configures a constant backoff strategy for retries. |
There was a problem hiding this comment.
Please refrain from adding changes not related to your PR. It leaves the reviewer confused, especially when they are not qualified to review them.
There was a problem hiding this comment.
We added a step in the CI to check the docs too. These changes have been done to make the pipeline happy.
Testing would have been very hard that way. Plus we can decide to change the storage backend later, so it will be easy to switch to a new one without breaking anything. This keystore impl could be used for further projects too, not only for the mediator. |
No description provided.