crypto-selftests: test CFB8 ciphers with different chunksizes
[gd/gnutls] / CONTRIBUTING.md
1 # GnuTLS -- Information about our contribution rules and coding style
2
3  Anyone is welcome to contribute to GnuTLS. You can either take up
4 tasks from our [planned list](https://gitlab.com/gnutls/gnutls/milestones),
5 or suprise us with enhancement we didn't plan for. In all cases be prepared
6 to defend and justify your enhancements, and get through few rounds
7 of changes. 
8
9 We try to stick to the following rules, so when contributing please
10 try to follow them too.
11
12
13 # Git commits:
14
15 Note that when contributing code you will need to assert that the contribution is
16 in accordance to the "Developer's Certificate of Origin" as found in the 
17 file [DCO.txt](doc/DCO.txt).
18
19 To indicate that, make sure that your contributions (patches or merge requests),
20 contain a "Signed-off-by" line, with your real name and e-mail address. 
21 To automate the process use "git am -s" to produce patches and/or set the
22 a template to simplify this process, as follows.
23
24 ```
25 $ cp devel/git-template ~/.git-template
26 [edit]
27 $ git config commit.template ~/.git-template
28 ```
29
30
31 # Test suite:
32
33    New functionality should be accompanied by a test case which verifies
34 the correctness of GnuTLS' operation on successful use of the new
35 functionality, as well as on fail cases. The GnuTLS test suite is run on "make check"
36 on every system GnuTLS is installed, except for the tests/suite part
37 which is only run during development.
38
39 For testing functionality of gnutls we use two test unit testing frameworks:
40 1. The gnutls testing framework as in [utils.h](tests/utils.h), usually for high level
41    tests such as testing a client against a server. See [set_x509_key_mem.c](tests/set_x509_key_mem.c).
42 2. The cmocka unit testing framework, for unit testing of functions
43    or interfaces. See [dtls-sliding-window.c](tests/dtls-sliding-window.c).
44
45 Certificates for testing purposes are available at [cert-common.h](tests/cert-common.h).
46 Note that we do not regenerate test certificates when they expire, but
47 we rather fix the test's time using datefudge or gnutls_global_set_time_function().
48 For example, see [x509cert-tl.c](tests/x509cert-tl.c).
49
50
51 # File names:
52
53   Files are split to directories according to the subsystem
54 they belong to. Examples are x509/, minitasn1/, openpgp/,
55 opencdk/ etc. The files in the root directory related
56 to the main TLS protocol implementation.
57
58
59 # C dialect:
60
61   While parts of GnuTLS were written in older dialects, new code
62 in GnuTLS are expected to conform to C99. Exceptions could be made
63 for C99 features that are not supported in popular platforms on a
64 case by case basis.
65
66
67 # Indentation style:
68
69  In general, use [the Linux kernel coding style](https://www.kernel.org/doc/html/latest/process/coding-style.html).
70 You may indent the source using GNU indent, e.g. "indent -linux *.c".
71
72
73 # Commenting style
74
75 In general for documenting new code we prefer self-documented code to comments. That is:
76   - Meaningful function and macro names
77   - Short functions which do a single thing
78
79 That does not mean that no comments are allowed, but that when they are
80 used, they are used to document something that is not obvious, or the protocol
81 expectations. Though we haven't followed that rule strictly in the past, it
82 should be followed on new code.
83
84
85 # Function names:
86
87   All the function names use underscore ```_```, to separate words,
88 functions like ```gnutlsDoThat``` are not used. The exported function names
89 usually start with the ```gnutls_``` prefix, and the words that follow
90 specify the exact subsystem of gnutls that this function refers to.
91 E.g. ```gnutls_x509_crt_get_dn```, refers to the X.509
92 certificate parsing part of gnutls. Some of the used prefixes are the
93 following.
94  * ```gnutls_x509_crt_``` for the X.509 certificate part
95  * ```gnutls_session_``` for the TLS session part (but this may be omited)
96  * ```gnutls_handshake_``` for the TLS handshake part
97  * ```gnutls_record_``` for the TLS record protocol part
98  * ```gnutls_alert_``` for the TLS alert protocol part
99  * ```gnutls_credentials_``` for the credentials structures
100  * ```gnutls_global_``` for the global structures handling
101
102 All exported API functions must be listed in libgnutls.map
103 in order to be exported.
104
105 Internal functions, i.e, functions that are not exported in the API but
106 are used internally by multiple files, should be prefixed with an underscore.
107 For example `_gnutls_handshake_begin()`.
108
109 Internal functions restricted to a file (static), or inline functions, should
110 not use the `_gnutls` prefix for simplicity, e.g., `get_version()`.
111
112 Internal structures should not be exported. Especially pointers to
113 internal data. Doing so harms future reorganization/rewrite of subsystems.
114 They can however be used by unit tests in tests/ directory; in that
115 case they should be part of the GNUTLS_PRIVATE_3_4 tag in libgnutls.map.
116
117
118 # Header guards
119
120   Each private C header file SHOULD have a header guard consisting of the
121 project name and the file path relative to the project directory, all uppercase.
122
123 Example: `lib/srp.h` uses the header guard `GNUTLS_LIB_SRP_H`.
124
125 The header guard is used as first and last effective code in a header file,
126 like e.g. in lib/srp.h:
127
128 ```
129 #ifndef GNUTLS_LIB_SRP_H
130 #define GNUTLS_LIB_SRP_H
131
132 ...
133
134 #endif /* GNUTLS_LIB_SRP_H */
135 ```
136
137 The public header files follow a similar convention but use the relative
138 install directory as template, e.g. `GNUTLS_GNUTLS_H` for `gnutls/gnutls.h`.
139
140
141 # Introducing new functions / API
142
143   Prior to introducing any new API consider all options to offer the same
144 functionality without introducing a new function. The reason is that  we want
145 to avoid breaking the ABI, and thus we cannot typically remove any function
146 that was added (though we have few exceptions). Since we cannot remove, it
147 means that experimental APIs, or helper APIs that are not typically needed
148 may become a burden to maintain in the future. That is, they may prevent
149 a refactoring, or require to keep legacy code.
150
151 As such, some questions to answer before adding a new API:
152  * Is this API useful for a large class of applications, or is it limited
153    to few?
154  * If it is limited to few, can we work around that functionality without
155    a new API?
156  * Would that function be relevant in the future when a new protocol such TLS
157    13.0 is made available? Would it harm the addition of a new protocol?
158
159
160 The make rule 'abi-check' verifies that the ABI remained compatible since
161 the last tagged release. It relies on the git tree and abi-compliance-checker.
162
163 The above do not apply to the C++ library; this library's ABI should not
164 be considered stable.
165
166
167 # Introducing new features / modifying behavior
168
169   When a new feature is introduced which may affect already deployed code, 
170 it must be disabled by default. For example a new TLS extension should be
171 enabled when explicitly requested by the application. That can happen for
172 example with a gnutls_init() flag.
173
174 The same should be followed when an existing function behavior is modified
175 in a way that may break existing applications which use the API in a
176 reasonable way. If the existing function allows flags, then a new flag
177 should be introduced to enable the new behavior.
178
179 When it is necessary, or desireable to enable the new features by default
180 (e.g., TLS1.3 introduction), the "next" releases should be used (and
181 introduced if necessary), to allow the modification to be tested for an
182 extended amount of time (see the [Release policy](RELEASES.md)).
183
184
185 # API documentation
186
187 When introducing a new API, we provide the function documentation as
188 inline comments, in a way that it can be used to generate a man-page
189 and be included in our manual. For that we use gnome-style comments
190 as in the example below. The detailed form is documented on `doc/scripts/gdoc`.
191
192 /**
193  * gnutls_init:
194  * @session: is a pointer to a #gnutls_session_t type.
195  * @flags: indicate if this session is to be used for server or client.
196  *
197  * This function initializes the provided session. Every
198  * session must be initialized before use, and must be deinitialized
199  * after used by calling gnutls_deinit().
200  *
201  * @flags can be any combination of flags from %gnutls_init_flags_t.
202  *
203  * Note that since version 3.1.2 this function enables some common
204  * TLS extensions such as session tickets and OCSP certificate status
205  * request in client side by default. To prevent that use the %GNUTLS_NO_EXTENSIONS
206  * flag.
207  *
208  * Returns: %GNUTLS_E_SUCCESS on success, or a negative error code.
209  **/
210
211
212 # Constructed types:
213
214   The constructed types in gnutls always have the ```gnutls_``` prefix.
215 Definitions, value defaults and enumerated values should be in
216 capitals. E.g. ```GNUTLS_CIPHER_3DES_CBC```.
217
218 Structures should have the ```_st``` suffix in their name even
219 if they are a typedef. One can use the sizeof() on types with 
220 ```_st``` as suffix to get the structure's size.
221
222 Other constructed types should have the ```_t``` suffix. A pointer
223 to a structure also has the ```_t``` suffix.
224
225
226 # Function parameters:
227
228 The gnutls functions accept parameters in the order:
229  1. Input parameters
230  2. Output parameters
231
232 When data and size are expected as input, a const gnutls_datum_t structure
233 should be used (or more precisely a pointer to the structure).
234
235 When data pointer and size are to be returned as output, a gnutls_datum_t
236 structure should be used.
237
238 When output is to be copied to caller an array of fixed data should be
239 provided.
240
241
242 # Callback function parameters:
243
244  Callback functions should be avoided, if this is possible. 
245 Callbacks that refer to a TLS session should include the
246 current session as a parameter, in order for the called function to
247 be able to retrieve the data associated with the session.
248 This is not always done though -- see the push/pull callbacks.
249
250
251 # Return values:
252
253  Functions in gnutls return an int type, when possible. In that
254 case 0 should be returned in case of success, or maybe a positive
255 value, if some other indication is needed.
256
257 A negative value always indicates failure. All the available
258 error codes are defined in gnutls.h and a description
259 is available in gnutls_errors.c
260
261
262 Functions which are intended to return a boolean value should return
263 a type of bool, and it is recommended to contain the string '_is_'
264 on its function name; e.g.,
265 ```
266 bool _gnutls_is_not_prehashed();
267 ```
268
269 That allows the distinguishing functions that return negative errors
270 from boolean functions to both the developer and the compiler. Note
271 that in the past the 'unsigned' type was used to distinguish boolean functions
272 and several of these still exist.
273
274 ## Selecting the right return value
275
276 When selecting the return value for a TLS protocol parsing function
277 a suggested approach is to check which alert fits best on that error
278 (see `alert.c`), and then select from the error codes which are mapped
279 to that alert (see `gnutls_error_to_alert()`). For more generic parsing
280 errors consider using the `GNUTLS_E_RECEIVED_ILLEGAL_PARAMETER`.
281
282
283 # Usage of assert()
284
285  The assert() macro --not to be confused with gnutls_assert()-- is used
286 exceptionally on impossible situations to assist static analysis tools.
287 That is, it should be used when the static analyzer used in CI (currently
288 clang analyzer), detects an error which is on an impossible situation.
289 In these cases assert() is used to rule out that case.
290
291 For example in the situation where a pointer is known to be non-null,
292 but the static analyzer cannot rule it out, we use code like the following:
293 ```
294 assert(ptr != NULL);
295 ptr->deref = 3;
296 ```
297
298 Since GnuTLS is a library no other uses of assert() macro are acceptable.
299
300 The NDEBUG macro is not used in GnuTLS compilation, so the assert() macros
301 are always active.
302
303
304 # Gnulib
305
306 The directories `gl/`, `src/gl/` and `lib/unistring` contain gnulib files
307 copied/created by `./bootstrap`. Gnulib is a portability source code library
308 to handle API or behavior incompatibilities between target systems.
309
310 To take advantage of the latest gnulib files, we have to update the
311 `gnulib/` submodule from time to time:
312 ```
313 $ make glimport
314 ```
315
316 Note that the gnulib library in `gl/` is used by the GnuTLS library
317 and is kept separate from the gnulib used by the GnuTLS tools because
318 of license issues, and also to prevent any gnulib networking modules
319 from entering the library (gnulib networking re-implements the windows
320 network stack and causes issues to gnutls applications running on windows).
321
322
323 # Compiler warnings
324
325 The compiler prints warnings for several reasons; these warnings are
326 also not constant in time, different versions of the same compiler may
327 warn about different issues.
328
329 In GnuTLS we enable as many as possible warnings available in the compiler
330 via configure.ac. On certain cases however we silence or disable warnings
331 and the following subsections go case by case.
332
333 ## Switch unintended fall-through warnings
334
335 These we silence by using the macro FALLTHROUGH under a switch
336 statement which intentionally falls through. Example:
337 ```
338 switch (session->internals.recv_state) {
339         case RECV_STATE_DTLS_RETRANSMIT:
340                 ret = _dtls_retransmit(session);
341                 if (ret < 0)
342                         return gnutls_assert_val(ret);
343
344                 session->internals.recv_state = RECV_STATE_0;
345                 FALLTHROUGH;
346         case RECV_STATE_0:
347
348                 _dtls_async_timer_check(session);
349                 return 1;
350 }
351 ```
352
353
354 # Symbol and library versioning
355
356  The library uses the libtool versioning system, which in turn
357 results to a soname bump on incompatible changes. That is described
358 in [hooks.m4](m4/hooks.m4). Despite its complexity that system is
359 only sufficient to distinguish between versions of the library that
360 have broke ABI (i.e., soname bump occurred).
361
362 Today however, soname versioning isn't sufficient. Symbol versioning
363 as provided by [libgnutls.map](lib/libgnutls.map) have several
364 advantages.
365  * they allow for symbol clashing between different gnutls library
366    versions being in the same address space.
367  * they allow installers to detect the library version used for
368    an application utilizing a specific symbol
369  * the allow introducing multiple versions of a symbol a la libc,
370    keeping the semantics of old functions while introducing new.
371
372 As such for every symbol introduced on a particular version, we
373 create an entry in libgnutls.map based on the version and containing
374 the new symbols. For example, if in version 3.6.2 we introduce symbol
375 ```gnutls_xyz```, the entry would be:
376
377 GNUTLS_3_6_2 {
378   global:
379         gnutls_xyz;
380 } GNUTLS_3_6_1;
381
382 where ```GNUTLS_3_6_1``` is the last version that symbols were introduced,
383 and indicates a dependency of 3.6.2 to symbols of 3.6.1.
384
385 Note that when the soname version is bumped, i.e., the ABI is broken
386 all the previous symbol versions should be combined into a single. For
387 example on the 3.4.0 soname bump, all symbols were put under the
388 GNUTLS_3_4 version.
389
390 Backporting new symbols to an old version which is soname compatible
391 is not allowed (can cause quite some problems). Backporting symbols
392 to an incompatible soname version is allowed, but must ensure that
393 the symbol version used for the backported symbol version is distinct from
394 the original library symbol version. E.g., if symbol ```gnutls_xyz```
395 with version GNUTLS_3_6_3 is backported on gnutls 3.3.15, it should
396 use version GNUTLS_3_3_15.
397
398
399 # Auto-generated files:
400  Several parts of the documentation and the command line tools parameters
401 files (.def) are auto-generated. Normally when introducing new functions,
402 or adding new command line options to tools you need to run 'make
403 files-update', review the output (when feasible) and commit it separately,
404 e.g., with a message: "auto-generated files update".
405
406
407 # Guile bindings:
408
409  Parts of the Guile bindings, such as types (aka. "SMOBs"), enum values,
410 constants, are automatically generated.  This is handled by the modules
411 under `guile/modules/gnutls/build/'; these modules are only used at
412 build-time and are not installed.
413
414 The Scheme variables they generate (e.g., constants, type predicates,
415 etc.) are exported to user programs through `gnutls.scm' and
416 `gnutls/extra.scm', both of which are installed.
417
418 For instance, when adding/removing/renaming enumerates or constants,
419 two things must be done:
420
421  1. Update the enum list in `build/enums.scm' (currently dependencies
422     are not tracked, so you have to run "make clean all" in `guile/'
423     after).
424
425  2. Update the export list of `gnutls.scm' (or `extra.scm').
426
427 Note that, for constants and enums, "schemefied" names are used, as
428 noted under the "Guile API Conventions" node of the manual.
429
430 # Automated testing
431
432  GnuTLS primarily relies on gitlab-ci which is configured in .gitlab-ci.yml
433 file in the repository. The goal is to have a test suite which runs for
434 every new merge request prior to merging. There are no particular rules for
435 the test targets, except for them being reliable and running in a reasonable
436 timeframe (~1 hour).
437
438
439 # Reviewing code
440
441  A review as part of the gitlab merge requests, is a way to prevent errors due to
442 these guidelines not being followed, e.g., verify there is a reasonable test suite,
443 and whether it covers reasonably the new code, that the function naming is
444 consistent with these guidelines, as well as check for obvious mistakes in the new
445 code.
446
447 The intention is to keep reviews lightweight, and rely on CI for tasks such
448 as compiling and testing code and features.
449
450 A proposed checklist to assist such reviews follows.
451  * [ ] Any issues marked for closing are addressed
452  * [ ] There is a test suite reasonably covering new functionality or modifications
453  * [ ] Function naming, parameters, return values, types, etc., are consistent and according to `CONTRIBUTION.md`
454  * [ ] This feature/change has adequate documentation added
455  * [ ] No obvious mistakes in the code
456
457
458 [Guidelines to consider when reviewing.](https://github.com/thoughtbot/guides/tree/master/code-review)