On Tue, Jun 07, 2016 at 03:08:36PM +0200, Jakub Hrozek wrote: > On Fri, Jun 03, 2016 at 08:17:01PM +0200, Sumit Bose wrote: > > Hi, > > > > currently the code which generates ssh key from the public keys in the > > user certificates fails if one certificate cannot be validated and > > terminates the whole request. It is of course valid that the user entry > > might contain certificates which SSSD cannot validate and since we just > > won't generate a ssh-key in this case SSSD should just skip those > > entires and return ssh-keys for every valid certificate. > > > > You can test the patch even without a real certificate by e.g. adding a > > ssh-key to an IPA user object. Then 'sss_ssh_authorizedkeys username' > > should return this key. If you now add some random data the the > > userCertificate object of the same user, call 'sss_cache -E' and call > > 'sss_ssh_authorizedkeys username' again, you get nothing because the > > random data cannot be validated and hence the whole request is aborted. > > With the attached patch sss_ssh_authorizedkeys should return the ssh-key > > again. > > > > bye, > > Sumit > > > From 540c69184a128bb840c7f41cabfb0cfe62f344a7 Mon Sep 17 00:00:00 2001 > > From: Sumit Bose <sb...@redhat.com> > > Date: Thu, 2 Jun 2016 18:22:03 +0200 > > Subject: [PATCH] ssh: skip invalid certificates > > > > Current an invalid certificate cause the whole ssh key lookup request to > > abort. Since it is possible that e.g. the LDAP user entry contains > > certificates where the client does not have the needed CA certificates > > for validation we should just ignore invalid certificates. > > > > Resolves https://fedorahosted.org/sssd/ticket/2977 > > --- > > src/responder/ssh/sshsrv_cmd.c | 171 > > ++++++++++++++++++++++++++++++----------- > > 1 file changed, 126 insertions(+), 45 deletions(-) > > > > diff --git a/src/responder/ssh/sshsrv_cmd.c b/src/responder/ssh/sshsrv_cmd.c > > index > > 5954cec1ba3a688ae2d14f2304d722d91c26d592..51922d59a8150ceb7cd96e728c1c482b373639a8 > > 100644 > > --- a/src/responder/ssh/sshsrv_cmd.c > > +++ b/src/responder/ssh/sshsrv_cmd.c > > @@ -781,9 +781,94 @@ ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx) > > return EOK; > > } > > > > +static errno_t get_valid_certs_keys(TALLOC_CTX *mem_ctx, > > + struct ssh_cmd_ctx *cmd_ctx, > > + struct ldb_message_element *el_cert, > > + struct ssh_ctx *ssh_ctx, > > + struct ldb_message_element **_el_res) > > +{ > > + TALLOC_CTX *tmp_ctx; > > + uint8_t *key; > > + size_t key_len; > > + char *cert_verification_opts; > > + struct cert_verify_opts *cert_verify_opts; > > + int ret; > > + struct ldb_message_element *el_res; > > + struct cli_ctx *cctx = cmd_ctx->cctx; > > + size_t d; > > + > > + if (el_cert == NULL) { > > + DEBUG(SSSDBG_TRACE_ALL, "Mssing element, nothing to do.\n"); > > + return EOK; > > + } > > + > > + tmp_ctx = talloc_new(NULL); > > + if (tmp_ctx == NULL) { > > + DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n"); > > + return ENOMEM; > > + } > > + > > [...] > > > + if (el_res->num_values == 0) { > > + *_el_res = NULL; > > + } else { > > + *_el_res = talloc_steal(mem_ctx, el_res); > > + } > > + > > + return EOK; > > You need to free tmp_ctx in this function.
ah. sorry, I added it to the caller and forgot to add it to the new function. New version attached. bye, Sumit > > +} > > + > > The rest of the patch LGTM. > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
From 6c8776a370fde6caae653a322a7ca358f31e4de3 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Thu, 2 Jun 2016 18:22:03 +0200 Subject: [PATCH] ssh: skip invalid certificates Current an invalid certificate cause the whole ssh key lookup request to abort. Since it is possible that e.g. the LDAP user entry contains certificates where the client does not have the needed CA certificates for validation we should just ignore invalid certificates. Resolves https://fedorahosted.org/sssd/ticket/2977 --- src/responder/ssh/sshsrv_cmd.c | 179 ++++++++++++++++++++++++++++++----------- 1 file changed, 134 insertions(+), 45 deletions(-) diff --git a/src/responder/ssh/sshsrv_cmd.c b/src/responder/ssh/sshsrv_cmd.c index 5954cec1ba3a688ae2d14f2304d722d91c26d592..ba3b694d97821696ca0218c279c1f9d9859ab13e 100644 --- a/src/responder/ssh/sshsrv_cmd.c +++ b/src/responder/ssh/sshsrv_cmd.c @@ -781,9 +781,102 @@ ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx) return EOK; } +static errno_t get_valid_certs_keys(TALLOC_CTX *mem_ctx, + struct ssh_cmd_ctx *cmd_ctx, + struct ldb_message_element *el_cert, + struct ssh_ctx *ssh_ctx, + struct ldb_message_element **_el_res) +{ + TALLOC_CTX *tmp_ctx; + uint8_t *key; + size_t key_len; + char *cert_verification_opts; + struct cert_verify_opts *cert_verify_opts; + int ret; + struct ldb_message_element *el_res; + struct cli_ctx *cctx = cmd_ctx->cctx; + size_t d; + + if (el_cert == NULL) { + DEBUG(SSSDBG_TRACE_ALL, "Mssing element, nothing to do.\n"); + return EOK; + } + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n"); + return ENOMEM; + } + + ret = confdb_get_string(cctx->rctx->cdb, tmp_ctx, + CONFDB_MONITOR_CONF_ENTRY, + CONFDB_MONITOR_CERT_VERIFICATION, NULL, + &cert_verification_opts); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to read p11_child_timeout from confdb: [%d] %s\n", + ret, sss_strerror(ret)); + goto done; + } + + if (cert_verification_opts != NULL) { + ret = parse_cert_verify_opts(tmp_ctx, cert_verification_opts, + &cert_verify_opts); + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to parse verifiy option.\n"); + goto done; + } + } + + el_res = talloc_zero(tmp_ctx, struct ldb_message_element); + if (el_res == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_zero failed.\n"); + ret = ENOMEM; + goto done; + } + + el_res->values = talloc_array(el_res, struct ldb_val, el_cert->num_values); + if (el_res->values == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n"); + ret = ENOMEM; + goto done; + } + + for (d = 0; d < el_cert->num_values; d++) { + ret = cert_to_ssh_key(tmp_ctx, ssh_ctx->ca_db, + el_cert->values[d].data, + el_cert->values[d].length, + cert_verify_opts, &key, &key_len); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "cert_to_ssh_key failed, ignoring.\n"); + continue; + } + + el_res->values[el_res->num_values].data = + talloc_steal(el_res->values, key); + el_res->values[el_res->num_values].length = key_len; + el_res->num_values++; + } + + if (el_res->num_values == 0) { + *_el_res = NULL; + } else { + *_el_res = talloc_steal(mem_ctx, el_res); + } + + ret = EOK; + +done: + + talloc_free(tmp_ctx); + + return ret; +} + static errno_t decode_and_add_base64_data(struct ssh_cmd_ctx *cmd_ctx, struct ldb_message_element *el, - bool cert_data, + bool skip_base64_decode, struct ssh_ctx *ssh_ctx, size_t fqname_len, const char *fqname, @@ -797,8 +890,6 @@ static errno_t decode_and_add_base64_data(struct ssh_cmd_ctx *cmd_ctx, int ret; size_t d; TALLOC_CTX *tmp_ctx; - char *cert_verification_opts; - struct cert_verify_opts *cert_verify_opts; if (el == NULL) { DEBUG(SSSDBG_TRACE_ALL, "Mssing element, nothing to do.\n"); @@ -812,36 +903,9 @@ static errno_t decode_and_add_base64_data(struct ssh_cmd_ctx *cmd_ctx, } for (d = 0; d < el->num_values; d++) { - if (cert_data) { - - ret = confdb_get_string(cctx->rctx->cdb, tmp_ctx, - CONFDB_MONITOR_CONF_ENTRY, - CONFDB_MONITOR_CERT_VERIFICATION, NULL, - &cert_verification_opts); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, - "Failed to read p11_child_timeout from confdb: [%d] %s\n", - ret, sss_strerror(ret)); - return ret; - } - - if (cert_verification_opts != NULL) { - ret = parse_cert_verify_opts(tmp_ctx, cert_verification_opts, - &cert_verify_opts); - if (ret != EOK) { - DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to parse verifiy option.\n"); - return ret; - } - } - - ret = cert_to_ssh_key(tmp_ctx, ssh_ctx->ca_db, - el->values[d].data, el->values[d].length, - cert_verify_opts, &key, &key_len); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, "cert_to_ssh_key failed.\n"); - return ret; - } + if (skip_base64_decode) { + key = el->values[d].data; + key_len = el->values[d].length; } else { key = sss_base64_decode(tmp_ctx, (const char *) el->values[d].data, &key_len); @@ -888,12 +952,14 @@ ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) struct ldb_message_element *el_override = NULL; struct ldb_message_element *el_orig = NULL; struct ldb_message_element *el_user_cert = NULL; + struct ldb_message_element *el_user_cert_keys = NULL; uint32_t count = 0; const char *name; char *fqname; uint32_t fqname_len; struct ssh_ctx *ssh_ctx = talloc_get_type(cctx->rctx->pvt_ctx, struct ssh_ctx); + TALLOC_CTX *tmp_ctx; ret = sss_packet_new(cctx->creq, 0, sss_packet_get_cmd(cctx->creq->in), @@ -902,6 +968,12 @@ ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) return ret; } + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n"); + return ENOMEM; + } + el = ldb_msg_find_element(cmd_ctx->result, SYSDB_SSH_PUBKEY); if (el) { count = el->num_values; @@ -923,13 +995,21 @@ ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) el_user_cert = ldb_msg_find_element(cmd_ctx->result, SYSDB_USER_CERT); if (el_user_cert) { - /* TODO check if cert is valid */ - count += el_user_cert->num_values; + ret = get_valid_certs_keys(cmd_ctx, cmd_ctx, el_user_cert, ssh_ctx, + &el_user_cert_keys); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "get_valid_certs_keys failed.\n"); + goto done; + } + + if (el_user_cert_keys) { + count += el_user_cert_keys->num_values; + } } ret = sss_packet_grow(cctx->creq->out, 2*sizeof(uint32_t)); if (ret != EOK) { - return ret; + goto done; } sss_packet_get_body(cctx->creq->out, &body, &body_len); @@ -937,7 +1017,8 @@ ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) SAFEALIGN_SET_UINT32(body+c, 0, &c); if (count == 0) { - return EOK; + ret = EOK; + goto done; } name = ldb_msg_find_attr_as_string(cmd_ctx->result, SYSDB_NAME, NULL); @@ -945,13 +1026,15 @@ ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) DEBUG(SSSDBG_OP_FAILURE, "Got unnamed result for [%s@%s]\n", cmd_ctx->name, cmd_ctx->domain->name); - return ENOENT; + ret = ENOENT; + goto done; } fqname = talloc_asprintf(cmd_ctx, "%s@%s", name, cmd_ctx->domain->name); if (!fqname) { - return ENOMEM; + ret = ENOMEM; + goto done; } fqname_len = strlen(fqname)+1; @@ -960,31 +1043,37 @@ ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) fqname_len, fqname, &c); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "decode_and_add_base64_data failed.\n"); - return ret; + goto done; } ret = decode_and_add_base64_data(cmd_ctx, el_orig, false, ssh_ctx, fqname_len, fqname, &c); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "decode_and_add_base64_data failed.\n"); - return ret; + goto done; } ret = decode_and_add_base64_data(cmd_ctx, el_override, false, ssh_ctx, fqname_len, fqname, &c); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "decode_and_add_base64_data failed.\n"); - return ret; + goto done; } - ret = decode_and_add_base64_data(cmd_ctx, el_user_cert, true, ssh_ctx, + ret = decode_and_add_base64_data(cmd_ctx, el_user_cert_keys, true, ssh_ctx, fqname_len, fqname, &c); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "decode_and_add_base64_data failed.\n"); - return ret; + goto done; } - return EOK; + ret = EOK; + +done: + + talloc_free(tmp_ctx); + + return ret; } static errno_t -- 2.1.0
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
