From jik@kamens.brookline.ma.us Tue Nov 23 16:48:18 1999 Received: from MIT.EDU (PACIFIC-CARRIER-ANNEX.MIT.EDU [18.69.0.28]) by rt-11.MIT.EDU (8.7.5/8.7.3) with SMTP id QAA02226 for ; Tue, 23 Nov 1999 16:48:18 -0500 Received: from nautilus.shore.net by MIT.EDU with SMTP id AA03876; Tue, 23 Nov 99 16:48:45 EST Received: from jik.shore.net [206.243.167.15] by nautilus.shore.net with esmtp (Exim) for krb5-bugs@mit.edu id 11qNnE-0001v5-00; Tue, 23 Nov 1999 16:48:08 -0500 Received: from jik2.kamens.brookline.ma.us (IDENT:root@jik2.kamens.brookline.ma.us [192.168.0.3]) by jik.shore.net (8.9.0/8.9.0) with ESMTP id QAA20904; Tue, 23 Nov 1999 16:47:34 -0500 Received: (from jik@localhost) by jik2.kamens.brookline.ma.us (8.9.3/8.9.3) id QAA20937; Tue, 23 Nov 1999 16:48:23 -0500 Message-Id: <199911232148.QAA20937@jik2.kamens.brookline.ma.us> Date: Tue, 23 Nov 1999 16:48:23 -0500 From: Jonathan Kamens Reply-To: jik@kamens.brookline.ma.us To: krb5-bugs@MIT.EDU Cc: jik@kamens.brookline.ma.us Subject: Keytab code should cache last-read key X-Send-Pr-Version: 3.99 >Number: 786 >Category: krb5-libs >Synopsis: Keytab code should cache last-read key >Confidential: yes >Severity: non-critical >Priority: medium >Responsible: krb5-unassigned >State: open >Class: change-request >Submitter-Id: unknown >Arrival-Date: Tue Nov 23 16:49:00 EST 1999 >Last-Modified: Tue Nov 30 15:58:01 EST 1999 >Originator: Jonathan Kamens >Organization: >Release: krb5-1.1 >Environment: System: Linux jik2 2.2.12-20 #14 Thu Nov 18 20:57:02 EST 1999 i686 unknown Architecture: i686 >Description: Every time a Kerberos application server gets a request, it has to read its ticket file to process the request. For a very busy server, this is very inefficient. It's also unnecessary, since the vast majority of servers receive requests for the same service principal over and over. The patch below caches the last key read from the keytab, so that the keytab does not have to be read over and over as long as requests keep coming in for the same service principal. Note that these patches were previously submitted against krb5 1.0. I tested them pretty extensively before submitting them against 1.0, but I have *not* tested them extensively against 1.1, since I'm not currently using any krb5 application servers. I've simply merged my changes from 1.0 into a current 1.1 tree, which required some additional changes since the 1.1 code in question had changed a bit. Therefore, somebody who is more actively involved in doing krb5 development than I am right now should test these changes before committing and releasing them. >How-To-Repeat: >Fix: Index: ./src/lib/krb5/keytab/file/ktf_close.c --- ktf_close.c 1999/11/23 20:25:26 1.1.1.1 +++ ktf_close.c 1999/11/23 21:37:04 1.3 @@ -44,6 +44,8 @@ */ { krb5_xfree(KTFILENAME(id)); + if (KTCACHE(id).magic) + krb5_kt_free_entry(context, &KTCACHE(id)); krb5_xfree(id->data); id->ops = 0; krb5_xfree(id); Index: ./src/lib/krb5/keytab/file/ktf_g_ent.c --- ktf_g_ent.c 1999/11/23 20:25:26 1.1.1.1 +++ ktf_g_ent.c 1999/11/23 21:37:04 1.3 @@ -45,11 +45,11 @@ krb5_error_code kerror = 0; int found_wrong_kvno = 0; krb5_boolean similar; + /* 0 - haven't checked the cache yet + 1 - checked the cache, but haven't opened the ticket file yet + 2 - opened the ticket file */ + int checking_state = 0; - /* Open the keyfile for reading */ - if ((kerror = krb5_ktfileint_openr(context, id))) - return(kerror); - /* * For efficiency and simplicity, we'll use a while true that * is exited with a break statement. @@ -59,8 +59,28 @@ cur_entry.key.contents = 0; while (TRUE) { - if ((kerror = krb5_ktfileint_read_entry(context, id, &new_entry))) + if (checking_state == 0) { + checking_state++; + if (! KTCACHE(id).magic) + continue; + krb5_ktfileint_copy_entry(context, &KTCACHE(id), &new_entry); + } + else { + if (checking_state == 1) { + /* Free the cache entry, if there is one */ + if (KTCACHE(id).magic) { + krb5_kt_free_entry(context, &KTCACHE(id)); + KTCACHE(id).magic = 0; + } + /* Open the keyfile for reading */ + if ((kerror = krb5_ktfileint_openr(context, id))) + return(kerror); + checking_state++; + } + + if ((kerror = krb5_ktfileint_read_entry(context, id, &new_entry))) break; + } /* by the time this loop exits, it must either free cur_entry, and copy new_entry there, or free new_entry. Otherwise, it @@ -137,6 +157,8 @@ krb5_kt_free_entry(context, &cur_entry); return kerror; } + if (checking_state == 2) + (void) krb5_ktfileint_copy_entry(context, &cur_entry, &KTCACHE(id)); *entry = cur_entry; return 0; } Index: ./src/lib/krb5/keytab/file/ktfile.h --- ktfile.h 1999/11/23 20:25:26 1.1.1.1 +++ ktfile.h 1999/11/23 21:37:04 1.3 @@ -52,6 +52,7 @@ char *name; /* Name of the file */ FILE *openf; /* open file, if any. */ int version; /* Version number of keytab */ + krb5_keytab_entry cache; /* Cached last keytab entry returned by get_entry */ } krb5_ktfile_data; /* @@ -61,6 +62,7 @@ #define KTFILENAME(id) (((krb5_ktfile_data *)(id)->data)->name) #define KTFILEP(id) (((krb5_ktfile_data *)(id)->data)->openf) #define KTVERSION(id) (((krb5_ktfile_data *)(id)->data)->version) +#define KTCACHE(id) (((krb5_ktfile_data *)(id)->data)->cache) extern struct _krb5_kt_ops krb5_ktf_ops; extern struct _krb5_kt_ops krb5_ktf_writable_ops; @@ -163,6 +165,11 @@ krb5_keytab, krb5_int32 *, krb5_int32 *)); + +krb5_error_code krb5_ktfileint_copy_entry + PROTOTYPE((krb5_context, + krb5_keytab_entry *, + krb5_keytab_entry *)); #endif /* KRB5_KTFILE__ */ Index: ./src/lib/krb5/keytab/file/ktf_resolv.c --- ktf_resolv.c 1999/11/23 20:25:26 1.1.1.1 +++ ktf_resolv.c 1999/11/23 21:37:04 1.3 @@ -56,6 +56,7 @@ (void) strcpy(data->name, name); data->openf = 0; + memset(&data->cache, 0, sizeof(data->cache)); data->version = 0; (*id)->data = (krb5_pointer)data; Index: ./src/lib/krb5/keytab/file/ktf_util.c --- ktf_util.c 1999/11/23 20:25:26 1.1.1.1 +++ ktf_util.c 1999/11/23 21:37:04 1.3 @@ -774,3 +774,31 @@ return 0; } +/* + * Copy a keytab entry's data from an old structure into a new + * (already allocated) structure. + */ + +krb5_error_code +krb5_ktfileint_copy_entry(context, old_entry, new_entry) + krb5_context context; + krb5_keytab_entry *old_entry, *new_entry; +{ + krb5_error_code code; + krb5_keytab_entry tmp_entry; + + tmp_entry = *old_entry; + + if ((code = krb5_copy_principal(context, old_entry->principal, + &tmp_entry.principal))) + return code; + + if ((code = krb5_copy_keyblock_contents(context, &old_entry->key, + &tmp_entry.key))) { + (void) krb5_free_principal(context, tmp_entry.principal); + return code; + } + + *new_entry = tmp_entry; + return 0; +} Index: ./src/lib/krb5/keytab/file/ktf_wreslv.c --- ktf_wreslv.c 1999/11/23 20:25:26 1.1.1.1 +++ ktf_wreslv.c 1999/11/23 21:37:04 1.3 @@ -56,6 +56,7 @@ (void) strcpy(data->name, name); data->openf = 0; + memset(&data->cache, 0, sizeof(data->cache)); data->version = 0; (*id)->data = (krb5_pointer)data; >Audit-Trail: From: tytso@MIT.EDU To: jik@kamens.brookline.ma.us Cc: krb5-bugs@MIT.EDU, krb5-unassigned@RT-11.MIT.EDU, jik@kamens.brookline.ma.us, gnats-admin@RT-11.MIT.EDU, krb5-prs@RT-11.MIT.EDU Subject: Re: krb5-libs/786: Keytab code should cache last-read key Date: Mon, 29 Nov 1999 14:25:23 -0500 Date: Tue, 23 Nov 1999 16:48:23 -0500 From: Jonathan Kamens Every time a Kerberos application server gets a request, it has to read its ticket file to process the request. For a very busy server, this is very inefficient. It's also unnecessary, since the vast majority of servers receive requests for the same service principal over and over. The patch below caches the last key read from the keytab, so that the keytab does not have to be read over and over as long as requests keep coming in for the same service principal. Should there perhaps be a stat() test to see if the keytab has changed, or is the assumption that you will kill and restart all application daemons when the keytab file is changed? - Ted From: Jonathan Kamens To: tytso@MIT.EDU Cc: krb5-bugs@MIT.EDU, krb5-unassigned@RT-11.MIT.EDU, gnats-admin@RT-11.MIT.EDU, krb5-prs@RT-11.MIT.EDU Subject: Re: krb5-libs/786: Keytab code should cache last-read key Date: Mon, 29 Nov 1999 14:40:44 -0500 > Date: Mon, 29 Nov 1999 14:25:23 -0500 > From: tytso@MIT.EDU > > Should there perhaps be a stat() test to see if the keytab has changed, > or is the assumption that you will kill and restart all application > daemons when the keytab file is changed? It doesn't really matter if the keytab has changed, since the caching I implemented checks the key version number of the cached key. That is, if a new key is issued for the server, then the tickets issued to clients will use the new key, and therefore they will not match the cached key, so the keytab file will be read. I suppose you could say that a site might bump the version number on an application server key to lock out clients which snooped an old key version or something like that, but that seems exceedingly unlikely, and I don't think protecting against that remote possibility is worth the additional disk activity of a stat() for every client connection. jik From: tytso@MIT.EDU To: jik@kamens.brookline.ma.us Cc: krb5-bugs@MIT.EDU, krb5-unassigned@RT-11.MIT.EDU, gnats-admin@RT-11.MIT.EDU, krb5-prs@RT-11.MIT.EDU Subject: Re: krb5-libs/786: Keytab code should cache last-read key Date: Tue, 30 Nov 1999 15:55:54 -0500 Date: Mon, 29 Nov 1999 14:40:44 -0500 From: Jonathan Kamens It doesn't really matter if the keytab has changed, since the caching I implemented checks the key version number of the cached key. That is, if a new key is issued for the server, then the tickets issued to clients will use the new key, and therefore they will not match the cached key, so the keytab file will be read. Good point, and if a site wants to bump the version number to lock out a compromised key, it's probably not unreasonable to require that the adminsitrator kill and restart the application server. (And of course, none of this applies for servers run out of inetd.) - Ted >Unformatted: