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 "--version" option to the usbguard CLI #576

Closed Cropi closed 1 month ago

Cropi commented 1 year ago

Running usbguard --version should help us handle fixes and issues more easily in the future. By doing so, it's easier to track down which options have been enabled during build time. Fixes #570

Any suggestions on what else to display are welcomed.

Cropi commented 1 year ago

Hi Sebastian,

Hi @Cropi,

I had a quick look through the changes and also inspected the output locally:

# ./usbguard --version
usbguard 1.1.2 compiled with:
        Platform:                       x86_64-pc-linux-gnu
        Platform (lsb_release -d)       Description:    Gentoo Linux
        Linux audit support:            1
        Libcapng support:               1
        Seccomp support:                1
        Crypto backend library:         libsodium

Based on what I saw I would like to feedback the following:

* I like the idea of including some additional information to ease reporting.
  At least we can ask for that output from users running a recent enough version.

* Currently, all of that information included is gathered at _compile_ time.
  For two of those, that could be trouble:

  * "Platform"
  * "Platform (lsb_release -d)"

  I suggest to drop these two because of risk of being out of sync with the running
  machine and because gathering them live from C++ code is probably more code
  and effort than healthy here. Also, theyes can be asked for rather easily.

I am fine either way. I saw these in another open source project, rsyslog , that's why I thought we could give it a try.

* The `Description:    ` part from output of `lsb_release -d` looks out of place and unintended.

* The new code roughly follows pattern `if foo_enabled: print("foo: 1")`.
  The problem here is that:

  * Absence is not communicated, there will be no line `foo: 0` for absent features. That's rather unpractical.
  * A constant value of 1 does not seem to offer much value.
    I would like to suggest adding `#else` branches and to go to human `enabled`
    and `disabled` or similar.

I agree.

* The use of tabs in the output seems well intended but does not feel "organic".
  I suggest use of only two spaces rather than tabs for a more "human" feel.

I agree.

* Adding two new files for the new functions seems overkill, given that the new code could live right next to sibling function `showTopLevelHelp` in file `src/CLI/usbguard.cpp`.

* Bash completion at `scripts/bash_completion/usbguard` should probably learn about `--version` as well.

Seems good to me.

What do you think?

Best, Sebastian

I wonder if there is anything else we could include in the output. Maybe if you build usbguard from sources, then the last commit ID could be mentioned as well.

hartwork commented 1 year ago

I wonder if there is anything else we could include in the output. Maybe if you build usbguard from sources, then the last commit ID could be mentioned as well.

@Cropi sounds good! It's a classic way to break the build for release tarballs without a .git folder around but if done properly... :smiley: