On Mon, 2009-09-14 at 07:48 -0400, Stephen Gallagher wrote: > Just a nitpick, but why did you replace sbus_conn_send_reply() in > be_check_online with sbus_get_connection and dbus_connection_send()? > They are functionally identical. (except that sbus_conn_send_reply() > can > get the connection in one fewer stack frame, since it can access the > sbus_connection object directly)
I merged together what previously was an async request. It was only asking the id provider about offline status. Since now the offline status is directly accessible by the dp_provider_be.c there is no need to make a request to any provider, we directly return the answer to dp. (this call is not used by anything anyway so far but we were planning to use it to force a backend to go offline so it will come handy later on). > Assuming I'm reading this correctly, we're talking about considering a > single backend process as being online or offline as a whole. Why is > this dependent only on the ID provider for the backend? It isn't, attached new patch that add be_mark_offline() calls also to the auth backends. > Shouldn't we > consider that if the authentication module or password change modules > are offline that we are offline? yes, should be fixed in the new patch, now. > Furthermore, even if the ID provider is offline, if we have cached > user > information that allows us to initiate a connection to a still-live > authentication provider, isn't that perfectly reasonable? No, also because this is just an initial patch to start building infrastructure. If you remember we discussed the idea or allowing the monitor to put a backend forcibly offline, in that case all providers must respect this. Further more during auth we want to always refresh users, so if the id part is not available we can as well auth from the cached password (if caching passwords is allowed). This infrastructure is also need for DNS discovery later, where we want a central place to tell a specific server is unreachable. In any case I would rather put the infrastructure in place now, and tweak specific behaviors later. > I'd argue that if any ONE of the modules was the ultimate determinator > of online/offline status, it should be authentication rather than > identification. Nope the auth module contacts the servers more rarely than the id backend (auths are rare compared to requests to get id information), so normally the ID backend is more qualified. In any case I am not going to make any provider king, all of them should be able to signal that servers are unreachable. > Code itself is sensible, so this is a Nack until you can convince me > that the approach itself is right. Let's see if the new patch and explanations are enough :) Simo. -- Simo Sorce * Red Hat, Inc * New York
>From a0b02295b0e0c55174b37ad3b231d28e0e1d1f50 Mon Sep 17 00:00:00 2001 From: Simo Sorce <sso...@redhat.com> Date: Sat, 12 Sep 2009 00:05:55 -0400 Subject: [PATCH] Make the offline status backend-global Add helpers functions to query/set the offline status per backend. Now all providers share the same offline status. --- server/providers/data_provider_be.c | 117 +++++++++------------------------- server/providers/dp_backend.h | 11 +++- server/providers/krb5/krb5_auth.c | 17 ++++- server/providers/ldap/ldap_auth.c | 29 +++++++-- server/providers/ldap/ldap_id.c | 55 ++-------------- server/providers/proxy.c | 80 ++++-------------------- 6 files changed, 94 insertions(+), 215 deletions(-) diff --git a/server/providers/data_provider_be.c b/server/providers/data_provider_be.c index 55fc278..2e83ab6 100644 --- a/server/providers/data_provider_be.c +++ b/server/providers/data_provider_be.c @@ -133,66 +133,39 @@ static int be_file_request(struct be_ctx *ctx, return EOK; } -static void online_chk_callback(struct be_req *req, int status, - const char *errstr) -{ - struct be_online_req *oreq; - DBusMessage *reply; - DBusConnection *dbus_conn; - dbus_bool_t dbret; - dbus_uint16_t online; - dbus_uint16_t err_maj = 0; - dbus_uint32_t err_min = 0; - const char *err_msg = "Success"; - - oreq = talloc_get_type(req->req_data, struct be_online_req); - if (status != EOK) { - online = MOD_OFFLINE; - err_maj = DP_ERR_FATAL; - err_min = status; - err_msg = errstr; - } - - online = oreq->online; - reply = (DBusMessage *)req->pvt; +bool be_is_offline(struct be_ctx *ctx) +{ + time_t now = time(NULL); - dbret = dbus_message_append_args(reply, - DBUS_TYPE_UINT16, &online, - DBUS_TYPE_UINT16, &err_maj, - DBUS_TYPE_UINT32, &err_min, - DBUS_TYPE_STRING, &err_msg, - DBUS_TYPE_INVALID); - if (!dbret) { - DEBUG(1, ("Failed to generate dbus reply\n")); - return; + /* check if we are past the offline blackout timeout */ + /* FIXME: get offline_timeout from configuration */ + if (ctx->offstat.went_offline + 60 < now) { + ctx->offstat.offline = false; } - dbus_conn = sbus_get_connection(req->be_ctx->dp_conn); - dbus_connection_send(dbus_conn, reply, NULL); - dbus_message_unref(reply); + return ctx->offstat.offline; +} - DEBUG(4, ("Request processed. Returned %d,%d,%s\n", - err_maj, err_min, err_msg)); +void be_mark_offline(struct be_ctx *ctx) +{ + DEBUG(8, ("Going offline!\n")); - /* finally free the request */ - talloc_free(req); + ctx->offstat.went_offline = time(NULL); + ctx->offstat.offline = true; } - static int be_check_online(DBusMessage *message, struct sbus_connection *conn) { - struct be_online_req *req; - struct be_req *be_req; struct be_ctx *ctx; DBusMessage *reply; + DBusConnection *dbus_conn; dbus_bool_t dbret; void *user_data; - int ret; dbus_uint16_t online; - dbus_uint16_t err_maj; - dbus_uint32_t err_min; - const char *err_msg; + dbus_uint16_t err_maj = 0; + dbus_uint32_t err_min = 0; + static const char *err_msg = "Success"; user_data = sbus_conn_get_private_data(conn); if (!user_data) return EINVAL; @@ -202,45 +175,10 @@ static int be_check_online(DBusMessage *message, struct sbus_connection *conn) reply = dbus_message_new_method_return(message); if (!reply) return ENOMEM; - /* process request */ - be_req = talloc(ctx, struct be_req); - if (!be_req) { - online = MOD_OFFLINE; - err_maj = DP_ERR_FATAL; - err_min = ENOMEM; - err_msg = "Out of memory"; - goto done; - } - be_req->be_ctx = ctx; - be_req->fn = online_chk_callback; - be_req->pvt = reply; - - req = talloc(be_req, struct be_online_req); - if (!req) { + if (be_is_offline(ctx)) { online = MOD_OFFLINE; - err_maj = DP_ERR_FATAL; - err_min = ENOMEM; - err_msg = "Out of memory"; - goto done; - } - req->online = 0; - - be_req->req_data = req; - - ret = be_file_request(ctx, ctx->bet_info[BET_ID].bet_ops->check_online, be_req); - if (ret != EOK) { - online = MOD_OFFLINE; - err_maj = DP_ERR_FATAL; - err_min = ret; - err_msg = "Failed to file request"; - goto done; - } - - return EOK; - -done: - if (be_req) { - talloc_free(be_req); + } else { + online = MOD_ONLINE; } dbret = dbus_message_append_args(reply, @@ -249,16 +187,23 @@ done: DBUS_TYPE_UINT32, &err_min, DBUS_TYPE_STRING, &err_msg, DBUS_TYPE_INVALID); - if (!dbret) return EIO; + if (!dbret) { + DEBUG(1, ("Failed to generate dbus reply\n")); + return EIO; + } - /* send reply back */ - sbus_conn_send_reply(conn, reply); + dbus_conn = sbus_get_connection(ctx->dp_conn); + dbus_connection_send(dbus_conn, reply, NULL); dbus_message_unref(reply); + DEBUG(4, ("Request processed. Returned %d,%d,%s\n", + err_maj, err_min, err_msg)); + return EOK; } + static void acctinfo_callback(struct be_req *req, int status, const char *errstr) { diff --git a/server/providers/dp_backend.h b/server/providers/dp_backend.h index cce854e..f994963 100644 --- a/server/providers/dp_backend.h +++ b/server/providers/dp_backend.h @@ -60,6 +60,10 @@ struct bet_info { void *pvt_bet_data; }; +struct be_offline_status { + time_t went_offline; + bool offline; +}; struct be_ctx { struct tevent_context *ev; @@ -69,6 +73,8 @@ struct be_ctx { const char *identity; const char *conf_path; + struct be_offline_status offstat; + struct sbus_connection *mon_conn; struct sbus_connection *dp_conn; @@ -97,8 +103,7 @@ struct be_acct_req { char *filter_value; }; -struct be_online_req { - int online; -}; +bool be_is_offline(struct be_ctx *ctx); +void be_mark_offline(struct be_ctx *ctx); #endif /* __DP_BACKEND_H___ */ diff --git a/server/providers/krb5/krb5_auth.c b/server/providers/krb5/krb5_auth.c index 73d3ccd..1456276 100644 --- a/server/providers/krb5/krb5_auth.c +++ b/server/providers/krb5/krb5_auth.c @@ -489,6 +489,12 @@ static void krb5_pam_handler(struct be_req *be_req) pd = talloc_get_type(be_req->req_data, struct pam_data); + if (be_is_offline(be_req->be_ctx)) { + DEBUG(4, ("Backend is marked offline, retry later!\n")); + pam_status = PAM_AUTHINFO_UNAVAIL; + goto done; + } + if (pd->cmd != SSS_PAM_AUTHENTICATE && pd->cmd != SSS_PAM_CHAUTHTOK) { DEBUG(4, ("krb5 does not handles pam task %d.\n", pd->cmd)); pam_status = PAM_SUCCESS; @@ -653,6 +659,11 @@ static void krb5_pam_handler_done(struct tevent_req *req) pd->pam_status = *msg_status; + if (pd->pam_status == PAM_AUTHINFO_UNAVAIL) { + be_mark_offline(be_req->be_ctx); + goto done; + } + if (pd->pam_status == PAM_SUCCESS && pd->cmd == SSS_PAM_AUTHENTICATE) { env = talloc_asprintf(pd, "%s=%s", SSSD_REALM, krb5_ctx->realm); if (env == NULL) { @@ -741,20 +752,18 @@ static void krb5_pam_handler_cache_done(struct tevent_req *subreq) } struct bet_ops krb5_auth_ops = { - .check_online = NULL, .handler = krb5_pam_handler, .finalize = NULL, }; struct bet_ops krb5_chpass_ops = { - .check_online = NULL, .handler = krb5_pam_handler, .finalize = NULL, }; -int sssm_krb5_auth_init(struct be_ctx *bectx, struct bet_ops **ops, - void **pvt_auth_data) +int sssm_krb5_auth_init(struct be_ctx *bectx, + struct bet_ops **ops, void **pvt_auth_data) { struct krb5_ctx *ctx = NULL; char *value = NULL; diff --git a/server/providers/ldap/ldap_auth.c b/server/providers/ldap/ldap_auth.c index 47ed0f0..51afee3 100644 --- a/server/providers/ldap/ldap_auth.c +++ b/server/providers/ldap/ldap_auth.c @@ -40,7 +40,7 @@ #include "providers/ldap/sdap_async.h" struct sdap_auth_ctx { - struct be_ctx *bectx; + struct be_ctx *be; struct sdap_options *opts; }; @@ -87,8 +87,8 @@ struct tevent_req *get_user_dn_send(TALLOC_CTX *memctx, /* this sysdb call uses a sysdn operation, which means it will be * schedule only after we return, no timer hack needed */ - ret = sysdb_get_user_attr(state, state->ctx->bectx->sysdb, - state->ctx->bectx->domain, state->name, + ret = sysdb_get_user_attr(state, state->ctx->be->sysdb, + state->ctx->be->domain, state->name, state->attrs, get_user_dn_done, req); if (ret) { tevent_req_error(req, ret); @@ -292,7 +292,7 @@ int auth_recv(struct tevent_req *req, enum sdap_result *result, uint64_t err; if (tevent_req_is_error(req, &tstate, &err)) { - if (err == EAGAIN) *result = SDAP_UNAVAIL; + if (err == ETIMEDOUT) *result = SDAP_UNAVAIL; else *result = SDAP_ERROR; return EOK; } @@ -338,6 +338,12 @@ static void sdap_pam_chpass_send(struct be_req *breq) struct sdap_auth_ctx); pd = talloc_get_type(breq->req_data, struct pam_data); + if (be_is_offline(ctx->be)) { + DEBUG(4, ("Backend is marked offline, retry later!\n")); + pd->pam_status = PAM_AUTHINFO_UNAVAIL; + goto done; + } + DEBUG(2, ("starting password change request for user [%s].\n", pd->user)); pd->pam_status = PAM_SYSTEM_ERR; @@ -466,6 +472,12 @@ static void sdap_pam_auth_send(struct be_req *breq) ctx = talloc_get_type(breq->be_ctx->bet_info[BET_AUTH].pvt_bet_data, struct sdap_auth_ctx); pd = talloc_get_type(breq->req_data, struct pam_data); + if (be_is_offline(ctx->be)) { + DEBUG(4, ("Backend is marked offline, retry later!\n")); + pd->pam_status = PAM_AUTHINFO_UNAVAIL; + goto done; + } + pd->pam_status = PAM_SYSTEM_ERR; switch (pd->cmd) { @@ -531,6 +543,11 @@ static void sdap_pam_auth_done(struct tevent_req *req) state->pd->pam_status = PAM_SYSTEM_ERR; } + if (result == SDAP_UNAVAIL) { + be_mark_offline(state->breq->be_ctx); + goto done; + } + if (result == SDAP_AUTH_SUCCESS && state->breq->be_ctx->domain->cache_credentials) { @@ -589,13 +606,11 @@ static void sdap_shutdown(struct be_req *req) } struct bet_ops sdap_auth_ops = { - .check_online = NULL, .handler = sdap_pam_auth_send, .finalize = sdap_shutdown }; struct bet_ops sdap_chpass_ops = { - .check_online = NULL, .handler = sdap_pam_chpass_send, .finalize = sdap_shutdown }; @@ -612,7 +627,7 @@ int sssm_ldap_auth_init(struct be_ctx *bectx, ctx = talloc(bectx, struct sdap_auth_ctx); if (!ctx) return ENOMEM; - ctx->bectx = bectx; + ctx->be = bectx; ret = sdap_get_options(ctx, bectx->cdb, bectx->conf_path, &ctx->opts); diff --git a/server/providers/ldap/ldap_id.c b/server/providers/ldap/ldap_id.c index bebeea2..1984582 100644 --- a/server/providers/ldap/ldap_id.c +++ b/server/providers/ldap/ldap_id.c @@ -39,9 +39,6 @@ struct sdap_id_ctx { /* global sdap handler */ struct sdap_handle *gsh; - time_t went_offline; - bool offline; - /* enumeration loop timer */ struct timeval last_run; @@ -54,43 +51,6 @@ static void sdap_req_done(struct be_req *req, int ret, const char *err) return req->fn(req, ret, err); } -static bool is_offline(struct sdap_id_ctx *ctx) -{ - time_t now = time(NULL); - - /* check if we are past the offline blackout timeout */ - if (ctx->went_offline + ctx->opts->offline_timeout < now) { - ctx->offline = false; - } - - return ctx->offline; -} - -static void mark_offline(struct sdap_id_ctx *ctx) -{ - DEBUG(8, ("Going offline!\n")); - - ctx->went_offline = time(NULL); - ctx->offline = true; -} - -static void sdap_check_online(struct be_req *req) -{ - struct be_online_req *oreq; - struct sdap_id_ctx *ctx; - - ctx = talloc_get_type(req->be_ctx->bet_info[BET_ID].pvt_bet_data, struct sdap_id_ctx); - oreq = talloc_get_type(req->req_data, struct be_online_req); - - if (is_offline(ctx)) { - oreq->online = MOD_OFFLINE; - } else { - oreq->online = MOD_ONLINE; - } - - sdap_req_done(req, EOK, NULL); -} - static int build_attrs_from_map(TALLOC_CTX *memctx, struct sdap_id_map *map, size_t size, @@ -403,7 +363,7 @@ static void users_get_done(struct tevent_req *req) if (ret == ETIMEDOUT) { ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data, struct sdap_id_ctx); - mark_offline(ctx); + be_mark_offline(ctx->be); } } @@ -568,7 +528,7 @@ static void groups_get_done(struct tevent_req *req) if (ret == ETIMEDOUT) { ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data, struct sdap_id_ctx); - mark_offline(ctx); + be_mark_offline(ctx->be); } } @@ -708,7 +668,7 @@ static void groups_by_user_done(struct tevent_req *req) if (ret == ETIMEDOUT) { ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data, struct sdap_id_ctx); - mark_offline(ctx); + be_mark_offline(ctx->be); } } @@ -731,7 +691,7 @@ static void sdap_get_account_info(struct be_req *breq) ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data, struct sdap_id_ctx); - if (is_offline(ctx)) { + if (be_is_offline(ctx->be)) { return sdap_req_done(breq, EAGAIN, "Offline"); } @@ -830,7 +790,7 @@ static void ldap_id_enumerate(struct tevent_context *ev, struct tevent_timer *timeout; struct tevent_req *req; - if (is_offline(ctx)) { + if (be_is_offline(ctx->be)) { DEBUG(4, ("Backend is marked offline, retry later!\n")); /* schedule starting from now, not the last run */ ldap_id_enumerate_set_timer(ctx, tevent_timeval_current()); @@ -971,7 +931,7 @@ fail: (int)err, strerror(err))); if (err == ETIMEDOUT) { - mark_offline(state->ctx); + be_mark_offline(state->ctx->be); } } @@ -998,7 +958,7 @@ static void ldap_id_enum_groups_done(struct tevent_req *subreq) fail: if (err == ETIMEDOUT) { - mark_offline(state->ctx); + be_mark_offline(state->ctx->be); } DEBUG(1, ("Failed to enumerate groups, retrying later!\n")); @@ -1312,7 +1272,6 @@ static void sdap_shutdown(struct be_req *req) } struct bet_ops sdap_id_ops = { - .check_online = sdap_check_online, .handler = sdap_get_account_info, .finalize = sdap_shutdown }; diff --git a/server/providers/proxy.c b/server/providers/proxy.c index 5428a6d..1436a63 100644 --- a/server/providers/proxy.c +++ b/server/providers/proxy.c @@ -57,11 +57,12 @@ struct proxy_nss_ops { }; struct proxy_ctx { + struct be_ctx *be; struct proxy_nss_ops ops; - bool offline; }; struct proxy_auth_ctx { + struct be_ctx *be; char *pam_target; }; @@ -70,49 +71,6 @@ struct authtok_conv { uint8_t *authtok; }; -static void offline_timeout(struct tevent_context *ev, struct tevent_timer *tt, - struct timeval tv, void *pvt) -{ - struct proxy_ctx *ctx; - - ctx = talloc_get_type(pvt, struct proxy_ctx); - ctx->offline = false; -} - -static void go_offline(struct be_ctx *be_ctx) -{ - struct proxy_ctx *ctx; - struct tevent_timer *tt; - struct timeval timeout; - int ret; - - ctx = talloc_get_type(be_ctx->bet_info[BET_ID].pvt_bet_data, struct proxy_ctx); - - ret = gettimeofday(&timeout, NULL); - if (ret == -1) { - DEBUG(1, ("gettimeofday failed [%d][%s].\n", errno, strerror(errno))); - return; - } - timeout.tv_sec += 15; /* TODO: get from conf */ - - tt = tevent_add_timer(be_ctx->ev, ctx, timeout, offline_timeout, ctx); - if (tt == NULL) { - DEBUG(1, ("Failed to add timer\n")); - return; - } - - ctx->offline = true; -} - -static bool is_offline(struct be_ctx *be_ctx) -{ - struct proxy_ctx *ctx; - - ctx = talloc_get_type(be_ctx->bet_info[BET_ID].pvt_bet_data, struct proxy_ctx); - - return ctx->offline; -} - static int proxy_internal_conv(int num_msg, const struct pam_message **msgm, struct pam_response **response, void *appdata_ptr) { @@ -236,6 +194,10 @@ static void proxy_pam_handler(struct be_req *req) { DEBUG(4, ("Pam result: [%d][%s]\n", pam_status, pam_strerror(pamh, pam_status))); + if (pam_status == PAM_AUTHINFO_UNAVAIL) { + be_mark_offline(req->be_ctx); + } + ret = pam_end(pamh, pam_status); if (ret != PAM_SUCCESS) { pamh=NULL; @@ -1904,22 +1866,6 @@ static int get_group_from_gid_recv(struct tevent_req *req) /* =Proxy_Id-Functions====================================================*/ -/* TODO: actually do check something */ -static void proxy_check_online(struct be_req *req) -{ - struct be_online_req *oreq; - - oreq = talloc_get_type(req->req_data, struct be_online_req); - - if (is_offline(req->be_ctx)) { - oreq->online = MOD_OFFLINE; - } else { - oreq->online = MOD_ONLINE; - } - - req->fn(req, EOK, NULL); -} - static void proxy_get_account_info_done(struct tevent_req *subreq); /* TODO: See if we can use async_req code */ @@ -1940,7 +1886,7 @@ static void proxy_get_account_info(struct be_req *breq) sysdb = breq->be_ctx->sysdb; domain = breq->be_ctx->domain; - if (is_offline(breq->be_ctx)) { + if (be_is_offline(breq->be_ctx)) { return proxy_reply(breq, EAGAIN, "Offline"); } @@ -2082,7 +2028,7 @@ static void proxy_get_account_info_done(struct tevent_req *subreq) if (ret) { if (ret == ENXIO) { DEBUG(2, ("proxy returned UNAVAIL error, going offline!\n")); - go_offline(breq->be_ctx); + be_mark_offline(breq->be_ctx); } } proxy_reply(breq, ret, NULL); @@ -2101,25 +2047,21 @@ static void proxy_auth_shutdown(struct be_req *req) } struct bet_ops proxy_id_ops = { - .check_online = proxy_check_online, .handler = proxy_get_account_info, .finalize = proxy_shutdown }; struct bet_ops proxy_auth_ops = { - .check_online = proxy_check_online, .handler = proxy_pam_handler, .finalize = proxy_auth_shutdown }; struct bet_ops proxy_access_ops = { - .check_online = proxy_check_online, .handler = proxy_pam_handler, .finalize = proxy_auth_shutdown }; struct bet_ops proxy_chpass_ops = { - .check_online = proxy_check_online, .handler = proxy_pam_handler, .finalize = proxy_auth_shutdown }; @@ -2151,6 +2093,7 @@ int sssm_proxy_init(struct be_ctx *bectx, if (!ctx) { return ENOMEM; } + ctx->be = bectx; ret = confdb_get_string(bectx->cdb, ctx, bectx->conf_path, "libName", NULL, &libname); @@ -2271,7 +2214,10 @@ int sssm_proxy_auth_init(struct be_ctx *bectx, int ret; ctx = talloc(bectx, struct proxy_auth_ctx); - if (!ctx) return ENOMEM; + if (!ctx) { + return ENOMEM; + } + ctx->be = bectx; ret = confdb_get_string(bectx->cdb, ctx, bectx->conf_path, "pam-target", NULL, &ctx->pam_target); -- 1.6.2.5
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel