From cf30e7d0d1a0ff16569e76f210efbcefeface83e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 20 Apr 2020 10:30:54 +0200 Subject: [PATCH] Use switch instead of if when evaluating curves Previously, the code would do: REQUIRE(alg == CURVE1 || alg == CURVE2); [...] if (alg == CURVE1) { /* code for CURVE1 */ } else { /* code for CURVE2 */ } This approach is less extensible and also more prone to errors in case the initial REQUIRE() is forgotten. The code has been refactored to use: REQUIRE(alg == CURVE1 || alg == CURVE2); [...] switch (alg) { case CURVE1: /* code for CURVE1 */; break; case CURVE2: /* code for CURVE2 */; break; default: INSIST(0); } --- lib/dns/pkcs11ecdsa_link.c | 95 ++++++++++++++++++++++++++------------ lib/dns/pkcs11eddsa_link.c | 82 ++++++++++++++++++++++---------- 2 files changed, 123 insertions(+), 54 deletions(-) diff --git a/lib/dns/pkcs11ecdsa_link.c b/lib/dns/pkcs11ecdsa_link.c index 8e28a022d9..dbbdc48b65 100644 --- a/lib/dns/pkcs11ecdsa_link.c +++ b/lib/dns/pkcs11ecdsa_link.c @@ -179,12 +179,18 @@ pkcs11ecdsa_sign(dst_context_t *dctx, isc_buffer_t *sig) { key->key_alg == DST_ALG_ECDSA384); REQUIRE(ec != NULL); - if (key->key_alg == DST_ALG_ECDSA256) { + switch (key->key_alg) { + case DST_ALG_ECDSA256: dgstlen = ISC_SHA256_DIGESTLENGTH; siglen = DNS_SIG_ECDSA256SIZE; - } else { + break; + case DST_ALG_ECDSA384: siglen = DNS_SIG_ECDSA384SIZE; dgstlen = ISC_SHA384_DIGESTLENGTH; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } PK11_RET(pkcs_C_DigestFinal, (pk11_ctx->session, digest, &dgstlen), @@ -293,10 +299,16 @@ pkcs11ecdsa_verify(dst_context_t *dctx, const isc_region_t *sig) { key->key_alg == DST_ALG_ECDSA384); REQUIRE(ec != NULL); - if (key->key_alg == DST_ALG_ECDSA256) { + switch (key->key_alg) { + case DST_ALG_ECDSA256: dgstlen = ISC_SHA256_DIGESTLENGTH; - } else { + break; + case DST_ALG_ECDSA384: dgstlen = ISC_SHA384_DIGESTLENGTH; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } PK11_RET(pkcs_C_DigestFinal, (pk11_ctx->session, digest, &dgstlen), @@ -419,19 +431,24 @@ pkcs11ecdsa_compare(const dst_key_t *key1, const dst_key_t *key2) { } #define SETCURVE() \ - if (key->key_alg == DST_ALG_ECDSA256) { \ + switch (key->key_alg) { \ + case DST_ALG_ECDSA256: \ attr->pValue = isc_mem_get(key->mctx, \ sizeof(PK11_ECC_PRIME256V1)); \ memmove(attr->pValue, PK11_ECC_PRIME256V1, \ sizeof(PK11_ECC_PRIME256V1)); \ attr->ulValueLen = sizeof(PK11_ECC_PRIME256V1); \ - } else { \ + break; \ + case DST_ALG_ECDSA384: \ attr->pValue = isc_mem_get(key->mctx, \ sizeof(PK11_ECC_SECP384R1)); \ - \ memmove(attr->pValue, PK11_ECC_SECP384R1, \ sizeof(PK11_ECC_SECP384R1)); \ attr->ulValueLen = sizeof(PK11_ECC_SECP384R1); \ + break; \ + default: \ + INSIST(0); \ + ISC_UNREACHABLE(); \ } #define FREECURVE() \ @@ -532,10 +549,16 @@ pkcs11ecdsa_generate(dst_key_t *key, int unused, void (*callback)(int)) { memset(pk11_ctx, 0, sizeof(*pk11_ctx)); isc_mem_put(key->mctx, pk11_ctx, sizeof(*pk11_ctx)); - if (key->key_alg == DST_ALG_ECDSA256) { + switch (key->key_alg) { + case DST_ALG_ECDSA256: key->key_size = DNS_KEY_ECDSA256SIZE * 4; - } else { + break; + case DST_ALG_ECDSA384: key->key_size = DNS_KEY_ECDSA384SIZE * 4; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } return (ISC_R_SUCCESS); @@ -607,10 +630,16 @@ pkcs11ecdsa_todns(const dst_key_t *key, isc_buffer_t *data) { REQUIRE(key->keydata.pkey != NULL); - if (key->key_alg == DST_ALG_ECDSA256) { + switch (key->key_alg) { + case DST_ALG_ECDSA256: len = DNS_KEY_ECDSA256SIZE; - } else { + break; + case DST_ALG_ECDSA384: len = DNS_KEY_ECDSA384SIZE; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } ec = key->keydata.pkey; @@ -643,10 +672,16 @@ pkcs11ecdsa_fromdns(dst_key_t *key, isc_buffer_t *data) { REQUIRE(key->key_alg == DST_ALG_ECDSA256 || key->key_alg == DST_ALG_ECDSA384); - if (key->key_alg == DST_ALG_ECDSA256) { + switch (key->key_alg) { + case DST_ALG_ECDSA256: len = DNS_KEY_ECDSA256SIZE; - } else { + break; + case DST_ALG_ECDSA384: len = DNS_KEY_ECDSA384SIZE; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } isc_buffer_remainingregion(data, &r); @@ -664,20 +699,8 @@ pkcs11ecdsa_fromdns(dst_key_t *key, isc_buffer_t *data) { attr = ec->repr; attr->type = CKA_EC_PARAMS; - if (key->key_alg == DST_ALG_ECDSA256) { - attr->pValue = isc_mem_get(key->mctx, - sizeof(PK11_ECC_PRIME256V1)); - memmove(attr->pValue, PK11_ECC_PRIME256V1, - sizeof(PK11_ECC_PRIME256V1)); - attr->ulValueLen = sizeof(PK11_ECC_PRIME256V1); - } else { - attr->pValue = isc_mem_get(key->mctx, - sizeof(PK11_ECC_SECP384R1)); + SETCURVE(); - memmove(attr->pValue, PK11_ECC_SECP384R1, - sizeof(PK11_ECC_SECP384R1)); - attr->ulValueLen = sizeof(PK11_ECC_SECP384R1); - } attr++; attr->type = CKA_EC_POINT; attr->pValue = isc_mem_get(key->mctx, len + 3); @@ -945,10 +968,16 @@ pkcs11ecdsa_parse(dst_key_t *key, isc_lex_t *lexer, dst_key_t *pub) { dst__privstruct_free(&priv, mctx); memset(&priv, 0, sizeof(priv)); - if (key->key_alg == DST_ALG_ECDSA256) { + switch (key->key_alg) { + case DST_ALG_ECDSA256: key->key_size = DNS_KEY_ECDSA256SIZE * 4; - } else { + break; + case DST_ALG_ECDSA384: key->key_size = DNS_KEY_ECDSA384SIZE * 4; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } return (ISC_R_SUCCESS); @@ -1061,10 +1090,16 @@ pkcs11ecdsa_fromlabel(dst_key_t *key, const char *engine, const char *label, } key->label = isc_mem_strdup(key->mctx, label); - if (key->key_alg == DST_ALG_ECDSA256) { + switch (key->key_alg) { + case DST_ALG_ECDSA256: key->key_size = DNS_KEY_ECDSA256SIZE * 4; - } else { + break; + case DST_ALG_ECDSA384: key->key_size = DNS_KEY_ECDSA384SIZE * 4; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } pk11_return_session(pk11_ctx); diff --git a/lib/dns/pkcs11eddsa_link.c b/lib/dns/pkcs11eddsa_link.c index 4d3397ce74..1794128732 100644 --- a/lib/dns/pkcs11eddsa_link.c +++ b/lib/dns/pkcs11eddsa_link.c @@ -137,10 +137,16 @@ pkcs11eddsa_sign(dst_context_t *dctx, isc_buffer_t *sig) { key->key_alg == DST_ALG_ED448); REQUIRE(ec != NULL); - if (key->key_alg == DST_ALG_ED25519) { + switch (key->key_alg) { + case DST_ALG_ED25519: siglen = DNS_SIG_ED25519SIZE; - } else { + break; + case DST_ALG_ED448: siglen = DNS_SIG_ED448SIZE; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } pk11_ctx = isc_mem_get(dctx->mctx, sizeof(*pk11_ctx)); @@ -396,16 +402,22 @@ pkcs11eddsa_compare(const dst_key_t *key1, const dst_key_t *key2) { } #define SETCURVE() \ - if (key->key_alg == DST_ALG_ED25519) { \ + switch (key->key_alg) { \ + case DST_ALG_ED25519: \ attr->pValue = isc_mem_get(key->mctx, \ sizeof(PK11_ECX_ED25519)); \ memmove(attr->pValue, PK11_ECX_ED25519, \ sizeof(PK11_ECX_ED25519)); \ attr->ulValueLen = sizeof(PK11_ECX_ED25519); \ - } else { \ + break; \ + case DST_ALG_ED448: \ attr->pValue = isc_mem_get(key->mctx, sizeof(PK11_ECX_ED448)); \ memmove(attr->pValue, PK11_ECX_ED448, sizeof(PK11_ECX_ED448)); \ attr->ulValueLen = sizeof(PK11_ECX_ED448); \ + break; \ + default: \ + INSIST(0); \ + ISC_UNREACHABLE(); \ } #define FREECURVE() \ @@ -507,10 +519,16 @@ pkcs11eddsa_generate(dst_key_t *key, int unused, void (*callback)(int)) { memset(pk11_ctx, 0, sizeof(*pk11_ctx)); isc_mem_put(key->mctx, pk11_ctx, sizeof(*pk11_ctx)); - if (key->key_alg == DST_ALG_ED25519) { + switch (key->key_alg) { + case DST_ALG_ED25519: key->key_size = DNS_KEY_ED25519SIZE; - } else { + break; + case DST_ALG_ED448: key->key_size = DNS_KEY_ED448SIZE; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } return (ISC_R_SUCCESS); @@ -582,10 +600,16 @@ pkcs11eddsa_todns(const dst_key_t *key, isc_buffer_t *data) { REQUIRE(key->keydata.pkey != NULL); - if (key->key_alg == DST_ALG_ED25519) { + switch (key->key_alg) { + case DST_ALG_ED25519: len = DNS_KEY_ED25519SIZE; - } else { + break; + case DST_ALG_ED448: len = DNS_KEY_ED448SIZE; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } ec = key->keydata.pkey; @@ -614,10 +638,16 @@ pkcs11eddsa_fromdns(dst_key_t *key, isc_buffer_t *data) { REQUIRE(key->key_alg == DST_ALG_ED25519 || key->key_alg == DST_ALG_ED448); - if (key->key_alg == DST_ALG_ED25519) { + switch (key->key_alg) { + case DST_ALG_ED25519: len = DNS_KEY_ED25519SIZE; - } else { + break; + case DST_ALG_ED448: len = DNS_KEY_ED448SIZE; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } isc_buffer_remainingregion(data, &r); @@ -635,16 +665,8 @@ pkcs11eddsa_fromdns(dst_key_t *key, isc_buffer_t *data) { attr = ec->repr; attr->type = CKA_EC_PARAMS; - if (key->key_alg == DST_ALG_ED25519) { - attr->pValue = isc_mem_get(key->mctx, sizeof(PK11_ECX_ED25519)); - memmove(attr->pValue, PK11_ECX_ED25519, - sizeof(PK11_ECX_ED25519)); - attr->ulValueLen = sizeof(PK11_ECX_ED25519); - } else { - attr->pValue = isc_mem_get(key->mctx, sizeof(PK11_ECX_ED448)); - memmove(attr->pValue, PK11_ECX_ED448, sizeof(PK11_ECX_ED448)); - attr->ulValueLen = sizeof(PK11_ECX_ED448); - } + SETCURVE(); + attr++; attr->type = CKA_EC_POINT; attr->pValue = isc_mem_get(key->mctx, len); @@ -907,10 +929,16 @@ pkcs11eddsa_parse(dst_key_t *key, isc_lex_t *lexer, dst_key_t *pub) { dst__privstruct_free(&priv, mctx); memset(&priv, 0, sizeof(priv)); - if (key->key_alg == DST_ALG_ED25519) { + switch (key->key_alg) { + case DST_ALG_ED25519: key->key_size = DNS_KEY_ED25519SIZE; - } else { + break; + case DST_ALG_ED448: key->key_size = DNS_KEY_ED448SIZE; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } return (ISC_R_SUCCESS); @@ -1024,10 +1052,16 @@ pkcs11eddsa_fromlabel(dst_key_t *key, const char *engine, const char *label, } key->label = isc_mem_strdup(key->mctx, label); - if (key->key_alg == DST_ALG_ED25519) { + switch (key->key_alg) { + case DST_ALG_ED25519: key->key_size = DNS_KEY_ED25519SIZE; - } else { + break; + case DST_ALG_ED448: key->key_size = DNS_KEY_ED448SIZE; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); } pk11_return_session(pk11_ctx);