Conversation
* rustfmt
| // matching PSK identity | ||
| let session_name = b"test_session".to_vec(); | ||
| let psk_identity = PskIdentity::new(&session_name, &epoch_secret).unwrap(); | ||
| secret_state.find_match(psk_identity.clone()).unwrap(); |
There was a problem hiding this comment.
Do you want to assert anything on the returned PSK? It looks like you're not doing anything with what you returned.
There was a problem hiding this comment.
s2n-tls doesn't expose any getters for PSKs
| let current_epoch = epoch_schedule::current_epoch(); | ||
|
|
||
| let update = secret_state | ||
| .fetch_secrets(&kms_client, current_epoch, &failure_notification) | ||
| .await; | ||
| if update.is_err() { | ||
| anyhow::bail!("failed to fetch keys during startup"); | ||
| } |
There was a problem hiding this comment.
You technically don't need these lines since you just do the same thing in the async block. But I suppose you're wanting it for the error message?
There was a problem hiding this comment.
Ya, we want to ensure that a successful call to initialize means that all of the epoch keys have been successfully fetched.
If we just relied on the async block, then we might say that we had successfully initialized even though we have only fetched a single secret.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod secret_state_tests { |
There was a problem hiding this comment.
What about a test that the psk provider/receiver fetch generates the same thing? I mean, seems like the only difference is that the server fetches one more epoch than the client. I'm mostly interested in testing that the hmac output are the same.
There was a problem hiding this comment.
Hmm, it's gonna be a bit messy, because the secret state is internal so there isn't any place that we can get both the internal state of the receiver and the internal state of the provider, unless we add test-only getters.
More generally, I feel comfortable relying on the handshake tests and unit tests to assert that the results are individually what we want.
If we want more tests, I think I'd push for abstracting the unix timestamp into a global clock that we can mock, and using the tokio fast-forward utilities to skip time.
Co-authored-by: maddeleine <59030281+maddeleine@users.noreply.github.com>
Co-authored-by: maddeleine <59030281+maddeleine@users.noreply.github.com>
| struct ReceiverSecrets { | ||
| smoothing_factor: Duration, | ||
| pub trusted_key_arns: Vec<KeyArn>, | ||
| pub epoch_secrets: RwLock<HashMap<u64, HashMap<KeyArn, EpochSecret>>>, |
There was a problem hiding this comment.
Why do we need RwLock here - presumably to allow many concurrent handshake callbacks to read it while permitting exclusive writes when we update the secret?
Should we also have tests for concurrent client connections and make sure secrets are updated as expected?
There was a problem hiding this comment.
Testing these types of concurrent things is very difficult, so it's relatively rare to have explicit test cases for it. In general, I'm comfortable relying on the API of the read/write lock for safety, but I can look at adding some concurrent usage as part of #5552 (comment)
Co-authored-by: Jou Ho <43765840+jouho@users.noreply.github.com>
Co-authored-by: Jou Ho <43765840+jouho@users.noreply.github.com>
Description of changes:
This adds the PskReceiver component of the HMAC based PSK system.
Testing:
Following the approach from the PSK Provider. We extract the update into a single "poll update" method which can be neatly unit tested.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.