RNP development guide

The following are a set of conventions and items that are relevant to contributors.

Contributing

Pull Requests

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 master 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 most appropriate method (see here).

Branches

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

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.

Reproducing Locally

If tests fail in CI, you may attempt to reproduce those locally via ctest command:

ctest -j4 -V -R rnp_tests

Or, more specific:

ctest -V -R cli_tests-Misc

If test fails under the specific OS, you should construct corresponding Docker container and run tests inside, taking Github workflows as a guide.

Code Coverage

CodeCov is used for assessing our test coverage. The current coverage can always be viewed here: https://codecov.io/github/rnpgp/rnp/

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.

The results can be accessed on https://scan.coverity.com/projects/rnpgp-rnp. 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.

# 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 "american fuzzy lop" ("AFL") or "libfuzzer" to find ways to improve the robustness of the code base.

Presently, rnp builds in "OSS-Fuzz" 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/

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

env CXX=clang++ CXXFLAGS="-fsanitize=address,undefined" LDFLAGS="-fsanitize=address,undefined" ./configure
make -j4
src/tests/rnp_tests

Here we are using the AddressSanitizer and UndefinedBehaviorSanitizer.

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:

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:

docker run --name=clang-format alpine:latest apk --no-cache add clang
docker commit clang-format clang-format
docker rm clang-format

You can then reformat a file (say, src/lib/crypto/bn.cpp) like so:

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:

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:

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\)\$"`

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 here.
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.
```
if (something) {
  return val;
}
```
Do use a default failure (not success) value for ret variables. Example:
```
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

int somefunc() {
  static char buffer[256];
  //...
}

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:

/** 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);

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:

RFC 1991: PGP Message Exchange Formats. Now obsolete, but may have some historical interest.
RFC 2440: OpenPGP Message Format. Superseded by RFC 4880.
RFC 4880: OpenPGP Message Format. Latest RFC available at the moment, however has a lot of suggested changes via RFC 4880bis
RFC 5581: The Camellia cipher in OpenPGP.
RFC 4880bis-09: OpenPGP Message Format. Latest suggested update to the RFC 4880.

More information sources:

OpenPGP Working Group mailing list. Here you can pick up all the latest discussions and suggestions regarding the update of RFC 4880
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
Algorithms: @randombit, @dewyatt, @flowher, @catap, @ni4
Performance: @catap, @ni4
CLI: @ni4
GnuPG compatibility: @MohitKumarAgniotri, @frank-trampe, @ni4
Security Testing/Analysis: @MohitKumarAgniotri, @flowher
Autotools: @randombit, @zgyarmati, @catap

Data formats:

OpenPGP Packet: @randombit, @catap, @ni4
Keystore: @catap
JSON: @zgyarmati
SSH: @ni4

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