Re: kinit error message (Heimdal 7.4.0)

[Jeffrey Altman]

When possible, please submit patches and requests for review via GitHub
as a pull request.   This will ensure the patch is not lost and provides
the ability to apply comments both on the pull request as well as
comment inline to the patch.

The patch will be tested by the Continuous Integration system and the
results posted back to the conversation.

Thanks.


On 10/26/2017 8:44 AM, Harald Barth wrote:
> 
> I may have not yet shared my final version of the patch which gives
> the user as much error message(s) that are extractable from both the
> context and the return code:
> 
> $ diff -u error_string.c.orig error_string.c
> --- error_string.c.orig 2017-07-11 07:14:16.0 +0200
> +++ error_string.c  2017-10-05 21:04:50.064079947 +0200
> @@ -234,8 +234,6 @@
>  }
> HEIMDAL_MUTEX_unlock(&context->mutex);
>  
> -if (str)
> -return str;
>  }
>  else
>  {
> @@ -249,10 +247,26 @@
>  if (free_context)
>  krb5_free_context(context);
>  
> -if (cstr)
> -return strdup(cstr);
> +if (!cstr) {
> +   if (cstr = error_message(code)) {
> +   strlcpy(buf, cstr, sizeof(buf));
> +   cstr = buf;
> +   }
> +}
>  
> -cstr = error_message(code);
> +if (str && !cstr)
> +   return str;
> +
> +if (str && cstr) {
> +   if (strncmp(str, cstr, sizeof(buf)) == 0)
> +   return str;
> +   else {
> +   strlcat(buf, ", ", sizeof(buf));
> +   strlcat(buf,  str, sizeof(buf));
> +   return strdup(cstr);
> +   }
> +}
> +
>  if (cstr)
>  return strdup(cstr);
>  
> So...whatyouthinkaboutthat? Yes, str*foo() in C is a pain.
> 
> Harald.
> 



smime.p7s
Description: S/MIME Cryptographic Signature

Re: kinit error message (Heimdal 7.4.0)

[Harald Barth]

I may have not yet shared my final version of the patch which gives
the user as much error message(s) that are extractable from both the
context and the return code:

$ diff -u error_string.c.orig error_string.c
--- error_string.c.orig 2017-07-11 07:14:16.0 +0200
+++ error_string.c  2017-10-05 21:04:50.064079947 +0200
@@ -234,8 +234,6 @@
 }
HEIMDAL_MUTEX_unlock(&context->mutex);
 
-if (str)
-return str;
 }
 else
 {
@@ -249,10 +247,26 @@
 if (free_context)
 krb5_free_context(context);
 
-if (cstr)
-return strdup(cstr);
+if (!cstr) {
+   if (cstr = error_message(code)) {
+   strlcpy(buf, cstr, sizeof(buf));
+   cstr = buf;
+   }
+}
 
-cstr = error_message(code);
+if (str && !cstr)
+   return str;
+
+if (str && cstr) {
+   if (strncmp(str, cstr, sizeof(buf)) == 0)
+   return str;
+   else {
+   strlcat(buf, ", ", sizeof(buf));
+   strlcat(buf,  str, sizeof(buf));
+   return strdup(cstr);
+   }
+}
+
 if (cstr)
 return strdup(cstr);
 
So...whatyouthinkaboutthat? Yes, str*foo() in C is a pain.

Harald.

Re: kinit error message (Heimdal 7.4.0)

[Nico Williams]

On Thu, Oct 05, 2017 at 08:01:00PM +0200, Harald Barth wrote:
> I may have found it! The 1.5.X KDC has a mile long function
> _kdc_as_rep() which in the middle somewhere has
> 
>  e_text = "No ENC-TS found";

Ahh, ok, got it.

Nico
-- 

Re: kinit error message (Heimdal 7.4.0)

[Harald Barth]

> I looked for that and couldn't find it.

I may have found it! The 1.5.X KDC has a mile long function
_kdc_as_rep() which in the middle somewhere has

 e_text = "No ENC-TS found";

That function looks quite different in 7.X

> So, to reproduce just mark a principal as expired and try to kinit?

Yes. Your newer KDC may fill in something else in the
context->error_string or context->error_code compared to the return
value. So if you put a breakpoint at krb5_get_error_message and 
compare these with the variable "code" which should be -1765328383.

Harald.

Re: kinit error message (Heimdal 7.4.0)

[Nico Williams]

On Thu, Oct 05, 2017 at 12:28:29PM -0400, Jeffrey Hutzelman wrote:
> Both, I think. kinit (and other clients) ought to report something
> like "error_message (e_text)", unless the e_text is empty or the same
> as the message for the error code. of course, more complex variations
> are possible, what with both locally- and KDC-generated error codes
> and additional messages. but just blindly using the e_text and nothing
> else is clearly wrong.

Yes.  (And let's not even get into how to localize e-text.)

> That said, the KDC should not be sending this particular e_text in
> this situation. I'll bet there's a loop that looks for suitable PA
> data, and that message gets produced if it finishes without finding
> any, even though the problem is something else entirely.

I looked for that and couldn't find it.

There's only one place where the string "ENC-TS" occurs; I could not
find how it gets turned into "No ENC-TS found", now could I find a
printf format string that would do that.

Nico
-- 

Re: kinit error message (Heimdal 7.4.0)

[Nico Williams]

On Thu, Oct 05, 2017 at 12:06:51PM -0400, Greg Hudson wrote:
> If my theory is correct, the KDC is sending unhelpful e_text and/or
> Heimdal is too trusting in using the e_text in preference to the string
> corresponding to the error code.  In this case, concatenating the error
> code string with the e_text would produce a better result but not an
> ideal one, as "No ENC-TS found" shouldn't appear in the error message at
> all.

I concluded it's the KDC, but I couldn't find where that's produced.  My
theory was that it was _kdc_as_rep(), when it couldn't find a PA, but I
didn't see how.

Nico
-- 

Re: kinit error message (Heimdal 7.4.0)

[Jeffrey Hutzelman]

On October 5, 2017 12:06:51 PM EDT, Greg Hudson  wrote:
>On 10/05/2017 07:52 AM, Harald Barth wrote:
>> And because the return code ret is the same as the error_code in the
>> context, krb5_get_error_message() just copies the string from the
>> context. However, if krb5_get_error_message() does its own lookup of
>> -1765328383 it gets "Client's entry in database has expired" which is
>> more like it. But where does "No ENC-TS found" come from and why is
>it
>> "better" than the own lookup?
>
>I didn't find anything like "No ENC-TS found" in the Heimdal source
>code, so my best guess is that this is coming from
>rd_error.c:krb5_error_from_rd_error() which does:
>
>ret = error->error_code;
>if (error->e_text != NULL) {
>krb5_set_error_message(context, ret, "%s", *error->e_text);
>} ...
>
>If my theory is correct, the KDC is sending unhelpful e_text and/or
>Heimdal is too trusting in using the e_text in preference to the string
>corresponding to the error code.

Both, I think. kinit (and other clients) ought to report something like 
"error_message (e_text)", unless the e_text is empty or the same as the message 
for the error code. of course, more complex variations are possible, what with 
both locally- and KDC-generated error codes and additional messages. but just 
blindly using the e_text and nothing else is clearly wrong.

That said, the KDC should not be sending this particular e_text in this 
situation. I'll bet there's a loop that looks for suitable PA data, and that 
message gets produced if it finishes without finding any, even though the 
problem is something else entirely.

-- Jeff




  In this case, concatenating the error
>code string with the e_text would produce a better result but not an
>ideal one, as "No ENC-TS found" shouldn't appear in the error message
>at
>all.

Re: kinit error message (Heimdal 7.4.0)

[Greg Hudson]

On 10/05/2017 07:52 AM, Harald Barth wrote:
> And because the return code ret is the same as the error_code in the
> context, krb5_get_error_message() just copies the string from the
> context. However, if krb5_get_error_message() does its own lookup of
> -1765328383 it gets "Client's entry in database has expired" which is
> more like it. But where does "No ENC-TS found" come from and why is it
> "better" than the own lookup?

I didn't find anything like "No ENC-TS found" in the Heimdal source
code, so my best guess is that this is coming from
rd_error.c:krb5_error_from_rd_error() which does:

ret = error->error_code;
if (error->e_text != NULL) {
krb5_set_error_message(context, ret, "%s", *error->e_text);
} ...

If my theory is correct, the KDC is sending unhelpful e_text and/or
Heimdal is too trusting in using the e_text in preference to the string
corresponding to the error code.  In this case, concatenating the error
code string with the e_text would produce a better result but not an
ideal one, as "No ENC-TS found" shouldn't appear in the error message at
all.

Re: kinit error message (Heimdal 7.4.0)

[Nico Williams]

On Thu, Oct 05, 2017 at 10:37:26AM +0200, Harald Barth wrote:
> I'm currently looking at why kinit can not give a decent error message
> on the easy fact that a credential has expired. Well, now with 7.4.0
> it handles "password expired" but "principal expired" still gives:

So, to reproduce just mark a principal as expired and try to kinit?

Re: kinit error message (Heimdal 7.4.0)

[Nico Williams]

On Thu, Oct 05, 2017 at 10:37:26AM +0200, Harald Barth wrote:
> I'm currently looking at why kinit can not give a decent error message
> on the easy fact that a credential has expired. Well, now with 7.4.0
> it handles "password expired" but "principal expired" still gives:
> 
> kinit: krb5_get_init_creds: No ENC-TS found
> 
> which is very broken from a user support group view. I tracked this
> down to the call in kinit.c line 673 which gets handled by the
> default: in the following switch(ret) with ret=-1765328383 Is that
> KRB5KDC_ERR_NAME_EXP - but how does that get translated to "No ENC-TS
> found"?

Oh, yeah, that's lame.

> Questions:
> 
> 1. How do I get the list of all KRB5KDC_ERR_* values and where are
> these defined? 

The *.et files define them.  KRB5KDC_ERR_* errors come from RFC4120 and
related RFCs, but in the source tree they are defined in *.et files.

> 2. What possible error values can come back from krb5_init_creds_get()
> and how to deal with them better?

We don't have an exhaustive list.  Does MIT?  But whatever the case,
these errors should always come with a user-meaningful error message.
So let's improve this.

> 3. Should the error handling and generation of the error string be in
> this switch() or should it be by some krb5_error_something function?

krb5_get_init_creds_*() should definitely set appropriate error
messages, however, kinit probably does need to remap them or add
additional text (mostly prefixes).

Nico
-- 

Re: kinit error message (Heimdal 7.4.0)

[Harald Barth]

I wrote:

> +   strncat(cstr, ", ", sizeof(buf));
> +   return strdup(strncat(cstr, str, sizeof(buf)));

which is broken and buggy usage of strncat(), so don't copy that.

Harald.

kinit error message (Heimdal 7.4.0)

[Harald Barth]

Well, I' making some progress.

The context I get when krb5_init_creds_get() returns is

$23 = {etypes = 0x0, cfg_etypes = 0x0, etypes_des = 0x0, as_etypes = 0x0, 
  tgs_etypes = 0x0, permitted_enctypes = 0x0, default_realms = 0x0, 
  max_skew = 300, kdc_timeout = 30, host_timeout = 3, max_retries = 3, 
  kdc_sec_offset = 0, kdc_usec_offset = 0, cf = 0x60bb40, et_list = 0x60a0c0, 
  warn_dest = 0x0, debug_dest = 0x0, cc_ops = 0x60ebe0, num_cc_ops = 6, 
  http_proxy = 0x0, time_fmt = 0x779abada "%Y-%m-%dT%H:%M:%S", 
  log_utc = 0, default_keytab = 0x779ab985 "FILE:/etc/krb5.keytab", 
  default_keytab_modify = 0x0, use_admin_kdc = 0, extra_addresses = 0x0, 
  scan_interfaces = 1, srv_lookup = 1, srv_try_txt = 0, fcache_vno = 0, 
  num_kt_types = 6, kt_types = 0x60ecd0, date_fmt = 0x779abaf8 "%Y-%m-%d", 
  error_string = 0x6112e0 "No ENC-TS found", error_code = -1765328383, 
(...)

And because the return code ret is the same as the error_code in the
context, krb5_get_error_message() just copies the string from the
context. However, if krb5_get_error_message() does its own lookup of
-1765328383 it gets "Client's entry in database has expired" which is
more like it. But where does "No ENC-TS found" come from and why is it
"better" than the own lookup?

Of course the error string printing function could return all text it
finds and leave the interpretation to the user. Like this, patch at
the end.

$ /usr/heimdal/bin/kinit 
haba/t...@nada.kth.se's Password: 
kinit: krb5_get_init_creds: Client's entry in database has expired, No ENC-TS 
found


String operations i C are always fun.

Harald.


--- error_string.c.orig 2017-07-11 07:14:16.0 +0200
+++ error_string.c  2017-10-05 13:46:04.654728188 +0200
@@ -234,8 +234,8 @@
 }
HEIMDAL_MUTEX_unlock(&context->mutex);
 
-if (str)
-return str;
+/*if (str)
+ return str; return later*/
 }
 else
 {
@@ -249,10 +249,19 @@
 if (free_context)
 krb5_free_context(context);
 
-if (cstr)
-return strdup(cstr);
+if (!cstr)
+   cstr = error_message(code);
+
+if (str && !cstr)
+   return str;
 
-cstr = error_message(code);
+if (str && cstr)
+   if (strncmp(str, cstr, sizeof(buf)) == 0)
+   return str;
+   else
+   strncat(cstr, ", ", sizeof(buf));
+   return strdup(strncat(cstr, str, sizeof(buf)));
+
 if (cstr)
 return strdup(cstr);
 


Harald.

kinit error message (Heimdal 7.4.0)

[Harald Barth]

I'm currently looking at why kinit can not give a decent error message
on the easy fact that a credential has expired. Well, now with 7.4.0
it handles "password expired" but "principal expired" still gives:

kinit: krb5_get_init_creds: No ENC-TS found

which is very broken from a user support group view. I tracked this
down to the call in kinit.c line 673 which gets handled by the
default: in the following switch(ret) with ret=-1765328383 Is that
KRB5KDC_ERR_NAME_EXP - but how does that get translated to "No ENC-TS
found"?



ret = krb5_init_creds_get(context, ctx);

#ifndef NO_NTLM
if (ntlm_domain && passwd[0])
heim_ntlm_nt_key(passwd, &ntlmkey);
#endif
memset(passwd, 0, sizeof(passwd));

switch(ret){
case 0:
break;
case KRB5_LIBOS_PWDINTR: /* don't print anything if it was just C-c:ed */
exit(1);
case KRB5KRB_AP_ERR_BAD_INTEGRITY:
case KRB5KRB_AP_ERR_MODIFIED:
case KRB5KDC_ERR_PREAUTH_FAILED:
case KRB5_GET_IN_TKT_LOOP:
krb5_warnx(context, N_("Password incorrect", ""));
goto out;
case KRB5KRB_AP_ERR_V4_REPLY:
krb5_warnx(context, N_("Looks like a Kerberos 4 reply", ""));
goto out;
case KRB5KDC_ERR_KEY_EXPIRED:
krb5_warnx(context, N_("Password expired", ""));
goto out;
default:
krb5_warn(context, ret, "krb5_get_init_creds");
goto out;
}

---

Questions:

1. How do I get the list of all KRB5KDC_ERR_* values and where are
these defined? 

2. What possible error values can come back from krb5_init_creds_get()
and how to deal with them better?

3. Should the error handling and generation of the error string be in
this switch() or should it be by some krb5_error_something function?

Harald.