[PATCH] krb5: fix entry order in MEMORY keytab

[Sumit Bose]

Hi,

this patch should fix https://fedorahosted.org/sssd/ticket/2557 . Please
see the commit message for details.

bye,
Sumit

[Lukas Slebodnik]

On (15/01/15 11:15), Sumit Bose wrote:
> Hi,
>
> this patch should fix https://fedorahosted.org/sssd/ticket/2557 . Please
> see the commit message for details.
>
> bye,
> Sumit
> From 92c1edab650835f4a06bceeff20637a87683aa7a Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Thu, 15 Jan 2015 10:38:33 +0100
> Subject: [PATCH] krb5: fix entry order in MEMORY keytab
>
> Since krb5_kt_add_entry() adds new entries at the beginning of a MEMORY
> type keytab and not at the end a simple copy into a MEMORY type keytab
> will revert the order of the keytab entries. Since e.g. the sssd_krb5
> man page give hints about where to add entries into keytab files to help
> SSSD to find a right entry we have to keep the order when coping a
> keytab into a MEMORY type keytab. This patch fixes this by doing a
> second copy to retain the original order.
> Resolves https://fedorahosted.org/sssd/ticket/2557

I can confirm that ticket 2557 is fixed.
I would like to run other krb5 related test to be sure
we won't introduce another regression.

LS

[Jakub Hrozek]

On Thu, Jan 15, 2015 at 02:00:57PM +0100, Lukas Slebodnik wrote:
> On (15/01/15 11:15), Sumit Bose wrote:
> > Hi,
> >
> > this patch should fix https://fedorahosted.org/sssd/ticket/2557 . Please
> > see the commit message for details.
> >
> > bye,
> > Sumit
> > From 92c1edab650835f4a06bceeff20637a87683aa7a Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > Date: Thu, 15 Jan 2015 10:38:33 +0100
> > Subject: [PATCH] krb5: fix entry order in MEMORY keytab
> >
> > Since krb5_kt_add_entry() adds new entries at the beginning of a MEMORY
> > type keytab and not at the end a simple copy into a MEMORY type keytab
> > will revert the order of the keytab entries. Since e.g. the sssd_krb5
> > man page give hints about where to add entries into keytab files to help
> > SSSD to find a right entry we have to keep the order when coping a
> > keytab into a MEMORY type keytab. This patch fixes this by doing a
> > second copy to retain the original order.
> > Resolves https://fedorahosted.org/sssd/ticket/2557
>
> I can confirm that ticket 2557 is fixed.
> I would like to run other krb5 related test to be sure
> we won't introduce another regression.

Please go ahead and double-ack the patch if your tests pass. Thank you!

[Jakub Hrozek]

On Thu, Jan 15, 2015 at 11:15:23AM +0100, Sumit Bose wrote:
> Hi,
>
> this patch should fix https://fedorahosted.org/sssd/ticket/2557 . Please
> see the commit message for details.
>
> bye,
> Sumit

Thank you for a fast fix along with a unit test.

The tests in my environment passed and the code looks good to me, so I
give my ACK.

Before pushing, I've asked the original reporter to confirm the fix
helps solve his problem and submitted the patch for automated checks.

[Lukas Slebodnik]

On (15/01/15 11:15), Sumit Bose wrote:
> Hi,
>
> this patch should fix https://fedorahosted.org/sssd/ticket/2557 . Please
> see the commit message for details.
>
> bye,
> Sumit

realmd test (regression) passed.
ldap + krb5              passed.
ad                       passed.

There are two tiny warnings from static analysers.
> From 92c1edab650835f4a06bceeff20637a87683aa7a Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Thu, 15 Jan 2015 10:38:33 +0100
> Subject: [PATCH] krb5: fix entry order in MEMORY keytab
>
> Since krb5_kt_add_entry() adds new entries at the beginning of a MEMORY
> type keytab and not at the end a simple copy into a MEMORY type keytab
> will revert the order of the keytab entries. Since e.g. the sssd_krb5
> man page give hints about where to add entries into keytab files to help
> SSSD to find a right entry we have to keep the order when coping a
> keytab into a MEMORY type keytab. This patch fixes this by doing a
> second copy to retain the original order.
> Resolves https://fedorahosted.org/sssd/ticket/2557
>
> src/providers/krb5/krb5_keytab.c    | 106 +++++++++++++++++++++++++++---------
> src/tests/cmocka/test_copy_keytab.c |  82 ++++++++++++++++++++++++++++
> 2 files changed, 161 insertions(+), 27 deletions(-)
>
> diff --git a/src/providers/krb5/krb5_keytab.c b/src/providers/krb5/krb5_keytab.c
> index 0d6a85c0b8b02937fb2ee6b058174243b2e56114..5f439440c5f153057c4fd3220d7f466f6e191fea 100644
> --- a/src/providers/krb5/krb5_keytab.c
> +++ b/src/providers/krb5/krb5_keytab.c
> @@ -25,20 +25,68 @@
> #include "util/util.h"
> #include "util/sss_krb5.h"
>
> +static krb5_error_code do_keytab_copy(krb5_context kctx, krb5_keytab s_keytab,
>                                  krb5_keytab d_keytab)
>
> +{
> krb5_error_code kerr;
> krb5_error_code kt_err;
> krb5_kt_cursor cursor;
> krb5_keytab_entry entry;
> memset(&cursor, 0, sizeof(cursor));
> kerr = krb5_kt_start_seq_get(kctx, s_keytab, &cursor);
> if (kerr != 0) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "error reading keytab.\n");
>    return kerr;
> }
> memset(&entry, 0, sizeof(entry));
> while ((kt_err = krb5_kt_next_entry(kctx, s_keytab, &entry,
>                                    &cursor)) == 0) {
>    kerr = krb5_kt_add_entry(kctx, d_keytab, &entry);
>    if (kerr != 0) {
>        DEBUG(SSSDBG_OP_FAILURE, "krb5_kt_add_entry failed.\n");
>        kt_err = krb5_kt_end_seq_get(kctx, s_keytab, &cursor);

^^^^^^
              this value is ignored
>        return kerr;
>    }
>    kerr = sss_krb5_free_keytab_entry_contents(kctx, &entry);
>    if (kerr != 0) {
>        DEBUG(SSSDBG_MINOR_FAILURE, "Failed to free keytab entry.\n");
>        kt_err = krb5_kt_end_seq_get(kctx, s_keytab, &cursor);

^^^^^^ this value is ignored

Error: UNUSED_VALUE (CWE-563): [#def1]
sssd-1.12.90/src/providers/krb5/krb5_keytab.c:49: returned_value: Value from "krb5_kt_end_seq_get(kctx, s_keytab, &cursor)" is assigned to "kt_err" here, but that stored value is not used.

Error: UNUSED_VALUE (CWE-563): [#def2]
sssd-1.12.90/src/providers/krb5/krb5_keytab.c:56: returned_value: Value from "krb5_kt_end_seq_get(kctx, s_keytab, &cursor)" is assigned to "kt_err" here, but that stored value is not used.
>        return kerr;
>    }
>    memset(&entry, 0, sizeof(entry));
> }
> kerr = krb5_kt_end_seq_get(kctx, s_keytab, &cursor);
> if (kerr != 0) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "krb5_kt_end_seq_get failed.\n");
>    return kerr;
> }
> /* check if we got any errors from krb5_kt_next_entry */
> if (kt_err != 0 && kt_err != KRB5_KT_END) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "error reading keytab.\n");
>    return kt_err;
> }
> return 0;
>
> +}
>
> krb5_error_code copy_keytab_into_memory(TALLOC_CTX *mem_ctx, krb5_context kctx,
>                                         char *inp_keytab_file,
>                                         char **_mem_name,
>                                         krb5_keytab *_mem_keytab)
> {
>     krb5_error_code kerr;
> krb5_error_code kt_err;
> krb5_keytab keytab = NULL;
> krb5_keytab mem_keytab = NULL;
> krb5_kt_cursor cursor;
> krb5_keytab_entry entry;
> krb5_keytab tmp_mem_keytab = NULL;
> char keytab_name[MAX_KEYTAB_NAME_LEN];
> char *sep;
> char *mem_name = NULL;
> char *tmp_mem_name = NULL;
> char *keytab_file;
> char default_keytab_name[MAX_KEYTAB_NAME_LEN];
>
> @@ -103,6 +151,13 @@ krb5_error_code copy_keytab_into_memory(TALLOC_CTX *mem_ctx, krb5_context kctx,
>         goto done;
>     }
> tmp_mem_name = talloc_asprintf(mem_ctx, "MEMORY:%s.tmp", sep + 1);
> if (tmp_mem_name == NULL) {
>    DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n");
>    kerr = KRB5KRB_ERR_GENERIC;
>    goto done;
> }
> kerr = krb5_kt_resolve(kctx, mem_name, &mem_keytab);
>   if (kerr != 0) {
>       DEBUG(SSSDBG_CRIT_FAILURE, "error resolving keytab [%s].\n",
>
> @@ -110,38 +165,29 @@ krb5_error_code copy_keytab_into_memory(TALLOC_CTX *mem_ctx, krb5_context kctx,
>         goto done;
>     }
> memset(&cursor, 0, sizeof(cursor));
> kerr = krb5_kt_start_seq_get(kctx, keytab, &cursor);
> kerr = krb5_kt_resolve(kctx, tmp_mem_name, &tmp_mem_keytab);
> if (kerr != 0) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "error reading keytab [%s].\n", keytab_file);
>    DEBUG(SSSDBG_CRIT_FAILURE, "error resolving keytab [%s].\n",
>                                tmp_mem_name);
>   goto done;
> }
> memset(&entry, 0, sizeof(entry));
> while ((kt_err = krb5_kt_next_entry(kctx, keytab, &entry, &cursor)) == 0) {
>    kerr = krb5_kt_add_entry(kctx, mem_keytab, &entry);
>    if (kerr != 0) {
>        DEBUG(SSSDBG_OP_FAILURE, "krb5_kt_add_entry failed.\n");
>        goto done;
>    }
>    kerr = sss_krb5_free_keytab_entry_contents(kctx, &entry);
>    if (kerr != 0) {
>        DEBUG(SSSDBG_MINOR_FAILURE, "Failed to free keytab entry.\n");
>    }
>    memset(&entry, 0, sizeof(entry));
> }
> kerr = krb5_kt_end_seq_get(kctx, keytab, &cursor);
> kerr = do_keytab_copy(kctx, keytab, tmp_mem_keytab);
> if (kerr != 0) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "krb5_kt_end_seq_get failed.\n");
>    DEBUG(SSSDBG_CRIT_FAILURE, "Failed to copy keytab [%s] into [%s].\n",
>                                keytab_file, tmp_mem_name);
>   goto done;
> }
> /* check if we got any errors from krb5_kt_next_entry */
> if (kt_err != 0 && kt_err != KRB5_KT_END) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "error reading keytab [%s].\n", keytab_file);
>    kerr = KRB5KRB_ERR_GENERIC;
> /* krb5_kt_add_entry() adds new entries into MEMORY keytabs at the
> * beginning and not at the end as for FILE keytabs. Since we want to keep
> * the processing order we have to copy the MEMORY keytab again to retain
> * the order from the FILE keytab. */
> kerr = do_keytab_copy(kctx, tmp_mem_keytab, mem_keytab);
> if (kerr != 0) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Failed to copy keytab [%s] into [%s].\n",
>                                tmp_mem_name, mem_name);
>   goto done;
> }
>
> @@ -153,10 +199,16 @@ krb5_error_code copy_keytab_into_memory(TALLOC_CTX *mem_ctx, krb5_context kctx,
>     kerr = 0;
> done:
>
> talloc_free(tmp_mem_name);
>
> if (kerr != 0) {
>       talloc_free(mem_name);
>   }
>
> if (tmp_mem_keytab != NULL && krb5_kt_close(kctx, tmp_mem_keytab) != 0) {
>        DEBUG(SSSDBG_MINOR_FAILURE, "krb5_kt_close failed");

^^^^
You will be changing code anyway. So you can fix this as well :-)
 4 spaces are enough
> }
> if (keytab != NULL && krb5_kt_close(kctx, keytab) != 0) {
>           DEBUG(SSSDBG_MINOR_FAILURE, "krb5_kt_close failed");
>   }

LS

[Sumit Bose]

On Fri, Jan 16, 2015 at 09:28:30PM +0100, Lukas Slebodnik wrote:
> On (15/01/15 11:15), Sumit Bose wrote:
> > Hi,
> >
> > this patch should fix https://fedorahosted.org/sssd/ticket/2557 . Please
> > see the commit message for details.
> >
> > bye,
> > Sumit
>
> realmd test (regression) passed.
> ldap + krb5              passed.
> ad                       passed.
>
> There are two tiny warnings from static analysers.

...
> >                                    &cursor)) == 0) {
> >    kerr = krb5_kt_add_entry(kctx, d_keytab, &entry);
> >    if (kerr != 0) {
> >        DEBUG(SSSDBG_OP_FAILURE, "krb5_kt_add_entry failed.\n");
> >        kt_err = krb5_kt_end_seq_get(kctx, s_keytab, &cursor);
>           ^^^^^^
>           this value is ignored 
> >        return kerr;
> >    }
> >    kerr = sss_krb5_free_keytab_entry_contents(kctx, &entry);
> >    if (kerr != 0) {
> >        DEBUG(SSSDBG_MINOR_FAILURE, "Failed to free keytab entry.\n");
> >        kt_err = krb5_kt_end_seq_get(kctx, s_keytab, &cursor);
>           ^^^^^^ this value is ignored
>
> Error: UNUSED_VALUE (CWE-563): [#def1]
> sssd-1.12.90/src/providers/krb5/krb5_keytab.c:49: returned_value: Value from "krb5_kt_end_seq_get(kctx, s_keytab, &cursor)" is assigned to "kt_err" here, but that stored value is not used.
>
> Error: UNUSED_VALUE (CWE-563): [#def2]
> sssd-1.12.90/src/providers/krb5/krb5_keytab.c:56: returned_value: Value from "krb5_kt_end_seq_get(kctx, s_keytab, &cursor)" is assigned to "kt_err" here, but that stored value is not used.

I skipped them intentionally, because they are of no use, but I forgot
about the analysers. Fixed.
> >        return kerr;
> >    }
> >    memset(&entry, 0, sizeof(entry));
> > }
> > kerr = krb5_kt_end_seq_get(kctx, s_keytab, &cursor);
> > if (kerr != 0) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "krb5_kt_end_seq_get failed.\n");
> >    return kerr;
> > }

...
> > @@ -153,10 +199,16 @@ krb5_error_code copy_keytab_into_memory(TALLOC_CTX *mem_ctx, krb5_context kctx,
> >     kerr = 0;
> > done:
> >
> > talloc_free(tmp_mem_name);
> >
> > if (kerr != 0) {
> >       talloc_free(mem_name);
> >   }
> >
> > if (tmp_mem_keytab != NULL && krb5_kt_close(kctx, tmp_mem_keytab) != 0) {
> >        DEBUG(SSSDBG_MINOR_FAILURE, "krb5_kt_close failed");
>   ^^^^
>
> You will be changing code anyway. So you can fix this as well :-)
>  4 spaces are enough

fixed
> > }
> > if (keytab != NULL && krb5_kt_close(kctx, keytab) != 0) {
> >           DEBUG(SSSDBG_MINOR_FAILURE, "krb5_kt_close failed");
> >   }
>
> LS

Thank you for the checks and the review, new version attached.

bye,
Sumit

[Lukas Slebodnik]

On (19/01/15 09:23), Sumit Bose wrote:
> On Fri, Jan 16, 2015 at 09:28:30PM +0100, Lukas Slebodnik wrote:
> > On (15/01/15 11:15), Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch should fix https://fedorahosted.org/sssd/ticket/2557 . Please
> > > see the commit message for details.
> > >
> > > bye,
> > > Sumit
> >
> > realmd test (regression) passed.
> > ldap + krb5              passed.
> > ad                       passed.
> >
> > There are two tiny warnings from static analysers.
>
> Thank you for the checks and the review, new version attached.
>
> bye,
> Sumit
> From cb449c569fe18f0a5fc150de4c6724ab0a3146e5 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Thu, 15 Jan 2015 10:38:33 +0100
> Subject: [PATCH] krb5: fix entry order in MEMORY keytab
>
> Since krb5_kt_add_entry() adds new entries at the beginning of a MEMORY
> type keytab and not at the end a simple copy into a MEMORY type keytab
> will revert the order of the keytab entries. Since e.g. the sssd_krb5
> man page give hints about where to add entries into keytab files to help
> SSSD to find a right entry we have to keep the order when coping a
> keytab into a MEMORY type keytab. This patch fixes this by doing a
> second copy to retain the original order.
> Resolves https://fedorahosted.org/sssd/ticket/2557

ACK++

http://sssd-ci.duckdns.org/logs/job/6/19/summary.html

LS

[Jakub Hrozek]

On Mon, Jan 19, 2015 at 10:29:08AM +0100, Lukas Slebodnik wrote:
> On (19/01/15 09:23), Sumit Bose wrote:
> > On Fri, Jan 16, 2015 at 09:28:30PM +0100, Lukas Slebodnik wrote:
> > > On (15/01/15 11:15), Sumit Bose wrote:
> > > > Hi,
> > > >
> > > > this patch should fix https://fedorahosted.org/sssd/ticket/2557 . Please
> > > > see the commit message for details.
> > > >
> > > > bye,
> > > > Sumit
> > >
> > > realmd test (regression) passed.
> > > ldap + krb5              passed.
> > > ad                       passed.
> > >
> > > There are two tiny warnings from static analysers.
> >
> > Thank you for the checks and the review, new version attached.
> >
> > bye,
> > Sumit
> > From cb449c569fe18f0a5fc150de4c6724ab0a3146e5 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > Date: Thu, 15 Jan 2015 10:38:33 +0100
> > Subject: [PATCH] krb5: fix entry order in MEMORY keytab
> >
> > Since krb5_kt_add_entry() adds new entries at the beginning of a MEMORY
> > type keytab and not at the end a simple copy into a MEMORY type keytab
> > will revert the order of the keytab entries. Since e.g. the sssd_krb5
> > man page give hints about where to add entries into keytab files to help
> > SSSD to find a right entry we have to keep the order when coping a
> > keytab into a MEMORY type keytab. This patch fixes this by doing a
> > second copy to retain the original order.
> > Resolves https://fedorahosted.org/sssd/ticket/2557
>
> ACK++
>
> http://sssd-ci.duckdns.org/logs/job/6/19/summary.html

master: 576ad637181b80d39a4e136c9afbf34c57f76156
sssd-1-12: 24df1487413d13248dcc70d2548a763930da4c65