Rework set_external_psks to use opaque structures #2557
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lrstewart
changed the title
Codecov Report
@@ Coverage Diff @@
## main #2557 +/- ##
==========================================
- Coverage 82.16% 82.14% -0.02%
==========================================
Files 272 272
Lines 19092 19133 +41
==========================================
+ Hits 15686 15717 +31
- Misses 3406 3416 +10 |
maddeleine
reviewed
Feb 5, 2021
maddeleine
reviewed
Feb 5, 2021
tls/s2n_psk.c
Outdated
| if (psk->type == S2N_PSK_TYPE_EXTERNAL) { | ||
| GUARD(s2n_psk_free(psk)); | ||
| GUARD_AS_POSIX(s2n_psk_wipe(psk)); |
There was a problem hiding this comment.
Why are we wiping the psk instead of freeing it?
There was a problem hiding this comment.
No logic has actually changed here-- I just renamed "s2n_psk_free" to "s2n_psk_wipe", which is more accurate. We need to free all the blobs allocated for the psk, but not the memory allocated for the psk data structure itself. The memory allocated for the psk data structure is owned by the s2n_array.
We should use uint32_t to count PSKs, since the max length of an s2n_array is uint32_max. I also added a test to makes sure that we handle too-large extensions, in case the customer configures too many PSKs.
Only the client sends its identity list over the wire.
maddeleine
reviewed
Feb 8, 2021
ttjsu-aws
reviewed
Feb 9, 2021
maddeleine
approved these changes
Feb 9, 2021
ttjsu-aws
reviewed
Feb 9, 2021
ttjsu-aws
reviewed
Feb 10, 2021
camshaft
reviewed
Feb 10, 2021
ttjsu-aws
reviewed
Feb 10, 2021
camshaft
approved these changes
Feb 10, 2021
maddeleine
approved these changes
Feb 10, 2021
SalusaSecondus
approved these changes
Feb 10, 2021
ttjsu-aws
approved these changes
Feb 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
Part 1 of correcting the PSK APIs to use opaque structures. This change is solely focused on s2n_connection_set_external_psks. s2n_psk_selection_callback is out of scope.
Most of the test changes are due to name / return type changes, but I also add some additional validation since more APIs will now be public.
Call-outs:
I kept the methods for setting identity and secret separate, although they could possibly be combined with each other or with the _new method. However, I think separate methods are clearer and easier to use.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Unit tests
Is this a refactor change? Yes, although it changes inputs rather than the underlying logic. The old tests continue working, just with some name changes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.