On (29/04/15 12:21), Pavel Reichl wrote:
Hello,
please see attached patches, thanks!
From e824d4aa1051bf405243deb06cef1e05ed3ffd25 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 27 Feb 2015 11:37:50 -0500 Subject: [PATCH 1/3] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
Resolves: https://fedorahosted.org/sssd/ticket/2509
src/config/SSSDConfig/__init__.py.in | 1 + src/config/SSSDConfigTest.py | 9 ++- src/config/etc/sssd.api.d/sssd-ad.conf | 1 + src/config/etc/sssd.api.d/sssd-ipa.conf | 1 + src/config/etc/sssd.api.d/sssd-krb5.conf | 1 + src/man/sssd-krb5.5.xml | 26 ++++++++ src/providers/ad/ad_opts.h | 1 + src/providers/ipa/ipa_opts.h | 1 + src/providers/krb5/krb5_access.c | 4 +- src/providers/krb5/krb5_auth.c | 40 +++++++++++- src/providers/krb5/krb5_auth.h | 2 + src/providers/krb5/krb5_common.h | 9 +++ src/providers/krb5/krb5_init_shared.c | 101 +++++++++++++++++++++++++++++++ src/providers/krb5/krb5_opts.h | 1 + src/util/util.c | 56 +++++++++++++++++ src/util/util.h | 6 ++ 16 files changed, 254 insertions(+), 6 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 613c559bb2002686c7833642d0946e46e5a9b5d6..a3da6c51d2b92a529b03632a26b6e07569b5da77 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -30,6 +30,62 @@ #include "util/util.h" #include "util/sss_utf8.h"
+errno_t split_str(TALLOC_CTX *mem_ctx,
const char delimiter,char *str,char **_first,char **_second)+{
Why do we need new function split_str? We already have function split_on_separator.
The function split_on_separator is tested unlike yours function.
I'm sorry but utility function without unit test(s) is automatic NACK. You should even have thought about sending such patch. :-)
- char *sep;
- errno_t ret;
- char *first;
- char *second;
- TALLOC_CTX *tmp_ctx;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;goto done;- }
- sep = strchr(str, delimiter);
- if (sep == NULL) {
ret = ENOENT;goto done;- } else {
if (sep == str) {first = NULL;} else {first = talloc_strndup(tmp_ctx, str, sep - str);if (first == NULL) {ret = ENOMEM;goto done;}}if (*(sep + 1) == '\0') {second = NULL;} else {second = talloc_strdup(tmp_ctx, sep+1);if (second == NULL) {ret = ENOMEM;goto done;}}- }
- ret = EOK;
+done:
- if (ret == EOK) {
*_first = talloc_steal(mem_ctx, first);*_second = talloc_steal(mem_ctx, second);- }
- talloc_free(tmp_ctx);
- return ret;
+}
int split_on_separator(TALLOC_CTX *mem_ctx, const char *str, const char sep, bool trim, bool skip_empty, char ***_list, int *size) diff --git a/src/util/util.h b/src/util/util.h index 3c5ce295ec4ad2ad3c3a7ae0e7482c232db79746..479e24cabe947419cf9d63e803b1b662359ad38b 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -408,6 +408,12 @@ const char * const * get_known_services(void);
errno_t sss_user_by_name_or_uid(const char *input, uid_t *_uid, gid_t *_gid);
+errno_t split_str(TALLOC_CTX *mem_ctx,
const char delimiter,char *str,char **_first,char **_second);int split_on_separator(TALLOC_CTX *mem_ctx, const char *str, const char sep, bool trim, bool skip_empty, char ***_list, int *size); -- 2.1.0
From 50839ec51277cb9aa45c21b71bfb9dc5e8b0b553 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 29 Apr 2015 06:03:04 -0400 Subject: [PATCH 2/3] krb5: remove field run_as_user
run_as_user is set set but never read.
LGTM
On 04/29/2015 07:00 PM, Lukas Slebodnik wrote:
On (29/04/15 12:21), Pavel Reichl wrote:
Hello,
please see attached patches, thanks! From e824d4aa1051bf405243deb06cef1e05ed3ffd25 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 27 Feb 2015 11:37:50 -0500 Subject: [PATCH 1/3] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
Resolves: https://fedorahosted.org/sssd/ticket/2509
src/config/SSSDConfig/__init__.py.in | 1 + src/config/SSSDConfigTest.py | 9 ++- src/config/etc/sssd.api.d/sssd-ad.conf | 1 + src/config/etc/sssd.api.d/sssd-ipa.conf | 1 + src/config/etc/sssd.api.d/sssd-krb5.conf | 1 + src/man/sssd-krb5.5.xml | 26 ++++++++ src/providers/ad/ad_opts.h | 1 + src/providers/ipa/ipa_opts.h | 1 + src/providers/krb5/krb5_access.c | 4 +- src/providers/krb5/krb5_auth.c | 40 +++++++++++- src/providers/krb5/krb5_auth.h | 2 + src/providers/krb5/krb5_common.h | 9 +++ src/providers/krb5/krb5_init_shared.c | 101 +++++++++++++++++++++++++++++++ src/providers/krb5/krb5_opts.h | 1 + src/util/util.c | 56 +++++++++++++++++ src/util/util.h | 6 ++ 16 files changed, 254 insertions(+), 6 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 613c559bb2002686c7833642d0946e46e5a9b5d6..a3da6c51d2b92a529b03632a26b6e07569b5da77 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -30,6 +30,62 @@ #include "util/util.h" #include "util/sss_utf8.h"
+errno_t split_str(TALLOC_CTX *mem_ctx,
const char delimiter,char *str,char **_first,char **_second)+{
Why do we need new function split_str? We already have function split_on_separator.
split_on_separator() does super set of split_str(), code is IMO better with split_str() which is sufficient here. In reviewed patch I used both. Are you sure you don't see the difference?
The function split_on_separator is tested unlike yours function.
I'm sorry but utility function without unit test(s) is automatic NACK.
Please could you point me to the source of such rules? I suppose other developers would appreciate such list too as according to https://jhrozek.fedorapeople.org/sssd-coverage/html/util/util.c.gcov.html there are already functions without unit tests...how that could happen?
You should even have thought about sending such patch. :-)
I agree that unit tests are cool and I would hate to lower code coverage of util.c so please see updated patch set.
- char *sep;
- errno_t ret;
- char *first;
- char *second;
- TALLOC_CTX *tmp_ctx;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;goto done;- }
- sep = strchr(str, delimiter);
- if (sep == NULL) {
ret = ENOENT;goto done;- } else {
if (sep == str) {first = NULL;} else {first = talloc_strndup(tmp_ctx, str, sep - str);if (first == NULL) {ret = ENOMEM;goto done;}}if (*(sep + 1) == '\0') {second = NULL;} else {second = talloc_strdup(tmp_ctx, sep+1);if (second == NULL) {ret = ENOMEM;goto done;}}- }
- ret = EOK;
+done:
- if (ret == EOK) {
*_first = talloc_steal(mem_ctx, first);*_second = talloc_steal(mem_ctx, second);- }
- talloc_free(tmp_ctx);
- return ret;
+}
int split_on_separator(TALLOC_CTX *mem_ctx, const char *str, const char sep, bool trim, bool skip_empty, char ***_list, int *size) diff --git a/src/util/util.h b/src/util/util.h index 3c5ce295ec4ad2ad3c3a7ae0e7482c232db79746..479e24cabe947419cf9d63e803b1b662359ad38b 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -408,6 +408,12 @@ const char * const * get_known_services(void);
errno_t sss_user_by_name_or_uid(const char *input, uid_t *_uid, gid_t *_gid);
+errno_t split_str(TALLOC_CTX *mem_ctx,
const char delimiter,char *str,char **_first,char **_second);int split_on_separator(TALLOC_CTX *mem_ctx, const char *str, const char sep, bool trim, bool skip_empty, char ***_list, int *size); -- 2.1.0
From 50839ec51277cb9aa45c21b71bfb9dc5e8b0b553 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 29 Apr 2015 06:03:04 -0400 Subject: [PATCH 2/3] krb5: remove field run_as_user
run_as_user is set set but never read.
LGTM _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (30/04/15 13:45), Pavel Reichl wrote:
On 04/29/2015 07:00 PM, Lukas Slebodnik wrote:
On (29/04/15 12:21), Pavel Reichl wrote:
Hello,
please see attached patches, thanks! From e824d4aa1051bf405243deb06cef1e05ed3ffd25 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 27 Feb 2015 11:37:50 -0500 Subject: [PATCH 1/3] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
Resolves: https://fedorahosted.org/sssd/ticket/2509
src/config/SSSDConfig/__init__.py.in | 1 + src/config/SSSDConfigTest.py | 9 ++- src/config/etc/sssd.api.d/sssd-ad.conf | 1 + src/config/etc/sssd.api.d/sssd-ipa.conf | 1 + src/config/etc/sssd.api.d/sssd-krb5.conf | 1 + src/man/sssd-krb5.5.xml | 26 ++++++++ src/providers/ad/ad_opts.h | 1 + src/providers/ipa/ipa_opts.h | 1 + src/providers/krb5/krb5_access.c | 4 +- src/providers/krb5/krb5_auth.c | 40 +++++++++++- src/providers/krb5/krb5_auth.h | 2 + src/providers/krb5/krb5_common.h | 9 +++ src/providers/krb5/krb5_init_shared.c | 101 +++++++++++++++++++++++++++++++ src/providers/krb5/krb5_opts.h | 1 + src/util/util.c | 56 +++++++++++++++++ src/util/util.h | 6 ++ 16 files changed, 254 insertions(+), 6 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 613c559bb2002686c7833642d0946e46e5a9b5d6..a3da6c51d2b92a529b03632a26b6e07569b5da77 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -30,6 +30,62 @@ #include "util/util.h" #include "util/sss_utf8.h"
+errno_t split_str(TALLOC_CTX *mem_ctx,
const char delimiter,char *str,char **_first,char **_second)+{
Why do we need new function split_str? We already have function split_on_separator.
split_on_separator() does super set of split_str(),
Agree
code is IMO better with split_str() which is sufficient here.
IMHO it is just premature optimization.
Did you do performance testing? How much faster is function split_str?
I'm sorry I cannot see a reason for two function with the same functionality. It make sense to "optimize" only if you have performance results and function is called very often.
So please reuse existing function unless all previous statements are true.
LS
On 04/30/2015 02:02 PM, Lukas Slebodnik wrote:
On (30/04/15 13:45), Pavel Reichl wrote:
On 04/29/2015 07:00 PM, Lukas Slebodnik wrote:
On (29/04/15 12:21), Pavel Reichl wrote:
Hello,
please see attached patches, thanks! From e824d4aa1051bf405243deb06cef1e05ed3ffd25 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 27 Feb 2015 11:37:50 -0500 Subject: [PATCH 1/3] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
Resolves: https://fedorahosted.org/sssd/ticket/2509
src/config/SSSDConfig/__init__.py.in | 1 + src/config/SSSDConfigTest.py | 9 ++- src/config/etc/sssd.api.d/sssd-ad.conf | 1 + src/config/etc/sssd.api.d/sssd-ipa.conf | 1 + src/config/etc/sssd.api.d/sssd-krb5.conf | 1 + src/man/sssd-krb5.5.xml | 26 ++++++++ src/providers/ad/ad_opts.h | 1 + src/providers/ipa/ipa_opts.h | 1 + src/providers/krb5/krb5_access.c | 4 +- src/providers/krb5/krb5_auth.c | 40 +++++++++++- src/providers/krb5/krb5_auth.h | 2 + src/providers/krb5/krb5_common.h | 9 +++ src/providers/krb5/krb5_init_shared.c | 101 +++++++++++++++++++++++++++++++ src/providers/krb5/krb5_opts.h | 1 + src/util/util.c | 56 +++++++++++++++++ src/util/util.h | 6 ++ 16 files changed, 254 insertions(+), 6 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 613c559bb2002686c7833642d0946e46e5a9b5d6..a3da6c51d2b92a529b03632a26b6e07569b5da77 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -30,6 +30,62 @@ #include "util/util.h" #include "util/sss_utf8.h"
+errno_t split_str(TALLOC_CTX *mem_ctx,
const char delimiter,char *str,char **_first,char **_second)+{
Why do we need new function split_str? We already have function split_on_separator.
split_on_separator() does super set of split_str(),
Agree
code is IMO better with split_str() which is sufficient here.
IMHO it is just premature optimization.
It's not about speed or memory usage, its about code readability. Interface of split_str() is simpler. Only point to discussion I see is that I could probably use split_on_separator() to implement split_str().
Functionality of split_str() in actually not new. It's used in many places ad hoc as part of other functions.
Did you do performance testing? How much faster is function split_str?
I'm sorry I cannot see a reason for two function with the same functionality.
not same as you agreed
It make sense to "optimize" only if you have performance results and function is called very often.
So please reuse existing function unless all previous statements are true.
No.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Apr 30, 2015 at 02:13:04PM +0200, Pavel Reichl wrote:
Functionality of split_str() in actually not new. It's used in many places ad hoc as part of other functions.
(I haven't seen the code, just following the discussion)
Would it make sense to consolidate the code to use split_str(), then?
On 04/30/2015 03:33 PM, Jakub Hrozek wrote:
On Thu, Apr 30, 2015 at 02:13:04PM +0200, Pavel Reichl wrote:
Functionality of split_str() in actually not new. It's used in many places ad hoc as part of other functions.
(I haven't seen the code, just following the discussion)
Would it make sense to consolidate the code to use split_str(), then?
Well, it would be nice, but I'm afraid it's not worth the effort. For example 'split_extra_attr()' uses quite similar code but not exactly the same, it could be changed, but I don't think it's worth the risk of introducing bug.
In the posted patch I needed similar functionality so I could introduce static helper function for this purpose or (as I did) make decision that this is generally useful functionality that could be reused and move it to utils.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Apr 30, 2015 at 01:45:01PM +0200, Pavel Reichl wrote: Hi,
it seems this patch review stalled. I'll try to restart it..
From 2c7239f5466acb4a0989c4843b0b13e85f1d40b3 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:40:43 -0400 Subject: [PATCH 1/3] utils: new function split_str
I'm with Lukas here. It seems to me that the split function could be defined in the Kerberos tree and could use split_on_separator() as the underlying implementation. I don't see that this change requires a separate util function.
Feel free to add a function for krb5_utils instead that calls into split_on_separator()..
Or were you concerned about the delimeter being present in the Kerberos principal?
From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH 2/3] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
Resolves: https://fedorahosted.org/sssd/ticket/2509
[...]
</listitem> </varlistentry>
<varlistentry><term>krb5_map_user (string)</term><listitem><para>The list of mappings is given as a comma-separatedlist of pairs `username:principal` where `username`
It's not username:principal, but username:primary. I agree though that the primary term is not well known, so maybe could be say something like "username:primary, where primary is typically the user part of the principal" ?
is a UNIX user name and `principal` is a kerberosprincipal name. This mapping is used when user isauthenticating using `auth_provider = krb5`.</para><para>example:krb5_map_user = joe:juser,vince:rraines</para><para>`joe` and `vince` are UNIX user names and `juser`and `rraines` are kerberos principal names.
Please add krb5_realm to the example and illustrate the full principals here.
</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para></refsect1>
diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h index 0b7255a828e95785d31437968a37bc20fbf62aef..b03c74612d3141170dac84ab805529184fec5a49 100644 --- a/src/providers/ad/ad_opts.h +++ b/src/providers/ad/ad_opts.h @@ -168,6 +168,7 @@ struct dp_option ad_def_krb5_opts[] = { { "krb5_canonicalize", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE }, { "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
- { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING }, DP_OPTION_TERMINATOR
};
diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h index f2f164bc3cc6b6e13c30dbc6f5b37a03b4c5e289..5f215d7fbe2926eb020cbe58226727dacf0f1ba2 100644 --- a/src/providers/ipa/ipa_opts.h +++ b/src/providers/ipa/ipa_opts.h @@ -310,6 +310,7 @@ struct dp_option ipa_def_krb5_opts[] = { { "krb5_canonicalize", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE }, { "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
- { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING }, DP_OPTION_TERMINATOR
};
diff --git a/src/providers/krb5/krb5_access.c b/src/providers/krb5/krb5_access.c index 7fda2a37922a537f7fe53d629c4e0cb4df1bd4da..72b93180f9cdeb53ad78c821ed73281d38cda920 100644 --- a/src/providers/krb5/krb5_access.c +++ b/src/providers/krb5/krb5_access.c @@ -106,8 +106,8 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx, break; case 1: ret = find_or_guess_upn(state, res->msgs[0], krb5_ctx,
be_ctx->domain, pd->user, pd->domain,&state->kr->upn);
be_ctx->domain, state->kr->mapped_name,pd->domain, &state->kr->upn); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "find_or_guess_upn failed.\n"); goto done;diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c index 25caf7b788a3f373f47e9d8aad38a2ea6fc12621..f665fab786b27bf1e85d238cdb2567915d0ae0b8 100644 --- a/src/providers/krb5/krb5_auth.c +++ b/src/providers/krb5/krb5_auth.c @@ -175,10 +175,33 @@ static int krb5_cleanup(void *ptr) return EOK; }
+errno_t get_krb_princ(struct map_id_name_to_krb_princ *name_to_princ, int size,
char *id_prov_name, const char **_krb_princ)
Please make the function static and use size_t for the size. Or even cleaner, allocate one extra map_id_name_to_krb5_princ and set it to NULL:NULL as a sentinel, then we don't need size at all.
+{
- errno_t ret;
- int i;
- for (i=0; i < size; i++) {
if (strcmp(name_to_princ[i].id_name, id_prov_name) == 0) {
What about case-sensitiveness? What about using sss_utf8_case_eq() here instead?
*_krb_princ = name_to_princ[i].krb_princ;ret = EOK;goto done;}- }
- /* Handle also the case of size being zero */
- ret = ENOENT;
+done:
- return ret;
+}
errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd, struct krb5_ctx *krb5_ctx, struct krb5child_req **krb5_req) { struct krb5child_req *kr = NULL;
- const char *mapped_name;
This variable name is fine..but the variable in "kr" should be named differently. I think using just 'user' would be fine, same as what we use in "pd".
errno_t ret;
kr = talloc_zero(mem_ctx, struct krb5child_req); if (kr == NULL) {
@@ -192,6 +215,20 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd, kr->pd = pd; kr->krb5_ctx = krb5_ctx;
- ret = get_krb_princ(krb5_ctx->name_to_princ, krb5_ctx->name_to_princ_size,
pd->user, &mapped_name);- if (ret == EOK) {
DEBUG(SSSDBG_TRACE_FUNC, "Setting mapped name to: %s\n", mapped_name);kr->mapped_name = mapped_name;- } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_FUNC, "No mapping for: %s\n", pd->user);
I would decrease the DEBUG_LEVEL even lower, it would be quite a corner case to actually have the username mapped.
kr->mapped_name = pd->user;- } else {
DEBUG(SSSDBG_MINOR_FAILURE, "get_krb_princ failed - %s:[%d]\n",sss_strerror(ret), ret);kr->mapped_name = pd->user;
I think it's better to just fail here instead of allowing to run with a wrong principal name.
}
*krb5_req = kr;
return EOK;
@@ -350,6 +387,7 @@ struct krb5_auth_state { static void krb5_auth_resolve_done(struct tevent_req *subreq); static void krb5_auth_done(struct tevent_req *subreq);
Extra blank line.
struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct be_ctx *be_ctx, @@ -510,7 +548,7 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
case 1: ret = find_or_guess_upn(state, res->msgs[0], krb5_ctx,
be_ctx->domain, pd->user, pd->domain,
be_ctx->domain, kr->mapped_name, pd->domain, &kr->upn); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "find_or_guess_upn failed.\n");diff --git a/src/providers/krb5/krb5_auth.h b/src/providers/krb5/krb5_auth.h index 00cb658c418ade2e6a1d9b5362a40f97e0dc6c6b..354c74f5d806c9bd9355a9abdc2976f1fcf8b1aa 100644 --- a/src/providers/krb5/krb5_auth.h +++ b/src/providers/krb5/krb5_auth.h @@ -56,6 +56,8 @@ struct krb5child_req { bool run_as_user; bool upn_from_different_realm; bool send_pac;
- const char *mapped_name;
As said elsewhere, the name is not really mapped in most cases.
};
errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd, diff --git a/src/providers/krb5/krb5_common.h b/src/providers/krb5/krb5_common.h index 81e64688a6d93a4b3ecf41d75d1bf6166b4619ce..99ee3b4930fb275efebc09e510cd0459d11bf19e 100644 --- a/src/providers/krb5/krb5_common.h +++ b/src/providers/krb5/krb5_common.h @@ -67,6 +67,7 @@ enum krb5_opts { KRB5_CANONICALIZE, KRB5_USE_ENTERPRISE_PRINCIPAL, KRB5_USE_KDCINFO,
KRB5_MAP_USER,
KRB5_OPTS
}; @@ -89,6 +90,11 @@ enum krb5_config_type { K5C_IPA_SERVER };
+struct map_id_name_to_krb_princ {
- char *id_name;
- char* krb_princ;
Could we make these const?
+};
struct krb5_ctx { /* opts taken from kinit */ /* in seconds */ @@ -128,6 +134,9 @@ struct krb5_ctx { hash_table_t *wait_queue_hash;
enum krb5_config_type config_type;
- struct map_id_name_to_krb_princ *name_to_princ;
- int name_to_princ_size;
};
struct remove_info_files_ctx { diff --git a/src/providers/krb5/krb5_init_shared.c b/src/providers/krb5/krb5_init_shared.c index 3b4bf096ef49d0f2d369fda20462b7fd56d61127..e0c010fefe79ffa594a63a88c43279aecd4559c2 100644 --- a/src/providers/krb5/krb5_init_shared.c +++ b/src/providers/krb5/krb5_init_shared.c @@ -26,6 +26,98 @@ #include "providers/krb5/krb5_auth.h" #include "providers/krb5/krb5_init_shared.h"
+static errno_t +fill_name_to_princ_map(TALLOC_CTX *mem_ctx, char **map,
struct map_id_name_to_krb_princ *name_to_princ,int size)+{
- int i;
- errno_t ret;
- for (i=0; i < size; i++) {
ret = split_str(mem_ctx, ':', map[i],&name_to_princ[i].id_name,&name_to_princ[i].krb_princ);
As said earlier, I'm not sure if split_str needs to be in the generic util code.
if (ret != EOK) {DEBUG(SSSDBG_MINOR_FAILURE,"split_str failed - %s:[%d]\n", sss_strerror(ret), ret);goto done;} else if (name_to_princ[i].id_name == NULL ||name_to_princ[i].krb_princ == NULL) {DEBUG(SSSDBG_MINOR_FAILURE, "split_str failed - %s:%s\n",name_to_princ[i].id_name, name_to_princ[i].krb_princ );ret = EINVAL;goto done;}- }
- ret = EOK;
+done:
- return ret;
+}
+errno_t parse_krb5_map_user(TALLOC_CTX *mem_ctx, struct dp_option *opts,
struct map_id_name_to_krb_princ **_name_to_princ,int *_size)+{
- int size;
- char **map;
- errno_t ret;
- TALLOC_CTX *tmp_ctx;
- const char *krb5_map_user;
- struct map_id_name_to_krb_princ *name_to_princ;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;goto done;- }
- krb5_map_user = dp_opt_get_cstring(opts, KRB5_MAP_USER);
- if (krb5_map_user == NULL) {
DEBUG(SSSDBG_FUNC_DATA, "Option krb5_map_user is empty!\n");size = 0;
Here we could set the map to NULL and then shortcut in get_krb_princ() on receiving NULL.
ret = EOK;goto done;- }
- ret = split_on_separator(tmp_ctx, krb5_map_user, ',', true, true,
&map, &size);- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "Failed to parse krb5_map_user!\n");goto done;- }
- name_to_princ = talloc_array(tmp_ctx,
struct map_id_name_to_krb_princ, size);- if (name_to_princ == NULL) {
ret = ENOMEM;goto done;- }
- ret = fill_name_to_princ_map(name_to_princ, map, name_to_princ, size);
- if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE, "fill_name_to_princ_map failed: %s:[%d]\n",sss_strerror(ret), ret);goto done;- }
- ret = EOK;
+done:
- if (ret == EOK) {
if (size == 0) {*_name_to_princ = NULL;} else {*_name_to_princ = talloc_steal(mem_ctx, name_to_princ);}*_size = size;- }
- talloc_free(tmp_ctx);
- return ret;
+}
errno_t krb5_child_init(struct krb5_ctx *krb5_auth_ctx, struct be_ctx *bectx) { @@ -90,6 +182,15 @@ errno_t krb5_child_init(struct krb5_ctx *krb5_auth_ctx, goto done; }
- ret = parse_krb5_map_user(krb5_auth_ctx, krb5_auth_ctx->opts,
&krb5_auth_ctx->name_to_princ,&krb5_auth_ctx->name_to_princ_size);- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "parse_krb5_map_user failed: %s:[%d]\n",sss_strerror(ret), ret);goto done;- }
- ret = EOK;
done: diff --git a/src/providers/krb5/krb5_opts.h b/src/providers/krb5/krb5_opts.h index db62cc3b23d8a5eadd3df97a102fb84128b740da..50d701b8ba431719dbf67799230ec345cdee30a9 100644 --- a/src/providers/krb5/krb5_opts.h +++ b/src/providers/krb5/krb5_opts.h @@ -45,6 +45,7 @@ struct dp_option default_krb5_opts[] = { { "krb5_canonicalize", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
- { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING }, DP_OPTION_TERMINATOR
};
-- 2.1.0
From ffe3d9f0e0e697f565fb28861df41fdd9c7bbbe2 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 29 Apr 2015 06:03:04 -0400 Subject: [PATCH 3/3] krb5: remove field run_as_user
Oops, this is a leftover from the rootless sssd changes. ACK.
On 05/11/2015 01:56 PM, Jakub Hrozek wrote:
On Thu, Apr 30, 2015 at 01:45:01PM +0200, Pavel Reichl wrote: Hi,
it seems this patch review stalled. I'll try to restart it..
From 2c7239f5466acb4a0989c4843b0b13e85f1d40b3 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:40:43 -0400 Subject: [PATCH 1/3] utils: new function split_str
I'm with Lukas here. It seems to me that the split function could be defined in the Kerberos tree and could use split_on_separator() as the underlying implementation. I don't see that this change requires a separate util function.
Feel free to add a function for krb5_utils instead that calls into split_on_separator()..
Or were you concerned about the delimeter being present in the Kerberos principal?
From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH 2/3] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
Resolves: https://fedorahosted.org/sssd/ticket/2509
[...]
</listitem> </varlistentry>
<varlistentry><term>krb5_map_user (string)</term><listitem><para>The list of mappings is given as a comma-separatedlist of pairs `username:principal` where `username`It's not username:principal, but username:primary. I agree though that the primary term is not well known, so maybe could be say something like "username:primary, where primary is typically the user part of the principal" ?
is a UNIX user name and `principal` is a kerberosprincipal name. This mapping is used when user isauthenticating using `auth_provider = krb5`.</para><para>example:krb5_map_user = joe:juser,vince:rraines</para><para>`joe` and `vince` are UNIX user names and `juser`and `rraines` are kerberos principal names.Please add krb5_realm to the example and illustrate the full principals here.
</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para> </refsect1>diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h index 0b7255a828e95785d31437968a37bc20fbf62aef..b03c74612d3141170dac84ab805529184fec5a49 100644 --- a/src/providers/ad/ad_opts.h +++ b/src/providers/ad/ad_opts.h @@ -168,6 +168,7 @@ struct dp_option ad_def_krb5_opts[] = { { "krb5_canonicalize", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE }, { "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
- { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING }, DP_OPTION_TERMINATOR };
diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h index f2f164bc3cc6b6e13c30dbc6f5b37a03b4c5e289..5f215d7fbe2926eb020cbe58226727dacf0f1ba2 100644 --- a/src/providers/ipa/ipa_opts.h +++ b/src/providers/ipa/ipa_opts.h @@ -310,6 +310,7 @@ struct dp_option ipa_def_krb5_opts[] = { { "krb5_canonicalize", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE }, { "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
- { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING }, DP_OPTION_TERMINATOR };
diff --git a/src/providers/krb5/krb5_access.c b/src/providers/krb5/krb5_access.c index 7fda2a37922a537f7fe53d629c4e0cb4df1bd4da..72b93180f9cdeb53ad78c821ed73281d38cda920 100644 --- a/src/providers/krb5/krb5_access.c +++ b/src/providers/krb5/krb5_access.c @@ -106,8 +106,8 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx, break; case 1: ret = find_or_guess_upn(state, res->msgs[0], krb5_ctx,
be_ctx->domain, pd->user, pd->domain,&state->kr->upn);
be_ctx->domain, state->kr->mapped_name,pd->domain, &state->kr->upn); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "find_or_guess_upn failed.\n"); goto done;diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c index 25caf7b788a3f373f47e9d8aad38a2ea6fc12621..f665fab786b27bf1e85d238cdb2567915d0ae0b8 100644 --- a/src/providers/krb5/krb5_auth.c +++ b/src/providers/krb5/krb5_auth.c @@ -175,10 +175,33 @@ static int krb5_cleanup(void *ptr) return EOK; }
+errno_t get_krb_princ(struct map_id_name_to_krb_princ *name_to_princ, int size,
char *id_prov_name, const char **_krb_princ)Please make the function static and use size_t for the size. Or even cleaner, allocate one extra map_id_name_to_krb5_princ and set it to NULL:NULL as a sentinel, then we don't need size at all.
+{
- errno_t ret;
- int i;
- for (i=0; i < size; i++) {
if (strcmp(name_to_princ[i].id_name, id_prov_name) == 0) {What about case-sensitiveness? What about using sss_utf8_case_eq() here instead?
*_krb_princ = name_to_princ[i].krb_princ;ret = EOK;goto done;}- }
- /* Handle also the case of size being zero */
- ret = ENOENT;
+done:
- return ret;
+}
- errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd, struct krb5_ctx *krb5_ctx, struct krb5child_req **krb5_req) { struct krb5child_req *kr = NULL;
- const char *mapped_name;
This variable name is fine..but the variable in "kr" should be named differently. I think using just 'user' would be fine, same as what we use in "pd".
errno_t ret;
kr = talloc_zero(mem_ctx, struct krb5child_req); if (kr == NULL) {
@@ -192,6 +215,20 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd, kr->pd = pd; kr->krb5_ctx = krb5_ctx;
- ret = get_krb_princ(krb5_ctx->name_to_princ, krb5_ctx->name_to_princ_size,
pd->user, &mapped_name);- if (ret == EOK) {
DEBUG(SSSDBG_TRACE_FUNC, "Setting mapped name to: %s\n", mapped_name);kr->mapped_name = mapped_name;- } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_FUNC, "No mapping for: %s\n", pd->user);I would decrease the DEBUG_LEVEL even lower, it would be quite a corner case to actually have the username mapped.
kr->mapped_name = pd->user;- } else {
DEBUG(SSSDBG_MINOR_FAILURE, "get_krb_princ failed - %s:[%d]\n",sss_strerror(ret), ret);kr->mapped_name = pd->user;I think it's better to just fail here instead of allowing to run with a wrong principal name.
}
*krb5_req = kr; return EOK;@@ -350,6 +387,7 @@ struct krb5_auth_state { static void krb5_auth_resolve_done(struct tevent_req *subreq); static void krb5_auth_done(struct tevent_req *subreq);
Extra blank line.
struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct be_ctx *be_ctx, @@ -510,7 +548,7 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
case 1: ret = find_or_guess_upn(state, res->msgs[0], krb5_ctx,
be_ctx->domain, pd->user, pd->domain,
be_ctx->domain, kr->mapped_name, pd->domain, &kr->upn); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "find_or_guess_upn failed.\n");diff --git a/src/providers/krb5/krb5_auth.h b/src/providers/krb5/krb5_auth.h index 00cb658c418ade2e6a1d9b5362a40f97e0dc6c6b..354c74f5d806c9bd9355a9abdc2976f1fcf8b1aa 100644 --- a/src/providers/krb5/krb5_auth.h +++ b/src/providers/krb5/krb5_auth.h @@ -56,6 +56,8 @@ struct krb5child_req { bool run_as_user; bool upn_from_different_realm; bool send_pac;
- const char *mapped_name;
As said elsewhere, the name is not really mapped in most cases.
};
errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd, diff --git a/src/providers/krb5/krb5_common.h b/src/providers/krb5/krb5_common.h index 81e64688a6d93a4b3ecf41d75d1bf6166b4619ce..99ee3b4930fb275efebc09e510cd0459d11bf19e 100644 --- a/src/providers/krb5/krb5_common.h +++ b/src/providers/krb5/krb5_common.h @@ -67,6 +67,7 @@ enum krb5_opts { KRB5_CANONICALIZE, KRB5_USE_ENTERPRISE_PRINCIPAL, KRB5_USE_KDCINFO,
KRB5_MAP_USER,
KRB5_OPTS };
@@ -89,6 +90,11 @@ enum krb5_config_type { K5C_IPA_SERVER };
+struct map_id_name_to_krb_princ {
- char *id_name;
- char* krb_princ;
Could we make these const?
+};
- struct krb5_ctx { /* opts taken from kinit */ /* in seconds */
@@ -128,6 +134,9 @@ struct krb5_ctx { hash_table_t *wait_queue_hash;
enum krb5_config_type config_type;
struct map_id_name_to_krb_princ *name_to_princ;
int name_to_princ_size; };
struct remove_info_files_ctx {
diff --git a/src/providers/krb5/krb5_init_shared.c b/src/providers/krb5/krb5_init_shared.c index 3b4bf096ef49d0f2d369fda20462b7fd56d61127..e0c010fefe79ffa594a63a88c43279aecd4559c2 100644 --- a/src/providers/krb5/krb5_init_shared.c +++ b/src/providers/krb5/krb5_init_shared.c @@ -26,6 +26,98 @@ #include "providers/krb5/krb5_auth.h" #include "providers/krb5/krb5_init_shared.h"
+static errno_t +fill_name_to_princ_map(TALLOC_CTX *mem_ctx, char **map,
struct map_id_name_to_krb_princ *name_to_princ,int size)+{
- int i;
- errno_t ret;
- for (i=0; i < size; i++) {
ret = split_str(mem_ctx, ':', map[i],&name_to_princ[i].id_name,&name_to_princ[i].krb_princ);As said earlier, I'm not sure if split_str needs to be in the generic util code.
if (ret != EOK) {DEBUG(SSSDBG_MINOR_FAILURE,"split_str failed - %s:[%d]\n", sss_strerror(ret), ret);goto done;} else if (name_to_princ[i].id_name == NULL ||name_to_princ[i].krb_princ == NULL) {DEBUG(SSSDBG_MINOR_FAILURE, "split_str failed - %s:%s\n",name_to_princ[i].id_name, name_to_princ[i].krb_princ );ret = EINVAL;goto done;}- }
- ret = EOK;
+done:
- return ret;
+}
+errno_t parse_krb5_map_user(TALLOC_CTX *mem_ctx, struct dp_option *opts,
struct map_id_name_to_krb_princ **_name_to_princ,int *_size)+{
- int size;
- char **map;
- errno_t ret;
- TALLOC_CTX *tmp_ctx;
- const char *krb5_map_user;
- struct map_id_name_to_krb_princ *name_to_princ;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;goto done;- }
- krb5_map_user = dp_opt_get_cstring(opts, KRB5_MAP_USER);
- if (krb5_map_user == NULL) {
DEBUG(SSSDBG_FUNC_DATA, "Option krb5_map_user is empty!\n");size = 0;Here we could set the map to NULL and then shortcut in get_krb_princ() on receiving NULL.
ret = EOK;goto done;- }
- ret = split_on_separator(tmp_ctx, krb5_map_user, ',', true, true,
&map, &size);- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "Failed to parse krb5_map_user!\n");goto done;- }
- name_to_princ = talloc_array(tmp_ctx,
struct map_id_name_to_krb_princ, size);- if (name_to_princ == NULL) {
ret = ENOMEM;goto done;- }
- ret = fill_name_to_princ_map(name_to_princ, map, name_to_princ, size);
- if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE, "fill_name_to_princ_map failed: %s:[%d]\n",sss_strerror(ret), ret);goto done;- }
- ret = EOK;
+done:
- if (ret == EOK) {
if (size == 0) {*_name_to_princ = NULL;} else {*_name_to_princ = talloc_steal(mem_ctx, name_to_princ);}*_size = size;- }
- talloc_free(tmp_ctx);
- return ret;
+}
- errno_t krb5_child_init(struct krb5_ctx *krb5_auth_ctx, struct be_ctx *bectx) {
@@ -90,6 +182,15 @@ errno_t krb5_child_init(struct krb5_ctx *krb5_auth_ctx, goto done; }
ret = parse_krb5_map_user(krb5_auth_ctx, krb5_auth_ctx->opts,
&krb5_auth_ctx->name_to_princ,&krb5_auth_ctx->name_to_princ_size);if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "parse_krb5_map_user failed: %s:[%d]\n",sss_strerror(ret), ret);goto done;}
ret = EOK;done:
diff --git a/src/providers/krb5/krb5_opts.h b/src/providers/krb5/krb5_opts.h index db62cc3b23d8a5eadd3df97a102fb84128b740da..50d701b8ba431719dbf67799230ec345cdee30a9 100644 --- a/src/providers/krb5/krb5_opts.h +++ b/src/providers/krb5/krb5_opts.h @@ -45,6 +45,7 @@ struct dp_option default_krb5_opts[] = { { "krb5_canonicalize", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
- { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING }, DP_OPTION_TERMINATOR };
-- 2.1.0
From ffe3d9f0e0e697f565fb28861df41fdd9c7bbbe2 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 29 Apr 2015 06:03:04 -0400 Subject: [PATCH 3/3] krb5: remove field run_as_user
Oops, this is a leftover from the rootless sssd changes. ACK. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Please see updated patch.
On Thu, May 14, 2015 at 01:55:01PM +0200, Pavel Reichl wrote:
Please see updated patch.
Thanks, this looks much better. I only have some nitpicks now:
From 4f0bd09258e52ae633bc91603e30edf9ce02b279 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
</para><para>`joe` and `vince` are UNIX user names and `juser`and `rraines` are primaries of kerberos principals.For user `joe` resp. `dick` SSSD will try to kinitas `juser@REALM` resp. `richard@REALM`
Could we use something else than backticks in the XML. Remember we used XML because it's possible to render other formats than troff, we already render HTML. Would <quote> be a better choice?
</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para></refsect1>
[...]
+static errno_t split_tuple(TALLOC_CTX *mem_ctx, const char *tuple,
const char **_first, const char **_second)+{
- errno_t ret;
- char **list;
- int n;
- talloc(mem_ctx, int);
huh?
- ret = split_on_separator(mem_ctx, tuple, ':', true, true, &list, &n);
- if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"split_on_separator failed - %s:[%d]\n",sss_strerror(ret), ret);goto done;- } else if (n != 2) {
DEBUG(SSSDBG_MINOR_FAILURE, "split_on_separator failed.\n");ret = EINVAL;goto done;- }
- *_first = list[0];
- *_second = list[1];
+done:
- return ret;
+}
+static errno_t +fill_name_to_primary_map(TALLOC_CTX *mem_ctx, char **map,
struct map_id_name_to_krb_primary *name_to_primary,int size)
Can we use size_t for size?
+{
- int i;
- errno_t ret;
- for (i = 0; i < size; i++) {
ret = split_tuple(mem_ctx, map[i],&name_to_primary[i].id_name,&name_to_primary[i].krb_primary);if (ret != EOK) {DEBUG(SSSDBG_MINOR_FAILURE,"split_tuple failed - %s:[%d]\n", sss_strerror(ret), ret);goto done;}- }
- ret = EOK;
+done:
- return ret;
+}
+errno_t +parse_krb5_map_user(TALLOC_CTX *mem_ctx, const char *krb5_map_user,
struct map_id_name_to_krb_primary **_name_to_primary)+{
- int size;
- char **map;
- errno_t ret;
- TALLOC_CTX *tmp_ctx;
- struct map_id_name_to_krb_primary *name_to_primary;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;goto done;- }
- if (krb5_map_user == NULL || strlen(krb5_map_user) == 0) {
DEBUG(SSSDBG_FUNC_DATA, "Warning: krb5_map_user is empty!\n");size = 0;ret = EOK;goto done;- }
I won't block the patch over this but I still don't understand why you prefer to use the NULL pointer for "an empty mapping" instead of always allocating the sentinel. I'd argue that using the sentinel would ultimately make the code easier because you wouldn't have to check for NULL mapping pointer, but just loop over the mapping array..one less special case..
- ret = split_on_separator(tmp_ctx, krb5_map_user, ',', true, true,
&map, &size);- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "Failed to parse krb5_map_user!\n");goto done;- }
- name_to_primary = talloc_zero_array(tmp_ctx,
struct map_id_name_to_krb_primary,size + 1);- if (name_to_primary == NULL) {
ret = ENOMEM;goto done;- }
- /* sentinel */
- name_to_primary[size].id_name = NULL;
- name_to_primary[size].krb_primary = NULL;
- ret = fill_name_to_primary_map(name_to_primary, map, name_to_primary,
size);- if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"fill_name_to_primary_map failed: %s:[%d]\n",sss_strerror(ret), ret);goto done;- }
- ret = EOK;
+done:
- if (ret == EOK) {
if (size == 0) {*_name_to_primary = NULL;} else {*_name_to_primary = talloc_steal(mem_ctx, name_to_primary);}- }
- talloc_free(tmp_ctx);
- return ret;
+}
On 05/14/2015 05:10 PM, Jakub Hrozek wrote:
On Thu, May 14, 2015 at 01:55:01PM +0200, Pavel Reichl wrote:
Please see updated patch.
Thanks, this looks much better. I only have some nitpicks now:
From 4f0bd09258e52ae633bc91603e30edf9ce02b279 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
</para><para>`joe` and `vince` are UNIX user names and `juser`and `rraines` are primaries of kerberos principals.For user `joe` resp. `dick` SSSD will try to kinitas `juser@REALM` resp. `richard@REALM`Could we use something else than backticks in the XML. Remember we used XML because it's possible to render other formats than troff, we already render HTML. Would <quote> be a better choice?
done
</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para> </refsect1>[...]
+static errno_t split_tuple(TALLOC_CTX *mem_ctx, const char *tuple,
const char **_first, const char **_second)+{
- errno_t ret;
- char **list;
- int n;
- talloc(mem_ctx, int);
huh?
Sorry, but I need more changed lines to stats :-). fixed
- ret = split_on_separator(mem_ctx, tuple, ':', true, true, &list, &n);
- if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"split_on_separator failed - %s:[%d]\n",sss_strerror(ret), ret);goto done;- } else if (n != 2) {
DEBUG(SSSDBG_MINOR_FAILURE, "split_on_separator failed.\n");ret = EINVAL;goto done;- }
- *_first = list[0];
- *_second = list[1];
+done:
- return ret;
+}
+static errno_t +fill_name_to_primary_map(TALLOC_CTX *mem_ctx, char **map,
struct map_id_name_to_krb_primary *name_to_primary,int size)Can we use size_t for size?
sure, but it takes input from split_on_separator() which is int, should I fix split_on_separator() to use size_t instead if int?
+{
- int i;
- errno_t ret;
- for (i = 0; i < size; i++) {
ret = split_tuple(mem_ctx, map[i],&name_to_primary[i].id_name,&name_to_primary[i].krb_primary);if (ret != EOK) {DEBUG(SSSDBG_MINOR_FAILURE,"split_tuple failed - %s:[%d]\n", sss_strerror(ret), ret);goto done;}- }
- ret = EOK;
+done:
- return ret;
+}
+errno_t +parse_krb5_map_user(TALLOC_CTX *mem_ctx, const char *krb5_map_user,
struct map_id_name_to_krb_primary **_name_to_primary)+{
- int size;
- char **map;
- errno_t ret;
- TALLOC_CTX *tmp_ctx;
- struct map_id_name_to_krb_primary *name_to_primary;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;goto done;- }
- if (krb5_map_user == NULL || strlen(krb5_map_user) == 0) {
DEBUG(SSSDBG_FUNC_DATA, "Warning: krb5_map_user is empty!\n");size = 0;ret = EOK;goto done;- }
I won't block the patch over this but I still don't understand why you prefer to use the NULL pointer for "an empty mapping" instead of always allocating the sentinel. I'd argue that using the sentinel would ultimately make the code easier because you wouldn't have to check for NULL mapping pointer, but just loop over the mapping array..one less special case..
done
- ret = split_on_separator(tmp_ctx, krb5_map_user, ',', true, true,
&map, &size);- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "Failed to parse krb5_map_user!\n");goto done;- }
- name_to_primary = talloc_zero_array(tmp_ctx,
struct map_id_name_to_krb_primary,size + 1);- if (name_to_primary == NULL) {
ret = ENOMEM;goto done;- }
- /* sentinel */
- name_to_primary[size].id_name = NULL;
- name_to_primary[size].krb_primary = NULL;
- ret = fill_name_to_primary_map(name_to_primary, map, name_to_primary,
size);- if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"fill_name_to_primary_map failed: %s:[%d]\n",sss_strerror(ret), ret);goto done;- }
- ret = EOK;
+done:
- if (ret == EOK) {
if (size == 0) {*_name_to_primary = NULL;} else {*_name_to_primary = talloc_steal(mem_ctx, name_to_primary);}- }
- talloc_free(tmp_ctx);
- return ret;
+}
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (14/05/15 18:37), Pavel Reichl wrote:
+static errno_t split_tuple(TALLOC_CTX *mem_ctx, const char *tuple,
const char **_first, const char **_second)+{
- errno_t ret;
- char **list;
- int n;
- talloc(mem_ctx, int);
huh?
Sorry, but I need more changed lines to stats :-). fixed
You would have more lines in stat you sent patches with unit tests.
LS
On Thu, May 14, 2015 at 06:37:37PM +0200, Pavel Reichl wrote:
+static errno_t +fill_name_to_primary_map(TALLOC_CTX *mem_ctx, char **map,
struct map_id_name_to_krb_primary *name_to_primary,int size)Can we use size_t for size?
sure, but it takes input from split_on_separator() which is int, should I fix split_on_separator() to use size_t instead if int?
Nah, that's fine.
I'll take a look at the rest..
On Thu, May 14, 2015 at 06:37:37PM +0200, Pavel Reichl wrote: See some more comments inline. Hopefully we're almost there :-)
From 2b027b7f702bfdc8453e0a4c7096e6943d7141ca Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
Resolves: https://fedorahosted.org/sssd/ticket/2509
src/config/SSSDConfig/__init__.py.in | 1 + src/config/SSSDConfigTest.py | 9 ++- src/config/etc/sssd.api.d/sssd-ad.conf | 1 + src/config/etc/sssd.api.d/sssd-ipa.conf | 1 + src/config/etc/sssd.api.d/sssd-krb5.conf | 1 + src/man/sssd-krb5.5.xml | 36 ++++++++++ src/providers/ad/ad_opts.h | 1 + src/providers/ipa/ipa_opts.h | 1 + src/providers/krb5/krb5_access.c | 5 +- src/providers/krb5/krb5_auth.c | 55 +++++++++++++-- src/providers/krb5/krb5_auth.h | 2 + src/providers/krb5/krb5_common.h | 8 +++ src/providers/krb5/krb5_init_shared.c | 11 +++ src/providers/krb5/krb5_opts.h | 1 + src/providers/krb5/krb5_utils.c | 110 ++++++++++++++++++++++++++++++ src/providers/krb5/krb5_utils.h | 5 ++ src/tests/krb5_utils-tests.c | 112 +++++++++++++++++++++++++++++++ 17 files changed, 349 insertions(+), 11 deletions(-)
[...]
+static errno_t +get_krb_primary(struct map_id_name_to_krb_primary *name_to_primary,
char *id_prov_name, const char **_krb_primary)+{
- errno_t ret;
- int i = 0;
- while(name_to_primary != NULL &&
name_to_primary[i].id_name != NULL &&name_to_primary[i].krb_primary != NULL) {ret = sss_utf8_case_eq((const uint8_t*)name_to_primary[i].id_name,(const uint8_t*)id_prov_name);
This is mostly my fault, I told you to use sss_utf8_case_eq..but this should only be done for case-insensitive domains :-)
It looks like sss_string_equal() is a nice wrapper we can use.
if (ret == EOK) {*_krb_primary = name_to_primary[i].krb_primary;goto done;}i++;- }
- /* Handle also the case of name_to_primary being NULL */
- ret = ENOENT;
+done:
- return ret;
+}
errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd, struct krb5_ctx *krb5_ctx, struct krb5child_req **krb5_req) { struct krb5child_req *kr = NULL;
const char *mapped_name;
errno_t ret;
kr = talloc_zero(mem_ctx, struct krb5child_req); if (kr == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
return ENOMEM;
ret = ENOMEM;goto done;@@ -191,9 +221,25 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd, kr->pd = pd; kr->krb5_ctx = krb5_ctx;
- ret = get_krb_primary(krb5_ctx->name_to_primary,
pd->user, &mapped_name);- if (ret == EOK) {
DEBUG(SSSDBG_TRACE_FUNC, "Setting mapped name to: %s\n", mapped_name);kr->user = mapped_name;- } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "No mapping for: %s\n", pd->user);kr->user = pd->user;- } else {
DEBUG(SSSDBG_CRIT_FAILURE, "get_krb_primary failed - %s:[%d]\n",sss_strerror(ret), ret);goto done;- }
- *krb5_req = kr;
- ret = EOK;
- return EOK;
+done:
Since we added the done handler, can we also free kr on error to avoid polluting mem_ctx in that case?
- return ret;
}
[...]
--- a/src/providers/krb5/krb5_utils.c +++ b/src/providers/krb5/krb5_utils.c @@ -465,3 +465,113 @@ errno_t get_domain_or_subdomain(struct be_ctx *be_ctx,
return EOK;}
+static errno_t split_tuple(TALLOC_CTX *mem_ctx, const char *tuple,
const char **_first, const char **_second)+{
- errno_t ret;
- char **list;
- int n;
- ret = split_on_separator(mem_ctx, tuple, ':', true, true, &list, &n);
- if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"split_on_separator failed - %s:[%d]\n",sss_strerror(ret), ret);goto done;- } else if (n != 2) {
DEBUG(SSSDBG_MINOR_FAILURE, "split_on_separator failed.\n");
Please use a better error message that will make it clear the tuple has unexpected number of components.
ret = EINVAL;goto done;- }
- *_first = list[0];
- *_second = list[1];
+done:
- return ret;
+}
+static errno_t +fill_name_to_primary_map(TALLOC_CTX *mem_ctx, char **map,
struct map_id_name_to_krb_primary *name_to_primary,size_t size)+{
- int i;
- errno_t ret;
- for (i = 0; i < size; i++) {
ret = split_tuple(mem_ctx, map[i],&name_to_primary[i].id_name,&name_to_primary[i].krb_primary);if (ret != EOK) {DEBUG(SSSDBG_MINOR_FAILURE,"split_tuple failed - %s:[%d]\n", sss_strerror(ret), ret);goto done;}- }
- ret = EOK;
+done:
- return ret;
+}
+errno_t +parse_krb5_map_user(TALLOC_CTX *mem_ctx, const char *krb5_map_user,
struct map_id_name_to_krb_primary **_name_to_primary)+{
- int size;
- char **map;
- errno_t ret;
- TALLOC_CTX *tmp_ctx;
- struct map_id_name_to_krb_primary *name_to_primary;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;goto done;- }
- if (krb5_map_user == NULL || strlen(krb5_map_user) == 0) {
DEBUG(SSSDBG_FUNC_DATA, "Warning: krb5_map_user is empty!\n");size = 0;- } else {
ret = split_on_separator(tmp_ctx, krb5_map_user, ',', true, true,&map, &size);if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE, "Failed to parse krb5_map_user!\n");goto done;}- }
- name_to_primary = talloc_zero_array(tmp_ctx,
struct map_id_name_to_krb_primary,size + 1);- if (name_to_primary == NULL) {
ret = ENOMEM;goto done;- }
- /* sentinel */
- name_to_primary[size].id_name = NULL;
- name_to_primary[size].krb_primary = NULL;
- ret = fill_name_to_primary_map(name_to_primary, map, name_to_primary,
size);
Coverity complains here: sssd-1.12.90/src/providers/krb5/krb5_utils.c:526: var_decl: Declaring variable "map" without initializer. sssd-1.12.90/src/providers/krb5/krb5_utils.c:560: uninit_use_in_call: Using uninitialized value "map" when calling "fill_name_to_primary_map". sssd-1.12.90/src/providers/krb5/krb5_utils.c:505:9: read_parm: Reading a parameter value. # 558| name_to_primary[size].krb_primary = NULL; # 559| # 560|-> ret = fill_name_to_primary_map(name_to_primary, map, name_to_primary, # 561| size); # 562| if (ret != EOK) {
I'll test the patch now..
On Tue, May 26, 2015 at 11:13:38AM +0200, Jakub Hrozek wrote:
I'll test the patch now..
Functionality passed:
[jhrozek@client] sssd $ [(review)] su - jhrozek Password: (I used the IPA admin password here) [jhrozek@client] ~ $ [] klist Ticket cache: KEYRING:persistent:1000:krb_ccache_tovv73R Default principal: admin@LINUX.TEST
Valid starting Expires Service principal 05/26/2015 15:07:31 05/27/2015 15:07:31 krbtgt/LINUX.TEST@LINUX.TEST
So fix the nitpicks and I'll ack :-)
On 05/26/2015 03:09 PM, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 11:13:38AM +0200, Jakub Hrozek wrote:
I'll test the patch now..
Functionality passed:
[jhrozek@client] sssd $ [(review)] su - jhrozek Password: (I used the IPA admin password here) [jhrozek@client] ~ $ [] klist Ticket cache: KEYRING:persistent:1000:krb_ccache_tovv73R Default principal: admin@LINUX.TEST
Valid starting Expires Service principal 05/26/2015 15:07:31 05/27/2015 15:07:31 krbtgt/LINUX.TEST@LINUX.TEST
So fix the nitpicks and I'll ack :-)
Great, thanks.
Please see attached patch. I'm completely sure that I've absolutely sorted out the nitpicks...unless I've made them even worse. :-)
I think that the coverity warning was false positive, because the map value would never be read when uninitialized, but to get rid of the warning I added a check and call the function conditionally. Would you prefer If I rather initialized the variable?
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, May 26, 2015 at 03:39:20PM +0200, Pavel Reichl wrote:
On 05/26/2015 03:09 PM, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 11:13:38AM +0200, Jakub Hrozek wrote:
I'll test the patch now..
Functionality passed:
[jhrozek@client] sssd $ [(review)] su - jhrozek Password: (I used the IPA admin password here) [jhrozek@client] ~ $ [] klist Ticket cache: KEYRING:persistent:1000:krb_ccache_tovv73R Default principal: admin@LINUX.TEST
Valid starting Expires Service principal 05/26/2015 15:07:31 05/27/2015 15:07:31 krbtgt/LINUX.TEST@LINUX.TEST
So fix the nitpicks and I'll ack :-)
Great, thanks.
Please see attached patch. I'm completely sure that I've absolutely sorted out the nitpicks...unless I've made them even worse. :-)
I think that the coverity warning was false positive, because the map value would never be read when uninitialized, but to get rid of the warning I added a check and call the function conditionally. Would you prefer If I rather initialized the variable?
This is fine.
I found one typo in manpage (sorry..), the rest looks good to me now. I tested proxy user, IPA user and AD trust user, all worked fine.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 81473f2441dcdfb3c04864414d9bb30a20a2740d Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
diff --git a/src/man/sssd-krb5.5.xml b/src/man/sssd-krb5.5.xml index 8d5bbeed6ce6ec6bcb2db09895ca045905338639..eee6dfbdf9f8ae75b6b20d8f3d3cf21d7e38971f 100644 --- a/src/man/sssd-krb5.5.xml +++ b/src/man/sssd-krb5.5.xml @@ -516,6 +516,42 @@ </listitem> </varlistentry>
<varlistentry><term>krb5_map_user (string)</term><listitem><para>The list of mappings is given as a comma-separatedlist of pairs <quote>username:primary</quote>where <quote>username</quote> is a UNIX user nameand <quote>primary</quote> is a user part ofa kerberos principal. This mapping is used whenuser is authenticating using<quote>auth_provider = krb5</quote>.</para><para>example:+<programlisting> +krb5_realm = REALM +krb5_map_user = joe:juser,dick:richard +</programlisting>
</para><para><quote>joe</quote> and <quote>vince</quote> areUNIX user names and <quote>juser</quote> and<quote>rraines</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.<quote>richard@REALM</quote>.
The example gives joe and dick but the text talks about joe and vince.
On 05/26/2015 04:22 PM, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 03:39:20PM +0200, Pavel Reichl wrote:
On 05/26/2015 03:09 PM, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 11:13:38AM +0200, Jakub Hrozek wrote:
I'll test the patch now..
Functionality passed:
[jhrozek@client] sssd $ [(review)] su - jhrozek Password: (I used the IPA admin password here) [jhrozek@client] ~ $ [] klist Ticket cache: KEYRING:persistent:1000:krb_ccache_tovv73R Default principal: admin@LINUX.TEST
Valid starting Expires Service principal 05/26/2015 15:07:31 05/27/2015 15:07:31 krbtgt/LINUX.TEST@LINUX.TEST
So fix the nitpicks and I'll ack :-)
Great, thanks.
Please see attached patch. I'm completely sure that I've absolutely sorted out the nitpicks...unless I've made them even worse. :-)
I think that the coverity warning was false positive, because the map value would never be read when uninitialized, but to get rid of the warning I added a check and call the function conditionally. Would you prefer If I rather initialized the variable?
This is fine.
I found one typo in manpage (sorry..), the rest looks good to me now. I tested proxy user, IPA user and AD trust user, all worked fine.
Sorry for missing that. Fixed.
I'm happy that testing passed.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 81473f2441dcdfb3c04864414d9bb30a20a2740d Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
diff --git a/src/man/sssd-krb5.5.xml b/src/man/sssd-krb5.5.xml index 8d5bbeed6ce6ec6bcb2db09895ca045905338639..eee6dfbdf9f8ae75b6b20d8f3d3cf21d7e38971f 100644 --- a/src/man/sssd-krb5.5.xml +++ b/src/man/sssd-krb5.5.xml @@ -516,6 +516,42 @@ </listitem> </varlistentry>
<varlistentry><term>krb5_map_user (string)</term><listitem><para>The list of mappings is given as a comma-separatedlist of pairs <quote>username:primary</quote>where <quote>username</quote> is a UNIX user nameand <quote>primary</quote> is a user part ofa kerberos principal. This mapping is used whenuser is authenticating using<quote>auth_provider = krb5</quote>.</para><para>example:+<programlisting> +krb5_realm = REALM +krb5_map_user = joe:juser,dick:richard +</programlisting>
</para><para><quote>joe</quote> and <quote>vince</quote> areUNIX user names and <quote>juser</quote> and<quote>rraines</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.<quote>richard@REALM</quote>.The example gives joe and dick but the text talks about joe and vince. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, May 26, 2015 at 04:36:18PM +0200, Pavel Reichl wrote:
From 923e68ba56f276db473a38fffe339a0dc9770a4f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
<quote>joe</quote> and <quote>dick</quote> areUNIX user names and <quote>juser</quote> and<quote>richard</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.
kinit as juser@REALM right?
<quote>richard@REALM</quote>.</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para></refsect1>
But since this is the last nitpick I found (for real this time :-)) I can fix this up locally and push..
On 05/27/2015 09:40 AM, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 04:36:18PM +0200, Pavel Reichl wrote:
From 923e68ba56f276db473a38fffe339a0dc9770a4f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
<quote>joe</quote> and <quote>dick</quote> areUNIX user names and <quote>juser</quote> and<quote>richard</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.kinit as juser@REALM right?
Yes, you are right.
<quote>richard@REALM</quote>.</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para> </refsect1>But since this is the last nitpick I found (for real this time :-)) I can fix this up locally and push..
Please do. I've read this paragraph several times yesterday I can't believe I missed that.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, May 27, 2015 at 09:40:13AM +0200, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 04:36:18PM +0200, Pavel Reichl wrote:
From 923e68ba56f276db473a38fffe339a0dc9770a4f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
<quote>joe</quote> and <quote>dick</quote> areUNIX user names and <quote>juser</quote> and<quote>richard</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.kinit as juser@REALM right?
<quote>richard@REALM</quote>.</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para></refsect1>
But since this is the last nitpick I found (for real this time :-)) I can fix this up locally and push..
Attached is a patch I'm about to push.
On 05/28/2015 11:04 AM, Jakub Hrozek wrote:
On Wed, May 27, 2015 at 09:40:13AM +0200, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 04:36:18PM +0200, Pavel Reichl wrote:
From 923e68ba56f276db473a38fffe339a0dc9770a4f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
<quote>joe</quote> and <quote>dick</quote> areUNIX user names and <quote>juser</quote> and<quote>richard</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.kinit as juser@REALM right?
<quote>richard@REALM</quote>.</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para> </refsect1>But since this is the last nitpick I found (for real this time :-)) I can fix this up locally and push..
Attached is a patch I'm about to push.
Man page looks good to me. Sorry for so many mistakes in such a short text.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, May 28, 2015 at 11:09:50AM +0200, Pavel Reichl wrote:
On 05/28/2015 11:04 AM, Jakub Hrozek wrote:
On Wed, May 27, 2015 at 09:40:13AM +0200, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 04:36:18PM +0200, Pavel Reichl wrote:
From 923e68ba56f276db473a38fffe339a0dc9770a4f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
<quote>joe</quote> and <quote>dick</quote> areUNIX user names and <quote>juser</quote> and<quote>richard</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.kinit as juser@REALM right?
<quote>richard@REALM</quote>.</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para></refsect1>
But since this is the last nitpick I found (for real this time :-)) I can fix this up locally and push..
Attached is a patch I'm about to push.
Man page looks good to me. Sorry for so many mistakes in such a short text.
CI link: http://sssd-ci.duckdns.org/logs/job/16/06/summary.html * master: aa8a8318aaa3270e9d9957d0c22dec6342360a37 * sssd-1-12: c494e100f9b2422e2890507f63019afcaff9b7c6
I still think it makes sense to push the patch to sssd-1-12 as well -- it's not too risky and there's quite a few users who'd like to see this feature.
On Thu, 2015-05-28 at 11:29 +0200, Jakub Hrozek wrote:
On Thu, May 28, 2015 at 11:09:50AM +0200, Pavel Reichl wrote:
On 05/28/2015 11:04 AM, Jakub Hrozek wrote:
On Wed, May 27, 2015 at 09:40:13AM +0200, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 04:36:18PM +0200, Pavel Reichl wrote:
From 923e68ba56f276db473a38fffe339a0dc9770a4f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
<quote>joe</quote> and <quote>dick</quote> areUNIX user names and <quote>juser</quote> and<quote>richard</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.kinit as juser@REALM right?
<quote>richard@REALM</quote>.</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para></refsect1>
But since this is the last nitpick I found (for real this time :-)) I can fix this up locally and push..
Attached is a patch I'm about to push.
Man page looks good to me. Sorry for so many mistakes in such a short text.
CI link: http://sssd-ci.duckdns.org/logs/job/16/06/summary.html
- master: aa8a8318aaa3270e9d9957d0c22dec6342360a37
- sssd-1-12: c494e100f9b2422e2890507f63019afcaff9b7c6
I still think it makes sense to push the patch to sssd-1-12 as well -- it's not too risky and there's quite a few users who'd like to see this feature.
I know this has been pushed already, but I am not very happy about this feature. Why is krb5_primary missing the realm part ? We already support trust relationships with kerberos why can't I have: joe:jbar@SUB1.REALM.COM, jane:jbar@SUB2.REALM.COM ?
I think allowing shortcuts may be fine but we should also allow admin to explicitly map to a specific realm.
Simo.
On Mon, Jun 01, 2015 at 08:02:43PM -0400, Simo Sorce wrote:
On Thu, 2015-05-28 at 11:29 +0200, Jakub Hrozek wrote:
On Thu, May 28, 2015 at 11:09:50AM +0200, Pavel Reichl wrote:
On 05/28/2015 11:04 AM, Jakub Hrozek wrote:
On Wed, May 27, 2015 at 09:40:13AM +0200, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 04:36:18PM +0200, Pavel Reichl wrote:
From 923e68ba56f276db473a38fffe339a0dc9770a4f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
<quote>joe</quote> and <quote>dick</quote> areUNIX user names and <quote>juser</quote> and<quote>richard</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.kinit as juser@REALM right?
<quote>richard@REALM</quote>.</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para></refsect1>
But since this is the last nitpick I found (for real this time :-)) I can fix this up locally and push..
Attached is a patch I'm about to push.
Man page looks good to me. Sorry for so many mistakes in such a short text.
CI link: http://sssd-ci.duckdns.org/logs/job/16/06/summary.html
- master: aa8a8318aaa3270e9d9957d0c22dec6342360a37
- sssd-1-12: c494e100f9b2422e2890507f63019afcaff9b7c6
I still think it makes sense to push the patch to sssd-1-12 as well -- it's not too risky and there's quite a few users who'd like to see this feature.
I know this has been pushed already, but I am not very happy about this feature. Why is krb5_primary missing the realm part ? We already support trust relationships with kerberos why can't I have: joe:jbar@SUB1.REALM.COM, jane:jbar@SUB2.REALM.COM ?
I think allowing shortcuts may be fine but we should also allow admin to explicitly map to a specific realm.
We can extend the feature, I don't think it would would be too problematic. But the main use-case so far is very narrow -- laptop users who don't want to convert away from their UNIX user but would like to use the SSSD goodies like delayed kinit. So we started simple.
Feel free to file a ticket, Pavel can extend the mapping..
On 06/02/2015 09:01 AM, Jakub Hrozek wrote:
On Mon, Jun 01, 2015 at 08:02:43PM -0400, Simo Sorce wrote:
On Thu, 2015-05-28 at 11:29 +0200, Jakub Hrozek wrote:
On Thu, May 28, 2015 at 11:09:50AM +0200, Pavel Reichl wrote:
On 05/28/2015 11:04 AM, Jakub Hrozek wrote:
On Wed, May 27, 2015 at 09:40:13AM +0200, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 04:36:18PM +0200, Pavel Reichl wrote: > From 923e68ba56f276db473a38fffe339a0dc9770a4f Mon Sep 17 00:00:00 2001 > From: Pavel Reichl preichl@redhat.com > Date: Thu, 30 Apr 2015 06:43:05 -0400 > Subject: [PATCH] krb5: new option krb5_map_user > > New option `krb5_map_user` providing mapping of ID provider names to > Kerberos principals. > > Resolves: > https://fedorahosted.org/sssd/ticket/2509 [...]
> + <quote>joe</quote> and <quote>dick</quote> are > + UNIX user names and <quote>juser</quote> and > + <quote>richard</quote> are primaries of kerberos > + principals. For user <quote>joe</quote> resp. > + <quote>dick</quote> SSSD will try to kinit as > + <quote>dick@REALM</quote> resp. kinit as juser@REALM right?
> + <quote>richard@REALM</quote>. > + </para> > + > + <para> > + Default: not set > + </para> > + </listitem> > + </varlistentry> > + > </variablelist> > </para> > </refsect1> But since this is the last nitpick I found (for real this time :-)) I can fix this up locally and push..
Attached is a patch I'm about to push.
Man page looks good to me. Sorry for so many mistakes in such a short text.
CI link: http://sssd-ci.duckdns.org/logs/job/16/06/summary.html
- master: aa8a8318aaa3270e9d9957d0c22dec6342360a37
- sssd-1-12: c494e100f9b2422e2890507f63019afcaff9b7c6
I still think it makes sense to push the patch to sssd-1-12 as well -- it's not too risky and there's quite a few users who'd like to see this feature.
I know this has been pushed already, but I am not very happy about this feature. Why is krb5_primary missing the realm part ? We already support trust relationships with kerberos why can't I have: joe:jbar@SUB1.REALM.COM, jane:jbar@SUB2.REALM.COM ?
I think allowing shortcuts may be fine but we should also allow admin to explicitly map to a specific realm.
We can extend the feature, I don't think it would would be too problematic. But the main use-case so far is very narrow -- laptop users who don't want to convert away from their UNIX user but would like to use the SSSD goodies like delayed kinit. So we started simple.
Feel free to file a ticket, Pavel can extend the mapping..
Sure, It should not take long to add this feature.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (26/05/15 16:36), Pavel Reichl wrote:
On 05/26/2015 04:22 PM, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 03:39:20PM +0200, Pavel Reichl wrote:
On 05/26/2015 03:09 PM, Jakub Hrozek wrote:
On Tue, May 26, 2015 at 11:13:38AM +0200, Jakub Hrozek wrote:
I'll test the patch now..
Functionality passed:
[jhrozek@client] sssd $ [(review)] su - jhrozek Password: (I used the IPA admin password here) [jhrozek@client] ~ $ [] klist Ticket cache: KEYRING:persistent:1000:krb_ccache_tovv73R Default principal: admin@LINUX.TEST
Valid starting Expires Service principal 05/26/2015 15:07:31 05/27/2015 15:07:31 krbtgt/LINUX.TEST@LINUX.TEST
So fix the nitpicks and I'll ack :-)
Great, thanks.
Please see attached patch. I'm completely sure that I've absolutely sorted out the nitpicks...unless I've made them even worse. :-)
I think that the coverity warning was false positive, because the map value would never be read when uninitialized, but to get rid of the warning I added a check and call the function conditionally. Would you prefer If I rather initialized the variable?
This is fine.
I found one typo in manpage (sorry..), the rest looks good to me now. I tested proxy user, IPA user and AD trust user, all worked fine.
Sorry for missing that. Fixed.
I'm happy that testing passed.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 81473f2441dcdfb3c04864414d9bb30a20a2740d Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
[...]
diff --git a/src/man/sssd-krb5.5.xml b/src/man/sssd-krb5.5.xml index 8d5bbeed6ce6ec6bcb2db09895ca045905338639..eee6dfbdf9f8ae75b6b20d8f3d3cf21d7e38971f 100644 --- a/src/man/sssd-krb5.5.xml +++ b/src/man/sssd-krb5.5.xml @@ -516,6 +516,42 @@ </listitem> </varlistentry>
<varlistentry><term>krb5_map_user (string)</term><listitem><para>The list of mappings is given as a comma-separatedlist of pairs <quote>username:primary</quote>where <quote>username</quote> is a UNIX user nameand <quote>primary</quote> is a user part ofa kerberos principal. This mapping is used whenuser is authenticating using<quote>auth_provider = krb5</quote>.</para><para>example:+<programlisting> +krb5_realm = REALM +krb5_map_user = joe:juser,dick:richard +</programlisting>
</para><para><quote>joe</quote> and <quote>vince</quote> areUNIX user names and <quote>juser</quote> and<quote>rraines</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.<quote>richard@REALM</quote>.The example gives joe and dick but the text talks about joe and vince. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 923e68ba56f276db473a38fffe339a0dc9770a4f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user
New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals.
Resolves: https://fedorahosted.org/sssd/ticket/2509
src/config/SSSDConfig/__init__.py.in | 1 + src/config/SSSDConfigTest.py | 9 ++- src/config/etc/sssd.api.d/sssd-ad.conf | 1 + src/config/etc/sssd.api.d/sssd-ipa.conf | 1 + src/config/etc/sssd.api.d/sssd-krb5.conf | 1 + src/man/sssd-krb5.5.xml | 36 ++++++++++ src/providers/ad/ad_opts.h | 1 + src/providers/ipa/ipa_opts.h | 1 + src/providers/krb5/krb5_access.c | 8 +-- src/providers/krb5/krb5_auth.c | 76 ++++++++++++++++++--- src/providers/krb5/krb5_auth.h | 5 +- src/providers/krb5/krb5_common.h | 8 +++ src/providers/krb5/krb5_init_shared.c | 11 +++ src/providers/krb5/krb5_opts.h | 1 + src/providers/krb5/krb5_utils.c | 114 +++++++++++++++++++++++++++++++ src/providers/krb5/krb5_utils.h | 5 ++ src/tests/krb5_utils-tests.c | 111 ++++++++++++++++++++++++++++++ 17 files changed, 372 insertions(+), 18 deletions(-)
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in index 31c9c648045f1e2b031c6f9b2196b44e9c4c4313..f58c52faf7dd3f9199bd0af4286546d4fe804a88 100644 --- a/src/config/SSSDConfig/__init__.py.in +++ b/src/config/SSSDConfig/__init__.py.in @@ -215,6 +215,7 @@ option_strings = { 'krb5_fast_principal' : _("Selects the principal to use for FAST"), 'krb5_canonicalize' : _("Enables principal canonicalization"), 'krb5_use_enterprise_principal' : _("Enables enterprise principals"),
'krb5_map_user' : _('A mapping from user names to kerberos principal names'),
# [provider/krb5/chpass] 'krb5_kpasswd' : _('Server where the change password service is running if not on the KDC'),
diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py index db16bc433cf4c47c6a15760d85b322a6655aa0c1..476e30806ceda64b14d25f5545a63785efaacf15 100755 --- a/src/config/SSSDConfigTest.py +++ b/src/config/SSSDConfigTest.py @@ -622,7 +622,8 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase): 'krb5_fast_principal', 'krb5_canonicalize', 'krb5_use_enterprise_principal',
'krb5_use_kdcinfo'])
'krb5_use_kdcinfo','krb5_map_user']) options = domain.list_options()@@ -782,7 +783,8 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase): 'krb5_fast_principal', 'krb5_canonicalize', 'krb5_use_enterprise_principal',
'krb5_use_kdcinfo']
'krb5_use_kdcinfo','krb5_map_user'] self.assertTrue(type(options) == dict, "Options should be a dictionary")@@ -983,7 +985,8 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase): 'krb5_fast_principal', 'krb5_canonicalize', 'krb5_use_enterprise_principal',
'krb5_use_kdcinfo'])
'krb5_use_kdcinfo','krb5_map_user']) options = domain.list_options()diff --git a/src/config/etc/sssd.api.d/sssd-ad.conf b/src/config/etc/sssd.api.d/sssd-ad.conf index 5a5ea0c36b07d7497c1caa4208c7270d6de6dcc9..c46f3a7cb50db519d113e15f425c7f746d34ad81 100644 --- a/src/config/etc/sssd.api.d/sssd-ad.conf +++ b/src/config/etc/sssd.api.d/sssd-ad.conf @@ -139,6 +139,7 @@ krb5_renew_interval = str, None, false krb5_use_fast = str, None, false krb5_fast_principal = str, None, false krb5_use_enterprise_principal = bool, None, false +krb5_map_user = str, None, false
[provider/ad/access]
diff --git a/src/config/etc/sssd.api.d/sssd-ipa.conf b/src/config/etc/sssd.api.d/sssd-ipa.conf index 230bdd7df3e7512eab9096c136624cdd7923ed96..6bae609fa9ff57e70c195b858eeea4eca680f62f 100644 --- a/src/config/etc/sssd.api.d/sssd-ipa.conf +++ b/src/config/etc/sssd.api.d/sssd-ipa.conf @@ -155,6 +155,7 @@ krb5_renew_interval = str, None, false krb5_use_fast = str, None, false krb5_fast_principal = str, None, false krb5_use_enterprise_principal = bool, None, false +krb5_map_user = str, None, false
[provider/ipa/access] ipa_hbac_refresh = int, None, false diff --git a/src/config/etc/sssd.api.d/sssd-krb5.conf b/src/config/etc/sssd.api.d/sssd-krb5.conf index e65ed01b688078aff090ff53b91779595fd6f465..b7423b74f7b6845d235cc523a8e249d6a74d69ab 100644 --- a/src/config/etc/sssd.api.d/sssd-krb5.conf +++ b/src/config/etc/sssd.api.d/sssd-krb5.conf @@ -21,6 +21,7 @@ krb5_use_fast = str, None, false krb5_fast_principal = str, None, false krb5_canonicalize = bool, None, false krb5_use_enterprise_principal = bool, None, false +krb5_map_user = str, None, false
[provider/krb5/access]
diff --git a/src/man/sssd-krb5.5.xml b/src/man/sssd-krb5.5.xml index 8d5bbeed6ce6ec6bcb2db09895ca045905338639..3d3c58cf6dfbd31c76d7a88e0ec849c10e15fe76 100644 --- a/src/man/sssd-krb5.5.xml +++ b/src/man/sssd-krb5.5.xml @@ -516,6 +516,42 @@ </listitem> </varlistentry>
<varlistentry><term>krb5_map_user (string)</term><listitem><para>The list of mappings is given as a comma-separatedlist of pairs <quote>username:primary</quote>where <quote>username</quote> is a UNIX user nameand <quote>primary</quote> is a user part ofa kerberos principal. This mapping is used whenuser is authenticating using<quote>auth_provider = krb5</quote>.</para><para>example:+<programlisting> +krb5_realm = REALM +krb5_map_user = joe:juser,dick:richard +</programlisting>
</para><para><quote>joe</quote> and <quote>dick</quote> areUNIX user names and <quote>juser</quote> and<quote>richard</quote> are primaries of kerberosprincipals. For user <quote>joe</quote> resp.<quote>dick</quote> SSSD will try to kinit as<quote>dick@REALM</quote> resp.<quote>richard@REALM</quote>.</para><para>Default: not set</para></listitem></varlistentry></variablelist> </para></refsect1>
diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h index 15b140434fec815aeee989e24cc1b7930f040add..6e859447f927ef683d53bf08d25d658764581348 100644 --- a/src/providers/ad/ad_opts.h +++ b/src/providers/ad/ad_opts.h @@ -168,6 +168,7 @@ struct dp_option ad_def_krb5_opts[] = { { "krb5_canonicalize", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE }, { "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
- { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING }, DP_OPTION_TERMINATOR
};
diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h index 8a0764265521e86ca86249e4b62f0f967bc44189..34e9e167eb46f290d017b5af817571122b359b4f 100644 --- a/src/providers/ipa/ipa_opts.h +++ b/src/providers/ipa/ipa_opts.h @@ -310,6 +310,7 @@ struct dp_option ipa_def_krb5_opts[] = { { "krb5_canonicalize", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE }, { "krb5_use_enterprise_principal", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "krb5_use_kdcinfo", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
- { "krb5_map_user", DP_OPT_STRING, NULL_STRING, NULL_STRING }, DP_OPTION_TERMINATOR
};
diff --git a/src/providers/krb5/krb5_access.c b/src/providers/krb5/krb5_access.c index 7fda2a37922a537f7fe53d629c4e0cb4df1bd4da..3afb90150d77ef4ab2c1b5b79abb95d68eb131f6 100644 --- a/src/providers/krb5/krb5_access.c +++ b/src/providers/krb5/krb5_access.c @@ -64,7 +64,8 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx, state->krb5_ctx = krb5_ctx; state->access_allowed = false;
- ret = krb5_setup(state, pd, krb5_ctx, &state->kr);
- ret = krb5_setup(state, pd, krb5_ctx, be_ctx->domain->case_sensitive,
&state->kr);@@ -105,9 +106,8 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx, goto done; break; case 1:
ret = find_or_guess_upn(state, res->msgs[0], krb5_ctx,be_ctx->domain, pd->user, pd->domain,&state->kr->upn);
ret = find_or_guess_upn(state, res->msgs[0], krb5_ctx, be_ctx->domain,state->kr->user, pd->domain, &state->kr->upn); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "find_or_guess_upn failed.\n"); goto done;diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c index 148b08fdf860e92d00be0582eb73a822113f3880..8c851442b31994f819f33722bb67d19bb01e4b77 100644 --- a/src/providers/krb5/krb5_auth.c +++ b/src/providers/krb5/krb5_auth.c @@ -36,6 +36,7 @@ #include "util/find_uid.h" #include "util/auth_utils.h" #include "db/sysdb.h" +#include "util/sss_utf8.h" #include "util/child_common.h" #include "providers/krb5/krb5_auth.h" #include "providers/krb5/krb5_utils.h" @@ -175,15 +176,51 @@ static int krb5_cleanup(void *ptr) return EOK; }
+static errno_t +get_krb_primary(struct map_id_name_to_krb_primary *name_to_primary,
char *id_prov_name, bool cs, const char **_krb_primary)+{
- errno_t ret;
- int i = 0;
- while(name_to_primary != NULL &&
name_to_primary[i].id_name != NULL &&
^^ I thought we have a convention to use binary operatort in the begining of line and not at the end.
IIRC Stephen sent mail with proposal and we agreed. Sumit uses it quite often.
LS
On Wed, May 27, 2015 at 09:52:11AM +0200, Lukas Slebodnik wrote:
+static errno_t +get_krb_primary(struct map_id_name_to_krb_primary *name_to_primary,
char *id_prov_name, bool cs, const char **_krb_primary)+{
- errno_t ret;
- int i = 0;
- while(name_to_primary != NULL &&
name_to_primary[i].id_name != NULL &&^^ I thought we have a convention to use binary operatort in the begining of line and not at the end.IIRC Stephen sent mail with proposal and we agreed. Sumit uses it quite often.
Is that a nack or a remark?
btw these proposals should be really codified in the coding style on freeipa wiki (or our own coding style forked from freeipa).
On (27/05/15 10:38), Jakub Hrozek wrote:
On Wed, May 27, 2015 at 09:52:11AM +0200, Lukas Slebodnik wrote:
+static errno_t +get_krb_primary(struct map_id_name_to_krb_primary *name_to_primary,
char *id_prov_name, bool cs, const char **_krb_primary)+{
- errno_t ret;
- int i = 0;
- while(name_to_primary != NULL &&
name_to_primary[i].id_name != NULL &&^^ I thought we have a convention to use binary operatort in the begining of line and not at the end.IIRC Stephen sent mail with proposal and we agreed. Sumit uses it quite often.
Is that a nack or a remark?
It is a start of discussion (remark).
We should make an agreement what do we want to use. If we don't like Stephen's proposal then We should agree on something else.
It's really strange to see different convention in different places. and sometimes difficult (slower?) to read a code.
LS
On 05/27/2015 10:41 AM, Lukas Slebodnik wrote:
On (27/05/15 10:38), Jakub Hrozek wrote:
On Wed, May 27, 2015 at 09:52:11AM +0200, Lukas Slebodnik wrote:
+static errno_t +get_krb_primary(struct map_id_name_to_krb_primary *name_to_primary,
char *id_prov_name, bool cs, const char **_krb_primary)+{
- errno_t ret;
- int i = 0;
- while(name_to_primary != NULL &&
name_to_primary[i].id_name != NULL &&^^ I thought we have a convention to use binary operatort in the begining of line and not at the end.IIRC Stephen sent mail with proposal and we agreed. Sumit uses it quite often.
Is that a nack or a remark?
It is a start of discussion (remark).
We should make an agreement what do we want to use. If we don't like Stephen's proposal then We should agree on something else.
It's really strange to see different convention in different places. and sometimes difficult (slower?) to read a code.
LS
I honestly don't care much I don't think that this slowers my understanding of the code, for me other things are way more important. Lukas do you have subject of the mail? I must have missed that.
I did some fast grepping:
git grep '^[\t ]*(|||&&).*' -- '*.c' | wc -l 253
git grep '.*(|||&&)[ \t]*$' -- '*.c' | wc -l 354
It seems to me that logical operator && and || are more commonly used in the end of lines.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, May 27, 2015 at 10:41:14AM +0200, Lukas Slebodnik wrote:
On (27/05/15 10:38), Jakub Hrozek wrote:
On Wed, May 27, 2015 at 09:52:11AM +0200, Lukas Slebodnik wrote:
+static errno_t +get_krb_primary(struct map_id_name_to_krb_primary *name_to_primary,
char *id_prov_name, bool cs, const char **_krb_primary)+{
- errno_t ret;
- int i = 0;
- while(name_to_primary != NULL &&
name_to_primary[i].id_name != NULL &&^^ I thought we have a convention to use binary operatort in the begining of line and not at the end.IIRC Stephen sent mail with proposal and we agreed. Sumit uses it quite often.
Is that a nack or a remark?
It is a start of discussion (remark).
We should make an agreement what do we want to use. If we don't like Stephen's proposal then We should agree on something else.
It's really strange to see different convention in different places. and sometimes difficult (slower?) to read a code.
Well, I /do/ like what Stephen proposed, but if we start using it, we will have different convention in different places, because previously we were using the boolean operators at the end of the line, as Pavel did..
btw in the interest of moving on with this patch, I'll amend the manpage and push..we can keep discussing about new code..
On (27/05/15 11:40), Jakub Hrozek wrote:
On Wed, May 27, 2015 at 10:41:14AM +0200, Lukas Slebodnik wrote:
On (27/05/15 10:38), Jakub Hrozek wrote:
On Wed, May 27, 2015 at 09:52:11AM +0200, Lukas Slebodnik wrote:
+static errno_t +get_krb_primary(struct map_id_name_to_krb_primary *name_to_primary,
char *id_prov_name, bool cs, const char **_krb_primary)+{
- errno_t ret;
- int i = 0;
- while(name_to_primary != NULL &&
name_to_primary[i].id_name != NULL &&^^ I thought we have a convention to use binary operatort in the begining of line and not at the end.IIRC Stephen sent mail with proposal and we agreed. Sumit uses it quite often.
Is that a nack or a remark?
It is a start of discussion (remark).
We should make an agreement what do we want to use. If we don't like Stephen's proposal then We should agree on something else.
It's really strange to see different convention in different places. and sometimes difficult (slower?) to read a code.
Well, I /do/ like what Stephen proposed, but if we start using it, we will have different convention in different places, because previously we were using the boolean operators at the end of the line, as Pavel did..
We already have code with both version.
So we should decide which one is prefered and which one should not be used.
LS
On Thu, May 28, 2015 at 07:52:32PM +0200, Lukas Slebodnik wrote:
On (27/05/15 11:40), Jakub Hrozek wrote:
On Wed, May 27, 2015 at 10:41:14AM +0200, Lukas Slebodnik wrote:
On (27/05/15 10:38), Jakub Hrozek wrote:
On Wed, May 27, 2015 at 09:52:11AM +0200, Lukas Slebodnik wrote:
+static errno_t +get_krb_primary(struct map_id_name_to_krb_primary *name_to_primary,
char *id_prov_name, bool cs, const char **_krb_primary)+{
- errno_t ret;
- int i = 0;
- while(name_to_primary != NULL &&
name_to_primary[i].id_name != NULL &&^^ I thought we have a convention to use binary operatort in the begining of line and not at the end.IIRC Stephen sent mail with proposal and we agreed. Sumit uses it quite often.
Is that a nack or a remark?
It is a start of discussion (remark).
We should make an agreement what do we want to use. If we don't like Stephen's proposal then We should agree on something else.
It's really strange to see different convention in different places. and sometimes difficult (slower?) to read a code.
Well, I /do/ like what Stephen proposed, but if we start using it, we will have different convention in different places, because previously we were using the boolean operators at the end of the line, as Pavel did..
We already have code with both version.
So we should decide which one is prefered and which one should not be used.
OK, let's amend the FreeIPA coding guidelines then.
I like the multi-line condition splitting Stephen uses: if (fooptr == NULL || barptr == NULL) { do_stuff(); }
I would also like to formally say we can use C99. In particular, dynamic arrays are often handy if we know the size is within certain limits.
Would you like to start a separate thread?
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Reichl -
Simo Sorce