-
Notifications
You must be signed in to change notification settings - Fork 148
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
Code quality improvements #929
Comments
Maybe use black to unify the code format? |
Good idea. I did a test run and it touches basically 1/3 of the code base. We will probably have to find a configuration that does not change as much files. On a similar note we should then also use isort for the imports. |
If we're not backporting patches and have a small PR queue we could just run it over the whole code base... |
Yeah you are right. I only backported very small bug fixes in the past. Once the style fixes are merged we can just make a PR changes the formatting. The IMA device mapper PR, does not change much existing code, so rebasing that is not an issue. |
W0602: #946 |
pyrightStatic code analysis reports several typing issues in most Python files. To reproduce, run any Python file through the |
@mdrocco thanks for introducing me to pyright. It seems that it has a big overlap with mypy. Do you have any experience using it and how does it compare to mypy? We should definitely start adding more type annotations, but this only makes sense for the part of Keylime that does not depend on the TPM abstraction layer, because most of that will be removed when we remove the Python agent. |
I have no experience with
|
Ok, good to know
Yes that one and most of the functions in tpm_main.py that are not used by the server components. If we have fixed enough of the warnings it make sense to integrate it into our pre-commit hooks. |
Sounds good, I will keep ignoring them.
I am about to complete my first batch of fixes, I will create a PR as soon as I am reasonably happy with it. |
@purujit thanks for taking these on. I'll have a closer look at the PR tomorrow. |
Hi @THS-on I would like to contribute to this project and found this issue to be a good starting point as this would be my first contribution to OSS (so please bare with me :)). |
@L77H welcome and awesome that you want to contribute to Keylime. I updated the list to be now fairly up to date. I suggest to start looking into the disabled warnings of the pylintrc and try to remove them |
@THS-on Thanks! I will get onto it! |
This issue tracks the different things that can be done to improve the code quality. If you want to fix some one of the issues please comment on this issue, so I can update it.
pylint
We currently have a large list of warnings that we ignore. Those have to be reviewed and removed where possible.
Current warnings:
See: https://github.com/keylime/keylime/blob/master/pylintrc
flake8 integration
Flake8 does catches some errors that pylint does not and vice versa. We should integrate it into our style checks.
List of potential useful plugins:
MyPy
We currently not enforce types. This makes it harder to reason about some functions and allows for subtile errors.
A strict mypy configuration currently returns about 2000 warnings that can probably removed by going through Keylime and adding type annotations.
Configuration used:
DONE
pyright
Static code analysis reports several typing issues in most Python files. Investigate if we should integrate it.
DONE
LGTM, semgrep and Bandit
There are multiple tools for analyzing the AST of the Python code and finding common security and code quality issues.
We should at least integrate one of them into our CI.
Fuzzing methods that take user controlled input with something like https://github.com/google/atheris can also be useful (mentioned by @kkaarreell).
TODOs
Converting dicts to classes
For the agent event loop we use a dict to store all the necessary state of an agent. This is not ideal because it is hard to reason about what is actually in this dict. We should convert this dict to a class that holds all the required state. For this class we can then also provide conversion functions from and to the ORM class.
WIP partly by #1523
Unit tests
Most of the older code in Keylime is only covered by the restful test or the e2e tests. We should add unit tests for this code and refactor it to make it more testable where necessary.
New code should be ideally only added with unit tests.
Automatic code formatting
The current code has no clear style guidelines. We should try to enforce a style with automated tooling.
TODOs:
Add pre-commit hook to enforce code style with isort and black #982
Removal of
config.REQUIRE_ROOT
With #900 merged this flag should be fully removed.
Removal of vTPM code
Originally Keylime supported vTPMs a feature from Xen. This is no longer the case and the deep quote feature is not implemented in swtpm. Most of the code is already removed but there is still some broken skeleton code in there which should be removed.
I think we can move the issue on how to trust the underlying hypervisor out of Keylime and into the platform that manages the hypervisor (e.g. OpenStack).
DONE
Removal of STUB TPM logic
There are some parts of Keylime that have code for canned values. There seems to be no documentation on how to use that code and newer parts of Keylime do not implement it. I think this code can be removed.
DONE
Removing dependency of the TPM abstraction for the registrar and verifier
Both components initialize an abstract TPM without actually using a TPM. The registrar uses a software implementation of
TPM_MakeCredential
and the verifier usestpm2_checkquote
andtpm2_eventlog
. keylime/enhancements#59 already proposes to move the quote verification code into a separate module which is more flexible and easier to test.Once the rust agent is the official one, the entire TPM abstraction code could then be removed.
DONE with the exception of tpm2_eventlog, python only parser is being worked on
The text was updated successfully, but these errors were encountered: