[Freeipa-devel] [PATCHES 0089-0093] Authentication Indicators
Nathaniel McCallum
npmccallum at redhat.com
Tue May 24 16:21:43 UTC 2016
New versions again. This time I just removed the stray "TODO: assign
OID" line in the commit as it no longer applies.
On Tue, 2016-05-24 at 12:08 -0400, Nathaniel McCallum wrote:
> I have attached new versions of the patches. Comments below.
>
> On Tue, 2016-05-24 at 15:25 +0200, Sumit Bose wrote:
> > 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/t
> > > > > ic
> > > > > ket/
> > > > > 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 <npmccallum at 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.
>
> I would like for us to merge the first four patches first and then
> concentrate on this one.
>
> In particular, the following issue needs to be discussed. We
> currently
> only have two, hard-coded indicator values: otp and radius. Thus, we
> use a StrEnum for this property. However, in the long-term, I'd like
> to
> have more flexibility; such as per-token indicators. This implies
> String.
>
> Is there some way to do StrEnum now and easily migrate to String
> later?
> I think this will break API. Thoughts?
>
> > > From 356daafb001bd868f37f6f0b58338bd0c4da135c Mon Sep 17 00:00:00
> > > 2001
> > > From: Nathaniel McCallum <npmccallum at 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 <npmccallum at 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 <npmccallum at 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..022525b786705b4f58f861b
> > > c3
> > > b0a745ab8693755 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..184962095029edec15dac80
> > > 07
> > > 21eb63b3353ec50 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 ?
>
> I have added the assigned OID now.
>
> > > bool otpctrl_present(Slapi_PBlock *pb, const char *oid);
> > >
> >
> > > From ec1c3e3ef0e3b06c6fd93b3d4709b55f28e12873 Mon Sep 17 00:00:00
> > > 2001
> > > From: Nathaniel McCallum <npmccallum at redhat.com>
> > > Date: Thu, 12 May 2016 14:57:29 -0400
> > > Subject: [PATCH 1/5] Rename syncreq.[ch] to otpctrl.[ch]
> > >
> >
> > ...
> >
> > > index
> > > 98a97c4c9f6d2e6bf74f97fc93053b3aebbc7821..0b5cbc586118fe43b6e9ac8
> > > 23
> > > 179174a7b3ba91e 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.
>
> I have used guards in the latest patch. I now have another patch on
> the
> list to migrate all files from using guards to pragma.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Enable-service-authentication-indicator-management.patch
Type: text/x-patch
Size: 6254 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160524/644418ff/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Enable-authentication-indicators-for-OTP-and-RADIUS.patch
Type: text/x-patch
Size: 1986 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160524/644418ff/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Return-password-only-preauth-if-passwords-are-allowe.patch
Type: text/x-patch
Size: 1676 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160524/644418ff/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Ensure-that-ipa-otpd-bind-auths-validate-an-OTP.patch
Type: text/x-patch
Size: 5556 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160524/644418ff/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Rename-syncreq.-ch-to-otpctrl.-ch.patch
Type: text/x-patch
Size: 4960 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160524/644418ff/attachment-0004.bin>
More information about the Freeipa-devel
mailing list