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

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

[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
http://lcamtuf.coredump.cx/afl/["american fuzzy lop" ("AFL")] or
https://llvm.org/docs/LibFuzzer.html["libfuzzer"] to find
ways to improve the robustness of the code base.

Presently, rnp builds in
https://github.com/google/oss-fuzz/tree/master/projects/rnp["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/

===== 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
https://clang.llvm.org/docs/AddressSanitizer.html[AddressSanitizer]
and
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html[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:

[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
  https://help.github.com/articles/closing-issues-via-commit-messages/[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.
+
[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);
--

== 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/rfc2440.txt[RFC 2440]: OpenPGP Message Format. Superseded by RFC 4880.
* 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://tools.ietf.org/rfc/rfc5581.txt[RFC 5581]: The Camellia cipher in OpenPGP.
* 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