[pacman-dev] [PATCH] Add per-repo PinnedPubKey option
Travis Burtrum
travis.archlinux at burtrum.org
Tue Nov 1 13:40:45 UTC 2016
On 10/31/2016 05:03 PM, Dave Reisner wrote:
> On Mon, Oct 31, 2016 at 04:36:23PM -0400, Travis Burtrum wrote:
>> From abb057844eec0e5707c31b643d0f2187b4cf0eb6 Mon Sep 17 00:00:00 2001
>> From: Travis Burtrum <travis.archlinux at burtrum.org>
>> Date: Mon, 31 Oct 2016 02:12:31 -0400
>> Subject: [PATCH] Add per-repo PinnedPubKey option
>>
>> This sets curl's CURLOPT_PINNEDPUBLICKEY option in the built-in
>> downloader, or replaces %p in XferCommand. This pins public
>> keys to ensure your TLS connection is not man-in-the-middled
>> without relying on CAs etc. Probably most useful currently
>> for very small groups or single servers.
>>
>> It would obviously be best as a per-mirror option, but such
>> a thing currently does not exist.
>
> But perhaps as part of a larger scope, it could... As mentioned on IRC,
> I'm not a huge fan of this.
What would a per-mirror option even look like? Something like:
Server = https://example.org/ sha256//hash=
Would work, but wouldn't be super flexible so more options could be
added in the future.
>> Signed-off-by: Travis Burtrum <travis.archlinux at burtrum.org>
>> ---
>> doc/pacman.conf.5.txt | 12 +++++++++++-
>> lib/libalpm/alpm.h | 10 +++++++++-
>> lib/libalpm/be_sync.c | 4 ++++
>> lib/libalpm/db.c | 12 ++++++++++++
>> lib/libalpm/db.h | 1 +
>> lib/libalpm/dload.c | 8 +++++++-
>> lib/libalpm/dload.h | 1 +
>> lib/libalpm/sync.c | 7 ++++---
>> src/pacman/conf.c | 26 ++++++++++++++++++++++++--
>> src/pacman/conf.h | 1 +
>> 10 files changed, 74 insertions(+), 8 deletions(-)
>>
>> diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt
>> index c665870..b1859c7 100644
>> --- a/doc/pacman.conf.5.txt
>> +++ b/doc/pacman.conf.5.txt
>> @@ -126,7 +126,8 @@ Options
>> All instances of `%u` will be replaced with the download URL. If present,
>> instances of `%o` will be replaced with the local filename, plus a
>> ``.part'' extension, which allows programs like wget to do file resumes
>> - properly.
>> + properly. If present, instances of `%p` will be replaced with the value of
>> + the repo specific PinnedPubKey directive, or `` if that is not set.
>> +
>> This option is useful for users who experience problems with built-in
>> HTTP/FTP support, or need the more advanced proxy support that comes with
>> @@ -276,6 +277,15 @@ even be used for different architectures.
>> Note that an enabled repository can be operated on explicitly, regardless of the Usage
>> level set.
>>
>> +*PinnedPubKey =* pinnedpubkey::
>> + The string can be the file name of your pinned public key. The file format expected
>> + is "PEM" or "DER". The string can also be any number of base64 encoded sha256
>> + hashes preceded by "sha256//" and separated by ";"
>> ++
>> +When negotiating a TLS or SSL connection, the server sends a certificate indicating
>> +its identity. A public key is extracted from this certificate and if it does not
>> +exactly match the public key provided to this option, pacman will abort the
>> +connection before sending or receiving any data.
>>
>> Package and Database Signature Checking[[SC]]
>> ---------------------------------------------
>> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
>> index 2d2491d..e942559 100644
>> --- a/lib/libalpm/alpm.h
>> +++ b/lib/libalpm/alpm.h
>> @@ -750,11 +750,12 @@ typedef void (*alpm_cb_totaldl)(off_t total);
>> * @param url the URL of the file to be downloaded
>> * @param localpath the directory to which the file should be downloaded
>> * @param force whether to force an update, even if the file is the same
>> + * @param pinnedpubkey a pinned public key string
>> * @return 0 on success, 1 if the file exists and is identical, -1 on
>> * error.
>> */
>> typedef int (*alpm_cb_fetch)(const char *url, const char *localpath,
>> - int force);
>> + int force, const char *pinnedpubkey);
>
> If the signature of this must change, I'd strongly suggest we think
> about some sort of more public payload struct which we can extend, and
> shipping that across the callback. However, see below where I explain
> more about why I think this is a Bad Idea.
That does make sense, we could also alternatively define a new callback
function and leave/deprecate this one alone.
>>
>> /** Fetch a remote pkg.
>> * @param handle the context handle
>> @@ -1037,6 +1038,13 @@ alpm_list_t *alpm_db_get_groupcache(alpm_db_t *db);
>> */
>> alpm_list_t *alpm_db_search(alpm_db_t *db, const alpm_list_t *needles);
>>
>> +/** Sets the pinned public key of a database.
>> + * @param db pointer to the package database to set the status for
>> + * @param pinnedpubkey a pinned public key string
>> + * @return 0 on success, or -1 on error
>> + */
>> +int alpm_db_set_pinnedpubkey(alpm_db_t *db, char *pinnedpubkey);
>> +
>> typedef enum _alpm_db_usage_ {
>> ALPM_DB_USAGE_SYNC = 1,
>> ALPM_DB_USAGE_SEARCH = (1 << 1),
>> diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
>> index 7774975..be2cd64 100644
>> --- a/lib/libalpm/be_sync.c
>> +++ b/lib/libalpm/be_sync.c
>> @@ -242,6 +242,8 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>> payload.handle = handle;
>> payload.force = force;
>> payload.unlink_on_fail = 1;
>> +
>> + payload.pinnedpubkey = db->pinnedpubkey;
>>
>> ret = _alpm_download(&payload, syncpath, NULL, &final_db_url);
>> _alpm_dload_payload_reset(&payload);
>> @@ -297,6 +299,8 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>> /* set hard upper limit of 16KiB */
>> payload.max_size = 16 * 1024;
>>
>> + payload.pinnedpubkey = db->pinnedpubkey;
>> +
>> sig_ret = _alpm_download(&payload, syncpath, NULL, NULL);
>> /* errors_ok suppresses error messages, but not the return code */
>> sig_ret = payload.errors_ok ? 0 : sig_ret;
>> diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c
>> index 5d78ee5..0dab1e7 100644
>> --- a/lib/libalpm/db.c
>> +++ b/lib/libalpm/db.c
>> @@ -306,6 +306,15 @@ alpm_list_t SYMEXPORT *alpm_db_search(alpm_db_t *db, const alpm_list_t *needles)
>> return _alpm_db_search(db, needles);
>> }
>>
>> +/** Sets the pinned public key for a repo */
>> +int SYMEXPORT alpm_db_set_pinnedpubkey(alpm_db_t *db, char *pinnedpubkey)
>> +{
>> + ASSERT(db != NULL, return -1);
>> + ASSERT(pinnedpubkey != NULL, return 0);
>> + db->pinnedpubkey = strdup(pinnedpubkey);
>> + return 0;
>> +}
>> +
>> /** Sets the usage bitmask for a repo */
>> int SYMEXPORT alpm_db_set_usage(alpm_db_t *db, int usage)
>> {
>> @@ -351,6 +360,9 @@ void _alpm_db_free(alpm_db_t *db)
>> FREELIST(db->servers);
>> FREE(db->_path);
>> FREE(db->treename);
>> + if(db->pinnedpubkey != NULL) {
>> + FREE(db->pinnedpubkey);
>> + }
>> FREE(db);
>>
>> return;
>> diff --git a/lib/libalpm/db.h b/lib/libalpm/db.h
>> index 8e50196..4ce98c8 100644
>> --- a/lib/libalpm/db.h
>> +++ b/lib/libalpm/db.h
>> @@ -67,6 +67,7 @@ struct __alpm_db_t {
>> char *treename;
>> /* do not access directly, use _alpm_db_path(db) for lazy access */
>> char *_path;
>> + char *pinnedpubkey;
>> alpm_pkghash_t *pkgcache;
>> alpm_list_t *grpcache;
>> alpm_list_t *servers;
>> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
>> index 9d80358..6d5b371 100644
>> --- a/lib/libalpm/dload.c
>> +++ b/lib/libalpm/dload.c
>> @@ -327,6 +327,12 @@ static void curl_set_handle_opts(struct dload_payload *payload,
>> curl_easy_setopt(curl, CURLOPT_USERAGENT, useragent);
>> }
>>
>> + if(payload->pinnedpubkey != NULL) {
>> + _alpm_log(handle, ALPM_LOG_DEBUG,
>> + "using curl pinnedpubkey: %s\n", payload->pinnedpubkey);
>> + curl_easy_setopt(curl, CURLOPT_PINNEDPUBLICKEY, payload->pinnedpubkey);
>> + }
>> +
>> if(!payload->allow_resume && !payload->force && payload->destfile_name &&
>> stat(payload->destfile_name, &st) == 0) {
>> /* start from scratch, but only download if our local is out of date. */
>> @@ -646,7 +652,7 @@ int _alpm_download(struct dload_payload *payload, const char *localpath,
>> RET_ERR(handle, ALPM_ERR_EXTERNAL_DOWNLOAD, -1);
>> #endif
>> } else {
>> - int ret = handle->fetchcb(payload->fileurl, localpath, payload->force);
>> + int ret = handle->fetchcb(payload->fileurl, localpath, payload->force, payload->pinnedpubkey);
>> if(ret == -1 && !payload->errors_ok) {
>> RET_ERR(handle, ALPM_ERR_EXTERNAL_DOWNLOAD, -1);
>> }
>> diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
>> index 427c486..b938fdc 100644
>> --- a/lib/libalpm/dload.h
>> +++ b/lib/libalpm/dload.h
>> @@ -41,6 +41,7 @@ struct dload_payload {
>> int errors_ok;
>> int unlink_on_fail;
>> int trust_remote_name;
>> + char *pinnedpubkey;
>> #ifdef HAVE_LIBCURL
>> CURLcode curlerr; /* last error produced by curl */
>> #endif
>> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
>> index 837639d..05e1ffe 100644
>> --- a/lib/libalpm/sync.c
>> +++ b/lib/libalpm/sync.c
>> @@ -862,7 +862,7 @@ static int validate_deltas(alpm_handle_t *handle, alpm_list_t *deltas)
>> }
>>
>> static struct dload_payload *build_payload(alpm_handle_t *handle,
>> - const char *filename, size_t size, alpm_list_t *servers)
>> + const char *filename, size_t size, alpm_list_t *servers, char *pinnedpubkey)
>> {
>> struct dload_payload *payload;
>>
>> @@ -870,6 +870,7 @@ static struct dload_payload *build_payload(alpm_handle_t *handle,
>> STRDUP(payload->remote_name, filename, FREE(payload); RET_ERR(handle, ALPM_ERR_MEMORY, NULL));
>> payload->max_size = size;
>> payload->servers = servers;
>> + payload->pinnedpubkey = pinnedpubkey;
>> return payload;
>> }
>>
>> @@ -898,7 +899,7 @@ static int find_dl_candidates(alpm_db_t *repo, alpm_list_t **files, alpm_list_t
>> alpm_delta_t *delta = dlts->data;
>> if(delta->download_size != 0) {
>> struct dload_payload *payload = build_payload(
>> - handle, delta->delta, delta->delta_size, repo->servers);
>> + handle, delta->delta, delta->delta_size, repo->servers, repo->pinnedpubkey);
>> ASSERT(payload, return -1);
>> *files = alpm_list_add(*files, payload);
>> }
>> @@ -909,7 +910,7 @@ static int find_dl_candidates(alpm_db_t *repo, alpm_list_t **files, alpm_list_t
>> } else if(spkg->download_size != 0) {
>> struct dload_payload *payload;
>> ASSERT(spkg->filename != NULL, RET_ERR(handle, ALPM_ERR_PKG_INVALID_NAME, -1));
>> - payload = build_payload(handle, spkg->filename, spkg->size, repo->servers);
>> + payload = build_payload(handle, spkg->filename, spkg->size, repo->servers, repo->pinnedpubkey);
>> ASSERT(payload, return -1);
>> *files = alpm_list_add(*files, payload);
>> }
>> diff --git a/src/pacman/conf.c b/src/pacman/conf.c
>> index d8d64fb..8dd23e5 100644
>> --- a/src/pacman/conf.c
>> +++ b/src/pacman/conf.c
>> @@ -163,6 +163,9 @@ void config_repo_free(config_repo_t *repo)
>> if(repo == NULL) {
>> return;
>> }
>> + if(repo->pinnedpubkey != NULL) {
>> + free(repo->pinnedpubkey);
>> + }
>
> No need for the if -- free(NULL) is a no-op.
>
>> free(repo->name);
>> FREELIST(repo->servers);
>> free(repo);
>> @@ -204,7 +207,7 @@ static char *get_tempfile(const char *path, const char *filename)
>>
>> /** External fetch callback */
>> static int download_with_xfercommand(const char *url, const char *localpath,
>> - int force)
>> + int force, const char *pinnedpubkey)
>> {
>> int ret = 0, retval;
>> int usepart = 0;
>> @@ -232,6 +235,16 @@ static int download_with_xfercommand(const char *url, const char *localpath,
>> }
>>
>> tempcmd = strdup(config->xfercommand);
>> + /* replace all occurrences of %p with pinnedpubkey */
>
> Why does this need to exist? You suggested on IRC that the pinned key
> could simply be a list and apply to your semi-trusted set of mirrors.
> You could already hardcode that set in the XferCommand (you could do
> that right now, too, without this patch).
Without it you can't have trust per repo (set of mirrors).
>
>> + if(strstr(tempcmd, "%p")) {
>> + if(pinnedpubkey == NULL) {
>> + parsedcmd = strreplace(tempcmd, "%p", "");
>
> This seems like it would lead to badness and cause transfers to fail in
> bad ways. Something like:
>
> XferCommand = /usr/bin/curl -C - -f --pinnedpubkey %p %u > %o
>
> Would no longer have a URL, as the --pinnedpubkey would swallow it.
> Dealing with this gracefully seems quite difficult. Perhaps another
> reason not to include this new formatter token...
It was either that or leave %p alone, also breaking things, your example
would work though with some quotes:
XferCommand = /usr/bin/curl -C - -f --pinnedpubkey '%p' %u > %o
The only other option is we could replace %p with the full:
--pinnedpubkey hashhere
Which limits it's usefulness to wget and curl, but then the whole .part
business for %o is specific to wget only anyway so maybe it's worth it?
>
>> + } else {
>> + parsedcmd = strreplace(tempcmd, "%p", pinnedpubkey);
>> + }
>> + free(tempcmd);
>> + tempcmd = parsedcmd;
>> + }
>> /* replace all occurrences of %o with fn.part */
>> if(strstr(tempcmd, "%o")) {
>> usepart = 1;
>> @@ -668,6 +681,8 @@ static int register_repo(config_repo_t *repo)
>> repo->name);
>> alpm_db_set_usage(db, repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage);
>>
>> + alpm_db_set_pinnedpubkey(db, repo->pinnedpubkey);
>> +
>> for(i = repo->servers; i; i = alpm_list_next(i)) {
>> char *value = i->data;
>> if(_add_mirror(db, value) != 0) {
>> @@ -915,12 +930,19 @@ static int _parse_repo(const char *key, char *value, const char *file,
>> }
>> FREELIST(values);
>> }
>> + } else if(strcmp(key, "PinnedPubKey") == 0) {
>> + if(!value) {
>> + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: directive '%s' needs a value\n"),
>> + file, line, key);
>> + } else {
>> + repo->pinnedpubkey = strdup(value);
>> + pm_printf(ALPM_LOG_DEBUG, "repo config: pinnedpubkey: %s\n", value);
>> + }
>> } else {
>> pm_printf(ALPM_LOG_WARNING,
>> _("config file %s, line %d: directive '%s' in section '%s' not recognized.\n"),
>> file, line, key, repo->name);
>> }
>> -
>> return ret;
>> }
>>
>> diff --git a/src/pacman/conf.h b/src/pacman/conf.h
>> index 945de7c..7c021bf 100644
>> --- a/src/pacman/conf.h
>> +++ b/src/pacman/conf.h
>> @@ -40,6 +40,7 @@ typedef struct __config_repo_t {
>> int usage;
>> int siglevel;
>> int siglevel_mask;
>> + char *pinnedpubkey;
>> } config_repo_t;
>>
>> typedef struct __config_t {
>> --
>> 2.10.2
More information about the pacman-dev
mailing list