[PATCH] ssh: skip invalid certificates

[Sumit Bose]

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

[Jakub Hrozek]

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 
> 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.
> +}

The rest of the patch LGTM.

[Sumit Bose]

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 
> > 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
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Jakub Hrozek]

On Tue, Jun 07, 2016 at 03:58:31PM +0200, Sumit Bose wrote:
> > 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

Thank you, ACK

CI: http://sssd-ci.duckdns.org/logs/job/45/27/summary.html
(dyndns test failed on F-22 which is unrelated)

[Jakub Hrozek]

On Thu, Jun 16, 2016 at 01:33:47PM +0200, Jakub Hrozek wrote:
> On Tue, Jun 07, 2016 at 03:58:31PM +0200, Sumit Bose wrote:
> > > 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
>
> Thank you, ACK
>
> CI: http://sssd-ci.duckdns.org/logs/job/45/27/summary.html
> (dyndns test failed on F-22 which is unrelated)

* master: 60787fb44924e84a0c7ddfe9d5e62e64ea1edcd1

[Jakub Hrozek]

On Thu, Jun 16, 2016 at 01:38:32PM +0200, Jakub Hrozek wrote:
> On Thu, Jun 16, 2016 at 01:33:47PM +0200, Jakub Hrozek wrote:
> > On Tue, Jun 07, 2016 at 03:58:31PM +0200, Sumit Bose wrote:
> > > > 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
> >
> > Thank you, ACK
> >
> > CI: http://sssd-ci.duckdns.org/logs/job/45/27/summary.html
> > (dyndns test failed on F-22 which is unrelated)
> master: 60787fb44924e84a0c7ddfe9d5e62e64ea1edcd1

I would like to push this patch to the stable brach, anyone against?

[Lukas Slebodnik]

On (19/09/16 09:41), Jakub Hrozek wrote:
> On Thu, Jun 16, 2016 at 01:38:32PM +0200, Jakub Hrozek wrote:
> > On Thu, Jun 16, 2016 at 01:33:47PM +0200, Jakub Hrozek wrote:
> > > On Tue, Jun 07, 2016 at 03:58:31PM +0200, Sumit Bose wrote:
> > > > > 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
> > >
> > > Thank you, ACK
> > >
> > > CI: http://sssd-ci.duckdns.org/logs/job/45/27/summary.html
> > > (dyndns test failed on F-22 which is unrelated)
> > master: 60787fb44924e84a0c7ddfe9d5e62e64ea1edcd1
>
> I would like to push this patch to the stable brach, anyone against?

go ahead.

LS

[Jakub Hrozek]

On Mon, Sep 19, 2016 at 09:51:09AM +0200, Lukas Slebodnik wrote:
> On (19/09/16 09:41), Jakub Hrozek wrote:
> > On Thu, Jun 16, 2016 at 01:38:32PM +0200, Jakub Hrozek wrote:
> > > On Thu, Jun 16, 2016 at 01:33:47PM +0200, Jakub Hrozek wrote:
> > > > On Tue, Jun 07, 2016 at 03:58:31PM +0200, Sumit Bose wrote:
> > > > > > 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
> > > >
> > > > Thank you, ACK
> > > >
> > > > CI: http://sssd-ci.duckdns.org/logs/job/45/27/summary.html
> > > > (dyndns test failed on F-22 which is unrelated)
> > > master: 60787fb44924e84a0c7ddfe9d5e62e64ea1edcd1
> >
> > I would like to push this patch to the stable brach, anyone against?
>
> go ahead.

* sssd-1-13: 4dbb3bec93cda57e8336847dff0822f31425004d