Skip to content

BundlePublisher exec plugin #5939

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

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

BundlePublisher exec plugin #5939

wants to merge 27 commits into from

Conversation

kfox1111
Copy link
Contributor

@kfox1111 kfox1111 commented Mar 10, 2025

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Description
Adds a BundlePublisher that execs a command with the bundle on stdin

Fixes: #5944

@sorindumitru
Copy link
Collaborator

@kfox1111 Could you also open an issue for this so we can have some discussion about this there?

@kfox1111
Copy link
Contributor Author

#5944

Comment on lines 55 to 57
if newConfig.Format == "" {
status.ReportError("configuration is missing the bundle format")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe have default format?

}

if len(newConfig.Cmd) < 1 {
status.ReportError("configuration is missing cmd")
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no cmd should we exit and return error?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the intention behind status.Report is to try to get all configuration errors at once,
So we may continue processing and core is going to return error in case status is not empty,
an example of this use case is if cmd is not present but at the same time format is invalid,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed


// Config holds the configuration of the plugin.
type Config struct {
Cmd []string `hcl:"cmd" json:"cmd"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the Args be a separate configurable?

Co-authored-by: Faisal Memon <[email protected]>
Signed-off-by: kfox1111 <[email protected]>
// We use gosec -- the annotation below will disable a security check that users didn't specify the command
// Its their command.
/* #nosec G204 */
cmd := exec.Command(config.Cmd, config.Args...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should run use exec.CommandContext so the execution is bound by the lifetime of the context. I wonder if there's anything else we can do to make sure we don't end up with lots of spawned processes that can't be killed.

We should also clear the environment so that the spawned command doesn't see any of the environment variables of spire-server (e.g. database credentials).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesnt go reap child processes automatically?

We could put a timeout around it perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use exec.CommandContext

}

if len(newConfig.Cmd) < 1 {
status.ReportError("configuration is missing cmd")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the intention behind status.Report is to try to get all configuration errors at once,
So we may continue processing and core is going to return error in case status is not empty,
an example of this use case is if cmd is not present but at the same time format is invalid,

// We use gosec -- the annotation below will disable a security check that users didn't specify the command
// Its their command.
/* #nosec G204 */
cmd := exec.CommandContext(ctx, config.Cmd, config.Args...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we validate the command before execution? For example, should we explicitly prevent dangerous commands like rm?

What types of commands are expected here? Can we restrict execution to a predefined set of allowed commands?

Should we also enforce privilege restrictions to prevent running commands with elevated permissions?"

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 one, I pretty strongly disagree with. We can not protect the user from all possible commands a shell can execute. There are infinite possibilities. We can take a stab at it, but will always miss things. and then we are more liable, as "we didn't project me from doing this other unwise thing". As we accepted that we will protect them at all, then not fully protecting them is on us, not them. :/

Its completely under the sysadmins control. They know their systems well and what is appropriate. They configure the exec plugin, they can test it on their test cluster to ensure proper functioning before deploying to production, its up to them exactly what it will do.

They may write the bundle to a temporary file, scp it somewhere else, then remove the temporary file, for example. It would be exceptionally hard to parse bash or powershell or whatever and guess all the ways things could break. Or, if we only allow a few commands possible to run, it completely defeats the purpose of the plugin I think.

Security wise, forking off and running in the same context as the server is basically how the plugins themselves work now. So, not sure how much it buys us to add settings like "switch user to this before execing". could do that in the command itself I think.

An exec plugin allows certain things to be written quickly/easily by a sysadmin. But it doesn't preclude those things from eventually being made regular BundlePublisher plugins at some point too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we achieve this with a BundlePublisher that writes bundle updates to disk?

Instead of adding an exec command, we could update this plugin to store updates on disk. Then, another process could watch for file updates. This approach would avoid the need to add the capability to run any command that consumes standard input.

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 if (I do), want to simply
cmd = "bash"
args = ["-c", "ssh othermachine 'cat > trustbundle.spiffe"]

No disk writes needed locally.

Inotify would mean more daemons for a sysadmin to manage, and have to carefully secure communications on. I'd rather pull the data right from a pipe and push it to gpg, openssl, or ssh or other tools as part of my scripting, rather then let it land on disk in the middle and have to reconcile the local copy my own code. The idea is let the sysadmin be able to simply use trust bundles without a lot of work. Otherwise, writing a custom BundlePublisher plugin is probably worth the pain.


The trust bundle is formatted using PEM encoding. Only the X.509 authorities are included.

## Sample configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

can this be used on windows? if that is the case, can we provide an example there too?
in case it is not supported in windows we will need to return error

Copy link
Contributor Author

@kfox1111 kfox1111 Mar 20, 2025

Choose a reason for hiding this comment

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

It should work on all platforms.

I just took a rough stab, but don't have a windows box to test on. Do you?

}
}

func TestPublishBundle(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add tests for Validate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a stab at it.

{
name: "success",
bundle: testBundle,
config: &Config{
Copy link
Collaborator

@MarcosDY MarcosDY Mar 19, 2025

Choose a reason for hiding this comment

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

This test does not currently verify whether bundles are actually being sent. What do you think about saving the response to a temporary file or creating a fake for exec.CommandContext using a hook?

By capturing and storing the executed command, we can ensure that the expected command is being run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BundlePublisher exec plugin
4 participants