DRILL-8465: Check Input Data for Iceberg Plugin

[pjfanning]

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]

@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]

@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.

drill/contrib/format-iceberg/src/main/java/org/apache/drill/exec/store/iceberg/IcebergGroupScan.java

Lines 236 to 241 in fe57fd1

	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]

@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]

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

[jnturton]

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]

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]

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