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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libstore: Enable kerberos negotiation for http binary caches #10568

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

libstore: Enable kerberos negotiation for http binary caches #10568

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/libstore/filetransfer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ struct curlFileTransfer : public FileTransfer
curl_easy_setopt(req, CURLOPT_PIPEWAIT, 1);
#endif
#if LIBCURL_VERSION_NUM >= 0x072f00
if (fileTransferSettings.enableHttp2)
if (fileTransferSettings.enableHttp2 && !request.negotiate)
curl_easy_setopt(req, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2TLS);
else
curl_easy_setopt(req, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1);
Expand Down Expand Up @@ -353,6 +353,12 @@ struct curlFileTransfer : public FileTransfer
curl_easy_setopt(req, CURLOPT_SSL_VERIFYHOST, 0);
}

if (request.negotiate) {
curl_easy_setopt(req, CURLOPT_HTTPAUTH, CURLAUTH_NEGOTIATE);
curl_easy_setopt(req, CURLOPT_USERNAME, "");
curl_easy_setopt(req, CURLOPT_PASSWORD, "");
}

curl_easy_setopt(req, CURLOPT_CONNECTTIMEOUT, fileTransferSettings.connectTimeout.get());

curl_easy_setopt(req, CURLOPT_LOW_SPEED_LIMIT, 1L);
Expand Down
1 change: 1 addition & 0 deletions src/libstore/filetransfer.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ struct FileTransferRequest
std::string expectedETag;
bool verifyTLS = true;
bool head = false;
bool negotiate = false;
size_t tries = fileTransferSettings.tries;
unsigned int baseRetryTimeMs = 250;
ActivityId parentAct;
Expand Down
7 changes: 6 additions & 1 deletion src/libstore/http-binary-cache-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ struct HttpBinaryCacheStoreConfig : virtual BinaryCacheStoreConfig

const std::string name() override { return "HTTP Binary Cache Store"; }

const Setting<bool> negotiate{this, false, "negotiate",
"Whether to do kerberos negotiate when talking to the http binary cache."};
Comment on lines +17 to +18
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be a configuration item, or could it be autodetected?

I think negotiate is a bit vague. Would kerberos be a better name?

Copy link
Author

Choose a reason for hiding this comment

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

We could add CURLAUTH_ANY instead of CURLAUTH_NEGOITATE, which would do the detection for you. We could add that flag at all times, and libcurl will use the best available authentication it finds. This is what git does, and that works pretty well. The downside is that if the HTTP server will accept negotiation OR another protocol, libcurl will always choose negotiation, which has a very slim chance of breaking use cases. This may be a very acceptable risk as git has been doing this for a decade already.

negotiate is the best name here, it matches the curl command line argument --negotiate, and is widely known as that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not too familiar with that, but it's generally good to mirror existing interfaces.
What about CURLAUTH_ANYSAFE ?
Maybe we should similarly expose a single auth setting, instead of specific ones for each possible solution.

Copy link
Author

Choose a reason for hiding this comment

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

A decent idea, I've created #10584 as an alternate proposal to this one.


std::string doc() override
{
return
Expand Down Expand Up @@ -144,10 +147,12 @@ class HttpBinaryCacheStore : public virtual HttpBinaryCacheStoreConfig, public v

FileTransferRequest makeRequest(const std::string & path)
{
return FileTransferRequest(
auto request = FileTransferRequest(
hasPrefix(path, "https://") || hasPrefix(path, "http://") || hasPrefix(path, "file://")
? path
: cacheUri + "/" + path);
request.negotiate = negotiate;
return request;

}

Expand Down