[pacman-dev] Disable embedded signatures by default

Message ID 20200810213415.215601-1-anatol.pomozov@gmail.com
State New
Headers show
Series
  • [pacman-dev] Disable embedded signatures by default
Related show

Commit Message

Anatol Pomozov Aug. 10, 2020, 9:34 p.m. UTC
Switching from embedded to detached signatures is a big change. This
feature needs to be thoroughly tested before embedded signatures will be
completely removed from the database.

To help with detached signatures testing we enable it by default. But in
case if an user needs to go back to embedded signatures we add a config
option to reenable it - "UseEmbeddedSignatures".

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 doc/pacman.conf.5.asciidoc |  4 ++++
 lib/libalpm/alpm.h         |  3 +++
 lib/libalpm/be_sync.c      |  4 +++-
 lib/libalpm/handle.c       | 13 +++++++++++++
 lib/libalpm/handle.h       |  1 +
 src/pacman/conf.c          |  4 ++++
 src/pacman/conf.h          |  2 ++
 src/pacman/pacman-conf.c   |  3 +++
 8 files changed, 33 insertions(+), 1 deletion(-)

Comments

Eli Schwartz Aug. 10, 2020, 9:44 p.m. UTC | #1
On 8/10/20 5:34 PM, Anatol Pomozov wrote:
> Switching from embedded to detached signatures is a big change. This
> feature needs to be thoroughly tested before embedded signatures will be
> completely removed from the database.
> 
> To help with detached signatures testing we enable it by default. But in
> case if an user needs to go back to embedded signatures we add a config
> option to reenable it - "UseEmbeddedSignatures".
What is the purpose of this? Either signature source should be
equivalent, and you should be able to trivially test this by creating a
repo with unsigned packages, then bulk-signing the packages after they
were repo-added. I don't believe that pacman should include such an
end-user option purely to double-check whether a current feature
actually works.
Allan McRae Aug. 11, 2020, 1:24 p.m. UTC | #2
On 11/8/20 7:44 am, Eli Schwartz wrote:
> On 8/10/20 5:34 PM, Anatol Pomozov wrote:
>> Switching from embedded to detached signatures is a big change. This
>> feature needs to be thoroughly tested before embedded signatures will be
>> completely removed from the database.
>>
>> To help with detached signatures testing we enable it by default. But in
>> case if an user needs to go back to embedded signatures we add a config
>> option to reenable it - "UseEmbeddedSignatures".
> What is the purpose of this? Either signature source should be
> equivalent, and you should be able to trivially test this by creating a
> repo with unsigned packages, then bulk-signing the packages after they
> were repo-added. I don't believe that pacman should include such an
> end-user option purely to double-check whether a current feature
> actually works.

Agreed - the user should not care where the signatures come from, so
this option should not exist.

Also, I see this was proposed on arch-dev-public first.  I am not
subscribed there, and decisions on what is included in pacman are not
dictated by Arch Linux.  Proposals should be posted here.


Now, thinking out loud here...  Would an alternative be to add an
"--embed-signatures" option to repo-add?  So two versions of a repo
could be created and those that want to test without embedded signatures
can.

Allan
Eli Schwartz Aug. 11, 2020, 1:56 p.m. UTC | #3
On 8/11/20 9:24 AM, Allan McRae wrote:
> On 11/8/20 7:44 am, Eli Schwartz wrote:
>> On 8/10/20 5:34 PM, Anatol Pomozov wrote:
>>> Switching from embedded to detached signatures is a big change. This
>>> feature needs to be thoroughly tested before embedded signatures will be
>>> completely removed from the database.
>>>
>>> To help with detached signatures testing we enable it by default. But in
>>> case if an user needs to go back to embedded signatures we add a config
>>> option to reenable it - "UseEmbeddedSignatures".
>> What is the purpose of this? Either signature source should be
>> equivalent, and you should be able to trivially test this by creating a
>> repo with unsigned packages, then bulk-signing the packages after they
>> were repo-added. I don't believe that pacman should include such an
>> end-user option purely to double-check whether a current feature
>> actually works.
> 
> Agreed - the user should not care where the signatures come from, so
> this option should not exist.
> 
> Also, I see this was proposed on arch-dev-public first.  I am not
> subscribed there, and decisions on what is included in pacman are not
> dictated by Arch Linux.  Proposals should be posted here.

More specifically -- decisions on what is included in pacman are not
dictated by consensus of the Arch Linux team, but by the pacman team
(which is in turn guided, but not dictated, by what is useful for
archlinux).

Making a bad or confusing package manager simply because archlinux wants
it, would be a bad move due to making a bad or confusing package manager.

> Now, thinking out loud here...  Would an alternative be to add an
> "--embed-signatures" option to repo-add?  So two versions of a repo
> could be created and those that want to test without embedded signatures
> can.

This is the right approach, yeah. I was thinking we'd wait until pacman
6.1 before stopping the signature embedding, to provide a transition
period for people depending on SigLevel = Required (which should be
everyone, and certainly includes Arch!) to upgrade to 6.x before
repo-add starts generating databases useless to pacman 5.x

But I'd also be fine with --no-embed-signatures for opting in early, and
switching to --embed-signatures for opting out once we default to --no-*
Anatol Pomozov Aug. 27, 2020, 12:26 a.m. UTC | #4
Hi

On Mon, Aug 10, 2020 at 2:45 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>
> On 8/10/20 5:34 PM, Anatol Pomozov wrote:
> > Switching from embedded to detached signatures is a big change. This
> > feature needs to be thoroughly tested before embedded signatures will be
> > completely removed from the database.
> >
> > To help with detached signatures testing we enable it by default. But in
> > case if an user needs to go back to embedded signatures we add a config
> > option to reenable it - "UseEmbeddedSignatures".
> What is the purpose of this? Either signature source should be
> equivalent,

Indeed the signatures are equivalent. The only difference whether they
are stored inside the database file or as *.sig next to the packages
itself.

> and you should be able to trivially test this by creating a
> repo with unsigned packages, then bulk-signing the packages after they
> were repo-added. I don't believe that pacman should include such an
> end-user option purely to double-check whether a current feature
> actually works.

The purpose of the change is to start using the detached signatures
codepath. The detached signatures are shipped with repos for a long
time and pacman can handle it. Now it is time to actually enable it by
default.

 "UseEmbeddedSignatures" option has been added as a fallback plan in
case we find that the detached signatures codepath is broken. Do you
think this is too much hassle and we should just start using detached
signatures by default without any fallback config option?

> This is the right approach, yeah. I was thinking we'd wait until pacman
> 6.1 before stopping the signature embedding, to provide a transition
> period for people depending on SigLevel = Required (which should be
> everyone, and certainly includes Arch!) to upgrade to 6.x before
> repo-add starts generating databases useless to pacman 5.x

There are 2 sets of changes that need to be done:
1) make pacman to use detached signatures instead of embedded ones
2) change "repo-add" to avoid adding embedded signatures

We should release changes for #1 first, test it, make sure that
detached signatures fully work (while dbs still have pacman
5.x-compatible embedded sigs). And only then release #2 to get smaller
databases compatible with pacman version >= 6.0.

I was thinking #1 can be released with 6.0 and #2 with 6.1.
Allan McRae Aug. 28, 2020, 4:37 a.m. UTC | #5
On 27/8/20 10:26 am, Anatol Pomozov wrote:
> Hi
> 
> On Mon, Aug 10, 2020 at 2:45 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>>
>> On 8/10/20 5:34 PM, Anatol Pomozov wrote:
>>> Switching from embedded to detached signatures is a big change. This
>>> feature needs to be thoroughly tested before embedded signatures will be
>>> completely removed from the database.
>>>
>>> To help with detached signatures testing we enable it by default. But in
>>> case if an user needs to go back to embedded signatures we add a config
>>> option to reenable it - "UseEmbeddedSignatures".
>> What is the purpose of this? Either signature source should be
>> equivalent,
> 
> Indeed the signatures are equivalent. The only difference whether they
> are stored inside the database file or as *.sig next to the packages
> itself.
> 
>> and you should be able to trivially test this by creating a
>> repo with unsigned packages, then bulk-signing the packages after they
>> were repo-added. I don't believe that pacman should include such an
>> end-user option purely to double-check whether a current feature
>> actually works.
> 
> The purpose of the change is to start using the detached signatures
> codepath. The detached signatures are shipped with repos for a long
> time and pacman can handle it. Now it is time to actually enable it by
> default.
> 
>  "UseEmbeddedSignatures" option has been added as a fallback plan in
> case we find that the detached signatures codepath is broken. Do you
> think this is too much hassle and we should just start using detached
> signatures by default without any fallback config option?
> 

I think we should test using detached signatures, and release without
pacman having a fallback option in and of itself.  The fallback option
should be in repo-add, whether it adds the signatures to the database or
not.

>> This is the right approach, yeah. I was thinking we'd wait until pacman
>> 6.1 before stopping the signature embedding, to provide a transition
>> period for people depending on SigLevel = Required (which should be
>> everyone, and certainly includes Arch!) to upgrade to 6.x before
>> repo-add starts generating databases useless to pacman 5.x
> 
> There are 2 sets of changes that need to be done:
> 1) make pacman to use detached signatures instead of embedded ones
> 2) change "repo-add" to avoid adding embedded signatures
> 
> We should release changes for #1 first, test it, make sure that
> detached signatures fully work (while dbs still have pacman
> 5.x-compatible embedded sigs). And only then release #2 to get smaller
> databases compatible with pacman version >= 6.0.
> 
> I was thinking #1 can be released with 6.0 and #2 with 6.1.

I was thinking #2 would be an option to repo-add.  I'm looking at making
signature embedding only occur with the "--add-signatures" option (or
whatever I decide to call it).  Arch would need to patch devtools to use
this option.  They would then make a News announcement about the need to
have pacman-6.0 installed after 3-6 months and stop repo-add including
signatures.

However, I think pacman should always use the signatures in the database
if they are present.  Particularly if they are not embedded by default.

So to actually test the detached signature path, I am thinking it best
to tag 6.0.0beta1, make a package from that tag with a patch to enable
using detached signatures as a priority.  While that is not an ideal
approach to testing, I think the current code path is well tested, and
this should be a reasonably trivial patch.

Allan
Eli Schwartz Aug. 30, 2020, 5:41 p.m. UTC | #6
On 8/26/20 8:26 PM, Anatol Pomozov wrote:
> The purpose of the change is to start using the detached signatures
> codepath. The detached signatures are shipped with repos for a long
> time and pacman can handle it. Now it is time to actually enable it by
> default.
> 
>  "UseEmbeddedSignatures" option has been added as a fallback plan in
> case we find that the detached signatures codepath is broken. Do you
> think this is too much hassle and we should just start using detached
> signatures by default without any fallback config option?

I already stated my opinion.

Firstly, that we should NOT add a configuration option for this, since
it is a burden on the manpage and is completely useless except for
development testing.

Secondly, I believe we should continue to check both, because I see no
compelling reason to reject perfectly working functionality for no
reason, plus you have no way of knowing if there are thirdparty
repositories which locally generate databases with sigs, but then only
upload the packages and databases, but not the sigs (on the grounds that
they won't be used so why bother).

We don't just care about Arch Linux. Also, we don't just care about the
official repos, even for Arch Linux use. Before instituting a breaking
change, we need a better reason than "this is a convenient way for a
pacman developer to test whether or not pacman is broken".

>> This is the right approach, yeah. I was thinking we'd wait until pacman
>> 6.1 before stopping the signature embedding, to provide a transition
>> period for people depending on SigLevel = Required (which should be
>> everyone, and certainly includes Arch!) to upgrade to 6.x before
>> repo-add starts generating databases useless to pacman 5.x
> 
> There are 2 sets of changes that need to be done:
> 1) make pacman to use detached signatures instead of embedded ones
> 2) change "repo-add" to avoid adding embedded signatures
> 
> We should release changes for #1 first, test it, make sure that
> detached signatures fully work (while dbs still have pacman
> 5.x-compatible embedded sigs). And only then release #2 to get smaller
> databases compatible with pacman version >= 6.0.
> 
> I was thinking #1 can be released with 6.0 and #2 with 6.1.

My vote is to not do #1 at all. I do not see why you keep insisting it
"needs" to be done. #2 is all we need to generate test repositories.

Here is a script to generate test repositories:

ssh pkgbuild.com
cd public_html/repo
mkdir x86_64-detachedsigs
cd mkdir x86_64-detachedsigs

bsdtar -xOf ../x86_64/eschwartz.db.tar.gz | awk
'/^%FILENAME%/{getline;print}' | while read -r line; do
    cp -v ../x86_64/"$line" ./
done

repo-add eschwartz.db.tar.gz *.pkg.tar*

bsdtar -xOf ../x86_64/eschwartz.db.tar.gz | awk
'/^%FILENAME%/{getline;print}' | while read -r line; do
    cp -v ../x86_64/"$line".sig ./
done



Here is a test repo you can verify against:

[eschwartz]
Server = https://pkgbuild.com/~eschwartz/repo/x86_64-detachedsigs/
Andrew Gregory Aug. 30, 2020, 8:21 p.m. UTC | #7
On 08/28/20 at 02:37pm, Allan McRae wrote:
> On 27/8/20 10:26 am, Anatol Pomozov wrote:
> > Hi
> > 
> > On Mon, Aug 10, 2020 at 2:45 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
> >> This is the right approach, yeah. I was thinking we'd wait until pacman
> >> 6.1 before stopping the signature embedding, to provide a transition
> >> period for people depending on SigLevel = Required (which should be
> >> everyone, and certainly includes Arch!) to upgrade to 6.x before
> >> repo-add starts generating databases useless to pacman 5.x
> > 
> > There are 2 sets of changes that need to be done:
> > 1) make pacman to use detached signatures instead of embedded ones
> > 2) change "repo-add" to avoid adding embedded signatures
> > 
> > We should release changes for #1 first, test it, make sure that
> > detached signatures fully work (while dbs still have pacman
> > 5.x-compatible embedded sigs). And only then release #2 to get smaller
> > databases compatible with pacman version >= 6.0.
> > 
> > I was thinking #1 can be released with 6.0 and #2 with 6.1.
> 
> I was thinking #2 would be an option to repo-add.  I'm looking at making
> signature embedding only occur with the "--add-signatures" option (or
> whatever I decide to call it).  Arch would need to patch devtools to use
> this option.  They would then make a News announcement about the need to
> have pacman-6.0 installed after 3-6 months and stop repo-add including
> signatures.
> 
> However, I think pacman should always use the signatures in the database
> if they are present.  Particularly if they are not embedded by default.
> 
> So to actually test the detached signature path, I am thinking it best
> to tag 6.0.0beta1, make a package from that tag with a patch to enable
> using detached signatures as a priority.  While that is not an ideal
> approach to testing, I think the current code path is well tested, and
> this should be a reasonably trivial patch.

We should implement FS#33091.  Instead of adding an option to disable
detached signatures, add one to disable embedded signatures.  This
gives anybody that wants to help test a way to do so without forcing
it on people and provides a useful feature for any repos that continue
providing embedded signatures.  I don't even know that we'd need
a beta release because the new behavior would be opt-in and could be
disabled at any time.

Patch

diff --git a/doc/pacman.conf.5.asciidoc b/doc/pacman.conf.5.asciidoc
index 3c2f1fea..dd1d3d5e 100644
--- a/doc/pacman.conf.5.asciidoc
+++ b/doc/pacman.conf.5.asciidoc
@@ -214,6 +214,10 @@  Options
 	positive integer. If this config option is not set then only one download
 	stream is used (i.e. downloads happen sequentially).
 
+*UseEmbeddedSignatures*::
+  Enable database embedded signatures usage. If this flag is not set then
+  only detached signatures (*.sig files) are used.
+
 
 Repository Sections
 -------------------
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 614a530c..f88428f2 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -920,6 +920,9 @@  int alpm_option_set_arch(alpm_handle_t *handle, const char *arch);
 int alpm_option_get_checkspace(alpm_handle_t *handle);
 int alpm_option_set_checkspace(alpm_handle_t *handle, int checkspace);
 
+int alpm_option_get_embeddedsigs(alpm_handle_t *handle);
+int alpm_option_set_embeddedsigs(alpm_handle_t *handle, int embeddedsigs);
+
 const char *alpm_option_get_dbext(alpm_handle_t *handle);
 int alpm_option_set_dbext(alpm_handle_t *handle, const char *dbext);
 
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
index 225df76d..d31f4862 100644
--- a/lib/libalpm/be_sync.c
+++ b/lib/libalpm/be_sync.c
@@ -602,7 +602,9 @@  static int sync_db_read(alpm_db_t *db, struct archive *archive,
 			} else if(strcmp(line, "%SHA256SUM%") == 0) {
 				READ_AND_STORE(pkg->sha256sum);
 			} else if(strcmp(line, "%PGPSIG%") == 0) {
-				READ_AND_STORE(pkg->base64_sig);
+				if(db->handle->embeddedsigs) {
+					READ_AND_STORE(pkg->base64_sig);
+				}
 			} else if(strcmp(line, "%REPLACES%") == 0) {
 				READ_AND_SPLITDEP(pkg->replaces);
 			} else if(strcmp(line, "%DEPENDS%") == 0) {
diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
index 1310601a..d0077cd4 100644
--- a/lib/libalpm/handle.c
+++ b/lib/libalpm/handle.c
@@ -294,6 +294,12 @@  int SYMEXPORT alpm_option_get_checkspace(alpm_handle_t *handle)
 	return handle->checkspace;
 }
 
+int SYMEXPORT alpm_option_get_embeddedsigs(alpm_handle_t *handle)
+{
+	CHECK_HANDLE(handle, return -1);
+	return handle->embeddedsigs;
+}
+
 const char SYMEXPORT *alpm_option_get_dbext(alpm_handle_t *handle)
 {
 	CHECK_HANDLE(handle, return NULL);
@@ -754,6 +760,13 @@  int SYMEXPORT alpm_option_set_checkspace(alpm_handle_t *handle, int checkspace)
 	return 0;
 }
 
+int SYMEXPORT alpm_option_set_embeddedsigs(alpm_handle_t *handle, int embeddedsigs)
+{
+	CHECK_HANDLE(handle, return -1);
+	handle->embeddedsigs = embeddedsigs;
+	return 0;
+}
+
 int SYMEXPORT alpm_option_set_dbext(alpm_handle_t *handle, const char *dbext)
 {
 	CHECK_HANDLE(handle, return -1);
diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h
index 9fef0fbf..de278d88 100644
--- a/lib/libalpm/handle.h
+++ b/lib/libalpm/handle.h
@@ -98,6 +98,7 @@  struct __alpm_handle_t {
 	char *arch;              /* Architecture of packages we should allow */
 	int usesyslog;           /* Use syslog instead of logfile? */ /* TODO move to frontend */
 	int checkspace;          /* Check disk space before installing */
+	int embeddedsigs;        /* Use embedded signatures instead of detached one */
 	char *dbext;             /* Sync DB extension */
 	int siglevel;            /* Default signature verification level */
 	int localfilesiglevel;   /* Signature verification level for local file
diff --git a/src/pacman/conf.c b/src/pacman/conf.c
index 3a3ef605..6268f229 100644
--- a/src/pacman/conf.c
+++ b/src/pacman/conf.c
@@ -598,6 +598,9 @@  static int _parse_options(const char *key, char *value,
 		} else if(strcmp(key, "ILoveCandy") == 0) {
 			config->chomp = 1;
 			pm_printf(ALPM_LOG_DEBUG, "config: chomp\n");
+		} else if(strcmp(key, "UseEmbeddedSignatures") == 0) {
+			config->embeddedsigs = 1;
+			pm_printf(ALPM_LOG_DEBUG, "config: embeddedsigs\n");
 		} else if(strcmp(key, "VerbosePkgLists") == 0) {
 			config->verbosepkglists = 1;
 			pm_printf(ALPM_LOG_DEBUG, "config: verbosepkglists\n");
@@ -899,6 +902,7 @@  static int setup_libalpm(void)
 	alpm_option_set_arch(handle, config->arch);
 	alpm_option_set_checkspace(handle, config->checkspace);
 	alpm_option_set_usesyslog(handle, config->usesyslog);
+	alpm_option_set_embeddedsigs(handle, config->embeddedsigs);
 
 	alpm_option_set_ignorepkgs(handle, config->ignorepkg);
 	alpm_option_set_ignoregroups(handle, config->ignoregrp);
diff --git a/src/pacman/conf.h b/src/pacman/conf.h
index b8a451ad..e1dbd1bc 100644
--- a/src/pacman/conf.h
+++ b/src/pacman/conf.h
@@ -111,6 +111,8 @@  typedef struct __config_t {
 	/* conf file options */
 	/* I Love Candy! */
 	unsigned short chomp;
+	/* Use database embedded signatures if present */
+	unsigned short embeddedsigs;
 	/* format target pkg lists as table */
 	unsigned short verbosepkglists;
 	/* When downloading, display the amount downloaded, rate, ETA, and percent
diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c
index 463badf1..b96fb6b8 100644
--- a/src/pacman/pacman-conf.c
+++ b/src/pacman/pacman-conf.c
@@ -267,6 +267,7 @@  static void dump_config(void)
 	show_bool("VerbosePkgLists", config->verbosepkglists);
 	show_bool("DisableDownloadTimeout", config->disable_dl_timeout);
 	show_bool("ILoveCandy", config->chomp);
+	show_bool("UseEmbeddedSignatures", config->embeddedsigs);
 	show_bool("NoProgressBar", config->noprogressbar);
 
 	show_int("ParallelDownloads", config->parallel_downloads);
@@ -381,6 +382,8 @@  static int list_directives(void)
 			show_bool("DisableDownloadTimeout", config->disable_dl_timeout);
 		} else if(strcasecmp(i->data, "ILoveCandy") == 0) {
 			show_bool("ILoveCandy", config->chomp);
+		} else if(strcasecmp(i->data, "UseEmbeddedSignatures") == 0) {
+			show_bool("UseEmbeddedSignatures", config->embeddedsigs);
 		} else if(strcasecmp(i->data, "NoProgressBar") == 0) {
 			show_bool("NoProgressBar", config->noprogressbar);