search

USBGuard / usbguard

USBGuard is a software framework for implementing USB device authorization policies (what kind of USB devices are authorized) as well as method of use policies (how a USB device may interact with the system)
https://usbguard.github.io/
GNU General Public License v2.0
1.1k stars 133 forks source link

Add missing .adoc files to the tarball #561

Closed Cropi closed 1 year ago

Cropi commented 1 year ago

If one attempted to build usbguard from the tarball, some documentation related files would be left out. The patch resolves this issue.

Cropi commented 1 year ago

Hi @Cropi I see two things here: adding 3 files for tarball generation and dropping sudo at 3 places. Dropping sudo seems unrelated to the official mission of the pull request and the commit, and it is also something I would like to discuss and understand the motivation, e.g. because I need sudo to run these commands in practice over here. Regarding the build system fix, I am wondering why the error does not show up in CI (where we do cover make distcheck) and hence maybe we should extend CI to produce this error, so that we can see that this is a complete fix and that we won't have regressions. How can the issue be reproduced that's being fixed here? Thanks!

Hi @hartwork, thank you for the review. The issue can be reproduced when you download the tarball from e.g. from https://github.com/USBGuard/usbguard/releases/tag/usbguard-1.1.2. The mentioned files were not included because were not part of the make dist command. I've built usbguard from the tarball many times but no error was shown during that process even though some files were not present. It seems like asciidoc(a2x) simply ignores missing files when creating those particular man pages. You are right that dropping sudo does not have to do anything with the mission of the PR. I though it would be better to remove it. In general, you should be able to generate initial policy and change the state of any device using the usbguard CLI as long as you have enough permission specified inside the IPC access control file(s).

hartwork commented 1 year ago

Hi @hartwork, thank you for the review. The issue can be reproduced when you download the tarball from e.g. from https://github.com/USBGuard/usbguard/releases/tag/usbguard-1.1.2. The mentioned files were not included because were not part of the make dist command. I've built usbguard from the tarball many times but no error was shown during that process even though some files were not present. It seems like asciidoc(a2x) simply ignores missing files when creating those particular man pages.

@Cropi I was now able to verify the issue as following:

# cd "$(mktemp -d)"
# wget https://github.com/USBGuard/usbguard/releases/download/usbguard-1.1.2/usbguard-1.1.2.tar.gz
# tar xf usbguard-1.1.2.tar.gz
# cd usbguard-1.1.2/
# A2X=a2x ./configure --with-bundled-catch  # note A2X=a2x!
# make -j$(nproc) |& tee build.log
# grep -F 'include file not found' build.log | awk '{print $NF}' | sort -u
/tmp/tmp.vXptX2zvUW/usbguard-1.1.2/doc/man/example-allow-device.adoc
/tmp/tmp.vXptX2zvUW/usbguard-1.1.2/doc/man/example-initial-policy.adoc
/tmp/tmp.vXptX2zvUW/usbguard-1.1.2/doc/man/footer.adoc

It seems that we should add A2X=a2x to configure calls in the CI somewhere (or everywhere) so that documentation is built in CI and that we'll have protection against this issue coming back as a regression in the future.

If there is no way of making asciidoc fail on missing include files, maybe our CI should grep build logs for 'include file not found'. That would make the CI bullet-proof.

This very pull request seems like the perfect place to make that change, to me. What do you think?

You are right that dropping sudo does not have to do anything with the mission of the PR. I though it would be better to remove it. In general, you should be able to generate initial policy and change the state of any device using the usbguard CLI as long as you have enough permission specified inside the IPC access control file(s).

I would like to vote against dropping sudo, assuming that:

To summarize, I believe we would lose a feature and add new trouble by dropping sudo. Please let's keep it, okay? :pray:

Cropi commented 1 year ago

Sorry @hartwork for the late answer. It seems like that I did not receive a notification from the comment you added.

I agree that we should extend the CI to not face similar issues in the future. I have not found a way to force asciidoc to error out. However, we could check the output and search for common mistakes as you suggested, e.g. whether build logs contain "include file not found" or maybe "asciidoc: WARNING" to capture more problems.

If I take a look at github actions, I can find the mentioned warning message inside the distcheck section:

asciidoc: WARNING: usbguard-daemon.conf.5.adoc: line 197: include file not found: /home/runner/work/usbguard/usbguard/build/usbguard-1.1.2/doc/man/footer.adoc

Regarding the sudo part, I am fine with removing that part of the commit. However, in an ideal situation, you do not need root privileges to alter the devices as long as the usbguard IPC permissions are configured properly.

hartwork commented 1 year ago

Sorry @hartwork for the late answer. It seems like that I did not receive a notification from the comment you added.

I agree that we should extend the CI to not face similar issues in the future. I have not found a way to force asciidoc to error out. However, we could check the output and search for common mistakes as you suggested, e.g. whether build logs contain "include file not found" or maybe "asciidoc: WARNING" to capture more problems.

If I take a look at github actions, I can find the mentioned warning message inside the distcheck section:

asciidoc: WARNING: usbguard-daemon.conf.5.adoc: line 197: include file not found: /home/runner/work/usbguard/usbguard/build/usbguard-1.1.2/doc/man/footer.adoc

@Cropi I have made the CI do that in pull request #567 now. Your fixes to Makefile.am are included.

Regarding the sudo part, I am fine with removing that part of the commit. However, in an ideal situation, you do not need root privileges to alter the devices as long as the usbguard IPC permissions are configured properly.

I see the point in avoiding root permissions but let's not drop sudo please before there is a well-documented approach on how to set-up a working alternative, in detail. Are there tried-and-tested docs on that somewhere?

Cropi commented 1 year ago

Sorry @hartwork for the late answer. It seems like that I did not receive a notification from the comment you added. I agree that we should extend the CI to not face similar issues in the future. I have not found a way to force asciidoc to error out. However, we could check the output and search for common mistakes as you suggested, e.g. whether build logs contain "include file not found" or maybe "asciidoc: WARNING" to capture more problems. If I take a look at github actions, I can find the mentioned warning message inside the distcheck section:

asciidoc: WARNING: usbguard-daemon.conf.5.adoc: line 197: include file not found: /home/runner/work/usbguard/usbguard/build/usbguard-1.1.2/doc/man/footer.adoc

@Cropi I have made the CI do that in pull request #567 now. Your fixes to Makefile.am are included.

Okay, let's continue the discussion there.

Regarding the sudo part, I am fine with removing that part of the commit. However, in an ideal situation, you do not need root privileges to alter the devices as long as the usbguard IPC permissions are configured properly.

I see the point in avoiding root permissions but let's not drop sudo please before there is a well-documented approach on how to set-up a working alternative, in detail. Are there tried-and-tested docs on that somewhere?

The usbguard-daemon.conf man page describes it in the IPC access control section. Let me know if you think something is missing from there or it's not obvious at first sight. Thanks.

hartwork commented 1 year ago

The usbguard-daemon.conf man page describes it in the IPC access control section. Let me know if you think something is missing from there or it's not obvious at first sight. Thanks.

@Cropi interesting pointer, thanks! Based on the man page and…

sudo usbguard add-user "${USER}" --policy list,modify --devices list,listen,modify --exceptions listen --parameters list,listen,modify
sudo /etc/init.d/usbguard restart  # openrc

…I was able to elevate my unprivileged user to what I'd normally need root for with USBguard now, e.g. running usbguard list-devices without root. I'm not sure that's an approach we can and should expect from all users operating USBGuard. Maybe let me think and sleep about that more.