strongswan / swidGenerator

Application which generates SWID-Tags from Linux package managers like dpkg, rpm or pacman.
MIT License
16 stars 11 forks source link

Distinguish package architectures, in tagId and version #45

Closed adelton closed 6 years ago

adelton commented 6 years ago

This is necessary for multiarch packages, and also for uninstalled package files where the architecture of the OS doesn't imply architecture of the package.

This pull request is on top of https://github.com/strongswan/swidGenerator/pull/44.

tobiasbrunner commented 6 years ago

This pull request is on top of #44.

I'm not commenting on the contents of this pull request, but what's the point of this? Why not one pull request with two commits? (Which this one actually is, rendering #44 completely pointless.)

adelton commented 6 years ago

Well, my plan was to be ready for situation when we will eventually go with different approach for https://github.com/strongswan/swidGenerator/pull/44 or when that change is not accepted. So this one is here mostly as staged commit. If you prefer, I can certainly do less pull requests and include multiple commits in them.

tobiasbrunner commented 6 years ago

Well, my plan was to be ready for situation when we will eventually go with different approach for #44 or when that change is not accepted. So this one is here mostly as staged commit.

You could just update the branch if that were the case (e.g. remove/modify a commit if necessary and force push the branch). Just make sure to use a separate branch for each PR.

If you prefer, I can certainly do less pull requests and include multiple commits in them.

For related, and in particular dependent, commits I'd definitely prefer a single PR.

Anyway, today Andreas mentioned that he agrees with the first commit (to unify the output) but not with the second, as the architecture is not included in the version string in dpkg environments either (due to some problems when querying the install history I think - however, this results in the same limitation regarding multi-arch packages). Maybe he could provide more details on the latter.

Not sure if he'd object if the architecture were made optional (i.e. via a command line argument), or if it was a separate attribute in PackageInfo, which might then override e.g. ctx['architecture'] when generating unique IDs (don't know exactly if that even makes sense, though - would probably not work if the x86 version of a package on x86_64 is different from the native x86 version of that package).

adelton commented 6 years ago

I admit I have little expertise in dpkg, so my pull requests will focus on the rpm side of things.

To me, the ctx['architecture'] is completely irrelevant. If I run swid_generator on x86_64 machine with --package-file pointing at aarch64 file, I don't expect that x86_64 to show up anywhere in the output. Things are a little bit different for installed packages but even there, multilibs trump the "environment" architecture. So I plan to run all my invocations with --arch='' and I will send additional pull requests to prune the dashes when the string is empty. If you prefer, I can certainly provide patch which would add some command-line option to enable the use of package's architecture. Or maybe even better, equivalent of rpm's --qf to exactly specify what the tagId should look like.

At least in rpm world and the way build systems are setup for RHEL, CentOS, and Fedora, the i386/i686 packages would be built once, there is no distinction of i686 package "for" x86_64 setups.

adelton commented 6 years ago

If the consistency with rpm is the concern, I've now rebased the patch and changed the second commit to also add .${Architecture} in dpkg environments.

I initially wanted to go with :${Architecture} but create_unique_id changes that colon to tilda.

adelton commented 6 years ago

I've now fixed tests -> 8a0d85237948c6b853722e1586a8b9f22921c92d.

tobiasbrunner commented 6 years ago

According to Andreas, the reason the architecture is not included for Debian packages is due to some problems with the contents of the apt history log file (which might or might not contain the architecture of packages). strongSwan's IMC uses this file to collect SWIMA install/uninstall events. So to be compatible with existing strongTNC it would be preferable if the addition of the architecture to the package's version string would only done if enabled via a command line option.

And since this adds a similar redundancy as #44 replacing/omitting the global architecture in tagId/Meta tags might also be something to consider. However, I think strongTNC uses the Meta tag to associate packages with platforms they are installed on, so this could be tricky with multiarch/noarch/all-arch packages.

adelton commented 6 years ago

Would you consider a patch which would make the package architecture information as part of tagId and version optional only on dpkg* platforms but on rpm-based system, the command line option would not be needed?

Or better yet, changing the default to include it but have a command line option to turn it off for dpkg*?

tobiasbrunner commented 6 years ago

Would you consider a patch which would make the package architecture information as part of tagId and version optional only on dpkg* platforms but on rpm-based system, the command line option would not be needed?

Yeah, I guess, even though it's a bit inconsistent. But I suppose the option could be named dpkg-specific somehow to clarify that it is just ignored when used on rpm platforms. Adding the architecture generally makes sense on both platforms, otherwise you'd run into problems if you have cross-architecture packages installed. That it isn't added is only due to some practical limitations that occur in combination with strongTNC and strongSwan's SWIMA IMC on dpkg platforms (originally Andreas did add it, but because the history log didn't always include the architecture maintaining a consistent database was apparently tricky, so he just removed it everywhere, which worked better - but I guess only because he doesn't have any cross-architecture packages installed on his systems :-).

Or better yet, changing the default to include it but have a command line option to turn it off for dpkg*?

I suggested that to Andreas too. But he doesn't want current users (mostly himself) to have to change anything in all their installations (even though it would be the better API as adding the architecture generally makes sense).

adelton commented 6 years ago

Rebased on master, added commit which makes the architecture information optional on dpkg platforms and can be enabled with --dpkg-include-package-arch.

I wonder if having some default file like /etc/swd_generator.conf would be a good addition to avoid growing too many command line options.

tobiasbrunner commented 6 years ago

Merged to master. Thanks!

tobiasbrunner commented 6 years ago

Hm, actually this option should probably also be in the parent parser (similar to --id-prefix), so the two commands produce consistent output. I did so in the fixes branch.