Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip/3mdeb/qubes-wrapper #2710

Merged
merged 2 commits into from
Feb 26, 2021
Merged

wip/3mdeb/qubes-wrapper #2710

merged 2 commits into from
Feb 26, 2021

Conversation

Asiderr
Copy link
Collaborator

@Asiderr Asiderr commented Dec 30, 2020

This patch is adding the fwupd wrapper for Qubes OS. The wrapper provides fwupd functionalities for Qubes R4.1.
It creates three packages (two RPMs and one deb):
fwupd-qubes-dom0 (RPM)
fwupd-qubes-vm (RPM)
fwupd-qubes-vm-whonix (deb)
More information about the wrapper could be found in the contrib/qubes/README.md

Signed-off-by: Norbert Kamiński [email protected]

Type of pull request:

@lgtm-com
Copy link

lgtm-com bot commented Dec 30, 2020

This pull request introduces 3 alerts when merging b91d725 into f3510f5 - view on LGTM.com

new alerts:

  • 3 for Duplication in regular expression character class

contrib/fwupd.spec.in Outdated Show resolved Hide resolved
contrib/fwupd.spec.in Outdated Show resolved Hide resolved
Comment on lines 553 to 562
qvm-run --pass-io sys-whonix 'rm /home/user/QubesIncoming/dom0/GPG-KEY-Linux-Vendor-Firmware-Service'
qvm-copy-to-vm sys-whonix %PKI/GPG-KEY-Linux-Vendor-Firmware-Service
qvm-run --pass-io sys-whonix 'gpg --import /home/user/QubesIncoming/dom0/GPG-KEY-Linux-Vendor-Firmware-Service'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the GPG keys? I think you want the PKCS ones too, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPG keys are used three times to verify the metadata and firmware update files. The first time, they are used when the files are downloaded, the second time after copying the files from UpdateVM to Dom0, and the third time when Dom0 copies files to sys-usb VM. For now, we are not using the PKCS keys, but if we want to use jcat-tool they are available in standard fwupd directory /etc/pki/fwupd

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted these lines. GPG verification has been replaced with jcat verification.

Comment on lines 101 to 102
wget -P $FWUPD_UPDATEVM_DIR/metadata $URL.jcat
wget -P $FWUPD_UPDATEVM_DIR/metadata $URL.asc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need both files to cover fwupd versions from 0.9.5 to the current one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why support such old fwupd versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ferdora 25 is base for dom0 in Qubes R4.0 (https://www.qubes-os.org/doc/supported-versions/) and it provides fwupd 0.9.5.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file doesn't exist. It's replaced with the python script. There is no need to download the GPG signature, thus I left only jcat file. The wrapper works correctly with the current libjcat master and the changes connected to the hughsie/libjcat#48 and hughsie/libjcat#49 need to be available on Fedora 32.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libjcat update just went stable for f32: https://bodhi.fedoraproject.org/updates/FEDORA-2021-a83da4c566

contrib/debian/control.in Outdated Show resolved Hide resolved
@@ -198,3 +198,9 @@ Depends: ${shlibs:Depends}, ${misc:Depends}, make | build-essential | dpkg-dev
Description: Template for signed fwupd package
This package is used to control code signing by the Debian signing
service.

Package: fwupd-qubes-vm-whonix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other packages in the Debian archive that have set precedent for this already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no such packages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning on we should have some sort of flag that determines whether this binary package gets built as part of https://github.com/fwupd/fwupd/blob/master/contrib/ci/generate_debian.py

I don't think it should be done in general purpose Debian or Ubuntu, right? It should only be when the Debian derivatives Whonix generates the binary packages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be done in general purpose Debian or Ubuntu, right?

That's right. It's Qubes specific package.

I'm leaning on we should have some sort of flag that determines whether this binary package gets built as part of https://github.com/fwupd/fwupd/blob/master/contrib/ci/generate_debian.py

It could be handled as another OS environment variable e.g. Debian-x86_64-Whonix and it would be added to the TARGET split: https://github.com/fwupd/fwupd/blob/master/contrib/ci/generate_debian.py#L24

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good enough approach to me. If you find while you're doing it that doesn't end up scaling well enough feel free to add argparse support there instead though. The environment parsing was just a convenient way for our first CI jobs to do it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a possibility to generate separate fedora-qubes and debian-x86_64-qubes containers that build qubes packages. Standard containers don't provide qubes DEBs and RPMs.

contrib/debian/control.in Outdated Show resolved Hide resolved
contrib/fwupd.spec.in Outdated Show resolved Hide resolved
contrib/qubes/README.md Outdated Show resolved Hide resolved
contrib/qubes/README.md Outdated Show resolved Hide resolved
contrib/qubes/README.md Outdated Show resolved Hide resolved
contrib/qubes/README.md Outdated Show resolved Hide resolved
@Asiderr Asiderr force-pushed the wip/3mdeb/qubes-wrapper branch from b91d725 to 17f44be Compare January 12, 2021 14:18
@lgtm-com
Copy link

lgtm-com bot commented Jan 12, 2021

This pull request introduces 4 alerts when merging 17f44be into 8fa65d9 - view on LGTM.com

new alerts:

  • 3 for Duplication in regular expression character class
  • 1 for Conflicting attributes in base classes

@Asiderr Asiderr force-pushed the wip/3mdeb/qubes-wrapper branch from 17f44be to 1ab7324 Compare January 12, 2021 16:10
@lgtm-com
Copy link

lgtm-com bot commented Jan 12, 2021

This pull request introduces 4 alerts when merging 1ab7324 into 8fa65d9 - view on LGTM.com

new alerts:

  • 3 for Duplication in regular expression character class
  • 1 for Conflicting attributes in base classes

@@ -181,6 +183,30 @@ This provides the optional package which is only required on hardware that
can be flashed using flashrom. It is probably not required on servers.
%endif

%package qubes-dom0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to the Debian case I think we want to special case the generation of the qubes binary package. This won't go into real Fedora archives.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a possibility to generate separate fedora-qubes and debian-x86_64-qubes containers that build qubes packages. Standard containers don't provide qubes DEBs and RPMs.

@hughsie
Copy link
Member

hughsie commented Feb 1, 2021

The ColorHug2 firmware updates must provide SHA256 for testing purposes

Which version do you need resigned? I don't want to do them all ideally to avoid churn.

@Asiderr Asiderr force-pushed the wip/3mdeb/qubes-wrapper branch from cd0e3a9 to d5ca239 Compare February 1, 2021 13:36
@lgtm-com
Copy link

lgtm-com bot commented Feb 1, 2021

This pull request introduces 3 alerts when merging d5ca239 into 4d8163c - view on LGTM.com

new alerts:

  • 3 for Duplication in regular expression character class

@Asiderr
Copy link
Collaborator Author

Asiderr commented Feb 1, 2021

Which version do you need resigned? I don't want to do them all ideally to avoid churn.

@hughsie Ideally one for update (2.0.7) and one for downgrade (2.0.6 or 2.0.5).

@hughsie
Copy link
Member

hughsie commented Feb 1, 2021

@Asiderr: All three done!

@hughsie
Copy link
Member

hughsie commented Feb 1, 2021

3 for Duplication in regular expression character class

There's also this warning from LGTM; but we're getting there now! Nearly there.

@Asiderr Asiderr force-pushed the wip/3mdeb/qubes-wrapper branch from d5ca239 to b336edb Compare February 8, 2021 15:19
@lgtm-com
Copy link

lgtm-com bot commented Feb 8, 2021

This pull request introduces 1 alert when merging b336edb into 0cde61d - view on LGTM.com

new alerts:

  • 1 for Unused import

@Asiderr Asiderr force-pushed the wip/3mdeb/qubes-wrapper branch from b336edb to 93006e7 Compare February 8, 2021 15:31
@Asiderr
Copy link
Collaborator Author

Asiderr commented Feb 8, 2021

@hughsie @superm1

There's also this warning from LGTM; but we're getting there now! Nearly there.

I fixed LGTM problems.

I've dropped GPG verification and now metadata and firmware updates are validated with jcat verification. I'm still waiting for metadata sync (fwupdagent still doesn't provide sha256 for ColorHug2 cabinets).

The wrapper has been successfully tested on Qubes R4.1. For test purposes, I've replaced jcat-tool in the following lines with the directories of the jcat-tool binary, built from the current master:

@hughsie
Copy link
Member

hughsie commented Feb 8, 2021

I'm still waiting for metadata sync

This should have happened yesterday, no? "fwupdmgr refresh --force"

@Asiderr
Copy link
Collaborator Author

Asiderr commented Feb 8, 2021

@hughsie I guess so, but fwupdagent still doesn't provide the second checksum:

$ fwupdmgr refresh --force
Fetching metadata https://cdn.fwupd.org/downloads/firmware.xml.gz
Downloading…             [***************************************]
Fetching signature https://cdn.fwupd.org/downloads/firmware.xml.gz.asc

Successfully downloaded new metadata: 1 local device supported

$ fwupdagent get-updates
{
  "Devices" : [
    {
      "Name" : "ColorHug2",
      "DeviceId" : "983c3cffc6fd36d32b00b62928d30721eaeb93db",
      "Guid" : [
        "2082b5e0-7a64-478a-b1b2-e3404fab6dad",
        "aa4b4156-9732-55db-9500-bf6388508ee3",
        "101ee86a-7bea-59fb-9f89-6b6297ceed3b",
        "2fa8891f-3ece-53a4-adc4-0dd875685f30",
        "182ab329-1976-5d99-a662-ce618d2ccf88"
      ],
      "Summary" : "An open source display colorimeter",
      "Plugin" : "colorhug",
      "Protocol" : "com.hughski.colorhug",
      "Flags" : [
        "updatable",
        "supported",
        "registered",
        "self-recovery"
      ],
      "Vendor" : "Hughski Ltd.",
      "VendorId" : "USB:0x273F",
      "Version" : "2.0.6",
      "VersionFormat" : "triplet",
      "Icons" : [
        "colorimeter-colorhug"
      ],
      "InstallDuration" : 8,
      "Created" : 1612800403,
      "Releases" : [
        {
          "AppstreamId" : "com.hughski.ColorHug2.firmware",
          "RemoteId" : "lvfs",
          "Summary" : "Firmware for the Hughski ColorHug2 Colorimeter",
          "Description" : "<p>This release fixes prevents the firmware returning an error when the remote SHA1 hash was never sent.</p>",
          "Version" : "2.0.7",
          "Filename" : "658851e6f27c4d87de19cd66b97b610d100efe09",
          "Protocol" : "com.hughski.colorhug",
          "Checksum" : [
            "80bddeb898cda5b87d9837e13a9ace19846053bf"
          ],
          "License" : "GPL-2.0+",
          "Size" : 16384,
          "Uri" : "https://fwupd.org/downloads/0a29848de74d26348bc5a6e24fc9f03778eddf0e-hughski-colorhug2-2.0.7.cab",
          "Homepage" : "http://www.hughski.com/",
          "SourceUrl" : "https://github.com/hughski/colorhug2-firmware",
          "Vendor" : "Hughski Limited",
          "Flags" : [
            "is-upgrade"
          ],
          "InstallDuration" : 8
        }
      ]
    }
  ]
}

@hughsie
Copy link
Member

hughsie commented Feb 8, 2021

Fetching signature https://cdn.fwupd.org/downloads/firmware.xml.gz.asc

That looks like a very old fwupd version; possibly one without the SHA256 support?

@Asiderr
Copy link
Collaborator Author

Asiderr commented Feb 8, 2021

@hughsie The same result on Fedora32, fwupd 1.5.5 (updates-testing). Metadata contains the SHA256 of the .cab archive, but fwupdagent doesn't provide the checksum.

@hughsie
Copy link
Member

hughsie commented Feb 8, 2021

Hmm, will investigate. https://bodhi.fedoraproject.org/updates/FEDORA-2021-a83da4c566 now exists for libjcat if that helps.

@hughsie
Copy link
Member

hughsie commented Feb 8, 2021

Hmm, will investigate

Ha, so I think we accidentally fixed this in master a few weeks ago when we added the artifact support. I Can tag a new tarball release this week if that helps.

@Asiderr
Copy link
Collaborator Author

Asiderr commented Feb 8, 2021

I Can tag a new tarball release this week if that helps.

This will help a lot. I will run the wrapper tests after the release.

@superm1

This comment has been minimized.

@hughsie
Copy link
Member

hughsie commented Feb 23, 2021

@Asiderr I've built 1.5.7 for F32 just now: https://bodhi.fedoraproject.org/updates/FEDORA-2021-449618a8a8 -- can you rebase this patchset, do the testing to fix up the remaining issues, and then I think we should aim to get this upstream before it bitrots any more than it has already.

@Asiderr
Copy link
Collaborator Author

Asiderr commented Feb 23, 2021

@hughsie Tomorrow I'll rebase and test the newest changes and we can focus on the upstream. Also until the end of this week I want to push fwupd for BSD fixes.

@Asiderr Asiderr force-pushed the wip/3mdeb/qubes-wrapper branch from 93006e7 to fdd7796 Compare February 25, 2021 11:04
Copy link
Member

@hughsie hughsie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@superm1 can you verify the Debian stuff please.

@Asiderr
Copy link
Collaborator Author

Asiderr commented Feb 25, 2021

@hughsie Wrapper is working fine. I added some minor fixes to the tests and docs. I'll push force one more change in a minute. The RPM packages must require specific versions of the libjcat and fwupd.

Norbert Kamiński added 2 commits February 25, 2021 12:41
This patch is adding the fwupd wrapper for Qubes.
The wrapper provides fwupd functionalities for Qubes R4.1.
It creates three packages (two RPMs and one Debian package):
fwupd-qubes-dom0 (RPM)
fwupd-qubes-vm (RPM)
fwupd-qubes-vm-whonix (deb)
More information about the wrapper could be found in the
contrib/qubes/README.md

Signed-off-by: Norbert Kamiński <[email protected]>
@Asiderr Asiderr force-pushed the wip/3mdeb/qubes-wrapper branch from fdd7796 to 7ea1bc9 Compare February 25, 2021 11:43
@hughsie hughsie merged commit ea70435 into fwupd:master Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants