Code quality improvements #929

THS-on · 2022-03-21T15:40:50Z

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:

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:

- flake8-bugbear
- flake8-comprehensions
- flake8-simplify
- flake8-builtins

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:

[mypy]
plugins = sqlmypy
strict = True
follow_imports = silent
ignore_missing_imports = True

pyright

Static code analysis reports several typing issues in most Python files. Investigate if we should integrate it.

TODOs:

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

Integrate LGTM/GraphQL into the CI: ci: add CodeQL analysis #975
- have a look at the found issues: https://lgtm.com/projects/g/keylime/keylime/alerts/?mode=list
Integrate Bandit into the CI

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.

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:

Reformat code with black and add it to the CI pipeline
Add isort to CI
Add pre-commit hooks to make it easier to use
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.

remove REQUIRE_ROOT environment option #972

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

Full removal of vTPM support #974

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.

Remove stubbing support for TPM, VTPM and IMA #957

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 uses tpm2_checkquote and tpm2_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.

The text was updated successfully, but these errors were encountered:

aplanas · 2022-03-22T13:49:46Z

Maybe use black to unify the code format?

THS-on · 2022-03-22T14:06:13Z

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.

stefanberger · 2022-03-24T20:00:50Z

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

THS-on · 2022-03-24T20:27:08Z

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.

stefanberger · 2022-03-29T14:07:58Z

W0602: #946

mdrocco · 2022-10-18T13:47:48Z

pyright

Static code analysis reports several typing issues in most Python files. To reproduce, run any Python file through the pyright type analyzer.

THS-on · 2022-10-18T15:49:51Z

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

mdrocco · 2022-10-18T15:55:16Z

I have no experience with mypy, as I had no experience with type checking for Python whatsoever before starting with pyright. I am taking a first pass in the spirit of "the more type issues we fix, the better" and I should point out that:

most of the issues I am seeing (and hopefully fixing) do not pertain type annotations
I am avoiding keylime_agent.py for now as my understanding is it will be eventually replaced by the rust agent <- please correct me if this assumption is wrong

THS-on · 2022-10-18T16:04:57Z

most of the issues I am seeing (and hopefully fixing) do not pertain type annotations

Ok, good to know

I am avoiding keylime_agent.py for now as my understanding is it will be eventually replaced by the rust agent

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.

mdrocco · 2022-10-18T16:12:30Z

Yes that one and most of the functions in tpm_main.py that are not used by the server components.

Sounds good, I will keep ignoring them.

If we have fixed enough of the warnings it make sense to integrate it into our pre-commit hooks.

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 · 2023-10-04T18:53:34Z

@THS-on I took a stab at fixing some naming issues (C0103) in #1478. PTAL. This is my first OSS contribution. If it looks good, I'll merge and continue on the next batch.

THS-on · 2023-10-04T18:58:53Z

@purujit thanks for taking these on. I'll have a closer look at the PR tomorrow.

THS-on mentioned this issue Mar 21, 2022

Add type hints and initial Mypy configuration #930

Merged

THS-on added the good first issue label Mar 30, 2022

THS-on mentioned this issue Apr 1, 2022

Support push model for agent attestation keylime/enhancements#60

Closed

THS-on pinned this issue Jun 17, 2022

THS-on mentioned this issue Oct 18, 2022

Code improvement: fix Python typing issues #1131

Closed

This was referenced Oct 21, 2022

Upgrade minimum python version to 3.10 #1137

Closed

Chore fix type issues #1139

Closed

outstanding type issues #1143

Open

purujit mentioned this issue Oct 4, 2023

Fix pylint C0103 (naming) errors in some files #1478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code quality improvements #929

Code quality improvements #929

THS-on commented Mar 21, 2022 •

edited

aplanas commented Mar 22, 2022

THS-on commented Mar 22, 2022

stefanberger commented Mar 24, 2022

THS-on commented Mar 24, 2022

stefanberger commented Mar 29, 2022

mdrocco commented Oct 18, 2022

THS-on commented Oct 18, 2022

mdrocco commented Oct 18, 2022 •

edited

THS-on commented Oct 18, 2022

mdrocco commented Oct 18, 2022

purujit commented Oct 4, 2023

THS-on commented Oct 4, 2023

Code quality improvements #929

Code quality improvements #929

Comments

THS-on commented Mar 21, 2022 • edited

pylint

flake8 integration

MyPy

pyright

LGTM, semgrep and Bandit

Converting dicts to classes

Unit tests

Automatic code formatting

Removal of config.REQUIRE_ROOT

Removal of vTPM code

Removal of STUB TPM logic

Removing dependency of the TPM abstraction for the registrar and verifier

aplanas commented Mar 22, 2022

THS-on commented Mar 22, 2022

stefanberger commented Mar 24, 2022

THS-on commented Mar 24, 2022

stefanberger commented Mar 29, 2022

mdrocco commented Oct 18, 2022

pyright

THS-on commented Oct 18, 2022

mdrocco commented Oct 18, 2022 • edited

THS-on commented Oct 18, 2022

mdrocco commented Oct 18, 2022

purujit commented Oct 4, 2023

THS-on commented Oct 4, 2023

THS-on commented Mar 21, 2022 •

edited

Removal of `config.REQUIRE_ROOT`

mdrocco commented Oct 18, 2022 •

edited