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 <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.
> 
> 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 <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..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 <npmccal...@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.
From 3aa1417dcc35c70ad7e391ee7ba4caebb1d981df 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

https://fedorahosted.org/freeipa/ticket/433
---
 API.txt                   |  9 ++++++---
 VERSION                   |  4 ++--
 ipalib/plugins/service.py | 10 +++++++++-
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/API.txt b/API.txt
index b2aec7313b6b9496179beddb68e4a0f5a09608bf..7bf4cba0d29e89afbfd465f3f30d9c3de7701465 100644
--- a/API.txt
+++ b/API.txt
@@ -3888,7 +3888,7 @@ output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDA
 output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None)
 output: PrimaryKey('value', None, None)
 command: service_add
-args: 1,11,3
+args: 1,12,3
 arg: Str('krbprincipalname', attribute=True, cli_name='principal', multivalue=False, primary_key=True, required=True)
 option: Str('addattr*', cli_name='addattr', exclude='webui')
 option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
@@ -3896,6 +3896,7 @@ option: Flag('force', autofill=True, default=False)
 option: StrEnum('ipakrbauthzdata', attribute=True, cli_name='pac_type', csv=True, multivalue=True, required=False, values=(u'MS-PAC', u'PAD', u'NONE'))
 option: Bool('ipakrbokasdelegate', attribute=False, cli_name='ok_as_delegate', multivalue=False, required=False)
 option: Bool('ipakrbrequirespreauth', attribute=False, cli_name='requires_pre_auth', multivalue=False, required=False)
+option: StrEnum('krbprincipalauthind', attribute=True, cli_name='auth_ind', multivalue=True, required=False, values=(u'otp', u'radius'))
 option: Flag('no_members', autofill=True, default=False, exclude='webui')
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
 option: Str('setattr*', cli_name='setattr', exclude='webui')
@@ -3998,10 +3999,11 @@ output: Output('completed', <type 'int'>, None)
 output: Output('failed', <type 'dict'>, None)
 output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 command: service_find
-args: 1,11,4
+args: 1,12,4
 arg: Str('criteria?', noextrawhitespace=False)
 option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
 option: StrEnum('ipakrbauthzdata', attribute=True, autofill=False, cli_name='pac_type', csv=True, multivalue=True, query=True, required=False, values=(u'MS-PAC', u'PAD', u'NONE'))
+option: StrEnum('krbprincipalauthind', attribute=True, autofill=False, cli_name='auth_ind', multivalue=True, query=True, required=False, values=(u'otp', u'radius'))
 option: Str('krbprincipalname', attribute=True, autofill=False, cli_name='principal', multivalue=False, primary_key=True, query=True, required=False)
 option: Str('man_by_host*', cli_name='man_by_hosts', csv=True)
 option: Flag('no_members', autofill=True, default=False, exclude='webui')
@@ -4016,7 +4018,7 @@ output: ListOfEntries('result', (<type 'list'>, <type 'tuple'>), Gettext('A list
 output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None)
 output: Output('truncated', <type 'bool'>, None)
 command: service_mod
-args: 1,12,3
+args: 1,13,3
 arg: Str('krbprincipalname', attribute=True, cli_name='principal', multivalue=False, primary_key=True, query=True, required=True)
 option: Str('addattr*', cli_name='addattr', exclude='webui')
 option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
@@ -4024,6 +4026,7 @@ option: Str('delattr*', cli_name='delattr', exclude='webui')
 option: StrEnum('ipakrbauthzdata', attribute=True, autofill=False, cli_name='pac_type', csv=True, multivalue=True, required=False, values=(u'MS-PAC', u'PAD', u'NONE'))
 option: Bool('ipakrbokasdelegate', attribute=False, autofill=False, cli_name='ok_as_delegate', multivalue=False, required=False)
 option: Bool('ipakrbrequirespreauth', attribute=False, autofill=False, cli_name='requires_pre_auth', multivalue=False, required=False)
+option: StrEnum('krbprincipalauthind', attribute=True, autofill=False, cli_name='auth_ind', multivalue=True, required=False, values=(u'otp', u'radius'))
 option: Flag('no_members', autofill=True, default=False, exclude='webui')
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
 option: Flag('rights', autofill=True, default=False)
diff --git a/VERSION b/VERSION
index 36d3fe267d9d8e8da54f3baa9f0038bcb12bfaae..1eb126c7046e6f16cb49bfd6cc00f4df24cfc51f 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=20100614120000
 #                                                      #
 ########################################################
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=166
-# Last change: tbabej - idviews: Add user certificate attribute to user ID overrides
+IPA_API_VERSION_MINOR=167
+# Last change: npmccallum - enable setting authinds on services
diff --git a/ipalib/plugins/service.py b/ipalib/plugins/service.py
index 644b07f463a008eff12d3ff36de9404c03cf8a69..1f7159d8619091f0ee7bceeb85b0faa35eba0004 100644
--- a/ipalib/plugins/service.py
+++ b/ipalib/plugins/service.py
@@ -411,7 +411,7 @@ class service(LDAPObject):
     permission_filter_objectclasses = ['ipaservice']
     search_attributes = ['krbprincipalname', 'managedby', 'ipakrbauthzdata']
     default_attributes = ['krbprincipalname', 'usercertificate', 'managedby',
-        'ipakrbauthzdata', 'memberof', 'ipaallowedtoperform']
+        'ipakrbauthzdata', 'memberof', 'ipaallowedtoperform', 'krbprincipalauthind']
     uuid_attribute = 'ipauniqueid'
     attribute_members = {
         'managedby': ['host'],
@@ -506,6 +506,14 @@ class service(LDAPObject):
             values=(u'MS-PAC', u'PAD', u'NONE'),
             csv=True,
         ),
+        StrEnum('krbprincipalauthind',
+            cli_name='auth_ind',
+            label=_('Authentication Indicators'),
+            doc=_('Authentication indicator whitelist'),
+            values=(u'otp', u'radius'),
+            multivalue=True,
+            required=False,
+        ),
     ) + ticket_flags_params
 
     def validate_ipakrbauthzdata(self, entry):
-- 
2.8.3

From ccd0d394ca5f0ca6584f9a95e2a11ffafc7bfa99 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

If the user is configured for OTP or RADIUS authentication, insert the
relevant authentication indicator.

https://fedorahosted.org/freeipa/ticket/433
---
 daemons/ipa-kdb/ipa_kdb_principals.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index 910d55c4a3e3eecedc1f32eb9f724ae76f40aa57..d4adf27f2de7c7ccd050063e779a30fdae35bc83 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -512,7 +512,8 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
                                               krb5_db_entry **kentry,
                                               uint32_t *polmask)
 {
-    krb5_octet otp_string[] = {'o', 't', 'p', 0, '[', ']', 0 };
+    const krb5_octet rad_string[] = "otp\0[{\"indicators\": [\"radius\"]}]";
+    const krb5_octet otp_string[] = "otp\0[{\"indicators\": [\"otp\"]}]";
     struct ipadb_context *ipactx;
     enum ipadb_user_auth ua;
     LDAP *lcontext;
@@ -842,11 +843,16 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext,
     }
 
     /* If enabled, set the otp user string, enabling otp. */
-    if (ua & (IPADB_USER_AUTH_RADIUS | IPADB_USER_AUTH_OTP)) {
+    if (ua & IPADB_USER_AUTH_OTP) {
         kerr = ipadb_set_tl_data(entry, KRB5_TL_STRING_ATTRS,
                                  sizeof(otp_string), otp_string);
         if (kerr)
             goto done;
+    } else if (ua & IPADB_USER_AUTH_RADIUS) {
+        kerr = ipadb_set_tl_data(entry, KRB5_TL_STRING_ATTRS,
+                                 sizeof(rad_string), rad_string);
+        if (kerr)
+            goto done;
     }
 
     kerr = 0;
-- 
2.8.3

From 8f38ea00b68725851543202de7d2c683c1c7374c 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

Before this patch, if either password or password+otp were permitted,
only the otp preauth mech would be returned to the client. Now, the
client will receive either enc_ts or enc_chl in addition to otp.

https://fedorahosted.org/freeipa/ticket/433
---
 daemons/ipa-kdb/ipa_kdb_principals.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index 50278108769c9032a277f0b53f14380ad4257566..910d55c4a3e3eecedc1f32eb9f724ae76f40aa57 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -302,6 +302,8 @@ static void ipadb_validate_radius(struct ipadb_context *ipactx,
                                "ipatokenRadiusConfigLink");
     if (vals == NULL || vals[0] == NULL)
         *ua &= ~IPADB_USER_AUTH_RADIUS;
+    else
+        *ua = IPADB_USER_AUTH_RADIUS;
 
     if (vals != NULL)
         ldap_value_free_len(vals);
@@ -314,10 +316,6 @@ static void ipadb_validate_password(struct ipadb_context *ipactx,
     /* If no mechanisms are set, use password. */
     if (*ua == IPADB_USER_AUTH_NONE)
         *ua |= IPADB_USER_AUTH_PASSWORD;
-
-    /* If any other mechanism has passed validation, don't use password. */
-    else if (*ua & ~IPADB_USER_AUTH_PASSWORD)
-        *ua &= ~IPADB_USER_AUTH_PASSWORD;
 }
 
 static enum ipadb_user_auth ipadb_get_user_auth(struct ipadb_context *ipactx,
-- 
2.8.3

From fc52a8cf68a92bb22e764a834d6147fa22a88f15 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

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.

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 c38d4915aa11dc329c1b2d08949eb1141117d5ae..4cbc17b998fd52e68d3c9b6baa83b2d7e93097c5 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h
@@ -55,6 +55,9 @@
  */
 #define OTP_SYNC_REQUEST_OID "2.16.840.1.113730.3.8.10.6"
 
+/* This control has no data. */
+#define OTP_REQUIRED_OID "2.16.840.1.113730.3.8.10.7"
+
 bool otpctrl_present(Slapi_PBlock *pb, const char *oid);
 
 bool otpctrl_sync_handle(const struct otp_config *cfg, Slapi_PBlock *pb,
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
index f41b1ac9d5208cff9c6542d639a59f8c8d44e264..5c700211b59e8707fa2d1d27347c73708dbbde61 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
@@ -1172,7 +1172,7 @@ done:
  *       validation.
  */
 static bool ipapwd_pre_bind_otp(const char *bind_dn, Slapi_Entry *entry,
-                                struct berval *creds)
+                                struct berval *creds, bool otpreq)
 {
     uint32_t auth_types;
 
@@ -1204,10 +1204,10 @@ static bool ipapwd_pre_bind_otp(const char *bind_dn, Slapi_Entry *entry,
             return false;
         }
 
-        /* If the user has no active tokens, succeed. */
+        /* With no tokens, succeed if tokens aren't required. */
         if (tokens[0] == NULL) {
             otp_token_free_array(tokens);
-            return true;
+            return !otpreq;
         }
 
         if (otp_token_validate_berval(tokens, creds, NULL)) {
@@ -1218,7 +1218,7 @@ static bool ipapwd_pre_bind_otp(const char *bind_dn, Slapi_Entry *entry,
         otp_token_free_array(tokens);
     }
 
-    return auth_types & OTP_CONFIG_AUTH_TYPE_PASSWORD;
+    return (auth_types & OTP_CONFIG_AUTH_TYPE_PASSWORD) && !otpreq;
 }
 
 static int ipapwd_authenticate(const char *dn, Slapi_Entry *entry,
@@ -1394,6 +1394,7 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
     char *dn = NULL;
     int method = 0;
     bool syncreq;
+    bool otpreq;
     int ret = 0;
     time_t current_time;
     time_t expire_time;
@@ -1451,7 +1452,8 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
 
     /* Try to do OTP first. */
     syncreq = otpctrl_present(pb, OTP_SYNC_REQUEST_OID);
-    if (!syncreq && !ipapwd_pre_bind_otp(dn, entry, credentials))
+    otpreq = otpctrl_present(pb, OTP_REQUIRED_OID);
+    if (!syncreq && !ipapwd_pre_bind_otp(dn, entry, credentials, otpreq))
         goto invalid_creds;
 
     /* Ensure that there is a password. */
@@ -1488,6 +1490,7 @@ int ipapwd_pre_init(Slapi_PBlock *pb)
     int ret;
 
     slapi_register_supported_control(OTP_SYNC_REQUEST_OID, SLAPI_OPERATION_BIND);
+    slapi_register_supported_control(OTP_REQUIRED_OID, SLAPI_OPERATION_BIND);
 
     ret = slapi_pblock_set(pb, SLAPI_PLUGIN_VERSION, SLAPI_PLUGIN_VERSION_01);
     if (!ret) ret = slapi_pblock_set(pb, SLAPI_PLUGIN_DESCRIPTION, (void *)&ipapwd_plugin_desc);
-- 
2.8.3

From 872f611a2e602fb255477bf5e09414d7c03d94e8 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]

This gives us a place to handle all OTP related controls. Also,
genericize otpctrl_present() so that the OID can be specified as an
argument to the function call.

These changes are preparatory for the subsequent patches.

https://fedorahosted.org/freeipa/ticket/433
---
 daemons/ipa-slapi-plugins/ipa-pwd-extop/Makefile.am            |  2 +-
 .../ipa-slapi-plugins/ipa-pwd-extop/{syncreq.c => otpctrl.c}   |  8 ++++----
 .../ipa-slapi-plugins/ipa-pwd-extop/{syncreq.h => otpctrl.h}   | 10 +++++-----
 daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c              |  6 +++---
 4 files changed, 13 insertions(+), 13 deletions(-)
 rename daemons/ipa-slapi-plugins/ipa-pwd-extop/{syncreq.c => otpctrl.c} (94%)
 rename daemons/ipa-slapi-plugins/ipa-pwd-extop/{syncreq.h => otpctrl.h} (92%)

diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/Makefile.am b/daemons/ipa-slapi-plugins/ipa-pwd-extop/Makefile.am
index 078ff9c8476b8547db58d2cf5c5d941846a3143c..46a649187504d6c76965dc72ae0032292a560a81 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/Makefile.am
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/Makefile.am
@@ -47,7 +47,7 @@ libipa_pwd_extop_la_SOURCES = 		\
 	encoding.c			\
 	prepost.c			\
 	ipa_pwd_extop.c			\
-	syncreq.c			\
+	otpctrl.c			\
 	$(KRB5_UTIL_SRCS)		\
 	$(NULL)
 
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/syncreq.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.c
similarity index 94%
rename from daemons/ipa-slapi-plugins/ipa-pwd-extop/syncreq.c
rename to daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.c
index 3a31529f7a69c0bb24a1cc845fa4f9da981be138..ce26abeeb9115a52d0f25d7138e11ed54e907204 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/syncreq.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.c
@@ -38,19 +38,19 @@
  * END COPYRIGHT BLOCK **/
 
 #include "../libotp/otp_token.h"
-#include "syncreq.h"
+#include "otpctrl.h"
 
-bool sync_request_present(Slapi_PBlock *pb)
+bool otpctrl_present(Slapi_PBlock *pb, const char *oid)
 {
     LDAPControl **controls = NULL;
 
     if (slapi_pblock_get(pb, SLAPI_REQCONTROLS, &controls) != 0)
         return false;
 
-    return ldap_control_find(OTP_SYNC_REQUEST_OID, controls, NULL) != NULL;
+    return ldap_control_find(oid, controls, NULL) != NULL;
 }
 
-bool sync_request_handle(const struct otp_config *cfg, Slapi_PBlock *pb,
+bool otpctrl_sync_handle(const struct otp_config *cfg, Slapi_PBlock *pb,
                          const char *user_dn)
 {
     struct otp_token **tokens = NULL;
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/syncreq.h b/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h
similarity index 92%
rename from daemons/ipa-slapi-plugins/ipa-pwd-extop/syncreq.h
rename to daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h
index 98a97c4c9f6d2e6bf74f97fc93053b3aebbc7821..c38d4915aa11dc329c1b2d08949eb1141117d5ae 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/syncreq.h
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/otpctrl.h
@@ -38,8 +38,8 @@
  * END COPYRIGHT BLOCK **/
 
 
-#ifndef SYNCREQ_H_
-#define SYNCREQ_H_
+#ifndef OTPCTRL_H_
+#define OTPCTRL_H_
 
 #include "../libotp/otp_config.h"
 #include <stdbool.h>
@@ -55,9 +55,9 @@
  */
 #define OTP_SYNC_REQUEST_OID "2.16.840.1.113730.3.8.10.6"
 
-bool sync_request_present(Slapi_PBlock *pb);
+bool otpctrl_present(Slapi_PBlock *pb, const char *oid);
 
-bool sync_request_handle(const struct otp_config *cfg, Slapi_PBlock *pb,
+bool otpctrl_sync_handle(const struct otp_config *cfg, Slapi_PBlock *pb,
                          const char *user_dn);
 
-#endif /* SYNCREQ_H_ */
+#endif /* OTPCTRL_H_ */
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
index c1fc7fe3345e227054b8118b35d8f504fe957ea6..f41b1ac9d5208cff9c6542d639a59f8c8d44e264 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
@@ -62,7 +62,7 @@
 
 #include "ipapwd.h"
 #include "util.h"
-#include "syncreq.h"
+#include "otpctrl.h"
 
 #define IPAPWD_OP_NULL 0
 #define IPAPWD_OP_ADD 1
@@ -1450,7 +1450,7 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
     }
 
     /* Try to do OTP first. */
-    syncreq = sync_request_present(pb);
+    syncreq = otpctrl_present(pb, OTP_SYNC_REQUEST_OID);
     if (!syncreq && !ipapwd_pre_bind_otp(dn, entry, credentials))
         goto invalid_creds;
 
@@ -1466,7 +1466,7 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
     }
 
     /* Attempt to handle a token synchronization request. */
-    if (syncreq && !sync_request_handle(otp_config, pb, dn))
+    if (syncreq && !otpctrl_sync_handle(otp_config, pb, dn))
         goto invalid_creds;
 
     /* Attempt to write out kerberos keys for the user. */
-- 
2.8.3

-- 
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