Thanks Chris. I have added that as well. I think I can start the vote on this KIP if there are no further comments.
Thanks. Regards, Tina On Tue, Dec 12, 2023 at 1:43 PM Chris Egerton <chr...@aiven.io.invalid> wrote: > Thanks Tina! LGTM as long as we take care to document that recursive access > to directories will be granted when we release this feature. > > On Tue, Dec 12, 2023 at 8:16 AM Gantigmaa Selenge <gsele...@redhat.com> > wrote: > > > Hi Chris, > > > > Thank you for the feedback. > > > > > > 1. Addressed > > > > > > 2. I have updated the type to be List. The configure() function is more > > likely to process the value as String and convert to List using the comma > > separation but I think it makes sense to specify it as List, as that is > the > > final type. > > > > > > 3. Yes, it's mentioned under the Public Interfaces section but I also > added > > another sentence to make it clearer. > > > > > > 4. Yes, I have just tested this to confirm and it looks like "/" gives > > access to the entire file system. > > > > > > Thanks. > > Regards, > > Tina > > > > > > > > > > On Thu, Dec 7, 2023 at 2:58 PM Chris Egerton <chr...@aiven.io.invalid> > > wrote: > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
