From: Alberto Leiva Popper Date: Thu, 23 May 2019 20:56:00 +0000 (-0500) Subject: Remove the incidence framework X-Git-Tag: v0.0.2~23 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c719f7a79ea307b609be0747d1a080b3452917d7;p=thirdparty%2FFORT-validator.git Remove the incidence framework The two incidences I had planned to include have been resolved as "wontfix," basically: 1. A re-read of RFC 3370 has revealed that whether the parameters field is supposed to be absent or NULL is completely ambiguous, so we'll accept both now. 2. As for rsaEncryption vs sha256WithRSAEncryption for public keys, the relevant sidr mailing list thread is currently favoring the former. And the vast majority of the global RPKI does the same, so there's no error to silence. --- diff --git a/docs/doc/incidence.md b/docs/doc/incidence.md deleted file mode 100644 index be97eb56..00000000 --- a/docs/doc/incidence.md +++ /dev/null @@ -1,91 +0,0 @@ ---- -title: Incidence ---- - -[Documentation](index.html) > {{ page.title }} - -# {{ page.title }} - -## Index - -1. [Introduction](#introduction) -2. [`incidences` definition](#incidences-definition) -3. [Incidence types](#incidence-types) - 1. [rsaEncryption signature algorithm has parameters](#rsaencryption-signature-algorithm-has-parameters) - 2. [certificate public key algorithm is rsaEncryption](#certificate-public-key-algorithm-is-rsaencryption) - -## Introduction - -The RPKI RFCs define fairly strict profiles for RPKI objects, and are unequivocal in stating that incorrectly-formed objects are supposed to be rejected by Relying Party validation. In practice, however, this does not stop a significant amount of Certificate Authorities from issuing incorrect objects. - -By default, Fort is as pedantic as it can reasonably be. The `incidence` section of its configuration file is a means to modify its behavior upon encountering profile violations that, from experience, are often overlooked. - -## `incidences` definition - -`incidences` is a JSON array that contains (anonymous) incidence elements. Here's an example: - -``` -"incidences": [ - { - "name": "rsaEncryption signature algorithm has parameters", - "action": "warn" - }, { - "name": "certificate public key algorithm is rsaEncryption", - "action": "ignore" - } -] -``` - -`name` is the identifier of an incidence. It's case-sensitive and developer-defined. It states the particular error condition that will be handled by the remaining field. - -`action` is an enumeration that states the outcome of a violation of the corresponding incidence. It can take one of three values: - -1. `error`: Print error message in `error` log level, fail validation of the offending object (and all of its children). -2. `warn`: Print error message in `warning` log level, continue validation as if nothing happened. -3. `ignore`: Do not print error message, continue validation as if nothing happened. - -By Fort's pedantic nature, most incidences have an `action` of `error` by default. - -## Incidence types - -Presently, there are only two incidence types defined. This list might grow over time, depending on the state of the global RPKI and user demand. - -### rsaEncryption signature algorithm has parameters - -[RFC 6488](https://tools.ietf.org/html/rfc6488) (RPKI Signed Objects) defers signature algorithm specification to RFC 6485: - -``` -2.1.6.5. signatureAlgorithm - - The signatureAlgorithm MUST conform to the RPKI Algorithms and Key - Size Profile specification [RFC6485]. -``` - -[6485](https://tools.ietf.org/html/rfc6485) has been obsoleted by [7935](https://tools.ietf.org/html/rfc7935), which states the following: - -``` - RPKI implementations MUST - accept either rsaEncryption or sha256WithRSAEncryption for the - SignerInfo signatureAlgorithm field when verifying CMS SignedData - objects (for compatibility with objects produced by implementations - conforming to [RFC6485]). -``` - -Regarding `rsaEncryption`, [3370](https://tools.ietf.org/html/rfc3370) states - -``` - When the rsaEncryption algorithm identifier is used, the - AlgorithmIdentifier parameters field MUST contain NULL. -``` - -As of 2019-05-21, many signed objects in the global RPKI break this rule. (`parameters` is often defined as an empty object, but not NULL nonetheless.) - -If not `ignore`d, Fort will report this incidence with the following error message: - -``` -: : rsaEncryption signature algorithm has parameters. -``` - -### certificate public key algorithm is rsaEncryption - -> TODO missing code diff --git a/docs/doc/index.md b/docs/doc/index.md index 0dd1f817..9111656c 100644 --- a/docs/doc/index.md +++ b/docs/doc/index.md @@ -10,4 +10,3 @@ title: Documentation Index 4. [Running Fort](run.html) 5. [Fort usage](usage.html) 6. [SLURM](slurm.html) -7. [Incidences](incidence.html) diff --git a/docs/doc/usage.md b/docs/doc/usage.md index 03bdf1e9..62d4a49c 100644 --- a/docs/doc/usage.md +++ b/docs/doc/usage.md @@ -34,7 +34,6 @@ command: fort 17. [`rsync.program`](#rsyncprogram) 18. [`rsync.arguments-recursive`](#rsyncarguments-recursive) 19. [`rsync.arguments-flat`](#rsyncarguments-flat) - 20. [`incidences`](#incidences) ## Syntax @@ -365,14 +364,7 @@ The configuration options are mostly the same as the ones from the `argv` interf "$REMOTE", "$LOCAL" ] - }, - - "incidences": [ - { - "name": "rsaEncryption signature algorithm has parameters", - "action": "ignore" - } - ] + } } @@ -440,10 +432,3 @@ Fort will replace `"$REMOTE"` with the remote URL it needs to download, and `"$L Arguments needed by [`rsync.program`](#rsyncprogram) to perform a single-file rsync. Fort will replace `"$REMOTE"` with the remote URL it needs to download, and `"$LOCAL"` with the target local directory where the file is supposed to be dropped. - -### `incidences` - -- **Type:** JSON Object -- **Availability:** JSON only - -A listing of actions to be performed by validation upon encountering certain error conditions. See [incidence](incidence.html). diff --git a/src/Makefile.am b/src/Makefile.am index 40a8d9b1..d14e2319 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -41,7 +41,6 @@ fort_SOURCES += asn1/signed_data.h asn1/signed_data.c fort_SOURCES += config/boolean.c config/boolean.h fort_SOURCES += config/filename_format.h config/filename_format.c -fort_SOURCES += config/incidences.h config/incidences.c fort_SOURCES += config/str.c config/str.h fort_SOURCES += config/string_array.h config/string_array.c fort_SOURCES += config/sync_strategy.h config/sync_strategy.c @@ -57,8 +56,6 @@ fort_SOURCES += data_structure/common.h fort_SOURCES += data_structure/uthash.h fort_SOURCES += data_structure/uthash_nonfatal.h -fort_SOURCES += incidence/incidence.h incidence/incidence.c - fort_SOURCES += object/certificate.h object/certificate.c fort_SOURCES += object/crl.h object/crl.c fort_SOURCES += object/ghostbusters.h object/ghostbusters.c diff --git a/src/algorithm.c b/src/algorithm.c index 9b671717..493aed78 100644 --- a/src/algorithm.c +++ b/src/algorithm.c @@ -1,5 +1,6 @@ #include "algorithm.h" +#include #include #include "log.h" @@ -20,13 +21,14 @@ int validate_certificate_public_key_algorithm(int nid) { /* - * TODO (whatever) RFC says sha256WithRSAEncryption, but everyone uses - * rsaEncryption. + * RFC says sha256WithRSAEncryption, but current IETF concensus (and + * practice) say that the right one is rsaEncryption. + * https://mailarchive.ietf.org/arch/browse/sidr/ */ - if (nid == NID_rsaEncryption || nid == NID_sha256WithRSAEncryption) + if (nid == NID_rsaEncryption) return 0; - return pr_err("Certificate's public key format is NID '%d', not RSA nor RSA+SHA256.", + return pr_err("Certificate's public key format is NID '%d', not rsaEncryption.", nid); } @@ -86,6 +88,20 @@ incorrect_oid: return pr_err("The hash algorithm of the '%s' is not SHA256.", what); } +static int +is_asn1_null(ANY_t *any) +{ + if (any == NULL) + return true; + + if (any->size != 2) + return false; + if (any->buf[0] != 5 || any->buf[1] != 0) + return false; + + return true; +} + int validate_cms_signature_algorithm(AlgorithmIdentifier_t *id) { @@ -101,7 +117,6 @@ validate_cms_signature_algorithm(AlgorithmIdentifier_t *id) 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, }; uint8_t last; - int error; if (id == NULL) return pr_err("The signature algorithm is absent."); @@ -119,6 +134,8 @@ validate_cms_signature_algorithm(AlgorithmIdentifier_t *id) goto incorrect_oid; /* + * This one's a royal mess. + * * RFC 7935: * The object identifier and parameters for rsaEncryption * [RFC3370] MUST be used for the SignerInfo signatureAlgorithm field @@ -132,13 +149,35 @@ validate_cms_signature_algorithm(AlgorithmIdentifier_t *id) * When any of these four object identifiers appears within an * AlgorithmIdentifier, the parameters MUST be NULL. Implementations * MUST accept the parameters being absent as well as present. + * + * I don't know if "MUST contain NULL" means that it must be absent, + * or whether it must contain a NULL ASN object. Everyone is doing the + * latter, which you think would be the logical option, but then 3370 + * throws this curveball at us: + * + * There are two possible encodings for the SHA-1 AlgorithmIdentifier + * parameters field. The two alternatives arise from the fact that when + * the 1988 syntax for AlgorithmIdentifier was translated into the 1997 + * syntax, the OPTIONAL associated with the AlgorithmIdentifier + * parameters got lost. Later the OPTIONAL was recovered via a defect + * report, but by then many people thought that algorithm parameters + * were mandatory. Because of this history some implementations encode + * parameters as a NULL element and others omit them entirely. The + * correct encoding is to omit the parameters field; however, + * implementations MUST also handle a SHA-1 AlgorithmIdentifier + * parameters field which contains a NULL. + * + * It does seem to be talking about SHA-1 only, but it's just not clear, + * because it doesn't care to ellaborate in the rsaEncryption section at + * all. Even though it's aware of the ambiguity, it just says "MUST + * contain NULL." It doesn't say "MUST be empty" or "MUST contain a NULL + * object." + * + * I really don't think this is worth making a fuss over, so we'll just + * accept both. */ - if (last == 1 && id->parameters != NULL) { - error = incidence(INID_RSAENCRYPTION_SIGNALG_HAS_PARAMS, - "rsaEncryption signature algorithm has parameters."); - if (error) - return error; - } + if (id->parameters != NULL && !is_asn1_null(id->parameters)) + return pr_err("The signature algorithm has parameters."); return 0; diff --git a/src/config.c b/src/config.c index 9b107355..192d4efb 100644 --- a/src/config.c +++ b/src/config.c @@ -11,7 +11,6 @@ #include "json_handler.h" #include "log.h" #include "config/boolean.h" -#include "config/incidences.h" #include "config/str.h" #include "config/uint.h" #include "config/uint32.h" @@ -272,15 +271,6 @@ static const struct option_field options[] = { .doc = "File name variant to print during debug/error messages", }, - /* Incidences */ - { - .id = 4001, - .name = "incidences", - .type = >_incidences, - .doc = "Override actions on validation errors", - .availability = AVAILABILITY_JSON, - }, - { 0 }, }; diff --git a/src/config/incidences.c b/src/config/incidences.c deleted file mode 100644 index cc068164..00000000 --- a/src/config/incidences.c +++ /dev/null @@ -1,22 +0,0 @@ -#include "config/incidences.h" - -#include -#include "incidence/incidence.h" - -static void -incidences_print(struct option_field const *field, void *_value) -{ - incidence_print(); -} - -static int -incidences_parse_json(struct option_field const *opt, json_t *json, - void *_result) -{ - return incidence_update(json); -} - -const struct global_type gt_incidences = { - .print = incidences_print, - .parse.json = incidences_parse_json, -}; diff --git a/src/config/incidences.h b/src/config/incidences.h deleted file mode 100644 index 9e731b5f..00000000 --- a/src/config/incidences.h +++ /dev/null @@ -1,8 +0,0 @@ -#ifndef SRC_CONFIG_INCIDENCES_H_ -#define SRC_CONFIG_INCIDENCES_H_ - -#include "config/types.h" - -extern const struct global_type gt_incidences; - -#endif /* SRC_CONFIG_INCIDENCES_H_ */ diff --git a/src/incidence/incidence.c b/src/incidence/incidence.c deleted file mode 100644 index dff05049..00000000 --- a/src/incidence/incidence.c +++ /dev/null @@ -1,162 +0,0 @@ -#include "incidence/incidence.h" - -#include -#include -#include -#include "common.h" -#include "json_parser.h" -#include "log.h" -#include "data_structure/common.h" - -struct incidence { - const enum incidence_id id; - char const *const name; - const enum incidence_action default_action; - enum incidence_action action; -}; - -static struct incidence incidences[__INID_MAX] = { - { - INID_RSAENCRYPTION_SIGNALG_HAS_PARAMS, - "rsaEncryption signature algorithm has parameters", - INAC_ERROR, - }, -}; - -static int -name2id(char const *name, enum incidence_id *id) -{ - array_index i; - - for (i = 0; i < __INID_MAX; i++) { - if (strcmp(name, incidences[i].name) == 0) { - *id = i; - return 0; - } - } - - return pr_err("Unknown incidence name: %s", name); -} - -static char const * -action2str(enum incidence_action action) -{ - switch (action) { - case INAC_IGNORE: - return "ignore"; - case INAC_WARN: - return "warn"; - case INAC_ERROR: - return "error"; - } - - return "unknown"; -} - -static int -init_action(json_t *json) -{ - enum incidence_id id; - char const *name; - char const *action_str; - enum incidence_action action; - int error; - - error = json_get_string(json, "name", &name); - if (error) - return error; - error = name2id(name, &id); - if (error) - return error; - error = json_get_string(json, "action", &action_str); - if (error) - return error; - - if (strcmp("ignore", action_str) == 0) - action = INAC_IGNORE; - else if (strcmp("warn", action_str) == 0) - action = INAC_WARN; - else if (strcmp("error", action_str) == 0) - action = INAC_ERROR; - else - return pr_err("Unknown incidence action: '%s'", action_str); - - if (action > incidences[id].action) - return pr_err("The '%s' incidence cannot have a more severe action than '%s'.", - name, action2str(incidences[id].action)); - - incidences[id].action = action; - return 0; -} - -/** - * Concurrent inits are allowed. - */ -int -incidence_init(void) -{ - array_index i; - - /* Make sure the programmer didn't desync the id enum and the array. */ - assert(__INID_MAX == ARRAY_LEN(incidences)); - for (i = 0; i < __INID_MAX; i++) { - assert(i == incidences[i].id); - /* Also init. */ - incidences[i].action = incidences[i].default_action; - } - - return 0; -} - -/** - * Concurrent calls to this function are allowed. - */ -int -incidence_update(json_t *json) -{ - array_index i; - json_t *child; - int error; - - if (!json_is_array(json)) - return pr_err("The incidences JSON element is supposed to be an array."); - - json_array_foreach(json, i, child) { - error = init_action(child); - if (error) - return error; - } - - return 0; -} - -void -incidence_print(void) -{ - array_index i; - bool printed; - - pr_info("Custom incidences:"); - pr_indent_add(); - - printed = false; - - for (i = 0; i < __INID_MAX; i++) { - if (incidences[i].action != incidences[i].default_action) { - pr_info("%s: %s", incidences[i].name, - action2str(incidences[i].action)); - printed = true; - } - } - - if (!printed) - pr_info(""); - - pr_indent_rm(); -} - -enum incidence_action -incidence_get_action(enum incidence_id id) -{ - return incidences[id].action; -} diff --git a/src/incidence/incidence.h b/src/incidence/incidence.h deleted file mode 100644 index 7b53284d..00000000 --- a/src/incidence/incidence.h +++ /dev/null @@ -1,40 +0,0 @@ -#ifndef SRC_INCIDENCE_INCIDENCE_H_ -#define SRC_INCIDENCE_INCIDENCE_H_ - -#include - -/* - * Note: If you need to add, modify or delete an element from this enum, - * remember that you also need to add it to the incidences array. That's all. - */ -enum incidence_id { - INID_RSAENCRYPTION_SIGNALG_HAS_PARAMS, - - __INID_MAX, -}; - -enum incidence_action { - /** - * Do not print error message, continue validation as if nothing - * happened. - */ - INAC_IGNORE, - /** - * Print error message in warning log level, continue validation as if - * nothing happened. - */ - INAC_WARN, - /** - * Print error message in error log level, fail validation of the - * offending object (and all of its children). - */ - INAC_ERROR, -}; - -int incidence_init(void); /* incidence_destroy() is not needed. */ -int incidence_update(json_t *); - -void incidence_print(void); -enum incidence_action incidence_get_action(enum incidence_id); - -#endif /* SRC_INCIDENCE_INCIDENCE_H_ */ diff --git a/src/log.c b/src/log.c index 118e1497..eba8efe5 100644 --- a/src/log.c +++ b/src/log.c @@ -268,30 +268,3 @@ pr_crit(const char *format, ...) PR_SUFFIX(STDERR); return -EINVAL; } - -/** - * Prints the [format, ...] error message using the configured logging severity - * of the @id incidence. - */ -int -incidence(enum incidence_id id, const char *format, ...) -{ - enum incidence_action action; - va_list args; - - action = incidence_get_action(id); - switch (action) { - case INAC_IGNORE: - return 0; - case INAC_WARN: - PR_PREFIX(STDERR, COLOR_WARNING, "WRN", args); - PR_SUFFIX(STDERR); - return 0; - case INAC_ERROR: - PR_PREFIX(STDERR, COLOR_ERROR, "ERR", args); - PR_SUFFIX(STDERR); - return -EINVAL; - } - - return pr_crit("Unknown incidence action: %u", action); -} diff --git a/src/log.h b/src/log.h index f7f2e3a6..466a4cfc 100644 --- a/src/log.h +++ b/src/log.h @@ -1,8 +1,6 @@ #ifndef SRC_LOG_H_ #define SRC_LOG_H_ -#include "incidence/incidence.h" - /* * I know that the OpenBSD style guide says that we shouldn't declare our own * error printing functions, but we kind of need to do it: @@ -61,8 +59,6 @@ int crypto_err(const char *, ...) CHECK_FORMAT(1, 2); int pr_enomem(void); int pr_crit(const char *, ...) CHECK_FORMAT(1, 2); -int incidence(enum incidence_id, const char *, ...) CHECK_FORMAT(2, 3); - #define PR_DEBUG printf("%s:%d (%s())\n", __FILE__, __LINE__, __func__) #define PR_DEBUG_MSG(msg, ...) printf("%s:%d (%s()): " msg "\n", \ __FILE__, __LINE__, __func__, ##__VA_ARGS__) diff --git a/src/main.c b/src/main.c index 4c129c4b..22426ed1 100644 --- a/src/main.c +++ b/src/main.c @@ -33,9 +33,6 @@ main(int argc, char **argv) print_stack_trace_on_segfault(); error = thvar_init(); - if (error) - return error; - error = incidence_init(); if (error) return error; diff --git a/src/object/certificate.c b/src/object/certificate.c index 1570d2dc..3f1cbd5a 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -214,6 +214,7 @@ validate_public_key(X509 *cert, bool is_root) int ok; int error; + /* Reminder: X509_PUBKEY is the same as SubjectPublicKeyInfo. */ pubkey = X509_get_X509_PUBKEY(cert); if (pubkey == NULL) return crypto_err("X509_get_X509_PUBKEY() returned NULL");