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

Merged
merged 39 commits into from
Oct 31, 2024

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
Contributor

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
drivers/shared/validators/validators.go Show resolved Hide resolved
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
drivers/exec/driver.go Outdated Show resolved Hide resolved

func (d *Driver) Validate(cfg drivers.TaskConfig) error {
usernameToLookup := cfg.User
var user *user.User
Copy link
Member

Choose a reason for hiding this comment

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

This variable collides with the imported package of the same name, could we change it?

ErrInvalidRange = errors.New("lower bound cannot be greater than upper bound")
)

type validator struct {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to export this type considering it is an used by other packages with exported functions?

Copy link
Member

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

If validator.NewValidator returned an interface instead of a concrete type, we wouldn't want to / need to export this type. But I think I've been convinced by @Juanadelacuesta here #20073 (comment) that we can return the unexported concrete type from NewValidator and then the callers can use their own package-local version of the interface to reduce the scope of methods they care about.

(Sort of like what we do in lots of places in the client with the concrete TaskRunner and the many tiny one/two-function interfaces expected by hooks.)

Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

Added some inline comments.

The semgrep CI check is also failing:

    drivers/shared/validators/validators.go 
       semgrep.mpl_busl                                                         
          BUSL package `github.com/hashicorp/nomad/client/lib/idset` imported in
  MPL package                                                                   
          `validators`                                                          
                                                                                
           14┆ "github.com/hashicorp/nomad/client/lib/idset"
            ⋮┆----------------------------------------
       semgrep.mpl_busl                                                         
          BUSL package `github.com/hashicorp/nomad/client/lib/numalib/hw`       
  imported in MPL package                                                       
          `validators`                                                          
                                                                                
           15┆ "github.com/hashicorp/nomad/client/lib/numalib/hw"
                                                    
    drivers/shared/validators/validators_default.go 
       semgrep.mpl_busl                                                         
          BUSL package `github.com/hashicorp/nomad/client/lib/numalib/hw`       
  imported in MPL package                                                       
          `validators`                                                          
                                                                                
           11┆ "github.com/hashicorp/nomad/client/lib/numalib/hw"
                                                 
    drivers/shared/validators/validators_unix.go 
       semgrep.mpl_busl                                                         
          BUSL package `github.com/hashicorp/nomad/client/lib/numalib/hw`       
  imported in MPL package                                                       
          `validators`                                                          
                                                                                
           13┆ "github.com/hashicorp/nomad/client/lib/numalib/hw"
                            
  BLOCKING CODE RULES FIRED:
    semgrep.mpl_busl

@tgross
Copy link
Member

tgross commented Oct 31, 2024

The semgrep CI check is also failing:

It's detecting the issue described here #19833 because the files were touched. We should fix that, but probably under a separate PR.

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.

LGTM once the test compilation error is fixed.

Copy link
Member

@mismithhisler mismithhisler left a comment

Choose a reason for hiding this comment

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

Looks good!

@Juanadelacuesta Juanadelacuesta merged commit c18418f into main Oct 31, 2024
27 of 28 checks passed
@Juanadelacuesta Juanadelacuesta deleted the feat/uid-gid-restriction branch October 31, 2024 14:48
@Juanadelacuesta Juanadelacuesta restored the feat/uid-gid-restriction branch October 31, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.9.x backport to 1.9.x release line 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.

7 participants