From 0e618c36ed74c240f7acd071ccb7bfd405b2d827 Mon Sep 17 00:00:00 2001 From: Sumit Bose Date: Tue, 22 Nov 2022 14:43:21 +0100 Subject: [PATCH 19/19] pac: relax default check To avoid issues with the UPN check during PAC validation when 'ldap_user_principal' is set to a not existing attribute to skip reading user principals a new 'pac_check' option, 'check_upn_allow_missing' is added to the default options. With this option only a log message is shown but the check will not fail. Resolves: https://github.com/SSSD/sssd/issues/6451 (cherry picked from commit 51b11db8b99a77ba5ccf6f850c2e81b5a6ee9f79) Reviewed-by: Alexey Tikhonov --- src/confdb/confdb.h | 2 +- src/man/sssd.conf.5.xml | 30 +++++++++++++++++++++++++++++- src/providers/ad/ad_pac_common.c | 24 ++++++++++++++++++++---- src/util/pac_utils.c | 10 ++++++++++ src/util/util.h | 2 ++ 5 files changed, 62 insertions(+), 6 deletions(-) diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 83f6be7f9..5fda67585 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -181,7 +181,7 @@ #define CONFDB_PAC_LIFETIME "pac_lifetime" #define CONFDB_PAC_CHECK "pac_check" #define CONFDB_PAC_CHECK_DEFAULT "no_check" -#define CONFDB_PAC_CHECK_IPA_AD_DEFAULT "check_upn, check_upn_dns_info_ex" +#define CONFDB_PAC_CHECK_IPA_AD_DEFAULT "check_upn, check_upn_allow_missing, check_upn_dns_info_ex" /* InfoPipe */ #define CONFDB_IFP_CONF_ENTRY "config/ifp" diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 7a9920815..d9f4a7481 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -2275,6 +2275,34 @@ pam_gssapi_indicators_map = sudo:pkinit, sudo-i:pkinit consistent. + + check_upn_allow_missing + + This option should be used together + with 'check_upn' and handles the case where + a UPN is set on the server-side but is not + read by SSSD. The typical example is a + FreeIPA domain where 'ldap_user_principal' + is set to a not existing attribute name. + This was typically done to work-around + issues in the handling of enterprise + principals. But this is fixed since quite + some time and FreeIPA can handle enterprise + principals just fine and there is no need + anymore to set 'ldap_user_principal'. + Currently this option is set by + default to avoid regressions in such + environments. A log message will be added + to the system log and SSSD's debug log in + case a UPN is found in the PAC but not in + SSSD's cache. To avoid this log message it + would be best to evaluate if the + 'ldap_user_principal' option can be removed. + If this is not possible, removing + 'check_upn' will skip the test and avoid the + log message. + + upn_dns_info_present @@ -2305,7 +2333,7 @@ pam_gssapi_indicators_map = sudo:pkinit, sudo-i:pkinit Default: no_check (AD and IPA provider - 'check_upn, check_upn_dns_info_ex') + 'check_upn, check_upn_allow_missing, check_upn_dns_info_ex') diff --git a/src/providers/ad/ad_pac_common.c b/src/providers/ad/ad_pac_common.c index 79f79b7a7..fcb54cd2c 100644 --- a/src/providers/ad/ad_pac_common.c +++ b/src/providers/ad/ad_pac_common.c @@ -215,10 +215,26 @@ errno_t check_upn_and_sid_from_user_and_pac(struct ldb_message *msg, DEBUG(SSSDBG_MINOR_FAILURE, "User object does not have a UPN but PAC " "says otherwise, maybe ldap_user_principal option is set.\n"); if (pac_check_opts & CHECK_PAC_CHECK_UPN) { - DEBUG(SSSDBG_CRIT_FAILURE, - "UPN is missing but PAC UPN check required, " - "PAC validation failed.\n"); - return ERR_CHECK_PAC_FAILED; + if (pac_check_opts & CHECK_PAC_CHECK_UPN_ALLOW_MISSING) { + DEBUG(SSSDBG_IMPORTANT_INFO, + "UPN is missing but PAC UPN check required, " + "PAC validation failed. However, " + "'check_upn_allow_missing' is set and the error is " + "ignored. To make this message go away please check " + "why the UPN is not read from the server. In FreeIPA " + "environments 'ldap_user_principal' is most probably " + "set to a non-existing attribute name to avoid " + "issues with enterprise principals. This is not " + "needed anymore with recent versions of FreeIPA.\n"); + sss_log(SSS_LOG_CRIT, "PAC validation issue, please check " + "sssd_pac.log for details"); + return EOK; + } else { + DEBUG(SSSDBG_CRIT_FAILURE, + "UPN is missing but PAC UPN check required, " + "PAC validation failed.\n"); + return ERR_CHECK_PAC_FAILED; + } } } diff --git a/src/util/pac_utils.c b/src/util/pac_utils.c index c53b0c082..4499d8dfd 100644 --- a/src/util/pac_utils.c +++ b/src/util/pac_utils.c @@ -64,6 +64,8 @@ static errno_t check_check_pac_opt(const char *inp, uint32_t *check_pac_flags) flags |= CHECK_PAC_CHECK_UPN_DNS_INFO_EX; flags |= CHECK_PAC_UPN_DNS_INFO_PRESENT; flags |= CHECK_PAC_CHECK_UPN; + } else if (strcasecmp(list[c], CHECK_PAC_CHECK_UPN_ALLOW_MISSING_STR) == 0) { + flags |= CHECK_PAC_CHECK_UPN_ALLOW_MISSING; } else { DEBUG(SSSDBG_OP_FAILURE, "Unknown value [%s] for pac_check.\n", list[c]); @@ -72,6 +74,14 @@ static errno_t check_check_pac_opt(const char *inp, uint32_t *check_pac_flags) } } + if ((flags & CHECK_PAC_CHECK_UPN_ALLOW_MISSING) + && !(flags & CHECK_PAC_CHECK_UPN)) { + DEBUG(SSSDBG_CONF_SETTINGS, + "pac_check option '%s' is set but '%s' is not set, this means " + "the UPN is not checked.\n", + CHECK_PAC_CHECK_UPN_ALLOW_MISSING_STR, CHECK_PAC_CHECK_UPN_STR); + } + ret = EOK; done: diff --git a/src/util/util.h b/src/util/util.h index 6d9111874..4b2651c2c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -818,6 +818,8 @@ uint64_t get_spend_time_us(uint64_t st); #define CHECK_PAC_CHECK_UPN_DNS_INFO_EX (1 << 3) #define CHECK_PAC_UPN_DNS_INFO_EX_PRESENT_STR "upn_dns_info_ex_present" #define CHECK_PAC_UPN_DNS_INFO_EX_PRESENT (1 << 4) +#define CHECK_PAC_CHECK_UPN_ALLOW_MISSING_STR "check_upn_allow_missing" +#define CHECK_PAC_CHECK_UPN_ALLOW_MISSING (1 << 5) errno_t get_pac_check_config(struct confdb_ctx *cdb, uint32_t *pac_check_opts); #endif /* __SSSD_UTIL_H__ */ -- 2.37.3