Validate Dataset modules for the Kedro HTTP server - #5726
Conversation
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
|
Thanks @lrcouto! I think before we add allowlisting mechanism here, we should wait to finalise the protocol re: #5652 |
I'm thinking on how this could be done, because any dataset type entry is "executable" as in "running the module code", as it resolves to the dataset object. But not all of those have the possibility of being vulnerable to RCE. In the example of the PickleDataset, it'll load and arbitrary python object, which can be dangerous, but that same danger is not going to happen on a CSV dataset. The idea of using a whitelist would be to add that element of human oversight to the type selection before the dataset object itself is loaded. So I'm considering blocking dataset selection via |
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Signed-off-by: L. R. Couto <57910428+lrcouto@users.noreply.github.com>
|
Pushed a different solution. A little less flexible than the allowlist, but simpler. |
ankatiyar
left a comment
There was a problem hiding this comment.
Thanks @lrcouto this solution looks better to me - left some suggestions
Also wondering if this can be done in a slightly cleaner way without adding too much code in OmegaConfigLoader. I don't have a clean solution yet myself, will think about it some more and get back to you about this.
| k: v for k, v in merged_config_container.items() if not k.startswith("_") | ||
| } | ||
|
|
||
| @typing.no_type_check |
There was a problem hiding this comment.
Is this necessary here and on line 435?
There was a problem hiding this comment.
Mypy complains about the big Union that returns from OmegaConf.to_container. It's the same reason why this decorator is being used for load_and_merge_dir_config, for example.
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
|
Thank you @ankatiyar ! Applied the suggested changes. |
merelcht
left a comment
There was a problem hiding this comment.
The approach looks good to me. My main suggestion is to clean up the docstring/comments a bit, because they're quite verbose.
Signed-off-by: Laura Couto <laurarccouto@gmail.com>
|
Thank you @merelcht ! Applied the suggested changes. |
merelcht
left a comment
There was a problem hiding this comment.
Thanks Laura, this looks good to me now! 👍
Description
Addresses a possible RCE vulnerability on catalog
typeresolution through non-trustedruntime_paramson the HTTP server.Development notes
This PR was reworked after initial review. The first version added a
dataset_modules_whitelistallowlist toOmegaConfigLoaderbased on #5568. We decided to not use an allowlist for this specific issue (an unified allowlist mechanism for Kedro is being currently discussed here: #5652)For this current version,
OmegaConfigLoadergainsrestrict_runtime_params_type_selection: bool = False. When set, any catalogtypefield actually resolved viaruntime_params(at any nesting depth) is simply rejected.Instead,
KedroServiceSession.run()/.load_context()/._get_config_loader()gains atrusted_runtime_params: bool = Trueparameter.trusted_runtime_params=Falseis passed byhttp_server.py's_execute_pipeline, sincerequest.paramsis where untrusted and possibly malicious input is going to come from.Impact to existing projects
Only projects whose catalog uses the
runtime_params:resolver to set a dataset'stypefield, and run that catalog through the Kedro HTTP server, are affected. For example:If the catalog does not contain this pattern, no changes need to be made.
If it does and it serves it over the HTTP server,
POST /runrequests that receivetypefromparamswill now fail withInterpolationResolutionError,