[C++][Parquet] Thoughts on classes in key_toolkit.h

[kapoisu]

Describe the enhancement requested

KeyToolkit is used internally according to the comment. It looks more like a mere aggregate. I suggest redefine it as a struct and change existing member functions to free functions, which can be implemented by the public interfaces of existing member variables. The only exception which may introduce invariants is the first part of KeyToolket::RotateMasterKeys(),

arrow/cpp/src/parquet/encryption/key_toolkit.cc

Lines 54 to 61 in 6e84d99

	const auto now = internal::CurrentTimePoint();
	auto lock = last_cache_clean_for_key_rotation_time_mutex_.Lock();
	if (now > last_cache_clean_for_key_rotation_time_ +
	std::chrono::duration<double>(kCacheCleanPeriodForKeyRotation)) {
	kek_write_cache_per_token().Clear();
	last_cache_clean_for_key_rotation_time_ = now;
	}
	lock.Unlock();

However, TwoLevelCacheWithExpiration (the object return by kek_write_cache_per_token() in the code above) has nearly identical logic,

arrow/cpp/src/parquet/encryption/two_level_cache_with_expiration.h

Lines 114 to 124 in 6e84d99

	void CheckCacheForExpiredTokens(double cache_cleanup_period_seconds) {
	auto lock = mutex_.Lock();
	const auto now = internal::CurrentTimePoint();
	if (now > (last_cache_cleanup_timestamp_ +
	std::chrono::duration<double>(cache_cleanup_period_seconds))) {
	RemoveExpiredEntriesNoMutex();
	last_cache_cleanup_timestamp_ =
	now + std::chrono::duration<double>(cache_cleanup_period_seconds);
	}
	}

except that the timestamp is set to now + cache_cleanup_period_seconds, which looks like a bug.

The same change can be applied to KeyWithMasterId.
I'll work on it if this gets approved.

Component(s)

C++

[wgtmac]

cc @adamreeve

[adamreeve]

KeyToolkit is technically public so this would be a breaking change, but I don't think this was intentional. Eg. for key rotation users should be using the CryptoFactory, so I think it's OK to change the KeyToolkit API in theory.

except that the timestamp is set to now + cache_cleanup_period_seconds, which looks like a bug.

Yes that definitely looks like a bug and should be fixed.

I don't think you can simply remove the cleanup logic from RotateMasterKeys though. It's important that that has its own "last clean up" time, as calling RotateMasterKeys is an indication that the master keys have been updated so key encryption keys need to be re-encrypted, and the full KEK cache should immediately be cleared including keys that have not yet expired.

The reason this isn't done on every call to RotateMasterKeys is that after master keys have been updated you'll often want to re-encrypt DEKs for multiple files and re-use the new KEKs.

There are also other methods on KeyToolkit that require access to multiple members, eg. GetKmsClient, RemoveCacheEntriesForToken and RemoveCacheEntriesForAllTokens. So the current structure of KeyToolkit being a class with these methods makes sense to me, and I don't see much benefit in changing it to a struct and separate free functions.

[kapoisu]

Thanks for the clarification Adam! I'll submit a PR to fix the timestamp then.

In addition, I noticed that when I built parquet with PARQUET_REQUIRE_ENCRYPTION=ON, the OpenSSL was required but didn't get downloaded as other dependencies did. Is this the intended behavior?

[adamreeve]

In addition, I noticed that when I built parquet with PARQUET_REQUIRE_ENCRYPTION=ON, the OpenSSL was required but didn't get downloaded as other dependencies did. Is this the intended behavior?

It looks like this is intentional and we always try to use the system OpenSSL, but I'm not sure what the reason is for this:

arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake

Line 1586 in 6e84d99

set(OpenSSL_SOURCE "SYSTEM")

[kapoisu]

@adamreeve I want to confirm it again. The two-level cache object itself already contains a timestamp that is independent of the expiration time per key. And this timestamp cannot be used to handle the situations you mentioned so that an additional timestamp within the KeyToolkit is necessary?

Btw I've found another potential fix within parquet/encryption/two_level_cache_with_expiration.h.

arrow/cpp/src/parquet/encryption/two_level_cache_with_expiration.h

Lines 98 to 112 in 6e84d99

	std::shared_ptr<ConcurrentMap<std::string, V>> GetOrCreateInternalCache(
	const std::string& access_token, double cache_entry_lifetime_seconds) {
	auto lock = mutex_.Lock();
	auto external_cache_entry = cache_.find(access_token);
	if (external_cache_entry == cache_.end() ||
	external_cache_entry->second.IsExpired()) {
	cache_.insert({access_token, internal::ExpiringCacheMapEntry<V>(
	std::shared_ptr<ConcurrentMap<std::string, V>>(
	new ConcurrentMap<std::string, V>()),
	cache_entry_lifetime_seconds)});
	}
	return cache_[access_token].cached_item();
	}

The insert() does nothing when the key exists. This should be either:

1. Remove the isExpired() check.
2. Update the expiration timestamp when the key is expired.

RemoveExpiredEntriesNoMutex() also seems duplicate. It's currently used by CheckCacheForExpiredTokens() only and the mutex is acquired anyway.

[adamreeve]

@adamreeve I want to confirm it again. The two-level cache object itself already contains a timestamp that is independent of the expiration time per key. And this timestamp cannot be used to handle the situations you mentioned so that an additional timestamp within the KeyToolkit is necessary?

Yes, we can't use the last_cache_cleanup_timestamp_ in TwoLevelCacheWithExpiration to handle the key rotation use case.

The insert() does nothing when the key exists

Yes that also looks like a bug. I think rather than just updating the expiration timestamp we want to replace the entry with a new empty cache. We could probably replace insert with insert_or_assign?

RemoveExpiredEntriesNoMutex() also seems duplicate. It's currently used by CheckCacheForExpiredTokens() only and the mutex is acquired anyway.

It's also used by RemoveExpiredEntriesFromCache, although as far as I can see that's only used in one test.

[kapoisu]

It's also used by RemoveExpiredEntriesFromCache, although as far as I can see that's only used in one test.

Yup. I meant RemoveExpiredEntriesFromCache is just "acquire the mutex and call RemoveExpiredEntriesNoMutex". While CheckCacheForExpiredTokens calls RemoveExpiredEntriesNoMutex, it also acquires the mutex before that. That means it can call RemoveExpiredEntriesFromCache directly. Although I'm not sure if moving RemoveExpiredEntriesNoMutex's logic to RemoveExpiredEntriesFromCache is a good a idea, considering that there may be usages that don't require the mutex in the future?

[adamreeve]

I don't think you can simply move the RemoveExpiredEntriesNoMutex logic into RemoveExpiredEntriesFromCache, as the mutex is also needed to handle updating last_cache_cleanup_timestamp_ in CheckCacheForExpiredTokens and prevent repeated clean ups. And std::mutex is non-recursive so you can't call RemoveExpiredEntriesFromCache while already holding a lock.

[kapoisu]

I don't think you can simply move the RemoveExpiredEntriesNoMutex logic into RemoveExpiredEntriesFromCache, as the mutex is also needed to handle updating last_cache_cleanup_timestamp_ in CheckCacheForExpiredTokens and prevent repeated clean ups. And std::mutex is non-recursive so you can't call RemoveExpiredEntriesFromCache while already holding a lock.

I got it. Does last_cache_cleanup_timestamp_ need to be updated within RemoveExpiredEntriesFromCache or the timestamp is solely for CheckCacheForExpiredTokens? If it did, RemoveExpiredEntriesFromCache would literally be CheckCacheForExpiredTokens(0.0).

[adamreeve]

Yeah I think you're right, you could replace RemoveExpiredEntriesFromCache with a call to CheckCacheForExpiredTokens(0.0), it would make sense to me that RemoveExpiredEntriesFromCache should be updating last_cache_cleanup_timestamp_.