Add support for cryptex #511
Conversation
|
hi, it would be good to get the CI build passing as soon as possible, are you able to fix that or doe you need some help? |
|
Great to have this on a branch but I would like to see the IETF draft to get further along before we merge it to the main branch. |
|
@fluffy I agree. My main intent was to get feedback on the changes and spot any issue on the implementation that could impact the spec. Would be great if it could be thoroughly reviewed so we can confirm that the test vectors can be incorporated into the rfc and be ready to merge the branch as soon as the rfc is ready. |
|
Given the IETF draft is about at WGLC, we should merge this in once we are happy with the tests. |
@murillo128 are you able to fix the compile issues? Maybe rebase on master as well? @paulej have you thought to review these changes ? |
|
I'll do it later this month when I am back from vacations |
| /* Get CSRCs block position or profile if no CSRCs */ | ||
| uint32_t *csrcs = (uint32_t *)hdr + uint32s_in_rtp_header; | ||
| /* Move CSRCS so block is contiguous with extension header block */ | ||
| for (unsigned char i = hdr->cc; i > 0; --i) |
There was a problem hiding this comment.
Any reason to do this vs. memcpy() or memmove()? Loops are usually slower, but maybe this doesn't matter.
There was a problem hiding this comment.
just thought it would be easier to review the change and check it was spec compliant, I can change it to memcpy or memmove if needed
backport changes from master
|
I have also rebased to master and fixed the CI errors of the previous version. Could anyone run the github workflows to check if everything is correct now? |
|
fixed format and mem leak on deallocating recv sessions on tests |
|
When will this be merged? |
|
hmm, I see this made it to an RFC now, https://datatracker.ietf.org/doc/html/rfc9335 , this was not the case when this code was committed or originally reviewed. I am unsure if the current implementation follows the RFC or not, would need some input from @murillo128 on that. |
|
Mainly to be used with WebRTC |
|
If this gets merged and upstreamed by libwebrtc I will provide a patch for adding support to cryptex in libwebrtc |
|
ok, I can update it to work on top of the v3 work if you like, it will give me a chance to better review the code. |
|
i have rebased the PR and addressed the comments. could we run the tests? not completly sure I did everything correctly |
|
If you want to test interop, once you have the WebRTC patch, Jitsi Meet's Videobridge supports cryptex, and I should be able to configure an instance to negotiate it in our WebRTC client. |
| @@ -125,6 +125,8 @@ typedef struct srtp_stream_ctx_t_ { | |||
| uint8_t *enc_xtn_hdr; | |||
| size_t enc_xtn_hdr_count; | |||
| uint32_t pending_roc; | |||
| bool use_cryptex; | |||
| struct srtp_stream_ctx_t_ *next; /* linked list of streams */ | |||
There was a problem hiding this comment.
next was removed, I guess this slipped in while merging ?
There was a problem hiding this comment.
There have been a lot of changes in main, are you sure the merge went well ? All builds are failing.
Also the input buffer to the protect/unprotect functions is now const, not sure you should just cast that away and move the memory around.
There was a problem hiding this comment.
i think I have been too optimistic about my rebasing 😅
Wil try to get a look next week
There was a problem hiding this comment.
Yeah, I had a quick attempt a few weeks ago and understood that this was not an easy merge :( . Let me know if you want to discuss anything.
|
@murillo128 I have created a new PR #724 , that tries to merge your changes in to main. It takes a slightly different approach given the non in-place io support that is in main now.
If option 1 is done then it could still be possible to modify the input to supportone call to encrypt when it is detected that this is in-place io. |
|
@pabuhler thank you very much for taking the time to work on this! Removing the On my experience, some backends (expecially OpenSSL 3.0) have a huge overhead when making multiple calls to the api, but we are already having that performance issue anyway: #645 |
OK, I will tryout gcm api supporting multiple calls to verify that it is possible. Then at least cryptex can be "functionaly" complete and can look at optimizations afterwards. It is not sure that the performance problems in #645 will be such and issue when making multiple calls to openssl if IV has not changed but time will tell. |
|
I managed to get it to fully work with gcm and openssl, cryptex in this mode also requires multiple calls to srtp_cipher_set_aad but it seamed to work. Will look into the other backends. |
|
@murillo128, I have updated the code in #724, I have abandoned the attempt to support calling encrypt/decrypt multiple times when using a gcm cipher as it is not well support by the different backends. Can look in to this again later. So the current state is that if the io is in-place and there are CC's in the header then the input buffer will be rearranged similar to your original PR, this is supported by both icm and gcm. If there are CC's in the header and io is not in-place then there will multiple calls to encrypt or decrypt, this is only supported by icm. If there are no CC's in the header then the start offset is just updated and there is a single call to encrypt or decrypt, this is supported by both icm and gcm. Short summary is if you want to use cryptex then you should use in-place io. Do you think could merge that code in to this PR and it could get reviewed and hopefully merged? |
Implementation of cryptex as per https://github.com/juberti/cryptex
Discussion:
juberti/cryptex#5