ipa-kdb: refactor MS-PAC processing to prepare for krb5 1.20

Make sure both krb5 pre 1.20 and 1.20 or later would call into the same
PAC generation code while driven by different API callbacks from the
krb5 KDB interface.

Fixes: https://pagure.io/freeipa/issue/9083
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Reviewed-By: Julien Rische <jrische@redhat.com>
Reviewed-By: Rob Crittenden <rcritten@redhat.com>

daemons/ipa-kdb/ipa_kdb_mspac.c

@@ -983,8 +983,10 @@ static krb5_error_code ipadb_get_pac_attrs_blob(TALLOC_CTX *mem_ctx,
 #endif
 
 krb5_error_code ipadb_get_pac(krb5_context kcontext,
-                              krb5_db_entry *client,
                               unsigned int flags,
+                              krb5_db_entry *client,
+                              krb5_db_entry *server,
+                              krb5_keyblock *replaced_reply_key,
                               krb5_timestamp authtime,
                               krb5_pac *pac)
 {
@@ -1064,9 +1066,13 @@ krb5_error_code ipadb_get_pac(krb5_context kcontext,
         goto done;
     }
 
-    kerr = krb5_pac_init(kcontext, pac);
+    /* krb5 1.20+ passes in a pre-created PAC structure but for previous
-    if (kerr) {
+     * versions we have to create it ourselves */
-        goto done;
+    if (pac != NULL && *pac == NULL) {
+        kerr = krb5_pac_init(kcontext, pac);
+        if (kerr) {
+            goto done;
+        }
     }
 
     data.magic = KV5M_DATA;
@@ -1637,7 +1643,7 @@ static void filter_logon_info_log_message_rid(struct dom_sid *sid, uint32_t rid)
 
 static krb5_error_code check_logon_info_consistent(krb5_context context,
                                                    TALLOC_CTX *memctx,
-                                                   krb5_const_principal client_princ,
+                                                   krb5_db_entry *client,
                                                    krb5_boolean is_s4u,
                                                    struct PAC_LOGON_INFO_CTR *info)
 {
@@ -1645,7 +1651,6 @@ static krb5_error_code check_logon_info_consistent(krb5_context context,
     struct ipadb_context *ipactx;
     bool result;
     bool is_from_trusted_domain = false;
-    krb5_db_entry *client_actual = NULL;
     struct ipadb_e_data *ied = NULL;
     int flags = 0;
     struct dom_sid client_sid;
@@ -1701,18 +1706,12 @@ static krb5_error_code check_logon_info_consistent(krb5_context context,
         }
     }
 
-    if (is_s4u && is_from_trusted_domain) {
+    if (client == NULL || (is_s4u && is_from_trusted_domain)) {
         /* If the PAC belongs to a user from the trusted domain, we cannot compare SIDs */
         return 0;
     }
 
-    kerr = ipadb_get_principal(context, client_princ, flags, &client_actual);
+    ied = (struct ipadb_e_data *)client->e_data;
-    if (kerr != 0) {
-        krb5_klog_syslog(LOG_ERR, "PAC issue: ipadb_get_principal failed.");
-        return KRB5KDC_ERR_TGT_REVOKED;
-    }
-
-    ied = (struct ipadb_e_data *)client_actual->e_data;
     if (ied == NULL || ied->magic != IPA_E_DATA_MAGIC) {
         krb5_klog_syslog(LOG_ERR, "PAC issue: client e_data fetching failed.");
         kerr = EINVAL;
@@ -1748,8 +1747,6 @@ static krb5_error_code check_logon_info_consistent(krb5_context context,
     }
 
 done:
-    ipadb_free_principal(context, client_actual);
-
     return kerr;
 }
 
@@ -1980,7 +1977,7 @@ krb5_error_code filter_logon_info(krb5_context context,
 
 
 static krb5_error_code ipadb_check_logon_info(krb5_context context,
-                                              krb5_const_principal client_princ,
+                                              krb5_db_entry *client,
                                               krb5_boolean is_cross_realm,
                                               krb5_boolean is_s4u,
                                               krb5_data *pac_blob,
@@ -1989,7 +1986,7 @@ static krb5_error_code ipadb_check_logon_info(krb5_context context,
     struct PAC_LOGON_INFO_CTR info;
     krb5_error_code kerr;
     TALLOC_CTX *tmpctx;
-    krb5_data origin_realm = client_princ->realm;
+    krb5_data origin_realm = {0};
     bool result;
 
     tmpctx = talloc_new(NULL);
@@ -2051,7 +2048,7 @@ static krb5_error_code ipadb_check_logon_info(krb5_context context,
          * is ours but operates on behalf of the cross-realm principal, we will
          * search through the trusted domains but otherwise skip the exact SID check
          * as we are not responsible for the principal from the trusted domain */
-        kerr = check_logon_info_consistent(context, tmpctx, client_princ, is_s4u, &info);
+        kerr = check_logon_info_consistent(context, tmpctx, client, is_s4u, &info);
         goto done;
     }
 
@@ -2224,24 +2221,19 @@ done:
     return kerr;
 }
 
-krb5_error_code ipadb_verify_pac(krb5_context context,
+krb5_error_code ipadb_common_verify_pac(krb5_context context,
-                                 unsigned int flags,
+                                        unsigned int flags,
-                                 krb5_const_principal client_princ,
+                                        krb5_db_entry *client,
-                                 krb5_db_entry *proxy,
+                                        krb5_db_entry *server,
-                                 krb5_db_entry *server,
+                                        krb5_db_entry *signing_krbtgt,
-                                 krb5_db_entry *krbtgt,
+                                        krb5_keyblock *krbtgt_key,
-                                 krb5_keyblock *server_key,
+                                        krb5_timestamp authtime,
-                                 krb5_keyblock *krbtgt_key,
+                                        krb5_pac old_pac,
-                                 krb5_timestamp authtime,
+                                        krb5_pac *pac)
-                                 krb5_authdata **authdata,
-                                 krb5_pac *pac)
 {
-    krb5_keyblock *srv_key = NULL;
-    krb5_keyblock *priv_key = NULL;
     krb5_error_code kerr;
     krb5_ui_4 *types = NULL;
     size_t num_buffers;
-    krb5_pac old_pac = NULL;
     krb5_pac new_pac = NULL;
     krb5_data data;
     krb5_data pac_blob = { 0 , 0, NULL};
@@ -2250,42 +2242,15 @@ krb5_error_code ipadb_verify_pac(krb5_context context,
     struct dom_sid *requester_sid = NULL;
     struct dom_sid req_sid;
 
-    kerr = krb5_pac_parse(context,
+    if (signing_krbtgt != NULL &&
-                          authdata[0]->contents,
+        ipadb_is_cross_realm_krbtgt(signing_krbtgt->princ)) {
-                          authdata[0]->length,
-                          &old_pac);
-    if (kerr) {
-        goto done;
-    }
-
-    /* for cross realm trusts cases we need to check the right checksum.
-     * when the PAC is signed by our realm, we can always just check it
-     * passing our realm krbtgt key as the kdc checksum key (privsvr).
-     * But when a trusted realm passes us a PAC the kdc checksum is
-     * generated with that realm krbtgt key, so we need to use the cross
-     * realm krbtgt to check the 'server' checksum instead. */
-    if (ipadb_is_cross_realm_krbtgt(krbtgt->princ)) {
         /* krbtgt from a trusted realm */
         is_cross_realm = true;
-
-        srv_key = krbtgt_key;
-
-    } else {
-        /* krbtgt from our own realm */
-        priv_key = krbtgt_key;
     }
 
-    /* only pass with_realm TRUE when it is cross-realm ticket and S4U
+    /* In krb5 1.20 the PAC signatures are verified prior to call to issue_pac().
-     * extension (S4U2Self or S4U2Proxy (RBCD)) was requested */
+     * In krb5 before 1.20, we do verify PAC signatures before ipadb_common_verify_pac().
-    kerr = krb5_pac_verify_ext(context, old_pac, authtime,
+     * Now we can do additional checks.
-                               client_princ, srv_key, priv_key,
-                               (is_cross_realm &&
-                                (flags & KRB5_KDB_FLAG_PROTOCOL_TRANSITION)));
-    if (kerr) {
-        goto done;
-    }
-
-    /* Now that the PAC is verified, do additional checks.
      * Augment it with additional info if it is coming from a different realm */
     kerr = krb5_pac_get_buffer(context, old_pac,
                                KRB5_PAC_LOGON_INFO, &pac_blob);
@@ -2302,7 +2267,7 @@ krb5_error_code ipadb_verify_pac(krb5_context context,
 #endif
 
     kerr = ipadb_check_logon_info(context,
-                                  client_princ,
+                                  client,
                                   is_cross_realm,
                                   (flags & KRB5_KDB_FLAGS_S4U),
                                   &pac_blob,
@@ -2310,12 +2275,19 @@ krb5_error_code ipadb_verify_pac(krb5_context context,
     if (kerr != 0) {
         goto done;
     }
-    /* extract buffers and rebuilt pac from scratch so that when re-signing
+
-     * with a different cksum type does not cause issues due to mismatching
+    /* krb5 1.20+ passes in a pre-created PAC structure but for previous
-     * signature buffer lengths */
+     * versions we have to create it ourselves */
-    kerr = krb5_pac_init(context, &new_pac);
+    if (pac != NULL && *pac == NULL) {
-    if (kerr) {
+        /* extract buffers and rebuilt pac from scratch so that when re-signing
-        goto done;
+        * with a different cksum type does not cause issues due to mismatching
+        * signature buffer lengths */
+        kerr = krb5_pac_init(context, &new_pac);
+        if (kerr) {
+            goto done;
+        }
+    } else {
+        new_pac = *pac;
     }
 
     kerr = krb5_pac_get_types(context, old_pac, &num_buffers, &types);
@@ -2334,7 +2306,6 @@ krb5_error_code ipadb_verify_pac(krb5_context context,
             pac_blob.length != 0) {
             kerr = krb5_pac_add_buffer(context, new_pac, types[i], &pac_blob);
             if (kerr) {
-                krb5_pac_free(context, new_pac);
                 goto done;
             }
 
@@ -2351,21 +2322,18 @@ krb5_error_code ipadb_verify_pac(krb5_context context,
             TALLOC_CTX *tmpctx = talloc_new(NULL);
             if (tmpctx == NULL) {
                 kerr = ENOMEM;
-                krb5_pac_free(context, new_pac);
                 goto done;
             }
 
             kerr = ipadb_client_requested_pac(context, old_pac, tmpctx, &pac_requested);
             if (kerr != 0) {
                 talloc_free(tmpctx);
-                krb5_pac_free(context, new_pac);
                 goto done;
             }
 
             kerr = ipadb_get_pac_attrs_blob(tmpctx, &pac_requested, &pac_attrs_data);
             if (kerr) {
                 talloc_free(tmpctx);
-                krb5_pac_free(context, new_pac);
                 goto done;
             }
             data.magic = KV5M_DATA;
@@ -2375,7 +2343,6 @@ krb5_error_code ipadb_verify_pac(krb5_context context,
             kerr = krb5_pac_add_buffer(context, new_pac, PAC_TYPE_ATTRIBUTES_INFO, &data);
             if (kerr) {
                 talloc_free(tmpctx);
-                krb5_pac_free(context, new_pac);
                 goto done;
             }
 
@@ -2395,22 +2362,23 @@ krb5_error_code ipadb_verify_pac(krb5_context context,
             krb5_free_data_contents(context, &data);
         }
         if (kerr) {
-            krb5_pac_free(context, new_pac);
             goto done;
         }
     }
 
     if (flags & KRB5_KDB_FLAG_CONSTRAINED_DELEGATION) {
-        if (proxy == NULL) {
+        if (client == NULL) {
+            if (new_pac != *pac) {
+                krb5_pac_free(context, new_pac);
+            }
             *pac = NULL;
             kerr = 0;
             goto done;
         }
 
-        kerr = ipadb_add_transited_service(context, proxy, server,
+        kerr = ipadb_add_transited_service(context, client, server,
                                            old_pac, new_pac);
         if (kerr) {
-            krb5_pac_free(context, new_pac);
             goto done;
         }
     }
@@ -2418,8 +2386,9 @@ krb5_error_code ipadb_verify_pac(krb5_context context,
     *pac = new_pac;
 
 done:
-    krb5_free_authdata(context, authdata);
+    if (kerr != 0 && (new_pac != *pac)) {
-    krb5_pac_free(context, old_pac);
+        krb5_pac_free(context, new_pac);
+    }
     krb5_free_data_contents(context, &pac_blob);
     free(types);
     return kerr;

daemons/ipa-kdb/ipa_kdb_mspac_private.h

@@ -59,18 +59,18 @@ void get_authz_data_types(krb5_context context, krb5_db_entry *entry,
 
 bool ipadb_is_cross_realm_krbtgt(krb5_const_principal princ);
 krb5_error_code ipadb_get_pac(krb5_context kcontext,
-                              krb5_db_entry *client,
                               unsigned int flags,
+                              krb5_db_entry *client,
+                              krb5_db_entry *server,
+                              krb5_keyblock *replaced_reply_key,
                               krb5_timestamp authtime,
                               krb5_pac *pac);
-krb5_error_code ipadb_verify_pac(krb5_context context,
+krb5_error_code ipadb_common_verify_pac(krb5_context context,
-                                 unsigned int flags,
+                                        unsigned int flags,
-                                 krb5_const_principal client_princ,
+                                        krb5_db_entry *client,
-                                 krb5_db_entry *proxy,
+                                        krb5_db_entry *server,
-                                 krb5_db_entry *server,
+                                        krb5_db_entry *signing_krbtgt,
-                                 krb5_db_entry *krbtgt,
+                                        krb5_keyblock *krbtgt_key,
-                                 krb5_keyblock *server_key,
+                                        krb5_timestamp authtime,
-                                 krb5_keyblock *krbtgt_key,
+                                        krb5_pac old_pac,
-                                 krb5_timestamp authtime,
+                                        krb5_pac *pac);
-                                 krb5_authdata **authdata,
-                                 krb5_pac *pac);

daemons/ipa-kdb/ipa_kdb_mspac_v6.c

@@ -31,6 +31,68 @@
 
 #include "ipa_kdb_mspac_private.h"
 
+static
+krb5_error_code ipadb_verify_pac(krb5_context context,
+                                 unsigned int flags,
+                                 krb5_const_principal client_princ,
+                                 krb5_db_entry *proxy,
+                                 krb5_db_entry *server,
+                                 krb5_db_entry *krbtgt,
+                                 krb5_keyblock *server_key,
+                                 krb5_keyblock *krbtgt_key,
+                                 krb5_timestamp authtime,
+                                 krb5_authdata **authdata,
+                                 krb5_pac *pac)
+{
+    krb5_error_code kerr;
+    krb5_pac old_pac = NULL;
+    kerr = krb5_pac_parse(context,
+                          authdata[0]->contents,
+                          authdata[0]->length,
+                          &old_pac);
+    if (kerr == 0) {
+        krb5_keyblock *srv_key = NULL;
+        krb5_keyblock *priv_key = NULL;
+        bool is_cross_realm = false;
+
+        /* for cross realm trusts cases we need to check the right checksum.
+        * when the PAC is signed by our realm, we can always just check it
+        * passing our realm krbtgt key as the kdc checksum key (privsvr).
+        * But when a trusted realm passes us a PAC the kdc checksum is
+        * generated with that realm krbtgt key, so we need to use the cross
+        * realm krbtgt to check the 'server' checksum instead. */
+        if (ipadb_is_cross_realm_krbtgt(krbtgt->princ)) {
+            /* krbtgt from a trusted realm */
+            is_cross_realm = true;
+            srv_key = krbtgt_key;
+        } else {
+            /* krbtgt from our own realm */
+            priv_key = krbtgt_key;
+        }
+
+        /* only pass with_realm TRUE when it is cross-realm ticket and S4U
+        * extension (S4U2Self or S4U2Proxy (RBCD)) was requested */
+        kerr = krb5_pac_verify_ext(context, old_pac, authtime,
+                                NULL, srv_key, priv_key,
+                                (is_cross_realm &&
+                                    (flags & KRB5_KDB_FLAG_PROTOCOL_TRANSITION)));
+        if (kerr) {
+            goto done;
+        }
+
+        kerr = ipadb_common_verify_pac(context, flags,
+                                       proxy, server, krbtgt,
+                                       krbtgt_key,
+                                       authtime, old_pac, pac);
+    }
+
+done:
+    krb5_free_authdata(context, authdata);
+    krb5_pac_free(context, old_pac);
+    return kerr;
+}
+
+
 
 static krb5_error_code ipadb_sign_pac(krb5_context context,
                                       unsigned int flags,
@@ -239,7 +301,7 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
 
         (void)ipadb_reinit_mspac(ipactx, force_reinit_mspac);
 
-        kerr = ipadb_get_pac(context, client, flags, authtime, &pac);
+        kerr = ipadb_get_pac(context, flags, client, server, NULL, authtime, &pac);
         if (kerr != 0 && kerr != ENOENT) {
             goto done;
         }
@@ -253,7 +315,7 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
         /* check or generate pac data */
         if ((pac_auth_data == NULL) || (pac_auth_data[0] == NULL)) {
             if (flags & KRB5_KDB_FLAG_CONSTRAINED_DELEGATION) {
-                kerr = ipadb_get_pac(context, client_entry, flags, authtime, &pac);
+                kerr = ipadb_get_pac(context, flags, client_entry, server, NULL, authtime, &pac);
                 if (kerr != 0 && kerr != ENOENT) {
                     goto done;
                 }