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

Expose an API to make it easy to parse SSLyze's JSON output #487

Closed
dquitmann-op opened this issue Feb 3, 2021 · 7 comments
Closed

Expose an API to make it easy to parse SSLyze's JSON output #487

dquitmann-op opened this issue Feb 3, 2021 · 7 comments

Comments

@dquitmann-op
Copy link

Is your feature request related to a problem? Please describe.
We have a tool parsing SSLyze JSON output and giving us information about weaknesses. (Sidenote: we really like SSLyze for not doing this, as we think, interpretation is a "personal" decision).

Right now we read and parse the JSON and then handle it with multiple dict-accesses (json["scan_command_results"] or json.get("some_plugin") or if "some_plugin" in json:). This leads to regular errors and mistakes in our code base (either we try to access a key which is not there, as it might be optional, or we access a field by .get not realizing a typo and always get False). So we thought about how to improve and a natural way would be using a library like https://github.com/konradhalas/dacite to read the entire JSON structure back in the corresponding dataclasses. By this, we could write save and type-checked code afterwards and would easily be warned about access to optional or misspelled fields (by mypy or flake8).

I tried one example JSON yesterday and one big issue is the use of TypedDicts for ScanCommandResultsDict and ScanCommandExtraArgumentsDict:

class ScanCommandResultsDict(TypedDict, total=False):

class ScanCommandExtraArgumentsDict(TypedDict, total=False):

I just patched quick and dirty TypedDict handling in https://github.com/konradhalas/dacite and got it read at least the example I tested fully back to the _SslyzeOutputAsJson class. Very nice!

Describe the solution you'd like
I think, TypedDict handling for a library like https://github.com/konradhalas/dacite might not be really necessary and thus propose to ban TypedDicts in SSLyze and use dataclasses for the two mentioned containers like for all other container-classes in SSLyze.

Describe alternatives you've considered
Patching https://github.com/konradhalas/dacite as mentioned before.

Additional context
As on a first glance it looks like not a very big deal to me, I would be happy to create a PR for this in the upcoming weeks. But as I have a working "quick and dirty" solution right now, I first wanted to get in touch and ask, if there is any reason, why these classes are still TypedDicts, and if you would consider a PR to use dataclasses.

I would be happy about feedback and your thoughts on this @nabla-c0d3.

BR

@nabla-c0d3
Copy link
Owner

Hello,
Yes I would definitely consider removing the usage of TypedDict. I am not very happy with how it turned out, and it triggers typing errors when fetching the results.
We could even maybe maintain backward compatibility for now, by implementing the __getitem__()and items() methods on the corresponding dataclass.

@nabla-c0d3
Copy link
Owner

@dquitmann-op If you want, you can try this experimental branch: https://github.com/nabla-c0d3/sslyze/tree/%23487-stop-using-typed-dict

In this branch, the JSON output can be parsed using the following code:

from sslyze.cli.json_output import SslyzeOutputAsJson

result = SslyzeOutputAsJson.parse_file("result.json")
print(result)

A JSON Schema can also be generated:

print(SslyzeOutputAsJson.schema_json(indent=2))

Let me know your thoughts if you have any. Thanks!

@nabla-c0d3 nabla-c0d3 changed the title End using TypedDict and only use dataclasses Expose an API to make it easy to parse SSLyze's JSON output Apr 4, 2021
nabla-c0d3 added a commit that referenced this issue Apr 12, 2021
Fix some tests

Fix tests

Add tests and JSON schema

Fix requirements

Fix imports
@dquitmann-op
Copy link
Author

Sorry for the delay (in working on my approach as well as on answering). Was quite busy with other work. I will have a look at your branch in the next days and give feedback. On first look: this is obviously the much cleaner approch!

nabla-c0d3 added a commit that referenced this issue Apr 18, 2021
Fix some tests

Fix tests

Add tests and JSON schema

Fix requirements

Fix imports

Update documentation
@dquitmann-op
Copy link
Author

I just tested the dev-5.0.0 branch with a few quick test cases and this works like a charme. Obviously, using pydantic-Models is much more robust and clean than my initial hacky approach.

We like this very much, as it allows us to access the data in our code base in a type-checkable and comprehendible manner. How do you plan to put this forward if you have to change the models? Will one version just be able to parse it's output or do you plan some legacy-handling (at least for some time)?

@nabla-c0d3
Copy link
Owner

We like this very much, as it allows us to access the data in our code base in a type-checkable and comprehendible manner. How do you plan to put this forward if you have to change the models? Will one version just be able to parse it's output or do you plan some legacy-handling (at least for some time)?

Good question, For now I'm only planning on following Semantic Versioning, ie. a version of SSLyze will be able to parse JSON from any version that has the same major version number (ie. 6.1.0 can parse JSON from 6.0.0, but not from 5.x.x).

@dquitmann-op
Copy link
Author

dquitmann-op commented Apr 30, 2021

I think this sounds good. We would not expect more. We will evaluate and think about building some kind of legacy-support, when we get problems. If we do this in a clean way (and not just quick and hacky) we will come back to you and see, if you do want to implement it in SSLyze globally.

@nabla-c0d3
Copy link
Owner

Released in v5.0.0.

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

Successfully merging a pull request may close this issue.

2 participants