Revision control
Copy as Markdown
Other Tools
= RNP development guide
The following are a set of conventions and items that are relevant to
contributors.
== Contributing
=== Pull Requests
See also: https://github.com/thoughtbot/guides/tree/main/code-review[Thoughtbot’s Code Review guide]
Pull Requests should be used for any non-trivial changes. This presents
an opportunity for feedback and allows the CI tests to complete prior to
merging.
The `main` branch should generally always be in a buildable and
functional state.
Pull Requests should be:
* Focused. Do not include changes that are unrelated to the main purpose
of the PR.
* As small as possible. Sometimes large pull requests may be necessary
for adding complex features, but generally they should be kept as small
as possible to ensure a quick and thorough review process.
* Related to a GH issue to which you are assigned. If there is none,
file one (but search first!). This ensures there is no duplication of
effort and allows for a discussion prior to beginning work.
(This may not be necessary for PRs that are purely documentation updates)
* Approved by **2** reviewers before merging.
(Updates related to policies, like this section, should be approved by
the project owner)
* Merged by a reviewer via the `rebase and merge`
=== Tests
All newly added functionality should be covered by tests—either GTest-based
API tests (see the rnp_tests target) or CLI tests (see cli_tests.py).
It may also be helpful to check the Codecov report to identify any uncovered lines or cases.
=== Coding style
In general, the coding style should follow the existing modern C++ code rather than
the legacy C-style remnants. For example, prefer rnp::Key over pgp_key_t,
use .hpp headers instead of .h, and so on.
=== Branches
Git branches should be used generously. Most branches should be topic branches,
created for adding a specific feature or fixing a specific bug.
Keep branches short-lived (treat them as disposable/transient) and try to
avoid long-running branches.
A good example of using a branch would be:
* User `@joe` notices a bug where a NULL pointer is dereferenced during
key export. He creates GH issue `#500`.
* He creates a new branch to fix this bug named
`joe-500-fix-null-deref-in-pgp_export_key`.
* Joe commits a fix for the issue to this new branch.
* Joe creates a Pull Request to merge this branch in to main.
* Once merged, Joe deletes the branch since it is no longer useful.
Branch names may vary but should be somewhat descriptive, with words
separated by hyphens. It is also helpful to start the branch name with
your GitHub username, to make it clear who created the branch and
prevent naming conflicts.
Remember that branch names may be preserved permanently in the commit
history of `main`, depending on how they are merged.
=== Commits
* Try to keep commits as small as possible. This may be difficult or
impractical at times, so use your best judgement.
* Each commit should be buildable and should pass all tests. This helps
to ensure that git bisect remains a useful method of pinpointing issues.
* Commit messages should follow 50/72 rule.
* When integrating pull requests, merge function should be preferred over
squashing. From the other hand, developers should squash commits and
create meaningful commit stack before PR is merged into mainstream branch.
Merging commits like "Fix build" or "Implement comments from code review"
should be avoided.
== Continuous Integration (Github Actions)
Github actions are used for continuously testing new commits and pull requests.
Those include testing for different operating systems, linting via clang-format and shellcheck,
and code coverage and quality checks via `Codecov` and `CodeQL`.
For Github workflows sources see `.github/workflows/` folder and scripts from the `ci/` folder.
Also there is a Cirrus CI runner, configuration for which is stored in `.cirrus.yml`.
Downstream consumption of the installed library (CMake `find_package`, pkg-config, raw
compiler flags, against both shared and static builds) is smoke-tested by
`ci/tests/downstream-consumer.sh`; see link:packaging.adoc[Consuming librnp in downstream projects]
for the corresponding consumer documentation.
=== Reproducing Locally
If tests fail in CI, you may attempt to reproduce those locally via `ctest` command:
[source,console]
--
ctest -j4 -V -R rnp_tests
--
Or, more specific:
[source,console]
--
ctest -V -R cli_tests-Misc
python3 ../rnp/src/tests/cli_tests.py -v -d Misc.test_armor_with_spaces_import
--
If test fails under the specific OS, you may use corresponding Docker container which is used by the CI.
We maintain a bunch of containers which install all the prerequisites in this repository: https://github.com/rnpgp/rnp-ci-containers
Sample building and running process could be as following:
[source,console]
--
# Start docker, mounting rnp sources directory
docker run -it -v $(pwd):/opt/rnp:ro ghcr.io/rnpgp/ci-rnp-fedora-38-amd64 bash
# Configure and build rnp, adding additional options like backend selection or enabled sanitizers if needed
cd /opt
cmake -B build -DBUILD_SHARED_LIBS=On -DCRYPTO_BACKEND=Botan ./rnp
cmake --build build --parallel "$(nproc)"
# Run the tests. Please note that some tests would fail if run under the root as they check permissions.
# Please see how this is handled in centos-and-fedora.yml or other workflows.
ctest --parallel $(nproc) --test-dir build --output-on-failure
--
== Code Coverage
CodeCov is used for assessing our test coverage.
== Security / Bug Hunting
=== Static Analysis
==== Coverity Scan
Coverity Scan is used for static analysis of the code base.
It is run daily on the main branch via the Github actions.
See `.github/workflows/coverity.yml` for the details.
You will need to create an account and request access to the rnpgp/rnp project.
Since the scan results are not updated live, line numbers may no longer
be accurate against the `main` branch, issues may already be resolved,
etc.
==== Clang Static Analyzer
Clang includes a useful static analyzer that can also be used to locate
potential bugs.
Note: It is normal for the build time to increase significantly when using this static analyzer.
[source,console]
--
# it's important to start fresh for this!
rm -rf build && mkdir build && cd build
scan-build cmake .. && scan-build make -j8
[...]
scan-build: 61 bugs found.
scan-build: Run 'scan-view /tmp/scan-build-2018-09-17-085354-22998-1' to examine bug reports.
--
Then use `scan-view`, as indicated above, to start a web server and use
your web browser to view the results.
=== Dynamic Analysis
==== Fuzzing
It is often useful to utilize a fuzzer like
ways to improve the robustness of the code base.
Presently, rnp builds in
and certain fuzzers are enabled there.
In the `src/fuzzing` directory, we have the fuzzers that run in OSS-Fuzz.
Setting `-DENABLE_SANITIZERS=1 -DENABLE_FUZZERS=1` will build these fuzzers
with the libfuzzer engine; and running the resulting executables will perform
the fuzzing.
To build and run fuzzers locally, or reproduce an issue, see https://google.github.io/oss-fuzz/advanced-topics/reproducing/
===== Further Reading
* AFL's `README`, `parallel_fuzzing.txt`, and other bundled documentation.
* See https://fuzzing-project.org/tutorial3.html[Tutorial: Instrumented fuzzing with american fuzzy lop]
==== Clang Sanitizer
Clang and GCC both support a number of sanitizers that can help locate
issues in the code base during runtime.
To use them, you should rebuild with the sanitizers enabled, and then
run the tests (or any executable):
[source,console]
--
env CXX=clang++ CXXFLAGS="-fsanitize=address,undefined" LDFLAGS="-fsanitize=address,undefined" ./configure
make -j4
src/tests/rnp_tests
--
Here we are using the
and
This will produce output showing any memory leaks, heap overflows, or
other issues.
== Code Conventions
C is a very flexible and powerful language. Because of this, it is
important to establish a set of conventions to avoid common problems and
to maintain a consistent code base.
=== Code Formatting
`clang-format` (v11.0.0) can be used to format the code base, utilizing
the `.clang-format` file included in the repository.
==== clang-format git hook
A git pre-commit hook exists to perform this task automatically, and can
be enabled like so:
[source,console]
--
cd rnp
git-hooks/enable.sh
--
If you do not have clang-format v11.0.0 available, you can use a docker
container for this purpose by setting `USE_DOCKER="yes"` in
`git-hooks/pre-commit.sh`.
This should generally work if you commit from the command line.
Note that if you have unstaged changes on some of the files you are
attempting to commit, which have formatting issues detected, you will
have to resolve this yourself (the script will inform you of this).
If your commit does not touch any `.c`/`.h` files, you can skip the
pre-commit hook with git's `--no-verify`/`-n` option.
==== clang-format (manually)
If you are not able to use the git hook, you can run `clang-format`
manually in a docker container.
Create a suitable container image with:
[source,console]
--
docker run --name=clang-format alpine:latest apk --no-cache add clang
docker commit clang-format clang-format
docker rm clang-format
--
How to use pre-built docker container from the linter action please see below.
You can then reformat a file (say, `src/lib/crypto/bn.cpp`) like so:
[source,console]
--
cd rnp
docker run --rm -v $PWD:/rnp -w /rnp clang-format clang-format -style=file -i src/lib/crypto/bn.cpp
--
Also you may wish to reformat all modified uncommitted files:
[source,console]
--
docker run --rm -v $PWD:/rnp -w /rnp clang-format clang-format -style=file -i `git ls-files -m |grep "\.\(c\|h\|cpp\)\$"`
--
...or files, modified since referenced commit, say `54c5476`:
[source,console]
--
docker run --rm -v $PWD:/rnp -w /rnp clang-format clang-format -style=file -i `git diff --name-only 54c5476..HEAD |grep "\.\(c\|h\|cpp\)\$"`
--
==== clang-format (manually, using the docker container from the clang-format-lint-action)
Build container:
[source,console]
--
docker build -t clang-format-lint github.com/DoozyX/clang-format-lint-action
--
Get a diff with formatting errors:
[source,console]
--
docker run -it --rm --workdir /src -v $(pwd):/src clang-format-lint --clang-format-executable /clang-format/clang-format11.0.0 -r --exclude .git .
--
To edit files in-place, fixing the formatting errors, you should add `--inplace` parameter:
[source,console]
--
docker run -it --rm --workdir /src -v $(pwd):/src clang-format-lint --clang-format-executable /clang-format/clang-format11.0.0 -r --exclude .git . --inplace true
--
=== Style Guide
In order to keep the code base consistent, we should define and adhere
to a single style.
When in doubt, consult the existing code base.
==== Naming
The following are samples that demonstrate the style for naming
different things.
* Functions: `some_function`
* Variables: `some_variable`
* Filenames: `packet-parse.c` `packet-parse.h`
* Struct: `pgp_key_t`
* Typedefed Enums: `pgp_pubkey_alg_t`
* Enum Values: `PGP_PKA_RSA = 1`
* Constants (macro): `RNP_BUFSIZ`
==== General Guidelines
Do:
* Do use header guards (`#ifndef SOME_HEADER_H [...]`) in headers.
* Do use `sizeof(variable)`, rather than `sizeof(type)`. Or
`sizeof(*variable)` as appropriate.
* Do use commit messages that close GitHub issues automatically, when
applicable. `Fix XYZ. Closes #78.` See
* Do declare functions `static` when they do not need to be referenced
outside the current source file.
* Do always use braces for conditionals, even if the block only contains a
single statement.
+
[source,c]
--
if (something) {
return val;
}
--
* Do use a default failure (not success) value for `ret` variables. Example:
+
[source,c]
--
rnp_result_t ret = RNP_ERROR_GENERIC;
// ...
return ret;
--
Do not:
* Do not use the static storage class for local variables, *unless* they
are constant.
+
**Not OK**
+
[source,c]
--
int somefunc() {
static char buffer[256];
//...
}
--
+
**OK**
+
[source,c]
--
int somefunc() {
static const uint16_t some_data[] = {
0x00, 0x01, 0x02, //...
};
}
--
* Do not use `pragma`, and try to avoid `__attribute__` as well.
* Do not use uninitialized memory. Try to ensure your code will not cause any errors in valgrind and other memory checkers.
==== Documentation
Documentation is done in Doxygen comments format, which must be put in header files.
Exception are static or having only definition functions - it is not required to document them,
however if they are documented then this should be done in the source file and using the @private tag.
Comments should use doxygen markdown style, like the following example:
[source,c]
--
/** Some comments regarding the file purpose, like 'PGP packet parsing utilities'
* @file
*/
/** brief description of the sample function which does something, keyword 'brief' is omitted
* Which may be continued here
*
* After an empty line you may add detailed description in case it is needed. You may put
* details about the memory allocation, what happens if function fails and so on.
*
* @param param1 first parameter, null-terminated string which should not be NULL
* @param param2 integer, some number representing something
* @param size number of bytes available to store in buffer
* @param buffer buffer to store results, may be NULL. In this case size can be used to
* obtain the required buffer length
* @return 0 if operation succeeds, or error code otherwise. If operation succeeds then buffer
* is populated with the resulting data, and size contains the length of this data.
* if error code is E_BUF_TOOSMALL then size will contain the required size to store
* the result
**/
rnp_result_t
rnp_do_operation(const char *param1, const int param2, int *size, char *buffer);
--
== Man pages
Man pages are generated from AsciiDoc sources (`src/*/[name].[1-9].adoc`).
Two toolchains are supported, and every man page source must build and
render identically with both (`ci/check-man-pages.sh` enforces this in CI,
modeled on git's dual-toolchain documentation check):
* `asciidoctor -b manpage` (preferred, no extra dependencies), or
* classic Python `asciidoc` (asciidoc-py) via docbook XML and
`xsltproc`/docbook-xsl, using `cmake/Modules/adoc-manpage.xsl`.
CMake picks `asciidoctor` when available and falls back to
`asciidoc` + `xsltproc`; `ASCIIDOC_TOOL` forces either one. When neither
is installed, pre-generated pages from `docs/man/` are installed instead;
refresh them with `ci/regen-man-pages.sh` (always with asciidoctor) after
changing a man page source.
To keep the sources compatible with both toolchains, stick to their
common subset:
* The NAME section must start with the page's own name
(e.g. `rnp - ...` for `rnp.1.adoc`); docbook-xsl names its output
after the first refname.
* Prefer literal blocks (`----`) over `+` hard line breaks for verbatim
content such as headers and code; docbook-xsl drops `+` breaks inside
paragraphs unless they are plain text.
* Do not nest quotes inside emphasis markers (`*"text"*`, `*"a@b.com"*`):
the tools disagree on where the span ends.
* Avoid the `user@domain` and `<user@domain>` email shapes in running
text: asciidoctor turns them into email macros while asciidoc-py
leaves them as plain text. Wrap them in matched emphasis
(`_a@b.com_`) or monospace (`` `a@b.com` ``) instead.
== OpenPGP protocol specification
During development you'll need to reference OpenPGP protocol and related documents.
Here is the list of RFCs and Internet Drafts available at the moment:
* https://www.ietf.org/rfc/rfc1991.txt[RFC 1991]: PGP Message Exchange Formats. Now obsolete, but may have some historical interest.
* https://www.ietf.org/rfc/rfc4880.txt[RFC 4880]: OpenPGP Message Format. Latest RFC available at the moment, however has a lot of suggested changes via RFC 4880bis
* https://www.ietf.org/id/draft-ietf-openpgp-rfc4880bis-09.txt[RFC 4880bis-09]: OpenPGP Message Format. Latest suggested update to the RFC 4880.
More information sources:
* https://mailarchive.ietf.org/arch/browse/openpgp/[OpenPGP Working Group mailing list]. Here you can pick up all the latest discussions and suggestions regarding the update of RFC 4880
* https://gitlab.com/openpgp-wg/rfc4880bis[OpenPGP Working Group gitlab]. Latest work on RFC update is available here.
== Reviewers and Responsibility areas
The individuals are responsible for the following areas of `rnp`.
When submitting a Pull Request please seek reviews by whoever is
responsible according to this list.
General:
* Code style: @dewyatt, @ni4
* Algorithms: @randombit, @dewyatt, @catap, @ni4
* Performance: @catap, @ni4
* CLI: @ni4
* GnuPG compatibility: @ni4
* Security Testing/Analysis: @ni4
* CMake: @ni4
* CI/CD: @maxirmx, @ni4
Data formats:
* OpenPGP Packet: @ni4
* Keystore: @catap, @maxirmx
Bindings:
* FFI: @dewyatt
* Ruby: @dewyatt
* Java/JNI: @catap
* Obj-C/Swift: @ni4
* Python: @dewyatt, @ni4
Platforms:
* RHEL/CentOS: @dewyatt
* BSD:
* Windows: @rrrooommmaaa
* macOS / iOS / Homebrew: @ni4
* Debian: @zgyarmati