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

Adds ability to restrict uid and gids in exec and raw_exec #20073

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

mikenomitch
Copy link
Contributor

Adds ability to restrict host uid and gids in exec and raw_exec.

To Test:

Add the following to agent config:

plugin "exec" {
  enabled = true
  config {
    denied_host_uids = "0-65534"
    denied_host_gids = ""
  }
}

plugin "raw_exec" {
  config {
    enabled = true
    denied_host_uids = "1,2-9"
    denied_host_gids = "0-100"
  }
}

Then in raw_exec or exec tasks change the "user" value to become a user in any of these ranges. Note that you should see an error like the following:
Screenshot 2024-03-05 at 10 53 06 AM

It should also error on job submit if you give it bad ranges. IE "0,1-foo"

Note: This is only needed on raw_exec, but since it felt like the code was 90% reusable and would be appreciated in exec too, I figured I'd add it (at the risk of a bit of scope creep). It also felt like I'd set us up better to add this to exec_v2 by just adding this in a shared location.

drivers/exec/driver.go Outdated Show resolved Hide resolved
@shoenig
Copy link
Member

shoenig commented Mar 5, 2024

@mikenomitch looks like the linter is unhappy

you should be able to run make check yourself locally and see the same thing

==> Linting source code...
Error: drivers/shared/validators/validators.go:73:29: unnecessary conversion (unconvert)
		gids = append(gids, uint64(u))

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

This is looking pretty good @mikenomitch, but I've left some comments we should address.

Also:

  • We need a changelog entry
  • Note that the test-e2e-vault test failure isn't a flake, but a appears to be bug introduced by this PR:

Error: 2024-03-06T00:13:33.371Z [ERROR] client.alloc_runner.task_runner: running driver failed: alloc_id=ca431f74-e99e-8749-dc0e-62de3f1b96e9 task=cat error="failed driver config validation: failed to identify user "": user: unknown user "

drivers/shared/validators/validators_test.go Outdated Show resolved Hide resolved
drivers/shared/validators/validators.go Outdated Show resolved Hide resolved
drivers/rawexec/driver_test.go Outdated Show resolved Hide resolved
drivers/shared/validators/validators.go Outdated Show resolved Hide resolved
drivers/shared/validators/validators.go Outdated Show resolved Hide resolved
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package validators
Copy link
Member

Choose a reason for hiding this comment

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

The test is //go:build !windows, should this file be as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used by exec/driver.go which does not have !windows in it, so I think it gets tricky.

I could wrap the use in a function in exec/driver.go like I do with raw_exec and then move that function out to exec/driver_unix.go? And then this could safely be a 100% non-windows file.

Hmm... I'm also debating moving the os-level user getting bits out of this file wholesale, which might change the dynamics of this a bit. In which case, I can leave this as a general validator, then just make the test a _unix test.

Copy link
Member

Choose a reason for hiding this comment

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

This is used by exec/driver.go which does not have !windows in it, so I think it gets tricky.

Right, but I think that's probably ok here? For example, if I later decide to add UID/GID restrictions to the java driver, which does run on Windows, I would want the compile to fail if I don't properly split out the behavior so that the Windows build never calls this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I could do is remove the uid/gid bits from this and take those as args, and then this file could run on Windows just fine and work - you just wouldn't want to because it probably wouldn't be useful, but nothing would break. And then I could use the windows/unix builds of raw_exec to gate usage (like I already do).

If I am using the validators package to keep the struct for IdRange then I can't make it all unix only because then the import into the Config won't work. I could always make a structs package to get around that though.

drivers/shared/validators/validators.go Outdated Show resolved Hide resolved
website/content/docs/drivers/exec.mdx Outdated Show resolved Hide resolved
website/content/docs/drivers/exec.mdx Outdated Show resolved Hide resolved
drivers/rawexec/driver_unix_test.go Outdated Show resolved Hide resolved
drivers/exec/driver.go Outdated Show resolved Hide resolved
drivers/exec/driver_test.go Outdated Show resolved Hide resolved
drivers/exec/driver_test.go Outdated Show resolved Hide resolved
drivers/exec/driver_test.go Outdated Show resolved Hide resolved
@mikenomitch
Copy link
Contributor Author

Update:

Most of the in-code comments have been resolved and I've done some light refactoring outside of that too. Feels like its in a much nicer state.

I still need to figure out the e2e issue. Seems that I'm trying to look up an empty user, which would only be the case if users.Current() returns an empty username "" I think.

Re the Semgrep issue: If I want to import nomad/structs into a driver, the driver is MPL but structs is BSL, so Semgrep is yelling at me. I see that in some other driver files we do this though. I could work around this by putting my struct elsewhere (I think?) but it feels like the right place to put it. Will try to figure out what's ideal here.

drivers/exec/driver.go Outdated Show resolved Hide resolved
drivers/exec/driver_test.go Outdated Show resolved Hide resolved
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package validators
Copy link
Member

Choose a reason for hiding this comment

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

This is used by exec/driver.go which does not have !windows in it, so I think it gets tricky.

Right, but I think that's probably ok here? For example, if I later decide to add UID/GID restrictions to the java driver, which does run on Windows, I would want the compile to fail if I don't properly split out the behavior so that the Windows build never calls this code.

drivers/exec/driver.go Show resolved Hide resolved
@apollo13
Copy link
Contributor

apollo13 commented Mar 14, 2024

Probably a stupid question, but wouldn't it make more sense to have a whitelist for users you want to allow? I mean in general I can imagine forbidding everything below id 1000 which are usually system users. But above 1000 it is really hard to not end up with a list with plenty of items. And supporting names instead of just the raw ids would be lovely as well. Ie I want to block the groups "wheel" and "sudo" on every system without bothering which exact gid it has on the system (those are commonly used for sudo on rhel or debian and derivatives).

@mikenomitch mikenomitch requested a review from tgross March 22, 2024 15:05
@tgross tgross added stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows and removed backport/1.7.x backport to 1.7.x release line labels May 17, 2024
@Juanadelacuesta Juanadelacuesta self-assigned this May 31, 2024
@Juanadelacuesta Juanadelacuesta removed their assignment Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows theme/driver/exec theme/driver/raw_exec theme/driver type/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants