Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Added minimal CI + Signing section #503

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

iTrooz
Copy link
Contributor

@iTrooz iTrooz commented Jul 27, 2022

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
Copy link
Owner

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 iTrooz changed the title Added minimal CI Added minimal CI + Signing section Jul 29, 2022
@iTrooz
Copy link
Contributor Author

iTrooz commented Jul 29, 2022

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

@iTrooz
Copy link
Contributor Author

iTrooz commented Jul 29, 2022

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
Copy link
Contributor Author

iTrooz commented Jul 29, 2022

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

@iTrooz
Copy link
Contributor Author

iTrooz commented Aug 3, 2022

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
Copy link

Please add ci badge on top README 😉

PS: Windows 11 Pro last update, driver works fine

@iTrooz
Copy link
Contributor Author

iTrooz commented Aug 3, 2022

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

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

iTrooz commented Aug 3, 2022

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

@andrewc12
Copy link

@maharmstone
Copy link
Owner

Thanks @andrewc12, looks like it could be useful.

@iTrooz
Copy link
Contributor Author

iTrooz commented Aug 4, 2022

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

@iTrooz
Copy link
Contributor Author

iTrooz commented Aug 4, 2022

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

@andrewc12
Copy link

andrewc12 commented Aug 4, 2022

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
Copy link
Contributor Author

iTrooz commented Aug 4, 2022

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
Copy link

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
Copy link
Contributor Author

iTrooz commented Aug 5, 2022

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

@andrewc12
Copy link

andrewc12 commented Aug 5, 2022

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
Copy link
Contributor Author

iTrooz commented Aug 5, 2022

Ohhhhh, right ! Didn't think of that

@iTrooz
Copy link
Contributor Author

iTrooz commented Aug 5, 2022

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
Copy link

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
Copy link
Contributor Author

iTrooz commented Aug 5, 2022

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
Copy link
Contributor Author

iTrooz commented Aug 5, 2022

(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
Copy link

Nice

@iTrooz
Copy link
Contributor Author

iTrooz commented Aug 5, 2022

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
Copy link

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)

@iTrooz
Copy link
Contributor Author

iTrooz commented Aug 6, 2022

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
Copy link
Contributor Author

iTrooz commented Aug 6, 2022

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
Copy link

andrewc12 commented Aug 6, 2022

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
Copy link
Contributor Author

iTrooz commented Aug 6, 2022

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

@andrewc12
Copy link

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
Copy link
Contributor Author

iTrooz commented Aug 6, 2022

Lol

Is your workflow working now ?

@iTrooz
Copy link
Contributor Author

iTrooz commented Aug 6, 2022

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
Copy link

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
Copy link
Contributor Author

iTrooz commented Aug 6, 2022

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
Copy link

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
Copy link
Contributor Author

iTrooz commented Aug 13, 2022

Is anything missing for this to get merged ? @maharmstone

@maharmstone
Copy link
Owner

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
Copy link
Contributor Author

iTrooz commented Aug 15, 2022

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 Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@iTrooz
Copy link
Contributor Author

iTrooz commented Oct 5, 2022

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
Copy link
Contributor Author

iTrooz commented Oct 11, 2022 via email

Copy link

@ZeKap ZeKap left a comment

Choose a reason for hiding this comment

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

Seems nice to me! When will it be merged?

@iTrooz
Copy link
Contributor Author

iTrooz commented Mar 3, 2023

@maharmstone Hey, any news on this ?

Comment on lines +47 to +62
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 }}
Copy link

@brian6932 brian6932 Nov 4, 2023

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link

@brian6932 brian6932 Nov 4, 2023

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants