search

microsoft / sarif-sdk

.NET code and supporting files for working with the 'Static Analysis Results Interchange Format' (SARIF, see https://github.com/oasis-tcs/sarif-spec)
Other
194 stars 93 forks source link

Allow validation command to accept plug-ins with additional validation rules #1868

Open ghost opened 4 years ago

ghost commented 4 years ago

@michaelcfanning FYI

ghost commented 4 years ago

The SARIF driver framework on which the validate command is built provides AnalyzeOptionsBase.PluginsFilePaths, but it's not used anywhere. @michaelcfanning FYI.

michaelcfanning commented 4 years ago

The BinSkim checker utilizes this for managing plug-ins. Or did historically.

ghost commented 4 years ago

There is one piece of plumbing missing.

The driver framework defines the abstract class PlugInDriverCommand<T> (where T is an options class), with a property DefaultPlugInAssemblies. The driver provides derived (but still abstract) classes ExportConfigurationCommandBase and ExportRulesMetadataCommandBase, which look for the things they want to export in DefaultPluginAssemblies.

BinSkim provides concrete derived classes ExportOptionsCommand and ExportRulesMetadataCommand that set DefaultPluginAssemblies to BinSkim itself, and so those commands just work.

The driver also provides AnalyzeCommandBase, which looks for skimmers (rules) in DefaultPlugInAssemblies.

The validate command sets DefaultPlugInAssemblies to the Multitool itself, and so it just works.

What's missing is that AnalyzeCommandOptionsBase provides PluginFilePaths, but the driver framework doesn't use it: it doesn't add those assemblies to the ones found in DefaultPluginAssemblies before it goes searching for skimmers.

michaelcfanning commented 4 years ago

OK. Good to know. In the short-term, let's not get too wrapped up in the plug-in model, which is a lower priority than getting the core validation going. I see where you're headed with this, you'd like to separate the validation rules by plug-in (to correspond to specific standards). Fair enough but not an immediate goal.

ghost commented 4 years ago

Yes, agreed. That is indeed why I was thinking about this, and coincidentally I ran across this issue while closing some old work items, which made me look at it more carefully. See my suggestion in email to use taxonomies for classifying validation rules.

michaelcfanning commented 4 years ago

Please continue to pursue the taxonomies idea. I'd like to see a working prototype if you're able to get something working. Note that an API that accepts a taxonomy and which post-processes a SARIF log file to filter/reclassify failure levels is one scenario. A second is to use a taxonomy to update the default rule configuration of a run. Obviously, remappings that require enablding 'disabled by default' rules or which overcome a lack of setting a verbose option (to see notes) would require option #2. Both are valuable.