Bug 1454824

Summary: Sign akmods modules with local CA when secure boot enable
Product: [Fedora] Fedora Reporter: Stanislas Leduc (shann) <stanislas.leduc>
Component: akmodsAssignee: Nicolas Chauvet (kwizart) <kwizart>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 35CC: abuse, accounts+fedora, bordjukov, Daniel, dherault, duboismathieu_gaas, elia.f.geretto, fedoraproject, hdegoede, kwizart, ldelouw, leigh123linux, markus.linnala, markus.schuster, mihai, monteiro, nicolas.vieville, peter, rainer.neunteufel, ricky.tigg, roshan.shariff, sergio, shawn.starr
Target Milestone: ---Keywords: Reopened
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2021-12-09 11:31:08 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
akmods specfile
none
akmods local CA
none
akmods specfile
none
akmods
none
amods-genca script
none
README for usage with secureboot on
none
config file used by akmods-genca
none
akmods certificate config file
none
sign_modules_in_install_section_of_spec_file.txt
none
kmodtool_self_signing_funtionality.patch
none
Implementation of modsign feature
none
kmodtool_001_detects_akmods_group_presence.patch none

Description Stanislas Leduc (shann) 2017-05-23 14:06:27 UTC
Created attachment 1281568 [details]
akmods specfile

Description of problem:
Can't use akmods with secure boot enable, because modules was unsigned.

Version-Release number of selected component (if applicable):
akmods-0.5.6-1

How reproducible:
Install Fedora (any version) on station with Secure Boot enable.
Try to install akmods modules.


Steps to Reproduce:
1. Install Fedora
2. Install akmods modules (example akmod-VirtualBox)
3. Try to create or load Virtual Machine on VirtualBox

Actual results:
Modules not loaded because they unsigned with local CA present in MOK.

Expected results:
Create local CA and signing modules with it.

Additional info:
I begin stuff to add feature in akmods package.

Comment 1 Stanislas Leduc (shann) 2017-05-23 14:07:22 UTC
Created attachment 1281569 [details]
akmods local CA

Comment 2 Nicolas Chauvet (kwizart) 2017-05-23 14:34:45 UTC
Thx for this update.

Quick notes:
- It's usually better to use git diff against the current akmods content. Or even commit, then use git format-patch -o /tmp origin/master), but git diff would be a better step.
- You should not output any text on akmods post/postun installation.
Better to have a README.secureboot installed in docs to describe the enroll process.
- Please verify the perms for the private key to be at least 600 or even 640 (root:akmods)
- There is a need to generate a random number for earch generated keypairs, so two different key won't have the same name. (using hostname, but probably some others random numbers).
- The command to generate the keypair should probably be moved to a separate script. (so one can tweak the default akmods_cacert.config with it's own value and recreate the keypair).
- private/pub keys should probably be owned by the package (as %ghost).
- You shouldn't test if secureboot is enabled at all IMO, as it's possible to generate the signed kmods for another host that has secure boot enabled. Whereas the host used to produce them might or might not have secure boot at all.

Also having secure boot automatically handled is probably questionable a secure POV. I think it's a good path, but this also requires to understand that such keys need to be protected in order to keep the feature to be significant.
For example, one probably needs to have the whole system encrypted to avoid 3rd party to access the key and inject a key-logger or else into the system...

Have you looked into a mean to sign the produced kmod (when akmods build them)? 
(you probably need to look into the kernel.spec to re-use some RPM macros).

Comment 3 Stanislas Leduc (shann) 2017-05-23 19:07:50 UTC
Thanks Nicolas for your advices.

Infact, i join full file instead diff delta.

I continue work in follow notes.
I didn't think that modules build and sign on another system, which is the case with the Fedora infra :/.

For secure boot and 3rd party access infact, if private key compromise, attack can create new module and load them in kernel.
In this case, we can build and sign modules in dedicate node, but for final user not best usage.

For sign kmod produced by akmods, i look to add sign-file command on akmods build process afer module created, but i check kernel.spec.

Comment 4 Stanislas Leduc (shann) 2017-05-24 07:00:02 UTC
Created attachment 1281836 [details]
akmods specfile

Comment 5 Stanislas Leduc (shann) 2017-05-24 07:00:44 UTC
Created attachment 1281837 [details]
akmods

Comment 6 Sergio Basto 2017-05-24 10:12:02 UTC
(In reply to Stanislas Leduc (shann) from comment #5)
> Created attachment 1281837 [details]
> akmods

you need attach these 4 files: 	

Source11:       README.secureboot
Source12:       cacert.config
Source13:       akmods-genca
Source14:       sign-keypair.conf

Comment 7 Stanislas Leduc (shann) 2017-05-24 10:15:47 UTC
Created attachment 1281910 [details]
amods-genca script

Comment 8 Stanislas Leduc (shann) 2017-05-24 10:16:25 UTC
Created attachment 1281911 [details]
README for usage with secureboot on

Comment 9 Stanislas Leduc (shann) 2017-05-24 10:17:34 UTC
Created attachment 1281912 [details]
config file used by akmods-genca

Comment 10 Stanislas Leduc (shann) 2017-05-24 10:18:23 UTC
Created attachment 1281914 [details]
akmods certificate config file

Comment 11 Stanislas Leduc (shann) 2017-05-24 10:22:13 UTC
Hi,

I pushed all files.
In state, add feature to generate keypair.
I need to add function in akmodsbuild to read configfile and sign kmod during rmpbuild process.

I also create repository in github, to store stuff
https://github.com/shannara/akmods

Best regards

Comment 12 Sergio Basto 2017-05-25 10:39:24 UTC
(In reply to Stanislas Leduc (shann) from comment #11)
> I also create repository in github, to store stuff
> https://github.com/shannara/akmods


Hi, sorry I'm a little short of time, please use github import [1], clone https://src.fedoraproject.org/git/rpms/akmods.git and apply there yours patches will be more easier (and quick) to read 

Thanks 

[1] 
https://help.github.com/articles/importing-a-repository-with-github-importer/

Comment 13 Stanislas Leduc (shann) 2017-05-28 16:38:15 UTC
Hi Sergio,

No problem, i understood,
I imported akmods project as https://github.com/shannara/akmods-fedora.
I checkout f25 branch to f25-secureboot.

https://github.com/shannara/akmods-fedora/commit/e4daaf61
Changed files :
 - akmods.spec
New files :
 - README.secureboot
 - akmods-genca
 - cacert.config
 - sign-keypair.conf 

Best regards

Comment 14 Jan Kurik 2017-08-15 08:58:01 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 27 development cycle.
Changing version to '27'.

Comment 15 nicolas.vieville 2017-09-24 17:47:07 UTC
Created attachment 1330184 [details]
sign_modules_in_install_section_of_spec_file.txt

Hello,

As a future user of this feature on "f26 UEFI secure boot machines", I'm very interested in this enhancement of akmods package.

According to the files modified in your github repository, and keeping in mind fedora and el6/7 possibilities of signing modules (native sign-file program or perl script), it would be necessary to modify install section in modules specfiles as shown in the attached file. 

The attached file contains only the install section and is probably not valuable to be used now, but is provided as an example. I haven't tested it and cannot say if it is working or not. Sorry for that. 
The idea is to install modules, verify if signing option is enabled in the /etc/akmods/sign-keypair.conf file, then for each module, verify if sign-file is a perl script or a native program, then sign the modules with the public and private keys files extracted from the /etc/akmods/sign-keypair.conf file.

Hope this will be understandable. Feel free to drop me a line about this subject.

Cordially,


-- 
NVieville

Comment 16 Stanislas Leduc (shann) 2017-09-25 07:17:32 UTC
Hello Nicolas,

Thank you very much for your proposal, I realize that I did not continue my patch until the end :(.

I'm sorry, following a few events my motivation has been falling.
But thanks to the community, I regained the hair of the beast :).

So I will make every effort to make the patch available ASAP.

Best regards,

Comment 17 Nicolas Chauvet (kwizart) 2017-09-25 13:47:51 UTC
(In reply to nicolas.vieville from comment #15)

> According to the files modified in your github repository, and keeping in
> mind fedora and el6/7 possibilities of signing modules (native sign-file
> program or perl script), it would be necessary to modify install section in
> modules specfiles as shown in the attached file. 
You shoudn't need to modify anything in any individual *kmod.spec files.
Instead, please have a look at the kernel spec and try to re-use the same commands and rpm macros as a base (to sign/compress). Theses need to be emitted by kmodtool and installed into __os_install_post to be executed in after the %install step.

Comment 18 Stanislas Leduc (shann) 2017-09-26 15:23:21 UTC
Hi,

Thanks Nicolas for your help, i pushed in my repository stuff for kmodtool and akmods.

https://github.com/shannara/akmods-fedora/ (f27-secureboot)
https://github.com/shannara/kmodtool-fedora/ (f27-secureboot)

This steps by steps workflow :
- Install akmods package 
- Read the README.secureboot to create and enroll keypair in MOK
- Install akmod third package, script kmodtool is called during build process.
- During build process, sign-keypair.conf file is reading to check if need sign module with keypair storing in /etc/akmods/keys.
- The third package is installed with sign and compress modules.

Enjoy, your third modules can be loaded in safe state :-).

It's WIP, i continue to work to purpose this for Fedora 27.

Best regards,

Comment 19 nicolas.vieville 2017-09-28 17:07:33 UTC
Hello,

Sorry for this naive question, but wouldn't it have been better to package these features in a single package? For example the kmodtool package.

As an experimentation, I gave a try in a VM (f26) with secure boot activated.
Install akmods stock package, generate keys, enrol keys.
Modify kmodtool and its dependencies as it is done in the attached patch.
I've picked up the interesting files from your akmods package and copied/renamed them for/to the kmodtool package.

README.secureboot
cacert.config
kmodtool-genca
kmodtool-kmodsign
kmodtool-kmodtool
sign-keypair.conf

The only thing I've modified in the cacert.config file is the emailAddress (root).
I've created the below directory in /etc

# ls -lR /etc/kmodtool
/etc/kmodtool:
total 12
-rw-r-----. 1 root akmods  356 28 sept. 11:24 cacert.config
drwxr-x---. 2 root akmods 4096 28 sept. 11:24 keys
-rw-r-----. 1 root akmods  460 28 sept. 12:14 sign-keypair.conf

/etc/kmodtool/keys:
total 8
-rw-r-----. 1 root akmods 1412 28 sept. 11:24 fedora64-3017304.der
-rw-r-----. 1 root akmods 3272 28 sept. 11:24 fedora64-3017304.priv
lrwxrwxrwx. 1 root root     40 28 sept. 11:24 private_key.priv -> /etc/kmodtool/keys/fedora64-3017304.priv
lrwxrwxrwx. 1 root root     39 28 sept. 11:24 public_key.der -> /etc/kmodtool/keys/fedora64-3017304.der

Install/copy kmodtool-kmodtool to /usr/bin/kmodtool and kmodtool-kmodsign /usr/bin/kmodsign.
Then, install one akmod-package from rpmfusion (for example akmod-wl).
modprobe wl (lsmod shows wl module).
modinfo wl (no keys listed but key field are present).
modprobe -r wl.

One thing I didn't investigate yet, is that in this VM (Qemu/KVM), the %ifarch %{ix86} x86_64 line in kmodtool is not correctly evaluated and fails always in the %else part with zipmodules set to 0, when building the module with akmods.
If I force zipmodules to 1, the module is correctly compacted at build time.
Maybe an issue related to the VM.

One last thing. I've had kmodtool the modifications I proposed to Kwizard in order to be compatible with rhel6. Akmods needs also modifications. 
See https://bugzilla.redhat.com/show_bug.cgi?id=1484293 and https://bugzilla.redhat.com/show_bug.cgi?id=1484295 for this.

Feel free to make any comment about this.

Cordially,


-- 
NVieville

Comment 20 nicolas.vieville 2017-09-28 17:08:51 UTC
Created attachment 1332068 [details]
kmodtool_self_signing_funtionality.patch

Comment 21 nicolas.vieville 2017-09-29 13:59:31 UTC
> I've had kmodtool the modifications
I've added kmodtool the modifications

Sorry for the mistake.

I didn't mention in my previous post that the patch provided doesn't modify the kmodtool.spec file in order to reflect its new features (new files included for example).

Cordially,


-- 
NVieville

Comment 22 Stanislas Leduc (shann) 2017-09-30 07:36:11 UTC
Hi Nicolas,

Thanks you for your purpose and investigation :).
Infact your are right, it makes more sense that it is only kmodtool implement all stuff, genca and keypair. 
akmods packages call only kmodtool to rebuild module.

In my case i used VirtualBox, and module load and sign successfully, for zipmodules fail, it's my fault, i check them and see that compress modules only in sign modules case :/.

I check also patch for rhel6 to implement them.

Best regards,

Comment 23 Stanislas Leduc (shann) 2017-10-04 21:03:12 UTC
Hi Nicolas,

I rebuild kmodtool package with stuff, it's work but :

In Qemu/KVM, same issue with zipmodules function, modules was signed but not compressed.
Because work with VirtualBox, i suspect issue with Qemu/KVM.

Also i have an issue with SELinux with 'systemd-modules'

```
type=AVC msg=audit(1507149223.903:298): avc:  denied  { map } for  pid=8403 comm="systemd-modules" path="/usr/lib/modules/4.13.3-301.fc27.x86_64/extra/VirtualBox/vboxguest.ko" dev="dm-0" ino=814519 scontext=system_u:system_r:systemd_modules_load_t:s0 tcontext=system_u:object_r:modules_object_t:s0 tclass=file permissive=0
type=AVC msg=audit(1507149621.355:390): avc:  denied  { map } for  pid=14268 comm="systemd-modules" path="/usr/lib/modules/4.13.3-301.fc27.x86_64/modules.dep.bin" dev="dm-0" ino=808556 scontext=system_u:system_r:systemd_modules_load_t:s0 tcontext=unconfined_u:object_r:modules_object_t:s0 tclass=file permissive=1
type=AVC msg=audit(1507149621.355:391): avc:  denied  { map } for  pid=14268 comm="systemd-modules" path="/usr/lib/modules/4.13.3-301.fc27.x86_64/extra/VirtualBox/vboxguest.ko" dev="dm-0" ino=814522 scontext=system_u:system_r:systemd_modules_load_t:s0 tcontext=system_u:object_r:modules_object_t:s0 tclass=file permissive=1
```

In case i think issue with context "modules_object_t" and "systemd_modules_load_t".

Best regards,

Comment 24 Stanislas Leduc (shann) 2017-10-04 21:06:29 UTC
(In reply to Stanislas Leduc (shann) from comment #23)
> Hi Nicolas,
> 
> I rebuild kmodtool package with stuff, it's work but :
> 
> In Qemu/KVM, same issue with zipmodules function, modules was signed but not
> compressed.
> Because work with VirtualBox, i suspect issue with Qemu/KVM.
> 
> Also i have an issue with SELinux with 'systemd-modules'
> 
> ```
> type=AVC msg=audit(1507149223.903:298): avc:  denied  { map } for  pid=8403
> comm="systemd-modules"
> path="/usr/lib/modules/4.13.3-301.fc27.x86_64/extra/VirtualBox/vboxguest.ko"
> dev="dm-0" ino=814519 scontext=system_u:system_r:systemd_modules_load_t:s0
> tcontext=system_u:object_r:modules_object_t:s0 tclass=file permissive=0
> type=AVC msg=audit(1507149621.355:390): avc:  denied  { map } for  pid=14268
> comm="systemd-modules"
> path="/usr/lib/modules/4.13.3-301.fc27.x86_64/modules.dep.bin" dev="dm-0"
> ino=808556 scontext=system_u:system_r:systemd_modules_load_t:s0
> tcontext=unconfined_u:object_r:modules_object_t:s0 tclass=file permissive=1
> type=AVC msg=audit(1507149621.355:391): avc:  denied  { map } for  pid=14268
> comm="systemd-modules"
> path="/usr/lib/modules/4.13.3-301.fc27.x86_64/extra/VirtualBox/vboxguest.ko"
> dev="dm-0" ino=814522 scontext=system_u:system_r:systemd_modules_load_t:s0
> tcontext=system_u:object_r:modules_object_t:s0 tclass=file permissive=1
> ```
> 
> In case i think issue with context "modules_object_t" and
> "systemd_modules_load_t".
> 
> Best regards,

Seem related to #1426906

Comment 25 Sergio Basto 2017-10-05 14:48:24 UTC
FYI, may be useful information , I found this project [1] today. 


[1]
https://github.com/aneesh-neelam/UEFI-SecureBoot-SignTool
https://github.com/UnitedRPMs/sb-signtool/blob/master/sb-signtool.spec

Comment 26 Stanislas Leduc (shann) 2017-10-05 20:46:36 UTC
Hi sergio, 

I check this, i seem see you contribute on this project.
it's third-project no ?

@Nicolas (kwizart), i build new kmodtool package, (my github repository is up-to-date with change).
I try it on my laptop (Fedora 27 + UEFI with Secure Boot), work great, but not compress modules, because selinux issue.

Best regards,

Comment 27 Stanislas Leduc (shann) 2017-10-05 20:52:54 UTC
Created attachment 1335005 [details]
Implementation of modsign feature

This is a bundle path for sigmod feature to kmodtool.

Comment 28 nicolas.vieville 2017-10-07 07:49:57 UTC
Created attachment 1335638 [details]
kmodtool_001_detects_akmods_group_presence.patch

Hello,

Trying to make an rpm package with your last source for kmodtool from github, all goes well, except when installing the resulting package. Scriptlet fails when adding akmods group if this group already exists.

Here is a patch correcting this on the F26 station where I tested your new package.

Once the kmodtool.spec file corrected, all is working as wanted, and provides an easy way to install third party akmod-modules with secure-boot activated. Great job.

Cordially,


-- 
NVieville

Comment 29 Stanislas Leduc (shann) 2017-10-08 13:48:57 UTC
Hi Nicolas,

I test my package in fresh installation, my bad not check them in case of akmods already installed.
Thanks for review, i fix spec file and push them on github.

Thanks a lot, it's pleasure to contribute on community ;).

Best regards,

Comment 30 nicolas.vieville 2017-11-12 21:19:49 UTC
Hi Stanislas,

If you want to have a look at it, I made some re-adjustments (new features, re-ordered code, fixed some minor glitches, etc. ) to kmodtool from your work. This WIP is available on a pagure fork of Fedora official kmodtool repository, here:

https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_bug1484041_and_more

The tests I made on a Fedora 26 bare metal workstation (signing and zipping) or on Centos 7.x and 6.x Qemu/KVM VMs (zipping only on 7.x) were conclusive. Maybe it would be interesting to make more tests with this new version, in order to be sure it is working as it should?

Feel free to make any comment about this.

Cordially,


-- 
NVieville

Comment 31 Stanislas Leduc (shann) 2017-11-13 08:15:31 UTC
Hi Nicolas,

I go to show that, thanks for your help, seem good work, i test them as soon as on my laptop (running on F27 Beta) and VMs to confirm workflow work.

I see that i not implement all tests for backward compatibility (el6, el7).
With your refactoring i think understood all stuff necessary for provide package with great level.

Best regards,

Comment 32 Stanislas Leduc (shann) 2017-11-22 07:44:37 UTC
Hi Nicolas,

I test your branch on my laptop and VirtualBox, work very weel ;).
On VirtualBox i upgrade Fedora 27 Beta to latest release, including new kernel, after reboot kernel modules was rebuilt and signed auto with enrolled key.

Best regards,

Comment 33 nicolas.vieville 2017-11-24 08:53:02 UTC
Hi Stanislas,

Thank you very much for your feedback. 

Did you notice if the package was also zipped while signed?

modinfo will gives you all this information.

Cordially,


-- 
NVieville

Comment 34 Stanislas Leduc (shann) 2017-11-24 09:22:19 UTC
Hi Nicolas,

Nothing, it's normal :)
Yes infact modules was signed and zipped.
I checking them with modinfo to confirm sign work

Best regards,

Comment 35 Nicolas Chauvet (kwizart) 2017-11-26 09:18:26 UTC
Few comments: (only partial review).

- The ident fixes are (probably okay).
- The %if 0%{?rhel} condition to handle the kABI is not accurate for the long run. Best is to set a separate condition such as _kmodtool_kabi or else. That will fix a corner case when using a fedora userspace with a rhel kernel (if the issue arise using containers).
Then , a good approximation for now would be to set:
%if 0%{?rhel}
%global _kmodtool_kabi 1
%end
- I don't think it worth to fix the group for rhel kernel, Group could be dropped completely. RPM Fusion still uses it to set the category with repoview, but it's not reliable.
- The generated keys should probably be better installed in /etc/pki/akmods (from the akmods package to avoid to install akmods user from kmodtool), then sign-keypair.conf can be publicly readable (but provided from akmods).
- The option parsing in kmodtool-kmodtool for sign-keypair.conf is weak, it should be easier to just "source" the file instead of using grep.
Even better would be to use rpm macros: such as signmodules (instead of SIGNMOD=enabled/disabled). and sigmodules_pubkey,privkey for keys.
- The /usr/bin/kmodsign should be moved into /usr/lib/rpm/kmodsign (and re-use the logical for the other scripts in this directory when possible.
- You need to squash some commit (re-order and others fixes)
- I don't think using rpm macro for install/mkdir/sed worth it. It just make things harder to read.

For me, theses commit can be merged for devel (f28):
- kmodtool-kmodtool clean-up
- Disable globally the build of package-debuginfo
- Add -e to depmod (i.e. -aeF)

(I will add you to the ACL) for kmodtool/akmods

Comment 36 Stanislas Leduc (shann) 2018-04-13 08:13:55 UTC
Hi,

Sorry for this absent :/.

I show that Nicolas Vieville continue on them based on my work.
I test akmods and kmodtool 

https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features

https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/branch/enhancements_and_secure_boot

Build on my F28 Beta, work very well.

I notice seem module not zipped, i check for them.

Best regards

Comment 37 Stanislas Leduc (shann) 2018-04-13 09:34:18 UTC
Few review,


When reboot with new kernels, akmods generate new modules sucessfully, but no zipped.

But also, akmods seem recreate new couple of keypair, also that already exist :/.

I check that.

Best regards

Comment 38 nicolas.vieville 2018-04-13 17:45:12 UTC
Hello Stanislas,

Thank you for your feedback.

I didn't noticed the problem with the zipping of the newly created modules (nvidia) on fc27. Maybe you should provide the log file in order to try to understand this issue (/var/cache/akmods/<your_module>/.last.log).

The couple of keys are auto-generated only if the symlinks /etc/pki/akmods/keys/public_key.der and /etc/pki/akmods/keys/private_key.priv are not present or doesn't point to any valid file on the first run of the akmods service (at boot time). The previous version of these features used to deal with keys located in /etc/kmodtool/keys directory, that's probably why you get new keys. In order not to have to enroll again these new keys, you can manually move the keys from /etc/kmodtool/keys directory to /etc/pki/akmods/keys one and make the symlinks accordingly.

Hope this help. Please feel free to make any comment about this subject.

Best regards,


-- 
NVieville

Comment 39 Stanislas Leduc (shann) 2018-04-16 19:48:13 UTC
Hi Nicolas,

I install akmods/kmodtool with your repository, and build rpms.

Run /usr/lib/rpm/kmodgenca, enroll key and reboot :

[root@jaralen stan]# ls /etc/pki/akmods/keys/ -l
total 16
-rw-r-----. 1 root akmods 1406 Apr 16 21:28 jaralen-195563097.der
-rw-r-----. 1 root akmods 3272 Apr 16 21:28 jaralen-195563097.priv
-rw-r-----. 1 root akmods 1408 Apr 16 21:26 jaralen-4077600672.der
-rw-r-----. 1 root akmods 3272 Apr 16 21:26 jaralen-4077600672.priv
lrwxrwxrwx. 1 root root     43 Apr 16 21:28 private_key.priv -> /etc/pki/akmods/keys/jaralen-195563097.priv
lrwxrwxrwx. 1 root root     42 Apr 16 21:28 public_key.der -> /etc/pki/akmods/keys/jaralen-195563097.der

See that new pair created just after reboot, instead of use first generated.

For VirtualBox, i install VirtualBox from RPM Fusion, and akmods build modules

[root@jaralen keys]# ls /lib/modules/4.16.1-300.fc28.x86_64/extra/VirtualBox/ -l
total 1464
-rw-r--r--. 1 root root 712176 Apr 16 21:36 vboxdrv.ko
-rw-r--r--. 1 root root 559976 Apr 16 21:36 vboxguest.ko
-rw-r--r--. 1 root root  18704 Apr 16 21:36 vboxnetadp.ko
-rw-r--r--. 1 root root  54936 Apr 16 21:36 vboxnetflt.ko
-rw-r--r--. 1 root root  50992 Apr 16 21:36 vboxpci.ko
-rw-r--r--. 1 root root  90336 Apr 16 21:36 vboxsf.ko

See that not zipped and i see that also not signing o_O.

On /etc/akmods/sign-keypair.inc
%{!?__signmodules: %global __signmodules 1}

I not check script for moment, i test with fresh packages.

Now i diag scripts to see why i have issues (use Fedora 28 Beta).

Best regards,
Stanislas

Comment 40 Stanislas Leduc (shann) 2018-04-16 22:04:32 UTC
After investigation and debug, 

Always keys generated, infact wrong typo in akmod-keygen@.service

ConditionFileNotEmpty=|!/etc/pki/akmods/keys/public_key.der
ConditionFileNotEmpty=|!/etc/pki/akmods/keys/public_key.priv


Second condition need point to private_key.priv.

But for zip, i continue investigation.

Best regards,
Stanislas

Comment 41 Stanislas Leduc (shann) 2018-04-16 22:26:51 UTC
If i force "__zipmodules 1" in /usr/bin/kmodtool

        echo '## Zip modules on x86 and x86_64'
        echo '%ifarch %{ix86} x86_64'
        echo ' %{!?__zipmodules: %global __zipmodules %( if (( 0%{?rhel} > 6 )) || (( 0%{?fedora} > 0 )) ; then echo 1 ; else echo 0 ; fi )}'
        echo '%global __zipmodules 1'
        echo '%else'
        echo ' %{!?__zipmodules: %global __zipmodules 0}'
        echo '%endif'

Now zip modules work.
I think if condition failed to define value.

Best regards,
Stanislas

Comment 42 nicolas.vieville 2018-04-17 07:32:15 UTC
Hello Stanislas,

Concerning akmod-keygen@.service, I think you get an old commit on my akmods pagure fork repository. Please could you grab the last one and have a try?

Concerning zipping modules, could please precise if you're trying on a vm or a bare metal machine? I think we already hit some problems on vms. For my part, with the last commits of the two repositories, I didn't hit issues like yours, on bare metal f27 machines, or in kvm/Qemu vms running centos6 and centos7.

Thanks in advance,

Best regards,


-- 
NVieville

Comment 43 Stanislas Leduc (shann) 2018-04-17 08:45:10 UTC
Hi Nicolas,

Ok, i see on pagure, file is good, i need update my local repo.

For zipping modules, i use my laptop on F28 Beta for test them.

Best regards,
Stanislas

Comment 44 Stanislas Leduc (shann) 2018-04-17 09:15:37 UTC
Nicolas,

I install new packages from updated repository.

For first step, akmods code update and is good :).

But for zipping modules, always not zip modules, i check %{?fedora} macro, see 28, condition 028 > 0 is true, then __zipmodules should be set to 1.

Best regards,
Stanislas

Comment 45 Stanislas Leduc (shann) 2018-04-17 09:23:39 UTC
I check if condition in bash script

fedora=28
if (( 0$rhel > 6 )) || (( 0$fedora > 0 )) ; then echo 1 ; else echo 0 ; fi

line 5: ((: 028: value too great for base (error token is "028")

If use [[ in /usr/bin/kmodtool work great :)
echo ' %{!?__zipmodules: %global __zipmodules %( if [[  0%{?rhel} > 6 || 0%{?fedora} > 0 ]] ; then echo 1 ; else echo 0 ; fi )}'

Module is zipped and signed.

Best regards,
Stanislas

Comment 46 nicolas.vieville 2018-04-17 13:10:46 UTC
Hello Stanislas,

Good catch.

For the complete explanation see here:

https://stackoverflow.com/questions/24777597/shell-script-error-value-too-great-for-base-error-token-is-08

To sum-up, (( 0$fedora > 0 )) with the leading "0" is considered as an octal value, so 28 is not in the base but 27 is.
Unfortunately your proposition isn't working on centos6/7 but is right on fedora.

I will correct my pagure repository as soon as possible with this (actually at work with restrictive proxy ;)) ):

echo ' %{!?__zipmodules: %global __zipmodules %( if (( %{?rhel} + 0 > 6 || %{?fedora} + 0 > 0 )) ; then echo 1 ; else echo 0 ; fi )}'

Seems working here on fc27, fc28, centos6 and centos7.

Thank you very much for your review.

Best regards,


-- 
NVieville

Comment 47 Stanislas Leduc (shann) 2018-04-17 13:52:41 UTC
Hi Nicolas,

Infact, i see similar link, but answer explain use brackets [[.

Nothing, thanks to you for continue working on feature :).
I tested in Qemu/KVM with SecureBoot for F27, work very well.

Best regards,
Stanislas

Comment 48 nicolas.vieville 2018-04-17 18:59:44 UTC
Stanislas,

My pagure fork of kmodtool has been updated with the fix we discuss above (#46).

Best regards,


-- 
NVieville

Comment 49 Stanislas Leduc (shann) 2018-04-18 15:29:45 UTC
Hi Nicolas,

Thanks for update :).

Best regards,
Stanislas

Comment 50 Stanislas Leduc (shann) 2018-07-09 19:22:53 UTC
Hi,

This few months, i created copr repo from kmodtool / akmods fork of Nicolas.

https://copr.fedorainfracloud.org/coprs/shannara/akmods-secureboot/
https://copr.fedorainfracloud.org/coprs/shannara/kmodtool-secureboot/

My current system running on Fedora 28 with this copr activate (home and also at work), work very well, update kernel and same time, third modules (in my case VirtualBox), are transparency.

With Scheduler roadmap of Fedora 29, this feature can't be added on future release.
I purpose to continue tests/review this during 29 lifecycle, to integrate this on next release Fedora 30.

Best regards,
Stanislas

Comment 51 Nicolas Chauvet (kwizart) 2018-07-09 20:10:08 UTC
(In reply to Stanislas Leduc (shann) from comment #50)
[...]
> With Scheduler roadmap of Fedora 29, this feature can't be added on future
> release.
> I purpose to continue tests/review this during 29 lifecycle, to integrate
> this on next release Fedora 30.
This isn't quite true WRT kmodtool/akmods as theses tools aren't used in any Fedora deliverable. So, it will be possible to merge theses patches whenever they will be ready.
At least that's my interpretation of the feature process. 

Having a working akmods/kmodtool signing process is certainly a good way forward. But it's not more important that to merge tool with a good design for a better maintainability. That's  a 1 and several order or magnitude ratio in favor of good design for me.

However I should be able to head a code review for this hopefully this week. Thanks for the reminder.

Comment 52 Stanislas Leduc (shann) 2018-07-10 07:56:55 UTC
Hi Nicolas,

Thanks to explain.

Infact, better to have clean an maintainable code.
Thanks to your review.

Best regards,
Stanislas

Comment 53 Nicolas Chauvet (kwizart) 2018-09-21 21:27:00 UTC
Sorry for the late answer:

Here is quick code review: (nothing are tested yet).
I don't see a good way to annotate comments on pagure.
Reviewing that :
https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features

See the comments on the patch name:

* Improve detection of already installed (weak-)modules in akmods (RHEL)
Please drop the "if [[ "${this_kernelver}" == *"el"* ]] ; then" condition.
It should be possible to detect weak-modules support either or not the kernel is an EL kernel (RHEL/CentOS).
In others word one should be able to build a custom kernel with weak-modules that would run on Fedora/CentOS or any unknown downstream vendor OS.
So
-> the directory condition should be enough
-> Don't harcode the compression extension (if possible use a wildcard).


* Add Secure boot feature and support
- kmodgenca shoudn't be in rpmconfigdir as it's not used during the packaging, this is an admin script. you can move it to /usr/sbin or /usr/libexec. 
- kmodsign script should be moved into the kmodtool package. And follow the convention used by brp scripts (name should be brp-kmodsign to denote that it works in the buildroot.
- sign-keypair.inc is this file a rpmmacros file ? It should be /etc/rpm/macros.akmods then?! Or I don't get how the rpm macros would work.
The comments are probably wrong either the 
- akmods-kmodsign, I don't get the interest to test if the sign-file is a perl script. Doesn't the script cannot find the interpreter on purpose ? 

* Fix kernel list build when parsing command line options
It's unclear in which condition you have a problem and what problem you want to fix. Please explain.

* Ensure to build for grub default kernel
This is change is fuzzy
You are issuing the "grubby --default-kernel" command twice. Please get the default kernel and store it in a global variable early on the script. Also a basename call can be avoided using sed to also remove the directory path


About https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features

OK - Improve and fix kernel_uname_r* variables (interest ?)
OK - Fix minor glitches

- Try to fix: check if uname from Makefile is the same as kernel variable
I don't get the point of this check (even from the todo list)
If we enter in a kernel makefile then the uname -r in the directory is right. If the Makefile is wrong, we can do nothing about it , don't loose time checking.

- Add signing and zipping modules methods for Fedora and RHEL
There we are. You are parsing the sign-keypair.inc file instead or simply using the rpm macros defined as appropriate.

You are dealing with either or not one particular feature should be enabled based on the arch and kabi capability in the code instead of having the feature enabled as appropriate with an dedicated variable.

There is a mix between RPM macros and bash script, I don't think half of the options would work. Or only by chance.

Can you confirm that the signature is done before the compression ?
Can you sign after the compression ? (I would need to look into the sign-file code).

Comment 54 nicolas.vieville 2018-10-28 00:28:21 UTC
Hello Nicolas,

My turn to be sorry for the delay :)


> Here is quick code review: (nothing are tested yet).

Thank you for reviewing this code.

Comments are inserted in yours.

> I don't see a good way to annotate comments on pagure.
> Reviewing that :
> https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/
> secure_boot_key_features
> 
> See the comments on the patch name:
> 
> * Improve detection of already installed (weak-)modules in akmods (RHEL)
> Please drop the "if [[ "${this_kernelver}" == *"el"* ]] ; then" condition.
> It should be possible to detect weak-modules support either or not the
> kernel is an EL kernel (RHEL/CentOS).
> In others word one should be able to build a custom kernel with weak-modules
> that would run on Fedora/CentOS or any unknown downstream vendor OS.
> So
> -> the directory condition should be enough
> -> Don't harcode the compression extension (if possible use a wildcard).

Done.

> * Add Secure boot feature and support
> - kmodgenca shoudn't be in rpmconfigdir as it's not used during the
> packaging, this is an admin script. you can move it to /usr/sbin or
> /usr/libexec. 

Done. Moved to /usr/sbin and updated README.secureboot akmods-keygen@.service accordingly.

> - kmodsign script should be moved into the kmodtool package. And follow the
> convention used by brp scripts (name should be brp-kmodsign to denote that
> it works in the buildroot.

Done.

> - sign-keypair.inc is this file a rpmmacros file ? 

Yes.

> It should be /etc/rpm/macros.akmods then?! Or I don't get how the rpm 
> macros would work.

Corrected. It is now in /etc/rpm/macros.akmods.
This file was loaded for RHEL via the expand macro, and for Fedora via the load macro. Now, as it is in /etc/rpm directory expand or load macros are useless and all this have been dropped (see below).

> The comments are probably wrong either the 

The comment were deleted via the grep command when included via the expand macro, and were not deleted when using the load macro. This was working on the tests I made. The new file located in /etc/rpm directory is self documented and the tests I made didn't show any problem with the comments.

> - akmods-kmodsign, I don't get the interest to test if the sign-file is a
> perl script. Doesn't the script cannot find the interpreter on purpose ? 

I just followed the example provided here:

https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Kernel_Administration_Guide/sect-signing-kernel-modules-for-secure-boot.html

But, sign-file has the executable bit set on centOS (could not verify on RHEL), so it should be launched without the need for perl on the command line.
I dropped this with the hope it is not mandatory with RHEL.


> * Fix kernel list build when parsing command line options
> It's unclear in which condition you have a problem and what problem you want
> to fix. Please explain.

As the end of the akmods script is a for loop against the content of the kernels variable, all the items of this variable have to be separated by one space. If one invokes akmods script with multiple time the --kernel commutator, then we have to add a space between the items, but not for the first one. That's the reason of this patch.

> * Ensure to build for grub default kernel
> This is change is fuzzy
> You are issuing the "grubby --default-kernel" command twice. Please get the
> default kernel and store it in a global variable early on the script. Also a
> basename call can be avoided using sed to also remove the directory path

Done.

> About
> https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/
> secure_boot_key_features

I suppose you meant:
https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_and_secure_boot

> OK - Improve and fix kernel_uname_r* variables (interest ?)

Once the variables available, it's easier to deal with all the combination of kernels names between Fedora RHEL or centos.

> OK - Fix minor glitches
> 
> - Try to fix: check if uname from Makefile is the same as kernel variable
> I don't get the point of this check (even from the todo list)
> If we enter in a kernel makefile then the uname -r in the directory is
> right. If the Makefile is wrong, we can do nothing about it , don't loose
> time checking.

Maybe this FIXME was added to deal with self compiled kernels, and to avoid misspelling between kernel Makefile and the --for-kernels commutator of kmodtool.

Tell me if you want me to drop this patch.

> - Add signing and zipping modules methods for Fedora and RHEL
> There we are. You are parsing the sign-keypair.inc file instead or simply
> using the rpm macros defined as appropriate.

Maybe I shouldn't have use the grep command to delete the comments included as documentation in the sign-keypair.inc file (now the macros.akmods file). Now that this file is located in /etc/rpm directory, there's no need to load it manually.

> You are dealing with either or not one particular feature should be enabled
> based on the arch and kabi capability in the code instead of having the
> feature enabled as appropriate with an dedicated variable.

Sorry, but I don't really understand what you mean here. 

Deciding to sign modules for secure boot remains a manual operation from the system administrator, and he/she could want to define accordingly the rpm macros included in the macros.akmods file. It's possible to detect if a system was launched with secure boot activated and to get if right keys are enrolled in MOK to sign modules automatically at build time without the need to set a variable in the macros.akmods file, but we need to know what keys to use. For the moment, in this implementation we are dependent of the system administrator. It is probably possible to create a new script that tries to guess what are the right keys to use, but it needs some more investigations to check if possible and to write it.

> There is a mix between RPM macros and bash script, I don't think half of the
> options would work. Or only by chance.

I wonder if you could please tell me what part of the patch was concerned by this comment. 
For the moment the tests I made with the old file were OK (CentOS 6 and 7 in VMs, FC28 bare metal, and Rawhide in a VM), maybe it would have been necessary to try it widely.

Concerning the new macros.akmods file, everything that could be converted to pure rpm macros was converted. But (yes there's a but), I wasn't able to have working macros with the %if and %ifarch directives, so I came back to this old brave shell script encapsulated in rpm macros to achieve a working post install macro able to zip build modules. 
If that's not what you expected, I wondered if you could please tell me the way to do it in pure rpm macros.

The only possible problem encountered is the replacement of the %ifarch macro with shell comparison. To test this shell comparison, I used the --target flag of the rpm or rpmbuild command and set it to the possible value given by the expanded %{ix86} macro. But the other part of the comparison, the %_arch macro, translate the target arch to the canonical name of the arch. This is working correctly, but I wanted to point that all the arches expanded by %{ix86} are finally compared to i386 (the result of %_arch macro in case we use as a target one of arches contained in %{ix86}).

> Can you confirm that the signature is done before the compression ?

Yes. Signature is added in a special section of the module, so it has to be done after any stripping operation and before compression (see the link above on redhat.com).

> Can you sign after the compression ? (I would need to look into the
> sign-file code).

I didn't try this way. But what I have understood of the link above, is that the signing tool is looking for a special section in the compiled module or is able to add it in order to include the signature in the module file. The documentation doesn't say if this tool is able to deal with a compressed module files, so I preferred to keep it this way.

The two Pagure repositories have been updated accordingly to your comments and the responses I made. I kept the same branch name for each one, rebasing only the patches (hash were then modified), it is then necessary to re-import all the repository to get right git history and hashes.

Hope this will lead fedora to be able to deal with secure boot natively.

Any comments about this subject are welcome.

Cordially,


-- 
NVieville

Comment 55 nicolas.vieville 2018-11-08 07:38:43 UTC
Hello Nicolas,

One word to say that all the commit of the branch named enhancements_and_secure_boot on my kmodtool pagure repository fork (https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_and_secure_boot) has been re-based (hash, date, email address, etc. rewritten) upon the master branch of fedora kmodtool pagure repository.

The branch has to be "re-imported" locally if you're working on a local git repository, sorry for the inconvenience.

I still have to work on kmodname and kabi for RHEL as we discussed it in https://src.fedoraproject.org/rpms/kmodtool/pull-request/2.

Cordially,


-- 
NVieville

Comment 56 nicolas.vieville 2018-11-11 22:17:20 UTC
Hello Nicolas,

As discussed in https://src.fedoraproject.org/rpms/kmodtool/pull-request/2 I've modified my kmodtool pagure repository fork (https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_and_secure_boot) to globalize kmodname and to simplify kabi for RHEL.

The branch enhancements_and_secure_boot has been re-based accordingly, so it needs to be "re-imported" locally if you're working on a local git repository, sorry for the inconvenience.

My akmods pagure repository fork (https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features) has been re-based as well, upon the last commits made in Fedora master branch. Same inconvenience as for kmodtool, sorry.

The new akmods and kmodtool packages were tested successfully on VMs (Rawhide, Centos 6 and 7). I'll have to wait next week to test them on f29 real machines.

Hope that this will lead up to get secure boot support in Fedora.

Cordially,


-- 
NVieville

Comment 57 Stanislas Leduc (shann) 2018-11-19 19:42:11 UTC
Hi,

I tested new packages from my copr repository (build today Nov 19), and see issue in akmods package.

Infact with new package, module not signed, after investigations, i show that /etc/rpm/macros.akmods is missing in system, but seem present in package (on build from copr).

When i download package with "dnf download akmods".

akmods source and akmods packages seem not contain macros.akmods.

After create macros.akmods file from copr build, it's working.

I thinks issue with copr builds, because same version of akmods 0.5.7-1, not purpose last build version.

Cordially,
Stanislas

Comment 58 nicolas.vieville 2018-11-21 13:11:44 UTC
Hello Stanislas,

Thanks for testing these new versions of the akmods/kmodtool packages.

My own tests were made through mock and didn't produced any issues. As you suggested it, the issue you encountered was maybe a copr build one, even if your build logs doesn't seem to show anything that jumps to my eyes.

Feel free to make any comment about this subject.

Cordially,


-- 
NVieville

Comment 59 Stanislas Leduc (shann) 2018-11-23 06:28:17 UTC
Hi Nicolas,

Infact, on build copr seem ok, but version is same that previous build "0.5.7-1", in case dnf not see that new version.

If version is bumped to 0.5.7-2, dnf could see that new revision and purpose upgrade them.

Best regards,
Stanislas

Comment 60 Ben Cotton 2018-11-27 13:30:32 UTC
This message is a reminder that Fedora 27 is nearing its end of life.
On 2018-Nov-30  Fedora will stop maintaining and issuing updates for
Fedora 27. It is Fedora's policy to close all bug reports from releases
that are no longer maintained. At that time this bug will be closed as
EOL if it remains open with a Fedora  'version' of '27'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 27 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

Comment 61 Nicolas Chauvet (kwizart) 2018-12-06 13:19:21 UTC
Does the comment in 1454824#c53 was addressed ?
I want to see them implemented in code, I will have a very limited time to review them all before end of the year.

Comment 62 nicolas.vieville 2018-12-06 13:39:05 UTC
Nicolas,

(In reply to Nicolas Chauvet (kwizart) from comment #61)
> Does the comment in 1454824#c53 was addressed ?

Yes. See my response 1454824#c54 for the comments about that.

> I want to see them implemented in code, 

Sure, it would be great. Sources here 

https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features

and here

https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_and_secure_boot

contains all the modifications you wanted ordered by commits, and the resolution for https://bugzilla.redhat.com/show_bug.cgi?id=1607289

For your information, the last commits from these repositories are deployed since the first day on 12 bare metal machines, and to this day are functional. They also "survived" the upgrade process from Fedora 28 to Fedora 29 without any issue.
 
> I will have a very limited time to review them all before end of the year.

If I can be of any help, let me know.

Cordially,


-- 
NVieville

Comment 63 Nicolas Chauvet (kwizart) 2019-01-25 19:05:13 UTC
About akmods review.

Merged patches up to "Use %%{name} when possible" as I think it makes the spec harder to read. (I personally disagree on using the %{name} macro in spec everywhere,)


* Improve detection of already installed (weak-)modules in akmods (RHEL)
1/ I think parsing the module availability only based on one or another directory presence is weak. If a new directory is created, then we might miss it.
Instead maybe we can use something like modinfo nvidia -k 4.19.14-200.fc28.x86_64 -n (but then we need a given kmod to test)

* Add Secure boot feature and support
2/
The akmods.cacert.config should be located in /etc/pki/akmods instead, but one needed to adapt the way to set the default values
Use something like this: http://web.mit.edu/crypto/openssl.cnf
countryName			= Country Name (2 letter code)
countryName_default		= AU
Maybe there is a need to allow it to use the default value automatically if using a non-interactive shell (and ask for appropriates values if interactif)

3/ The usual convention is to use "certs" (for certificates) and "private" (for private keys) sub-directories in the /etc/pki/'foo' instead of 'keys'.

4/ Please use akmods as the default value

5/ akmods-kmodgenca should exit early if a key (cert and private keys) already exist. (the service condition need to be checked again if run from shell).
It should not have to deal with mokutil at all (one can sign package on a system that do not have UEFI support, but later copy the resulting kmod to another system that has).

6/ macros.akmods should all be moved to kmodtool package, I think I've already told this. kmodtool should contains the low level rpm macros and logic
Also you are doing thing more complex by trying to compute thing too hard if arch support zip or signature, this is not the appropriate place for this (but instead in kmodtool-kmodtool)

Stopped reviewing this file as there are too much to say...

* Fix kernel list build when parsing command line options
7/ if [[ ! "${kernels}" ]] ; then // This is weak, please test with -n instead

* Ensure to build for grub default kernel
This looks good for now (I might backport earlier in current stable release), but will break by f30+

Comment 64 nicolas.vieville 2019-04-22 19:52:54 UTC
(In reply to Nicolas Chauvet (kwizart) from comment #63)
Hello Nicolas,

First of all, I would try to explain what I was trying to do when I 
first post message about secure boot and third-party modules.
My goal was to have a working solution to keep secure boot active on 
recent machines with fedora and "special" devices.
I never thought of reviewing and re-founding the packages akmods and 
kmodtool. I just tried adding the features needed to reach the initial 
goal. As my skill in shell scripts and rpm macros is limited, my goal
was limited. I'm not a developer. I'm fine with the direction you want 
to go, improve these tools, but please, I wonder if you might be more 
understanding and be more explicit in the way forward for the novice 
that I am.

> About akmods review.
> 
> Merged patches up to "Use %%{name} when possible" as I think it makes 
> the spec harder to read. (I personally disagree on using the %{name} 
> macro in spec everywhere,)

Defining one name globally in a macro avoids mistakes in spelling
lately, or when names are not so simple. I just applied advice from 
examples taken from:
https://rpm-packaging-guide.github.io/
https://docs.fedoraproject.org/en-US/packaging-guidelines/
https://docs.pagure.org/docs-fedora/rpm-packaging-basics.html

I can revert this if you think it's easier to read. For the moment this
was kept.

> * Improve detection of already installed (weak-)modules in akmods (RHEL)
> 1/ I think parsing the module availability only based on one or 
> another directory presence is weak. If a new directory is created, 
> then we might miss it.
> Instead maybe we can use something like 
> modinfo nvidia -k 4.19.14-200.fc28.x86_64 -n (but then we need a given
> kmod to test)

You mean modifying the design of the original file. As I explained in my
introduction, my first goal was not to review and correct the original
file, but to amend it. That's why I used the same method here to add the
weak-modules case.

I'm not opposed to change the method. The new proposed akmods file uses 
your suggestion on the module itself. This has the advantage to work for 
"native" kernel modules and for weak-modules (symlinks).

I also reworked the lines below in order to be able to deal correctly
with weak modules.

Comment about that are welcome.

> * Add Secure boot feature and support
> 2/
> The akmods.cacert.config should be located in /etc/pki/akmods instead,
> but one needed to adapt the way to set the default values
> Use something like this: http://web.mit.edu/crypto/openssl.cnf
> countryName			= Country Name (2 letter code)
> countryName_default		= AU
> Maybe there is a need to allow it to use the default value 
> automatically if using a non-interactive shell (and ask for 
> appropriates values if interactif)

All done. Please review and comments are welcome.

> 3/ The usual convention is to use "certs" (for certificates) and 
> "private" (for private keys) sub-directories in the /etc/pki/'foo' 
> instead of 'keys'.

Done.

> 4/ Please use akmods as the default value

Done.

> 5/ akmods-kmodgenca should exit early if a key (cert and private keys)
> already exist. (the service condition need to be checked again if run
> from shell).

Done.

> It should not have to deal with mokutil at all (one can sign package 
> on a system that do not have UEFI support, but later copy the 
> resulting kmod to another system that has).

This was reworked and commented inline. But to clarify the use of
mokutil, the only goal here is to help the user to identify already 
enrolled keys while backing-up them. When creating new keys, if mokutil
is present, and if a key is already enrolled, the script add a special
suffix to the backup. If mokutil is not present, no special suffix is
build, but the key is saved anyway. It's just a feature to help dealing
with keys. 
While testing this package on bare metal machine (install/uninstall), it
was easier to sort the key files to erase with this feature, as there 
was already one key enrolled.
The use of mokutil here is neutral. There is no need for mokutil to be 
installed. It just help if needed. No dependency on mokutil is needed 
for akmods package.
 
> 6/ macros.akmods should all be moved to kmodtool package, I think I've
> already told this. 

Yes in bug report #1484041.

> kmodtool should contains the low level rpm macros and logic

Done.

> Also you are doing thing more complex by trying to compute thing too 
> hard if arch support zip or signature, this is not the appropriate 
> place for this (but instead in kmodtool-kmodtool)

I've just tried to do as the kernel.spec file do. That is to say, use 
rpm macros to first verify that architecture is appropriate to deal with
secure boot (signing modules) and second to zip them.
See here in the kernel.spec file:
https://src.fedoraproject.org/rpms/kernel/blob/master/f/kernel.spec#_11

The final macros were inspired by those found in the same file (see 
here):
https://src.fedoraproject.org/rpms/kernel/blob/master/f/kernel.spec#_1565

According to this spec file, the problem is essentially about zipping.
I had to take also in account the fact that zipping is not compatible 
with RHEL < 7 and Fedora < 21. 
Simply copying the kernel.spec file macros and adding the ones to deal 
with distributions versions doesn't work on all distributions. I had no 
success with the attempt I made on centos6 and centos7 VMs using the 
same macros as kernel.spec file. As this is the first time for me to try
to write not so basic rpm macros, maybe my skill in it is too low. 

The only way that gave the right results on all distributions (RHEL en 
Fedora, in VMs or not) is the one you can see in the kmodtool.macros 
file (was akmods.macros at the time of your comment). I know it is not 
really good to merge rpm macros with shell script instructions, but I 
wasn't able to obtain the right results without them.

I remain open to any proposal working on all distributions that could 
change that.

> Stopped reviewing this file as there are too much to say...

Please do. I would be graceful if you could tell me how you want it to 
be.

> * Fix kernel list build when parsing command line options
> 7/ if [[ ! "${kernels}" ]] ; then // This is weak, please test 
> with -n instead

I've just copied it from init() function line 158. 
Corrected, the two lines use -n now.
I've also corrected every similar constructions (akmods package and
kmodtool package).

> * Ensure to build for grub default kernel
> This looks good for now (I might backport earlier in current stable 
> release), but will break by f30+

Thanks. The problem here is to implement a manner to detect the default
kernel without grubby and with Systemd Boot Loader Specification. There 
seems that grubby package for Rawhide (f31) have a new shell script 
/usr/libexec/grubby/grubby-bls to take this in account. There is also a
wrapper shell script (/usr/sbin/grubby) to call either grubby binary or 
the new shell script. Both of them recognize the --default-kernel 
commutator and provide the appropriate response. For the moment, it 
seems reasonable to me to keep the actual manner to get the default 
kernel in akmods. If grubby disappears from fedora we will see at that 
time.

As usual, the branch of my akmods pagure repository fork 
"secure_boot_key_features" has been re-based to reflect all these 
modifications.

I've also added one commit that address the rhbz #1542658 (akmods needs
to use logrotate and clean-up /var/cache/akmods directory). 
https://src.fedoraproject.org/fork/nvieville/rpms/akmods/c/d4965661dd4a0b0f201836181d17d95e15087f08?branch=secure_boot_key_features
Review about it is welcome.

The new akmods package has been tested and worked as expected on VMs 
(CentOS 6 and 7, Rawhide) and real UEFI Secure Boot HP Z440 machines 
with Fedora 29. Some more testing are probably needed. 

Cordially,


-- 
NVieville

Comment 65 dherault 2019-05-04 22:14:19 UTC
Hi, Don't hesitate if you have some updates / some tests to do. I'm waiting some advances eagerly on this project.

Thanks

Comment 66 Sergio Basto 2019-10-15 05:49:04 UTC
(In reply to dherault from comment #65)
> Hi, Don't hesitate if you have some updates / some tests to do. I'm waiting
> some advances eagerly on this project.
> 
> Thanks

I guess, you may test https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features ...

Comment 67 nicolas.vieville 2019-10-18 18:43:45 UTC
Hello,

Fork repositories about signing modules for secure boot have been updated.
They also have been rebased on their respective fedora_rhel_from_master
branches.
Review and tests are welcome.

For akmods:
https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features

Changelog from fedora_rhel_from_master branch:
- Add local akmods CA signing keys and support tools to sign modules for
  Secure boot thanks to Stanislas Leduc <stanislas.leduc>
- Add akmods-keygen service to generate MOK key pair on first run

For kmodtool:
https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_and_secure_boot

Changelog from fedora_rhel_from_master branch:
- Add signing module method to use with UEFI Secure Boot machines thanks
  to Stanislas Leduc (#1454824) (reworked method)
- Add as well zipping module method and support for it in Fedora and RHEL

Note: kmodtool version has been bumped to 1.1 in order to easy the test
process and update.

Any comment are welcome.

Cordially,


-- 
NVieville

Comment 68 Ben Cotton 2019-10-31 18:45:33 UTC
This message is a reminder that Fedora 29 is nearing its end of life.
Fedora will stop maintaining and issuing updates for Fedora 29 on 2019-11-26.
It is Fedora's policy to close all bug reports from releases that are no longer
maintained. At that time this bug will be closed as EOL if it remains open with a
Fedora 'version' of '29'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 29 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

Comment 69 Ben Cotton 2019-11-27 23:33:46 UTC
Fedora 29 changed to end-of-life (EOL) status on 2019-11-26. Fedora 29 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.

Comment 70 Mathieu Dubois 2020-01-16 11:38:05 UTC
Hi,

I'm looking for this feature for a long time (having to sign virtualbox modules after each kernel update is a pain). Any chance this land sometimes ?

While I'm at it, some keys require signing. According to [1, 2], this is achieved through the KBUILD_SIGN_PIN environment variable. Is this feature planned here ?

Thanks in advance.

  [1] https://gist.github.com/reillysiemens/ac6bea1e6c7684d62f544bd79b2182a4
  [2] https://www.kernel.org/doc/html/v4.20/admin-guide/module-signing.html#manually-signing-modules

Comment 71 Ben Cotton 2020-02-11 15:39:50 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 32 development cycle.
Changing version to 32.

Comment 72 André 2020-05-08 22:13:27 UTC
Is there a COPR to test this?

Comment 73 scroolik 2020-10-18 17:47:37 UTC
I have built packages from latest HEADs in comment #67

[root@merceditas ~]# rpm -q akmods kmodtool
akmods-0.5.7-1.fc32.noarch
kmodtool-1-37.fc32.noarch

[root@merceditas ~]# rpm -qV akmods kmodtool && echo "ok"
ok
[root@merceditas ~]# 

[root@merceditas ~]# ls -l /etc/pki/akmods/certs/ /etc/pki/akmods/private/
/etc/pki/akmods/certs/:
total 0

/etc/pki/akmods/private/:
total 0


With force option:

[root@merceditas ~]# kmodgenca -f
Update cacert.config...
Generate new keypair...
Generating a RSA private key
.....................................................++++
.............................................................................................................................................++++
writing new private key to '/etc/pki/akmods/private/merceditas-2884890530.priv'
-----
You are about to be asked to enter information that will be incorporated
into your certificate request.
What you are about to enter is what is called a Distinguished Name or a DN.
There are quite a few fields but you can leave some blank
For some fields there will be a default value,
If you enter '.', the field will be left blank.
-----
Organization Name (eg, company) [akmods local]:problems making Certificate Request
chmod: cannot access '/etc/pki/akmods/certs/merceditas-2884890530.*': No such file or directory
/usr/sbin/restorecon: lstat(/etc/pki/akmods/certs/merceditas-2884890530.der) failed: No such file or directory
[root@merceditas ~]# 

and certificate is indeed not generated and links to nonexistent cert:

[root@merceditas ~]# ls -l /etc/pki/akmods/certs/ /etc/pki/akmods/private/
/etc/pki/akmods/certs/:
total 0
lrwxrwxrwx. 1 root root 47 Oct 18 19:43 public_key.der -> /etc/pki/akmods/certs/merceditas-2884890530.der

/etc/pki/akmods/private/:
total 4
-rw-r-----. 1 root akmods 3276 Oct 18 19:43 merceditas-2884890530.priv
lrwxrwxrwx. 1 root root     50 Oct 18 19:43 private_key.priv -> /etc/pki/akmods/private/merceditas-2884890530.priv
[root@merceditas ~]# 

@nicolas.vieville, I believe this is a bug, isn't it?

Comment 74 nicolas.vieville 2020-10-19 00:30:29 UTC
Hello,

Thanks for reporting this issue.

> @nicolas.vieville, I believe this is a bug, isn't it?

Yes this is a bug and it has been addressed.

Like you already done it, you should now build packages from latest 
HEADs in comment #67. The akmods fork repositories about signing 
modules for secure boot have been updated and has also been 
rebased on his respective fedora_rhel_from_master branches. Note 
that git history has been rewritten in the process.

Tests and comments are welcome.

Cordially,


-- 
NVieville

Comment 75 Nicolas Chauvet (kwizart) 2020-10-19 07:25:31 UTC
@nicolas.vieville

Thanks for your constant work on this feature.

I think we need to go forward with this. Specially as, as always, I won't have much time to dedicate on this feature.
- Have sergio or others kmod-foo maintainers (Neal ?) to ACk on this changes.
- Have leigh to fetch the akmod/kmodtool in rpmfusion override for rawhide and verify that akmod-foo generation are done correctly (no regression).

Once we have that, we could land theses changes in rawhide (only for now) and let have in tested and corrected for a few months.

Then we will verify that the code still work on others branches (and drop the support for el6 if needed, as CentOS6 will last until Nov 2020)

Is this good enough ?

Comment 76 scroolik 2020-10-19 10:15:47 UTC
I'm wondering if akmod/kmodtool cannot use similar approach as dkms. Its /etc/dkms/framework.conf contains (by default commmented out) configuration:

## Script to sign modules during build, script is called with kernel version
## and module name
# sign_tool="/etc/dkms/sign_helper.sh"

and as an example it ships with

$ cat /usr/share/doc/dkms/sign_helper.sh
#!/bin/sh
/lib/modules/"$1"/build/scripts/sign-file sha512 /root/mok.priv /root/mok.der "$2"


This way one is not limited with implementation provided by the package and can use e.g. external signing server instead.

If an interface ($1 and $2) for external script would stay the same one could reuse the same tool either for dkms or akmod.

Comment 77 Nicolas Chauvet (kwizart) 2020-10-19 11:16:54 UTC
(In reply to scroolik from comment #76)
...
> This way one is not limited with implementation provided by the package and
> can use e.g. external signing server instead.
Which signing server do you have in mind ? (if not sigul).

The way forward is probably to use RPM macros, so we will have the appropriate interface and override it as needed.
If signing with a "local key", this will mean using a script that may have parameters, such scripts are usually located in /usr/lib/rpm/brp-*
So one could override the script and uses a custom one, indeed.

Comment 78 scroolik 2020-10-19 11:56:33 UTC
(In reply to Nicolas Chauvet (kwizart) from comment #77)
> (In reply to scroolik from comment #76)
> ...
> > This way one is not limited with implementation provided by the package and
> > can use e.g. external signing server instead.
> Which signing server do you have in mind ? (if not sigul).
I didn't have any particular module on my mind. I wanted to explore options there are to not keep private key on the machine in readable form.

Comment 79 scroolik 2020-10-19 11:57:04 UTC
(In reply to scroolik from comment #78)
> (In reply to Nicolas Chauvet (kwizart) from comment #77)
> > (In reply to scroolik from comment #76)
> > ...
> > > This way one is not limited with implementation provided by the package and
> > > can use e.g. external signing server instead.
> > Which signing server do you have in mind ? (if not sigul).
> I didn't have any particular module on my mind. I wanted to explore options
> there are to not keep private key on the machine in readable form.
s/module/server/

Comment 80 Markus Schuster 2021-01-22 12:09:46 UTC
Just some quick feedback from my side: Built RPMs from those two git repos, replaced/upgraded the already installed RPMs and followed /usr/share/doc/akmods/README.secureboot.
It *almost* worked out-of-the-box - I think the akmods package (more precisely the /usr/sbin/kmodgenca script) is missing a dependency to openssl. I'm on Fedora 33 (KDE spin) and I had no openssl installed. Does not seem to be part of a default installation in my case, so it should be a package dependency I guess.

Comment 81 nicolas.vieville 2021-01-22 18:25:15 UTC
Hello,

(In reply to Markus Schuster from comment #80)
> Just some quick feedback from my side: Built RPMs from those two git repos,
> replaced/upgraded the already installed RPMs and followed
> /usr/share/doc/akmods/README.secureboot.
> It *almost* worked out-of-the-box - I think the akmods package (more
> precisely the /usr/sbin/kmodgenca script) is missing a dependency to
> openssl. I'm on Fedora 33 (KDE spin) and I had no openssl installed. Does
> not seem to be part of a default installation in my case, so it should be a
> package dependency I guess.

You are right, openssl doesn't seem to be part of any Fedora's flavor default 
installation. Thanks for reporting this issue.

It has been addressed and like you already done it, you should now be able to 
build packages from latest HEADs in comment #67. The akmods fork repositories 
about signing modules for secure boot have been updated and its last commit 
amended in order to keep git history as short as possible.

Tests and comments are welcome, if you wish.

Cordially,


-- 
NVieville

Comment 82 Elia Geretto 2021-01-22 18:47:01 UTC
Hello,

Since I could not find any up-to-date COPR repositories with those packages, I have created my own. They rebuild automatically, so they should remain up-to-date if the branches are updated (just verified). I have tested them on Fedora 32 and 33. If someone wants to use them, feel free to do so.

https://copr.fedorainfracloud.org/coprs/egeretto/kmodtool-secureboot/
https://copr.fedorainfracloud.org/coprs/egeretto/akmods-secureboot/

Thanks a lot for implementing this! My only comment is that, when reusing an existing certificate, it would be nice if `akmods-kmodgenca` checked and maybe fixed the permissions instead of just checking for the existence of the certificate files.

Comment 84 Fedora Program Management 2021-04-29 15:53:09 UTC
This message is a reminder that Fedora 32 is nearing its end of life.
Fedora will stop maintaining and issuing updates for Fedora 32 on 2021-05-25.
It is Fedora's policy to close all bug reports from releases that are no longer
maintained. At that time this bug will be closed as EOL if it remains open with a
Fedora 'version' of '32'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 32 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

Comment 85 ricky.tigg 2021-05-07 14:05:02 UTC
Similar observation | Bug 1955108. Illustration:

# modprobe vboxdrv
modprobe: ERROR: could not insert 'vboxdrv': Key was rejected by service

Comment 86 Shawn Starr 2021-08-09 17:23:06 UTC
Could we get this merged please, I'm currently using this for.. evil green GPU driver and Secure Boot... this really is needed to simplify this for users.

Comment 87 Ben Cotton 2021-08-10 12:45:09 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 35 development cycle.
Changing version to 35.

Comment 88 Sergio Basto 2021-08-22 23:50:14 UTC
Talking with amessina today he wrote 

"My approach is the same as what's placed here https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/c/a3e95b33891e4c5afe1c9b4fd9161c34fc8b1a71?branch=enhancements_bug1484041_and_more but I don't regen keys with each module build (else folks would need to mockutil install them for every reboot) -- I bind mount them into the secure-build root from an encrypted drive."

@nvieville , I liked what I see in this commit , script to sign in others files ... , but can you clean up a little your forks , it hard to understand what is the state of art on sign kernel modules ? and the order of the branches 

I can merge https://src.fedoraproject.org/fork/nvieville/rpms/akmods/c/210ded0cea21d8703ad5901c8534136af9dae34e?branch=fedora_rhel_from_master

but I don't agree with https://src.fedoraproject.org/fork/nvieville/rpms/akmods/c/13c31a0d64170d598a6a8952ba84924f4ffd8764?branch=fedora_rhel_from_master etc 

also can you delete already merged forks ? 

Thank you

Comment 89 nicolas.vieville 2021-08-23 11:03:10 UTC
(In reply to Sergio Basto from comment #88)

Hello,

Thank you Sergio for the feedback about the uses and the discussions you had 
about signing akmods kernel modules.

> Talking with amessina today he wrote 
> 
> "My approach is the same as what's placed here
> https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/c/
> a3e95b33891e4c5afe1c9b4fd9161c34fc8b1a71?branch=enhancements_bug1484041_and_m
> ore but I don't regen keys with each module build (else folks would need to
> mockutil install them for every reboot) -- I bind mount them into the
> secure-build root from an encrypted drive."

Explanation about this point below in my last comment.

> @nvieville , I liked what I see in this commit , script to sign in others
> files ... , but can you clean up a little your forks , it hard to understand
> what is the state of art on sign kernel modules ? and the order of the
> branches 

Sorry for the multiple forks and the lack of explanation how to use them. I'll 
try to clean-up this in the next weeks. The lack of time currently does not 
allow me to do it quickly. Sorry.

But I can explain how these were build.

The key is here for akmods package (same process for kmodtool package):

https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features

As you can see the "fedora_rhel_from_master" branch is based upon the official
Fedora "master" branch. The commits in this branch (fedora_rhel_from_master) applies
some minor fixes, cleans-up and some fixes requested in various bug reports or
enhancements requests that don't modify deeply the current functionalities of akmods. 
This branch needs to be re-base upon the actual "master" Fedora branch.

Then, once the fedora_rhel_from_master re-based upon "master" Fedora branch, the 
"secure_boot_key_features" branch is based upon the fedora_rhel_from_master
branch and provides all the files and patches needed to get the signing kernel
modules for secure boot functionalities.
This branch needs to be rebase too upon the actual "fedora_rhel_from_master"
branch.

I chose this process (proceed by "stepped branches") in order to simplify the re-base 
procedure from official Fedora master branch, at least for me. Maybe I was wrong in 
this choice.

Note: the secure_boot_key_features branch of akmods package must be used with its 
corresponding branch "enhancements_and_secure_boot" from my forked repository of 
the kmodtool package (URL below).
 
> I can merge
> https://src.fedoraproject.org/fork/nvieville/rpms/akmods/c/
> 210ded0cea21d8703ad5901c8534136af9dae34e?branch=fedora_rhel_from_master
> 
> but I don't agree with
> https://src.fedoraproject.org/fork/nvieville/rpms/akmods/c/
> 13c31a0d64170d598a6a8952ba84924f4ffd8764?branch=fedora_rhel_from_master etc 


I know there is a debate around this (using %{name} macro in specs files). For my 
part, I find that this way of doing things is a source of less error, at least for
me again. This can be dropped if it does not meet the actual Fedora standards of 
writing specs files.


> also can you delete already merged forks ? 

Sure. But as I explained above (currently lacking of time - work and personal life
combined), I'll do it in the next weeks (i.e., ASAP).

> Thank you

Thank you as well for your comment and feedback about these proposals.

One last word and just for the information, I use the forked 
enhancements_and_secure_boot branch of kmodtool and the secure_boot_key_features
branch of akmods to build and use local (organization) packages repository for 
Fedora (34 currently) since 2019, and it has been working well for these 2 years, 
at least for the use we have. 
These branches no longer require the need to regen keys with each module build as 
amessina reported. Using mockutil to install the key is needed one time, at first use
of modified kmodtool and akmods.

Maybe some other uses-cases brought to us through this bug report comments (as there 
are already some) should be sources of future enhancements and fixes to get this 
functionality better and more useful?

As a reminder, here are the URLs of the two branches I currently use daily on 25 
Fedora 34 HP workstations:

https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features
https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_and_secure_boot

Any comment and question about this message, or if my explanations are not so clear, 
are welcome.

Cordially,


-- 
NVieville

Comment 90 nicolas.vieville 2021-10-22 23:01:00 UTC
Hello,

As announced in comment #89, the two forks for akmods and kmodtool have been updated.

Each one gets a "main" branch as the exact copy of the upstream one.

For akmods, they are two more branches:
- fedora_rhel_from_master - based on "main" branch and providing some minor fixes 
  and new functionalities.
- secure_boot_key_features - based on "fedora_rhel_from_master" branch and providing
  signing modules for secure boot functionalities.

For kmodtool, they are two more branches:
- fedora_rhel_from_master - based on "main" branch and providing some minor fixes 
  and new functionalities.
- enhancements_and_secure_boot - based on "fedora_rhel_from_master" branch and providing
  signing modules for secure boot functionalities.

The signing modules for secure boot functionalities are available at these URLs:

https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features
https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_and_secure_boot

They have to be used together in order to get signing modules functionalities.

Any comment or question about these updates are welcome.

Cordially,


-- 
NVieville

Comment 92 nicolas.vieville 2021-11-28 20:34:11 UTC
Hello Sergio,

(In reply to Sergio Basto from comment #91)
> and
> https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/tree/
> enhancements_bug1484041_and_more ? 
> 
> https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/
> suse_version ?

As explained in my previous message, only the branches cited in this message 
provide the signing kernel modules for secure boot feature.
So these two ones are present in my fork repositories because at one moment
they were useful for one purpose and only one:
- enhancements_bug1484041_and_more proposed a fix for #1484041 bug report, and
  tried to fix some minor glitches.
- suse_version tried to address the request to allow building kernel modules 
  and signing them for secure boot for *Suse* distributions.

These 2 branches are not needed to get signing kernel modules for secure boot
feature in akmods and kmodtool tools.


> I'm going merge the
> https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/
> enhancements_and_secure_boot in first place

Yes, this is the branch that provides this feature for kmodmool.
The secure_boot_key_features branch provides this feature for akmods.

Thank you very much for your work about this.

Cordially,


-- 
NVieville

Comment 93 Sergio Basto 2021-11-28 21:43:37 UTC
(In reply to nicolas.vieville from comment #92)
> Hello Sergio,
> 
> (In reply to Sergio Basto from comment #91)
> > and
> > https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/tree/
> > enhancements_bug1484041_and_more ? 
> > 
> > https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/
> > suse_version ?
> 
> As explained in my previous message, only the branches cited in this message 
> provide the signing kernel modules for secure boot feature.
> So these two ones are present in my fork repositories because at one moment
> they were useful for one purpose and only one:
> - enhancements_bug1484041_and_more proposed a fix for #1484041 bug report,
> and
>   tried to fix some minor glitches.
> - suse_version tried to address the request to allow building kernel modules 
>   and signing them for secure boot for *Suse* distributions.
> 
> These 2 branches are not needed to get signing kernel modules for secure boot
> feature in akmods and kmodtool tools.

but can rebase it , I already merge the other branches we can test it in my copr repo :

https://copr.fedorainfracloud.org/coprs/sergiomb/vboxfor23/builds/ 

Thank you

Comment 94 Sergio Basto 2021-11-29 12:26:21 UTC
Hi, nicolas.vieville !

Can you rebase the others branches with rawhide please ? 

I got my computer with secure boot enabled [1] and have nvidia works 

built kmodtools and akmods on rawhide 


[1]
dmesg | grep -i secur
[    0.000000] secureboot: Secure boot enabled


Comment 95 nicolas.vieville 2021-11-29 13:31:18 UTC
Hello Sergio Basto,

> Can you rebase the others branches with rawhide please ? 

As already stated in my previous messages, the other branches of my 
repository are not supposed to be used to get the signing kernel modules 
feature.
So no need to rebase them.

Your already picked-up the good ones to get signing kernel modules 
feature in Rawhide. Here there are:
For akmods:  secure_boot_key_features 
For kmodtool: enhancements_and_secure_boot

You don't need to pick-up any of the other branches listed below.
For akmods:
- rhbz-1644430
- suse_version 
For kmodtool:
- enhancements_bug1484041_and_more 
- rhbz-1703715
- suse_version

Hope my explanation are clear enough. Please don't hesitate to ask if this 
is not the case.

 
> I got my computer with secure boot enabled [1] and have nvidia works 
> 
> built kmodtools and akmods on rawhide 

Great!

Feel free to make any comments about these subjects.

Cordially,


-- 
NVieville

Comment 97 Shawn Starr 2021-11-29 15:36:45 UTC
While we're on the topic of this... 

I see these two COPRs and I'm currently using them in Fedora rawhide for signing my evil green kernel module.

egeretto/akmods-secureboot
egeretto/kmodtool-secureboot

Or does this work supersede that as the official Fedora approach?

Comment 98 Sergio Basto 2021-11-29 15:57:15 UTC
(In reply to Shawn Starr from comment #97)
> While we're on the topic of this... 
> 
> I see these two COPRs and I'm currently using them in Fedora rawhide for
> signing my evil green kernel module.
> 
> egeretto/akmods-secureboot
> egeretto/kmodtool-secureboot
> 
> Or does this work supersede that as the official Fedora approach?

the sources are equal ! 

diff . egeretto/ -rs -x .git
Files ./95-akmodsposttrans.install and egeretto/95-akmodsposttrans.install are identical
Files ./95-akmods.preset and egeretto/95-akmods.preset are identical
Files ./akmods and egeretto/akmods are identical
Files ./akmodsbuild and egeretto/akmodsbuild are identical
Files ./akmods.h2m and egeretto/akmods.h2m are identical
Files ./akmodsinit and egeretto/akmodsinit are identical
Files ./akmods-keygen@.service and egeretto/akmods-keygen@.service are identical
Files ./akmods-keygen.target and egeretto/akmods-keygen.target are identical
Files ./akmods-kmodgenca and egeretto/akmods-kmodgenca are identical
Files ./akmods.log and egeretto/akmods.log are identical
Files ./akmods-ostree-post and egeretto/akmods-ostree-post are identical
Files ./akmodsposttrans and egeretto/akmodsposttrans are identical
Only in .: akmods.rpmlintrc
Files ./akmods@.service and egeretto/akmods@.service are identical
Files ./akmods.service.in and egeretto/akmods.service.in are identical
Files ./akmods-shutdown and egeretto/akmods-shutdown are identical
Files ./akmods-shutdown.service and egeretto/akmods-shutdown.service are identical
Files ./akmods.spec and egeretto/akmods.spec are identical
Files ./cacert.config.in and egeretto/cacert.config.in are identical
Only in .: egeretto
Files ./.gitignore and egeretto/.gitignore are identical
Files ./LICENSE and egeretto/LICENSE are identical
Files ./README and egeretto/README are identical
Files ./README.secureboot and egeretto/README.secureboot are identical
Files ./sources and egeretto/sources are identical

Comment 99 Elia Geretto 2021-11-29 16:01:43 UTC
Yes yes, they are. Simply NVieville was not providing a COPR for his branches, so I just made one. Every time he pushes something on the `secure_boot_key_features` and `enhancements_and_secure_boot` branches, an automatic rebuild is triggered, so the content should be exactly the same. If you merge those, I can drop the COPR-s.

Comment 100 nicolas.vieville 2021-11-29 17:28:26 UTC
(In reply to Sergio Basto from comment #96)
> OK , but if you had fix more bugs we would like to include it ? . to test
> all together ? 
> 
> in practice what is fixed now in rawhide ?  bug #1607289 and bug #1542658
> are in rawhide now isn't it ?

Yes

> , and have you fixes for other bugs that
> wasn't merge yet to rawhide ? 

No. All the fixes were included in the branches you merged in Rawhide. I tried 
to make one commit for each fix.
 
> https://bugzilla.redhat.com/buglist.
> cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=__open__&classification=Fed
> ora&component=akmods&list_id=12287720&product=Fedora&product=Fedora%20EPEL&qu
> ery_format=advanced 
> 
> https://bugzilla.redhat.com/buglist.
> cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=__open__&classification=Fed
> ora&component=kmodtool&list_id=12287723&product=Fedora&product=Fedora%20EPEL&
> query_format=advanced

Some of these bug reports were the place of discussions where it was debated to 
include or not the fixes or the new features in the Fedora repositories. 

bug #1761361 has already a commit:
https://src.fedoraproject.org/rpms/akmods/c/dad8a0461af884ef07de7d569d784827812b48ad?branch=master
But last comment seems to say it is not fixed.

bug #1780853 has one solution in a Rawhide provided in my repository, but I still 
don't know if it was the good one regarding the comments on the bug report.

bug #1729460 has some similarities with bug #1729911. But, as you can see in the
discussion, tuning akmods and kmodtool to help building modules for OpenSuse was
not a priority. That's why there is one branch on my repository as an attempt to
bring this functionality, but it wasn't updated since it's last commit. I'm not 
sure if this is a wanted feature.

bug #1484041 is in Rawhide now.

For all the other bugs, if they provides some patches, I don't think they are in
Rawhide now. But I don't really know if they have the same goal of bringing 
signing kernel modules feature that my branches have.

Hope this will help to sort out what is in Rawhide and what is not.

Maybe it should be let to users some delay to test the new features and see if
other bugs are fixed.

For my part, I use the exact same code as Rawhide since few years now on nearly
60 bare metal machines without any problem. All of them are motorized by the 
"last" Fedora release (since 26 to 35 now). Some use signed for secure boot 
NVidia drivers, others HP-Flash tools for UEFI including the HP kernel module.
Each software update or system upgrade didn't caused any problem.

Any comments are welcome.

Cordially,


-- 
NVieville

Comment 101 Sergio Basto 2021-12-02 01:32:17 UTC
Hi , thank you for your last report , I read it carefully .
In my point of view (and Kwizart I guess) since so many code enter in rawhide , now we can / should add all pending commits into rawhide , because we need test it all in all aspects, on el8 and el7 with other architectures, with weak-modules etc etc . 

The commits of branch of "help building modules for OpenSuse" can be added , if you please rebase it with master. 

I will check the others bug reports later .

Best regards,

Comment 102 nicolas.vieville 2021-12-02 21:31:30 UTC
Hello Sergio,

New branches were created to help building kernel modules for OpenSuse in the forks 
of akmods and kmodtool packages.
They are named "take_care_of_suse".
They are available here:

https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/take_care_of_suse
https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/take_care_of_suse

The old branches "suse_version" were removed because it was impossible to rebase 
them against main upstream branches.

In order to debate if this feature should land in these tools, maybe it would be
useful to re-open one of the bug reports about it: either bug #1729460 or 
bug #1729911.

Just for the record, there still one pending bug that is a real problem for akmods: 
bug #2020889. I will try to propose a solution in the next days by adding in akmods
spec file the necessary to set a valid selinux context to /var/cache/akmods 
directory. 
For the moment, a quick and dirty fix was:

semanage fcontext -a -t var_log_t '/var/cache/akmods/akmods.log

But it won't survive to any relabeling (while upgrading Fedora from 34 to 35 for 
example). 
And maybe the context should be different (logrotate_t for example) ? Any hints 
on this are welcome.

I have to dig in the documentation before any proposal or if someone has already
one example to share, I will really appreciate it.

Feel free to make any comment.

Cordially,


-- 
NVieville

Comment 103 nicolas.vieville 2021-12-03 22:52:01 UTC
Hello,

As explained in #102, first attempt to fix bug #2020889 to allow logrotate to 
work correctly on /var/cache/akmods directory in SElinux context.
Must be reviewed, as my SElinux skills are not that smooth.

New fix_selinux_for_logrotate_rhbz-2020889 branch available here:

https://src.fedoraproject.org/fork/nvieville/rpms/akmods/tree/fix_selinux_for_logrotate_rhbz-2020889

Should this post be added to bug #2020889?
Any comments are welcome.

Cordially,


-- 
NVieville

Comment 104 Sergio Basto 2021-12-04 00:35:19 UTC
ok , as I predict this will go me give a lot of work where I need to stay focus ...
I already detect a problem in centos 8 stream [1] seems I see two errors 

the name should be kmod-VirtualBox-`uname -r`-%{VERSION}-%{RELEASE}.%{ARCH} 
it misses ARCH and in kmod-VirtualBox-4.18.0-348.2.1 it missesthe minor version 

So my plane is 
1- https://src.fedoraproject.org/rpms/akmods/pull-request/5 

2- drop kernel-abi-whitelists  bug #1761361, the name has changed to kernel-abi-stablelists

3- merge take_care_of_suse , and fix template versions ... 

4- about selinux haven't good skills on it , but I also ill check if log rotates works well

and yes of course post it in bug #2020889 please 

[1] 
with old kmodtool           
kmod-VirtualBox-4.18.0-338.el8.x86_64-6.1.30-1.el8.x86_64
kmod-VirtualBox-4.18.0-348.2.1.el8_5.x86_64-6.1.30-1.el8.x86_64
kmod-VirtualBox-4.18.0-348.el8.x86_64-6.1.30-1.el8.x86_64

with new kmodtool 

kmod-VirtualBox-4.18.0-338.el8       -6.1.30-1.el8.x86_64
kmod-VirtualBox-4.18.0-348.    el8_5       -6.1.30-1.el8.x86_64
kmod-VirtualBox-4.18.0-348.el8      -6.1.30-1.el8.x86_64

Comment 105 Sergio Basto 2021-12-04 01:06:46 UTC
I also want drop weak-modules , for example kmod-kvdo on centos 8 stream (c8s) [1] works with weak-modules just is built one time for release , on installation of a new kernel  
weak modules are automatically installed  we don't need akmods [2] 


[1]
https://git.centos.org/rpms/kmod-kvdo/blob/c8s/f/SPECS/kmod-kvdo.spec


[2]
rpm -q  kmod-kvdo -l
/etc/depmod.d/kvdo.conf
/lib/modules/4.18.0-348.el8.x86_64
/lib/modules/4.18.0-348.el8.x86_64/extra
/lib/modules/4.18.0-348.el8.x86_64/extra/kmod-kvdo
/lib/modules/4.18.0-348.el8.x86_64/extra/kmod-kvdo/uds
/lib/modules/4.18.0-348.el8.x86_64/extra/kmod-kvdo/uds/uds.ko
/lib/modules/4.18.0-348.el8.x86_64/extra/kmod-kvdo/vdo
/lib/modules/4.18.0-348.el8.x86_64/extra/kmod-kvdo/vdo/kvdo.ko
/usr/share/doc/kmod-kvdo/greylist.txt

cat /etc/depmod.d/kvdo.conf 
override uds 4.18.0-348.el8.* weak-updates/kmod-kvdo/uds
override kvdo 4.18.0-348.el8.* weak-updates/kmod-kvdo/vdo


ls -l /usr/lib/modules/*/weak-updates/kmod-kvdo/* 
/usr/lib/modules/4.18.0-338.el8.x86_64/weak-updates/kmod-kvdo/uds:
lrwxrwxrwx 1 root root 61 Dec  4 00:53 uds.ko -> /lib/modules/4.18.0-348.el8.x86_64/extra/kmod-kvdo/uds/uds.ko

/usr/lib/modules/4.18.0-338.el8.x86_64/weak-updates/kmod-kvdo/vdo:
lrwxrwxrwx 1 root root 62 Dec  4 00:53 kvdo.ko -> /lib/modules/4.18.0-348.el8.x86_64/extra/kmod-kvdo/vdo/kvdo.ko

/usr/lib/modules/4.18.0-348.2.1.el8_5.x86_64/weak-updates/kmod-kvdo/uds:
lrwxrwxrwx 1 root root 61 Dec  4 00:51 uds.ko -> /lib/modules/4.18.0-348.el8.x86_64/extra/kmod-kvdo/uds/uds.ko

/usr/lib/modules/4.18.0-348.2.1.el8_5.x86_64/weak-updates/kmod-kvdo/vdo:
lrwxrwxrwx 1 root root 62 Dec  4 00:51 kvdo.ko -> /lib/modules/4.18.0-348.el8.x86_64/extra/kmod-kvdo/vdo/kvdo.ko

Comment 106 nicolas.vieville 2021-12-06 15:06:01 UTC
(In reply to Sergio Basto from comment #104)
Hello Sergio,

Thanks for your response.

> ok , as I predict this will go me give a lot of work where I need to stay
> focus ...
> I already detect a problem in centos 8 stream [1] seems I see two errors 
> 
> the name should be kmod-VirtualBox-`uname -r`-%{VERSION}-%{RELEASE}.%{ARCH} 
> it misses ARCH and in kmod-VirtualBox-4.18.0-348.2.1 it missesthe minor
> version 

Yes, minor release version and "arch" are missing. This seems to be the way 
things are done for rhel/centos distributions.
I took the examples from:

https://github.com/elrepo/templates/blob/master/el8/template-kmod.spec

and from others kmod packages from the same repository:

https://github.com/elrepo/packages

The reasons why I choose to not proceed as it is done for Fedora are explained 
at the end of comment #19 in the bug report #1484041:

https://bugzilla.redhat.com/show_bug.cgi?id=1484041#c19

Note: it was also the only way to get drivers and weak-modules built/available
in all use-cases (installing a new kernel, installing a kernel with minor 
version bump, upgrading kmod-driver package, etc. ). All the tests were done 
at this time with centos 6 and centos 7. Rhel8 and centos 8 were not released
yet.
Each time, the release minor numbers or the "arch" were introduced in the 
kernel's strings, either the kernel module or it's weak-module link were
missing.
Maybe things have changed since then and new tests have to be done.

You can find the interesting lines building the kernel's strings in 
kmodtool-kmodtool file here:

https://src.fedoraproject.org/rpms/kmodtool/blob/main/f/kmodtool-kmodtool#_141

And the built of all the strings line 70 of the same file:

https://src.fedoraproject.org/rpms/kmodtool/blob/main/f/kmodtool-kmodtool#_70

Sorry, but I didn't find in the Redhat documentation about third party 
weak-modules the explicit way of doing things. I had to extrapolate from
experimentation. Maybe it was the wrong way.

> So my plane is 
> 1- https://src.fedoraproject.org/rpms/akmods/pull-request/5 
> 
> 2- drop kernel-abi-whitelists  bug #1761361, the name has changed to
> kernel-abi-stablelists

You mean replace kernel-abi-whitelists with kernel-abi-stablelists?

> 3- merge take_care_of_suse , and fix template versions ... 
> 
> 4- about selinux haven't good skills on it , but I also ill check if log
> rotates works well
> 
> and yes of course post it in bug #2020889 please 

Done.

As usual, any comments are welcome.

Cordially,


-- 
NVieville

Comment 107 nicolas.vieville 2021-12-06 15:13:06 UTC
(In reply to Sergio Basto from comment #105)
> I also want drop weak-modules , for example kmod-kvdo on centos 8 stream
> (c8s) [1] works with weak-modules just is built one time for release , on
> installation of a new kernel  
> weak modules are automatically installed  we don't need akmods [2] 
> 
> 
> [1]
> https://git.centos.org/rpms/kmod-kvdo/blob/c8s/f/SPECS/kmod-kvdo.spec

In this example, there seems that weak-modules are still built during the 
posttrans scriptlet (line 92):

https://git.centos.org/rpms/kmod-kvdo/blob/c8s/f/SPECS/kmod-kvdo.spec#_92

So, maybe it should be good to keep the same things in akmods/kmodtool 
scripts?

Cordially,


-- 
NVieville

Comment 108 nicolas.vieville 2021-12-07 19:01:23 UTC
Hello,

Here are two new branches in the forked repositories to take care of the switch
from kernel-abi-whitelists to kernel-abi-stablelists between rhel6/7 and rhel8.
They are only propositions.

https://src.fedoraproject.org/fork/nvieville/rpms/akmods/tree/fix_rhel8_kernel-abi-stablelists
https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/tree/fix_rhel8_kernel-abi-stablelists

Specs files changelogs sections were not modified.

Cordially,


-- 
NVieville

Comment 109 Sergio Basto 2021-12-07 20:23:41 UTC
(In reply to nicolas.vieville from comment #108)
> Hello,
> 
> Here are two new branches in the forked repositories to take care of the
> switch
> from kernel-abi-whitelists to kernel-abi-stablelists between rhel6/7 and
> rhel8.
> They are only propositions.
> 
> https://src.fedoraproject.org/fork/nvieville/rpms/akmods/tree/
> fix_rhel8_kernel-abi-stablelists
> https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/tree/
> fix_rhel8_kernel-abi-stablelists
> 
> Specs files changelogs sections were not modified.

I want remove that dependency , I think we don't use any kernel-abi-stablelist or please tell me where  kernel-abi-stablelist or white list is used ?

Comment 110 nicolas.vieville 2021-12-07 21:31:31 UTC
Hello Sergio,

This special interest group has maybe some interesting things for centos stream:

https://wiki.centos.org/SpecialInterestGroup/Kmods

Their pagure repository is available here:

https://pagure.io/group/centos-sig-kmods

It seems to own some kmod specs files that requires kernel-abi-stablelist to be 
built, for example this one at line 40:

https://pagure.io/centos-sig-kmods/kmod-exfat/blob/c8s-sig-kmods/f/SPECS/kmod-exfat.spec

It's also interesting to note that it also needs weak-modules script at line 55 and 56.

I'm not sure if this repo is of any interest or relevant for the goal pursued here, but
it contains some good examples that should probably be of any help.
This probably requires some investigation and long readings.

Cordially,


-- 
NVieville

Comment 111 Sergio Basto 2021-12-08 18:13:04 UTC
nicolas.vieville , lets close this ticket and talk over chat and or send pull request over pagure.io

Comment 112 Nicolas Chauvet (kwizart) 2021-12-08 20:05:49 UTC
(In reply to Sergio Basto from comment #111)
> nicolas.vieville , lets close this ticket and talk over chat and or send
> pull request over pagure.io

Please be kind enough to keep discussion in public.
Also it's best to use pull request on src.fedoraproject.org rather than pointing branches from bugzilla.

CentOS kmod Sig are using an outdated fork of our kmodtool framework. So one can fetch changes from it but it's not the way forward for everything.

Comment 113 nicolas.vieville 2021-12-09 10:34:04 UTC
Hello Nicolas,

(In reply to Nicolas Chauvet (kwizart) from comment #112)
> (In reply to Sergio Basto from comment #111)
> > nicolas.vieville , lets close this ticket and talk over chat and or send
> > pull request over pagure.io
> 
> Please be kind enough to keep discussion in public.
> Also it's best to use pull request on src.fedoraproject.org rather than
> pointing branches from bugzilla.

Understood. Done.

> CentOS kmod Sig are using an outdated fork of our kmodtool framework. So one
> can fetch changes from it but it's not the way forward for everything.

Thanks for the advice. For now on, we'll read and fetch carefully.

Cordially,


-- 
NVieville

Comment 114 Sergio Basto 2021-12-09 11:31:08 UTC
Kwizart of course we will keep discussion in public , just in better place