[PATCH] documentation fixes and keytab handling regression
Jeremy Allison
jra at samba.org
Fri Dec 16 00:54:16 UTC 2016
On Thu, Dec 15, 2016 at 09:59:51PM +0100, Andreas Schneider wrote:
> On Wednesday, 14 December 2016 10:53:07 CET jim wrote:
> > This isn't correct.
> > cmp1 != 0 test is only proper if strncmp(keytab_name_req, "WRFILE:", 7)
> > == 0.
> > cmp2 != 0 test is only proper if strncmp(keytab_name_req, "FILE:", 5) == 0.
> >
> > On 12/14/2016 10:48 AM, Andreas Schneider wrote:
> > > + int cmp1, cmp2;
> > > +
> > > + cmp1 = strncmp(keytab_name_req, "WRFILE:/", 8);
> > > + cmp2 = strncmp(keytab_name_req,"FILE:/", 6);
> > > + if (cmp1 != 0 && cmp2 != 0) {
> > > + return KRB5_KT_BADNAME;
> > > + }
> > > +
> > >
> > > if (keytab_name_req[0] != '/') {
> > >
> > > return KRB5_KT_BADNAME;
> > >
> > > }
>
> Here is a patchset including unit tests written in cmocka. Sadly our autobuild
> host only offers cmocka 0.3.2 which is way too old. The test requires at least
> version 1.0 which has support for subunit output. So it will not run on
> autobuild until we upgrade to a newer version.
>
>
> Review and push appreciated.
LGTM Andreas ! Thanks for the tests. Hopefully
the autobuild host will update cmocka soon.
Pushed.
Cheers,
Jeremy.
>
>
> Andreas
>
> --
> Andreas Schneider GPG-ID: CC014E3D
> Samba Team asn at samba.org
> www.samba.org
> From 02f12973068334ceddc3c0c7b07d3e5d1f9f6e4a Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Wed, 14 Dec 2016 16:43:53 +0100
> Subject: [PATCH 1/5] s3:crypto: Use smb_krb5_kt_open_relative() for MEMORY
> keytab
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/librpc/crypto/gse_krb5.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/librpc/crypto/gse_krb5.c b/source3/librpc/crypto/gse_krb5.c
> index 71b2338c0ba..f7aac9ebe07 100644
> --- a/source3/librpc/crypto/gse_krb5.c
> +++ b/source3/librpc/crypto/gse_krb5.c
> @@ -431,7 +431,7 @@ static krb5_error_code fill_mem_keytab_from_system_keytab(krb5_context krbctx,
> ZERO_STRUCT(kt_entry);
> ZERO_STRUCT(kt_cursor);
>
> - ret = smb_krb5_kt_open(krbctx, NULL, false, &keytab);
> + ret = smb_krb5_kt_open_relative(krbctx, NULL, false, &keytab);
> if (ret) {
> DEBUG(1, ("smb_krb5_kt_open failed (%s)\n",
> error_message(ret)));
> --
> 2.11.0
>
>
> From a185d69a8a2363b50c23af32c0271dfd39da81b3 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Wed, 14 Dec 2016 16:37:17 +0100
> Subject: [PATCH 2/5] krb5_wrap: More checks for absolute path in
> smb_krb5_kt_open()
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> lib/krb5_wrap/krb5_samba.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/lib/krb5_wrap/krb5_samba.c b/lib/krb5_wrap/krb5_samba.c
> index 28884d9044d..6991e585eef 100644
> --- a/lib/krb5_wrap/krb5_samba.c
> +++ b/lib/krb5_wrap/krb5_samba.c
> @@ -1130,12 +1130,29 @@ krb5_error_code smb_krb5_kt_open(krb5_context context,
> bool write_access,
> krb5_keytab *keytab)
> {
> - if (keytab_name_req != NULL) {
> - if (keytab_name_req[0] != '/') {
> - return KRB5_KT_BADNAME;
> - }
> + int cmp;
> +
> + if (keytab_name_req == NULL) {
> + return KRB5_KT_BADNAME;
> + }
> +
> + if (keytab_name_req[0] == '/') {
> + goto open_keytab;
> }
>
> + cmp = strncmp(keytab_name_req, "FILE:/", 6);
> + if (cmp == 0) {
> + goto open_keytab;
> + }
> +
> + cmp = strncmp(keytab_name_req, "WRFILE:/", 8);
> + if (cmp == 0) {
> + goto open_keytab;
> + }
> +
> + return KRB5_KT_BADNAME;
> +
> +open_keytab:
> return smb_krb5_kt_open_relative(context,
> keytab_name_req,
> write_access,
> --
> 2.11.0
>
>
> From db2971d55ff953b23d5c3126e0468eae0890c6f4 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Wed, 14 Dec 2016 16:40:23 +0100
> Subject: [PATCH 3/5] krb5_wrap: Remove incorrect absolute path checks in
> smb_krb5_kt_open_relative()
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> lib/krb5_wrap/krb5_samba.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/krb5_wrap/krb5_samba.c b/lib/krb5_wrap/krb5_samba.c
> index 6991e585eef..a8eafcd402e 100644
> --- a/lib/krb5_wrap/krb5_samba.c
> +++ b/lib/krb5_wrap/krb5_samba.c
> @@ -1023,8 +1023,8 @@ krb5_error_code smb_krb5_kt_open_relative(krb5_context context,
> goto out;
> }
>
> - if ((strncmp(keytab_name_req, "WRFILE:/", 8) == 0) ||
> - (strncmp(keytab_name_req, "FILE:/", 6) == 0)) {
> + if ((strncmp(keytab_name_req, "WRFILE:", 7) == 0) ||
> + (strncmp(keytab_name_req, "FILE:", 5) == 0)) {
> tmp = keytab_name_req;
> goto resolve;
> }
> --
> 2.11.0
>
>
> From d77f3bd09308864568c65aca41bf9ce549813e19 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Wed, 14 Dec 2016 16:44:10 +0100
> Subject: [PATCH 4/5] docs: Update doc to use absolute path for 'dedicated
> keytab file'
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> docs-xml/smbdotconf/security/dedicatedkeytabfile.xml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/docs-xml/smbdotconf/security/dedicatedkeytabfile.xml b/docs-xml/smbdotconf/security/dedicatedkeytabfile.xml
> index d516315af4f..8405b48028d 100644
> --- a/docs-xml/smbdotconf/security/dedicatedkeytabfile.xml
> +++ b/docs-xml/smbdotconf/security/dedicatedkeytabfile.xml
> @@ -5,7 +5,7 @@
> xmlns:samba="http://www.samba.org/samba/DTD/samba-doc">
> <description>
> <para>
> - Specifies the path to the kerberos keytab file when
> + Specifies the absolute path to the kerberos keytab file when
> <smbconfoption name="kerberos method"/> is set to "dedicated
> keytab".
> </para>
> --
> 2.11.0
>
>
> From 2c9ac23c82e7f89293c38901d4448a7cb4243196 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Thu, 15 Dec 2016 10:33:59 +0100
> Subject: [PATCH 5/5] testsuite: Add cmocka unit test for smb_krb5_kt_open()
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> selftest/tests.py | 15 +++-
> testsuite/unittests/test_krb5_samba.c | 145 ++++++++++++++++++++++++++++++++++
> testsuite/unittests/wscript | 15 ++++
> wscript | 1 +
> wscript_build | 1 +
> 5 files changed, 175 insertions(+), 2 deletions(-)
> create mode 100644 testsuite/unittests/test_krb5_samba.c
> create mode 100644 testsuite/unittests/wscript
>
> diff --git a/selftest/tests.py b/selftest/tests.py
> index 04a8df2d950..eabe71401fc 100644
> --- a/selftest/tests.py
> +++ b/selftest/tests.py
> @@ -26,13 +26,20 @@ except KeyError:
> samba4bindir = bindir()
> config_h = os.path.join(samba4bindir, "default/include/config.h")
>
> -# define here var to check what we support
> +# check available features
> +config_hash = dict()
> f = open(config_h, 'r')
> try:
> - have_man_pages_support = ("XSLTPROC_MANPAGES 1" in f.read())
> + lines = f.readlines()
> + config_hash = dict((x[0], ' '.join(x[1:]))
> + for x in map(lambda line: line.strip().split(' ')[1:],
> + filter(lambda line: (line[0:7] == '#define') and (len(line.split(' ')) > 2), lines)))
> finally:
> f.close()
>
> +have_man_pages_support = ("XSLTPROC_MANPAGES" in config_hash)
> +with_cmocka = ("HAVE_CMOCKA" in config_hash)
> +
> planpythontestsuite("none", "samba.tests.source")
> if have_man_pages_support:
> planpythontestsuite("none", "samba.tests.docs")
> @@ -123,3 +130,7 @@ planpythontestsuite("none", "samba.tests.kcc.graph_utils")
> planpythontestsuite("none", "samba.tests.kcc.kcc_utils")
> planpythontestsuite("none", "samba.tests.kcc.ldif_import_export")
> plantestsuite("wafsamba.duplicate_symbols", "none", [os.path.join(srcdir(), "buildtools/wafsamba/test_duplicate_symbol.sh")])
> +
> +if with_cmocka:
> + plantestsuite("samba.unittests.krb5samba", "none",
> + [os.path.join(bindir(), "default/testsuite/unittests/test_krb5samba")])
> diff --git a/testsuite/unittests/test_krb5_samba.c b/testsuite/unittests/test_krb5_samba.c
> new file mode 100644
> index 00000000000..8b7e843b85f
> --- /dev/null
> +++ b/testsuite/unittests/test_krb5_samba.c
> @@ -0,0 +1,145 @@
> +#include <stdarg.h>
> +#include <stddef.h>
> +#include <setjmp.h>
> +#include <cmocka.h>
> +
> +#include <krb5.h>
> +
> +#include "includes.h"
> +#include "lib/krb5_wrap/krb5_samba.h"
> +
> +
> +static int setup_krb5_context(void **state)
> +{
> + krb5_context context = NULL;
> + krb5_error_code code;
> +
> + code = krb5_init_context(&context);
> + assert_return_code(code, code);
> +
> + *state = context;
> +
> + return 0;
> +}
> +
> +static int teardown_krb5_context(void **state)
> +{
> + krb5_context context = *state;
> +
> + if (context != NULL) {
> + krb5_free_context(context);
> + }
> + return 0;
> +}
> +
> +static void test_smb_krb5_kt_open(void **state)
> +{
> + krb5_context context = *state;
> + krb5_keytab keytab = NULL;
> + krb5_error_code code;
> + char keytab_template[] = "/tmp/keytab.XXXXXX";
> + int fd;
> +
> + fd = mkstemp(keytab_template);
> + assert_return_code(fd, errno);
> + unlink(keytab_template);
> +
> + code = smb_krb5_kt_open(context,
> + keytab_template,
> + false,
> + &keytab);
> + assert_int_equal(code, 0);
> +
> + krb5_kt_close(context, keytab);
> + close(fd);
> +}
> +
> +static void test_smb_krb5_kt_open_file(void **state)
> +{
> + krb5_context context = *state;
> + krb5_keytab keytab = NULL;
> + krb5_error_code code;
> + char keytab_template[] = "/tmp/keytab.XXXXXX";
> + char keytab_file[6 + strlen(keytab_template)];
> + int fd;
> +
> + fd = mkstemp(keytab_template);
> + assert_return_code(fd, errno);
> + unlink(keytab_template);
> +
> + snprintf(keytab_file, sizeof(keytab_file), "FILE:%s", keytab_template);
> +
> + code = smb_krb5_kt_open(context,
> + keytab_file,
> + false,
> + &keytab);
> + assert_int_equal(code, 0);
> +
> + krb5_kt_close(context, keytab);
> + close(fd);
> +}
> +
> +static void test_smb_krb5_kt_open_fail(void **state)
> +{
> + krb5_context context = *state;
> + krb5_keytab keytab = NULL;
> + krb5_error_code code;
> +
> + code = smb_krb5_kt_open(context,
> + NULL,
> + false,
> + &keytab);
> + assert_int_equal(code, KRB5_KT_BADNAME);
> + code = smb_krb5_kt_open(context,
> + "wurst",
> + false,
> + &keytab);
> + assert_int_equal(code, KRB5_KT_BADNAME);
> +
> + code = smb_krb5_kt_open(context,
> + "FILE:wurst",
> + false,
> + &keytab);
> + assert_int_equal(code, KRB5_KT_BADNAME);
> +
> + code = smb_krb5_kt_open(context,
> + "WRFILE:wurst",
> + false,
> + &keytab);
> + assert_int_equal(code, KRB5_KT_BADNAME);
> +}
> +
> +static void test_smb_krb5_kt_open_relative_memory(void **state)
> +{
> + krb5_context context = *state;
> + krb5_keytab keytab = NULL;
> + krb5_error_code code;
> +
> + code = smb_krb5_kt_open_relative(context,
> + NULL,
> + true,
> + &keytab);
> + assert_int_equal(code, 0);
> +
> + krb5_kt_close(context, keytab);
> +}
> +
> +int main(void) {
> + const struct CMUnitTest tests[] = {
> + cmocka_unit_test_setup_teardown(test_smb_krb5_kt_open,
> + setup_krb5_context,
> + teardown_krb5_context),
> + cmocka_unit_test_setup_teardown(test_smb_krb5_kt_open_file,
> + setup_krb5_context,
> + teardown_krb5_context),
> + cmocka_unit_test_setup_teardown(test_smb_krb5_kt_open_fail,
> + setup_krb5_context,
> + teardown_krb5_context),
> + cmocka_unit_test_setup_teardown(test_smb_krb5_kt_open_relative_memory,
> + setup_krb5_context,
> + teardown_krb5_context),
> + };
> +
> + cmocka_set_message_output(CM_OUTPUT_SUBUNIT);
> + return cmocka_run_group_tests(tests, NULL, NULL);
> +}
> diff --git a/testsuite/unittests/wscript b/testsuite/unittests/wscript
> new file mode 100644
> index 00000000000..ea4af07799f
> --- /dev/null
> +++ b/testsuite/unittests/wscript
> @@ -0,0 +1,15 @@
> +#!/usr/bin/env python
> +
> +import os
> +
> +def configure(conf):
> + pkg_name = 'cmocka'
> + pkg_minversion = '1.0'
> +
> + conf.CHECK_BUNDLED_SYSTEM_PKG(pkg_name, minversion=pkg_minversion)
> +
> +def build(bld):
> + if bld.CONFIG_SET('HAVE_CMOCKA'):
> + bld.SAMBA_BINARY('test_krb5samba',
> + source='test_krb5_samba.c',
> + deps='krb5samba cmocka')
> diff --git a/wscript b/wscript
> index 9b0ee3912dd..9168db122ae 100644
> --- a/wscript
> +++ b/wscript
> @@ -188,6 +188,7 @@ def configure(conf):
> if conf.env.with_ctdb:
> conf.RECURSE('ctdb')
> conf.RECURSE('lib/socket')
> + conf.RECURSE('testsuite/unittests')
>
> conf.SAMBA_CHECK_UNDEFINED_SYMBOL_FLAGS()
>
> diff --git a/wscript_build b/wscript_build
> index 93b1832cf90..0c3a2aee864 100644
> --- a/wscript_build
> +++ b/wscript_build
> @@ -122,6 +122,7 @@ bld.RECURSE('libcli/samsync')
> bld.RECURSE('libcli/registry')
> bld.RECURSE('source4/lib/policy')
> bld.RECURSE('libcli/named_pipe_auth')
> +bld.RECURSE('testsuite/unittests')
>
> if bld.CONFIG_GET('KRB5_VENDOR') in (None, 'heimdal'):
> if bld.CONFIG_GET("HEIMDAL_KRB5_CONFIG") and bld.CONFIG_GET("USING_SYSTEM_KRB5"):
> --
> 2.11.0
>
More information about the samba-technical
mailing list