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

Catch metadata realm decryption. #4285

Merged
merged 18 commits into from
May 11, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* Windows `InterprocessCondVar` changes reverted.
* Fix an issue when using `Results::freeze` across threads with different transaction versions. Previously, copying the `Results`'s tableview could result in a stale state or objects from a future version. Now there is a comparison for the source and desitnation transaction version when constructing `ConstTableView`, which will cause the tableview to reflect the correct state if needed ([#4254](https://github.com/realm/realm-core/pull/4254)).
* `@min` and `@max` queries on a list of float, double or Decimal128 values could match the incorrect value if NaN or null was present in the list (since 5.0.0).
* Opening a metadata realm with the wrong encryption key will remove that metadata realm and create a new realm using the new key. [#4285](https://github.com/realm/realm-core/pull/4285)

### Breaking changes
* None.
Expand Down
16 changes: 13 additions & 3 deletions src/realm/object-store/sync/impl/sync_metadata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,19 @@ SyncMetadataManager::SyncMetadataManager(std::string path, bool should_encrypt,
}
};

SharedRealm realm = Realm::get_shared_realm(config);
m_metadata_config = std::move(config);

SharedRealm realm;
try {
realm = get_realm();
}
catch (const RealmFileException&) {
if (!should_encrypt)
Copy link
Member

Choose a reason for hiding this comment

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

This should check the error code from the exception rather than should_encrypt.

Copy link
Contributor Author

@ejm01 ejm01 Jan 19, 2021

Choose a reason for hiding this comment

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

Opening with the wrong key and opening when should_encrypt configuration doesn't match the original configuration are both RealmFileException::Kind::AccessError and have the same error message. What other ways are there to differentiate?

Copy link
Member

Choose a reason for hiding this comment

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

In both of those cases we want to delete the Realm and recreate it. Currently this code will delete the Realm when any error occurs if we want to encrypt the Realm even if it's an unrelated error, and will fail to delete the Realm in a case where we want to (disabling encryption when the existing file is encrypted).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I understand. I previously was trying to retain the response of throwing an error when should_encrypt == false was applied to a path that was encrypted.

throw;

util::File::remove(m_metadata_config.path);
realm = get_realm();
}

// Get data about the (hardcoded) schemas
auto object_schema = realm->schema().find(c_sync_userMetadata);
Expand Down Expand Up @@ -221,8 +233,6 @@ SyncMetadataManager::SyncMetadataManager(std::string path, bool should_encrypt,
object_schema->persisted_properties[2].column_key, object_schema->persisted_properties[3].column_key,
object_schema->persisted_properties[4].column_key};

m_metadata_config = std::move(config);

m_client_uuid = [&]() -> std::string {
TableRef table = ObjectStore::table_for_object_type(realm->read_group(), c_sync_clientMetadata);
if (table->is_empty()) {
Expand Down
35 changes: 33 additions & 2 deletions test/object-store/sync/metadata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <realm/object-store/schema.hpp>

#include <realm/object-store/impl/object_accessor_impl.hpp>
#include <realm/object-store/impl/realm_coordinator.hpp>
#include "util/test_utils.hpp"

#include <realm/util/file.hpp>
Expand Down Expand Up @@ -513,8 +514,38 @@ TEST_CASE("sync_metadata: encryption", "[sync]") {

SECTION("prohibits opening the metadata Realm with different keys") {
SECTION("different keys") {
SyncMetadataManager first_manager(metadata_path, true, make_test_encryption_key(10));
REQUIRE_THROWS(SyncMetadataManager(metadata_path, true, make_test_encryption_key(11)));
const auto identity0 = "identity0";
const auto auth_url = "https://realm.example.org";

// Open metadata realm, make metadata
std::vector<char> key0 = make_test_encryption_key(10);
SyncMetadataManager manager0(metadata_path, true, key0);

auto user_metadata = manager0.get_or_make_user_metadata(identity0, auth_url);
REQUIRE(bool(user_metadata));
CHECK(user_metadata->identity() == identity0);
CHECK(user_metadata->provider_type() == auth_url);
CHECK(user_metadata->access_token().empty());
CHECK(user_metadata->is_valid());

// Close realm
_impl::RealmCoordinator::get_coordinator(metadata_path)->clear_cache();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tgoyne Is clearing the cache here bad? I couldn't figure any other way to successfully close the realm. I understand there are dangers for doing this normally, but would it be suitable for a test context?

Copy link
Member

Choose a reason for hiding this comment

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

The above code needs to be wrapped in another scope. The metadata realm will be closed when everything using it goes out of scope (in this case, just user_metadata as the manager itself doesn't keep the Realm open).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if 8317f78 is what you mean.


// Open metadata realm with different key
std::vector<char> key1 = make_test_encryption_key(11);
SyncMetadataManager manager1(metadata_path, true, key1);

auto user_metadata1 = manager1.get_or_make_user_metadata(identity0, auth_url, false);
// Expect previous metadata to no longer be stored
CHECK_FALSE(bool(user_metadata1));

// But new metadata can still be created
const auto identity1 = "identity1";
auto user_metadata2 = manager1.get_or_make_user_metadata(identity1, auth_url);
CHECK(user_metadata2->identity() == identity1);
CHECK(user_metadata2->provider_type() == auth_url);
CHECK(user_metadata2->access_token().empty());
CHECK(user_metadata2->is_valid());
}
SECTION("different encryption settings") {
SyncMetadataManager first_manager(metadata_path, true, make_test_encryption_key(10));
Expand Down