On Thu, May 12, 2016 at 05:33:26PM -0400, Nathaniel McCallum wrote:
> On Fri, 2016-05-06 at 14:44 +0200, Sumit Bose wrote:
> > On Wed, May 04, 2016 at 05:33:55PM -0400, Nathaniel McCallum wrote:
> > > This series of patches implements authentication indicator
> > > insertion,
> > > evaluation and management in FreeIPA. Besides these patches, two
> > > other
> > > patches are needed to round out support.
> > > 
> > > First, we need a UI patch: https://fedorahosted.org/freeipa/ticket/
> > > 5872
> > > 
> > > Second, we need a SSSD patch to handle the new case where multiple
> > > responders are set (when either 1FA or 2FA can be used).
> > 
> > I've already some initial work done here and will continue with your
> > patches.
> > 
> > > 
> > > Please note that the last patch in this series (0093) is untested
> > > and
> > > simply represents my desire to get these patches off of my hard
> > > disk
> > > before I take a long weekend. This patch also requires mrogers'
> > > patch
> > > 0001 (already merged to master).
> > > 
> > > Also worthy of note is the need for an OID for the authentication
> > > control. Hopefully Simo can assign this after we agree that this
> > > control method is sufficient. One question I had was whether or not
> > > it
> > > would be possible to send the control only on UNIX sockets (0089;
> > > report_auth_method()).
> > > 
> > > Please review the approaches taken here. I plan to hit this hard on
> > > Monday.
> > 
> > I'm on a conference next week and currently busy preparing my
> > presentation. I will give you feedback in the following week.
> 
> Thanks!

sorry for the delay, I was distracted because on some of my VMs OTP in
general does not work anymore. I assume some issue, maybe with libverto
on 32bit systems, but I'm still debugging.

Nevertheless I was able to successfully test the patches in 64bits.

Simo, please see OID assignment request below.

> 
> The attached patches offer the latest version of the work. The only
> major outstanding item that I see is OID assignment (which we can do
> just before committing).
> 
> I have tested the full stack both for appropriate approvals and denials
> across all possible scenarios. In short it works.
> 
> The easiest way to test this is as following:
> 
> # After Clean Install of FreeIPA
> $ kinit admin
> 
> # Add a service allowed by either 1FA or 2FA
> $ ipa service-add ANY/ipa.example.com
> $ ipa-getkeytab -p ANY/ipa.example.com -k /tmp/any.keytab
> 
> # Add a service allowed only by 2FA
> $ ipa service-add OTP/ipa.example.com --auth-ind=otp
> $ ipa-getkeytab -p OTP/ipa.example.com -k /tmp/otp.keytab
> 
> # Add the test user
> $ ipa user-add test --user-auth-type=otp --user-auth-type=password
> $ ipa passwd test
> $ kinit test
> 
> # Try to get tickets for the services
> $ kvno ANY/ipa.example.com # Expected success
> $ kvno OTP/ipa.example.com # Expected failure
> 
> # Add a token and login with 2FA
> $ ipa otptoken-add
> $ kinit -T <ccache> test # Log in with 2FA
> 
> # Try to get tickets for the services
> $ kvno ANY/ipa.example.com #
> Expected success
> $ kvno OTP/ipa.example.com # Expected success

> From c9e2b50248493fb5a283cf8c88c8e20c312d6348 Mon Sep 17 00:00:00 2001
> From: Nathaniel McCallum <npmccal...@redhat.com>
> Date: Wed, 4 May 2016 17:08:45 -0400
> Subject: [PATCH 5/5] Enable service authentication indicator management
> 

For me the patch looks good, but it would be nice if someone more used
to our usage of python can have a short look to see if all conventioens
are met. ACK for the functionality.

> From 356daafb001bd868f37f6f0b58338bd0c4da135c Mon Sep 17 00:00:00 2001
> From: Nathaniel McCallum <npmccal...@redhat.com>
> Date: Sun, 21 Feb 2016 19:44:19 -0500
> Subject: [PATCH 4/5] Enable authentication indicators for OTP and RADIUS
> 

ACK

> From c33d0d2af5284a6eb5e50a4f5864f94fa8b3cf21 Mon Sep 17 00:00:00 2001
> From: Nathaniel McCallum <npmccal...@redhat.com>
> Date: Sun, 21 Feb 2016 19:43:52 -0500
> Subject: [PATCH 3/5] Return password-only preauth if passwords are allowed
> 

ACK, on the client krb5_responder_list_questions() return both
"password" and "otp" if the user is configured for both.

Btw, what is the right way for a client to skip "otp" and only do
"password" should something like krb5_responder_otp_set_answer(ctx,
rctx, i, NULL, NULL); work ?

> From 42768f63cdfd47ff3ac0bcdc228fb363b421e2b2 Mon Sep 17 00:00:00 2001
> From: Nathaniel McCallum <npmccal...@redhat.com>
> Date: Thu, 12 May 2016 15:10:47 -0400
> Subject: [PATCH 2/5] Ensure that ipa-otpd bind auths validate an OTP

ACK. Depending on the user setting LDAP simple bind does work with
password, password+token or both.

> 
> Before this patch, if the user was configured for either OTP or password
> it was possible to do a 1FA authentication through ipa-otpd. Because this
> correctly respected the configuration, it is not a security error.
> 
> However, once we begin to insert authentication indicators into the
> Kerberos tickets, we cannot allow 1FA authentications through this
> code path. Otherwise the ticket would contain a 2FA indicator when
> only 1FA was actually performed.
> 
> To solve this problem, we have ipa-otpd send a critical control during
> the bind operation which informs the LDAP server that it *MUST* validate
> an OTP token for authentication to be successful. Next, we implement
> support for this control in the ipa-pwd-extop plugin. The end result is
> that the bind operation will always fail if the control is present and
> no OTP is validated.
> 
> TODO: assign an OID
> 
> https://fedorahosted.org/freeipa/ticket/433
> ---
>  daemons/ipa-otpd/bind.c                           |  5 ++++-
>  daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h |  3 +++
>  daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 13 ++++++++-----
>  3 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/daemons/ipa-otpd/bind.c b/daemons/ipa-otpd/bind.c
> index 
> c985ccd7e73c6e8182cafcef49fd9873ab3340ea..022525b786705b4f58f861bc3b0a745ab8693755
>  100644
> --- a/daemons/ipa-otpd/bind.c
> +++ b/daemons/ipa-otpd/bind.c
> @@ -26,9 +26,12 @@
>   */
>  
>  #include "internal.h"
> +#include "../ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h"
>  
>  static void on_bind_writable(verto_ctx *vctx, verto_ev *ev)
>  {
> +    LDAPControl control = { OTP_REQUIRED_OID, {}, true };
> +    LDAPControl *ctrls[] = { &control, NULL };
>      struct otpd_queue *push = &ctx.stdio.responses;
>      const krb5_data *data;
>      struct berval cred;
> @@ -55,7 +58,7 @@ static void on_bind_writable(verto_ctx *vctx, verto_ev *ev)
>      cred.bv_val = data->data;
>      cred.bv_len = data->length;
>      i = ldap_sasl_bind(verto_get_private(ev), item->user.dn, 
> LDAP_SASL_SIMPLE,
> -                       &cred, NULL, NULL, &item->msgid);
> +                       &cred, ctrls, NULL, &item->msgid);
>      if (i != LDAP_SUCCESS) {
>          otpd_log_err(errno, "Unable to initiate bind: %s", 
> ldap_err2string(i));
>          verto_break(ctx.vctx);
> diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h 
> b/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h
> index 
> 0b5cbc586118fe43b6e9ac823179174a7b3ba91e..184962095029edec15dac800721eb63b3353ec50
>  100644
> --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h
> +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h
> @@ -53,6 +53,9 @@
>   */
>  #define OTP_SYNC_REQUEST_OID "2.16.840.1.113730.3.8.10.6"
>  
> +/* This control has no data. */
> +#define OTP_REQUIRED_OID "1.2.3.4.5.6.7.8.9"
> +

Simo, can you assign a proper OID for OTP_REQUIRED_OID ?

>  bool otpctrl_present(Slapi_PBlock *pb, const char *oid);
>  

> From ec1c3e3ef0e3b06c6fd93b3d4709b55f28e12873 Mon Sep 17 00:00:00 2001
> From: Nathaniel McCallum <npmccal...@redhat.com>
> Date: Thu, 12 May 2016 14:57:29 -0400
> Subject: [PATCH 1/5] Rename syncreq.[ch] to otpctrl.[ch]
> 

...

> index 
> 98a97c4c9f6d2e6bf74f97fc93053b3aebbc7821..0b5cbc586118fe43b6e9ac823179174a7b3ba91e
>  100644
> --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/syncreq.h
> +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h
> @@ -37,9 +37,7 @@
>   * All rights reserved.
>   * END COPYRIGHT BLOCK **/
>  
> -
> -#ifndef SYNCREQ_H_
> -#define SYNCREQ_H_
> +#pragma once
>  

I would prefer to keep the old way for now and discuss on the list if we
should move to '#pragma once'. If we can get an agreement we can switch
to '#pragma once' completely later.

bye,
Sumit

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to