From f5a547aa99a3697f9b9e7452f729ffb4411a29de Mon Sep 17 00:00:00 2001 From: Emil Vardar Date: Thu, 26 Sep 2024 11:06:07 +0000 Subject: [PATCH] Make encrypted versions of RTP extension headers be stopped by default. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By this change we aim to remove the flag enable-webrtc-srtp-encrypted-headers. Bug: chromium:40623740 Change-Id: I74692c90ff1caf2a11d7b73211c1ae4472edfb4d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/362740 Reviewed-by: Harald Alvestrand Commit-Queue: Emil Vardar (xWF) Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/main@{#43105} --- api/rtp_parameters.cc | 9 +++ api/rtp_parameters.h | 5 +- pc/media_session.cc | 82 +++++----------------- pc/media_session_unittest.cc | 129 +++++++++++++---------------------- 4 files changed, 77 insertions(+), 148 deletions(-) diff --git a/api/rtp_parameters.cc b/api/rtp_parameters.cc index c919f7c1dd..178aeb8b66 100644 --- a/api/rtp_parameters.cc +++ b/api/rtp_parameters.cc @@ -76,6 +76,15 @@ RtpHeaderExtensionCapability::RtpHeaderExtensionCapability( int preferred_id, RtpTransceiverDirection direction) : uri(uri), preferred_id(preferred_id), direction(direction) {} +RtpHeaderExtensionCapability::RtpHeaderExtensionCapability( + absl::string_view uri, + int preferred_id, + bool preferred_encrypt, + RtpTransceiverDirection direction) + : uri(uri), + preferred_id(preferred_id), + preferred_encrypt(preferred_encrypt), + direction(direction) {} RtpHeaderExtensionCapability::~RtpHeaderExtensionCapability() = default; RtpExtension::RtpExtension() = default; diff --git a/api/rtp_parameters.h b/api/rtp_parameters.h index b393c3c7d9..ec2243dc85 100644 --- a/api/rtp_parameters.h +++ b/api/rtp_parameters.h @@ -212,7 +212,6 @@ struct RTC_EXPORT RtpHeaderExtensionCapability { std::optional preferred_id; // If true, it's preferred that the value in the header is encrypted. - // TODO(deadbeef): Not implemented. bool preferred_encrypt = false; // The direction of the extension. The kStopped value is only used with @@ -227,6 +226,10 @@ struct RTC_EXPORT RtpHeaderExtensionCapability { RtpHeaderExtensionCapability(absl::string_view uri, int preferred_id, RtpTransceiverDirection direction); + RtpHeaderExtensionCapability(absl::string_view uri, + int preferred_id, + bool preferred_encrypt, + RtpTransceiverDirection direction); ~RtpHeaderExtensionCapability(); bool operator==(const RtpHeaderExtensionCapability& o) const { diff --git a/pc/media_session.cc b/pc/media_session.cc index 50c4260548..0acca3760a 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -62,7 +62,8 @@ using webrtc::RtpTransceiverDirection; webrtc::RtpExtension RtpExtensionFromCapability( const webrtc::RtpHeaderExtensionCapability& capability) { return webrtc::RtpExtension(capability.uri, - capability.preferred_id.value_or(1)); + capability.preferred_id.value_or(1), + capability.preferred_encrypt); } cricket::RtpHeaderExtensions RtpHeaderExtensionsFromCapabilities( @@ -866,6 +867,7 @@ std::vector ComputeCodecsUnion(const std::vector& codecs1, // and `encrypted_extensions` are used for both audio and video. There could be // overlap between audio extensions and video extensions. void MergeRtpHdrExts(const RtpHeaderExtensions& reference_extensions, + bool enable_encrypted_rtp_header_extensions, RtpHeaderExtensions* offered_extensions, RtpHeaderExtensions* regular_extensions, RtpHeaderExtensions* encrypted_extensions, @@ -875,6 +877,9 @@ void MergeRtpHdrExts(const RtpHeaderExtensions& reference_extensions, *offered_extensions, reference_extension.uri, reference_extension.encrypt)) { if (reference_extension.encrypt) { + if (!enable_encrypted_rtp_header_extensions) { + continue; + } const webrtc::RtpExtension* existing = webrtc::RtpExtension::FindHeaderExtensionByUriAndEncryption( *encrypted_extensions, reference_extension.uri, @@ -903,55 +908,6 @@ void MergeRtpHdrExts(const RtpHeaderExtensions& reference_extensions, } } -void AddEncryptedVersionsOfHdrExts(RtpHeaderExtensions* offered_extensions, - RtpHeaderExtensions* encrypted_extensions, - UsedRtpHeaderExtensionIds* used_ids) { - RtpHeaderExtensions encrypted_extensions_to_add; - for (const auto& extension : *offered_extensions) { - // Skip existing encrypted offered extension - if (extension.encrypt) { - continue; - } - - // Skip if we cannot encrypt the extension - if (!webrtc::RtpExtension::IsEncryptionSupported(extension.uri)) { - continue; - } - - // Skip if an encrypted extension with that URI already exists in the - // offered extensions. - const bool have_encrypted_extension = - webrtc::RtpExtension::FindHeaderExtensionByUriAndEncryption( - *offered_extensions, extension.uri, true); - if (have_encrypted_extension) { - continue; - } - - // Determine if a shared encrypted extension with that URI already exists. - const webrtc::RtpExtension* shared_encrypted_extension = - webrtc::RtpExtension::FindHeaderExtensionByUriAndEncryption( - *encrypted_extensions, extension.uri, true); - if (shared_encrypted_extension) { - // Re-use the shared encrypted extension - encrypted_extensions_to_add.push_back(*shared_encrypted_extension); - continue; - } - - // None exists. Create a new shared encrypted extension from the - // non-encrypted one. - webrtc::RtpExtension new_encrypted_extension(extension); - new_encrypted_extension.encrypt = true; - used_ids->FindAndSetIdUsed(&new_encrypted_extension); - encrypted_extensions->push_back(new_encrypted_extension); - encrypted_extensions_to_add.push_back(new_encrypted_extension); - } - - // Append the additional encrypted extensions to be offered - offered_extensions->insert(offered_extensions->end(), - encrypted_extensions_to_add.begin(), - encrypted_extensions_to_add.end()); -} - // Mostly identical to RtpExtension::FindHeaderExtensionByUri but discards any // encrypted extensions that this implementation cannot encrypt. const webrtc::RtpExtension* FindHeaderExtensionByUriDiscardUnsupported( @@ -1927,6 +1883,7 @@ MediaSessionDescriptionFactory::GetOfferedRtpHeaderExtensionsWithIds( UsedRtpHeaderExtensionIds used_ids( extmap_allow_mixed ? UsedRtpHeaderExtensionIds::IdDomain::kTwoByteAllowed : UsedRtpHeaderExtensionIds::IdDomain::kOneByteOnly); + RtpHeaderExtensions all_regular_extensions; RtpHeaderExtensions all_encrypted_extensions; @@ -1938,10 +1895,12 @@ MediaSessionDescriptionFactory::GetOfferedRtpHeaderExtensionsWithIds( for (const ContentInfo* content : current_active_contents) { if (IsMediaContentOfType(content, MEDIA_TYPE_AUDIO)) { MergeRtpHdrExts(content->media_description()->rtp_header_extensions(), + enable_encrypted_rtp_header_extensions_, &offered_extensions.audio, &all_regular_extensions, &all_encrypted_extensions, &used_ids); } else if (IsMediaContentOfType(content, MEDIA_TYPE_VIDEO)) { MergeRtpHdrExts(content->media_description()->rtp_header_extensions(), + enable_encrypted_rtp_header_extensions_, &offered_extensions.video, &all_regular_extensions, &all_encrypted_extensions, &used_ids); } @@ -1956,22 +1915,15 @@ MediaSessionDescriptionFactory::GetOfferedRtpHeaderExtensionsWithIds( entry.header_extensions, all_regular_extensions, all_encrypted_extensions)); if (entry.type == MEDIA_TYPE_AUDIO) - MergeRtpHdrExts(filtered_extensions, &offered_extensions.audio, - &all_regular_extensions, &all_encrypted_extensions, - &used_ids); + MergeRtpHdrExts(filtered_extensions, + enable_encrypted_rtp_header_extensions_, + &offered_extensions.audio, &all_regular_extensions, + &all_encrypted_extensions, &used_ids); else if (entry.type == MEDIA_TYPE_VIDEO) - MergeRtpHdrExts(filtered_extensions, &offered_extensions.video, - &all_regular_extensions, &all_encrypted_extensions, - &used_ids); - } - // TODO(jbauch): Support adding encrypted header extensions to existing - // sessions. - if (enable_encrypted_rtp_header_extensions_ && - current_active_contents.empty()) { - AddEncryptedVersionsOfHdrExts(&offered_extensions.audio, - &all_encrypted_extensions, &used_ids); - AddEncryptedVersionsOfHdrExts(&offered_extensions.video, - &all_encrypted_extensions, &used_ids); + MergeRtpHdrExts(filtered_extensions, + enable_encrypted_rtp_header_extensions_, + &offered_extensions.video, &all_regular_extensions, + &all_encrypted_extensions, &used_ids); } return offered_extensions; } diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index ce81d6ccd8..91e4eacd70 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -124,8 +124,6 @@ const RtpExtension kAudioRtpExtension1[] = { const RtpExtension kAudioRtpExtensionEncrypted1[] = { RtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level", 8), - RtpExtension("http://google.com/testing/audio_something", 10), - RtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level", 12, true), RtpExtension("http://google.com/testing/audio_something", 11, true), }; @@ -135,37 +133,24 @@ const RtpExtension kAudioRtpExtension2[] = { RtpExtension("http://google.com/testing/both_audio_and_video", 7), }; +const RtpExtension kAudioRtpExtensionEncrypted2[] = { + RtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level", 2), + RtpExtension("http://google.com/testing/audio_something", 13, true), + RtpExtension("http://google.com/testing/audio_something_else", 5, true), +}; + const RtpExtension kAudioRtpExtension3[] = { RtpExtension("http://google.com/testing/audio_something", 2), RtpExtension("http://google.com/testing/both_audio_and_video", 3), }; -const RtpExtension kAudioRtpExtension3ForEncryption[] = { - RtpExtension("http://google.com/testing/audio_something", 2), - // Use RTP extension that supports encryption. - RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 3), -}; - -const RtpExtension kAudioRtpExtension3ForEncryptionOffer[] = { - RtpExtension("http://google.com/testing/audio_something", 2), - RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 3), - RtpExtension("http://google.com/testing/audio_something", 14, true), - RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 13, true), -}; - -const RtpExtension kVideoRtpExtension3ForEncryptionOffer[] = { - RtpExtension("http://google.com/testing/video_something", 4), - RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 3), - RtpExtension("http://google.com/testing/video_something", 12, true), - RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 13, true), -}; - const RtpExtension kAudioRtpExtensionAnswer[] = { RtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level", 8), }; const RtpExtension kAudioRtpExtensionEncryptedAnswer[] = { - RtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level", 12, true), + RtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level", 8), + RtpExtension("http://google.com/testing/audio_something", 11, true), }; const RtpExtension kVideoRtpExtension1[] = { @@ -175,8 +160,6 @@ const RtpExtension kVideoRtpExtension1[] = { const RtpExtension kVideoRtpExtensionEncrypted1[] = { RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 14), - RtpExtension("http://google.com/testing/video_something", 13), - RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 9, true), RtpExtension("http://google.com/testing/video_something", 7, true), }; @@ -186,23 +169,24 @@ const RtpExtension kVideoRtpExtension2[] = { RtpExtension("http://google.com/testing/both_audio_and_video", 7), }; +const RtpExtension kVideoRtpExtensionEncrypted2[] = { + RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 8), + RtpExtension("http://google.com/testing/video_something", 10, true), + RtpExtension("http://google.com/testing/video_something_else", 4, true), +}; + const RtpExtension kVideoRtpExtension3[] = { RtpExtension("http://google.com/testing/video_something", 4), RtpExtension("http://google.com/testing/both_audio_and_video", 5), }; -const RtpExtension kVideoRtpExtension3ForEncryption[] = { - RtpExtension("http://google.com/testing/video_something", 4), - // Use RTP extension that supports encryption. - RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 5), -}; - const RtpExtension kVideoRtpExtensionAnswer[] = { RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 14), }; const RtpExtension kVideoRtpExtensionEncryptedAnswer[] = { - RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 9, true), + RtpExtension("urn:ietf:params:rtp-hdrext:toffset", 14), + RtpExtension("http://google.com/testing/video_something", 7, true), }; const RtpExtension kRtpExtensionTransportSequenceNumber01[] = { @@ -602,7 +586,7 @@ class MediaSessionDescriptionFactoryTest : public testing::Test { std::vector capabilities; for (const auto& extension : extensions) { webrtc::RtpHeaderExtensionCapability capability( - extension.uri, extension.id, + extension.uri, extension.id, extension.encrypt, webrtc::RtpTransceiverDirection::kSendRecv); capabilities.push_back(capability); } @@ -612,8 +596,10 @@ class MediaSessionDescriptionFactoryTest : public testing::Test { void SetAudioVideoRtpHeaderExtensions(RtpHeaderExtensions audio_exts, RtpHeaderExtensions video_exts, MediaSessionOptions* opts) { - auto audio_caps = HeaderExtensionCapabilitiesFromRtpExtensions(audio_exts); - auto video_caps = HeaderExtensionCapabilitiesFromRtpExtensions(video_exts); + std::vector audio_caps = + HeaderExtensionCapabilitiesFromRtpExtensions(audio_exts); + std::vector video_caps = + HeaderExtensionCapabilitiesFromRtpExtensions(video_exts); for (auto& entry : opts->media_description_options) { switch (entry.type) { case MEDIA_TYPE_AUDIO: @@ -2042,16 +2028,20 @@ TEST_F(MediaSessionDescriptionFactoryTest, MediaSessionOptions opts; AddAudioVideoSections(RtpTransceiverDirection::kRecvOnly, &opts); + // TODO: bugs.webrtc.org/358039777 - Reverse all the tests since the default + // value will be set to true in the future. f1_.set_enable_encrypted_rtp_header_extensions(true); f2_.set_enable_encrypted_rtp_header_extensions(true); - SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtension1), - MAKE_VECTOR(kVideoRtpExtension1), &opts); + SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtensionEncrypted1), + MAKE_VECTOR(kVideoRtpExtensionEncrypted1), + &opts); std::unique_ptr offer = f1_.CreateOfferOrError(opts, nullptr).MoveValue(); ASSERT_TRUE(offer.get()); - SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtension2), - MAKE_VECTOR(kVideoRtpExtension2), &opts); + SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtensionEncrypted2), + MAKE_VECTOR(kVideoRtpExtensionEncrypted2), + &opts); std::unique_ptr answer = f2_.CreateAnswerOrError(offer.get(), opts, nullptr).MoveValue(); @@ -2074,15 +2064,19 @@ TEST_F(MediaSessionDescriptionFactoryTest, MediaSessionOptions opts; AddAudioVideoSections(RtpTransceiverDirection::kRecvOnly, &opts); + // TODO: bugs.webrtc.org/358039777 - Reverse all the tests since the default + // value will be true. f1_.set_enable_encrypted_rtp_header_extensions(true); - SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtension1), - MAKE_VECTOR(kVideoRtpExtension1), &opts); + SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtensionEncrypted1), + MAKE_VECTOR(kVideoRtpExtensionEncrypted1), + &opts); std::unique_ptr offer = f1_.CreateOfferOrError(opts, nullptr).MoveValue(); ASSERT_TRUE(offer.get()); - SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtension2), - MAKE_VECTOR(kVideoRtpExtension2), &opts); + SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtensionEncrypted2), + MAKE_VECTOR(kVideoRtpExtensionEncrypted2), + &opts); std::unique_ptr answer = f2_.CreateAnswerOrError(offer.get(), opts, nullptr).MoveValue(); @@ -2105,23 +2099,27 @@ TEST_F(MediaSessionDescriptionFactoryTest, MediaSessionOptions opts; AddAudioVideoSections(RtpTransceiverDirection::kRecvOnly, &opts); + // TODO: bugs.webrtc.org/358039777 - Reverse all the tests since the default + // value will be true. f2_.set_enable_encrypted_rtp_header_extensions(true); - SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtension1), - MAKE_VECTOR(kVideoRtpExtension1), &opts); + SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtensionEncrypted1), + MAKE_VECTOR(kVideoRtpExtensionEncrypted1), + &opts); std::unique_ptr offer = f1_.CreateOfferOrError(opts, nullptr).MoveValue(); ASSERT_TRUE(offer.get()); - SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtension2), - MAKE_VECTOR(kVideoRtpExtension2), &opts); + SetAudioVideoRtpHeaderExtensions(MAKE_VECTOR(kAudioRtpExtensionEncrypted2), + MAKE_VECTOR(kVideoRtpExtensionEncrypted2), + &opts); std::unique_ptr answer = f2_.CreateAnswerOrError(offer.get(), opts, nullptr).MoveValue(); EXPECT_EQ( - MAKE_VECTOR(kAudioRtpExtension1), + MAKE_VECTOR(kAudioRtpExtensionAnswer), GetFirstAudioContentDescription(offer.get())->rtp_header_extensions()); EXPECT_EQ( - MAKE_VECTOR(kVideoRtpExtension1), + MAKE_VECTOR(kVideoRtpExtensionAnswer), GetFirstVideoContentDescription(offer.get())->rtp_header_extensions()); EXPECT_EQ( MAKE_VECTOR(kAudioRtpExtensionAnswer), @@ -3512,39 +3510,6 @@ TEST_F(MediaSessionDescriptionFactoryTest, RtpExtensionIdReused) { ->rtp_header_extensions()); } -// Same as "RtpExtensionIdReused" above for encrypted RTP extensions. -TEST_F(MediaSessionDescriptionFactoryTest, RtpExtensionIdReusedEncrypted) { - MediaSessionOptions opts; - AddAudioVideoSections(RtpTransceiverDirection::kRecvOnly, &opts); - - f1_.set_enable_encrypted_rtp_header_extensions(true); - f2_.set_enable_encrypted_rtp_header_extensions(true); - - SetAudioVideoRtpHeaderExtensions( - MAKE_VECTOR(kAudioRtpExtension3ForEncryption), - MAKE_VECTOR(kVideoRtpExtension3ForEncryption), &opts); - std::unique_ptr offer = - f1_.CreateOfferOrError(opts, nullptr).MoveValue(); - - EXPECT_EQ( - MAKE_VECTOR(kAudioRtpExtension3ForEncryptionOffer), - GetFirstAudioContentDescription(offer.get())->rtp_header_extensions()); - EXPECT_EQ( - MAKE_VECTOR(kVideoRtpExtension3ForEncryptionOffer), - GetFirstVideoContentDescription(offer.get())->rtp_header_extensions()); - - // Nothing should change when creating a new offer - std::unique_ptr updated_offer( - f1_.CreateOfferOrError(opts, offer.get()).MoveValue()); - - EXPECT_EQ(MAKE_VECTOR(kAudioRtpExtension3ForEncryptionOffer), - GetFirstAudioContentDescription(updated_offer.get()) - ->rtp_header_extensions()); - EXPECT_EQ(MAKE_VECTOR(kVideoRtpExtension3ForEncryptionOffer), - GetFirstVideoContentDescription(updated_offer.get()) - ->rtp_header_extensions()); -} - TEST(MediaSessionDescription, CopySessionDescription) { SessionDescription source; ContentGroup group(CN_AUDIO);