krb5 commit: Improve S4U2Self realm identification internals

https://github.com/krb5/krb5/commit/9d6847c8f0187a0dd6466420335b5460de5c449e
commit 9d6847c8f0187a0dd6466420335b5460de5c449e
Author: Greg Hudson <[email protected]>
Date:   Wed Jan 2 16:54:28 2019 -0500

    Improve S4U2Self realm identification internals
    
    Realm identification for S4U2Self requests ([MS-SFU] 3.1.5.1.1.1) uses
    the AS code path with some differences: we might want to include a
    subject certificate in pa-data, we want to stop as soon as we get a
    reply indicating which realm the client is in, and we want to
    communicate that realm to the caller.  The current method of making
    these changes is fragile--it uses an optimistic preauth type but does
    not actually pre-authenticate, and it assumes that the AS code will
    terminate with a predictable error if there is no prompter and a
    trivial GAK function.
    
    Instead, add fields to krb5_get_init_creds_context for realm
    identification, and support them in the AS state machine, making sure
    never to invoke preauth modules.  Add a new library-internal function
    k5_identify_realm() to set up an appropriate context, run the state
    machine, and copy out the client principal of the last request on
    success.

 src/include/k5-trace.h            |    2 +
 src/lib/krb5/krb/get_in_tkt.c     |   59 +++++++++++++++++++++++++++++++++++++
 src/lib/krb5/krb/init_creds_ctx.h |    2 +
 src/lib/krb5/krb/int-proto.h      |   11 +++++++
 src/lib/krb5/krb/preauth2.c       |   56 +---------------------------------
 src/lib/krb5/krb/s4u_creds.c      |   57 +-----------------------------------
 src/tests/gssapi/t_s4u.py         |    4 +--
 7 files changed, 78 insertions(+), 113 deletions(-)

diff --git a/src/include/k5-trace.h b/src/include/k5-trace.h
index f3ed6a4..d08e5ec 100644
--- a/src/include/k5-trace.h
+++ b/src/include/k5-trace.h
@@ -227,6 +227,8 @@ void krb5int_trace(krb5_context context, const char *fmt, ...);
 #define TRACE_INIT_CREDS_GAK(c, salt, s2kparams)                    \
     TRACE(c, "Getting AS key, salt \"{data}\", params \"{data}\"",  \
           salt, s2kparams)
+#define TRACE_INIT_CREDS_IDENTIFIED_REALM(c, realm)                     \
+    TRACE(c, "Identified realm of client principal as {data}", realm)
 #define TRACE_INIT_CREDS_KEYTAB_LOOKUP(c, etypes)               \
     TRACE(c, "Looked up etypes in keytab: {etypes}", etypes)
 #define TRACE_INIT_CREDS_KEYTAB_LOOKUP_FAILED(c, code)          \
diff --git a/src/lib/krb5/krb/get_in_tkt.c b/src/lib/krb5/krb/get_in_tkt.c
index 79dede2..1e53d7a 100644
--- a/src/lib/krb5/krb/get_in_tkt.c
+++ b/src/lib/krb5/krb/get_in_tkt.c
@@ -1424,6 +1424,13 @@ init_creds_step_request(krb5_context context,
     if (code)
         goto cleanup;
 
+    if (ctx->subject_cert != NULL) {
+        code = add_padata(&ctx->request->padata, KRB5_PADATA_S4U_X509_USER,
+                          ctx->subject_cert->data, ctx->subject_cert->length);
+        if (code)
+            return code;
+    }
+
     code = maybe_add_pac_request(context, ctx);
     if (code)
         goto cleanup;
@@ -1566,6 +1573,12 @@ init_creds_step_reply(krb5_context context,
              * FAST upgrade. */
             ctx->restarted = FALSE;
             code = restart_init_creds_loop(context, ctx, FALSE);
+        } else if (ctx->identify_realm &&
+                   (reply_code == KDC_ERR_PREAUTH_REQUIRED ||
+                    reply_code == KDC_ERR_KEY_EXP)) {
+            /* The client exists in this realm; we can stop. */
+            ctx->complete = TRUE;
+            goto cleanup;
         } else if (reply_code == KDC_ERR_PREAUTH_REQUIRED && retry) {
             note_req_timestamp(context, ctx, ctx->err_reply->stime,
                                ctx->err_reply->susec);
@@ -1625,6 +1638,12 @@ init_creds_step_reply(krb5_context context,
     if (code != 0)
         goto cleanup;
 
+    if (ctx->identify_realm) {
+        /* Just getting a reply means the client exists in this realm. */
+        ctx->complete = TRUE;
+        goto cleanup;
+    }
+
     code = sort_krb5_padata_sequence(context, &ctx->request->client->realm,
                                      ctx->reply->padata);
     if (code != 0)
@@ -1850,6 +1869,46 @@ cleanup:
 }
 
 krb5_error_code
+k5_identify_realm(krb5_context context, krb5_principal client,
+                  const krb5_data *subject_cert, krb5_principal *client_out)
+{
+    krb5_error_code ret;
+    krb5_get_init_creds_opt *opts = NULL;
+    krb5_init_creds_context ctx = NULL;
+    int use_master = 0;
+
+    *client_out = NULL;
+
+    ret = krb5_get_init_creds_opt_alloc(context, &opts);
+    if (ret)
+        goto cleanup;
+    krb5_get_init_creds_opt_set_tkt_life(opts, 15);
+    krb5_get_init_creds_opt_set_renew_life(opts, 0);
+    krb5_get_init_creds_opt_set_forwardable(opts, 0);
+    krb5_get_init_creds_opt_set_proxiable(opts, 0);
+    krb5_get_init_creds_opt_set_canonicalize(opts, 1);
+
+    ret = krb5_init_creds_init(context, client, NULL, NULL, 0, opts, &ctx);
+    if (ret)
+        goto cleanup;
+
+    ctx->identify_realm = TRUE;
+    ctx->subject_cert = subject_cert;
+
+    ret = k5_init_creds_get(context, ctx, &use_master);
+    if (ret)
+        goto cleanup;
+
+    TRACE_INIT_CREDS_IDENTIFIED_REALM(context, &ctx->request->client->realm);
+    ret = krb5_copy_principal(context, ctx->request->client, client_out);
+
+cleanup:
+    krb5_get_init_creds_opt_free(context, opts);
+    krb5_init_creds_free(context, ctx);
+    return ret;
+}
+
+krb5_error_code
 k5_populate_gic_opt(krb5_context context, krb5_get_init_creds_opt **out,
                     krb5_flags options, krb5_address *const *addrs,
                     krb5_enctype *ktypes, krb5_preauthtype *pre_auth_types,
diff --git a/src/lib/krb5/krb/init_creds_ctx.h b/src/lib/krb5/krb/init_creds_ctx.h
index 7a6219b..5bd67a1 100644
--- a/src/lib/krb5/krb/init_creds_ctx.h
+++ b/src/lib/krb5/krb/init_creds_ctx.h
@@ -20,6 +20,8 @@ struct gak_password {
 struct _krb5_init_creds_context {
     krb5_get_init_creds_opt *opt;
     krb5_get_init_creds_opt opt_storage;
+    krb5_boolean identify_realm;
+    const krb5_data *subject_cert;
     char *in_tkt_service;
     krb5_prompter_fct prompter;
     void *prompter_data;
diff --git a/src/lib/krb5/krb/int-proto.h b/src/lib/krb5/krb/int-proto.h
index 4464a13..3e80241 100644
--- a/src/lib/krb5/krb/int-proto.h
+++ b/src/lib/krb5/krb/int-proto.h
@@ -291,6 +291,17 @@ k5_get_init_creds(krb5_context context, krb5_creds *creds,
                   get_as_key_fn gak, void *gak_data, int *master,
                   krb5_kdc_rep **as_reply);
 
+/*
+ * Make AS requests with the canonicalize flag set, stopping when we get a
+ * message indicating which realm the client principal is in.  Set *client_out
+ * to a copy of client with the canonical realm.  If subject_cert is non-null,
+ * include PA_S4U_X509_USER pa-data with the subject certificate each request.
+ * (See [MS-SFU] 3.1.5.1.1.1 and 3.1.5.1.1.2.)
+ */
+krb5_error_code
+k5_identify_realm(krb5_context context, krb5_principal client,
+                  const krb5_data *subject_cert, krb5_principal *client_out);
+
 krb5_error_code
 k5_populate_gic_opt(krb5_context context, krb5_get_init_creds_opt **opt,
                     krb5_flags options, krb5_address *const *addrs,
diff --git a/src/lib/krb5/krb/preauth2.c b/src/lib/krb5/krb/preauth2.c
index a73568c..ffca476 100644
--- a/src/lib/krb5/krb/preauth2.c
+++ b/src/lib/krb5/krb/preauth2.c
@@ -879,49 +879,6 @@ error:
     return ENOMEM;
 }
 
-static krb5_error_code
-add_s4u_x509_user_padata(krb5_context context, krb5_s4u_userid *userid,
-                         krb5_principal client, krb5_pa_data ***out_pa_list,
-                         int *out_pa_list_size)
-{
-    krb5_pa_data *s4u_padata;
-    krb5_error_code code;
-    krb5_principal client_copy;
-
-    if (userid == NULL)
-        return EINVAL;
-    code = krb5_copy_principal(context, client, &client_copy);
-    if (code != 0)
-        return code;
-    krb5_free_principal(context, userid->user);
-    userid->user = client_copy;
-
-    if (userid->subject_cert.length != 0) {
-        s4u_padata = malloc(sizeof(*s4u_padata));
-        if (s4u_padata == NULL)
-            return ENOMEM;
-
-        s4u_padata->magic = KV5M_PA_DATA;
-        s4u_padata->pa_type = KRB5_PADATA_S4U_X509_USER;
-        s4u_padata->contents = k5memdup(userid->subject_cert.data,
-                                        userid->subject_cert.length, &code);
-        if (s4u_padata->contents == NULL) {
-            free(s4u_padata);
-            return code;
-        }
-        s4u_padata->length = userid->subject_cert.length;
-
-        code = grow_pa_list(out_pa_list, out_pa_list_size, &s4u_padata, 1);
-        if (code) {
-            free(s4u_padata->contents);
-            free(s4u_padata);
-            return code;
-        }
-    }
-
-    return 0;
-}
-
 /*
  * If the module for pa_type can adjust its AS_REQ data using the contents of
  * err and err_padata, return 0 with *padata_out set to a padata list for the
@@ -1017,7 +974,8 @@ k5_preauth(krb5_context context, krb5_init_creds_context ctx,
     *padata_out = NULL;
     *pa_type_out = KRB5_PADATA_NONE;
 
-    if (in_padata == NULL)
+    /* We should never invoke preauth modules when identifying the realm. */
+    if (in_padata == NULL || ctx->identify_realm)
         return 0;
 
     TRACE_PREAUTH_INPUT(context, in_padata);
@@ -1032,16 +990,6 @@ k5_preauth(krb5_context context, krb5_init_creds_context ctx,
     if (ret)
         goto error;
 
-    if (krb5int_find_pa_data(context, in_padata,
-                             KRB5_PADATA_S4U_X509_USER) != NULL) {
-        /* Fulfill a private contract with krb5_get_credentials_for_user. */
-        ret = add_s4u_x509_user_padata(context, ctx->gak_data,
-                                       ctx->request->client,
-                                       &out_pa_list, &out_pa_list_size);
-        if (ret)
-            goto error;
-    }
-
     /* If we can't initialize the preauth context, stop with what we have. */
     k5_init_preauth_context(context);
     if (context->preauth_context == NULL) {
diff --git a/src/lib/krb5/krb/s4u_creds.c b/src/lib/krb5/krb/s4u_creds.c
index 614ed41..e72a64a 100644
--- a/src/lib/krb5/krb/s4u_creds.c
+++ b/src/lib/krb5/krb/s4u_creds.c
@@ -36,35 +36,12 @@
  */
 
 static krb5_error_code
-krb5_get_as_key_noop(
-    krb5_context context,
-    krb5_principal client,
-    krb5_enctype etype,
-    krb5_prompter_fct prompter,
-    void *prompter_data,
-    krb5_data *salt,
-    krb5_data *params,
-    krb5_keyblock *as_key,
-    void *gak_data,
-    k5_response_items *ritems)
-{
-    /* force a hard error, we don't actually have the key */
-    return KRB5_PREAUTH_FAILED;
-}
-
-static krb5_error_code
 s4u_identify_user(krb5_context context,
                   krb5_creds *in_creds,
                   krb5_data *subject_cert,
                   krb5_principal *canon_user)
 {
-    krb5_error_code code;
-    krb5_preauthtype ptypes[1] = { KRB5_PADATA_S4U_X509_USER };
-    krb5_creds creds;
-    int use_master = 0;
-    krb5_get_init_creds_opt *opts = NULL;
     krb5_principal_data client;
-    krb5_s4u_userid userid;
 
     *canon_user = NULL;
 
@@ -85,22 +62,6 @@ s4u_identify_user(krb5_context context,
                                    canon_user);
     }
 
-    memset(&creds, 0, sizeof(creds));
-
-    memset(&userid, 0, sizeof(userid));
-    if (subject_cert != NULL)
-        userid.subject_cert = *subject_cert;
-
-    code = krb5_get_init_creds_opt_alloc(context, &opts);
-    if (code != 0)
-        goto cleanup;
-    krb5_get_init_creds_opt_set_tkt_life(opts, 15);
-    krb5_get_init_creds_opt_set_renew_life(opts, 0);
-    krb5_get_init_creds_opt_set_forwardable(opts, 0);
-    krb5_get_init_creds_opt_set_proxiable(opts, 0);
-    krb5_get_init_creds_opt_set_canonicalize(opts, 1);
-    krb5_get_init_creds_opt_set_preauth_list(opts, ptypes, 1);
-
     if (in_creds->client != NULL) {
         client = *in_creds->client;
         client.realm = in_creds->server->realm;
@@ -113,23 +74,7 @@ s4u_identify_user(krb5_context context,
         client.type = KRB5_NT_ENTERPRISE_PRINCIPAL;
     }
 
-    code = k5_get_init_creds(context, &creds, &client, NULL, NULL, 0, NULL,
-                             opts, krb5_get_as_key_noop, &userid, &use_master,
-                             NULL);
-    if (!code || code == KRB5_PREAUTH_FAILED || code == KRB5KDC_ERR_KEY_EXP) {
-        *canon_user = userid.user;
-        userid.user = NULL;
-        code = 0;
-    }
-
-cleanup:
-    krb5_free_cred_contents(context, &creds);
-    if (opts != NULL)
-        krb5_get_init_creds_opt_free(context, opts);
-    if (userid.user != NULL)
-        krb5_free_principal(context, userid.user);
-
-    return code;
+    return k5_identify_realm(context, &client, subject_cert, canon_user);
 }
 
 static krb5_error_code
diff --git a/src/tests/gssapi/t_s4u.py b/src/tests/gssapi/t_s4u.py
index 164fec8..7aff9af 100755
--- a/src/tests/gssapi/t_s4u.py
+++ b/src/tests/gssapi/t_s4u.py
@@ -198,14 +198,12 @@ r1.run(['./t_s4u', 'p:' + r2.user_princ, '-', r1.keytab], env=no_default,
 # that we start at the server realm.
 mark('cross-realm S4U2Self with enterprise name')
 msgs = ('Getting initial credentials for enterprise\\@abc@SREALM',
-        'Processing preauth types: PA-FOR-X509-USER (130)',
         'Sending unauthenticated request',
         '/Realm not local to KDC',
         'Following referral to realm UREALM',
-        'Processing preauth types: PA-FOR-X509-USER (130)',
         'Sending unauthenticated request',
         '/Additional pre-authentication required',
-        '/Generic preauthentication failure',
+        'Identified realm of client principal as UREALM',
         'Getting credentials enterprise\\@abc@UREALM -> user@SREALM',
         'TGS reply is for enterprise\@abc@UREALM -> user@SREALM')
 r1.run(['./t_s4u', 'e:enterprise@abc@NOREALM', '-', r1.keytab],