diff options
Diffstat (limited to 'docs/develop.adoc')
| -rw-r--r-- | docs/develop.adoc | 435 |
1 files changed, 435 insertions, 0 deletions
diff --git a/docs/develop.adoc b/docs/develop.adoc new file mode 100644 index 0000000..e0c0e41 --- /dev/null +++ b/docs/develop.adoc @@ -0,0 +1,435 @@ += 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/master/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 `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 https://github.com/rnpgp/guides/tree/master/protocol/git[here]). + +=== Branches + +See also: https://github.com/rnpgp/guides/tree/master/protocol/git[Guides - Protocol / Git] + +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 `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: + +[source,console] +-- +ctest -j4 -V -R rnp_tests +-- + +Or, more specific: + +[source,console] +-- +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. + +[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` (v9.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 v9.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 +-- + +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\)\$"` +-- + +=== 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 +* 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 |