Added minimal CI + Signing section #503

iTrooz · 2022-07-27T19:26:32Z

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

maharmstone · 2022-07-27T22:20:13Z

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.

iTrooz · 2022-07-29T12:20:16Z

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

iTrooz · 2022-07-29T12:41:30Z

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 · 2022-07-29T13:06:51Z

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

iTrooz · 2022-08-03T10:51:02Z

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)

denisoster · 2022-08-03T19:31:18Z

Please add ci badge on top README 😉

PS: Windows 11 Pro last update, driver works fine

iTrooz · 2022-08-03T19:53:09Z

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

iTrooz · 2022-08-03T20:10:03Z

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

andrewc12 · 2022-08-04T10:24:57Z

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

maharmstone · 2022-08-04T17:12:57Z

Thanks @andrewc12, looks like it could be useful.

iTrooz · 2022-08-04T18:10:33Z

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

iTrooz · 2022-08-04T22:32:40Z

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

andrewc12 · 2022-08-04T22:51:12Z

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

iTrooz · 2022-08-04T22:54:17Z

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

andrewc12 · 2022-08-05T02:12:21Z

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
MicrosoftDocs/windows-powershell-docs#266

iTrooz · 2022-08-05T13:42:53Z

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

andrewc12 · 2022-08-05T14:02:03Z

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

iTrooz · 2022-08-05T14:07:10Z

Ohhhhh, right ! Didn't think of that

iTrooz · 2022-08-05T14:09:20Z

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

andrewc12 · 2022-08-05T15:10:54Z

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.

iTrooz · 2022-08-05T15:18:07Z

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 · 2022-08-05T15:19:45Z

(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)

andrewc12 · 2022-08-05T23:30:48Z

Nice

iTrooz · 2022-08-05T23:52:11Z

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 ?

andrewc12 · 2022-08-05T23:58:19Z

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

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

iTrooz · 2022-08-06T00:01:31Z

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

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 · 2022-08-06T00:05:35Z

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

andrewc12 · 2022-08-06T00:14:24Z

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:\ ?

iTrooz · 2022-08-06T00:29:50Z

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

andrewc12 · 2022-08-06T00:37:05Z

This is kinda funny to me because you're like where I was a week ago for workflows.
And a month ago for test signing.

I kept messing these things up all the time.
Heck the only reason I didn't make this exact mistake is I because I grabbed the drive from the output of another command. 😂

iTrooz · 2022-08-06T00:39:09Z

Lol

Is your workflow working now ?

iTrooz · 2022-08-06T00:41:23Z

By the way, some tests fail (400 out of 2500), which is weird, because, well... They shouldn't ?
I think it's because test.exe tests for everything possible with a filesystem, even things not supported by the driver right now

EDIT : some tests fails even on C:/, really weird

andrewc12 · 2022-08-06T01:24:11Z

Lol

Is your workflow working now ?

Yeah it is.
I even did a neat little thing where I broke out the tests into separate steps.
But then I had to figure out how to run steps even if the previous step failed.

Which I did by creating a dummy test step at the top of the tests that would only run if everything went well up untill that point
And then only running the tests if that step passed.

It's pretty good right now, but I need to change it so it throws an error if the successful tests is less than the total tests
At the moment it only throws an error if there's an error.

Right now my thoughts are to use PowerShell select-string to grab the n and m from "passed n/m"
And then see if n is < m and if it is throw the error

iTrooz · 2022-08-06T12:14:03Z

Which I did by creating a dummy test step at the top of the tests that would only run if everything went well up untill that point
And then only running the tests if that step passed.

Could you link to this ? I'm not sure I understand

Right now my thoughts are to use PowerShell select-string to grab the n and m from "passed n/m"
And then see if n is < m and if it is throw the error

Why not make the process return a non-zero exit code ?

andrewc12 · 2022-08-06T12:37:46Z

Could you link to this ? I'm not sure I understand

https://github.com/andrewc12/openzfs/blob/andrew_workflows-11_remainder/.github/workflows/wf.yaml#L120-L228
The dummy step and then all the steps below it

Why not make the process return a non-zero exit code ?

That's what I meant by throwing an error

iTrooz · 2022-08-13T18:15:48Z

Is anything missing for this to get merged ? @maharmstone

maharmstone · 2022-08-15T17:02:00Z

Is anything missing for this to get merged ? @maharmstone

Yes - the time I'll need to have a look at this. I'm not going to merge anything I don't understand myself, and at the moment my knowledge of both CI/CD and GitHub actions is limited.

I think it'd likely be a lot quicker if the CI process used Clang in Linux, rather than spinning up a Windows VM and installing MSVC... but I still need to see if Clang works for this, and if so how good it would be. GCC's a non-starter unfortunately, as it doesn't support either SEH or PDBs (though I'm working on that).

Also, as far as I can see, when it comes to Windows VMs GitHub only lets you use Windows 10, which isn't all that useful. I'd prefer something that lets you run tests on umpteen different versions of Windows, such as Wine has. It looks like this is possible with GitHub actions, but I'd have to set up and host the VMs etc. myself.

iTrooz · 2022-08-15T18:55:03Z

Oh sorry, take the time you need

think it'd likely be a lot quicker if the CI process used Clang in Linux, rather than spinning up a Windows VM and installing MSVC

Note that CI time is free for github repositories (the 2000 minutes per account thing is for private repositories), so while I understand your concerns about speed, I don't think it's this much of a problem

(On the other hand, I recently realised that the pbd file size might be a problem because you only have so much storage. You may want to diminish the storage period for artifacts, or move the pdb generation to a separate CI that runs on demand with a workflow_dispatch call)

It looks like this is possible with GitHub actions, but I'd have to set up and host the VMs etc. myself.

If you are talking about the self hosted runners, yes (or I think you could use another provider to get the W11/etc.. runners, and make github use them, no experience here)

.github/workflows/build.yml

README.md

iTrooz · 2022-10-05T20:44:24Z

Thanks ! I had to apply all of these manually because the github UI wouldn't let me automatically add your suggestions, so I hope I didn't miss anything

iTrooz · 2022-10-11T09:23:49Z

Nope, I ran it directly on my PC with a btrfs-formatted USB stick I'm not even sure where to begin to mount a btrfs volume on the github runner x) ------- Original Message -------

…

On Saturday, August 6th, 2022 at 02:14, Andrew Innes ***@***.***> wrote: 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 — Reply to this email directly, [view it on GitHub](#503 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AKFROC25ZJNIZMNQ45RTJKTVXWU6XANCNFSM542XZHWQ). You are receiving this because you authored the thread.Message ID: ***@***.***>

ZeKap

Seems nice to me! When will it be merged?

iTrooz · 2023-03-03T14:26:34Z

@maharmstone Hey, any news on this ?

brian6932 · 2023-11-04T08:27:17Z

.github/workflows/build.yml

+        mkdir upload
+        mkdir upload/${{ matrix.win_arch }}
+
+        copy src/btrfs.inf upload
+        copy build/ubtrfs.dll upload/${{ matrix.win_arch }}
+        copy build/shellbtrfs.dll upload/${{ matrix.win_arch }}
+        copy build/mkbtrfs.exe upload/${{ matrix.win_arch }}
+        copy build/btrfs.sys upload/${{ matrix.win_arch }}
+
+        mkdir upload-pdb
+        mkdir upload-pdb/${{ matrix.win_arch }}
+
+        copy build/ubtrfs.pdb upload-pdb/${{ matrix.win_arch }}
+        copy build/shellbtrfs.pdb upload-pdb/${{ matrix.win_arch }}
+        copy build/mkbtrfs.pdb upload-pdb/${{ matrix.win_arch }}
+        copy build/btrfs.pdb upload-pdb/${{ matrix.win_arch }}


Suggested change

mkdir upload

mkdir upload/${{ matrix.win_arch }}

copy src/btrfs.inf upload

copy build/ubtrfs.dll upload/${{ matrix.win_arch }}

copy build/shellbtrfs.dll upload/${{ matrix.win_arch }}

copy build/mkbtrfs.exe upload/${{ matrix.win_arch }}

copy build/btrfs.sys upload/${{ matrix.win_arch }}

mkdir upload-pdb

mkdir upload-pdb/${{ matrix.win_arch }}

copy build/ubtrfs.pdb upload-pdb/${{ matrix.win_arch }}

copy build/shellbtrfs.pdb upload-pdb/${{ matrix.win_arch }}

copy build/mkbtrfs.pdb upload-pdb/${{ matrix.win_arch }}

copy build/btrfs.pdb upload-pdb/${{ matrix.win_arch }}

'', '/${{ matrix.win_arch }}', '-pdb', '-pdb/${{ matrix.win_arch }}' | ForEach-Object { New-Item -ItemType Directory "upload$_" }

'', '-vol' | ForEach-Object { Copy-Item "src/btrfs$_.inf" upload }

'ubtrfs.dll', 'shellbtrfs.dll', 'mkbtrfs.exe', 'btrfs.sys' | ForEach-Object {

Copy-Item "build/$_" upload/${{ matrix.win_arch }}

Copy-Item "build/$($_.Substring(0, $_.Length - 3))pdb" upload-pdb/${{ matrix.win_arch }}

}

Should be PowerShell (mkdir is not PowerShell), and should copy btrfs-vol.inf as well

If your interest in this is because you are planning to run your own fork with different filename limits, do NOT do this. You will corrupt your filesystem when you use it on Linux, as even with the changes on that Russian site the kernel driver will overwrite other data with the long filename.

Nah that's not my sole reason, I did try that. It didn't corrupt anything, but I did get a warning on Linux when running btrfs commands, renaming such paths removes any warnings, and just works.
My goal wasn't really to run a fork (branch is deleted now), but just to see if the CI worked, it didn't work well enough for me, so I learned how to do the signing manually.

iTrooz force-pushed the master branch from 20dddbe to 46c890b Compare July 29, 2022 12:18

iTrooz changed the title ~~Added minimal CI~~ Added minimal CI + Signing section Jul 29, 2022

iTrooz force-pushed the master branch from 5d19f66 to f484711 Compare July 29, 2022 13:05

iTrooz force-pushed the master branch 3 times, most recently from 8f09ecb to 76ec7e8 Compare August 3, 2022 20:09

iTrooz added 2 commits August 5, 2022 22:20

added .gitignore

321ca2b

added notice about Ninja generator

b5e2683

iTrooz added 2 commits August 5, 2022 23:23

Refactored arch names

e179119

TEST : Sign driver in CI

ba03aab

iTrooz force-pushed the master branch from 76ec7e8 to ba03aab Compare August 5, 2022 23:11

Saibamen suggested changes Sep 28, 2022

View reviewed changes

iTrooz added 2 commits October 5, 2022 22:45

Linting corrections from Saibamen

6aa2376

use github native badge

d534ee9

iTrooz force-pushed the master branch from 28bec4f to d534ee9 Compare October 5, 2022 20:45

Saibamen approved these changes Oct 5, 2022

View reviewed changes

ZeKap approved these changes Jan 5, 2023

View reviewed changes

Merge branch 'master' into master

d96a877

brian6932 suggested changes Nov 4, 2023

View reviewed changes

Added minimal CI + Signing section #503

Are you sure you want to change the base?

Added minimal CI + Signing section #503

Conversation

iTrooz commented Jul 27, 2022

maharmstone commented Jul 27, 2022

iTrooz commented Jul 29, 2022

iTrooz commented Jul 29, 2022 • edited Loading

iTrooz commented Jul 29, 2022

iTrooz commented Aug 3, 2022 • edited Loading

denisoster commented Aug 3, 2022

iTrooz commented Aug 3, 2022

iTrooz commented Aug 3, 2022

andrewc12 commented Aug 4, 2022

maharmstone commented Aug 4, 2022

iTrooz commented Aug 4, 2022

iTrooz commented Aug 4, 2022

andrewc12 commented Aug 4, 2022 • edited Loading

iTrooz commented Aug 4, 2022

andrewc12 commented Aug 5, 2022

iTrooz commented Aug 5, 2022

andrewc12 commented Aug 5, 2022 • edited Loading

iTrooz commented Aug 5, 2022

iTrooz commented Aug 5, 2022 • edited Loading

andrewc12 commented Aug 5, 2022

iTrooz commented Aug 5, 2022

iTrooz commented Aug 5, 2022

andrewc12 commented Aug 5, 2022

iTrooz commented Aug 5, 2022

andrewc12 commented Aug 5, 2022

iTrooz commented Aug 6, 2022 • edited Loading

iTrooz commented Aug 6, 2022

andrewc12 commented Aug 6, 2022 • edited Loading

iTrooz commented Aug 6, 2022 • edited Loading

andrewc12 commented Aug 6, 2022

iTrooz commented Aug 6, 2022

iTrooz commented Aug 6, 2022 • edited Loading

andrewc12 commented Aug 6, 2022

iTrooz commented Aug 6, 2022 • edited Loading

andrewc12 commented Aug 6, 2022

iTrooz commented Aug 13, 2022

maharmstone commented Aug 15, 2022

iTrooz commented Aug 15, 2022

iTrooz commented Oct 5, 2022

iTrooz commented Oct 11, 2022 via email

ZeKap left a comment

Choose a reason for hiding this comment

iTrooz commented Mar 3, 2023

brian6932 Nov 4, 2023 • edited Loading

Choose a reason for hiding this comment

maharmstone Nov 4, 2023

Choose a reason for hiding this comment

brian6932 Nov 4, 2023 • edited Loading

Choose a reason for hiding this comment

iTrooz commented Jul 29, 2022 •

edited

Loading

iTrooz commented Aug 3, 2022 •

edited

Loading

andrewc12 commented Aug 4, 2022 •

edited

Loading

andrewc12 commented Aug 5, 2022 •

edited

Loading

iTrooz commented Aug 5, 2022 •

edited

Loading

iTrooz commented Aug 6, 2022 •

edited

Loading

andrewc12 commented Aug 6, 2022 •

edited

Loading

iTrooz commented Aug 6, 2022 •

edited

Loading

iTrooz commented Aug 6, 2022 •

edited

Loading

iTrooz commented Aug 6, 2022 •

edited

Loading

brian6932 Nov 4, 2023 •

edited

Loading

brian6932 Nov 4, 2023 •

edited

Loading