Refactor PKINIT KDF internal interfaces

Simplify the client and server PKINIT code by renaming
pkinit_alg_agility_kdf() to pkinit_kdf() and making it do RFC 4556
octet2string if alg_oid is null.  Move responsibility for tracing
inside the new interface.  Constify some parameters and remove some
unnecessary casts.  Rename "key" to "secret" in several internal
functions to avoid confusion between the input DH secret and the
output key.

6 files changed, 70 insertions, 132 deletions

diff --git a/src/plugins/preauth/pkinit/pkinit_clnt.c b/src/plugins/preauth/pkinit/pkinit_clnt.c
index b08022a..0f76a62 100644
--- a/src/plugins/preauth/pkinit/pkinit_clnt.c
+++ b/src/plugins/preauth/pkinit/pkinit_clnt.c
@@ -621,34 +621,13 @@ pkinit_as_rep_parse(krb5_context context,
         goto cleanup;
     }
 
-    /* If we have a KDF algorithm ID, call the algorithm agility KDF. */
-    if (kdc_reply->u.dh_Info.kdfID) {
-        secret.length = client_key_len;
-        secret.data = (char *)client_key;
-
-        retval = pkinit_alg_agility_kdf(context, &secret,
-                                        kdc_reply->u.dh_Info.kdfID,
-                                        request->client, request->server,
-                                        etype, encoded_request,
-                                        (krb5_data *)as_rep, key_block);
-        if (retval) {
-            pkiDebug("failed to create key pkinit_alg_agility_kdf %s\n",
-                     error_message(retval));
-            goto cleanup;
-        }
-        TRACE_PKINIT_CLIENT_KDF_ALG(context, kdc_reply->u.dh_Info.kdfID,
-                                    key_block);
-
-    } else {
-        /* Otherwise, use the older octetstring2key function. */
-        retval = pkinit_octetstring2key(context, etype, client_key,
-                                        client_key_len, key_block);
-        if (retval) {
-            pkiDebug("failed to create key pkinit_octetstring2key %s\n",
-                     error_message(retval));
-            goto cleanup;
-        }
-        TRACE_PKINIT_CLIENT_KDF_OS2K(context, key_block);
+    secret = make_data(client_key, client_key_len);
+    retval = pkinit_kdf(context, &secret, kdc_reply->u.dh_Info.kdfID,
+                        request->client, request->server, etype,
+                        encoded_request, as_rep, key_block);
+    if (retval) {
+        pkiDebug("pkinit_kdf failed: %s\n", error_message(retval));
+        goto cleanup;
     }
 
     retval = 0;
diff --git a/src/plugins/preauth/pkinit/pkinit_crypto.h b/src/plugins/preauth/pkinit/pkinit_crypto.h
index 900dba7..8e4a813 100644
--- a/src/plugins/preauth/pkinit/pkinit_crypto.h
+++ b/src/plugins/preauth/pkinit/pkinit_crypto.h
@@ -245,22 +245,6 @@ krb5_error_code crypto_check_cert_eku
 		    receives non-zero if an acceptable EKU was found */
 
 /*
- * this functions takes in generated DH secret key and converts
- * it in to a kerberos session key. it takes into the account the
- * enc type and then follows the procedure specified in the RFC p 22.
- */
-krb5_error_code pkinit_octetstring2key
-	(krb5_context context,				/* IN */
-	krb5_enctype etype,				/* IN
-		    specifies the enc type */
-	unsigned char *key,				/* IN
-		    contains the DH secret key */
-	unsigned int key_len,				/* IN
-		    contains length of key */
-	krb5_keyblock * krb5key);			/* OUT
-		    receives kerberos session key */
-
-/*
  * this function implements clients first part of the DH protocol.
  * client selects its DH parameters and pub key
  */
@@ -552,15 +536,11 @@ krb5_error_code pkinit_identity_set_prompter
 	void *prompter_data);				/* IN */
 
 krb5_error_code
-pkinit_alg_agility_kdf(krb5_context context,
-                       krb5_data *secret,
-                       krb5_data *alg_oid,
-                       krb5_const_principal party_u_info,
-                       krb5_const_principal party_v_info,
-                       krb5_enctype enctype,
-                       krb5_data *as_req,
-                       krb5_data *pk_as_rep,
-                       krb5_keyblock *key_block);
+pkinit_kdf(krb5_context context, krb5_data *secret, const krb5_data *alg_oid,
+	   krb5_const_principal party_u_info,
+	   krb5_const_principal party_v_info, krb5_enctype enctype,
+	   const krb5_data *as_req, const krb5_data *pk_as_rep,
+	   krb5_keyblock *key_block);
 
 extern const krb5_data sha1_id;
 extern const krb5_data sha256_id;
diff --git a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
index cc5befc..7c26899 100644
--- a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
+++ b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
@@ -2599,12 +2599,9 @@ cleanup:
     return retval;
 }
 
-krb5_error_code
-pkinit_octetstring2key(krb5_context context,
-                       krb5_enctype etype,
-                       unsigned char *key,
-                       unsigned int dh_key_len,
-                       krb5_keyblock *key_block)
+static krb5_error_code
+octetstring2key(krb5_context context, krb5_enctype etype,
+                const krb5_data *secret, krb5_keyblock *key_block)
 {
     krb5_error_code retval;
     unsigned char *buf = NULL;
@@ -2614,7 +2611,7 @@ pkinit_octetstring2key(krb5_context context,
     krb5_data random_data;
     EVP_MD_CTX *sha1_ctx = NULL;
 
-    buf = k5alloc(dh_key_len, &retval);
+    buf = k5alloc(secret->length, &retval);
     if (buf == NULL)
         goto cleanup;
 
@@ -2629,20 +2626,20 @@ pkinit_octetstring2key(krb5_context context,
     do {
         if (!EVP_DigestInit(sha1_ctx, EVP_sha1()) ||
             !EVP_DigestUpdate(sha1_ctx, &counter, 1) ||
-            !EVP_DigestUpdate(sha1_ctx, key, dh_key_len) ||
+            !EVP_DigestUpdate(sha1_ctx, secret->data, secret->length) ||
             !EVP_DigestFinal(sha1_ctx, md, NULL)) {
             retval = KRB5_CRYPTO_INTERNAL;
             goto cleanup;
         }
 
-        if (dh_key_len - offset < sizeof(md))
-            memcpy(buf + offset, md, dh_key_len - offset);
+        if (secret->length - offset < sizeof(md))
+            memcpy(buf + offset, md, secret->length - offset);
         else
             memcpy(buf + offset, md, sizeof(md));
 
         offset += sizeof(md);
         counter++;
-    } while (offset < dh_key_len);
+    } while (offset < secret->length);
 
     key_block->magic = 0;
     key_block->enctype = etype;
@@ -2660,6 +2657,10 @@ pkinit_octetstring2key(krb5_context context,
     random_data.data = (char *)buf;
 
     retval = krb5_c_random_to_key(context, etype, &random_data, key_block);
+    if (retval)
+        goto cleanup;
+
+    TRACE_PKINIT_KDF_OS2K(context, key_block);
 
 cleanup:
     EVP_MD_CTX_free(sha1_ctx);
@@ -2691,8 +2692,8 @@ algid_to_md(const krb5_data *alg_id)
 
 #define sskdf openssl_sskdf
 static krb5_error_code
-openssl_sskdf(krb5_context context, const EVP_MD *md, krb5_data *key,
-              krb5_data *info, size_t len, krb5_data *out)
+openssl_sskdf(krb5_context context, const EVP_MD *md, const krb5_data *secret,
+              const krb5_data *info, size_t len, krb5_data *out)
 {
     krb5_error_code ret;
     EVP_KDF *kdf = NULL;
@@ -2719,7 +2720,7 @@ openssl_sskdf(krb5_context context, const EVP_MD *md, krb5_data *key,
     *p++ = OSSL_PARAM_construct_utf8_string(OSSL_KDF_PARAM_DIGEST,
                                             (char *)EVP_MD_get0_name(md), 0);
     *p++ = OSSL_PARAM_construct_octet_string(OSSL_KDF_PARAM_KEY,
-                                             key->data, key->length);
+                                             secret->data, secret->length);
     *p++ = OSSL_PARAM_construct_octet_string(OSSL_KDF_PARAM_INFO,
                                              info->data, info->length);
     *p = OSSL_PARAM_construct_end();
@@ -2741,8 +2742,8 @@ cleanup:
 
 #define sskdf builtin_sskdf
 static krb5_error_code
-builtin_sskdf(krb5_context context, const EVP_MD *md, krb5_data *key,
-              krb5_data *info, size_t len, krb5_data *out)
+builtin_sskdf(krb5_context context, const EVP_MD *md, const krb5_data *secret,
+              const krb5_data *info, size_t len, krb5_data *out)
 {
     krb5_error_code ret;
     uint32_t counter = 1, reps;
@@ -2783,7 +2784,7 @@ builtin_sskdf(krb5_context context, const EVP_MD *md, krb5_data *key,
         /* -   Compute Hashi = H(counter || Z || OtherInfo). */
         if (!EVP_DigestInit(ctx, md) ||
             !EVP_DigestUpdate(ctx, be_counter, 4) ||
-            !EVP_DigestUpdate(ctx, key->data, key->length) ||
+            !EVP_DigestUpdate(ctx, secret->data, secret->length) ||
             !EVP_DigestUpdate(ctx, info->data, info->length) ||
             !EVP_DigestFinal(ctx, outptr, &s)) {
             ret = oerr(context, KRB5_CRYPTO_INTERNAL,
@@ -2805,13 +2806,14 @@ cleanup:
 
 #endif /* OPENSSL_VERSION_NUMBER < 0x30000000L */
 
-/* id-pkinit-kdf family, as specified by RFC 8636. */
+/* id-pkinit-kdf family, as specified by RFC 8636.  If alg_oid is null,
+ * octet2string(), as specified by RFC 4556. */
 krb5_error_code
-pkinit_alg_agility_kdf(krb5_context context, krb5_data *secret,
-                       krb5_data *alg_oid, krb5_const_principal party_u_info,
-                       krb5_const_principal party_v_info,
-                       krb5_enctype enctype, krb5_data *as_req,
-                       krb5_data *pk_as_rep, krb5_keyblock *key_block)
+pkinit_kdf(krb5_context context, krb5_data *secret, const krb5_data *alg_oid,
+           krb5_const_principal party_u_info,
+           krb5_const_principal party_v_info, krb5_enctype enctype,
+           const krb5_data *as_req, const krb5_data *pk_as_rep,
+           krb5_keyblock *key_block)
 {
     krb5_error_code ret;
     size_t rand_len = 0, key_len = 0;
@@ -2823,6 +2825,9 @@ pkinit_alg_agility_kdf(krb5_context context, krb5_data *secret,
     krb5_algorithm_identifier alg_id;
     char *hash_name = NULL;
 
+    if (alg_oid == NULL)
+        return octetstring2key(context, enctype, secret, key_block);
+
     ret = krb5_c_keylengths(context, enctype, &rand_len, &key_len);
     if (ret)
         goto cleanup;
@@ -2840,7 +2845,7 @@ pkinit_alg_agility_kdf(krb5_context context, krb5_data *secret,
     if (party_u_info &&
         krb5_principal_compare_any_realm(context, party_u_info,
                                          krb5_anonymous_principal())) {
-        party_u_info = (krb5_principal)krb5_anonymous_principal();
+        party_u_info = krb5_anonymous_principal();
     }
 
     md = algid_to_md(alg_oid);
@@ -2875,6 +2880,10 @@ pkinit_alg_agility_kdf(krb5_context context, krb5_data *secret,
         goto cleanup;
 
     ret = krb5_c_random_to_key(context, enctype, &random_data, key_block);
+    if (ret)
+        goto cleanup;
+
+    TRACE_PKINIT_KDF_ALG(context, alg_oid, key_block);
 
 cleanup:
     if (ret)
diff --git a/src/plugins/preauth/pkinit/pkinit_kdf_test.c b/src/plugins/preauth/pkinit/pkinit_kdf_test.c
index 7f38e84..0a8a69b 100644
--- a/src/plugins/preauth/pkinit/pkinit_kdf_test.c
+++ b/src/plugins/preauth/pkinit/pkinit_kdf_test.c
@@ -24,12 +24,8 @@
  * or implied warranty.
  */
 
-/*
- * pkinit_kdf_test.c -- Test to verify the correctness of the function
- * pkinit_alg_agility_kdf() in pkinit_crypto_openssl, which implements
- * the Key Derivation Function from the PKInit Algorithm Agility
- * document, currently draft-ietf-krb-wg-pkinit-alg-agility-04.txt.
- */
+/* Verify the correctness of pkinit_kdf() in pkinit_crypto_openssl, which
+ * implements the key derivation function from RFC 8636. */
 
 #include "k5-platform.h"
 #include "pkinit.h"
@@ -72,7 +68,6 @@ krb5_octet key3_hex[] =
 int
 main(int argc, char **argv)
 {
-    /* arguments for calls to pkinit_alg_agility_kdf() */
     krb5_context context = 0;
     krb5_data secret;
     krb5_algorithm_identifier alg_id;
@@ -131,12 +126,9 @@ main(int argc, char **argv)
 
     enctype = enctype_aes;
 
-    /* call pkinit_alg_agility_kdf() with test vector values*/
-    if (0 != (retval = pkinit_alg_agility_kdf(context, &secret,
-                                              &alg_id.algorithm,
-                                              u_principal, v_principal,
-                                              enctype, &as_req, &pk_as_rep,
-                                              &key_block))) {
+    retval = pkinit_kdf(context, &secret, &alg_id.algorithm, u_principal,
+                        v_principal, enctype, &as_req, &pk_as_rep, &key_block);
+    if (retval) {
         printf("ERROR in pkinit_kdf_test: kdf call failed, retval = %d\n",
                retval);
         goto cleanup;
@@ -162,12 +154,9 @@ main(int argc, char **argv)
 
     enctype = enctype_aes;
 
-    /* call pkinit_alg_agility_kdf() with test vector values*/
-    if (0 != (retval = pkinit_alg_agility_kdf(context, &secret,
-                                              &alg_id.algorithm,
-                                              u_principal, v_principal,
-                                              enctype, &as_req, &pk_as_rep,
-                                              &key_block))) {
+    retval = pkinit_kdf(context, &secret, &alg_id.algorithm, u_principal,
+                        v_principal, enctype, &as_req, &pk_as_rep, &key_block);
+    if (retval) {
         printf("ERROR in pkinit_kdf_test: kdf call failed, retval = %d\n",
                retval);
         goto cleanup;
@@ -193,12 +182,9 @@ main(int argc, char **argv)
 
     enctype = enctype_des3;
 
-    /* call pkinit_alg_agility_kdf() with test vector values*/
-    if (0 != (retval = pkinit_alg_agility_kdf(context, &secret,
-                                              &alg_id.algorithm,
-                                              u_principal, v_principal,
-                                              enctype, &as_req, &pk_as_rep,
-                                              &key_block))) {
+    retval = pkinit_kdf(context, &secret, &alg_id.algorithm, u_principal,
+                        v_principal, enctype, &as_req, &pk_as_rep, &key_block);
+    if (retval) {
         printf("ERROR in pkinit_kdf_test: kdf call failed, retval = %d\n",
                retval);
         goto cleanup;
diff --git a/src/plugins/preauth/pkinit/pkinit_srv.c b/src/plugins/preauth/pkinit/pkinit_srv.c
index e22bcb1..c7880e3 100644
--- a/src/plugins/preauth/pkinit/pkinit_srv.c
+++ b/src/plugins/preauth/pkinit/pkinit_srv.c
@@ -896,30 +896,13 @@ pkinit_server_return_padata(krb5_context context,
                          "/tmp/kdc_as_rep");
 #endif
 
-    /* If mutually supported KDFs were found, use the algorithm agility KDF. */
-    if (rep->u.dh_Info.kdfID) {
-        secret.data = (char *)server_key;
-        secret.length = server_key_len;
-
-        retval = pkinit_alg_agility_kdf(context, &secret, rep->u.dh_Info.kdfID,
-                                        request->client, request->server,
-                                        enctype, req_pkt, out_data,
-                                        &reply_key);
-        if (retval) {
-            pkiDebug("pkinit_alg_agility_kdf failed: %s\n",
-                     error_message(retval));
-            goto cleanup;
-        }
-
-        /* Otherwise, use the older octetstring2key() function */
-    } else {
-        retval = pkinit_octetstring2key(context, enctype, server_key,
-                                            server_key_len, &reply_key);
-        if (retval) {
-            pkiDebug("pkinit_octetstring2key failed: %s\n",
-                     error_message(retval));
-            goto cleanup;
-        }
+    secret = make_data(server_key, server_key_len);
+    retval = pkinit_kdf(context, &secret, rep->u.dh_Info.kdfID,
+                        request->client, request->server, enctype, req_pkt,
+                        out_data, &reply_key);
+    if (retval) {
+        pkiDebug("pkinit_kdf failed: %s\n", error_message(retval));
+        goto cleanup;
     }
 
     retval = cb->replace_reply_key(context, rock, &reply_key, FALSE);
diff --git a/src/plugins/preauth/pkinit/pkinit_trace.h b/src/plugins/preauth/pkinit/pkinit_trace.h
index 1c1ceb5..ab23b68 100644
--- a/src/plugins/preauth/pkinit/pkinit_trace.h
+++ b/src/plugins/preauth/pkinit/pkinit_trace.h
@@ -43,12 +43,6 @@
     TRACE(c, "PKINIT client skipping EKU check due to configuration")
 #define TRACE_PKINIT_CLIENT_FRESHNESS_TOKEN(c)                  \
     TRACE(c, "PKINIT client received freshness token from KDC")
-#define TRACE_PKINIT_CLIENT_KDF_ALG(c, kdf, keyblock)                   \
-    TRACE(c, "PKINIT client used KDF {hexdata} to compute reply key "   \
-          "{keyblock}", kdf, keyblock)
-#define TRACE_PKINIT_CLIENT_KDF_OS2K(c, keyblock)                       \
-    TRACE(c, "PKINIT client used octetstring2key to compute reply key " \
-          "{keyblock}", keyblock)
 #define TRACE_PKINIT_CLIENT_NO_IDENTITY(c)                              \
     TRACE(c, "PKINIT client has no configured identity; giving up")
 #define TRACE_PKINIT_CLIENT_REP_CHECKSUM_FAIL(c, expected, received)    \
@@ -95,6 +89,13 @@
     TRACE(c, "PKINIT client key has group {str}, need at least {str}",  \
           desc, mindesc)
 
+#define TRACE_PKINIT_KDF_ALG(c, kdf, keyblock)                          \
+    TRACE(c, "PKINIT used KDF {hexdata} to compute reply key {keyblock}", \
+          kdf, keyblock)
+#define TRACE_PKINIT_KDF_OS2K(c, keyblock)                              \
+    TRACE(c, "PKINIT used octetstring2key to compute reply key {keyblock}", \
+          keyblock)
+
 #define TRACE_PKINIT_OPENSSL_ERROR(c, msg)              \
     TRACE(c, "PKINIT OpenSSL error: {str}", msg)