Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for NULL ciphers #286

Merged
merged 1 commit into from
Jul 18, 2024
Merged

Conversation

sirzooro
Copy link
Contributor

Added support for NULL ciphers. When they are used, created SRTP and SRTCP packets are authenticated only (no encryption).

Received SRTP/SRTCP packets are checked if their authentication tag is valid, and extra SRTP protocol fields are removed before returning then to application.

Fixed processing of SRTCP packets with E (encryption) bit cleared, previously duplicate check and tag validation was not performed, and whole packet was returned as-is (with extra fields) from decryptRTCP.

Use of NULL ciphers can be enabled independently for SRTP and SRTCP using SRTPNoEncryption and SRTCPNoEncryption options. They can be used with key exchange protocols which allows to configure them separately.

Added support for SRTP_NULL_HMAC_SHA1_80 and SRTP_NULL_HMAC_SHA1_32 cipher suites. They use key and salt of the same length as AES_CM_128 ones.

Added new tests to verify test vectors from RFCs.

Verified results with ones produced by libsrtp2. All matches except for AEAD SRTP ones - libsrtp2 encrypts packet instead of adding authentication tag only.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 92.59259% with 10 lines in your changes missing coverage. Please review.

Project coverage is 80.73%. Comparing base (2a52aa0) to head (5cbacc4).

Files Patch % Lines
srtp_cipher_aes_cm_hmac_sha1.go 85.36% 3 Missing and 3 partials ⚠️
srtp.go 66.66% 1 Missing and 1 partial ⚠️
srtp_cipher_aead_aes_gcm.go 95.83% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   80.20%   80.73%   +0.53%     
==========================================
  Files          17       17              
  Lines        1081     1163      +82     
==========================================
+ Hits          867      939      +72     
- Misses        120      125       +5     
- Partials       94       99       +5     
Flag Coverage Δ
go 80.73% <92.59%> (+0.53%) ⬆️
wasm 80.30% <92.59%> (+0.56%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sirzooro sirzooro force-pushed the add_null_cipher branch 4 times, most recently from 902e9e5 to 06be86f Compare July 17, 2024 06:49
@sirzooro sirzooro requested a review from at-wat July 17, 2024 06:51
context.go Outdated Show resolved Hide resolved
@Sean-Der
Copy link
Member

Amazing work @sirzooro thank you so much for doing this, this will make a lot of people happy :)

@@ -84,3 +84,37 @@ func MasterKeyIndicator(mki []byte) ContextOption {
return nil
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to not have these options, and have everything configured just from Cipher choice?

I am just starting to read the PR, maybe I will understand better as I go :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. MIKEY (RFC 3830) and a=crypto SDP attribute (RFC 4568) allows to disable encryption for SRTP and SRTCP independently. I also would have to create 3 additional crypto profiles for every cipher.

Maybe down the road it would be better to change ProtectionProfile to struct and put various additional params there, and pass it to CreateContext instead of options like it is implemented now.

srtp_cipher.go Outdated Show resolved Hide resolved
Added support for NULL ciphers. When they are used, created SRTP and
SRTCP packets are authenticated only (no encryption).

Received SRTP/SRTCP packets are checked if their authentication tag
is valid, and extra SRTP protocol fields are removed before returning
then to application.

Fixed processing of SRTCP packets with E (encryption) bit cleared,
previously duplicate check and tag valiation was not performed, and
whole packet was returned as-is (with extra fields) from decryptRTCP.

Use of NULL ciphers can be enabled independently for SRTP and SRTCP
using SRTPNoEncryption and SRTCPNoEncryption options. They can be used
with key exchange protocols which allows to configure them separately.

Added support for SRTP_NULL_HMAC_SHA1_80 and SRTP_NULL_HMAC_SHA1_32
cipher suites. They use key and salt of the same length as AES_CM_128
ones.

Added new tests to verify test vectors from RFCs.
@sirzooro
Copy link
Contributor Author

Moved new test-related code to srtp_cipher_utils_test.go file.

@sirzooro sirzooro requested a review from Sean-Der July 17, 2024 15:51
@Sean-Der Sean-Der merged commit 590d1cf into pion:master Jul 18, 2024
14 checks passed
@sirzooro sirzooro deleted the add_null_cipher branch July 18, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants