SteNicholas commented on code in PR #274: URL: https://github.com/apache/paimon-cpp/pull/274#discussion_r4056632138
########## src/paimon/rest/rest_token_file_system.cpp: ########## @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "paimon/rest/rest_token_file_system.h" + +#include <string> +#include <utility> + +#include "paimon/common/utils/string_utils.h" +#include "paimon/core/core_options.h" + +namespace paimon { +namespace { + +// Length of the "fs.<scheme>." prefix of a file system option key, or 0 when `key` is not +// scheme-qualified. Only a scheme-qualified key such as "fs.oss.accessKeyId" can have a +// bucket-scoped variant such as "fs.oss.bucket.<bucket>.accessKeyId". +size_t FileSystemOptionPrefixLength(const std::string& key) { + if (!StringUtils::StartsWith(key, "fs.")) { + return 0; + } + size_t scheme_end = key.find('.', 3); + return scheme_end == std::string::npos ? 0 : scheme_end + 1; +} + +} // namespace + +std::shared_ptr<RestTokenFileSystemCache> RestTokenFileSystem::CreateFileSystemCache() { + RestTokenFileSystemCache::Options cache_options; + cache_options.max_weight = kMaxCachedFileSystems; + cache_options.expire_after_access_ms = kFileSystemCacheExpireAfterAccessMillis; + return std::make_shared<RestTokenFileSystemCache>(std::move(cache_options)); +} + +RestTokenFileSystem::RestTokenFileSystem(const std::shared_ptr<RestApi>& api, + const std::map<std::string, std::string>& catalog_options, + const Identifier& identifier, + std::shared_ptr<RestTokenFileSystemCache> fs_cache, + Clock clock) + : catalog_options_(catalog_options), + identifier_(identifier), + fs_cache_(fs_cache != nullptr ? std::move(fs_cache) : CreateFileSystemCache()), + provider_(std::make_shared<RestCredentialProvider>(api, catalog_options, identifier, + std::move(clock))) {} + +std::map<std::string, std::string> RestTokenFileSystem::MergeTokenOptions( + const std::map<std::string, std::string>& catalog_options, const RestToken& token) { + std::map<std::string, std::string> fs_options = catalog_options; + for (const auto& [key, value] : token.token) { + // A file system resolves a bucket-scoped option such as + // "fs.oss.bucket.<bucket>.accessKeyId" ahead of the flat "fs.oss.accessKeyId" the + // token carries. Keeping a catalog's bucket-scoped credential would sign an access + // with that stale key pair together with the token's security token, an invalid + // combination, so drop the bucket-scoped variant of every option the token sets: the + // issued credentials then win whichever bucket the table's data lives in. + size_t prefix_length = FileSystemOptionPrefixLength(key); + if (prefix_length != 0) { + std::string bucket_prefix = key.substr(0, prefix_length) + "bucket."; + std::string bucket_suffix = "." + key.substr(prefix_length); + for (auto it = fs_options.begin(); it != fs_options.end();) { + if (it->first.size() > bucket_prefix.size() + bucket_suffix.size() && + StringUtils::StartsWith(it->first, bucket_prefix) && + StringUtils::EndsWith(it->first, bucket_suffix)) { + it = fs_options.erase(it); Review Comment: **Preserve bucket-scoped credentials supplied by the token itself.** The erase loop runs against `fs_options` after earlier entries from the same token have already been inserted. If the response contains both global OSS credentials and a complete `fs.oss.bucket.b.*` credential set, `std::map` iteration inserts `fs.oss.bucket.b.securityToken` before processing `fs.oss.securityToken`; the latter then erases the token's own bucket STS value. The bucket access key and secret survive, so the OSS factory signs with the bucket key pair and the global STS token, and authentication fails even with an empty catalog-options map. I reproduced this with the current merge and OSS lookup bodies. Please remove conflicting catalog keys before overlaying the complete token map, or explicitly preserve keys present in `token.token`, and add a mixed global/bucket token test. ########## src/paimon/rest/rest_token_file_system.cpp: ########## @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "paimon/rest/rest_token_file_system.h" + +#include <string> +#include <utility> + +#include "paimon/common/utils/string_utils.h" +#include "paimon/core/core_options.h" + +namespace paimon { +namespace { + +// Length of the "fs.<scheme>." prefix of a file system option key, or 0 when `key` is not +// scheme-qualified. Only a scheme-qualified key such as "fs.oss.accessKeyId" can have a +// bucket-scoped variant such as "fs.oss.bucket.<bucket>.accessKeyId". +size_t FileSystemOptionPrefixLength(const std::string& key) { + if (!StringUtils::StartsWith(key, "fs.")) { + return 0; + } + size_t scheme_end = key.find('.', 3); + return scheme_end == std::string::npos ? 0 : scheme_end + 1; +} + +} // namespace + +std::shared_ptr<RestTokenFileSystemCache> RestTokenFileSystem::CreateFileSystemCache() { + RestTokenFileSystemCache::Options cache_options; + cache_options.max_weight = kMaxCachedFileSystems; + cache_options.expire_after_access_ms = kFileSystemCacheExpireAfterAccessMillis; + return std::make_shared<RestTokenFileSystemCache>(std::move(cache_options)); +} + +RestTokenFileSystem::RestTokenFileSystem(const std::shared_ptr<RestApi>& api, + const std::map<std::string, std::string>& catalog_options, + const Identifier& identifier, + std::shared_ptr<RestTokenFileSystemCache> fs_cache, + Clock clock) + : catalog_options_(catalog_options), + identifier_(identifier), + fs_cache_(fs_cache != nullptr ? std::move(fs_cache) : CreateFileSystemCache()), + provider_(std::make_shared<RestCredentialProvider>(api, catalog_options, identifier, + std::move(clock))) {} + +std::map<std::string, std::string> RestTokenFileSystem::MergeTokenOptions( + const std::map<std::string, std::string>& catalog_options, const RestToken& token) { + std::map<std::string, std::string> fs_options = catalog_options; + for (const auto& [key, value] : token.token) { + // A file system resolves a bucket-scoped option such as + // "fs.oss.bucket.<bucket>.accessKeyId" ahead of the flat "fs.oss.accessKeyId" the + // token carries. Keeping a catalog's bucket-scoped credential would sign an access + // with that stale key pair together with the token's security token, an invalid + // combination, so drop the bucket-scoped variant of every option the token sets: the + // issued credentials then win whichever bucket the table's data lives in. + size_t prefix_length = FileSystemOptionPrefixLength(key); + if (prefix_length != 0) { + std::string bucket_prefix = key.substr(0, prefix_length) + "bucket."; + std::string bucket_suffix = "." + key.substr(prefix_length); Review Comment: **Apply temporary-token precedence across the OSS token aliases.** A token containing fresh `fs.oss.accessKeyId`, `fs.oss.accessKeySecret`, and `fs.oss.sessionToken` leaves an existing catalog `fs.oss.securityToken` (including its bucket-scoped variant) untouched, because this cleanup only matches the exact option suffix. `OssFileSystemFactory::Create()` reads `securityToken` first and consults `sessionToken` only when it is empty, so it combines the refreshed key pair with the old STS token and fails authentication. Both aliases are supported by the OSS backend. Please normalize the aliases or clear the catalog values of both when the issued credentials supply either, and test flat as well as bucket-scoped stale `securityToken` values. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
