Hi Tina,

Thanks for the KIP! Looks good overall. A few minor thoughts:

1. We can remove the "This page is meant as a template for writing a KIP"
section from the beginning.

2. The type of the allowed.paths property is string in the KIP, but the
description mentions it'll contain multiple comma-separated paths.
Shouldn't it be described as a list? Or are we calling it a string in order
to allow for escape syntax for directories that may contain the delimiter
character (e.g., ',')?

3. I'm guessing the answer is yes but I want to make sure--will users be
allowed to specify files in the allowed.paths property?

4. Again, guessing the answer is yes but to make sure--if a directory is
specified in the allowed.paths property, will all files (nested or
otherwise) be accessible by the config provider? E.g., if I set
allowed.paths to "/", then everything on the entire file system would be
accessible, instead of just the files directly inside the root directory.

Cheers,

Chris

On Thu, Dec 7, 2023 at 9:33 AM Gantigmaa Selenge <gsele...@redhat.com>
wrote:

> Thank you Mickael.
>
> I'm going to leave the discussion thread open for a couple more days and if
> there are no further comments, I would like to start the vote for this KIP.
>
> Thanks.
> Regards,
> Tina
>
> On Wed, Dec 6, 2023 at 10:06 AM Mickael Maison <mickael.mai...@gmail.com>
> wrote:
>
> > Hi,
> >
> > I'm not aware of any other mechanisms to explore the filesystem. If
> > you have ideas, please reach out to the security list.
> >
> > Thanks,
> > Mickael
> >
> > On Tue, Dec 5, 2023 at 1:05 PM Gantigmaa Selenge <gsele...@redhat.com>
> > wrote:
> > >
> > > Hi everyone,
> > >
> > >
> > > Apologies for the very delayed response. Thank you both for the
> feedback.
> > >
> > >
> > > > For clarity it might make sense to mention this feature will be
> useful
> > >
> > > when using a ConfigProvider with Kafka Connect as providers are set in
> > >
> > > the runtime and can then be used by connectors. This feature has no
> > >
> > > use when using a ConfigProvider in server.properties or in clients.
> > >
> > >
> > > I have updated the KIP to address this suggestion. Please let me know
> if
> > > it's not clear enough.
> > >
> > >
> > > > When trying to use a path not allowed, you propose returning an
> error.
> > >
> > > With Connect does that mean the connector will be failed? The
> > >
> > > EnvVarConfigProvider returns empty string in case a user tries to
> > >
> > > access an environment variable not allowed. I wonder if we should
> > >
> > > follow the same pattern so the behavior is "consistent" across all
> > >
> > > built-in providers.
> > >
> > >
> > > I agree with this, it makes sense to have consistent behaviour across
> all
> > > the providers. I made this update.
> > >
> > >
> > > > 1. In the past Connect removed the FileStream connectors in order to
> > >
> > > prevent a REST API attacker from accessing the filesystem. Is this the
> > >
> > > only remaining attack vector for reading the file system? Meaning, if
> > >
> > > this feature is configured and all custom plugins are audited for
> > >
> > > filesystem accesses, would someone with access to the REST API be
> > >
> > > unable to access arbitrary files on disk?
> > >
> > >
> > > Once this feature is configured, it will stop someone from accessing
> the
> > > file system via config providers.
> > >
> > > However, I’m not sure whether there are other ways users can access
> file
> > > systems via REST API.
> > >
> > >
> > > Mickael, perhaps you have some thoughts on this?
> > >
> > >
> > > > 2. Could you explain how this feature would prevent a path traversal
> > >
> > > attack, and how we will verify that such attacks are not feasible?
> > >
> > >
> > > The intention is to generate File objects based on the String value
> > > provided for allowed.paths and the String path passed to the get()
> > function.
> > >
> > > This would allow validation of path inclusion within the specified
> > allowed
> > > paths using their corresponding Path objects, rather than doing String
> > > comparisons.
> > >
> > > This hopefully will mitigate the risk of path traversal. The
> > implementation
> > > should include unit tests to verify this.
> > >
> > >
> > > > 3. This applies a single "allowed paths" to a whole worker, but I've
> > >
> > > seen situations where preventing one connector from accessing
> > >
> > > another's secrets may also be desirable. Is there any way to extend
> > >
> > > this feature now or in the future to make that possible?
> > >
> > >
> > > One approach could be creating multiple providers, each assigned a
> unique
> > > name and specific allowed.paths configuration. Users would then be
> > assigned
> > > a provider name, granting them appropriate access on the file system to
> > > load variables for their connectors. However, during provider
> > > configuration, administrators would have to anticipate and specify the
> > > files and directories users may require access to.
> > >
> > >
> > > Regards,
> > >
> > > Tina
> > >
> > > On Wed, Nov 8, 2023 at 7:49 PM Greg Harris
> <greg.har...@aiven.io.invalid
> > >
> > > wrote:
> > >
> > > > Hey Tina,
> > > >
> > > > Thanks for the KIP! Unrestricted file system access over a REST API
> is
> > > > an unfortunate anti-pattern, so I'm glad that you're trying to change
> > > > it. I had a few questions, mostly from the Connect perspective.
> > > >
> > > > 1. In the past Connect removed the FileStream connectors in order to
> > > > prevent a REST API attacker from accessing the filesystem. Is this
> the
> > > > only remaining attack vector for reading the file system? Meaning, if
> > > > this feature is configured and all custom plugins are audited for
> > > > filesystem accesses, would someone with access to the REST API be
> > > > unable to access arbitrary files on disk?
> > > > 2. Could you explain how this feature would prevent a path traversal
> > > > attack, and how we will verify that such attacks are not feasible?
> > > > 3. This applies a single "allowed paths" to a whole worker, but I've
> > > > seen situations where preventing one connector from accessing
> > > > another's secrets may also be desirable. Is there any way to extend
> > > > this feature now or in the future to make that possible?
> > > >
> > > > Thanks!
> > > > Greg
> > > >
> > > > On Tue, Nov 7, 2023 at 7:06 AM Mickael Maison <
> > mickael.mai...@gmail.com>
> > > > wrote:
> > > > >
> > > > > Hi Tina,
> > > > >
> > > > > Thanks for the KIP.
> > > > > For clarity it might make sense to mention this feature will be
> > useful
> > > > > when using a ConfigProvider with Kafka Connect as providers are set
> > in
> > > > > the runtime and can then be used by connectors. This feature has no
> > > > > use when using a ConfigProvider in server.properties or in clients.
> > > > >
> > > > > When trying to use a path not allowed, you propose returning an
> > error.
> > > > > With Connect does that mean the connector will be failed? The
> > > > > EnvVarConfigProvider returns empty string in case a user tries to
> > > > > access an environment variable not allowed. I wonder if we should
> > > > > follow the same pattern so the behavior is "consistent" across all
> > > > > built-in providers.
> > > > >
> > > > > Thanks,
> > > > > Mickael
> > > > >
> > > > > On Tue, Nov 7, 2023 at 1:52 PM Gantigmaa Selenge <
> > gsele...@redhat.com>
> > > > wrote:
> > > > > >
> > > > > > Hi everyone,
> > > > > >
> > > > > > Please let me know if you have any comments on the KIP.
> > > > > >
> > > > > > I will leave it for a few more days. If there are still no
> > comments, I
> > > > will
> > > > > > start the vote on it.
> > > > > >
> > > > > > Regards,
> > > > > > Tina
> > > > > >
> > > > > > On Wed, Oct 25, 2023 at 8:31 AM Gantigmaa Selenge <
> > gsele...@redhat.com
> > > > >
> > > > > > wrote:
> > > > > >
> > > > > > > Hi everyone,
> > > > > > >
> > > > > > > I would like to start a discussion on KIP-933 that proposes
> > > > restricting
> > > > > > > files accessed by File and Directory ConfigProviders.
> > > > > > >
> > > > > > >
> > > > > > >
> > > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-993%3A+Allow+restricting+files+accessed+by+File+and+Directory+ConfigProviders
> > > > > > >
> > > > > > > Regards,
> > > > > > > Tina
> > > > > > >
> > > >
> > > >
> >
> >
>

Reply via email to