For a long time I felt constrained using just the error values that are available in system headers, they do not express the full range of errors we need to express within SSSD.
So this is a RFC patchset that demonstrates how we could about getting a richer set of error messages for SSSD that should also simplify some of our most convoluted code when it comes to returning errors.
The first patch just introduces a very simple error number space and 2 basic errors we always missed. ERR_INTERNAL is what we wanted to use in many places where we currently return EIO or EFAULT for example.
Patch 2 shows how we can dramatically improve error reporting for our offline caching authentication code with just a few more targeted errors. The main thing to note here is that now we can safely distinguish from a real system error about a missing file (ENOENT) and actually missing account.
Patch 3 is just an example of fixing EIO in a commonly used macro.
Patch 4 is the most complex example of converting a whole subsystem that is really complex in its error handling. Even after the recent refactoring patchesi, krb5_child error reporting remained quite complex as it uses all of errno, krb5 and pam status error codes.
The result is that we have to track multiple different error variables and that can easily lead to hard to read code as well as bugs where one error code does not reflect into others. With this patch I think we show how we are able to reduce the complexity of error handling and at the same time return precise information on what is worng.
NOTE: This patchset depends on my previous patches for refactoring the krb5_child as well as the patches that kix the krb5_auth style and the patches that introduce the authtoken structure.
Simo.
Simo Sorce (4): Add SSSD specific error codes and definitions Use SSSD specific errors for offline auth Return ERR_INTERNAL instead of EIO Cleanup error message handling for krb5 child
Makefile.am | 4 +- src/db/sysdb_ops.c | 17 +- src/providers/krb5/krb5_auth.c | 157 +++++++++------- src/providers/krb5/krb5_child.c | 388 ++++++++++++++++----------------------- src/tests/auth-tests.c | 6 +- src/tests/sysdb-tests.c | 12 +- src/util/auth_utils.h | 22 ++- src/util/util.h | 10 +- src/util/util_errors.c | 50 +++++ src/util/util_errors.h | 85 +++++++++ 10 files changed, 417 insertions(+), 334 deletions(-) create mode 100644 src/util/util_errors.c create mode 100644 src/util/util_errors.h
This code adds a new range of error codes specific to SSSD, It also provides helper functions to print out error defintions like you can do with system error messages and the strerror() function.
The sss_strerror() function can accept both the new sssd errors and system errno_t errors falling back to the system strerror() if the error code provide is not a valid SSSD error code. --- Makefile.am | 4 ++- src/util/util.h | 8 +---- src/util/util_errors.c | 41 ++++++++++++++++++++++++++ src/util/util_errors.h | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 8 deletions(-) create mode 100644 src/util/util_errors.c create mode 100644 src/util/util_errors.h
diff --git a/Makefile.am b/Makefile.am index 06f6280913e0419210852872663f0220455cc909..79088189bed685c50957ea167d1c28a88cb95406 100644 --- a/Makefile.am +++ b/Makefile.am @@ -354,6 +354,7 @@ dist_noinst_HEADERS = \ src/util/crypto/sss_crypto.h \ src/util/dlinklist.h \ src/util/util.h \ + src/util/util_errors.h \ src/util/strtonum.h \ src/util/sss_nss.h \ src/util/sss_ldap.h \ @@ -511,7 +512,8 @@ libsss_util_la_SOURCES = \ src/util/authtok.c \ src/util/sss_selinux.c \ src/util/domain_info_utils.c \ - src/util/util_lock.c + src/util/util_lock.c \ + src/util/util_errors.c libsss_util_la_LIBADD = \ $(SSSD_LIBS) \ $(UNICODE_LIBS) \ diff --git a/src/util/util.h b/src/util/util.h index 72b8a23b8668dce6dcd7d5b2b8001f64eb76859d..879569a97dc06987cbbc58014b3edeac07447595 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -44,11 +44,7 @@ #include <dhash.h>
#include "util/atomic_io.h" - -#ifndef HAVE_ERRNO_T -#define HAVE_ERRNO_T -typedef int errno_t; -#endif +#include "util/util_errors.h"
#define _(STRING) gettext (STRING)
@@ -218,8 +214,6 @@ errno_t set_debug_file_from_fd(const int fd);
#define ZERO_STRUCT(x) memset((char *)&(x), 0, sizeof(x))
-#define EOK 0 - #define SSSD_MAIN_OPTS SSSD_DEBUG_OPTS
#define FLAGS_NONE 0x0000 diff --git a/src/util/util_errors.c b/src/util/util_errors.c new file mode 100644 index 0000000000000000000000000000000000000000..92dced3c5f75f964adbaa1d886a07427c3f0a8c3 --- /dev/null +++ b/src/util/util_errors.c @@ -0,0 +1,41 @@ +/* + Copyright (C) 2012 Red Hat + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see http://www.gnu.org/licenses/. + + Authors: + Simo Sorce ssorce@redhat.com +*/ + +#include "util/util.h" + +struct err_string { + const char *msg; +}; + +struct err_string error_to_str[] = { + { "Invalid Error" }, /* ERR_INVALID */ + { "Internal Error" }, /* ERR_INTERNAL */ +}; + + +const char *sss_strerror(errno_t error) +{ + if (IS_SSSD_ERROR(error)) { + return error_to_str[SSSD_ERR_IDX(error)].msg; + } + + return strerror(error); +} + diff --git a/src/util/util_errors.h b/src/util/util_errors.h new file mode 100644 index 0000000000000000000000000000000000000000..eb0df77e6e8dbe2829cd59def3dee1d8abfce810 --- /dev/null +++ b/src/util/util_errors.h @@ -0,0 +1,75 @@ +/* + Copyright (C) 2012 Red Hat + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see http://www.gnu.org/licenses/. + + Authors: + Simo Sorce ssorce@redhat.com +*/ + +#ifndef __SSSD_UTIL_ERRORS_H__ +#define __SSSD_UTIL_ERRORS_H__ + +#ifndef HAVE_ERRNO_T +#define HAVE_ERRNO_T +typedef int errno_t; +#endif + +/* + * We define a specific number space so that we do not overlap with other + * generic errors returned by various libraries. This will make it easy + * to have functions that double check that what was returned was a SSSD + * specific error where it matters. For example we may want to ensure some + * particularly sensitive paths only return SSSD sepcific errors as that + * will insure all error conditions have been explicitly dealt with, + * and are not the result of assigning the wrong return result. + * + * Basic system errno errors can still be used, but when an error condition + * does not properly map to a system error we should use a SSSD specific one + */ + +#define ERR_BASE 0x555D0000 +#define ERR_MASK 0x0000FFFF + +/* never use ERR_INVALID, it is used for catching and returning + * information on invalid error numbers */ +/* never use ERR_LAST, this represent the maximum error value available + * and is used to validate error codes */ +enum sssd_errors { + ERR_INVALID = ERR_BASE + 0, + ERR_INTERNAL, + ERR_LAST /* ALWAYS LAST */ +}; + +#define SSSD_ERR_IDX(err) ((err) & ERR_MASK) +#define IS_SSSD_ERROR(err) \ + ((((err) & ERR_BASE) == ERR_BASE) && \ + SSSD_ERR_IDX(err) < ERR_LAST) + +#define ERR_OK 0 +/* Backwards compat */ +#ifndef EOK +#define EOK ERR_OK +#endif + +/** + * @brief return a string descriing the error number like strerror() + * + * @param error An errno_t number, can be a SSSD error or a system error + * + * @return A statically allocated string. + */ +const char *sss_strerror(errno_t error); + +#endif /* __SSSD_UTIL_ERRORS_H__ */
This prevents reportin false errors when internal functions return a generic EINVAL or EACCES that should just be treated as internal errors. --- src/db/sysdb_ops.c | 17 +++++++++-------- src/tests/auth-tests.c | 6 +++--- src/tests/sysdb-tests.c | 12 ++++++++---- src/util/auth_utils.h | 22 ++++++++++++---------- src/util/util_errors.c | 5 +++++ src/util/util_errors.h | 5 +++++ 6 files changed, 42 insertions(+), 25 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 741dd63c441dbaa0693a12fe12706d83eeecc163..d33d76bf638b39b16995987959b3718b157d0d5e 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -2622,7 +2622,7 @@ errno_t check_failed_login_attempts(struct confdb_ctx *cdb, if (ret != EOK) { DEBUG(1, ("Failed to read the number of allowed failed login " "attempts.\n")); - ret = EIO; + ret = ERR_INTERNAL; goto done; } ret = confdb_get_int(cdb, CONFDB_PAM_CONF_ENTRY, @@ -2631,7 +2631,7 @@ errno_t check_failed_login_attempts(struct confdb_ctx *cdb, &failed_login_delay); if (ret != EOK) { DEBUG(1, ("Failed to read the failed login delay.\n")); - ret = EIO; + ret = ERR_INTERNAL; goto done; } DEBUG(9, ("Failed login attempts [%d], allowed failed login attempts [%d], " @@ -2649,12 +2649,12 @@ errno_t check_failed_login_attempts(struct confdb_ctx *cdb, } else { DEBUG(7, ("login delayed until %lld.\n", (long long) end)); *delayed_until = end; - ret = EACCES; + ret = ERR_AUTH_DENIED; goto done; } } else { DEBUG(4, ("Too many failed logins.\n")); - ret = EACCES; + ret = ERR_AUTH_DENIED; goto done; } } @@ -2729,6 +2729,7 @@ int sysdb_cache_auth(struct sysdb_ctx *sysdb, if (ret != EOK) { DEBUG(1, ("sysdb_search_user_by_name failed [%d][%s].\n", ret, strerror(ret))); + if (ret == ENOENT) ret = ERR_ACCOUNT_UNKNOWN; goto done; }
@@ -2751,7 +2752,7 @@ int sysdb_cache_auth(struct sysdb_ctx *sysdb, if (expire_date < time(NULL)) { DEBUG(4, ("Cached user entry is too old.\n")); expire_date = 0; - ret = EACCES; + ret = ERR_CACHED_CREDS_EXPIRED; goto done; } } else { @@ -2770,14 +2771,14 @@ int sysdb_cache_auth(struct sysdb_ctx *sysdb, userhash = ldb_msg_find_attr_as_string(ldb_msg, SYSDB_CACHEDPWD, NULL); if (userhash == NULL || *userhash == '\0') { DEBUG(4, ("Cached credentials not available.\n")); - ret = ENOENT; + ret = ERR_NO_CACHED_CREDS; goto done; }
ret = s3crypt_sha512(tmp_ctx, password, userhash, &comphash); if (ret) { DEBUG(4, ("Failed to create password hash.\n")); - ret = EFAULT; + ret = ERR_INTERNAL; goto done; }
@@ -2863,7 +2864,7 @@ done: ret = EOK; } else { if (ret == EOK) { - ret = EINVAL; + ret = ERR_AUTH_FAILED; } } talloc_free(tmp_ctx); diff --git a/src/tests/auth-tests.c b/src/tests/auth-tests.c index ff8b9e1bd73c09bc16774af6b53715e8b9d2fcd3..2f96a1638bc544f571b663b6231ccbe004a03764 100644 --- a/src/tests/auth-tests.c +++ b/src/tests/auth-tests.c @@ -229,8 +229,8 @@ START_TEST(test_failed_login_attempts) * failed attempts >= offline_failed_login_attempts */ do_failed_login_test(0, 0, 2, 0, EOK, 0, -1); do_failed_login_test(0, time(NULL), 2, 0, EOK, 0, -1); - do_failed_login_test(2, 0, 2, 0, EACCES, 2, -1); - do_failed_login_test(2, time(NULL), 2, 0, EACCES, 2, -1); + do_failed_login_test(2, 0, 2, 0, ERR_AUTH_DENIED, 2, -1); + do_failed_login_test(2, time(NULL), 2, 0, ERR_AUTH_DENIED, 2, -1);
/* if offline_failed_login_attempts != 0 and * offline_failed_login_delay != 0 a login is denied only if the number of @@ -240,7 +240,7 @@ START_TEST(test_failed_login_attempts) do_failed_login_test(0, time(NULL), 2, 5, EOK, 0, -1); do_failed_login_test(2, 0, 2, 5, EOK, 0, -1); now = time(NULL); - do_failed_login_test(2, now, 2, 5, EACCES, 2, (now + 5 * 60)); + do_failed_login_test(2, now, 2, 5, ERR_AUTH_DENIED, 2, (now + 5 * 60));
} END_TEST diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c index ec82c8177719b1a04790a2f4a65a859f58be7945..251c8ddee91e4f39a4f44f9872aa8ebf08e00b3e 100644 --- a/src/tests/sysdb-tests.c +++ b/src/tests/sysdb-tests.c @@ -1579,8 +1579,10 @@ START_TEST (test_sysdb_cached_authentication_missing_password) username = talloc_asprintf(tmp_ctx, "testuser%d", _i); fail_unless(username != NULL, "talloc_asprintf failed.");
- cached_authentication_without_expiration(username, "abc", ENOENT); - cached_authentication_with_expiration(username, "abc", ENOENT); + cached_authentication_without_expiration(username, "abc", + ERR_NO_CACHED_CREDS); + cached_authentication_with_expiration(username, "abc", + ERR_NO_CACHED_CREDS);
talloc_free(tmp_ctx);
@@ -1598,8 +1600,10 @@ START_TEST (test_sysdb_cached_authentication_wrong_password) username = talloc_asprintf(tmp_ctx, "testuser%d", _i); fail_unless(username != NULL, "talloc_asprintf failed.");
- cached_authentication_without_expiration(username, "abc", EINVAL); - cached_authentication_with_expiration(username, "abc", EINVAL); + cached_authentication_without_expiration(username, "abc", + ERR_AUTH_FAILED); + cached_authentication_with_expiration(username, "abc", + ERR_AUTH_FAILED);
talloc_free(tmp_ctx);
diff --git a/src/util/auth_utils.h b/src/util/auth_utils.h index e9e60a085c386c1354732a816f8d982c5e6bf23f..8883c5ceb3691a89cc553c28bd6a76c5c611da0e 100644 --- a/src/util/auth_utils.h +++ b/src/util/auth_utils.h @@ -28,15 +28,17 @@ static inline int cached_login_pam_status(int auth_res) { switch (auth_res) { - case EOK: - return PAM_SUCCESS; - case ENOENT: - return PAM_AUTHINFO_UNAVAIL; - case EINVAL: - return PAM_AUTH_ERR; - case EACCES: - return PAM_PERM_DENIED; + case EOK: + return PAM_SUCCESS; + case ERR_ACCOUNT_UNKNOWN: + return PAM_AUTHINFO_UNAVAIL; + case ERR_NO_CACHED_CREDS: + case ERR_CACHED_CREDS_EXPIRED: + case ERR_AUTH_DENIED: + return PAM_PERM_DENIED; + case ERR_AUTH_FAILED: + return PAM_AUTH_ERR; + default: + return PAM_SYSTEM_ERR; } - - return PAM_SYSTEM_ERR; } diff --git a/src/util/util_errors.c b/src/util/util_errors.c index 92dced3c5f75f964adbaa1d886a07427c3f0a8c3..c196aae38a696df0333ed2de611202340066510e 100644 --- a/src/util/util_errors.c +++ b/src/util/util_errors.c @@ -27,6 +27,11 @@ struct err_string { struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ + { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ + { "No cached credentials available" }, /* ERR_NO_CACHED_CREDS */ + { "Cached credentials are expired" }, /* ERR_CACHED_CREDS_EXPIRED */ + { "Authentication Denied" }, /* ERR_AUTH_DENIED */ + { "Authentication Failed" }, /* ERR_AUTH_DENIED */ };
diff --git a/src/util/util_errors.h b/src/util/util_errors.h index eb0df77e6e8dbe2829cd59def3dee1d8abfce810..870d9d44bcb9a674e7208400ad649b7c761903de 100644 --- a/src/util/util_errors.h +++ b/src/util/util_errors.h @@ -49,6 +49,11 @@ typedef int errno_t; enum sssd_errors { ERR_INVALID = ERR_BASE + 0, ERR_INTERNAL, + ERR_ACCOUNT_UNKNOWN, + ERR_NO_CACHED_CREDS, + ERR_CACHED_CREDS_EXPIRED, + ERR_AUTH_DENIED, + ERR_AUTH_FAILED, ERR_LAST /* ALWAYS LAST */ };
EIO has always been an odd match, but was used as an error to indicate that something had gone wrong internally before we had specific SSSD errors available. Use ERR_INTERNAL instead going forward. --- src/util/util.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index 879569a97dc06987cbbc58014b3edeac07447595..f7c36bbd3bc8be50cb32d4c2c4c40d66b96ec9d9 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -264,7 +264,7 @@ errno_t set_debug_file_from_fd(const int fd); if (TRROEstate == TEVENT_REQ_USER_ERROR) { \ return TRROEerr; \ } \ - return EIO; \ + return ERR_INTERNAL; \ } \ } while (0)
Use the new internal SSSD errors, to simplify error handling. Instead of using up to 3 different error types (system, krb5 and pam_status), collapse all error reporting into one error type mapped on errno_t.
The returned error can contain either SSSD internal errors, kerberos errors or system errors, they all use different number spaces so there is no overlap and they can be safely merged.
This means that errors being sent from the child to the parent are not pam status error messages anymore. The callers have been changed to properly deal with that.
Also note that this patch removes returning SSS_PAM_SYSTEM_INFO from the krb5_child for kerberos errors as all it was doing was simply to make the parent emit the same debug log already emitted by the child, and the code is simpler if we do not do that. --- src/providers/krb5/krb5_auth.c | 157 +++++++++------- src/providers/krb5/krb5_child.c | 388 ++++++++++++++++----------------------- src/util/util_errors.c | 4 + src/util/util_errors.h | 5 + 4 files changed, 254 insertions(+), 300 deletions(-)
diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c index 87ec05f533996080c725430a5913d398daa0e594..a04797afb5f207856198dbb0bccb94c5629e2b19 100644 --- a/src/providers/krb5/krb5_auth.c +++ b/src/providers/krb5/krb5_auth.c @@ -919,22 +919,64 @@ static void krb5_auth_done(struct tevent_req *subreq)
/* If the child request failed, but did not return an offline error code, * return with the status */ - if (res->msg_status != PAM_SUCCESS && - res->msg_status != PAM_AUTHINFO_UNAVAIL && - res->msg_status != PAM_AUTHTOK_LOCK_BUSY && - res->msg_status != PAM_NEW_AUTHTOK_REQD) { - state->pam_status = res->msg_status; - state->dp_err = DP_ERR_OK; - ret = EOK; - goto done; - } else { - state->pam_status = res->msg_status; - } + switch (res->msg_status) { + case ERR_OK: + /* If the child request was successful and we run the first pass of the + * change password request just return success. */ + if (pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { + state->pam_status = PAM_SUCCESS; + state->dp_err = DP_ERR_OK; + ret = EOK; + goto done; + } + break;
- /* If the password is expired we can safely remove the ccache from the - * cache and disk if it is not actively used anymore. This will allow to - * create a new random ccache if sshd with privilege separation is used. */ - if (res->msg_status == PAM_NEW_AUTHTOK_REQD) { + case ERR_NETWORK_IO: + if (kr->kpasswd_srv != NULL && + (pd->cmd == SSS_PAM_CHAUTHTOK || + pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM)) { + /* if using a dedicated kpasswd server for a chpass operation... */ + + be_fo_set_port_status(state->be_ctx, + state->krb5_ctx->service->name, + kr->kpasswd_srv, PORT_NOT_WORKING); + /* ..try to resolve next kpasswd server */ + state->search_kpasswd = true; + subreq = be_resolve_server_send(state, state->ev, state->be_ctx, + state->krb5_ctx->kpasswd_service->name, + state->kr->kpasswd_srv == NULL ? true : false); + if (subreq == NULL) { + DEBUG(1, ("Resolver request failed.\n")); + ret = ENOMEM; + goto done; + } + tevent_req_set_callback(subreq, krb5_auth_resolve_done, req); + return; + } else if (kr->srv != NULL) { + /* failed to use the KDC... */ + be_fo_set_port_status(state->be_ctx, + state->krb5_ctx->service->name, + kr->srv, PORT_NOT_WORKING); + /* ..try to resolve next KDC */ + state->search_kpasswd = false; + subreq = be_resolve_server_send(state, state->ev, state->be_ctx, + state->krb5_ctx->service->name, + kr->srv == NULL ? true : false); + if (subreq == NULL) { + DEBUG(1, ("Resolver request failed.\n")); + ret = ENOMEM; + goto done; + } + tevent_req_set_callback(subreq, krb5_auth_resolve_done, req); + return; + } + break; + + case ERR_CREDS_EXPIRED: + /* If the password is expired we can safely remove the ccache from the + * cache and disk if it is not actively used anymore. This will allow + * to create a new random ccache if sshd with privilege separation is + * used. */ if (pd->cmd == SSS_PAM_AUTHENTICATE && !kr->active_ccache) { if (kr->old_ccname != NULL) { ret = safe_remove_old_ccache_file(kr->cc_be, kr->upn, @@ -952,70 +994,39 @@ static void krb5_auth_done(struct tevent_req *subreq) } }
- state->pam_status = res->msg_status; + state->pam_status = PAM_NEW_AUTHTOK_REQD; + state->dp_err = DP_ERR_OK; + ret = EOK; + goto done; + + case ERR_NO_CREDS: + state->pam_status = PAM_CRED_UNAVAIL; + state->dp_err = DP_ERR_OK; + ret = EOK; + goto done; + + case ERR_AUTH_FAILED: + state->pam_status = PAM_AUTH_ERR; state->dp_err = DP_ERR_OK; ret = EOK; goto done; - }
- /* If the child request was successful and we run the first pass of the - * change password request just return success. */ - if (res->msg_status == PAM_SUCCESS && pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { - state->pam_status = PAM_SUCCESS; + default: + state->pam_status = PAM_SYSTEM_ERR; state->dp_err = DP_ERR_OK; ret = EOK; goto done; }
- /* if using a dedicated kpasswd server for a chpass operation... */ if (kr->kpasswd_srv != NULL && - (pd->cmd == SSS_PAM_CHAUTHTOK || pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM)) { - /* ..which is unreachable by now.. */ - if (res->msg_status == PAM_AUTHTOK_LOCK_BUSY) { - be_fo_set_port_status(state->be_ctx, - state->krb5_ctx->service->name, - kr->kpasswd_srv, PORT_NOT_WORKING); - /* ..try to resolve next kpasswd server */ - state->search_kpasswd = true; - subreq = be_resolve_server_send(state, state->ev, state->be_ctx, - state->krb5_ctx->kpasswd_service->name, - state->kr->kpasswd_srv == NULL ? true : false); - if (subreq == NULL) { - DEBUG(1, ("Resolver request failed.\n")); - ret = ENOMEM; - goto done; - } - tevent_req_set_callback(subreq, krb5_auth_resolve_done, req); - return; - } else { - be_fo_set_port_status(state->be_ctx, - state->krb5_ctx->service->name, - kr->kpasswd_srv, PORT_WORKING); - } - } - - /* if the KDC for auth (PAM_AUTHINFO_UNAVAIL) or - * chpass (PAM_AUTHTOK_LOCK_BUSY) was not available while using KDC - * also for chpass operation... */ - if (res->msg_status == PAM_AUTHINFO_UNAVAIL || - (kr->kpasswd_srv == NULL && res->msg_status == PAM_AUTHTOK_LOCK_BUSY)) { - if (kr->srv != NULL) { - be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name, - kr->srv, PORT_NOT_WORKING); - /* ..try to resolve next KDC */ - state->search_kpasswd = false; - subreq = be_resolve_server_send(state, state->ev, state->be_ctx, - state->krb5_ctx->service->name, - kr->srv == NULL ? true : false); - if (subreq == NULL) { - DEBUG(1, ("Resolver request failed.\n")); - ret = ENOMEM; - goto done; - } - tevent_req_set_callback(subreq, krb5_auth_resolve_done, req); - return; - } + (pd->cmd == SSS_PAM_CHAUTHTOK || + pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM)) { + /* found a dedicated kpasswd server for a chpass operation */ + be_fo_set_port_status(state->be_ctx, + state->krb5_ctx->service->name, + kr->kpasswd_srv, PORT_WORKING); } else if (kr->srv != NULL) { + /* found a KDC */ be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name, kr->srv, PORT_WORKING); } @@ -1054,11 +1065,13 @@ static void krb5_auth_done(struct tevent_req *subreq) goto done; }
- if (res->msg_status == PAM_SUCCESS && - dp_opt_get_int(kr->krb5_ctx->opts, KRB5_RENEW_INTERVAL) > 0 && - (pd->cmd == SSS_PAM_AUTHENTICATE || pd->cmd == SSS_CMD_RENEW || + if (res->msg_status == ERR_OK && + (dp_opt_get_int(kr->krb5_ctx->opts, KRB5_RENEW_INTERVAL) > 0) && + (pd->cmd == SSS_PAM_AUTHENTICATE || + pd->cmd == SSS_CMD_RENEW || pd->cmd == SSS_PAM_CHAUTHTOK) && - res->tgtt.renew_till > res->tgtt.endtime && kr->ccname != NULL) { + (res->tgtt.renew_till > res->tgtt.endtime) && + (kr->ccname != NULL)) { DEBUG(7, ("Adding [%s] for automatic renewal.\n", kr->ccname)); ret = add_tgt_to_renew_table(kr->krb5_ctx, kr->ccname, &(res->tgtt), pd, kr->upn); diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index c53be4e67a5239eb20f6af986c2d3791b2fb3a6b..e56146f4c09b03b49a17d0d2aa7aa0b359db22d6 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -525,9 +525,11 @@ create_ccache(uid_t uid, gid_t gid, krb5_context ctx, return EINVAL; /* Should never get here */ }
-static errno_t pack_response_packet(struct response *resp, int status, - struct pam_data *pd) +static errno_t pack_response_packet(TALLOC_CTX *mem_ctx, errno_t error, + struct response_data *resp_list, + uint8_t **_buf, size_t *_len) { + uint8_t *buf; size_t size = 0; size_t p = 0; struct response_data *pdr; @@ -544,116 +546,69 @@ static errno_t pack_response_packet(struct response *resp, int status,
size = sizeof(int32_t);
- pdr = pd->resp_list; - while (pdr != NULL) { + for (pdr = resp_list; pdr != NULL; pdr = pdr->next) { size += 2*sizeof(int32_t) + pdr->len; - pdr = pdr->next; }
- resp->buf = talloc_array(resp, uint8_t, size); - if (!resp->buf) { + buf = talloc_array(mem_ctx, uint8_t, size); + if (!buf) { DEBUG(1, ("Insufficient memory to create message.\n")); return ENOMEM; }
- SAFEALIGN_SET_INT32(&resp->buf[p], status, &p); + SAFEALIGN_SET_INT32(&buf[p], error, &p);
- pdr = pd->resp_list; - while(pdr != NULL) { - SAFEALIGN_SET_INT32(&resp->buf[p], pdr->type, &p); - SAFEALIGN_SET_INT32(&resp->buf[p], pdr->len, &p); - safealign_memcpy(&resp->buf[p], pdr->data, pdr->len, &p); - - pdr = pdr->next; + for (pdr = resp_list; pdr != NULL; pdr = pdr->next) { + SAFEALIGN_SET_INT32(&buf[p], pdr->type, &p); + SAFEALIGN_SET_INT32(&buf[p], pdr->len, &p); + safealign_memcpy(&buf[p], pdr->data, pdr->len, &p); }
- resp->size = p; - DEBUG(SSSDBG_TRACE_INTERNAL, ("response packet size: [%d]\n", p));
+ *_buf = buf; + *_len = p; return EOK; }
-static struct response *prepare_response_message(struct krb5_req *kr, - krb5_error_code kerr, - int pam_status) +static errno_t k5c_attach_ccname_msg(struct krb5_req *kr) { char *msg = NULL; - const char *krb5_msg = NULL; int ret; - struct response *resp;
- resp = talloc_zero(kr, struct response); - if (resp == NULL) { - DEBUG(1, ("Initializing response failed.\n")); - return NULL; + if (kr->ccname == NULL) { + DEBUG(1, ("Error obtaining ccname.\n")); + return ERR_INTERNAL; }
- DEBUG(SSSDBG_TRACE_FUNC, ("Building response for result [%d]\n", kerr)); - - if (kerr == 0) { - if (kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { - pam_status = PAM_SUCCESS; - ret = EOK; - } else if (kr->pd->cmd == SSS_PAM_ACCT_MGMT) { - ret = EOK; - } else { - if (kr->ccname == NULL) { - DEBUG(1, ("Error obtaining ccname.\n")); - return NULL; - } - - msg = talloc_asprintf(kr, "%s=%s",CCACHE_ENV_NAME, kr->ccname); - if (msg == NULL) { - DEBUG(1, ("talloc_asprintf failed.\n")); - return NULL; - } - - pam_status = PAM_SUCCESS; - ret = pam_add_response(kr->pd, SSS_PAM_ENV_ITEM, strlen(msg) + 1, - (uint8_t *) msg); - talloc_zfree(msg); - } - } else { - krb5_msg = sss_krb5_get_error_message(krb5_error_ctx, kerr); - if (krb5_msg == NULL) { - DEBUG(1, ("sss_krb5_get_error_message failed.\n")); - return NULL; - } - - ret = pam_add_response(kr->pd, SSS_PAM_SYSTEM_INFO, - strlen(krb5_msg) + 1, - (const uint8_t *) krb5_msg); - sss_krb5_free_error_message(krb5_error_ctx, krb5_msg); - } - if (ret != EOK) { - DEBUG(1, ("pam_add_response failed.\n")); + msg = talloc_asprintf(kr, "%s=%s",CCACHE_ENV_NAME, kr->ccname); + if (msg == NULL) { + DEBUG(1, ("talloc_asprintf failed.\n")); + return ENOMEM; }
- ret = pack_response_packet(resp, pam_status, kr->pd); - if (ret != EOK) { - DEBUG(1, ("pack_response_packet failed.\n")); - return NULL; - } + ret = pam_add_response(kr->pd, SSS_PAM_ENV_ITEM, + strlen(msg) + 1, (uint8_t *)msg); + talloc_zfree(msg);
- return resp; + return ret; }
-static errno_t k5c_send_data(struct krb5_req *kr, int fd, - krb5_error_code kerr, int status) +static errno_t k5c_send_data(struct krb5_req *kr, int fd, errno_t error) { - struct response *resp; size_t written; + uint8_t *buf; + size_t len; int ret;
- resp = prepare_response_message(kr, kerr, status); - if (resp == NULL) { - DEBUG(1, ("prepare_response_message failed.\n")); - return ENOMEM; + ret = pack_response_packet(kr, error, kr->pd->resp_list, &buf, &len); + if (ret != EOK) { + DEBUG(1, ("pack_response_packet failed.\n")); + return ret; }
errno = 0; - written = sss_atomic_write_s(fd, resp->buf, resp->size); + written = sss_atomic_write_s(fd, buf, len); if (written == -1) { ret = errno; DEBUG(SSSDBG_CRIT_FAILURE, @@ -661,10 +616,10 @@ static errno_t k5c_send_data(struct krb5_req *kr, int fd, return ret; }
- if (written != resp->size) { + if (written != len) { DEBUG(SSSDBG_CRIT_FAILURE, ("Write error, wrote [%d] bytes, expected [%d]\n", - written, resp->size)); + written, len)); return EOK; }
@@ -915,10 +870,9 @@ done: static krb5_error_code get_and_save_tgt(struct krb5_req *kr, const char *password) { - krb5_error_code kerr = 0; - int ret; const char *realm_name; int realm_length; + krb5_error_code kerr;
kerr = sss_krb5_get_init_creds_opt_set_expire_callback(kr->ctx, kr->options, @@ -957,10 +911,10 @@ static krb5_error_code get_and_save_tgt(struct krb5_req *kr, /* We drop root privileges which were needed to read the keytab file * for the validation of the credentials or for FAST here to run the * ccache I/O operations with user privileges. */ - ret = become_user(kr->uid, kr->gid); - if (ret != EOK) { + kerr = become_user(kr->uid, kr->gid); + if (kerr != 0) { DEBUG(1, ("become_user failed.\n")); - return ret; + return kerr; } }
@@ -973,8 +927,8 @@ static krb5_error_code get_and_save_tgt(struct krb5_req *kr, goto done; }
- ret = add_ticket_times_and_upn_to_response(kr); - if (ret != EOK) { + kerr = add_ticket_times_and_upn_to_response(kr); + if (kerr != 0) { DEBUG(1, ("add_ticket_times_and_upn_to_response failed.\n")); }
@@ -987,59 +941,46 @@ done:
}
-static int kerr_handle_error(krb5_error_code kerr) +static errno_t map_krb5_error(krb5_error_code kerr) { - int pam_status; - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + switch (kerr) { - case KRB5_LIBOS_CANTREADPWD: - pam_status = PAM_CRED_UNAVAIL; - break; - case KRB5_KDC_UNREACH: - pam_status = PAM_AUTHINFO_UNAVAIL; - break; - case KRB5KDC_ERR_KEY_EXP: - pam_status = PAM_NEW_AUTHTOK_REQD; - break; - case KRB5KRB_AP_ERR_BAD_INTEGRITY: - pam_status = PAM_AUTH_ERR; - break; - case KRB5_PREAUTH_FAILED: - case KRB5KDC_ERR_PREAUTH_FAILED: - pam_status = PAM_CRED_ERR; - break; - default: - pam_status = PAM_SYSTEM_ERR; - break; - } + case 0: + return ERR_OK;
- return pam_status; -} + case KRB5_LIBOS_CANTREADPWD: + return ERR_NO_CREDS;
-static int kerr_to_status(krb5_error_code kerr) -{ - if (kerr == 0) { - return PAM_SUCCESS; - } + case KRB5_KDC_UNREACH: + return ERR_NETWORK_IO;
- return kerr_handle_error(kerr); + case KRB5KDC_ERR_KEY_EXP: + return ERR_CREDS_EXPIRED; + + case KRB5KRB_AP_ERR_BAD_INTEGRITY: + case KRB5_PREAUTH_FAILED: + case KRB5KDC_ERR_PREAUTH_FAILED: + return ERR_AUTH_FAILED; + + default: + return ERR_INTERNAL; + } }
-static errno_t changepw_child(struct krb5_req *kr, int *_status) +static errno_t changepw_child(struct krb5_req *kr, bool prelim) { int ret; krb5_error_code kerr = 0; const char *password = NULL; const char *newpassword = NULL; - int pam_status = PAM_SYSTEM_ERR; int result_code = -1; krb5_data result_code_string; krb5_data result_string; char *user_error_message = NULL; size_t user_resp_len; uint8_t *user_resp; - krb5_prompter_fct prompter = sss_krb5_prompter; + krb5_prompter_fct prompter = NULL; const char *realm_name; int realm_length; krb5_get_init_creds_opt *chagepw_options; @@ -1050,20 +991,18 @@ static errno_t changepw_child(struct krb5_req *kr, int *_status) if (ret != EOK) { DEBUG(1, ("Failed to fetch current password [%d] %s.\n", ret, strerror(ret))); - pam_status = PAM_CRED_INSUFFICIENT; - kerr = KRB5KRB_ERR_GENERIC; - goto done; + return ERR_NO_CREDS; }
- if (kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { + if (!prelim) { /* We do not need a password expiration warning here. */ - prompter = NULL; + prompter = sss_krb5_prompter; }
kerr = get_changepw_options(kr->ctx, &chagepw_options); if (kerr != 0) { DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); - goto done; + return kerr; }
sss_krb5_princ_realm(kr->ctx, kr->princ, &realm_name, &realm_length); @@ -1077,27 +1016,24 @@ static errno_t changepw_child(struct krb5_req *kr, int *_status) chagepw_options); sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options); if (kerr != 0) { - pam_status = kerr_handle_error(kerr); - goto done; + return kerr; }
sss_authtok_set_empty(&kr->pd->authtok);
- if (kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { + if (prelim) { DEBUG(SSSDBG_TRACE_LIBS, ("Initial authentication for change password operation " "successful.\n")); krb5_free_cred_contents(kr->ctx, kr->creds); - pam_status = PAM_SUCCESS; - goto done; + return EOK; }
ret = sss_authtok_get_password(&kr->pd->newauthtok, &newpassword, NULL); if (ret != EOK) { DEBUG(1, ("Failed to fetch new password [%d] %s.\n", ret, strerror(ret))); - kerr = KRB5KRB_ERR_GENERIC; - goto done; + return ERR_NO_CREDS; }
memset(&result_code_string, 0, sizeof(krb5_data)); @@ -1107,15 +1043,12 @@ static errno_t changepw_child(struct krb5_req *kr, int *_status) &result_code_string, &result_string);
if (kerr == KRB5_KDC_UNREACH) { - pam_status = PAM_AUTHTOK_LOCK_BUSY; - goto done; + return ERR_NETWORK_IO; }
if (kerr != 0 || result_code != 0) { if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - } else { - kerr = KRB5KRB_ERR_GENERIC; }
if (result_code_string.length > 0) { @@ -1153,8 +1086,7 @@ static errno_t changepw_child(struct krb5_req *kr, int *_status) } }
- pam_status = PAM_AUTHTOK_ERR; - goto done; + return ERR_CHPASS_FAILED; }
krb5_free_cred_contents(kr->ctx, kr->creds); @@ -1163,88 +1095,91 @@ static errno_t changepw_child(struct krb5_req *kr, int *_status)
sss_authtok_set_empty(&kr->pd->newauthtok);
- pam_status = kerr_to_status(kerr); - -done: - *_status = pam_status; - return ret; + if (kerr == 0) { + kerr = k5c_attach_ccname_msg(kr); + } + return map_krb5_error(kerr); }
-static errno_t tgt_req_child(struct krb5_req *kr, int *_status) +static errno_t tgt_req_child(struct krb5_req *kr) { - int ret; - krb5_error_code kerr = 0; - const char *password = NULL; - int pam_status = PAM_SYSTEM_ERR; krb5_get_init_creds_opt *chagepw_options; + const char *password = NULL; + krb5_error_code kerr; + char *pass_str; + int ret;
DEBUG(SSSDBG_TRACE_LIBS, ("Attempting to get a TGT\n"));
ret = sss_authtok_get_password(&kr->pd->authtok, &password, NULL); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, ("Unknown authtok type\n")); - pam_status = PAM_CRED_INSUFFICIENT; - kerr = KRB5KRB_ERR_GENERIC; - goto done; + switch (ret) { + if (ret == EACCES) { + DEBUG(SSSDBG_OP_FAILURE, ("Invalid authtok type\n")); + return ERR_INVALID_CRED_TYPE; + } + DEBUG(SSSDBG_OP_FAILURE, ("No credentials available\n")); + return ERR_NO_CREDS; }
kerr = get_and_save_tgt(kr, password);
+ if (kerr != KRB5KDC_ERR_KEY_EXP) { + if (kerr == 0) { + kerr = k5c_attach_ccname_msg(kr); + } + ret = map_krb5_error(kerr); + goto done; + } + /* If the password is expired the KDC will always return KRB5KDC_ERR_KEY_EXP regardless if the supplied password is correct or not. In general the password can still be used to get a changepw ticket. So we validate the password by trying to get a changepw ticket. */ - if (kerr == KRB5KDC_ERR_KEY_EXP) { - DEBUG(SSSDBG_TRACE_LIBS, ("Password was expired\n")); - kerr = sss_krb5_get_init_creds_opt_set_expire_callback(kr->ctx, - kr->options, - NULL, NULL); - if (kerr != 0) { - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - DEBUG(1, ("Failed to unset expire callback, continue ...\n")); - } + DEBUG(SSSDBG_TRACE_LIBS, ("Password was expired\n")); + kerr = sss_krb5_get_init_creds_opt_set_expire_callback(kr->ctx, + kr->options, + NULL, NULL); + if (kerr != 0) { + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + DEBUG(1, ("Failed to unset expire callback, continue ...\n")); + }
- kerr = get_changepw_options(kr->ctx, &chagepw_options); - if (kerr != 0) { - DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); - goto done; - } + kerr = get_changepw_options(kr->ctx, &chagepw_options); + if (kerr != 0) { + DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); + return kerr; + }
- kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ, - discard_const(password), - sss_krb5_prompter, kr, 0, - SSSD_KRB5_CHANGEPW_PRINCIPAL, - chagepw_options); + kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ, + discard_const(password), + sss_krb5_prompter, kr, 0, + SSSD_KRB5_CHANGEPW_PRINCIPAL, + chagepw_options);
- sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options); + sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options);
- krb5_free_cred_contents(kr->ctx, kr->creds); - if (kerr == 0) { - kerr = KRB5KDC_ERR_KEY_EXP; - } + krb5_free_cred_contents(kr->ctx, kr->creds); + if (kerr == 0) { + ret = ERR_CREDS_EXPIRED; + } else { + ret = map_krb5_error(kerr); }
- sss_authtok_set_empty(&kr->pd->authtok); - - pam_status = kerr_to_status(kerr); - done: - *_status = pam_status; + sss_authtok_set_empty(&kr->pd->authtok); return ret; }
-static errno_t kuserok_child(struct krb5_req *kr, int *_status) +static errno_t kuserok_child(struct krb5_req *kr) { krb5_boolean access_allowed; - int ret; krb5_error_code kerr;
DEBUG(SSSDBG_TRACE_LIBS, ("Verifying if principal can log in as user\n"));
/* krb5_kuserok tries to verify that kr->pd->user is a locally known * account, so we have to unset _SSS_LOOPS to make getpwnam() work. */ - ret = unsetenv("_SSS_LOOPS"); - if (ret != EOK) { + if (unsetenv("_SSS_LOOPS") != 0) { DEBUG(1, ("Failed to unset _SSS_LOOPS, " "krb5_kuserok will most certainly fail.\n")); } @@ -1259,17 +1194,19 @@ static errno_t kuserok_child(struct krb5_req *kr, int *_status) DEBUG(SSSDBG_TRACE_LIBS, ("Access was %s\n", access_allowed ? "allowed" : "denied"));
- *_status = access_allowed ? 0 : 1; - return ret; + if (access_allowed) { + return EOK; + } + + return ERR_AUTH_DENIED; }
-static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) +static errno_t renew_tgt_child(struct krb5_req *kr) { - int ret; - int status = PAM_AUTHTOK_ERR; - int kerr; const char *ccname; krb5_ccache ccache = NULL; + krb5_error_code kerr; + int ret;
DEBUG(SSSDBG_TRACE_LIBS, ("Renewing a ticket\n"));
@@ -1278,8 +1215,7 @@ static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) DEBUG(SSSDBG_OP_FAILURE, ("Unsupported authtok type for TGT renewal [%d].\n", sss_authtok_get_type(&kr->pd->authtok))); - kerr = EINVAL; - goto done; + return ERR_INVALID_CRED_TYPE; }
kerr = krb5_cc_resolve(kr->ctx, ccname, &ccache); @@ -1290,7 +1226,6 @@ static errno_t renew_tgt_child(struct krb5_req *kr, int *_status)
kerr = krb5_get_renewed_creds(kr->ctx, kr->creds, kr->princ, ccache, NULL); if (kerr != 0) { - status = kerr_handle_error(kerr); goto done; }
@@ -1309,10 +1244,9 @@ static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) /* We drop root privileges which were needed to read the keytab file * for the validation of the credentials or for FAST here to run the * ccache I/O operations with user privileges. */ - ret = become_user(kr->uid, kr->gid); - if (ret != EOK) { + kerr = become_user(kr->uid, kr->gid); + if (kerr != 0) { DEBUG(1, ("become_user failed.\n")); - kerr = ret; goto done; } } @@ -1329,13 +1263,12 @@ static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) goto done; }
- ret = add_ticket_times_and_upn_to_response(kr); - if (ret != EOK) { + kerr = add_ticket_times_and_upn_to_response(kr); + if (kerr != 0) { DEBUG(1, ("add_ticket_times_and_upn_to_response failed.\n")); }
- status = PAM_SUCCESS; - kerr = 0; + kerr = k5c_attach_ccname_msg(kr);
done: krb5_free_cred_contents(kr->ctx, kr->creds); @@ -1344,26 +1277,24 @@ done: krb5_cc_close(kr->ctx, ccache); }
- *_status = status; - return ret; + return map_krb5_error(kerr); }
-static errno_t create_empty_ccache(struct krb5_req *kr, int *_status) +static errno_t create_empty_ccache(struct krb5_req *kr) { - int ret; - int pam_status = PAM_SUCCESS; + krb5_error_code kerr;
DEBUG(SSSDBG_TRACE_LIBS, ("Creating empty ccache\n"));
- ret = create_ccache(kr->uid, kr->gid, kr->ctx, - kr->princ, kr->ccname, NULL); - if (ret != 0) { - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, ret); - pam_status = PAM_SYSTEM_ERR; + kerr = create_ccache(kr->uid, kr->gid, kr->ctx, + kr->princ, kr->ccname, NULL); + if (kerr != 0) { + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + } else { + kerr = k5c_attach_ccname_msg(kr); }
- *_status = pam_status; - return ret; + return map_krb5_error(kerr); }
static errno_t unpack_authtok(TALLOC_CTX *mem_ctx, struct sss_auth_token *tok, @@ -1898,7 +1829,6 @@ int main(int argc, const char *argv[]) int opt; poptContext pc; int debug_fd = -1; - int status; int ret;
struct poptOption long_options[] = { @@ -1971,30 +1901,32 @@ int main(int argc, const char *argv[]) /* If we are offline, we need to create an empty ccache file */ if (offline) { DEBUG(SSSDBG_TRACE_FUNC, ("Will perform offline auth\n")); - ret = create_empty_ccache(kr, &status); + ret = create_empty_ccache(kr); } else { DEBUG(SSSDBG_TRACE_FUNC, ("Will perform online auth\n")); - ret = tgt_req_child(kr, &status); + ret = tgt_req_child(kr); } break; case SSS_PAM_CHAUTHTOK: - case SSS_PAM_CHAUTHTOK_PRELIM: DEBUG(SSSDBG_TRACE_FUNC, ("Will perform password change\n")); - ret = changepw_child(kr, &status); + ret = changepw_child(kr, false); + break; + case SSS_PAM_CHAUTHTOK_PRELIM: + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform password change checks\n")); + ret = changepw_child(kr, true); break; case SSS_PAM_ACCT_MGMT: DEBUG(SSSDBG_TRACE_FUNC, ("Will perform account management\n")); - ret = kuserok_child(kr, &status); + ret = kuserok_child(kr); break; case SSS_CMD_RENEW: - if (!offline) { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform ticket renewal\n")); - ret = renew_tgt_child(kr, &status); - } else { + if (offline) { DEBUG(SSSDBG_CRIT_FAILURE, ("Cannot renew TGT while offline\n")); ret = KRB5_KDC_UNREACH; goto done; } + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform ticket renewal\n")); + ret = renew_tgt_child(kr); break; default: DEBUG(1, ("PAM command [%d] not supported.\n", kr->pd->cmd)); @@ -2002,7 +1934,7 @@ int main(int argc, const char *argv[]) goto done; }
- ret = k5c_send_data(kr, STDOUT_FILENO, ret, status); + ret = k5c_send_data(kr, STDOUT_FILENO, ret); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to send reply\n")); } diff --git a/src/util/util_errors.c b/src/util/util_errors.c index c196aae38a696df0333ed2de611202340066510e..5020fe97aede908aea300daceb872c655d3645ab 100644 --- a/src/util/util_errors.c +++ b/src/util/util_errors.c @@ -28,10 +28,14 @@ struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ + { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ + { "No credentials available" }, /* ERR_NO_CREDS */ + { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */ { "No cached credentials available" }, /* ERR_NO_CACHED_CREDS */ { "Cached credentials are expired" }, /* ERR_CACHED_CREDS_EXPIRED */ { "Authentication Denied" }, /* ERR_AUTH_DENIED */ { "Authentication Failed" }, /* ERR_AUTH_DENIED */ + { "Network I/O Error" }, /* ERR_NETWORK_IO */ };
diff --git a/src/util/util_errors.h b/src/util/util_errors.h index 870d9d44bcb9a674e7208400ad649b7c761903de..9292c9959bed1d1772ca761c04f6db417c6286c1 100644 --- a/src/util/util_errors.h +++ b/src/util/util_errors.h @@ -50,10 +50,15 @@ enum sssd_errors { ERR_INVALID = ERR_BASE + 0, ERR_INTERNAL, ERR_ACCOUNT_UNKNOWN, + ERR_INVALID_CRED_TYPE, + ERR_NO_CREDS, + ERR_CREDS_EXPIRED, ERR_NO_CACHED_CREDS, ERR_CACHED_CREDS_EXPIRED, ERR_AUTH_DENIED, ERR_AUTH_FAILED, + ERR_CHPASS_FAILED, + ERR_NETWORK_IO, ERR_LAST /* ALWAYS LAST */ };
On 11/24/2012 05:58 AM, Simo Sorce wrote:
For a long time I felt constrained using just the error values that are available in system headers, they do not express the full range of errors we need to express within SSSD.
So this is a RFC patchset that demonstrates how we could about getting a richer set of error messages for SSSD that should also simplify some of our most convoluted code when it comes to returning errors.
The first patch just introduces a very simple error number space and 2 basic errors we always missed. ERR_INTERNAL is what we wanted to use in many places where we currently return EIO or EFAULT for example.
Patch 2 shows how we can dramatically improve error reporting for our offline caching authentication code with just a few more targeted errors. The main thing to note here is that now we can safely distinguish from a real system error about a missing file (ENOENT) and actually missing account.
Patch 3 is just an example of fixing EIO in a commonly used macro.
Patch 4 is the most complex example of converting a whole subsystem that is really complex in its error handling. Even after the recent refactoring patchesi, krb5_child error reporting remained quite complex as it uses all of errno, krb5 and pam status error codes.
The result is that we have to track multiple different error variables and that can easily lead to hard to read code as well as bugs where one error code does not reflect into others. With this patch I think we show how we are able to reduce the complexity of error handling and at the same time return precise information on what is worng.
NOTE: This patchset depends on my previous patches for refactoring the krb5_child as well as the patches that kix the krb5_auth style and the patches that introduce the authtoken structure.
Simo.
Simo Sorce (4): Add SSSD specific error codes and definitions Use SSSD specific errors for offline auth Return ERR_INTERNAL instead of EIO Cleanup error message handling for krb5 child
Makefile.am | 4 +- src/db/sysdb_ops.c | 17 +- src/providers/krb5/krb5_auth.c | 157 +++++++++------- src/providers/krb5/krb5_child.c | 388 ++++++++++++++++----------------------- src/tests/auth-tests.c | 6 +- src/tests/sysdb-tests.c | 12 +- src/util/auth_utils.h | 22 ++- src/util/util.h | 10 +- src/util/util_errors.c | 50 +++++ src/util/util_errors.h | 85 +++++++++ 10 files changed, 417 insertions(+), 334 deletions(-) create mode 100644 src/util/util_errors.c create mode 100644 src/util/util_errors.h
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi, I like this idea. The patch set looks good from the first sight but doesn't apply to current master.
Rebased on master, this patchset depends on the 2 patch krb-child refactor patchset.
Simo.
Simo Sorce (4): Add SSSD specific error codes and definitions Use SSSD specific errors for offline auth Return ERR_INTERNAL instead of EIO Cleanup error message handling for krb5 child
Makefile.am | 4 +- src/db/sysdb_ops.c | 17 +- src/providers/krb5/krb5_auth.c | 157 +++++++++------- src/providers/krb5/krb5_child.c | 388 ++++++++++++++++----------------------- src/tests/auth-tests.c | 6 +- src/tests/sysdb-tests.c | 12 +- src/util/auth_utils.h | 22 ++- src/util/util.h | 10 +- src/util/util_errors.c | 51 +++++ src/util/util_errors.h | 85 +++++++++ 10 files changed, 418 insertions(+), 334 deletions(-) create mode 100644 src/util/util_errors.c create mode 100644 src/util/util_errors.h
This code adds a new range of error codes specific to SSSD, It also provides helper functions to print out error defintions like you can do with system error messages and the strerror() function.
The sss_strerror() function can accept both the new sssd errors and system errno_t errors falling back to the system strerror() if the error code provide is not a valid SSSD error code. --- Makefile.am | 4 ++- src/util/util.h | 8 +---- src/util/util_errors.c | 41 ++++++++++++++++++++++++++ src/util/util_errors.h | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 8 deletions(-) create mode 100644 src/util/util_errors.c create mode 100644 src/util/util_errors.h
diff --git a/Makefile.am b/Makefile.am index 98d8bba3f8cd9cf031332e9371ab36023d721347..e738dd4f2538e552ca873cc163154c08abcb1351 100644 --- a/Makefile.am +++ b/Makefile.am @@ -354,6 +354,7 @@ dist_noinst_HEADERS = \ src/util/crypto/sss_crypto.h \ src/util/dlinklist.h \ src/util/util.h \ + src/util/util_errors.h \ src/util/strtonum.h \ src/util/sss_nss.h \ src/util/sss_ldap.h \ @@ -512,7 +513,8 @@ libsss_util_la_SOURCES = \ src/util/authtok.c \ src/util/sss_selinux.c \ src/util/domain_info_utils.c \ - src/util/util_lock.c + src/util/util_lock.c \ + src/util/util_errors.c libsss_util_la_LIBADD = \ $(SSSD_LIBS) \ $(UNICODE_LIBS) \ diff --git a/src/util/util.h b/src/util/util.h index cc5a2bafbd74893e0a04b437ec8f95da837d5d40..61e5cb0e20a9f690ad26940992cefe17d20484c5 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -44,11 +44,7 @@ #include <dhash.h>
#include "util/atomic_io.h" - -#ifndef HAVE_ERRNO_T -#define HAVE_ERRNO_T -typedef int errno_t; -#endif +#include "util/util_errors.h"
#define _(STRING) gettext (STRING)
@@ -221,8 +217,6 @@ errno_t set_debug_file_from_fd(const int fd);
#define ZERO_STRUCT(x) memset((char *)&(x), 0, sizeof(x))
-#define EOK 0 - #define SSSD_MAIN_OPTS SSSD_DEBUG_OPTS
#define FLAGS_NONE 0x0000 diff --git a/src/util/util_errors.c b/src/util/util_errors.c new file mode 100644 index 0000000000000000000000000000000000000000..92dced3c5f75f964adbaa1d886a07427c3f0a8c3 --- /dev/null +++ b/src/util/util_errors.c @@ -0,0 +1,41 @@ +/* + Copyright (C) 2012 Red Hat + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see http://www.gnu.org/licenses/. + + Authors: + Simo Sorce ssorce@redhat.com +*/ + +#include "util/util.h" + +struct err_string { + const char *msg; +}; + +struct err_string error_to_str[] = { + { "Invalid Error" }, /* ERR_INVALID */ + { "Internal Error" }, /* ERR_INTERNAL */ +}; + + +const char *sss_strerror(errno_t error) +{ + if (IS_SSSD_ERROR(error)) { + return error_to_str[SSSD_ERR_IDX(error)].msg; + } + + return strerror(error); +} + diff --git a/src/util/util_errors.h b/src/util/util_errors.h new file mode 100644 index 0000000000000000000000000000000000000000..eb0df77e6e8dbe2829cd59def3dee1d8abfce810 --- /dev/null +++ b/src/util/util_errors.h @@ -0,0 +1,75 @@ +/* + Copyright (C) 2012 Red Hat + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see http://www.gnu.org/licenses/. + + Authors: + Simo Sorce ssorce@redhat.com +*/ + +#ifndef __SSSD_UTIL_ERRORS_H__ +#define __SSSD_UTIL_ERRORS_H__ + +#ifndef HAVE_ERRNO_T +#define HAVE_ERRNO_T +typedef int errno_t; +#endif + +/* + * We define a specific number space so that we do not overlap with other + * generic errors returned by various libraries. This will make it easy + * to have functions that double check that what was returned was a SSSD + * specific error where it matters. For example we may want to ensure some + * particularly sensitive paths only return SSSD sepcific errors as that + * will insure all error conditions have been explicitly dealt with, + * and are not the result of assigning the wrong return result. + * + * Basic system errno errors can still be used, but when an error condition + * does not properly map to a system error we should use a SSSD specific one + */ + +#define ERR_BASE 0x555D0000 +#define ERR_MASK 0x0000FFFF + +/* never use ERR_INVALID, it is used for catching and returning + * information on invalid error numbers */ +/* never use ERR_LAST, this represent the maximum error value available + * and is used to validate error codes */ +enum sssd_errors { + ERR_INVALID = ERR_BASE + 0, + ERR_INTERNAL, + ERR_LAST /* ALWAYS LAST */ +}; + +#define SSSD_ERR_IDX(err) ((err) & ERR_MASK) +#define IS_SSSD_ERROR(err) \ + ((((err) & ERR_BASE) == ERR_BASE) && \ + SSSD_ERR_IDX(err) < ERR_LAST) + +#define ERR_OK 0 +/* Backwards compat */ +#ifndef EOK +#define EOK ERR_OK +#endif + +/** + * @brief return a string descriing the error number like strerror() + * + * @param error An errno_t number, can be a SSSD error or a system error + * + * @return A statically allocated string. + */ +const char *sss_strerror(errno_t error); + +#endif /* __SSSD_UTIL_ERRORS_H__ */
This prevents reportin false errors when internal functions return a generic EINVAL or EACCES that should just be treated as internal errors. --- src/db/sysdb_ops.c | 17 +++++++++-------- src/tests/auth-tests.c | 6 +++--- src/tests/sysdb-tests.c | 12 ++++++++---- src/util/auth_utils.h | 22 ++++++++++++---------- src/util/util_errors.c | 5 +++++ src/util/util_errors.h | 5 +++++ 6 files changed, 42 insertions(+), 25 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 9974beb05ebfe6fdd295e7e6e07015fd26b631c2..8aacb816acdbbf24c17ed2191b1f008f5d462de2 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -2711,7 +2711,7 @@ errno_t check_failed_login_attempts(struct confdb_ctx *cdb, if (ret != EOK) { DEBUG(1, ("Failed to read the number of allowed failed login " "attempts.\n")); - ret = EIO; + ret = ERR_INTERNAL; goto done; } ret = confdb_get_int(cdb, CONFDB_PAM_CONF_ENTRY, @@ -2720,7 +2720,7 @@ errno_t check_failed_login_attempts(struct confdb_ctx *cdb, &failed_login_delay); if (ret != EOK) { DEBUG(1, ("Failed to read the failed login delay.\n")); - ret = EIO; + ret = ERR_INTERNAL; goto done; } DEBUG(9, ("Failed login attempts [%d], allowed failed login attempts [%d], " @@ -2738,12 +2738,12 @@ errno_t check_failed_login_attempts(struct confdb_ctx *cdb, } else { DEBUG(7, ("login delayed until %lld.\n", (long long) end)); *delayed_until = end; - ret = EACCES; + ret = ERR_AUTH_DENIED; goto done; } } else { DEBUG(4, ("Too many failed logins.\n")); - ret = EACCES; + ret = ERR_AUTH_DENIED; goto done; } } @@ -2818,6 +2818,7 @@ int sysdb_cache_auth(struct sysdb_ctx *sysdb, if (ret != EOK) { DEBUG(1, ("sysdb_search_user_by_name failed [%d][%s].\n", ret, strerror(ret))); + if (ret == ENOENT) ret = ERR_ACCOUNT_UNKNOWN; goto done; }
@@ -2840,7 +2841,7 @@ int sysdb_cache_auth(struct sysdb_ctx *sysdb, if (expire_date < time(NULL)) { DEBUG(4, ("Cached user entry is too old.\n")); expire_date = 0; - ret = EACCES; + ret = ERR_CACHED_CREDS_EXPIRED; goto done; } } else { @@ -2859,14 +2860,14 @@ int sysdb_cache_auth(struct sysdb_ctx *sysdb, userhash = ldb_msg_find_attr_as_string(ldb_msg, SYSDB_CACHEDPWD, NULL); if (userhash == NULL || *userhash == '\0') { DEBUG(4, ("Cached credentials not available.\n")); - ret = ENOENT; + ret = ERR_NO_CACHED_CREDS; goto done; }
ret = s3crypt_sha512(tmp_ctx, password, userhash, &comphash); if (ret) { DEBUG(4, ("Failed to create password hash.\n")); - ret = EFAULT; + ret = ERR_INTERNAL; goto done; }
@@ -2952,7 +2953,7 @@ done: ret = EOK; } else { if (ret == EOK) { - ret = EINVAL; + ret = ERR_AUTH_FAILED; } } talloc_free(tmp_ctx); diff --git a/src/tests/auth-tests.c b/src/tests/auth-tests.c index ff8b9e1bd73c09bc16774af6b53715e8b9d2fcd3..2f96a1638bc544f571b663b6231ccbe004a03764 100644 --- a/src/tests/auth-tests.c +++ b/src/tests/auth-tests.c @@ -229,8 +229,8 @@ START_TEST(test_failed_login_attempts) * failed attempts >= offline_failed_login_attempts */ do_failed_login_test(0, 0, 2, 0, EOK, 0, -1); do_failed_login_test(0, time(NULL), 2, 0, EOK, 0, -1); - do_failed_login_test(2, 0, 2, 0, EACCES, 2, -1); - do_failed_login_test(2, time(NULL), 2, 0, EACCES, 2, -1); + do_failed_login_test(2, 0, 2, 0, ERR_AUTH_DENIED, 2, -1); + do_failed_login_test(2, time(NULL), 2, 0, ERR_AUTH_DENIED, 2, -1);
/* if offline_failed_login_attempts != 0 and * offline_failed_login_delay != 0 a login is denied only if the number of @@ -240,7 +240,7 @@ START_TEST(test_failed_login_attempts) do_failed_login_test(0, time(NULL), 2, 5, EOK, 0, -1); do_failed_login_test(2, 0, 2, 5, EOK, 0, -1); now = time(NULL); - do_failed_login_test(2, now, 2, 5, EACCES, 2, (now + 5 * 60)); + do_failed_login_test(2, now, 2, 5, ERR_AUTH_DENIED, 2, (now + 5 * 60));
} END_TEST diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c index a5526280afc7f303daace8efaa00b2b486f9233a..50d2ca5e40728804b46ebe66b40dccb992f2c888 100644 --- a/src/tests/sysdb-tests.c +++ b/src/tests/sysdb-tests.c @@ -1663,8 +1663,10 @@ START_TEST (test_sysdb_cached_authentication_missing_password) username = talloc_asprintf(tmp_ctx, "testuser%d", _i); fail_unless(username != NULL, "talloc_asprintf failed.");
- cached_authentication_without_expiration(username, "abc", ENOENT); - cached_authentication_with_expiration(username, "abc", ENOENT); + cached_authentication_without_expiration(username, "abc", + ERR_NO_CACHED_CREDS); + cached_authentication_with_expiration(username, "abc", + ERR_NO_CACHED_CREDS);
talloc_free(tmp_ctx);
@@ -1682,8 +1684,10 @@ START_TEST (test_sysdb_cached_authentication_wrong_password) username = talloc_asprintf(tmp_ctx, "testuser%d", _i); fail_unless(username != NULL, "talloc_asprintf failed.");
- cached_authentication_without_expiration(username, "abc", EINVAL); - cached_authentication_with_expiration(username, "abc", EINVAL); + cached_authentication_without_expiration(username, "abc", + ERR_AUTH_FAILED); + cached_authentication_with_expiration(username, "abc", + ERR_AUTH_FAILED);
talloc_free(tmp_ctx);
diff --git a/src/util/auth_utils.h b/src/util/auth_utils.h index e9e60a085c386c1354732a816f8d982c5e6bf23f..8883c5ceb3691a89cc553c28bd6a76c5c611da0e 100644 --- a/src/util/auth_utils.h +++ b/src/util/auth_utils.h @@ -28,15 +28,17 @@ static inline int cached_login_pam_status(int auth_res) { switch (auth_res) { - case EOK: - return PAM_SUCCESS; - case ENOENT: - return PAM_AUTHINFO_UNAVAIL; - case EINVAL: - return PAM_AUTH_ERR; - case EACCES: - return PAM_PERM_DENIED; + case EOK: + return PAM_SUCCESS; + case ERR_ACCOUNT_UNKNOWN: + return PAM_AUTHINFO_UNAVAIL; + case ERR_NO_CACHED_CREDS: + case ERR_CACHED_CREDS_EXPIRED: + case ERR_AUTH_DENIED: + return PAM_PERM_DENIED; + case ERR_AUTH_FAILED: + return PAM_AUTH_ERR; + default: + return PAM_SYSTEM_ERR; } - - return PAM_SYSTEM_ERR; } diff --git a/src/util/util_errors.c b/src/util/util_errors.c index 92dced3c5f75f964adbaa1d886a07427c3f0a8c3..c196aae38a696df0333ed2de611202340066510e 100644 --- a/src/util/util_errors.c +++ b/src/util/util_errors.c @@ -27,6 +27,11 @@ struct err_string { struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ + { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ + { "No cached credentials available" }, /* ERR_NO_CACHED_CREDS */ + { "Cached credentials are expired" }, /* ERR_CACHED_CREDS_EXPIRED */ + { "Authentication Denied" }, /* ERR_AUTH_DENIED */ + { "Authentication Failed" }, /* ERR_AUTH_DENIED */ };
diff --git a/src/util/util_errors.h b/src/util/util_errors.h index eb0df77e6e8dbe2829cd59def3dee1d8abfce810..870d9d44bcb9a674e7208400ad649b7c761903de 100644 --- a/src/util/util_errors.h +++ b/src/util/util_errors.h @@ -49,6 +49,11 @@ typedef int errno_t; enum sssd_errors { ERR_INVALID = ERR_BASE + 0, ERR_INTERNAL, + ERR_ACCOUNT_UNKNOWN, + ERR_NO_CACHED_CREDS, + ERR_CACHED_CREDS_EXPIRED, + ERR_AUTH_DENIED, + ERR_AUTH_FAILED, ERR_LAST /* ALWAYS LAST */ };
EIO has always been an odd match, but was used as an error to indicate that something had gone wrong internally before we had specific SSSD errors available. Use ERR_INTERNAL instead going forward. --- src/util/util.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/util.h b/src/util/util.h index 61e5cb0e20a9f690ad26940992cefe17d20484c5..961fc42f400a502b7a8d2d9ee1422e858d8a9a09 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -263,7 +263,7 @@ errno_t set_debug_file_from_fd(const int fd); if (TRROEstate == TEVENT_REQ_USER_ERROR) { \ return TRROEerr; \ } \ - return EIO; \ + return ERR_INTERNAL; \ } \ } while (0)
Use the new internal SSSD errors, to simplify error handling. Instead of using up to 3 different error types (system, krb5 and pam_status), collapse all error reporting into one error type mapped on errno_t.
The returned error can contain either SSSD internal errors, kerberos errors or system errors, they all use different number spaces so there is no overlap and they can be safely merged.
This means that errors being sent from the child to the parent are not pam status error messages anymore. The callers have been changed to properly deal with that.
Also note that this patch removes returning SSS_PAM_SYSTEM_INFO from the krb5_child for kerberos errors as all it was doing was simply to make the parent emit the same debug log already emitted by the child, and the code is simpler if we do not do that. --- src/providers/krb5/krb5_auth.c | 157 +++++++++------- src/providers/krb5/krb5_child.c | 388 ++++++++++++++++----------------------- src/util/util_errors.c | 7 +- src/util/util_errors.h | 5 + 4 files changed, 256 insertions(+), 301 deletions(-)
diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c index 398f06a8433a324c22eed57ec5f394f0c34dcede..7c8286d63e0de4f4f87641ec75b3517809a40e3e 100644 --- a/src/providers/krb5/krb5_auth.c +++ b/src/providers/krb5/krb5_auth.c @@ -920,22 +920,64 @@ static void krb5_auth_done(struct tevent_req *subreq)
/* If the child request failed, but did not return an offline error code, * return with the status */ - if (res->msg_status != PAM_SUCCESS && - res->msg_status != PAM_AUTHINFO_UNAVAIL && - res->msg_status != PAM_AUTHTOK_LOCK_BUSY && - res->msg_status != PAM_NEW_AUTHTOK_REQD) { - state->pam_status = res->msg_status; - state->dp_err = DP_ERR_OK; - ret = EOK; - goto done; - } else { - state->pam_status = res->msg_status; - } + switch (res->msg_status) { + case ERR_OK: + /* If the child request was successful and we run the first pass of the + * change password request just return success. */ + if (pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { + state->pam_status = PAM_SUCCESS; + state->dp_err = DP_ERR_OK; + ret = EOK; + goto done; + } + break;
- /* If the password is expired we can safely remove the ccache from the - * cache and disk if it is not actively used anymore. This will allow to - * create a new random ccache if sshd with privilege separation is used. */ - if (res->msg_status == PAM_NEW_AUTHTOK_REQD) { + case ERR_NETWORK_IO: + if (kr->kpasswd_srv != NULL && + (pd->cmd == SSS_PAM_CHAUTHTOK || + pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM)) { + /* if using a dedicated kpasswd server for a chpass operation... */ + + be_fo_set_port_status(state->be_ctx, + state->krb5_ctx->kpasswd_service->name, + kr->kpasswd_srv, PORT_NOT_WORKING); + /* ..try to resolve next kpasswd server */ + state->search_kpasswd = true; + subreq = be_resolve_server_send(state, state->ev, state->be_ctx, + state->krb5_ctx->kpasswd_service->name, + state->kr->kpasswd_srv == NULL ? true : false); + if (subreq == NULL) { + DEBUG(1, ("Resolver request failed.\n")); + ret = ENOMEM; + goto done; + } + tevent_req_set_callback(subreq, krb5_auth_resolve_done, req); + return; + } else if (kr->srv != NULL) { + /* failed to use the KDC... */ + be_fo_set_port_status(state->be_ctx, + state->krb5_ctx->service->name, + kr->srv, PORT_NOT_WORKING); + /* ..try to resolve next KDC */ + state->search_kpasswd = false; + subreq = be_resolve_server_send(state, state->ev, state->be_ctx, + state->krb5_ctx->service->name, + kr->srv == NULL ? true : false); + if (subreq == NULL) { + DEBUG(1, ("Resolver request failed.\n")); + ret = ENOMEM; + goto done; + } + tevent_req_set_callback(subreq, krb5_auth_resolve_done, req); + return; + } + break; + + case ERR_CREDS_EXPIRED: + /* If the password is expired we can safely remove the ccache from the + * cache and disk if it is not actively used anymore. This will allow + * to create a new random ccache if sshd with privilege separation is + * used. */ if (pd->cmd == SSS_PAM_AUTHENTICATE && !kr->active_ccache) { if (kr->old_ccname != NULL) { ret = safe_remove_old_ccache_file(kr->cc_be, kr->upn, @@ -953,70 +995,39 @@ static void krb5_auth_done(struct tevent_req *subreq) } }
- state->pam_status = res->msg_status; + state->pam_status = PAM_NEW_AUTHTOK_REQD; + state->dp_err = DP_ERR_OK; + ret = EOK; + goto done; + + case ERR_NO_CREDS: + state->pam_status = PAM_CRED_UNAVAIL; + state->dp_err = DP_ERR_OK; + ret = EOK; + goto done; + + case ERR_AUTH_FAILED: + state->pam_status = PAM_AUTH_ERR; state->dp_err = DP_ERR_OK; ret = EOK; goto done; - }
- /* If the child request was successful and we run the first pass of the - * change password request just return success. */ - if (res->msg_status == PAM_SUCCESS && pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { - state->pam_status = PAM_SUCCESS; + default: + state->pam_status = PAM_SYSTEM_ERR; state->dp_err = DP_ERR_OK; ret = EOK; goto done; }
- /* if using a dedicated kpasswd server for a chpass operation... */ if (kr->kpasswd_srv != NULL && - (pd->cmd == SSS_PAM_CHAUTHTOK || pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM)) { - /* ..which is unreachable by now.. */ - if (res->msg_status == PAM_AUTHTOK_LOCK_BUSY) { - be_fo_set_port_status(state->be_ctx, - state->krb5_ctx->kpasswd_service->name, - kr->kpasswd_srv, PORT_NOT_WORKING); - /* ..try to resolve next kpasswd server */ - state->search_kpasswd = true; - subreq = be_resolve_server_send(state, state->ev, state->be_ctx, - state->krb5_ctx->kpasswd_service->name, - state->kr->kpasswd_srv == NULL ? true : false); - if (subreq == NULL) { - DEBUG(1, ("Resolver request failed.\n")); - ret = ENOMEM; - goto done; - } - tevent_req_set_callback(subreq, krb5_auth_resolve_done, req); - return; - } else { - be_fo_set_port_status(state->be_ctx, - state->krb5_ctx->kpasswd_service->name, - kr->kpasswd_srv, PORT_WORKING); - } - } - - /* if the KDC for auth (PAM_AUTHINFO_UNAVAIL) or - * chpass (PAM_AUTHTOK_LOCK_BUSY) was not available while using KDC - * also for chpass operation... */ - if (res->msg_status == PAM_AUTHINFO_UNAVAIL || - (kr->kpasswd_srv == NULL && res->msg_status == PAM_AUTHTOK_LOCK_BUSY)) { - if (kr->srv != NULL) { - be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name, - kr->srv, PORT_NOT_WORKING); - /* ..try to resolve next KDC */ - state->search_kpasswd = false; - subreq = be_resolve_server_send(state, state->ev, state->be_ctx, - state->krb5_ctx->service->name, - kr->srv == NULL ? true : false); - if (subreq == NULL) { - DEBUG(1, ("Resolver request failed.\n")); - ret = ENOMEM; - goto done; - } - tevent_req_set_callback(subreq, krb5_auth_resolve_done, req); - return; - } + (pd->cmd == SSS_PAM_CHAUTHTOK || + pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM)) { + /* found a dedicated kpasswd server for a chpass operation */ + be_fo_set_port_status(state->be_ctx, + state->krb5_ctx->service->name, + kr->kpasswd_srv, PORT_WORKING); } else if (kr->srv != NULL) { + /* found a KDC */ be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name, kr->srv, PORT_WORKING); } @@ -1055,11 +1066,13 @@ static void krb5_auth_done(struct tevent_req *subreq) goto done; }
- if (res->msg_status == PAM_SUCCESS && - dp_opt_get_int(kr->krb5_ctx->opts, KRB5_RENEW_INTERVAL) > 0 && - (pd->cmd == SSS_PAM_AUTHENTICATE || pd->cmd == SSS_CMD_RENEW || + if (res->msg_status == ERR_OK && + (dp_opt_get_int(kr->krb5_ctx->opts, KRB5_RENEW_INTERVAL) > 0) && + (pd->cmd == SSS_PAM_AUTHENTICATE || + pd->cmd == SSS_CMD_RENEW || pd->cmd == SSS_PAM_CHAUTHTOK) && - res->tgtt.renew_till > res->tgtt.endtime && kr->ccname != NULL) { + (res->tgtt.renew_till > res->tgtt.endtime) && + (kr->ccname != NULL)) { DEBUG(7, ("Adding [%s] for automatic renewal.\n", kr->ccname)); ret = add_tgt_to_renew_table(kr->krb5_ctx, kr->ccname, &(res->tgtt), pd, kr->upn); diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index c53be4e67a5239eb20f6af986c2d3791b2fb3a6b..e56146f4c09b03b49a17d0d2aa7aa0b359db22d6 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -525,9 +525,11 @@ create_ccache(uid_t uid, gid_t gid, krb5_context ctx, return EINVAL; /* Should never get here */ }
-static errno_t pack_response_packet(struct response *resp, int status, - struct pam_data *pd) +static errno_t pack_response_packet(TALLOC_CTX *mem_ctx, errno_t error, + struct response_data *resp_list, + uint8_t **_buf, size_t *_len) { + uint8_t *buf; size_t size = 0; size_t p = 0; struct response_data *pdr; @@ -544,116 +546,69 @@ static errno_t pack_response_packet(struct response *resp, int status,
size = sizeof(int32_t);
- pdr = pd->resp_list; - while (pdr != NULL) { + for (pdr = resp_list; pdr != NULL; pdr = pdr->next) { size += 2*sizeof(int32_t) + pdr->len; - pdr = pdr->next; }
- resp->buf = talloc_array(resp, uint8_t, size); - if (!resp->buf) { + buf = talloc_array(mem_ctx, uint8_t, size); + if (!buf) { DEBUG(1, ("Insufficient memory to create message.\n")); return ENOMEM; }
- SAFEALIGN_SET_INT32(&resp->buf[p], status, &p); + SAFEALIGN_SET_INT32(&buf[p], error, &p);
- pdr = pd->resp_list; - while(pdr != NULL) { - SAFEALIGN_SET_INT32(&resp->buf[p], pdr->type, &p); - SAFEALIGN_SET_INT32(&resp->buf[p], pdr->len, &p); - safealign_memcpy(&resp->buf[p], pdr->data, pdr->len, &p); - - pdr = pdr->next; + for (pdr = resp_list; pdr != NULL; pdr = pdr->next) { + SAFEALIGN_SET_INT32(&buf[p], pdr->type, &p); + SAFEALIGN_SET_INT32(&buf[p], pdr->len, &p); + safealign_memcpy(&buf[p], pdr->data, pdr->len, &p); }
- resp->size = p; - DEBUG(SSSDBG_TRACE_INTERNAL, ("response packet size: [%d]\n", p));
+ *_buf = buf; + *_len = p; return EOK; }
-static struct response *prepare_response_message(struct krb5_req *kr, - krb5_error_code kerr, - int pam_status) +static errno_t k5c_attach_ccname_msg(struct krb5_req *kr) { char *msg = NULL; - const char *krb5_msg = NULL; int ret; - struct response *resp;
- resp = talloc_zero(kr, struct response); - if (resp == NULL) { - DEBUG(1, ("Initializing response failed.\n")); - return NULL; + if (kr->ccname == NULL) { + DEBUG(1, ("Error obtaining ccname.\n")); + return ERR_INTERNAL; }
- DEBUG(SSSDBG_TRACE_FUNC, ("Building response for result [%d]\n", kerr)); - - if (kerr == 0) { - if (kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { - pam_status = PAM_SUCCESS; - ret = EOK; - } else if (kr->pd->cmd == SSS_PAM_ACCT_MGMT) { - ret = EOK; - } else { - if (kr->ccname == NULL) { - DEBUG(1, ("Error obtaining ccname.\n")); - return NULL; - } - - msg = talloc_asprintf(kr, "%s=%s",CCACHE_ENV_NAME, kr->ccname); - if (msg == NULL) { - DEBUG(1, ("talloc_asprintf failed.\n")); - return NULL; - } - - pam_status = PAM_SUCCESS; - ret = pam_add_response(kr->pd, SSS_PAM_ENV_ITEM, strlen(msg) + 1, - (uint8_t *) msg); - talloc_zfree(msg); - } - } else { - krb5_msg = sss_krb5_get_error_message(krb5_error_ctx, kerr); - if (krb5_msg == NULL) { - DEBUG(1, ("sss_krb5_get_error_message failed.\n")); - return NULL; - } - - ret = pam_add_response(kr->pd, SSS_PAM_SYSTEM_INFO, - strlen(krb5_msg) + 1, - (const uint8_t *) krb5_msg); - sss_krb5_free_error_message(krb5_error_ctx, krb5_msg); - } - if (ret != EOK) { - DEBUG(1, ("pam_add_response failed.\n")); + msg = talloc_asprintf(kr, "%s=%s",CCACHE_ENV_NAME, kr->ccname); + if (msg == NULL) { + DEBUG(1, ("talloc_asprintf failed.\n")); + return ENOMEM; }
- ret = pack_response_packet(resp, pam_status, kr->pd); - if (ret != EOK) { - DEBUG(1, ("pack_response_packet failed.\n")); - return NULL; - } + ret = pam_add_response(kr->pd, SSS_PAM_ENV_ITEM, + strlen(msg) + 1, (uint8_t *)msg); + talloc_zfree(msg);
- return resp; + return ret; }
-static errno_t k5c_send_data(struct krb5_req *kr, int fd, - krb5_error_code kerr, int status) +static errno_t k5c_send_data(struct krb5_req *kr, int fd, errno_t error) { - struct response *resp; size_t written; + uint8_t *buf; + size_t len; int ret;
- resp = prepare_response_message(kr, kerr, status); - if (resp == NULL) { - DEBUG(1, ("prepare_response_message failed.\n")); - return ENOMEM; + ret = pack_response_packet(kr, error, kr->pd->resp_list, &buf, &len); + if (ret != EOK) { + DEBUG(1, ("pack_response_packet failed.\n")); + return ret; }
errno = 0; - written = sss_atomic_write_s(fd, resp->buf, resp->size); + written = sss_atomic_write_s(fd, buf, len); if (written == -1) { ret = errno; DEBUG(SSSDBG_CRIT_FAILURE, @@ -661,10 +616,10 @@ static errno_t k5c_send_data(struct krb5_req *kr, int fd, return ret; }
- if (written != resp->size) { + if (written != len) { DEBUG(SSSDBG_CRIT_FAILURE, ("Write error, wrote [%d] bytes, expected [%d]\n", - written, resp->size)); + written, len)); return EOK; }
@@ -915,10 +870,9 @@ done: static krb5_error_code get_and_save_tgt(struct krb5_req *kr, const char *password) { - krb5_error_code kerr = 0; - int ret; const char *realm_name; int realm_length; + krb5_error_code kerr;
kerr = sss_krb5_get_init_creds_opt_set_expire_callback(kr->ctx, kr->options, @@ -957,10 +911,10 @@ static krb5_error_code get_and_save_tgt(struct krb5_req *kr, /* We drop root privileges which were needed to read the keytab file * for the validation of the credentials or for FAST here to run the * ccache I/O operations with user privileges. */ - ret = become_user(kr->uid, kr->gid); - if (ret != EOK) { + kerr = become_user(kr->uid, kr->gid); + if (kerr != 0) { DEBUG(1, ("become_user failed.\n")); - return ret; + return kerr; } }
@@ -973,8 +927,8 @@ static krb5_error_code get_and_save_tgt(struct krb5_req *kr, goto done; }
- ret = add_ticket_times_and_upn_to_response(kr); - if (ret != EOK) { + kerr = add_ticket_times_and_upn_to_response(kr); + if (kerr != 0) { DEBUG(1, ("add_ticket_times_and_upn_to_response failed.\n")); }
@@ -987,59 +941,46 @@ done:
}
-static int kerr_handle_error(krb5_error_code kerr) +static errno_t map_krb5_error(krb5_error_code kerr) { - int pam_status; - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + switch (kerr) { - case KRB5_LIBOS_CANTREADPWD: - pam_status = PAM_CRED_UNAVAIL; - break; - case KRB5_KDC_UNREACH: - pam_status = PAM_AUTHINFO_UNAVAIL; - break; - case KRB5KDC_ERR_KEY_EXP: - pam_status = PAM_NEW_AUTHTOK_REQD; - break; - case KRB5KRB_AP_ERR_BAD_INTEGRITY: - pam_status = PAM_AUTH_ERR; - break; - case KRB5_PREAUTH_FAILED: - case KRB5KDC_ERR_PREAUTH_FAILED: - pam_status = PAM_CRED_ERR; - break; - default: - pam_status = PAM_SYSTEM_ERR; - break; - } + case 0: + return ERR_OK;
- return pam_status; -} + case KRB5_LIBOS_CANTREADPWD: + return ERR_NO_CREDS;
-static int kerr_to_status(krb5_error_code kerr) -{ - if (kerr == 0) { - return PAM_SUCCESS; - } + case KRB5_KDC_UNREACH: + return ERR_NETWORK_IO;
- return kerr_handle_error(kerr); + case KRB5KDC_ERR_KEY_EXP: + return ERR_CREDS_EXPIRED; + + case KRB5KRB_AP_ERR_BAD_INTEGRITY: + case KRB5_PREAUTH_FAILED: + case KRB5KDC_ERR_PREAUTH_FAILED: + return ERR_AUTH_FAILED; + + default: + return ERR_INTERNAL; + } }
-static errno_t changepw_child(struct krb5_req *kr, int *_status) +static errno_t changepw_child(struct krb5_req *kr, bool prelim) { int ret; krb5_error_code kerr = 0; const char *password = NULL; const char *newpassword = NULL; - int pam_status = PAM_SYSTEM_ERR; int result_code = -1; krb5_data result_code_string; krb5_data result_string; char *user_error_message = NULL; size_t user_resp_len; uint8_t *user_resp; - krb5_prompter_fct prompter = sss_krb5_prompter; + krb5_prompter_fct prompter = NULL; const char *realm_name; int realm_length; krb5_get_init_creds_opt *chagepw_options; @@ -1050,20 +991,18 @@ static errno_t changepw_child(struct krb5_req *kr, int *_status) if (ret != EOK) { DEBUG(1, ("Failed to fetch current password [%d] %s.\n", ret, strerror(ret))); - pam_status = PAM_CRED_INSUFFICIENT; - kerr = KRB5KRB_ERR_GENERIC; - goto done; + return ERR_NO_CREDS; }
- if (kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { + if (!prelim) { /* We do not need a password expiration warning here. */ - prompter = NULL; + prompter = sss_krb5_prompter; }
kerr = get_changepw_options(kr->ctx, &chagepw_options); if (kerr != 0) { DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); - goto done; + return kerr; }
sss_krb5_princ_realm(kr->ctx, kr->princ, &realm_name, &realm_length); @@ -1077,27 +1016,24 @@ static errno_t changepw_child(struct krb5_req *kr, int *_status) chagepw_options); sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options); if (kerr != 0) { - pam_status = kerr_handle_error(kerr); - goto done; + return kerr; }
sss_authtok_set_empty(&kr->pd->authtok);
- if (kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { + if (prelim) { DEBUG(SSSDBG_TRACE_LIBS, ("Initial authentication for change password operation " "successful.\n")); krb5_free_cred_contents(kr->ctx, kr->creds); - pam_status = PAM_SUCCESS; - goto done; + return EOK; }
ret = sss_authtok_get_password(&kr->pd->newauthtok, &newpassword, NULL); if (ret != EOK) { DEBUG(1, ("Failed to fetch new password [%d] %s.\n", ret, strerror(ret))); - kerr = KRB5KRB_ERR_GENERIC; - goto done; + return ERR_NO_CREDS; }
memset(&result_code_string, 0, sizeof(krb5_data)); @@ -1107,15 +1043,12 @@ static errno_t changepw_child(struct krb5_req *kr, int *_status) &result_code_string, &result_string);
if (kerr == KRB5_KDC_UNREACH) { - pam_status = PAM_AUTHTOK_LOCK_BUSY; - goto done; + return ERR_NETWORK_IO; }
if (kerr != 0 || result_code != 0) { if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - } else { - kerr = KRB5KRB_ERR_GENERIC; }
if (result_code_string.length > 0) { @@ -1153,8 +1086,7 @@ static errno_t changepw_child(struct krb5_req *kr, int *_status) } }
- pam_status = PAM_AUTHTOK_ERR; - goto done; + return ERR_CHPASS_FAILED; }
krb5_free_cred_contents(kr->ctx, kr->creds); @@ -1163,88 +1095,91 @@ static errno_t changepw_child(struct krb5_req *kr, int *_status)
sss_authtok_set_empty(&kr->pd->newauthtok);
- pam_status = kerr_to_status(kerr); - -done: - *_status = pam_status; - return ret; + if (kerr == 0) { + kerr = k5c_attach_ccname_msg(kr); + } + return map_krb5_error(kerr); }
-static errno_t tgt_req_child(struct krb5_req *kr, int *_status) +static errno_t tgt_req_child(struct krb5_req *kr) { - int ret; - krb5_error_code kerr = 0; - const char *password = NULL; - int pam_status = PAM_SYSTEM_ERR; krb5_get_init_creds_opt *chagepw_options; + const char *password = NULL; + krb5_error_code kerr; + char *pass_str; + int ret;
DEBUG(SSSDBG_TRACE_LIBS, ("Attempting to get a TGT\n"));
ret = sss_authtok_get_password(&kr->pd->authtok, &password, NULL); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, ("Unknown authtok type\n")); - pam_status = PAM_CRED_INSUFFICIENT; - kerr = KRB5KRB_ERR_GENERIC; - goto done; + switch (ret) { + if (ret == EACCES) { + DEBUG(SSSDBG_OP_FAILURE, ("Invalid authtok type\n")); + return ERR_INVALID_CRED_TYPE; + } + DEBUG(SSSDBG_OP_FAILURE, ("No credentials available\n")); + return ERR_NO_CREDS; }
kerr = get_and_save_tgt(kr, password);
+ if (kerr != KRB5KDC_ERR_KEY_EXP) { + if (kerr == 0) { + kerr = k5c_attach_ccname_msg(kr); + } + ret = map_krb5_error(kerr); + goto done; + } + /* If the password is expired the KDC will always return KRB5KDC_ERR_KEY_EXP regardless if the supplied password is correct or not. In general the password can still be used to get a changepw ticket. So we validate the password by trying to get a changepw ticket. */ - if (kerr == KRB5KDC_ERR_KEY_EXP) { - DEBUG(SSSDBG_TRACE_LIBS, ("Password was expired\n")); - kerr = sss_krb5_get_init_creds_opt_set_expire_callback(kr->ctx, - kr->options, - NULL, NULL); - if (kerr != 0) { - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - DEBUG(1, ("Failed to unset expire callback, continue ...\n")); - } + DEBUG(SSSDBG_TRACE_LIBS, ("Password was expired\n")); + kerr = sss_krb5_get_init_creds_opt_set_expire_callback(kr->ctx, + kr->options, + NULL, NULL); + if (kerr != 0) { + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + DEBUG(1, ("Failed to unset expire callback, continue ...\n")); + }
- kerr = get_changepw_options(kr->ctx, &chagepw_options); - if (kerr != 0) { - DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); - goto done; - } + kerr = get_changepw_options(kr->ctx, &chagepw_options); + if (kerr != 0) { + DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); + return kerr; + }
- kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ, - discard_const(password), - sss_krb5_prompter, kr, 0, - SSSD_KRB5_CHANGEPW_PRINCIPAL, - chagepw_options); + kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ, + discard_const(password), + sss_krb5_prompter, kr, 0, + SSSD_KRB5_CHANGEPW_PRINCIPAL, + chagepw_options);
- sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options); + sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options);
- krb5_free_cred_contents(kr->ctx, kr->creds); - if (kerr == 0) { - kerr = KRB5KDC_ERR_KEY_EXP; - } + krb5_free_cred_contents(kr->ctx, kr->creds); + if (kerr == 0) { + ret = ERR_CREDS_EXPIRED; + } else { + ret = map_krb5_error(kerr); }
- sss_authtok_set_empty(&kr->pd->authtok); - - pam_status = kerr_to_status(kerr); - done: - *_status = pam_status; + sss_authtok_set_empty(&kr->pd->authtok); return ret; }
-static errno_t kuserok_child(struct krb5_req *kr, int *_status) +static errno_t kuserok_child(struct krb5_req *kr) { krb5_boolean access_allowed; - int ret; krb5_error_code kerr;
DEBUG(SSSDBG_TRACE_LIBS, ("Verifying if principal can log in as user\n"));
/* krb5_kuserok tries to verify that kr->pd->user is a locally known * account, so we have to unset _SSS_LOOPS to make getpwnam() work. */ - ret = unsetenv("_SSS_LOOPS"); - if (ret != EOK) { + if (unsetenv("_SSS_LOOPS") != 0) { DEBUG(1, ("Failed to unset _SSS_LOOPS, " "krb5_kuserok will most certainly fail.\n")); } @@ -1259,17 +1194,19 @@ static errno_t kuserok_child(struct krb5_req *kr, int *_status) DEBUG(SSSDBG_TRACE_LIBS, ("Access was %s\n", access_allowed ? "allowed" : "denied"));
- *_status = access_allowed ? 0 : 1; - return ret; + if (access_allowed) { + return EOK; + } + + return ERR_AUTH_DENIED; }
-static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) +static errno_t renew_tgt_child(struct krb5_req *kr) { - int ret; - int status = PAM_AUTHTOK_ERR; - int kerr; const char *ccname; krb5_ccache ccache = NULL; + krb5_error_code kerr; + int ret;
DEBUG(SSSDBG_TRACE_LIBS, ("Renewing a ticket\n"));
@@ -1278,8 +1215,7 @@ static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) DEBUG(SSSDBG_OP_FAILURE, ("Unsupported authtok type for TGT renewal [%d].\n", sss_authtok_get_type(&kr->pd->authtok))); - kerr = EINVAL; - goto done; + return ERR_INVALID_CRED_TYPE; }
kerr = krb5_cc_resolve(kr->ctx, ccname, &ccache); @@ -1290,7 +1226,6 @@ static errno_t renew_tgt_child(struct krb5_req *kr, int *_status)
kerr = krb5_get_renewed_creds(kr->ctx, kr->creds, kr->princ, ccache, NULL); if (kerr != 0) { - status = kerr_handle_error(kerr); goto done; }
@@ -1309,10 +1244,9 @@ static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) /* We drop root privileges which were needed to read the keytab file * for the validation of the credentials or for FAST here to run the * ccache I/O operations with user privileges. */ - ret = become_user(kr->uid, kr->gid); - if (ret != EOK) { + kerr = become_user(kr->uid, kr->gid); + if (kerr != 0) { DEBUG(1, ("become_user failed.\n")); - kerr = ret; goto done; } } @@ -1329,13 +1263,12 @@ static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) goto done; }
- ret = add_ticket_times_and_upn_to_response(kr); - if (ret != EOK) { + kerr = add_ticket_times_and_upn_to_response(kr); + if (kerr != 0) { DEBUG(1, ("add_ticket_times_and_upn_to_response failed.\n")); }
- status = PAM_SUCCESS; - kerr = 0; + kerr = k5c_attach_ccname_msg(kr);
done: krb5_free_cred_contents(kr->ctx, kr->creds); @@ -1344,26 +1277,24 @@ done: krb5_cc_close(kr->ctx, ccache); }
- *_status = status; - return ret; + return map_krb5_error(kerr); }
-static errno_t create_empty_ccache(struct krb5_req *kr, int *_status) +static errno_t create_empty_ccache(struct krb5_req *kr) { - int ret; - int pam_status = PAM_SUCCESS; + krb5_error_code kerr;
DEBUG(SSSDBG_TRACE_LIBS, ("Creating empty ccache\n"));
- ret = create_ccache(kr->uid, kr->gid, kr->ctx, - kr->princ, kr->ccname, NULL); - if (ret != 0) { - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, ret); - pam_status = PAM_SYSTEM_ERR; + kerr = create_ccache(kr->uid, kr->gid, kr->ctx, + kr->princ, kr->ccname, NULL); + if (kerr != 0) { + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + } else { + kerr = k5c_attach_ccname_msg(kr); }
- *_status = pam_status; - return ret; + return map_krb5_error(kerr); }
static errno_t unpack_authtok(TALLOC_CTX *mem_ctx, struct sss_auth_token *tok, @@ -1898,7 +1829,6 @@ int main(int argc, const char *argv[]) int opt; poptContext pc; int debug_fd = -1; - int status; int ret;
struct poptOption long_options[] = { @@ -1971,30 +1901,32 @@ int main(int argc, const char *argv[]) /* If we are offline, we need to create an empty ccache file */ if (offline) { DEBUG(SSSDBG_TRACE_FUNC, ("Will perform offline auth\n")); - ret = create_empty_ccache(kr, &status); + ret = create_empty_ccache(kr); } else { DEBUG(SSSDBG_TRACE_FUNC, ("Will perform online auth\n")); - ret = tgt_req_child(kr, &status); + ret = tgt_req_child(kr); } break; case SSS_PAM_CHAUTHTOK: - case SSS_PAM_CHAUTHTOK_PRELIM: DEBUG(SSSDBG_TRACE_FUNC, ("Will perform password change\n")); - ret = changepw_child(kr, &status); + ret = changepw_child(kr, false); + break; + case SSS_PAM_CHAUTHTOK_PRELIM: + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform password change checks\n")); + ret = changepw_child(kr, true); break; case SSS_PAM_ACCT_MGMT: DEBUG(SSSDBG_TRACE_FUNC, ("Will perform account management\n")); - ret = kuserok_child(kr, &status); + ret = kuserok_child(kr); break; case SSS_CMD_RENEW: - if (!offline) { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform ticket renewal\n")); - ret = renew_tgt_child(kr, &status); - } else { + if (offline) { DEBUG(SSSDBG_CRIT_FAILURE, ("Cannot renew TGT while offline\n")); ret = KRB5_KDC_UNREACH; goto done; } + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform ticket renewal\n")); + ret = renew_tgt_child(kr); break; default: DEBUG(1, ("PAM command [%d] not supported.\n", kr->pd->cmd)); @@ -2002,7 +1934,7 @@ int main(int argc, const char *argv[]) goto done; }
- ret = k5c_send_data(kr, STDOUT_FILENO, ret, status); + ret = k5c_send_data(kr, STDOUT_FILENO, ret); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to send reply\n")); } diff --git a/src/util/util_errors.c b/src/util/util_errors.c index c196aae38a696df0333ed2de611202340066510e..1760c8d84e8c4c4e3906692cc9acbe166a4279fc 100644 --- a/src/util/util_errors.c +++ b/src/util/util_errors.c @@ -28,10 +28,15 @@ struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ + { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ + { "No credentials available" }, /* ERR_NO_CREDS */ + { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */ { "No cached credentials available" }, /* ERR_NO_CACHED_CREDS */ { "Cached credentials are expired" }, /* ERR_CACHED_CREDS_EXPIRED */ { "Authentication Denied" }, /* ERR_AUTH_DENIED */ - { "Authentication Failed" }, /* ERR_AUTH_DENIED */ + { "Authentication Failed" }, /* ERR_AUTH_FAILED */ + { "Password Change Failed" }, /* ERR_CHPASS_FAILED */ + { "Network I/O Error" }, /* ERR_NETWORK_IO */ };
diff --git a/src/util/util_errors.h b/src/util/util_errors.h index 870d9d44bcb9a674e7208400ad649b7c761903de..9292c9959bed1d1772ca761c04f6db417c6286c1 100644 --- a/src/util/util_errors.h +++ b/src/util/util_errors.h @@ -50,10 +50,15 @@ enum sssd_errors { ERR_INVALID = ERR_BASE + 0, ERR_INTERNAL, ERR_ACCOUNT_UNKNOWN, + ERR_INVALID_CRED_TYPE, + ERR_NO_CREDS, + ERR_CREDS_EXPIRED, ERR_NO_CACHED_CREDS, ERR_CACHED_CREDS_EXPIRED, ERR_AUTH_DENIED, ERR_AUTH_FAILED, + ERR_CHPASS_FAILED, + ERR_NETWORK_IO, ERR_LAST /* ALWAYS LAST */ };
Rebased patch so that it applies on top of master + krb5 child refactoring rebased patch.
Simo.
On 02/27/2013 03:52 PM, Simo Sorce wrote:
Rebased patch so that it applies on top of master + krb5 child refactoring rebased patch.
Simo.
Hi, while reviewing subsequent patches I noticed that pass_str variable is unused in tgt_req_child().
On Mon, 2013-03-04 at 12:49 +0100, Pavel Březina wrote:
On 02/27/2013 03:52 PM, Simo Sorce wrote:
Rebased patch so that it applies on top of master + krb5 child refactoring rebased patch.
Simo.
Hi, while reviewing subsequent patches I noticed that pass_str variable is unused in tgt_req_child().
I noticed that too.
I'll send a new patch to remove it.
Simo.
On Mon, 2013-03-04 at 09:39 -0500, Simo Sorce wrote:
On Mon, 2013-03-04 at 12:49 +0100, Pavel Březina wrote:
On 02/27/2013 03:52 PM, Simo Sorce wrote:
Rebased patch so that it applies on top of master + krb5 child refactoring rebased patch.
Simo.
Hi, while reviewing subsequent patches I noticed that pass_str variable is unused in tgt_req_child().
I noticed that too.
I'll send a new patch to remove it.
Patch that removes pass_str attached.
Simo.
On Mon, Mar 04, 2013 at 01:35:13PM -0500, Simo Sorce wrote:
On Mon, 2013-03-04 at 09:39 -0500, Simo Sorce wrote:
On Mon, 2013-03-04 at 12:49 +0100, Pavel Březina wrote:
On 02/27/2013 03:52 PM, Simo Sorce wrote:
Rebased patch so that it applies on top of master + krb5 child refactoring rebased patch.
Simo.
Hi, while reviewing subsequent patches I noticed that pass_str variable is unused in tgt_req_child().
I noticed that too.
I'll send a new patch to remove it.
Patch that removes pass_str attached.
Simo.
All four patches pushed upstream.
Thank you for the patience.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 01/12/2013 03:29 PM, Simo Sorce wrote:
Rebased on master, this patchset depends on the 2 patch krb-child refactor patchset.
Simo.
Simo Sorce (4): Add SSSD specific error codes and definitions Use SSSD specific errors for offline auth Return ERR_INTERNAL instead of EIO Cleanup error message handling for krb5 child
Makefile.am | 4 +- src/db/sysdb_ops.c | 17 +- src/providers/krb5/krb5_auth.c | 157 +++++++++------- src/providers/krb5/krb5_child.c | 388 ++++++++++++++++----------------------- src/tests/auth-tests.c | 6 +- src/tests/sysdb-tests.c | 12 +- src/util/auth_utils.h | 22 ++- src/util/util.h | 10 +- src/util/util_errors.c | 51 +++++ src/util/util_errors.h | 85 +++++++++ 10 files changed, 418 insertions(+), 334 deletions(-) create mode 100644 src/util/util_errors.c create mode 100644 src/util/util_errors.h
I have several general concerns about this set of patches. I agree with the intent.
1) Please don't use ERR_ as the prefix. I'm wary that we'd eventually have a conflict with some other library that we decided to import. Perhaps SSS_ERR_* instead?
2) Can we please use 'enum sssd_errors ret' instead of 'int ret' in the code for all new additions? It adds better compile-time checking (and possibly optimizations) and will make it easier to transition, since as we switch to using this, it will reveal any attempts we make to assign an errno value to it (even if it's coming from another function).
3) Similarly, I'd like for our new functions to have 'enum sssd_errors' as their return type. It will make it explicitly clear what they are meant to be returning.
Minor nitpick on the first patch: util.h and util_errors.h both #require each other. If they are co-dependent, we probably want to handle this with 'extern' rather than a circular requirement.
On Thu, 2013-02-28 at 10:29 -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 01/12/2013 03:29 PM, Simo Sorce wrote:
Rebased on master, this patchset depends on the 2 patch krb-child refactor patchset.
Simo.
Simo Sorce (4): Add SSSD specific error codes and definitions Use SSSD specific errors for offline auth Return ERR_INTERNAL instead of EIO Cleanup error message handling for krb5 child
Makefile.am | 4 +- src/db/sysdb_ops.c | 17 +- src/providers/krb5/krb5_auth.c | 157 +++++++++------- src/providers/krb5/krb5_child.c | 388 ++++++++++++++++----------------------- src/tests/auth-tests.c | 6 +- src/tests/sysdb-tests.c | 12 +- src/util/auth_utils.h | 22 ++- src/util/util.h | 10 +- src/util/util_errors.c | 51 +++++ src/util/util_errors.h | 85 +++++++++ 10 files changed, 418 insertions(+), 334 deletions(-) create mode 100644 src/util/util_errors.c create mode 100644 src/util/util_errors.h
I have several general concerns about this set of patches. I agree with the intent.
- Please don't use ERR_ as the prefix. I'm wary that we'd eventually
have a conflict with some other library that we decided to import. Perhaps SSS_ERR_* instead?
Too long, I know of no librray that uses ERR_, I say we should worry when we find one.
- Can we please use 'enum sssd_errors ret' instead of 'int ret' in
the code for all new additions?
No, we use errno_t.
It adds better compile-time checking (and possibly optimizations) and will make it easier to transition, since as we switch to using this, it will reveal any attempts we make to assign an errno value to it (even if it's coming from another function).
No because we use the full range of errors, including errno and krb5 errors, using enum would be simply wrong.
- Similarly, I'd like for our new functions to have 'enum
sssd_errors' as their return type. It will make it explicitly clear what they are meant to be returning.
As above, no.
Minor nitpick on the first patch: util.h and util_errors.h both #require each other. If they are co-dependent, we probably want to handle this with 'extern' rather than a circular requirement.
Nah I am going to remove the include from util_errors.h, it should never be included directly anyway, just via util.h
Simo.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina -
Simo Sorce -
Stephen Gallagher