On Thu, 2013-09-05 at 10:49 +0200, Pavel Březina wrote:
> On 09/04/2013 09:12 PM, Simo Sorce wrote:
> > On Wed, 2013-09-04 at 17:06 +0200, Sumit Bose wrote:
> >> On Tue, Sep 03, 2013 at 02:07:29PM -0400, Simo Sorce wrote:
> >>> On Tue, 2013-09-03 at 15:25 +0200, Pavel Březina wrote:
> >>>> On 09/03/2013 01:27 PM, Sumit Bose wrote:
> >>>>> Hi,
> >>>>>
> >>>>> while looking at expand_ccname_template() becasue of shadowing rewind()
> >>>>> I realized that there a some issues with some of the new krb5.conf
> >>>>> templates. This patch fixes them and adds some tests to avoid similar
> >>>>> issues in the future.
> >>>>>
> >>>>> There is one change in behaviour. If the name in the %{} braces does not
> >>>>> match any of the known krb5.conf templates for UNIX the new code returns
> >>>>> an error while the old just returned something, which in most case will
> >>>>> not be the original input. Please tell me if you prefer the original
> >>>>> input in this case so that I can change the patch accordingly.
> >>>>
> >>>> I think we should expand the parameters we can, i.e. uid and euid and
> >>>> leave everything else intact (i.e. don't even check ${LIBDIR} etc.). Ad
> >>>> absurdum: for what we know, users can ran custom build that supports
> >>>> other variables...
> >>>
> >>> True, I was looking at this code yesterday again as I am hadling 2071
> >>> and I was thinking the same, we should just ignore and leave unchanged
> >>> %{xyz} patterns we do not understand, so that if libkrb5 adds something
> >>> the admin can use we do not have to explicitly support it.
> >>>
> >>>> But if you don't agree, I'll ack this patch.
> >>>
> >>> No, let's nack, and if you all do not mind I can come up with an
> >>> alternative patch that does what I describe above.
> >>
> >> Whatever you prefer, but I can do the needed changes as well. If you do
> >> them, please do not forget to add tests for the cases which are
> >> currently not covered.
> >
> > I opened bug #2076 to track this issue and attached find a different
> > approach with testes against current master.
> >
> > Simo.
>
> Nack.
>
> krb5_utils-tests.c: In function ‘test_krb5_style_expansion’:
> krb5_utils-tests.c:679:9: error: unused variable ‘ret’
>
> It looks good otherwise.
> Can we also include Sumit's tests?
Sumit's tests consider any 'unknown' expansion variable as invalid, so
they would fail.
Attached new version w/o ret and rebased on current master.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
>From a15e11c0cdfd81dcaa58f4e642033eb8ed57a740 Mon Sep 17 00:00:00 2001
From: Simo Sorce <[email protected]>
Date: Wed, 4 Sep 2013 15:02:43 -0400
Subject: [PATCH] krb5: Ingnore unknown expansion sequences
Recently support was added to use also libkrb5 style expansions that
uses a %{varname} type of template.
There are a number of templates we do not care/can't expand in sssd.
The current code misses tests and failed to properly preserve some of
the templates we do not want to handle.
Addiotionally in order to be future proof this patch treats unknown
templates as pass-through templates and defer any error checking to
libkrb5, so that sssd is consistent with how kinit would behave.
Resolves:
https://fedorahosted.org/sssd/ticket/2076
---
src/providers/krb5/krb5_utils.c | 45 ++++++++++++++---------------------------
src/tests/krb5_utils-tests.c | 30 +++++++++++++++++++++++++++
2 files changed, 45 insertions(+), 30 deletions(-)
diff --git a/src/providers/krb5/krb5_utils.c b/src/providers/krb5/krb5_utils.c
index df7892153d2c16fd3e485ed5013454233d228936..6bf1cf610dd20afc7a600e9505c4bae2ff675fcc 100644
--- a/src/providers/krb5/krb5_utils.c
+++ b/src/providers/krb5/krb5_utils.c
@@ -157,24 +157,14 @@ done:
return ret;
}
-#define S_EXP_TEMP "{TEMP}"
-#define L_EXP_TEMP (sizeof(S_EXP_TEMP) - 1)
#define S_EXP_UID "{uid}"
#define L_EXP_UID (sizeof(S_EXP_UID) - 1)
#define S_EXP_USERID "{USERID}"
#define L_EXP_USERID (sizeof(S_EXP_USERID) - 1)
#define S_EXP_EUID "{euid}"
#define L_EXP_EUID (sizeof(S_EXP_EUID) - 1)
-#define S_EXP_NULL "{null}"
-#define L_EXP_NULL (sizeof(S_EXP_NULL) - 1)
#define S_EXP_USERNAME "{username}"
#define L_EXP_USERNAME (sizeof(S_EXP_USERNAME) - 1)
-#define S_EXP_LIBDIR "{LIBDIR}"
-#define L_EXP_LIBDIR (sizeof(S_EXP_LIBDIR) - 1)
-#define S_EXP_BINDIR "{BINDIR}"
-#define L_EXP_BINDIR (sizeof(S_EXP_BINDIR) - 1)
-#define S_EXP_SBINDIR "{SBINDIR}"
-#define L_EXP_SBINDIR (sizeof(S_EXP_SBINDIR) - 1)
char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
const char *template, bool file_mode,
@@ -325,11 +315,7 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
/* Additional syntax from krb5.conf default_ccache_name */
case '{':
- if (strncmp(n, S_EXP_TEMP, L_EXP_TEMP) == 0) {
- /* let the libkrb5 library resolve this */
- result = talloc_asprintf_append(result, "%%"S_EXP_TEMP);
- n += L_EXP_TEMP - 1;
- } else if (strncmp(n , S_EXP_UID, L_EXP_UID) == 0) {
+ if (strncmp(n , S_EXP_UID, L_EXP_UID) == 0) {
action = 'U';
n += L_EXP_UID - 1;
rerun = true;
@@ -346,26 +332,25 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
n += L_EXP_EUID - 1;
rerun = true;
continue;
- } else if (strncmp(n , S_EXP_NULL, L_EXP_NULL) == 0) {
- /* skip immediately */
- n += L_EXP_NULL - 1;
} else if (strncmp(n , S_EXP_USERNAME, L_EXP_USERNAME) == 0) {
action = 'u';
n += L_EXP_USERNAME - 1;
rerun = true;
continue;
- } else if (strncmp(n , S_EXP_LIBDIR, L_EXP_LIBDIR) == 0) {
- /* skip, only the libkrb5 library can resolve this */
- result = talloc_asprintf_append(result, "%%"S_EXP_LIBDIR);
- n += L_EXP_LIBDIR - 1;
- } else if (strncmp(n , S_EXP_BINDIR, L_EXP_BINDIR) == 0) {
- /* skip, only the libkrb5 library can resolve this */
- result = talloc_asprintf_append(result, "%%"S_EXP_BINDIR);
- n += L_EXP_BINDIR - 1;
- } else if (strncmp(n , S_EXP_SBINDIR, L_EXP_SBINDIR) == 0) {
- /* skip, only the libkrb5 library can resolve this */
- result = talloc_asprintf_append(result, "%%"S_EXP_SBINDIR);
- n += L_EXP_SBINDIR - 1;
+ } else {
+ /* ignore any expansion variable we do not understand and
+ * let libkrb5 hndle it or fail */
+ name = n;
+ n = strchr(name, '}');
+ if (!n) {
+ DEBUG(SSSDBG_CRIT_FAILURE, (
+ "Invalid substitution sequence in cache "
+ "template. Missing closing '}' in [%s].\n",
+ template));
+ goto done;
+ }
+ result = talloc_asprintf_append(result, "%s%%%.*s", p,
+ (int)(n - name + 1), name);
}
break;
default:
diff --git a/src/tests/krb5_utils-tests.c b/src/tests/krb5_utils-tests.c
index 174d463b2927e3a8c301fd4423811ffd15e6c079..4715774ff189700bc1cbeb6c90f4438445a594b0 100644
--- a/src/tests/krb5_utils-tests.c
+++ b/src/tests/krb5_utils-tests.c
@@ -673,6 +673,35 @@ START_TEST(test_no_substitution)
}
END_TEST
+START_TEST(test_krb5_style_expansion)
+{
+ char *result;
+ bool private_path = false;
+ const char *file_template;
+ const char *expected;
+
+ file_template = BASE"/%{uid}/%{USERID}/%{euid}/%{username}";
+ expected = BASE"/"UID"/"UID"/"UID"/"USERNAME;
+ result = expand_ccname_template(tmp_ctx, kr, file_template, true,
+ true, &private_path);
+
+ fail_unless(result != NULL, "Cannot expand template [%s].", file_template);
+ fail_unless(strcmp(result, expected) == 0,
+ "Expansion failed, result [%s], expected [%s].",
+ result, expected);
+
+ file_template = BASE"/%{unknown}";
+ expected = BASE"/%{unknown}";
+ result = expand_ccname_template(tmp_ctx, kr, file_template, true,
+ false, &private_path);
+
+ fail_unless(result != NULL, "Cannot expand template [%s].", file_template);
+ fail_unless(strcmp(result, expected) == 0,
+ "Expansion failed, result [%s], expected [%s].",
+ result, expected);
+}
+END_TEST
+
START_TEST(test_compare_principal_realm)
{
int ret;
@@ -738,6 +767,7 @@ Suite *krb5_utils_suite (void)
tcase_add_test (tc_ccname_template, test_pid);
tcase_add_test (tc_ccname_template, test_percent);
tcase_add_test (tc_ccname_template, test_multiple_substitutions);
+ tcase_add_test (tc_ccname_template, test_krb5_style_expansion);
suite_add_tcase (s, tc_ccname_template);
TCase *tc_create_dir = tcase_create("create_dir");
--
1.8.3.1
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
