From ebe6d0dade99f9010ece6cfbaa10e2a1a90fdddb Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Thu, 28 Mar 2024 18:02:57 -0700 Subject: [PATCH] Clean up PairedKeyVerificationRunner PiperOrigin-RevId: 620111283 --- .../fake_nearby_share_certificate_manager.cc | 5 + sharing/nearby_sharing_service_impl.cc | 2 +- sharing/paired_key_verification_runner.cc | 130 +++++++----------- sharing/paired_key_verification_runner.h | 24 ++-- .../paired_key_verification_runner_test.cc | 99 ++++++++++--- 5 files changed, 139 insertions(+), 121 deletions(-) diff --git a/sharing/certificates/fake_nearby_share_certificate_manager.cc b/sharing/certificates/fake_nearby_share_certificate_manager.cc index 56ceb291..43f8c4f1 100644 --- a/sharing/certificates/fake_nearby_share_certificate_manager.cc +++ b/sharing/certificates/fake_nearby_share_certificate_manager.cc @@ -32,6 +32,7 @@ #include "sharing/internal/api/sharing_rpc_client.h" #include "sharing/internal/public/context.h" #include "sharing/local_device_data/nearby_share_local_device_data_manager.h" +#include "sharing/proto/enums.pb.h" #include "sharing/proto/rpc_resources.pb.h" namespace nearby { @@ -112,6 +113,10 @@ void FakeNearbyShareCertificateManager::OnStop() {} std::optional FakeNearbyShareCertificateManager::GetValidPrivateCertificate( DeviceVisibility visibility) const { + if (visibility == DeviceVisibility::DEVICE_VISIBILITY_EVERYONE || + visibility == DeviceVisibility::DEVICE_VISIBILITY_HIDDEN) { + return std::nullopt; + } auto cert = GetNearbyShareTestPrivateCertificate(visibility); cert.next_salts_for_testing() = std::queue>(); cert.next_salts_for_testing().push(next_salt_); diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index ee7555dc..c86cc6f3 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -3366,7 +3366,7 @@ void NearbySharingServiceImpl::RunPairedKeyVerification( context_->GetClock(), device_info_, share_target_id, share_target_info->IsIncoming(), settings_->GetVisibility(), settings_->GetLastVisibility(), - settings_->GetLastVisibilityTimestamp(), endpoint_id, *token, + settings_->GetLastVisibilityTimestamp(), *token, share_target_info->connection(), share_target_info->certificate(), GetCertificateManager(), share_target_info->frames_reader(), kReadFramesTimeout)); diff --git a/sharing/paired_key_verification_runner.cc b/sharing/paired_key_verification_runner.cc index c0552642..0b017621 100644 --- a/sharing/paired_key_verification_runner.cc +++ b/sharing/paired_key_verification_runner.cc @@ -117,15 +117,13 @@ PairedKeyVerificationRunner::PairedKeyVerificationRunner( Clock* clock, DeviceInfo& device_info, int64_t share_target_id, bool share_target_is_incoming, DeviceVisibility visibility, DeviceVisibility last_visibility, absl::Time last_visibility_time, - absl::string_view endpoint_id, const std::vector& token, - NearbyConnection* connection, + const std::vector& token, NearbyConnection* connection, const std::optional& certificate, NearbyShareCertificateManager* certificate_manager, IncomingFramesReader* frames_reader, absl::Duration read_frame_timeout) : clock_(clock), device_info_(device_info), share_target_id_(share_target_id), - endpoint_id_(std::string(endpoint_id)), raw_token_(token), connection_(connection), certificate_(certificate), @@ -143,11 +141,6 @@ PairedKeyVerificationRunner::PairedKeyVerificationRunner( visibility_ = visibility; last_visibility_ = last_visibility; last_visibility_time_ = last_visibility_time; - // Receiver will try to re-validate signature using previous visibility if - // it has changed recently. - relax_restrict_to_contacts_ = - RelaxRestrictToContactsIfNeeded() || - visibility_ == DeviceVisibility::DEVICE_VISIBILITY_EVERYONE; } else { remote_prefix_ = kNearbyShareReceiverVerificationPrefix; local_prefix_ = kNearbyShareSenderVerificationPrefix; @@ -155,7 +148,6 @@ PairedKeyVerificationRunner::PairedKeyVerificationRunner( visibility_ = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS; last_visibility_ = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS; last_visibility_time_ = absl::UnixEpoch(); - relax_restrict_to_contacts_ = true; } } @@ -165,6 +157,7 @@ void PairedKeyVerificationRunner::Run( std::function callback) { NL_DCHECK(!callback_); callback_ = std::move(callback); + verification_result_ = PairedKeyVerificationResult::kSuccess; SendPairedKeyEncryptionFrame(); frames_reader_->ReadFrame( @@ -190,35 +183,23 @@ void PairedKeyVerificationRunner::OnReadPairedKeyEncryptionFrame( return; } - std::vector verification_results; + PairedKeyVerificationResult auth_token_hash_result = + VerifyAuthTokenHashWithPrivateCertificate(visibility_, *frame); - PairedKeyVerificationResult remote_public_certificate_result = - VerifyRemotePublicCertificate(*frame); - - if (remote_public_certificate_result != - PairedKeyVerificationResult::kSuccess) { - if (!relax_restrict_to_contacts_) { - NL_VLOG(1) << __func__ - << ": we are only allowing connections with contacts. " - "Rejecting connection from unknown ShareTarget - " - << share_target_id_; - SendPairedKeyResultFrame(PairedKeyVerificationResult::kFail); - std::move(callback_)(PairedKeyVerificationResult::kFail, - OSType::UNKNOWN_OS_TYPE); - return; - } else { - remote_public_certificate_result = - VerifyRemotePublicCertificateRelaxed(*frame); + if (auth_token_hash_result != PairedKeyVerificationResult::kSuccess) { + if (IsVisibilityRecentlyUpdated()) { + auth_token_hash_result = + VerifyAuthTokenHashWithPrivateCertificate(last_visibility_, *frame); } } - verification_results.push_back(remote_public_certificate_result); + ApplyResult(auth_token_hash_result); NL_VLOG(1) << __func__ << ": Remote public certificate verification result " - << remote_public_certificate_result; + << auth_token_hash_result; PairedKeyVerificationResult local_result = VerifyPairedKeyEncryptionFrame(*frame); - verification_results.push_back(local_result); + ApplyResult(local_result); NL_VLOG(1) << __func__ << ": Paired key encryption verification result " << local_result; @@ -226,21 +207,18 @@ void PairedKeyVerificationRunner::OnReadPairedKeyEncryptionFrame( frames_reader_->ReadFrame( V1Frame::PAIRED_KEY_RESULT, - [&, runner = GetWeakPtr(), - verification_results = - std::move(verification_results)](std::optional frame) { + [this, runner = GetWeakPtr()](std::optional frame) { auto verification_runner = runner.lock(); if (verification_runner == nullptr) { NL_LOG(WARNING) << "PairedKeyVerificationRunner is released before."; return; } - OnReadPairedKeyResultFrame(verification_results, std::move(frame)); + OnReadPairedKeyResultFrame(std::move(frame)); }, read_frame_timeout_); } void PairedKeyVerificationRunner::OnReadPairedKeyResultFrame( - std::vector verification_results, std::optional frame) { if (!frame.has_value()) { NL_LOG(WARNING) << __func__ << ": Failed to read remote paired key result"; @@ -249,22 +227,21 @@ void PairedKeyVerificationRunner::OnReadPairedKeyResultFrame( return; } - PairedKeyVerificationResult key_result = + PairedKeyVerificationResult remote_result = Convert(frame->paired_key_result().status()); - verification_results.push_back(key_result); - NL_VLOG(1) << __func__ << ": Paired key result frame result " << key_result; + ApplyResult(remote_result); + NL_VLOG(1) << __func__ << ": Paired key result frame result " + << remote_result; - PairedKeyVerificationResult combined_result = - MergeResults(verification_results); NL_VLOG(1) << __func__ << ": Combined verification result " - << combined_result; + << verification_result_; OSType os_type = OSType::UNKNOWN_OS_TYPE; if (frame->paired_key_result().has_os_type()) { os_type = frame->paired_key_result().os_type(); } - std::move(callback_)(combined_result, os_type); + std::move(callback_)(verification_result_, os_type); } void PairedKeyVerificationRunner::SendPairedKeyResultFrame( @@ -326,7 +303,7 @@ void PairedKeyVerificationRunner::SendPairedKeyEncryptionFrame() { PairedKeyEncryptionFrame* encryption_frame = v1_frame->mutable_paired_key_encryption(); encryption_frame->set_signed_data(signature->data(), signature->size()); - if (RelaxRestrictToContactsIfNeeded()) { + if (IsVisibilityRecentlyUpdated()) { NL_LOG(INFO) << "Attempts to sign authentication token with a previous private key."; std::optional> optional_signature = @@ -347,24 +324,9 @@ void PairedKeyVerificationRunner::SendPairedKeyEncryptionFrame() { } PairedKeyVerificationRunner::PairedKeyVerificationResult -PairedKeyVerificationRunner::VerifyRemotePublicCertificate( - const V1Frame& frame) { - return VerifyRemotePublicCertificateWithPrivateCertificate(visibility_, - frame); -} - -PairedKeyVerificationRunner::PairedKeyVerificationResult -PairedKeyVerificationRunner::VerifyRemotePublicCertificateRelaxed( +PairedKeyVerificationRunner::VerifyAuthTokenHashWithPrivateCertificate( + DeviceVisibility visibility, const nearby::sharing::service::proto::V1Frame& frame) { - return VerifyRemotePublicCertificateWithPrivateCertificate(last_visibility_, - frame); -} - -PairedKeyVerificationRunner::PairedKeyVerificationResult -PairedKeyVerificationRunner:: - VerifyRemotePublicCertificateWithPrivateCertificate( - DeviceVisibility visibility, - const nearby::sharing::service::proto::V1Frame& frame) { std::optional> hash = certificate_manager_->HashAuthenticationTokenWithPrivateCertificate( visibility, raw_token_); @@ -404,15 +366,6 @@ PairedKeyVerificationRunner::VerifyPairedKeyEncryptionFrame( "no optional signed data."; return PairedKeyVerificationResult::kFail; } - - if (!RelaxRestrictToContactsIfNeeded()) { - NL_LOG(WARNING) - << __func__ - << ": Unable to verify remote paired key encryption frame. " - "no need to try relax check."; - return PairedKeyVerificationResult::kFail; - } - // Verify optional signed data. auto optional_signed_data = frame.paired_key_encryption().optional_signed_data(); @@ -435,23 +388,34 @@ PairedKeyVerificationRunner::VerifyPairedKeyEncryptionFrame( return PairedKeyVerificationResult::kSuccess; } -PairedKeyVerificationRunner::PairedKeyVerificationResult -PairedKeyVerificationRunner::MergeResults( - const std::vector& results) { - bool all_success = true; - for (const auto& result : results) { - if (result == PairedKeyVerificationResult::kFail) return result; - - if (result != PairedKeyVerificationResult::kSuccess) all_success = false; +void PairedKeyVerificationRunner::ApplyResult( + PairedKeyVerificationResult result) { + if (verification_result_ == PairedKeyVerificationResult::kFail) { + // If already failed, then nothing to do. + return; + } + switch (result) { + case PairedKeyVerificationResult::kSuccess: + // Success does not change final result. + break; + case PairedKeyVerificationResult::kFail: + // If new result is kFail, then the whole transaction failed. + verification_result_ = PairedKeyVerificationResult::kFail; + break; + case PairedKeyVerificationResult::kUnable: + verification_result_ = PairedKeyVerificationResult::kUnable; + break; + case PairedKeyVerificationResult::kUnknown: + default: + verification_result_ = PairedKeyVerificationResult::kUnable; + break; } - - return all_success ? PairedKeyVerificationResult::kSuccess - : PairedKeyVerificationResult::kUnable; } -bool PairedKeyVerificationRunner::RelaxRestrictToContactsIfNeeded() const { - return certificate_.has_value() && (clock_->Now() - last_visibility_time_ < - kRelaxAfterSetVisibilityTimeout); +bool PairedKeyVerificationRunner::IsVisibilityRecentlyUpdated() const { + return visibility_ != last_visibility_ && + (clock_->Now() - last_visibility_time_ < + kRelaxAfterSetVisibilityTimeout); } } // namespace sharing diff --git a/sharing/paired_key_verification_runner.h b/sharing/paired_key_verification_runner.h index 5680ccb8..981c266a 100644 --- a/sharing/paired_key_verification_runner.h +++ b/sharing/paired_key_verification_runner.h @@ -32,7 +32,6 @@ #include "sharing/certificates/nearby_share_decrypted_public_certificate.h" #include "sharing/incoming_frames_reader.h" #include "sharing/nearby_connection.h" -#include "sharing/nearby_sharing_settings.h" #include "sharing/proto/enums.pb.h" #include "sharing/proto/wire_format.pb.h" @@ -57,8 +56,7 @@ class PairedKeyVerificationRunner Clock* clock, DeviceInfo& device_info, int64_t share_target_id, bool share_target_is_incoming, proto::DeviceVisibility visibility, proto::DeviceVisibility last_visibility, absl::Time last_visibility_time, - absl::string_view endpoint_id, const std::vector& token, - NearbyConnection* connection, + const std::vector& token, NearbyConnection* connection, const std::optional& certificate, NearbyShareCertificateManager* certificate_manager, IncomingFramesReader* frames_reader, absl::Duration read_frame_timeout); @@ -79,22 +77,18 @@ class PairedKeyVerificationRunner void OnReadPairedKeyEncryptionFrame( std::optional frame); void OnReadPairedKeyResultFrame( - std::vector verification_results, std::optional frame); void SendPairedKeyResultFrame(PairedKeyVerificationResult result); - PairedKeyVerificationResult VerifyRemotePublicCertificate( - const nearby::sharing::service::proto::V1Frame& frame); - PairedKeyVerificationResult VerifyRemotePublicCertificateRelaxed( - const nearby::sharing::service::proto::V1Frame& frame); - PairedKeyVerificationResult - VerifyRemotePublicCertificateWithPrivateCertificate( + // Verifies auth token hash in frame using private certificate for visibility. + // Returns either kSuccess or kUnable. This function never returns kFail. + PairedKeyVerificationResult VerifyAuthTokenHashWithPrivateCertificate( proto::DeviceVisibility visibility, const nearby::sharing::service::proto::V1Frame& frame); PairedKeyVerificationResult VerifyPairedKeyEncryptionFrame( const nearby::sharing::service::proto::V1Frame& frame); - PairedKeyVerificationResult MergeResults( - const std::vector& results); - bool RelaxRestrictToContactsIfNeeded() const; + void ApplyResult(PairedKeyVerificationResult result); + // True if visibility has changed recently. + bool IsVisibilityRecentlyUpdated() const; nearby::Clock* const clock_; nearby::DeviceInfo& device_info_; @@ -102,7 +96,6 @@ class PairedKeyVerificationRunner proto::DeviceVisibility visibility_; proto::DeviceVisibility last_visibility_; absl::Time last_visibility_time_; - std::string endpoint_id_; std::vector raw_token_; NearbyConnection* connection_; std::optional certificate_; @@ -112,8 +105,7 @@ class PairedKeyVerificationRunner std::function callback_; - bool relax_restrict_to_contacts_ = false; - + PairedKeyVerificationResult verification_result_; char local_prefix_; char remote_prefix_; }; diff --git a/sharing/paired_key_verification_runner_test.cc b/sharing/paired_key_verification_runner_test.cc index de93f652..b0bfc958 100644 --- a/sharing/paired_key_verification_runner_test.cc +++ b/sharing/paired_key_verification_runner_test.cc @@ -59,8 +59,6 @@ using PairedKeyVerificationResult = PairedKeyVerificationRunner::PairedKeyVerificationResult; using ::location::nearby::proto::sharing::OSType; -constexpr char kEndpointId[] = "test_endpoint_id"; - const std::vector& GetAuthToken() { static std::vector* auth_token = new std::vector({0, 1, 2}); return *auth_token; @@ -119,6 +117,27 @@ std::list GeneratePairedKeyResultFrame() { return result; } +struct VisibilityChange { + DeviceVisibility visibility; + DeviceVisibility last_visibility; +}; + +std::list GenerateVisibilityChanges() { + constexpr DeviceVisibility kValidVisibilities[] = { + DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + DeviceVisibility::DEVICE_VISIBILITY_SELF_SHARE, + DeviceVisibility::DEVICE_VISIBILITY_EVERYONE, + DeviceVisibility::DEVICE_VISIBILITY_HIDDEN, + }; + std::list result; + for (DeviceVisibility visibility : kValidVisibilities) { + for (DeviceVisibility last_visibility : kValidVisibilities) { + result.push_back({visibility, last_visibility}); + } + } + return result; +} + const absl::Duration kTimeout = absl::Seconds(1); class MockIncomingFramesReader : public IncomingFramesReader { @@ -192,11 +211,10 @@ class PairedKeyVerificationRunnerTest : public testing::Test { : std::nullopt; auto runner = std::make_shared( - context_.GetClock(), fake_device_info_, share_target_.id, - is_incoming, visibility, last_visibility, - last_visibility_time, kEndpointId, GetAuthToken(), &connection_, - std::move(public_certificate), &certificate_manager_, &frames_reader_, - kTimeout); + context_.GetClock(), fake_device_info_, share_target_.id, is_incoming, + visibility, last_visibility, last_visibility_time, GetAuthToken(), + &connection_, std::move(public_certificate), &certificate_manager_, + &frames_reader_, kTimeout); runner->Run( [&, expected_result, expected_os_type]( @@ -326,9 +344,10 @@ class PairedKeyVerificationRunnerTest : public testing::Test { }; TEST_F(PairedKeyVerificationRunnerTest, - NullCertificate_InvalidPairedKeyEncryptionFrame_RestrictToContacts) { + NullCertificate_InvalidPairedKeyEncryptionFrame) { // Empty key encryption frame fails the certificate verification. SetUpPairedKeyEncryptionFrame(ReturnFrameType::kEmpty); + SetUpPairedKeyResultFrame(ReturnFrameType::kValid); RunVerification( share_target_.is_incoming, @@ -337,9 +356,10 @@ TEST_F(PairedKeyVerificationRunnerTest, DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, GetFakeClock()->Now(), /*expected_result=*/ - PairedKeyVerificationResult::kFail); + PairedKeyVerificationResult::kUnable); ExpectPairedKeyEncryptionFrameSent(); + ExpectPairedKeyResultFrameSent(PairedKeyResultFrame::UNABLE); } TEST_F(PairedKeyVerificationRunnerTest, @@ -368,6 +388,8 @@ struct TestParameters { PairedKeyVerificationRunnerTest::ReturnFrameType encryption_frame_type; PairedKeyVerificationRunner::PairedKeyVerificationResult result; } kParameters[] = { + {true, true, PairedKeyVerificationRunnerTest::ReturnFrameType::kNull, + PairedKeyVerificationRunner::PairedKeyVerificationResult::kFail}, {true, true, PairedKeyVerificationRunnerTest::ReturnFrameType::kEmpty, PairedKeyVerificationRunner::PairedKeyVerificationResult::kFail}, {true, true, PairedKeyVerificationRunnerTest::ReturnFrameType::kValid, @@ -377,6 +399,8 @@ struct TestParameters { PairedKeyVerificationRunner::PairedKeyVerificationResult::kSuccess}, {true, true, PairedKeyVerificationRunnerTest::ReturnFrameType::kInValid, PairedKeyVerificationRunner::PairedKeyVerificationResult::kFail}, + {true, false, PairedKeyVerificationRunnerTest::ReturnFrameType::kNull, + PairedKeyVerificationRunner::PairedKeyVerificationResult::kFail}, {true, false, PairedKeyVerificationRunnerTest::ReturnFrameType::kEmpty, PairedKeyVerificationRunner::PairedKeyVerificationResult::kUnable}, {true, false, PairedKeyVerificationRunnerTest::ReturnFrameType::kValid, @@ -389,7 +413,7 @@ struct TestParameters { }; using KeyVerificationTestParam = - std::tuple; + std::tuple; class ParameterisedPairedKeyVerificationRunnerTest : public PairedKeyVerificationRunnerTest, @@ -399,6 +423,7 @@ TEST_P(ParameterisedPairedKeyVerificationRunnerTest, ValidEncryptionFrame_ValidResultFrame) { const TestParameters& params = std::get<0>(GetParam()); PairedKeyResultFrame result_frame = std::get<1>(GetParam()); + VisibilityChange visibility_changes = std::get<2>(GetParam()); PairedKeyVerificationRunner::PairedKeyVerificationResult expected_result = Merge(params.result, result_frame.status()); @@ -409,26 +434,57 @@ TEST_P(ParameterisedPairedKeyVerificationRunnerTest, << (int)params.encryption_frame_type << ", result=" << (int)params.result << ", expected_result=" << (int)expected_result - << ", result_frame=" << (int)result_frame.status(); + << ", result_frame=" << (int)result_frame.status() + << ", visibility=" << (int)visibility_changes.visibility + << ", last_visibility=" + << (int)visibility_changes.last_visibility; SetUpPairedKeyEncryptionFrame(params.encryption_frame_type); - SetUpPairedKeyResultFrame( - PairedKeyVerificationRunnerTest::ReturnFrameType::kValid, - result_frame.status(), - result_frame.has_os_type() ? result_frame.os_type() - : OSType::UNKNOWN_OS_TYPE); + bool encryption_frame_timeout = + params.encryption_frame_type == + PairedKeyVerificationRunnerTest::ReturnFrameType::kNull; + if (!encryption_frame_timeout) { + // Result frame is only expected if Encryption frame read does not time out. + SetUpPairedKeyResultFrame( + PairedKeyVerificationRunnerTest::ReturnFrameType::kValid, + result_frame.status(), + result_frame.has_os_type() ? result_frame.os_type() + : OSType::UNKNOWN_OS_TYPE); + } + // If our visibility has no certificates, then downgrade expected result to + // kUnable if it is not expected to fail. + if ((visibility_changes.visibility == + DeviceVisibility::DEVICE_VISIBILITY_EVERYONE || + visibility_changes.visibility == + DeviceVisibility::DEVICE_VISIBILITY_HIDDEN) && + (visibility_changes.last_visibility == + DeviceVisibility::DEVICE_VISIBILITY_EVERYONE || + visibility_changes.last_visibility == + DeviceVisibility::DEVICE_VISIBILITY_HIDDEN)) { + if (expected_result == + PairedKeyVerificationRunner::PairedKeyVerificationResult::kSuccess) { + expected_result = + PairedKeyVerificationRunner::PairedKeyVerificationResult::kUnable; + } + } RunVerification( /*is_incoming=*/params.is_incoming, /*use_valid_public_certificate=*/params.has_valid_certificate, - DeviceVisibility::DEVICE_VISIBILITY_EVERYONE, - DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + visibility_changes.visibility, visibility_changes.last_visibility, GetFakeClock()->Now(), expected_result, - result_frame.has_os_type() ? result_frame.os_type() - : OSType::UNKNOWN_OS_TYPE); + result_frame.has_os_type() && !encryption_frame_timeout + ? result_frame.os_type() + : OSType::UNKNOWN_OS_TYPE); ExpectPairedKeyEncryptionFrameSent(); + if (encryption_frame_timeout) { + // If timed out waiting from PairedKeyEncryptionFrame, no result frame would + // be sent. + return; + } + // Check for result frame sent. if (!params.has_valid_certificate) { ExpectPairedKeyResultFrameSent(PairedKeyResultFrame::UNABLE); @@ -453,7 +509,8 @@ TEST_P(ParameterisedPairedKeyVerificationRunnerTest, INSTANTIATE_TEST_SUITE_P( /*no prefix*/, ParameterisedPairedKeyVerificationRunnerTest, testing::Combine(testing::ValuesIn(kParameters), - testing::ValuesIn(GeneratePairedKeyResultFrame()))); + testing::ValuesIn(GeneratePairedKeyResultFrame()), + testing::ValuesIn(GenerateVisibilityChanges()))); } // namespace } // namespace sharing