You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/08/30 15:15:30 UTC
[GitHub] [trafficserver] cmcfarlen commented on a diff in pull request #8790: General cleanup of the SSLSecret code.
cmcfarlen commented on code in PR #8790:
URL: https://github.com/apache/trafficserver/pull/8790#discussion_r958561064
##########
iocore/net/SSLSecret.cc:
##########
@@ -36,111 +35,87 @@ SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::s
secret_name.key_name = name2.data();
secret_name.key_name_len = name2.size();
while (curHook) {
- curHook->invoke(TS_EVENT_SSL_SECRET, &secret_name);
+ curHook->blocking_invoke(TS_EVENT_SSL_SECRET, &secret_name);
curHook = curHook->next();
}
- const std::string *data1 = this->getSecretItem(name1);
- const std::string *data2 = this->getSecretItem(name2);
- if ((nullptr == data1 || data1->length() == 0) || (!name2.empty() && (nullptr == data2 || data2->length() == 0))) {
+ data1 = this->getSecret(name1);
+ data2 = name2.empty() ? std::string{} : this->getSecret(name2);
+ if (data1.empty() || (!name2.empty() && data2.empty())) {
// If none of them loaded it, assume it is a file
- return loadFile(name1, data_item1) && (name2.empty() || loadFile(name2, data_item2));
+ data1 = loadFile(name1);
+ if (!name2.empty()) {
+ data2 = loadFile(name2);
+ }
}
- return true;
}
-bool
-SSLSecret::loadFile(const std::string &name, std::string &data_item)
+std::string
+SSLSecret::loadFile(const std::string &name)
{
+ Debug("ssl_secret", "SSLSecret::loadFile(%s)", name.c_str());
struct stat statdata;
// Load the secret and add it to the map
if (stat(name.c_str(), &statdata) < 0) {
- return false;
+ return std::string{};
}
std::error_code error;
- data_item = ts::file::load(ts::file::path(name), error);
+ std::string const data = ts::file::load(ts::file::path(name), error);
if (error) {
// Loading file failed
- return false;
+ return std::string{};
}
+ Debug("ssl_secret", "Secret data: %.50s", data.c_str());
if (SSLConfigParams::load_ssl_file_cb) {
SSLConfigParams::load_ssl_file_cb(name.c_str());
}
- return true;
+ return data;
}
-bool
-SSLSecret::setSecret(const std::string &name, const char *data, int data_len)
+void
+SSLSecret::setSecret(const std::string &name, std::string_view data)
{
std::scoped_lock lock(secret_map_mutex);
- auto iter = secret_map.find(name);
- if (iter == secret_map.end()) {
- secret_map[name] = "";
- iter = secret_map.find(name);
- }
- if (iter == secret_map.end()) {
- return false;
- }
- iter->second.assign(data, data_len);
+ secret_map[name] = std::string{data};
// The full secret data can be sensitive. Print only the first 50 bytes.
- Debug("ssl_secret", "Set secret for %s to %.50s", name.c_str(), iter->second.c_str());
- return true;
+ Debug("ssl_secret", "Set secret for %s to %.*s", name.c_str(), int(data.size() > 50 ? 50 : data.size()), data.data());
}
-const std::string *
-SSLSecret::getSecretItem(const std::string &name) const
+std::string
+SSLSecret::getSecret(const std::string &name) const
Review Comment:
With these changes the caller can no longer differentiate between not found and empty secrets. Perhaps this could return std::optional to restore that info.
##########
iocore/net/SSLSecret.cc:
##########
@@ -36,111 +35,87 @@ SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::s
secret_name.key_name = name2.data();
secret_name.key_name_len = name2.size();
while (curHook) {
- curHook->invoke(TS_EVENT_SSL_SECRET, &secret_name);
+ curHook->blocking_invoke(TS_EVENT_SSL_SECRET, &secret_name);
curHook = curHook->next();
}
- const std::string *data1 = this->getSecretItem(name1);
- const std::string *data2 = this->getSecretItem(name2);
- if ((nullptr == data1 || data1->length() == 0) || (!name2.empty() && (nullptr == data2 || data2->length() == 0))) {
+ data1 = this->getSecret(name1);
+ data2 = name2.empty() ? std::string{} : this->getSecret(name2);
+ if (data1.empty() || (!name2.empty() && data2.empty())) {
// If none of them loaded it, assume it is a file
- return loadFile(name1, data_item1) && (name2.empty() || loadFile(name2, data_item2));
+ data1 = loadFile(name1);
+ if (!name2.empty()) {
+ data2 = loadFile(name2);
+ }
}
- return true;
}
-bool
-SSLSecret::loadFile(const std::string &name, std::string &data_item)
+std::string
+SSLSecret::loadFile(const std::string &name)
{
+ Debug("ssl_secret", "SSLSecret::loadFile(%s)", name.c_str());
struct stat statdata;
// Load the secret and add it to the map
if (stat(name.c_str(), &statdata) < 0) {
- return false;
+ return std::string{};
}
Review Comment:
Seems like we could skip calling stat here since we don't do anything with the result and the load call will fail below if stat does.
##########
src/traffic_server/InkAPI.cc:
##########
@@ -1873,6 +1887,122 @@ _TSfree(void *ptr)
ats_free(ptr);
}
+// Support code for TSHeapBuf.
+namespace
+{
+class HeapBuf
Review Comment:
`HeapBuf` seems like its just `std::unique_ptr<std::string>`, would you consider using that to back TSHeapBuf instead of this custom class?
##########
proxy/InkAPIInternal.h:
##########
@@ -117,6 +117,12 @@ class APIHook
APIHook *next() const;
APIHook *prev() const;
LINK(APIHook, m_link);
+
+ // This is like invoke(), but allows for blocking on continuation mutexes. It is a hack, calling it can block
+ // the calling thread. It should be replaced with a version with a continuation parameter, providing a continuation
+ // to be called when the hook handling is complete.
+ //
+ int blocking_invoke(int event, void *edata) const;
Review Comment:
How is this hack related to cleanup of SSL secrets? Is this needed for this PR?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org