[pacman-dev] [PATCH 0/4] addition of API methods and backend work

newer
[pacman-dev] [GIT] The official...

older
[pacman-dev] [GIT] The official...

Dave Reisner

28 Mar 2011 28 Mar '11

7:15 p.m.

I've had the ensuing patches on my tree for a few days. A quick outline of what they accomplish: 1) Replace the old libfetch method of setting useragent (via an environment var) with proper get/set methods. 2) Refactor signature downloading to ensure that the sig file is downloaded from the same mirror as the file itself. 3) Add handling for VerifySig in the [options] section of pacman.conf. This should probably be split into two patches (front end/back end), as it adds another API interface. I'm not sold on the naming convention I've used either. Feedback (and dan's criticism) is welcome. d

Show replies by date

Dave Reisner

28 Mar 28 Mar

7:15 p.m.

New subject: [pacman-dev] [PATCH 1/4] lib/handle: add get/set methods for useragent

Signed-off-by: Dave Reisner --- lib/libalpm/alpm.h | 3 +++ lib/libalpm/dload.c | 4 ++-- lib/libalpm/handle.c | 15 +++++++++++++++ lib/libalpm/handle.h | 1 + 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index b08191d..43ff4ba 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -165,6 +165,9 @@ void alpm_option_set_checkspace(int checkspace); pmdb_t *alpm_option_get_localdb(void); alpm_list_t *alpm_option_get_syncdbs(void); +const char *alpm_option_get_useragent(void); +void alpm_option_set_useragent(char *useragent); + /* * Install reasons -- ie, why the package was installed */ diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index d9e9488..948e623 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -193,8 +193,8 @@ static int curl_download_internal(const char *url, const char *localpath, curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void*)&dlfile); - useragent = getenv("HTTP_USER_AGENT"); - if (useragent != NULL) { + useragent = alpm_option_get_useragent(); + if(useragent != NULL) { curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); } diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 42c0cd1..df4ed54 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -90,6 +90,7 @@ void _alpm_handle_free(pmhandle_t *handle) FREELIST(handle->noextract); FREELIST(handle->ignorepkg); FREELIST(handle->ignoregrp); + FREE(handle->useragent); FREE(handle); } @@ -597,4 +598,18 @@ void SYMEXPORT alpm_option_set_checkspace(int checkspace) handle->checkspace = checkspace; } +void SYMEXPORT alpm_option_set_useragent(char *useragent) +{ + if(handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } + handle->useragent = useragent; +} + +const char SYMEXPORT *alpm_option_get_useragent() +{ + return handle->useragent; +} + /* vim: set ts=2 sw=2 noet: */ diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index cf192bc..2a0bcd4 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -72,6 +72,7 @@ typedef struct _pmhandle_t { char *arch; /* Architecture of packages we should allow */ int usedelta; /* Download deltas if possible */ int checkspace; /* Check disk space before installing */ + char *useragent; /* User agent string to set for internal downloader */ } pmhandle_t; /* global handle variable */ -- 1.7.4.2

Rémy Oudompheng

7:21 p.m.

New subject: [pacman-dev] [PATCH 1/4] lib/handle: add get/set methods for useragent

On 2011/3/28 Dave Reisner wrote:

...

@@ -597,4 +598,18 @@ void SYMEXPORT alpm_option_set_checkspace(int checkspace) handle->checkspace = checkspace; }

+void SYMEXPORT alpm_option_set_useragent(char *useragent) +{ + if(handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } + handle->useragent = useragent; +} + +const char SYMEXPORT *alpm_option_get_useragent() +{ + return handle->useragent; +} + /* vim: set ts=2 sw=2 noet: */

I'd rather have alpm_option_set_useragent() return an int, just like alpm_option_set_root(), and make alpm_option_get_useragent() return NULL and set pm_errno if handle is NULL, just like alpm_option_set_root(). As a matter or personal taste (and a bit of consistency obsession). -- Rémy.

Dan McGee

21 Apr 21 Apr

midnight

New subject: [pacman-dev] [PATCH 1/4] lib/handle: add get/set methods for useragent

On Mon, Mar 28, 2011 at 2:15 PM, Dave Reisner wrote:

...

Signed-off-by: Dave Reisner I'm not applying these, I think our current approach is reasonable enough.

Dave Reisner

28 Mar 28 Mar

7:15 p.m.

New subject: [pacman-dev] [PATCH 2/4] pacman: use alpm method to set useragent

The call for this needs to be done a little later now since it requires that the handle be initialized. Signed-off-by: Dave Reisner --- src/pacman/pacman.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index eac9c9c..381d2bc 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -253,13 +253,13 @@ static void localize(void) */ static void setuseragent(void) { - char agent[101]; + char *agent; struct utsname un; uname(&un); - snprintf(agent, 100, "pacman/%s (%s %s) libalpm/%s", + pm_asprintf(&agent, "pacman/%s (%s %s) libalpm/%s", PACKAGE_VERSION, un.sysname, un.machine, alpm_version()); - setenv("HTTP_USER_AGENT", agent, 0); + alpm_option_set_useragent(agent); } static void setarch(const char *arch) @@ -1360,9 +1360,6 @@ int main(int argc, char *argv[]) localize(); #endif - /* set user agent for downloading */ - setuseragent(); - /* init config data */ config = config_new(); @@ -1387,6 +1384,9 @@ int main(int argc, char *argv[]) alpm_option_set_signaturedir(GPGDIR); alpm_option_set_logfile(LOGFILE); + /* set user agent for downloading */ + setuseragent(); + /* Priority of options: * 1. command line * 2. config file -- 1.7.4.2

Dave Reisner

7:15 p.m.

New subject: [pacman-dev] [PATCH 3/4] lib/dload: major refactor of signature downloading

There's a lot of related moving parts here: * iteration through mirrors is moved back to the calling functions. this allows removal of _alpm_download_single_file and _alpm_download_files * rename download => _alpm_download. also modified to accept an extra arg of type pgp_verify_t which is passed through to curl_download_internal * move the actual signature download to curl_download_internal. this allows us to ensure that the signature and the file came from the same mirror, and only to accept the file if signature verification passes. Signed-off-by: Dave Reisner --- lib/libalpm/be_sync.c | 71 ++++++---------------- lib/libalpm/dload.c | 159 +++++++++++++++++++++++++++++-------------------- lib/libalpm/dload.h | 8 +-- lib/libalpm/sync.c | 27 +++++++- 4 files changed, 139 insertions(+), 126 deletions(-) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 756f784..ce62c2b 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -79,11 +79,12 @@ */ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) { - char *dbfile, *syncpath; + char *syncpath; const char *dbpath; + alpm_list_t *i; struct stat buf; size_t len; - int ret; + int ret = -1; mode_t oldmask; ALPM_LOG_FUNC; @@ -91,14 +92,7 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) /* Sanity checks */ ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); ASSERT(db != NULL && db != handle->db_local, RET_ERR(PM_ERR_WRONG_ARGS, -1)); - - if(!alpm_list_find_ptr(handle->dbs_sync, db)) { - RET_ERR(PM_ERR_DB_NOT_FOUND, -1); - } - - len = strlen(db->treename) + 4; - MALLOC(dbfile, len, RET_ERR(PM_ERR_MEMORY, -1)); - sprintf(dbfile, "%s.db", db->treename); + ASSERT(db->servers != NULL, RET_ERR(PM_ERR_SERVER_NONE, -1)); dbpath = alpm_option_get_dbpath(); len = strlen(dbpath) + 6; @@ -112,20 +106,33 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) _alpm_log(PM_LOG_DEBUG, "database dir '%s' does not exist, creating it\n", syncpath); if(_alpm_makepath(syncpath) != 0) { - free(dbfile); free(syncpath); RET_ERR(PM_ERR_SYSTEM, -1); } } else if(!S_ISDIR(buf.st_mode)) { _alpm_log(PM_LOG_WARNING, _("removing invalid file: %s\n"), syncpath); if(unlink(syncpath) != 0 || _alpm_makepath(syncpath) != 0) { - free(dbfile); free(syncpath); RET_ERR(PM_ERR_SYSTEM, -1); } } - ret = _alpm_download_single_file(dbfile, db->servers, syncpath, force); + for(i = db->servers; i; i = i->next) { + const char *server = i->data; + char *fileurl; + size_t len; + + /* print server + filename into a buffer */ + len = strlen(server) + strlen(db->treename) + 5; + CALLOC(fileurl, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, -1)); + snprintf(fileurl, len, "%s/%s.db", server, db->treename); + + ret = _alpm_download(fileurl, syncpath, force, db->pgp_verify); + FREE(fileurl); + if(ret != -1) { + break; + } + } if(ret == 1) { /* files match, do nothing */ @@ -137,49 +144,11 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) goto cleanup; } - /* Download and check the signature of the database if needed */ - if(db->pgp_verify != PM_PGP_VERIFY_NEVER) { - char *sigfile, *sigfilepath; - int sigret; - - len = strlen(dbfile) + 5; - MALLOC(sigfile, len, RET_ERR(PM_ERR_MEMORY, -1)); - sprintf(sigfile, "%s.sig", dbfile); - - /* prevent old signature being used if the following download fails */ - len = strlen(syncpath) + strlen(sigfile) + 1; - MALLOC(sigfilepath, len, RET_ERR(PM_ERR_MEMORY, -1)); - sprintf(sigfilepath, "%s%s", syncpath, sigfile); - _alpm_rmrf(sigfilepath); - free(sigfilepath); - - sigret = _alpm_download_single_file(sigfile, db->servers, syncpath, 0); - free(sigfile); - - if(sigret == -1 && db->pgp_verify == PM_PGP_VERIFY_ALWAYS) { - _alpm_log(PM_LOG_ERROR, _("Failed to download signature for db: %s\n"), - alpm_strerrorlast()); - pm_errno = PM_ERR_SIG_INVALID; - ret = -1; - goto cleanup; - } - - sigret = alpm_db_check_pgp_signature(db); - if((db->pgp_verify == PM_PGP_VERIFY_ALWAYS && sigret != 0) || - (db->pgp_verify == PM_PGP_VERIFY_OPTIONAL && sigret == 1)) { - /* pm_errno was set by the checking code */ - /* TODO: should we just leave the unverified database */ - ret = -1; - goto cleanup; - } - } - /* Cache needs to be rebuilt */ _alpm_db_free_pkgcache(db); cleanup: - free(dbfile); free(syncpath); umask(oldmask); return ret; diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 948e623..69fc708 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -155,15 +155,15 @@ static int utimes_long(const char *path, long time) static int curl_download_internal(const char *url, const char *localpath, - int force) + int force, pgp_verify_t check_sig) { - int ret = -1; + int ret = -1, sig_ret = -1; FILE *localf = NULL; const char *open_mode, *useragent; char *destfile, *tempfile; char hostname[256]; /* RFC1123 states applications should support this length */ struct stat st; - long httpresp, timecond, remote_time; + long resp_code, timecond, remote_time; double remote_size, bytes_dl; struct sigaction sig_pipe[2], sig_int[2]; struct fileinfo dlfile; @@ -242,7 +242,7 @@ static int curl_download_internal(const char *url, const char *localpath, handle->curlerr = curl_easy_perform(handle->curl); /* retrieve info about the state of the transfer */ - curl_easy_getinfo(handle->curl, CURLINFO_RESPONSE_CODE, &httpresp); + curl_easy_getinfo(handle->curl, CURLINFO_RESPONSE_CODE, &resp_code); curl_easy_getinfo(handle->curl, CURLINFO_FILETIME, &remote_time); curl_easy_getinfo(handle->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &remote_size); curl_easy_getinfo(handle->curl, CURLINFO_SIZE_DOWNLOAD, &bytes_dl); @@ -279,15 +279,92 @@ static int curl_download_internal(const char *url, const char *localpath, ret = 0; + /* we were successful downloading the file. now we need to fetch a signature + * if necessary. we already know that a new file was actually fetched; the + * not modified case was handled and we jumped to label cleanup already. */ + if(check_sig != PM_PGP_VERIFY_NEVER) { + FILE *sigf = NULL; + char *sig_url, *sig_temp = NULL, *sig_dest = NULL; + size_t len; + CURLcode sig_curlerr; + + /* if we enter this block, assume failure unless we explicitly state + * otherwise */ + sig_ret = -1; + + len = strlen(url) + 5; + CALLOC(sig_url, len, sizeof(char), goto sig_cleanup); + snprintf(sig_url, len, "%s.sig", url); + + dlfile.filename = get_filename(sig_url); + dlfile.initial_size = 0; + + sig_dest = get_fullpath(localpath, dlfile.filename, ""); + sig_temp = get_fullpath(localpath, dlfile.filename, ".part"); + + curl_easy_setopt(handle->curl, CURLOPT_URL, sig_url); + curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)&dlfile); + curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, 0L); + curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, 0L); + curl_easy_setopt(handle->curl, CURLOPT_RESUME_FROM, 0L); + + sigf = fopen(sig_temp, "wb"); + if(sigf == NULL) { + goto sig_cleanup; + } + curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, sigf); + + /* Progress 0 - initialize */ + prevprogress = 0; + + /* perform transfer */ + sig_curlerr = curl_easy_perform(handle->curl); + + /* retrieve info about the state of the transfer */ + curl_easy_getinfo(handle->curl, CURLINFO_RESPONSE_CODE, &resp_code); + curl_easy_getinfo(handle->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &remote_size); + curl_easy_getinfo(handle->curl, CURLINFO_SIZE_DOWNLOAD, &bytes_dl); + + if(sig_curlerr == CURLE_ABORTED_BY_CALLBACK) { + goto sig_cleanup; + } else if((sig_curlerr == CURLE_REMOTE_FILE_NOT_FOUND + || sig_curlerr == CURLE_FTP_COULDNT_RETR_FILE + || (sig_curlerr == CURLE_HTTP_RETURNED_ERROR && resp_code == 404)) + && check_sig != PM_PGP_VERIFY_ALWAYS) { + /* an error case that is not fatal */ + sig_ret = 1; + goto sig_cleanup; + } else if(sig_curlerr != CURLE_OK) { + pm_errno = PM_ERR_LIBCURL; + handle->curlerr = sig_curlerr; + _alpm_log(PM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"), + dlfile.filename, hostname, curl_easy_strerror(handle->curlerr)); + /* unlink both files- they are only good if they both were downloaded */ + unlink(sig_temp); + unlink(tempfile); + goto sig_cleanup; + } + + sig_ret = 0; + +sig_cleanup: + if(sigf) { + fclose(sigf); + } + if(sig_ret == 0) { + rename(sig_temp, sig_dest); + } + FREE(sig_temp); + FREE(sig_dest); + } + cleanup: if(localf != NULL) { fclose(localf); utimes_long(tempfile, remote_time); } - /* TODO: A signature download will need to return success here as well before - * we're willing to rotate the new file into place. */ - if(ret == 0) { + if(ret == 0 && sig_ret != 0) { rename(tempfile, destfile); } @@ -306,12 +383,12 @@ cleanup: } #endif -static int download(const char *url, const char *localpath, - int force) +int _alpm_download(const char *url, const char *localpath, + int force, pgp_verify_t check_sig) { if(handle->fetchcb == NULL) { #ifdef HAVE_LIBCURL - return curl_download_internal(url, localpath, force); + return curl_download_internal(url, localpath, force, check_sig); #else RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1); #endif @@ -320,62 +397,14 @@ static int download(const char *url, const char *localpath, if(ret == -1) { RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1); } - return ret; - } -} - -/* - * Download a single file - * - servers must be a list of urls WITHOUT trailing slashes. - * - * RETURN: 0 for successful download - * 1 if the files are identical - * -1 on error - */ -int _alpm_download_single_file(const char *filename, - alpm_list_t *servers, const char *localpath, - int force) -{ - alpm_list_t *i; - int ret = -1; - - ASSERT(servers != NULL, RET_ERR(PM_ERR_SERVER_NONE, -1)); - - for(i = servers; i; i = i->next) { - const char *server = i->data; - char *fileurl = NULL; - size_t len; - - /* print server + filename into a buffer */ - len = strlen(server) + strlen(filename) + 2; - CALLOC(fileurl, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, -1)); - snprintf(fileurl, len, "%s/%s", server, filename); - - ret = download(fileurl, localpath, force); - FREE(fileurl); - if(ret != -1) { - break; - } - } - - return ret; -} - -int _alpm_download_files(alpm_list_t *files, - alpm_list_t *servers, const char *localpath) -{ - int ret = 0; - alpm_list_t *lp; - - for(lp = files; lp; lp = lp->next) { - char *filename = lp->data; - if(_alpm_download_single_file(filename, servers, - localpath, 0) == -1) { - ret++; + if(check_sig != PM_PGP_VERIFY_NEVER) { + int ret = handle->fetchcb(url, localpath, force); + if(ret == -1 && check_sig == PM_PGP_VERIFY_ALWAYS) { + RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1); + } } + return ret; } - - return ret; } /** Fetch a remote pkg. @@ -397,7 +426,7 @@ char SYMEXPORT *alpm_fetch_pkgurl(const char *url) cachedir = _alpm_filecache_setup(); /* download the file */ - ret = download(url, cachedir, 0); + ret = _alpm_download(url, cachedir, 0, 1); if(ret == -1) { _alpm_log(PM_LOG_WARNING, _("failed to download %s\n"), url); return NULL; diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h index 5ce44b8..60588ee 100644 --- a/lib/libalpm/dload.h +++ b/lib/libalpm/dload.h @@ -31,12 +31,8 @@ struct fileinfo { double initial_size; }; -int _alpm_download_single_file(const char *filename, - alpm_list_t *servers, const char *localpath, - int force); - -int _alpm_download_files(alpm_list_t *files, - alpm_list_t *servers, const char *localpath); +int _alpm_download(const char *url, const char *localpath, + int force, pgp_verify_t check_sig); #endif /* _ALPM_DLOAD_H */ diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 5428e40..bd4d8e9 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -689,8 +689,8 @@ static int test_md5sum(pmtrans_t *trans, const char *filepath, int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) { - alpm_list_t *i, *j, *files = NULL; - alpm_list_t *deltas = NULL; + alpm_list_t *i, *j, *k; + alpm_list_t *files = NULL, *deltas = NULL; size_t numtargs, current = 0, replaces = 0; int errors = 0; const char *cachedir = NULL; @@ -758,9 +758,28 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) if(files) { EVENT(trans, PM_TRANS_EVT_RETRIEVE_START, current->treename, NULL); - errors = _alpm_download_files(files, current->servers, cachedir); + for(j = files; j; j = j->next) { + const char *filename = j->data; + for(k = current->servers; k; k = k->next) { + const char *server = k->data; + char *fileurl; + size_t len; + + /* print server + filename into a buffer */ + len = strlen(server) + strlen(filename) + 2; + CALLOC(fileurl, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, -1)); + snprintf(fileurl, len, "%s/%s", server, filename); + + ret = _alpm_download(fileurl, cachedir, 0, 0); + FREE(fileurl); + if(ret != -1) { + break; + } + errors++; + } + } - if (errors) { + if(errors) { _alpm_log(PM_LOG_WARNING, _("failed to retrieve some files from %s\n"), current->treename); if(pm_errno == 0) { -- 1.7.4.2

Dan McGee

21 Apr 21 Apr

12:03 a.m.

New subject: [pacman-dev] [PATCH 3/4] lib/dload: major refactor of signature downloading

On Mon, Mar 28, 2011 at 2:15 PM, Dave Reisner wrote:

...

There's a lot of related moving parts here: * iteration through mirrors is moved back to the calling functions. this allows removal of _alpm_download_single_file and _alpm_download_files * rename download => _alpm_download. also modified to accept an extra arg of type pgp_verify_t which is passed through to curl_download_internal * move the actual signature download to curl_download_internal. this allows us to ensure that the signature and the file came from the same mirror, and only to accept the file if signature verification passes.

Signed-off-by: Dave Reisner

This one needs a little more testing...note the (now) non-existant core.db file, and the completely bogus community2. dmcgee@galway ~/projects/pacman (master) $ sudo rm /var/lib/pacman/sync/core.db dmcgee@galway ~/projects/pacman (master) $ sudo ./src/pacman/pacman -Sy :: Synchronizing package databases... testing is up to date core is up to date extra is up to date community-testing is up to date multilib is up to date community is up to date community2 is up to date -Dan

Dave Reisner

12:12 a.m.

New subject: [pacman-dev] [PATCH 3/4] lib/dload: major refactor of signature downloading

On Wed, Apr 20, 2011 at 07:03:49PM -0500, Dan McGee wrote:

...

On Mon, Mar 28, 2011 at 2:15 PM, Dave Reisner wrote:

...
There's a lot of related moving parts here: * iteration through mirrors is moved back to the calling functions. this allows removal of _alpm_download_single_file and _alpm_download_files * rename download => _alpm_download. also modified to accept an extra arg of type pgp_verify_t which is passed through to curl_download_internal * move the actual signature download to curl_download_internal. this allows us to ensure that the signature and the file came from the same mirror, and only to accept the file if signature verification passes.

Signed-off-by: Dave Reisner

This one needs a little more testing...note the (now) non-existant core.db file, and the completely bogus community2.

dmcgee@galway ~/projects/pacman (master) $ sudo rm /var/lib/pacman/sync/core.db

dmcgee@galway ~/projects/pacman (master) $ sudo ./src/pacman/pacman -Sy :: Synchronizing package databases... testing is up to date core is up to date extra is up to date community-testing is up to date multilib is up to date community is up to date community2 is up to date

-Dan

Good choice. I've noticed some other garbage in these patches as well. Hoping to tackle a lot of this next week when I'm off from work. d

Dave Reisner

28 Mar 28 Mar

7:15 p.m.

New subject: [pacman-dev] [PATCH 4/4] Allow VerifySig to act as a default verification in [options]

* add _alpm_get_sigverify_level * add alpm_option_{get,set}_default_sigverify This requires moving around protos in alpm.h so that pgp_verify_t is known to the get/set methods for default_sigverify. Signed-off-by: Dave Reisner --- lib/libalpm/alpm.h | 36 ++++++++++++++++++++---------------- lib/libalpm/be_sync.c | 2 +- lib/libalpm/handle.c | 12 ++++++++++++ lib/libalpm/handle.h | 11 ++++++----- lib/libalpm/signing.c | 21 +++++++++++++++++++-- lib/libalpm/signing.h | 1 + lib/libalpm/sync.c | 17 ++++++++++++++--- src/pacman/pacman.c | 20 ++++++++++++++++---- 8 files changed, 89 insertions(+), 31 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 43ff4ba..8d97aac 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -95,6 +95,23 @@ typedef int (*alpm_cb_fetch)(const char *url, const char *localpath, int force); /* + * Signatures + */ + +int alpm_pkg_check_pgp_signature(pmpkg_t *pkg); +int alpm_db_check_pgp_signature(pmdb_t *db); + +/* GPG signature verification option */ +typedef enum _pgp_verify_t { + PM_PGP_VERIFY_UNKNOWN, + PM_PGP_VERIFY_ALWAYS, + PM_PGP_VERIFY_OPTIONAL, + PM_PGP_VERIFY_NEVER +} pgp_verify_t; + +int alpm_db_set_pgp_verify(pmdb_t *db, pgp_verify_t verify); + +/* * Options */ @@ -168,6 +185,9 @@ alpm_list_t *alpm_option_get_syncdbs(void); const char *alpm_option_get_useragent(void); void alpm_option_set_useragent(char *useragent); +pgp_verify_t alpm_option_get_default_sigverify(void); +void alpm_option_set_default_sigverify(pgp_verify_t level); + /* * Install reasons -- ie, why the package was installed */ @@ -249,22 +269,6 @@ off_t alpm_pkg_download_size(pmpkg_t *newpkg); alpm_list_t *alpm_pkg_unused_deltas(pmpkg_t *pkg); /* - * Signatures - */ - -int alpm_pkg_check_pgp_signature(pmpkg_t *pkg); -int alpm_db_check_pgp_signature(pmdb_t *db); - -/* GPG signature verification option */ -typedef enum _pgp_verify_t { - PM_PGP_VERIFY_ALWAYS, - PM_PGP_VERIFY_OPTIONAL, - PM_PGP_VERIFY_NEVER -} pgp_verify_t; - -int alpm_db_set_pgp_verify(pmdb_t *db, pgp_verify_t verify); - -/* * Deltas */ diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index ce62c2b..4c1201a 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -127,7 +127,7 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) CALLOC(fileurl, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, -1)); snprintf(fileurl, len, "%s/%s.db", server, db->treename); - ret = _alpm_download(fileurl, syncpath, force, db->pgp_verify); + ret = _alpm_download(fileurl, syncpath, force, _alpm_get_sigverify_level(db)); FREE(fileurl); if(ret != -1) { break; diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index df4ed54..d211f16 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -50,6 +50,8 @@ pmhandle_t *_alpm_handle_new() CALLOC(handle, 1, sizeof(pmhandle_t), RET_ERR(PM_ERR_MEMORY, NULL)); + handle->sigverify = PM_PGP_VERIFY_ALWAYS; + return handle; } @@ -612,4 +614,14 @@ const char SYMEXPORT *alpm_option_get_useragent() return handle->useragent; } +void SYMEXPORT alpm_option_set_default_sigverify(pgp_verify_t level) +{ + handle->sigverify = level; +} + +pgp_verify_t SYMEXPORT alpm_option_get_default_sigverify() +{ + return handle->sigverify; +} + /* vim: set ts=2 sw=2 noet: */ diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 2a0bcd4..74c2e8e 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -68,11 +68,12 @@ typedef struct _pmhandle_t { alpm_list_t *ignoregrp; /* List of groups to ignore */ /* options */ - int usesyslog; /* Use syslog instead of logfile? */ /* TODO move to frontend */ - char *arch; /* Architecture of packages we should allow */ - int usedelta; /* Download deltas if possible */ - int checkspace; /* Check disk space before installing */ - char *useragent; /* User agent string to set for internal downloader */ + int usesyslog; /* Use syslog instead of logfile? */ /* TODO move to frontend */ + char *arch; /* Architecture of packages we should allow */ + int usedelta; /* Download deltas if possible */ + int checkspace; /* Check disk space before installing */ + char *useragent; /* User agent string to set for internal downloader */ + pgp_verify_t sigverify; /* Default signature verification level */ } pmhandle_t; /* global handle variable */ diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c index c30650b..bedf3bf 100644 --- a/lib/libalpm/signing.c +++ b/lib/libalpm/signing.c @@ -249,6 +249,24 @@ int _alpm_load_signature(const char *sigfile, pmpgpsig_t *pgpsig) { } /** + * Determines the necessity of checking for a valid pgp signature + * @param db the sync db to query + * + * @return signature verification level + */ +pgp_verify_t _alpm_get_sigverify_level(pmdb_t *db) +{ + ALPM_LOG_FUNC; + ASSERT(db != NULL, return PM_PGP_VERIFY_UNKNOWN); + + if(db->pgp_verify != PM_PGP_VERIFY_UNKNOWN) { + return db->pgp_verify; + } else { + return alpm_option_get_default_sigverify(); + } +} + +/** * Check the PGP package signature for the given package file. * @param pkg the package to check * @return a int value : 0 (valid), 1 (invalid), -1 (an error occured) @@ -270,11 +288,10 @@ int SYMEXPORT alpm_pkg_check_pgp_signature(pmpkg_t *pkg) int SYMEXPORT alpm_db_check_pgp_signature(pmdb_t *db) { ALPM_LOG_FUNC; - ASSERT(db != NULL, return(0)); + ASSERT(db != NULL, return 0); return _alpm_gpgme_checksig(_alpm_db_path(db), _alpm_db_pgpsig(db)); } - /* vim: set ts=2 sw=2 noet: */ diff --git a/lib/libalpm/signing.h b/lib/libalpm/signing.h index b37abf0..d4b55de 100644 --- a/lib/libalpm/signing.h +++ b/lib/libalpm/signing.h @@ -31,6 +31,7 @@ struct __pmpgpsig_t { unsigned char *rawdata; }; +pgp_verify_t _alpm_get_sigverify_level(pmdb_t *db); int _alpm_gpgme_checksig(const char *path, const pmpgpsig_t *sig); int _alpm_load_signature(const char *sigfile, pmpgpsig_t *pgpsig); diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index bd4d8e9..2c0e54b 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -857,6 +857,7 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) char *filepath = _alpm_filecache_find(filename); const char *md5sum = alpm_pkg_get_md5sum(spkg); const pmpgpsig_t *pgpsig = alpm_pkg_get_pgpsig(spkg); + pgp_verify_t check_sig; /* check md5sum first */ if(test_md5sum(trans, filepath, md5sum) != 0) { @@ -868,10 +869,20 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) /* check PGP signature next */ pmdb_t *sdb = alpm_pkg_get_db(spkg); - if(sdb->pgp_verify != PM_PGP_VERIFY_NEVER) { + check_sig = _alpm_get_sigverify_level(sdb); + + /* fprintf(stderr, "check_sig for %s = %d\n", sdb->treename, check_sig); */ + + if(check_sig == PM_PGP_VERIFY_UNKNOWN) { + _alpm_log(PM_LOG_ERROR, "failed to determine signature verification level " + "for db: %s\n", sdb->treename); + goto error; + } + + if(check_sig != PM_PGP_VERIFY_NEVER) { int ret = _alpm_gpgme_checksig(filepath, pgpsig); - if((sdb->pgp_verify == PM_PGP_VERIFY_ALWAYS && ret != 0) || - (sdb->pgp_verify == PM_PGP_VERIFY_OPTIONAL && ret == 1)) { + if((check_sig == PM_PGP_VERIFY_ALWAYS && ret != 0) || + (check_sig == PM_PGP_VERIFY_OPTIONAL && ret == 1)) { errors++; *data = alpm_list_add(*data, strdup(filename)); FREE(filepath); diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 381d2bc..012043d 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -1038,22 +1038,34 @@ static int _parse_options(const char *key, char *value, config->rootdir = strdup(value); pm_printf(PM_LOG_DEBUG, "config: rootdir: %s\n", value); } - } else if (strcmp(key, "GPGDir") == 0) { + } else if(strcmp(key, "GPGDir") == 0) { if(!config->gpgdir) { config->gpgdir = strdup(value); pm_printf(PM_LOG_DEBUG, "config: gpgdir: %s\n", value); } - } else if (strcmp(key, "LogFile") == 0) { + } else if(strcmp(key, "LogFile") == 0) { if(!config->logfile) { config->logfile = strdup(value); pm_printf(PM_LOG_DEBUG, "config: logfile: %s\n", value); } - } else if (strcmp(key, "XferCommand") == 0) { + } else if(strcmp(key, "XferCommand") == 0) { config->xfercommand = strdup(value); alpm_option_set_fetchcb(download_with_xfercommand); pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", value); - } else if (strcmp(key, "CleanMethod") == 0) { + } else if(strcmp(key, "CleanMethod") == 0) { setrepeatingoption(value, "CleanMethod", option_add_cleanmethod); + } else if(strcmp(key, "VerifySig") == 0) { + if (strcmp(value, "Always") == 0) { + alpm_option_set_default_sigverify(PM_PGP_VERIFY_ALWAYS); + } else if(strcmp(value, "Optional") == 0) { + alpm_option_set_default_sigverify(PM_PGP_VERIFY_OPTIONAL); + } else if(strcmp(value, "Never") == 0) { + alpm_option_set_default_sigverify(PM_PGP_VERIFY_NEVER); + } else { + pm_printf(PM_LOG_ERROR, _("invalid value for 'VerifySig' : '%s'\n"), value); + return 1; + } + pm_printf(PM_LOG_DEBUG, "config: setting default VerifySig: %s\n", value); } else { pm_printf(PM_LOG_WARNING, -- 1.7.4.2

Rémy Oudompheng

7:34 p.m.

New subject: [pacman-dev] [PATCH 4/4] Allow VerifySig to act as a default verification in [options]

On Mon 28 March 2011 at 15:15 -0400, Dave Reisner wrote:

...

+/* GPG signature verification option */ +typedef enum _pgp_verify_t { + PM_PGP_VERIFY_UNKNOWN, + PM_PGP_VERIFY_ALWAYS, + PM_PGP_VERIFY_OPTIONAL, + PM_PGP_VERIFY_NEVER +} pgp_verify_t; + +int alpm_db_set_pgp_verify(pmdb_t *db, pgp_verify_t verify);

Why is this enum name not prefixed by "pm" ? Why isn't alpm_db_set_pgp_verify with the other database functions (e.g. alpm_db_setserver()) ?

...

--- a/lib/libalpm/signing.c +++ b/lib/libalpm/signing.c @@ -249,6 +249,24 @@ int _alpm_load_signature(const char *sigfile, pmpgpsig_t *pgpsig) { }

/** + * Determines the necessity of checking for a valid pgp signature + * @param db the sync db to query + * + * @return signature verification level + */ +pgp_verify_t _alpm_get_sigverify_level(pmdb_t *db) +{ + ALPM_LOG_FUNC; + ASSERT(db != NULL, return PM_PGP_VERIFY_UNKNOWN); + + if(db->pgp_verify != PM_PGP_VERIFY_UNKNOWN) { + return db->pgp_verify; + } else { + return alpm_option_get_default_sigverify(); + } +} + +/** * Check the PGP package signature for the given package file. * @param pkg the package to check * @return a int value : 0 (valid), 1 (invalid), -1 (an error occured) @@ -270,11 +288,10 @@ int SYMEXPORT alpm_pkg_check_pgp_signature(pmpkg_t *pkg) int SYMEXPORT alpm_db_check_pgp_signature(pmdb_t *db) { ALPM_LOG_FUNC; - ASSERT(db != NULL, return(0)); + ASSERT(db != NULL, return 0);

return _alpm_gpgme_checksig(_alpm_db_path(db), _alpm_db_pgpsig(db)); }

- /* vim: set ts=2 sw=2 noet: */

I suggest using ASSERT(db != NULL, RET_ERR(PM_ERR_DB_NULL, ...)); to inform users of the error. And you should return -1 if db is NULL (in alpm_db_check_signature). Regards, -- Rémy.

Dave Reisner

7:53 p.m.

New subject: [pacman-dev] [PATCH 4/4] Allow VerifySig to act as a default verification in [options]

On Mon, Mar 28, 2011 at 09:34:57PM +0200, Rémy Oudompheng wrote:

...

On Mon 28 March 2011 at 15:15 -0400, Dave Reisner wrote:

...
+/* GPG signature verification option */ +typedef enum _pgp_verify_t { + PM_PGP_VERIFY_UNKNOWN, + PM_PGP_VERIFY_ALWAYS, + PM_PGP_VERIFY_OPTIONAL, + PM_PGP_VERIFY_NEVER +} pgp_verify_t; + +int alpm_db_set_pgp_verify(pmdb_t *db, pgp_verify_t verify);

Why is this enum name not prefixed by "pm" ?

Why isn't alpm_db_set_pgp_verify with the other database functions (e.g. alpm_db_setserver()) ?

Dan's naming, not mine. Adding the pm prefix makes sense here. I added it with the signature related functions since it's related to signatures and not strictly databases (think of pacman -U).

...

...
--- a/lib/libalpm/signing.c +++ b/lib/libalpm/signing.c @@ -249,6 +249,24 @@ int _alpm_load_signature(const char *sigfile, pmpgpsig_t *pgpsig) { }

/** + * Determines the necessity of checking for a valid pgp signature + * @param db the sync db to query + * + * @return signature verification level + */ +pgp_verify_t _alpm_get_sigverify_level(pmdb_t *db) +{ + ALPM_LOG_FUNC; + ASSERT(db != NULL, return PM_PGP_VERIFY_UNKNOWN); + + if(db->pgp_verify != PM_PGP_VERIFY_UNKNOWN) { + return db->pgp_verify; + } else { + return alpm_option_get_default_sigverify(); + } +} + +/** * Check the PGP package signature for the given package file. * @param pkg the package to check * @return a int value : 0 (valid), 1 (invalid), -1 (an error occured) @@ -270,11 +288,10 @@ int SYMEXPORT alpm_pkg_check_pgp_signature(pmpkg_t *pkg) int SYMEXPORT alpm_db_check_pgp_signature(pmdb_t *db) { ALPM_LOG_FUNC; - ASSERT(db != NULL, return(0)); + ASSERT(db != NULL, return 0);

return _alpm_gpgme_checksig(_alpm_db_path(db), _alpm_db_pgpsig(db)); }

- /* vim: set ts=2 sw=2 noet: */

I suggest using ASSERT(db != NULL, RET_ERR(PM_ERR_DB_NULL, ...)); to inform users of the error. And you should return -1 if db is NULL (in alpm_db_check_signature).

Regards, -- Rémy.

Dan McGee

8:52 p.m.

New subject: [pacman-dev] [PATCH 4/4] Allow VerifySig to act as a default verification in [options]

On Mon, Mar 28, 2011 at 2:53 PM, Dave Reisner wrote:

...

On Mon, Mar 28, 2011 at 09:34:57PM +0200, Rémy Oudompheng wrote:

...
On Mon 28 March 2011 at 15:15 -0400, Dave Reisner wrote:

...
+/* GPG signature verification option */ +typedef enum _pgp_verify_t { + PM_PGP_VERIFY_UNKNOWN, + PM_PGP_VERIFY_ALWAYS, + PM_PGP_VERIFY_OPTIONAL, + PM_PGP_VERIFY_NEVER +} pgp_verify_t; + +int alpm_db_set_pgp_verify(pmdb_t *db, pgp_verify_t verify);

Why is this enum name not prefixed by "pm" ?

Why isn't alpm_db_set_pgp_verify with the other database functions (e.g. alpm_db_setserver()) ?

Dan's naming, not mine. Adding the pm prefix makes sense here. I added it with the signature related functions since it's related to signatures and not strictly databases (think of pacman -U).

I've started to call some of this out. As long as we don't release in this awesome condition, I'm fine with bringing some of this work in so we can at least keep the ball rollling, but feel free to add any other concerns here: https://wiki.archlinux.org/index.php/User:Allan/Package_Signing#Other_Issues -Dan

Dan McGee

21 Apr 21 Apr

12:41 a.m.

New subject: [pacman-dev] [PATCH 4/4] Allow VerifySig to act as a default verification in [options]

On Mon, Mar 28, 2011 at 2:15 PM, Dave Reisner wrote:

...

diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 381d2bc..012043d 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -1038,22 +1038,34 @@ static int _parse_options(const char *key, char *value, config->rootdir = strdup(value); pm_printf(PM_LOG_DEBUG, "config: rootdir: %s\n", value); } - } else if (strcmp(key, "GPGDir") == 0) { + } else if(strcmp(key, "GPGDir") == 0) { if(!config->gpgdir) { config->gpgdir = strdup(value); pm_printf(PM_LOG_DEBUG, "config: gpgdir: %s\n", value); } - } else if (strcmp(key, "LogFile") == 0) { + } else if(strcmp(key, "LogFile") == 0) { if(!config->logfile) { config->logfile = strdup(value); pm_printf(PM_LOG_DEBUG, "config: logfile: %s\n", value); } - } else if (strcmp(key, "XferCommand") == 0) { + } else if(strcmp(key, "XferCommand") == 0) { config->xfercommand = strdup(value); alpm_option_set_fetchcb(download_with_xfercommand); pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", value); - } else if (strcmp(key, "CleanMethod") == 0) { + } else if(strcmp(key, "CleanMethod") == 0) { setrepeatingoption(value, "CleanMethod", option_add_cleanmethod); + } else if(strcmp(key, "VerifySig") == 0) { + if (strcmp(value, "Always") == 0) { + alpm_option_set_default_sigverify(PM_PGP_VERIFY_ALWAYS); + } else if(strcmp(value, "Optional") == 0) { + alpm_option_set_default_sigverify(PM_PGP_VERIFY_OPTIONAL); + } else if(strcmp(value, "Never") == 0) { + alpm_option_set_default_sigverify(PM_PGP_VERIFY_NEVER); + } else { + pm_printf(PM_LOG_ERROR, _("invalid value for 'VerifySig' : '%s'\n"), value); + return 1; + } + pm_printf(PM_LOG_DEBUG, "config: setting default VerifySig: %s\n", value); } else {

pm_printf(PM_LOG_WARNING,

Pro tip- please don't mix syntax/reformatting patches with actual work. I've cleaned this one up anyway since it doesn't apply cleanly without your other patch, but keep that in mind in the future... -Dan

4753

Age (days ago)

4777

Last active (days ago)

List overview

Download

12 comments

3 participants

participants (3)

Dan McGee
Dave Reisner
Rémy Oudompheng

[pacman-dev] [PATCH 0/4] addition of API methods and backend work

tags

participants (3)