main: help subcommand should work without root. #31

ericonr · 2020-10-28T23:03:01Z

I'm not sure I need to check the array length, because I don't think it enters the function if there isn't any command at all. But I didn't want to risk it.

Could be changed to strings.Fields(c.CommandPath())[1] directly.

ericonr · 2020-10-28T23:03:36Z

Could also try to use another function that takes the subcommand directly, which probably exists.

ericonr · 2020-10-28T23:23:34Z

cmd/sbctl/main.go

 		if strings.Contains(c.CommandPath(), "completion zsh") ||
 			strings.Contains(c.CommandPath(), "completion bash") ||
 			strings.Contains(c.CommandPath(), "completion fish") ||
-			strings.Contains(c.CommandPath(), "__complete") {
+			strings.Contains(c.CommandPath(), "__complete") ||
+			len(fields) > 1 && fields[1] == "help" {


I wanted to use Fields to protect against the case where a user has help in some path passed to the other commands. If that isn't necessary, we should be fine using Contains directly.

cobra has c.Name(), but that gets hairy, since the completion commands are actually subcommands, so it initially returns the shell name.

I'm wondering if we should swap the logic around. Instead of PersistentPreRun we can do a PreRun on all commands that require root. This can be a fairly simple:

for _, cmd := range []interface{createKeysCmd(), enrollKeysCmd(), .....}{ cmd.Prerun = func(....iforget) { // if check } rootCmd.AddCommand(cmd) }

Add an variable for the commands or something. Wouldn't this be simpler?

It has a chance of being much cleaner. I will see if I can implement it.

You could also abstract away the for loop and have

func WithRoot(cmds ...interface{}) *cobra.Command { for _, cmd := range cmds { ... something } } func main(){ rootCmd = WithRoot( createKeysCmd(). enrollKeysCmd(), ....etc ) }

ericonr · 2020-10-29T01:29:26Z

That said, I was considering removing this check entirely, due to the possibility of adding integration tests. Doing an acess test with the databases and/or keys would allow us to check for adequate permissions without hardcoding the required UID (or getting ugly errors).

For testing (unless it's always done inside a chroot environment, which I don't love), we would require command line options for the location of the/usr/share/secureboot folder - this would also enable sbctl to be run from an alternate root.

ericonr · 2020-10-31T20:55:16Z

Ping on the idea of moving to some access test instead of checking if uid is 0.

Foxboron · 2020-10-31T20:59:16Z

Yes, a much better idea instead of lazily checking for root.

ericonr · 2020-11-07T05:57:48Z

Ok, I've kinda turned the PR on its head; feel free to point out any issues you have with my changes, since I'm not 100% sure about them either.

I still need to review the changes again, but they look somewhat sane for now.

Foxboron · 2020-11-07T15:30:16Z

I need to pull and test this, but I think this is a better approach then gateing the command at the top-level. Allows for more fine-grained permission handling as well!

Thanks for the work :)

ericonr · 2020-11-07T21:03:03Z

Happy to help :)

If you don't think the PR will get too bloated, I could start adding stuff for the exit error codes as well. Though that would likely delay the PR a bit.

ericonr · 2020-12-11T18:24:05Z

Ping?

Foxboron · 2020-12-12T16:04:11Z

database.go

+	} else {
+		if os.IsPermission(err) {
+			warning.Printf(rootMsg)
+		}
+		return nil, err


Suggested change

} else {

if os.IsPermission(err) {

warning.Printf(rootMsg)

}

return nil, err

} else if os.IsPermission(err) {

warning.Printf(rootMsg)

return nil, err

}

Needs to be written like that, else we will always hit the else clause and return which makes you unable to sign. I think the root issue is that we don't really check if err is nil or not. If err is nil, the first if is False which gives us a return. This code is only handling permission error, but nothing else.

Might be an improvement for another time?

If err is nil, the first if is False which gives us a return. This code is only handling permission error, but nothing else.

I believe checking explicitly for err != nil is best, thanks for pointing this out!

ericonr · 2020-12-13T19:02:27Z

I've dropped my usage of unix.Access in database.go, since simply checking for errors when trying to read the file is less race-prone and works just as well.

As explained in the commit message, it's still necessary in keys.go, since we run sbsign as a command.

Foxboron · 2021-01-05T20:12:01Z

The replies are a bit inconsistent:

λ sbctl pr-31» ./sbctl verify    
==> WARNING: It might be necessary to run this tool as root
==> WARNING: Couldn't read file database: open /usr/share/secureboot/files.db: permission denied
==> Verifying EFI images in /efi...
  -> WARNING: /efi/EFI/BOOT/BOOTX64.EFI is not signed
  -> WARNING: /efi/EFI/Linux/linux-linux-mitigations.efi is not signed
  -> WARNING: /efi/EFI/Linux/linux-linux.efi is not signed
  -> WARNING: /efi/EFI/arch/fwupdx64.efi is not signed
  -> WARNING: /efi/EFI/systemd/systemd-bootx64.efi is not signed
  -> WARNING: /efi/vmlinuz-linux is not signed
  -> WARNING: /efi/vmlinuz-linux-lts is not signed

λ sbctl pr-31» ./sbctl list-files
==> WARNING: It might be necessary to run this tool as root
  -> ERROR: Couldn't open database: /usr/share/secureboot/files.db

λ sbctl pr-31» ./sbctl list-bundles
2021/01/05 21:07:47 open /usr/share/secureboot/bundles.db: permission denied

Part of the problem here is that ReadBundleDatabase doesn't implement the access checking. I think it's better at this point to just throw a ReadFile function into util.go that does this checking. It would allow us to better seek the correct permissions in the future.

ericonr · 2021-01-06T20:10:45Z

Oh, well spotted. I will try to organize it better.

This allows err to be used anywhere as the error variable, instead of having to use "e", for example. This commit also fixes a bug where the PrintGenerateError() calls in CombineFiles() were using "err" as the argument for error, when it should have been "e" - since "err" was the logger and could be used in that way, the compiler didn't complain. Signed-off-by: Érico Rolim <erico.erc@gmail.com>

If ReadFile errors out, the error would only be checked after the function attempts to read the buffer into the hasher. This commit fixes that, checking the error as soon as possible. Signed-off-by: Érico Rolim <erico.erc@gmail.com>

- Introduces dependency on sys/unix for unix.Access. This is necessary only in keys.go, since we run 'sbsign' as a command and can't check if it failed due to permissions. - Allows removing special casing in main.go for commands that don't require root permissions. - ReadFileDatabase() can now return errors due to the multiple ways in which it can fail; it also warns the user about possibly requiring root. - ReadFileDatabase() was using the global DBPath instead of its dbpath parameter in multiple places. This has been fixed. - VerifyESP() can now run without root. - SignFile() checks if it can read the DB key before running sbsign. Signed-off-by: Érico Rolim <erico.erc@gmail.com>

This function will try to read a file into a byte buffer, and, if the file doesn't exist, create its containing directory and the file itself. If any of those actions fail due to permissions, the function will print a warning about running the tool as root. Reading from the file and bundle databases works like this, so the error checking should be implemented in a single place. Also, use the new function in ReadFileDatabase(). Signed-off-by: Érico Rolim <erico.erc@gmail.com>

Using the function also removed code that had hardcoded globals for the location of some files instead of using the dbpath parameter. Add error checking around the function where appropriate. Also fail early when creating a new bundle if it isn't possible to access the bundle database. Signed-off-by: Érico Rolim <erico.erc@gmail.com>

ericonr · 2021-01-11T03:53:33Z

Ok, the PR has increased a bit. I haven't checked extensively if the UI got more consistent, but at least the code should have. It's quite a few changes, but I believe you should be able to follow them logically by looking at the commit history; I tried to explain each change as well as possible.

ericonr commented Oct 28, 2020

View reviewed changes

ericonr force-pushed the help branch from 8475b15 to 1e3e24c Compare November 7, 2020 05:55

Foxboron requested changes Dec 12, 2020

View reviewed changes

ericonr force-pushed the help branch from 1e3e24c to 8f8b229 Compare December 13, 2020 19:00

ericonr added 2 commits January 11, 2021 00:37

Fix potential seg fault in ChecksumFile.

0a7c6e8

If ReadFile errors out, the error would only be checked after the function attempts to read the buffer into the hasher. This commit fixes that, checking the error as soon as possible. Signed-off-by: Érico Rolim <erico.erc@gmail.com>

ericonr force-pushed the help branch from 8f8b229 to 4ef8f5b Compare January 11, 2021 03:48

ericonr added 2 commits January 11, 2021 00:49

ericonr force-pushed the help branch from 4ef8f5b to ccb25a9 Compare January 11, 2021 03:49

ericonr force-pushed the help branch from ccb25a9 to f94f185 Compare January 11, 2021 03:53

Foxboron merged commit 79d986f into Foxboron:master Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

main: help subcommand should work without root. #31

main: help subcommand should work without root. #31

ericonr commented Oct 28, 2020

ericonr commented Oct 28, 2020 •

edited

Loading

ericonr Oct 28, 2020

Foxboron Oct 28, 2020 •

edited

Loading

ericonr Oct 28, 2020

Foxboron Oct 28, 2020

ericonr commented Oct 29, 2020 •

edited

Loading

ericonr commented Oct 31, 2020

Foxboron commented Oct 31, 2020

ericonr commented Nov 7, 2020

Foxboron commented Nov 7, 2020 •

edited

Loading

ericonr commented Nov 7, 2020 •

edited

Loading

ericonr commented Dec 11, 2020

Foxboron Dec 12, 2020

ericonr Dec 13, 2020

ericonr commented Dec 13, 2020

Foxboron commented Jan 5, 2021 •

edited

Loading

ericonr commented Jan 6, 2021

ericonr commented Jan 11, 2021

main: help subcommand should work without root. #31

main: help subcommand should work without root. #31

Conversation

ericonr commented Oct 28, 2020

ericonr commented Oct 28, 2020 • edited Loading

ericonr Oct 28, 2020

Choose a reason for hiding this comment

Foxboron Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

ericonr Oct 28, 2020

Choose a reason for hiding this comment

Foxboron Oct 28, 2020

Choose a reason for hiding this comment

ericonr commented Oct 29, 2020 • edited Loading

ericonr commented Oct 31, 2020

Foxboron commented Oct 31, 2020

ericonr commented Nov 7, 2020

Foxboron commented Nov 7, 2020 • edited Loading

ericonr commented Nov 7, 2020 • edited Loading

ericonr commented Dec 11, 2020

Foxboron Dec 12, 2020

Choose a reason for hiding this comment

ericonr Dec 13, 2020

Choose a reason for hiding this comment

ericonr commented Dec 13, 2020

Foxboron commented Jan 5, 2021 • edited Loading

ericonr commented Jan 6, 2021

ericonr commented Jan 11, 2021

ericonr commented Oct 28, 2020 •

edited

Loading

Foxboron Oct 28, 2020 •

edited

Loading

ericonr commented Oct 29, 2020 •

edited

Loading

Foxboron commented Nov 7, 2020 •

edited

Loading

ericonr commented Nov 7, 2020 •

edited

Loading

Foxboron commented Jan 5, 2021 •

edited

Loading