Skip to content
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

DRILL-8465: Check Input Data for Iceberg Plugin #2853

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pjfanning
Copy link
Contributor

Description

https://issues.apache.org/jira/browse/DRILL-8465

I'm not happy with the class check here. I don't know to what extent that we need to support subclasses that a user might make of Iceberg classes. If we need to support subclasses of Iceberg classes, it might be better to support a config option that allows users to extend the allow list for classes in this deserialization code.

In this PR, the allow list is:

  • java primitives
  • java/javax classes
  • org.apache.iceberg classes
  • org.apache.drill classes
  • arrays of the types listed above
  • messiest bit is the support for any subclass of iceberg ScanTask class

@pjfanning pjfanning marked this pull request as draft December 1, 2023 15:41
@cgivre cgivre added security backport-to-stable This bug fix is applicable to the latest stable release and should be considered for inclusion there labels Dec 1, 2023
@cgivre cgivre changed the title DRILL-8465. check input data for iceberg plugin DRILL-8465: Check Input Data for Iceberg Plugin Dec 3, 2023
@pjfanning
Copy link
Contributor Author

pjfanning commented Dec 10, 2023

@cgivre I'm not that familiar with how Drill plugin configurations work. If I was to extend IcebergFormatPluginConfig to add a configurable allowPackageList (String[] or List<String> -- whichever works best) -- how would I go about accessing this from the IcebergWork class. It is IcebergWork where I need to apply the allow list?

@vvysotskyi you appear to have written most of the Iceberg code. Would you have any idea if this issue is one that we need to worry about? If it is, it looks like it will be hard to get the config values injected into IcebergWork because the class seems to only be instantiated by a custom Jackson Deserializer that itself only created only Java reflection.

@cgivre
Copy link
Contributor

cgivre commented Dec 10, 2023

@cgivre I'm not that familiar with how Drill plugin configurations work. If I was to extend IcebergFormatPluginConfig to add a configurable allowPackageList (String[] or List<String> -- whichever works best) -- how would I go about accessing this from the IcebergWork class. It is IcebergWork where I need to apply the allow list?

It think the thing to do would be to add an argument to the IcebergWork constructor. Then once that's done, it looks like the IcebergWork is instantiated in the IcebergGroupScan.

private List<IcebergWork> convertWorkList(List<IcebergCompleteWork> workList) {
return workList.stream()
.map(IcebergCompleteWork::getScanTask)
.map(IcebergWork::new)
.collect(Collectors.toList());
}

The GroupScan has access to the IcebergPlugin and from there you can access the IcebergConfig. Does that make sense?

@vvysotskyi you appear to have written most of the Iceberg code. Would you have any idea if this issue is one that we need to worry about? If it is, it looks like it will be hard to get the config values injected into IcebergWork because the class seems to only be instantiated by a custom Jackson Deserializer that itself only created only Java reflection.

@pjfanning
Copy link
Contributor Author

@cgivre unfortunately, IcebergWork is also created by IcebergWorkDeserializer but this class is constructed using reflection based on @JsonDeserialize(using = IcebergWork.IcebergWorkDeserializer.class).

There is no point in changing IcebergWork unless we can find a way to inject the config value into IcebergWorkDeserializer too.

@cgivre
Copy link
Contributor

cgivre commented Jan 3, 2024

@jnturton Do you have any thoughts here? This seems like this would be a good PR to get into the bug fix release.

@jnturton
Copy link
Contributor

jnturton commented Jan 5, 2024

I've started looking at this. First question: if we're adding dynamically loaded class checks to protect against untrusted code then is checking the package name worth much? Or do we need to do something like verify signatures against a list of trusted keys? Second question: if this is about security then is the code we're loading actually untrusted or is it only ever loaded from serialisations that we produced ourselves (e.g. in IcebergWorkSerializer)?

P.S. Please include this "Why we're doing this" background that I'm lacking in the Jira issue when it's nontrivial.

EDIT: I've just seen the security label on this PR so that gives some clue. There's also a Security "component" in Jira that we should add to the issue (and the background mentioned above)

@pjfanning
Copy link
Contributor Author

pjfanning commented Jan 5, 2024

The short background to this in this link - https://lists.apache.org/thread/vpjz467rg8449m63v1n9nl3o56twwyzt (a private thread requiring ASF login).

I'm no expert on Iceberg or the Drill Iceberg Plugin but I was hoping to maybe engage with someone who knows more about how they work and to get an understanding of whether we need some constraints. Due to the security aspect of this, I'm not too comfortable going into more detail here.

@jnturton
Copy link
Contributor

jnturton commented Jan 5, 2024

Got it @pjfanning. Let's discuss further in the right forum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-stable This bug fix is applicable to the latest stable release and should be considered for inclusion there security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants