btrfs icon indicating copy to clipboard operation
btrfs copied to clipboard

Added minimal CI + Signing section

Open iTrooz opened this issue 3 years ago • 42 comments

Hey, following what I said in #499, I made a minimal CI setup you can check https://github.com/iTrooz/btrfs/actions/runs/2748809550 for an example run

Unfortunately, I couldn't test the binaries because I didn't understand how to sign the driver

A few questions :

Do you want the CI to automatically sign the driver ? From what I understand, drivers have to be signed by a Microsoft-trusted CA, so I guess you have a key with such trust. It could be added as a secret in the CI (See Settings -> Secrets -> Actions ) so the CI can use the key to sign the driver while keeping it secure. This would allow people to test nightly versions of the driver (to test if their issue was resolved for example) without going through the hassle of signing it themselves

In case you do not want to add the key to the CI :

  • Could you add a "Signing" section after "Compilation", so that people know how to sign the driver for test purposes ?
  • Do you think it's still useful for the CI to compile/output artifacts for all targeted architectures ? I guess just compiling for x64 to test that it works would suffice

I understand that you did not agree for such a feature and that you may refuse it, I only worked on it because I had some free time and wanted to learn how to compile the driver

iTrooz avatar Jul 27 '22 19:07 iTrooz

Thanks for this, this looks really cool - I'll give it a proper look when I get the chance.

One thing that does stand out is that I think "Release" should be "RelWithDebInfo" - and it needs to be saving the PDB files somewhere.

As for signing, I think you can generate a dummy key that's usable in test mode. I have to use a USB dongle for my actual key unfortunately, so I can't upload that.

maharmstone avatar Jul 27 '22 22:07 maharmstone

Here we go ! Test-signing the driver is really harder than it look

iTrooz avatar Jul 29 '22 12:07 iTrooz

Okay, after thinking about it, the "test-signing" thing seems way too complicated/dangerous to do from the perspective of an user wanting to test a nightly version (disabling secure boot, putting the computer into a test mode, installing the WDK, adding a trusted CA to the computer)

Compared to just... Disabling signature checking

iTrooz avatar Jul 29 '22 12:07 iTrooz

Done ! I'd really like a review on my additions to the README

iTrooz avatar Jul 29 '22 13:07 iTrooz

I'm not familiar with the process of signing drivers, but how similar is it to something like GPG? Could it be possible to sign a certificate, and have that certificate as a secret in the Github's CI, then use that to sign the driver?

Hum... maybe ? @maharmstone, do you want me to take a look at it ? (or do you not want a key in the CI anyway)

iTrooz avatar Aug 03 '22 10:08 iTrooz

Please add ci badge on top README :wink:

PS: Windows 11 Pro last update, driver works fine

denisoster avatar Aug 03 '22 19:08 denisoster

I literally copied it from another repository, please tell me if I should change anything

iTrooz avatar Aug 03 '22 19:08 iTrooz

Okay I struggled way more than I should have, but it's done

iTrooz avatar Aug 03 '22 20:08 iTrooz

you might be interested in taking a look at what i did https://github.com/openzfsonwindows/openzfs/tree/windows/contrib/windows/TestCert

this has a script for generating certs https://github.com/openzfsonwindows/openzfs/blob/windows/contrib/windows/TestCert/create_test_sign_cert.ps1

this has some some commands for importing certificates to use for signing https://github.com/openzfsonwindows/openzfs/blob/windows/.github/workflows/windows-build-test.yml#L22

andrewc12 avatar Aug 04 '22 10:08 andrewc12

Thanks @andrewc12, looks like it could be useful.

maharmstone avatar Aug 04 '22 17:08 maharmstone

I'll take a look at it when I get time !

iTrooz avatar Aug 04 '22 18:08 iTrooz

Seems great ! Only one thing, how do you sign the driver with the certificate, in the CI ?

iTrooz avatar Aug 04 '22 22:08 iTrooz

That part I didn't work on But it looks like it's around here

https://github.com/openzfsonwindows/openzfs/blob/2f653a16dbdbe79d1caf733c8d3f1e0f943987f4/module/CMakeLists.txt#L117

I should also say alot of my inspiration came from the usbip driver documentation So you might want to look at them

This is where they sign their driver if you're interested. https://github.com/cezanne/usbip-win/blob/d9260e95b78e38e6320c7e156515f88d1669b9d6/driver/vhci/gencat.bat

andrewc12 avatar Aug 04 '22 22:08 andrewc12

Indeed ! Thanks, I'll do all that when I get the time

I'm sad all of my work on the README will get scrapped lol

iTrooz avatar Aug 04 '22 22:08 iTrooz

I just tested a script to install a cert as a root cert https://github.com/andrewc12/openzfs/blob/b3e35de590828a854cb3f4ee114106e68c519e1f/.github/workflows/wf.yaml#L27

I found the info about the cert store names here https://github.com/MicrosoftDocs/windows-powershell-docs/issues/266

andrewc12 avatar Aug 05 '22 02:08 andrewc12

Why would you want to install it as root in the CI ?

iTrooz avatar Aug 05 '22 13:08 iTrooz

I meant as a root certificate authority It's a scripted version of this step https://github.com/iTrooz/btrfs/blame/2308ca7d0f4b5ce5b3acb0d81b03aea43de5668e/README.md#L174 It's useful if you ever want to install btrfs inside the workflow for doing filesystem tests

andrewc12 avatar Aug 05 '22 14:08 andrewc12

Ohhhhh, right ! Didn't think of that

iTrooz avatar Aug 05 '22 14:08 iTrooz

After reviewing your CI, I found something that worries me The pfx file (which contains the private keys) is publicly available, so a malicious actor could use the keys to sign something else as a driver and deliver it to the users, or worse (we're talking about a certificate installed as root on the user's machine !) I think the private key of the certificate should be treated as a secret in the CI

iTrooz avatar Aug 05 '22 14:08 iTrooz

For us this is absolutely fine since it is only used on development devices. The author actually has a private cert that is used to sign public releases.

andrewc12 avatar Aug 05 '22 15:08 andrewc12

Hum... I understand your point, but this still scares me tbh I'm going the route of storing the .pfx file as a secret

iTrooz avatar Aug 05 '22 15:08 iTrooz

(If for any reason someone ever needs what I wrote in the README, which I'm going to erase at least partly : https://gist.github.com/iTrooz/da1a4fd780e3bfc0c10ec63fa55fe447)

iTrooz avatar Aug 05 '22 15:08 iTrooz

@maharmstone Just curious, what is your current workflow to test the driver ? Do you copy the artifacts from the build folder, create the directory structure, sign it, and install it, all of this manually ?

Oh, and second question : is the "install" section of CMake really used ? (=How about I modify that section to already create the directory structure needed to install the driver)

iTrooz avatar Aug 05 '22 20:08 iTrooz

Nice

andrewc12 avatar Aug 05 '22 23:08 andrewc12

Say, while rewrting the README I realized something

The certificate does not need to be installed on the user's computer to be able to install the driver All this will change is that the user will get a warning that "the certificate is not trusted", but can still install the driver

So... Is installing the certificate on the user's machine really that important ?

iTrooz avatar Aug 05 '22 23:08 iTrooz

I'm not sure about with what is happening here but with openzfsonwindows you either need to

  1. disable driver signing enforcement each time you want to install Or
  2. enable test signing Have the certificate installed Have a signed driver (signed with that signing certificate)

andrewc12 avatar Aug 05 '22 23:08 andrewc12

Interesting Here is the warning I get (Windows 11 btw) image

I guess I will tell users to still install the certificate, in case Microsoft randomly decides to remove this warning and outright not install the driver

iTrooz avatar Aug 06 '22 00:08 iTrooz

By the way @maharmstone, could you explain me how test.exe works ? I'd like to run these tests in the CI, but I can't figure out how to use it. I tried to put the path to a btrfs device as an argument (test.exe E:/) but I get the error STATUS_OBJECT_PATH_NOT_FOUND

iTrooz avatar Aug 06 '22 00:08 iTrooz

Could this be caused by the test program retuning an error and then github hiding everything else?

Maybe try running it once where it's

run: |
  test.exe E:/
  exit 0

So that it doesn't error out and you still get the test output

for example I get something similar running tests https://github.com/andrewc12/openzfs/runs/7687778508?check_suite_focus=true#step:29:5

finally I feel bad even mentioning it but what about trying E:\ ?

andrewc12 avatar Aug 06 '22 00:08 andrewc12

And I feel even worse because.... it works..

iTrooz avatar Aug 06 '22 00:08 iTrooz