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

High level wrapper for parsing binaries #77

Merged
merged 9 commits into from Sep 16, 2022
Merged

Conversation

Shnatsel
Copy link
Member

Performs the parsing in a single function call instead of requiring assembling the pieces yourself and also doing your own error handling.

Resolves #50

@Shnatsel
Copy link
Member Author

@pinkforest you wanted to use this, so let me know if this fits your use cases.

It still needs docs but other than that should be good to go.

@Shnatsel Shnatsel marked this pull request as ready for review September 16, 2022 22:43
@Shnatsel Shnatsel merged commit 5684765 into master Sep 16, 2022
@Shnatsel Shnatsel deleted the high-level-wrapper branch September 16, 2022 22:57
@Shnatsel
Copy link
Member Author

I'll keep refining it in-tree, but I still do want feedback on this.

@@ -1,8 +1,8 @@
[workspace]

members = [
"audit-info",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought this was going to be auditable-info ?

Copy link
Member Author

@Shnatsel Shnatsel Sep 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted a clean separation between the high-level crate and the low-level ones, but maybe this will just create confusion with the rust-audit-info binary.

We could still switch if you think auditable-info is better.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think auditable-info is best around those considerations re: confusion.

@@ -0,0 +1,21 @@
[package]
name = "audit-info"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are to change to auditable-info in case or another have to change here as well

@pinkforest
Copy link

btw what we'll be doing with the public API as I think whilst the raw_ etc. fn's are a okay for MVP we can probably design something more sugary that doesn't care about raw data

@Shnatsel
Copy link
Member Author

The raw_* functions are needed for rust-audit-info which doesn't perform JSON parsing/validation, so that step is unnecessary. It would be perhaps nice to return a String for clarity, but that performs a potentially useless UTF-8 validation.

Perhaps I should indeed make it return String - that will be a lot easier to understand, and the UTF-8 validation overhead should not be too big. And if someone needs a lower-level interface, the modular crates are right there.

@pinkforest
Copy link

pinkforest commented Sep 17, 2022

Hm. Maybe I need to explain what I would expect from a high level auditable-info API -

The high level API should be kept with consumer in mind and it should be easy to consume e.g. provide Iterables and like

Consumer should not need to care about messing with dependency tree structure and stuff like that or even care about whether the data structure is JSON or something else - that is implementation detail to auditable.

I can understand it for the low-level API but if we are to provide high-level API then it should be something like this:

Maybe one could use JSON Path ? but then that ties into a data format which is supposed to be implementation detail

info = auditable::Info::from(T); // T generic can be `File` | `OssString` | ..

info.path('json_path')

Above would assume auditable crate would have Info interface / re-exports from auditable-info on it's public interface

@Shnatsel
Copy link
Member Author

Based on the experience with integrating it into cargo-audit and Trivy, the API of just exposing the format directly appears to be the right level of abstraction. The scanners then convert it into their native data structures, which are fairly close to the JSON format. And since the format is stable, just exposing the format is the most API-stable thing we can do.

@Shnatsel
Copy link
Member Author

I believe you are correct about this not really being a high-level API, but I don't have any use cases for a high-level API, so I cannot design an API that will actually be useful that way. So for now I'm leaving it at the level of abstraction that is useful in cargo audit, rust-audit-info and Trivy.

@pinkforest
Copy link

Yeah I think we need consumer use-cases in order to design that - if any. correct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a single function that extracts audit data from an executable
2 participants