Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Send user agents with all our tools #1289

Merged
merged 1 commit into from
Aug 13, 2019
Merged

Send user agents with all our tools #1289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 2 additions & 14 deletions src/libaktualizr/http/httpclient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,6 @@
#include <assert.h>
#include <sstream>

#include <openssl/crypto.h>
#include <openssl/err.h>
#include <openssl/evp.h>
#include <openssl/opensslv.h>
#include <openssl/pem.h>
#include <openssl/pkcs12.h>
#include <openssl/rand.h>
#include <openssl/rsa.h>
#include <openssl/ssl.h>

#include "crypto/openssl_compat.h"
#include "utilities/aktualizr_version.h"
#include "utilities/utils.h"

Expand Down Expand Up @@ -46,8 +35,7 @@ static size_t writeString(void* contents, size_t size, size_t nmemb, void* userp
return size * nmemb;
}

HttpClient::HttpClient(const std::vector<std::string>* extra_headers)
: user_agent(std::string("Aktualizr/") + aktualizr_version()) {
HttpClient::HttpClient(const std::vector<std::string>* extra_headers) {
curl = curl_easy_init();
if (curl == nullptr) {
throw std::runtime_error("Could not initialize curl");
Expand All @@ -73,7 +61,7 @@ HttpClient::HttpClient(const std::vector<std::string>* extra_headers)
}
}
curlEasySetoptWrapper(curl, CURLOPT_HTTPHEADER, headers);
curlEasySetoptWrapper(curl, CURLOPT_USERAGENT, user_agent.c_str());
curlEasySetoptWrapper(curl, CURLOPT_USERAGENT, Utils::getUserAgent());
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might override the user agent header specified by an HttpClient user/client in extra_headers.
IMHO, statically predefined headers should be specified in a HTTP client factory configuration (which doesn't exist), so libaktualizr clients could specify desired mandatory headers in explicit way. Utils::set/getUserAgent() is an implicit API for libaktualizr clients/apps they need to be aware and set it before Aktualizr instantiation.
This is just my two cents and can be ignored since it will require more work :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree this is not especially elegant. The main problem I had is that different garage tools are using some common code that instantiates an HttpClient, that's why I opted for the less intrusive global setting.

I don't see much use case for HttpClient users redefining user agents dynamically though, it's maybe the (rare) kind of thing that can be setup globally.

But then I also question my choice of putting the API in utils.h. I'll try moving it to http client itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem, I understand that we need to be practical and balance between an ideal but time consuming and a just enough but quicker solutions.
BTW, at first glance, this PR, IMHO, is related to this one #1273 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

lol - i've been watching this PR to see how things went before pushing my change which will at best produce a merge conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha well... This PR is not really urgent anyway. Would you also prefer that to be done another way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no. I was just wanted to see the approach that was chosen here since there's a good chance I could do the same thing in my change.

}

HttpClient::HttpClient(const HttpClient& curl_in) : pkcs11_key(curl_in.pkcs11_key), pkcs11_cert(curl_in.pkcs11_key) {
Expand Down
1 change: 0 additions & 1 deletion src/libaktualizr/http/httpclient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ class HttpClient : public HttpInterface {
CURL *curl;
curl_slist *headers;
HttpResponse perform(CURL *curl_handler, int retry_times, int64_t size_limit);
std::string user_agent;

static CURLcode sslCtxFunction(CURL *handle, void *sslctx, void *parm);
std::unique_ptr<TemporaryFile> tls_ca_file;
Expand Down
20 changes: 20 additions & 0 deletions src/libaktualizr/http/httpclient_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,26 @@ TEST(PostTest, put_performed) {
EXPECT_EQ(json["data"]["key"].asString(), "val");
}

TEST(HttpClient, user_agent) {
{
// test the default, when setUserAgent hasn't been called yet
HttpClient http;

const auto resp = http.get(server + "/user_agent", HttpInterface::kNoLimit);
const auto app = resp.body.substr(0, resp.body.find('/'));
EXPECT_EQ(app, "Aktualizr");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you provide a brief comment to make it clear that you're testing the expected default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!
Done.

}

Utils::setUserAgent("blah");

{
HttpClient http;

auto resp = http.get(server + "/user_agent", HttpInterface::kNoLimit);
EXPECT_EQ(resp.body, "blah");
}
}

// TODO: add tests for HttpClient::download

#ifndef __NO_MAIN__
Expand Down
26 changes: 26 additions & 0 deletions src/libaktualizr/utilities/utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <boost/uuid/random_generator.hpp>
#include <boost/uuid/uuid_io.hpp>

#include "aktualizr_version.h"
#include "logging/logging.h"

const char *adverbs[] = {
Expand Down Expand Up @@ -765,6 +766,17 @@ void Utils::setStorageRootPath(const std::string &storage_root_path) { storage_r

boost::filesystem::path Utils::getStorageRootPath() { return storage_root_path_; }

void Utils::setUserAgent(std::string user_agent) { user_agent_ = std::move(user_agent); }

const char *Utils::getUserAgent() {
if (user_agent_.empty()) {
user_agent_ = (std::string("Aktualizr/") + aktualizr_version());
}
return user_agent_.c_str();
}

std::string Utils::user_agent_;

TemporaryFile::TemporaryFile(const std::string &hint)
: tmp_name_(SafeTempRoot::Get() / boost::filesystem::unique_path(std::string("%%%%-%%%%-").append(hint))) {}

Expand Down Expand Up @@ -857,3 +869,17 @@ int Socket::bind(in_port_t port, bool reuse) {
int Socket::connect() {
return ::connect(socket_fd_, reinterpret_cast<const struct sockaddr *>(&sock_address_), sizeof(sock_address_));
}

CurlEasyWrapper::CurlEasyWrapper() {
handle = curl_easy_init();
if (handle == nullptr) {
throw std::runtime_error("Could not initialize curl handle");
}
curlEasySetoptWrapper(handle, CURLOPT_USERAGENT, Utils::getUserAgent());
}

CurlEasyWrapper::~CurlEasyWrapper() {
if (handle != nullptr) {
curl_easy_cleanup(handle);
}
}
17 changes: 6 additions & 11 deletions src/libaktualizr/utilities/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,12 @@ struct Utils {
static void setStorageRootPath(const std::string &storage_root_path);
static boost::filesystem::path getStorageRootPath();

static void setUserAgent(std::string user_agent);
static const char *getUserAgent();

private:
static std::string storage_root_path_;
static std::string user_agent_;
};

/**
Expand Down Expand Up @@ -144,17 +148,8 @@ class Socket {
// wrapper for curl handles
class CurlEasyWrapper {
public:
CurlEasyWrapper() {
handle = curl_easy_init();
if (handle == nullptr) {
throw std::runtime_error("Could not initialize curl handle");
}
}
~CurlEasyWrapper() {
if (handle != nullptr) {
curl_easy_cleanup(handle);
}
}
CurlEasyWrapper();
~CurlEasyWrapper();
CURL *get() { return handle; }

private:
Expand Down
8 changes: 4 additions & 4 deletions src/libaktualizr/utilities/utils_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ TEST(Utils, TemporaryDirectory) {
EXPECT_TRUE(boost::filesystem::exists(p)); // The dir should exist
EXPECT_NE(p.string().find("ahint"), std::string::npos); // The hint is included in the filename

struct stat statbuf;
struct stat statbuf {};
EXPECT_GE(stat(p.parent_path().c_str(), &statbuf), 0);
EXPECT_EQ(statbuf.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO), S_IRWXU);
}
Expand All @@ -203,7 +203,7 @@ TEST(Utils, TemporaryFile) {
EXPECT_TRUE(boost::filesystem::exists(p)); // The file should exist here
EXPECT_NE(p.string().find("ahint"), std::string::npos); // The hint is included in the filename

struct stat statbuf;
struct stat statbuf {};
EXPECT_GE(stat(p.parent_path().c_str(), &statbuf), 0);
EXPECT_EQ(statbuf.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO), S_IRWXU);
}
Expand Down Expand Up @@ -298,7 +298,7 @@ TEST(Utils, ipUtils) {
EXPECT_NE(fd, -1);
SocketHandle hdl(new int(fd));

sockaddr_in6 sa;
sockaddr_in6 sa{};

memset(&sa, 0, sizeof(sa));
sa.sin6_family = AF_INET6;
Expand All @@ -312,7 +312,7 @@ TEST(Utils, ipUtils) {

EXPECT_NE(bind(*hdl, reinterpret_cast<const sockaddr *>(&sa), sizeof(sa)), -1);

sockaddr_storage ss;
sockaddr_storage ss{};
EXPECT_NO_THROW(ss = Utils::ipGetSockaddr(*hdl));

EXPECT_NE(Utils::ipDisplayName(ss), "unknown");
Expand Down
2 changes: 2 additions & 0 deletions src/sota_tools/garage_check.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ int main(int argc, char **argv) {
assert(0);
}

Utils::setUserAgent(std::string("garage-check/") + garage_tools_version());

if (vm.count("walk-tree") != 0u) {
mode = RunMode::kWalkTree;
}
Expand Down
2 changes: 2 additions & 0 deletions src/sota_tools/garage_deploy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ int main(int argc, char **argv) {
assert(0);
}

Utils::setUserAgent(std::string("garage-deploy/") + garage_tools_version());

if (vm.count("dry-run") != 0u) {
mode = RunMode::kDryRun;
}
Expand Down
2 changes: 2 additions & 0 deletions src/sota_tools/garage_push.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ int main(int argc, char **argv) {
assert(0);
}

Utils::setUserAgent(std::string("garage-push/") + garage_tools_version());

if (cacerts != "") {
if (!boost::filesystem::exists(cacerts)) {
LOG_FATAL << "--cacert path " << cacerts << " does not exist";
Expand Down
3 changes: 3 additions & 0 deletions src/sota_tools/ostree_object.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "logging/logging.h"
#include "ostree_repo.h"
#include "request_pool.h"
#include "utilities/utils.h"

using std::string;

Expand Down Expand Up @@ -191,6 +192,7 @@ void OSTreeObject::MakeTestRequest(const TreehubServer &push_target, CURLM *curl
push_target.InjectIntoCurl(Url(), curl_handle_);
curlEasySetoptWrapper(curl_handle_, CURLOPT_NOBODY, 1L); // HEAD

curlEasySetoptWrapper(curl_handle_, CURLOPT_USERAGENT, Utils::getUserAgent());
curlEasySetoptWrapper(curl_handle_, CURLOPT_WRITEFUNCTION, &OSTreeObject::curl_handle_write);
curlEasySetoptWrapper(curl_handle_, CURLOPT_WRITEDATA, this);
curlEasySetoptWrapper(curl_handle_, CURLOPT_PRIVATE, this); // Used by ostree_object_from_curl
Expand Down Expand Up @@ -221,6 +223,7 @@ void OSTreeObject::Upload(const TreehubServer &push_target, CURLM *curl_multi_ha
curlEasySetoptWrapper(curl_handle_, CURLOPT_VERBOSE, get_curlopt_verbose());
current_operation_ = CurrentOp::kOstreeObjectUploading;
push_target.InjectIntoCurl(Url(), curl_handle_);
curlEasySetoptWrapper(curl_handle_, CURLOPT_USERAGENT, Utils::getUserAgent());
curlEasySetoptWrapper(curl_handle_, CURLOPT_WRITEFUNCTION, &OSTreeObject::curl_handle_write);
curlEasySetoptWrapper(curl_handle_, CURLOPT_WRITEDATA, this);
http_response_.str(""); // Empty the response buffer
Expand Down
5 changes: 5 additions & 0 deletions tests/fake_http_server/fake_test_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ def do_GET(self):
for i in range(5):
self.wfile.write(b'aa')
sleep(1)
elif self.path == '/user_agent':
user_agent = self.headers.get('user-agent')
self.send_response(200)
self.end_headers()
self.wfile.write(user_agent.encode())
else:
if self.server.fail_injector is not None and self.server.fail_injector.fail(self):
return
Expand Down
20 changes: 8 additions & 12 deletions tests/test_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,19 +55,17 @@ void TestUtils::writePathToConfig(const boost::filesystem::path &toml_in, const
}

void TestUtils::waitForServer(const std::string &address) {
CURL *handle = curl_easy_init();
if (handle == nullptr) {
throw std::runtime_error("Could not initialize curl handle");
}
curlEasySetoptWrapper(handle, CURLOPT_URL, address.c_str());
curlEasySetoptWrapper(handle, CURLOPT_CONNECTTIMEOUT, 3L);
curlEasySetoptWrapper(handle, CURLOPT_NOBODY, 1L);
curlEasySetoptWrapper(handle, CURLOPT_VERBOSE, 1L);
curlEasySetoptWrapper(handle, CURLOPT_SSL_VERIFYPEER, 0L);
CurlEasyWrapper curl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1


curlEasySetoptWrapper(curl.get(), CURLOPT_URL, address.c_str());
curlEasySetoptWrapper(curl.get(), CURLOPT_CONNECTTIMEOUT, 3L);
curlEasySetoptWrapper(curl.get(), CURLOPT_NOBODY, 1L);
curlEasySetoptWrapper(curl.get(), CURLOPT_VERBOSE, 1L);
curlEasySetoptWrapper(curl.get(), CURLOPT_SSL_VERIFYPEER, 0L);

CURLcode result;
for (size_t counter = 1; counter <= 100; counter++) {
result = curl_easy_perform(handle);
result = curl_easy_perform(curl.get());
if (result == 0) {
// connection successful
break;
Expand All @@ -78,8 +76,6 @@ void TestUtils::waitForServer(const std::string &address) {
std::this_thread::sleep_for(std::chrono::milliseconds(200));
}

curl_easy_cleanup(handle);

if (result != 0) {
throw std::runtime_error("Wait for server timed out");
}
Expand Down