Open rfratto opened 1 year ago
Entirely for the capabilities model. Using bitwise operates feels a bit limiting, but that can always be changed if needed. Using string matching would mean there is less overhead for adding new capabilities.
Not sure on the loading section, we might be able to use a new method that returns the capabilities for a given config on a per component basis. This would allow us to interrogate before applying.
Capabilities(a component.Arguments) []string // Returns the list of capabilities this argument needs.
I like the idea of capabilities being defined in terms of the arguments 👍
Another idea: we could add the definition of capabilities to the registration of the component rather than as a method:
func init() {
component.Register(component.Registration{
Name: "local.file",
Args: Arguments{},
Exports: Exports{},
CapabilitiesFunc: func(args component.Arguments) []string {
return []string{"CAP_FILE"}
},
Build: func(opts component.Options, args component.Arguments) (component.Component, error) {
return New(opts, args.(Arguments))
},
})
}
This has the benefit of being more discoverable from the API compared to extension interfaces, and it's stateless, so you wouldn't have to construct a component to be able to tell if the capabilities are permitted.
I'm following along here and not fully understanding the practical use case for capabilities. Are we thinking someone would want to limit the capabilities of a module loader because they don't know what's inside the module they are loading? Or something else?
Are we thinking someone would want to limit the capabilities of a module loader because they don't know what's inside the module they are loading?
Yes, if you're using a module from the internet you might not want to trust it to be able to access your local filesystem.
Adding to component.Register
may make some twisting to avoid cyclic dependencies. I prefer using interface on arguments and having it be a func of the argument. This ensures that it is "owned" by the argument instead of somewhere else. You would also not need to create a component to figure it out, and inherently you have already created the argument to test.
Adding to component.Register may make some twisting to avoid cyclic dependencies. I prefer using interface on arguments and having it be a func of the argument. This ensures that it is "owned" by the argument instead of somewhere else. You would also not need to create a component to figure it out, and inherently you have already created the argument to test.
I don't think it introduces any cyclic dependencies; it's no more cyclic than the Build function.
The problem with adding it as a method is that you have to build the component first. This is a problem with local.file
, where it reads the file on construction and exports its contents, risking exposure to other components in the module.
Either the capabilities checking needs to be fully decoupled from the instance of the component, or components must be partially or completely responsible for self-checking that they're not breaking capabilities assigned to them.
You shouldn't need to build the component, only the argument.
// local.file arguments
type Arguments struct {
// Filename indicates the file to watch.
Filename string `river:"filename,attr"`
// Type indicates how to detect changes to the file.
Type Detector `river:"detector,attr,optional"`
// PollFrequency determines the frequency to check for changes when Type is
// UpdateTypePoll.
PollFrequency time.Duration `river:"poll_freqency,attr,optional"`
// IsSecret marks the file as holding a secret value which should not be
// displayed to the user.
IsSecret bool `river:"is_secret,attr,optional"`
}
func (a Arguments) Capabilities() []string {
return []string{"file"}
}
type Arguments interface {
Capabilities() []string
}
OK, that should be fine, as long as it's an extension interface and not required for all Arguments types going forward.
Background
Today, users of Grafana Agent Flow can use any component. With the upcoming introduction of modules, users can also modules, which are completely unrestricted. Users may not want to load modules which have full access to the filesystem (via
local.file
or other components), as it's possible to write modules to send the contents of the filesystem over the network.Some mechanism for restricting behavior of loaded components within a module is desirable.
In the meantime, however, users should only run modules from sources they trust, similar to how you would trust a software library to not act as malware.
Proposal
I propose adding a best-effort capabilities system in Flow which can determine what components or component functionality within components is available. Capabilities are defined at the controller level, and propagate to constructed components.
Capabilities are defined using bit flags:
This allows a total of 64 possible capabilities which could be defined.
For backwards compatibility, a controller defaults to
CapAll
.Defining capabilities
In configuration files, capabilities are defined as arrays of strings:
It should also be possible to provide capabilities to the root controller to create a locked down instance of Grafana Agent:
Rules for following capabilities
Since it is not possible to enforce what functionality a component uses, component implementations are trusted to read the capabilities permitted to them and reject configs appropriately.
Capabilities also can't be always associated with a component as a whole. For example, while
local.file
always reads from the filesystem and always requires thefilesystem
capability,prometheus.remote_write
only requires thefilesystem
capability when password_file or another file-based field is used.Given the above, the rules of capabilities are as follows:
Loading
Capabilities will be added to
component.Options
. This means that if the capabilities assigned to a controller change, all components within that controller must be terminated and reinitialized with the new capabilities set.cc @mattdurham @erikbaranowski as this relates to modules.