[pacman-dev] [PATCH] Add per-repo PinnedPubKey option
From abb057844eec0e5707c31b643d0f2187b4cf0eb6 Mon Sep 17 00:00:00 2001 From: Travis Burtrum <travis.archlinux@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. Signed-off-by: Travis Burtrum <travis.archlinux@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); /** 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); + } 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 */ + if(strstr(tempcmd, "%p")) { + if(pinnedpubkey == NULL) { + parsedcmd = strreplace(tempcmd, "%p", ""); + } 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
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@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.
Signed-off-by: Travis Burtrum <travis.archlinux@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.
/** 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).
+ 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...
+ } 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
On Mon, 2016-10-31 at 17:03 -0400, 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@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.
Perhaps Pacman should just learn to respect HPKP? It's actually supported by wget now, take a look at ~/.wget-hsts. Pacman could have a similar file in the sync database directory. Then it just kicks in after the first connection and as long as Pacman keeps accessing that mirror it will keep updating the date. It could work quite well since we don't support not upgrading for long periods of time.
On 10/31/2016 05:24 PM, Daniel Micay wrote:
Perhaps Pacman should just learn to respect HPKP? It's actually supported by wget now, take a look at ~/.wget-hsts. Pacman could have a similar file in the sync database directory. Then it just kicks in after the first connection and as long as Pacman keeps accessing that mirror it will keep updating the date. It could work quite well since we don't support not upgrading for long periods of time.
Those are 2 different things though, wget supports HSTS, not HPKP, though pinning public keys is part of HPKP. I plan eventually to write HPKP support for curl/wget, but that's a pretty ambitious project I don't have time for right now. However, with as often as pacman pushes the mirrorlist, it could include just a hard-coded set of hashes for TLS servers. Or a simple script generates and installs them for those who care.
On Tue, 2016-11-01 at 09:44 -0400, Travis Burtrum wrote:
On 10/31/2016 05:24 PM, Daniel Micay wrote:
Perhaps Pacman should just learn to respect HPKP? It's actually supported by wget now, take a look at ~/.wget-hsts. Pacman could have a similar file in the sync database directory. Then it just kicks in after the first connection and as long as Pacman keeps accessing that mirror it will keep updating the date. It could work quite well since we don't support not upgrading for long periods of time.
Those are 2 different things though, wget supports HSTS, not HPKP, though pinning public keys is part of HPKP. I plan eventually to write HPKP support for curl/wget, but that's a pretty ambitious project I don't have time for right now.
However, with as often as pacman pushes the mirrorlist, it could include just a hard-coded set of hashes for TLS servers. Or a simple script generates and installs them for those who care.
Ah, right. HPKP is just that though: a list of key hashes that are permitted (if they appear anywhere in the trust chain). We don't know how mirrors manage their HTTPS keys unless they use HPKP, so what good is pinning them manually? It'll eventually fail, and you can't know if it's an attack or they replaced the certificate.
On 11/01/2016 03:00 PM, Daniel Micay wrote:
We don't know how mirrors manage their HTTPS keys unless they use HPKP, so what good is pinning them manually? It'll eventually fail, and you can't know if it's an attack or they replaced the certificate.
Replacing certificates can, but doesn't need to, change the public key, and with this all you are trusting is the public key. Yes pinned public keys could not and should not be distributed with the mirrorlist unless you can get promises from the individual https mirrors that they will not rotate their public keys without sufficient notice. Again this is going to be generally only useful with a single or small handful of mirrors that you trust specifically. I wanted this because I run my own private mirror and connect from untrusted networks sometimes, some of which will attempt to mitm https connections.
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@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@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
On 01/11/16 06:36, Travis Burtrum wrote:
From abb057844eec0e5707c31b643d0f2187b4cf0eb6 Mon Sep 17 00:00:00 2001 From: Travis Burtrum <travis.archlinux@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.
I agree that this is a per mirror option. It is too out of place as a per repository setting (except maybe when there is only a single sever providing a repo). So I will not accept this patch. Allan
On November 1, 2016 7:28:07 PM EDT, Allan McRae <allan@archlinux.org> wrote:
I agree that this is a per mirror option.
What would you imagine a per mirror option might look like? Someone in IRC today mentioned key value pairs like so: Server = http://foo/ key=value key2=val2 Any thoughts?
On 02/11/16 09:56, Travis Burtrum wrote:
On November 1, 2016 7:28:07 PM EDT, Allan McRae <allan@archlinux.org> wrote:
I agree that this is a per mirror option.
What would you imagine a per mirror option might look like? Someone in IRC today mentioned key value pairs like so:
Server = http://foo/ key=value key2=val2
Any thoughts?
Seems reasonable at first glance. I will need to think about this a bit... Any other opinions? A
On 02.11.2016 02:31, Allan McRae wrote:
On 02/11/16 09:56, Travis Burtrum wrote:
On November 1, 2016 7:28:07 PM EDT, Allan McRae <allan@archlinux.org> wrote: Server = http://foo/ key=value key2=val2
Any other opinions?
Sounds fine since spaces in the URL can/have to be escaped. I'm not so sure about readability though. How about adding support for this? ### Server = http://foo/ [Server] URL = http://bar/ SomeOption = value Key2 = value2 Server = http://blub/ ### Obviously the old "Server = foo" syntax is a shortcut for the full section syntax. This is somewhat weird though so it might make more sense to separate the old style from the section-based ini style and let the user choose how the mirror list is parsed in pacman.conf. Just some ideas, not really fleshed out. Feel free to take them apart. Florian
On 11/02/16 at 11:57am, Florian Pritz via pacman-dev wrote:
On 02.11.2016 02:31, Allan McRae wrote:
On 02/11/16 09:56, Travis Burtrum wrote:
On November 1, 2016 7:28:07 PM EDT, Allan McRae <allan@archlinux.org> wrote: Server = http://foo/ key=value key2=val2
Any other opinions?
Sounds fine since spaces in the URL can/have to be escaped. I'm not so sure about readability though.
How about adding support for this?
### Server = http://foo/
[Server] URL = http://bar/ SomeOption = value Key2 = value2
Server = http://blub/ ###
Obviously the old "Server = foo" syntax is a shortcut for the full section syntax.
This is somewhat weird though so it might make more sense to separate the old style from the section-based ini style and let the user choose how the mirror list is parsed in pacman.conf.
Just some ideas, not really fleshed out. Feel free to take them apart.
Based on your example, it looks like you're suggesting that we make [Server] a sub-section for repos. Aside from the fact that it would add another reserved word that couldn't be used for a repo name, I don't think the ini-style format we used is well-suited to handling sub-sections. apg
participants (6)
-
Allan McRae
-
Andrew Gregory
-
Daniel Micay
-
Dave Reisner
-
Florian Pritz
-
Travis Burtrum