[PATCH] PAM/KRB5: optional otp and password prompting

[Sumit Bose]

Hi,

this patch is the SSSD part of the Authentication Indicator related
changes in FreeIPA. The basic part is that now it is possible to
authenticate at will with either password or 2FA as described on
https://fedorahosted.org/sssd/wiki/DesignDocs/PromptingForMultipleAuthent....

To test you need a FreeIPA server build from the FreeIPA master branch
and optionally Nathaniel's '[PATCH 0093] Enable service authentication
indicator management' which is currently under review for the kvno
related test. Additionally you need MIT Kerberos packages which contain
the fix for https://bugzilla.redhat.com//show_bug.cgi?id=1340304.

Since this patch changes how libkrb5 gets the password for password
authentication with newer version of MIT Kerberos it would be nice if
someone can run some regression tests with the AD or plain KRB5
auth_provider.

bye,
Sumit

[Lukas Slebodnik]

On (30/05/16 17:07), Sumit Bose wrote:
> Hi,
>
> this patch is the SSSD part of the Authentication Indicator related
> changes in FreeIPA. The basic part is that now it is possible to
> authenticate at will with either password or 2FA as described on
> https://fedorahosted.org/sssd/wiki/DesignDocs/PromptingForMultipleAuthent....
>
> To test you need a FreeIPA server build from the FreeIPA master branch
> and optionally Nathaniel's '[PATCH 0093] Enable service authentication
> indicator management' which is currently under review for the kvno
> related test. Additionally you need MIT Kerberos packages which contain
> the fix for https://bugzilla.redhat.com//show_bug.cgi?id=1340304.
>
> Since this patch changes how libkrb5 gets the password for password
> authentication with newer version of MIT Kerberos it would be nice if
> someone can run some regression tests with the AD or plain KRB5
> auth_provider.
>  
I can see failures in password change for AD provider.
I haven't tried KRB5 or IPA yet.


> From 31f0e621fa63764f9f89f8d6e01f94c49675216f Mon Sep 17 00:00:00
> 2001
> From: Sumit Bose <sbose(a)redhat.com&gt;
> Date: Thu, 26 May 2016 13:20:59 +0200
> Subject: [PATCH] PAM/KRB5: optional otp and password prompting
>
> Depending on the available Kerberos pre-authentication methods pam_sss
> will prompt the user for a password, 2 authentication factors or both.
>
> Resolves https://fedorahosted.org/sssd/ticket/2988
> ---
>  src/providers/krb5/krb5_child.c | 76 +++++++++++++++++++++++++++++++++++++++--
>  src/sss_client/pam_message.h    |  2 ++
>  src/sss_client/pam_sss.c        | 15 ++++++--
>  src/sss_client/sss_cli.h        |  5 +++
>  4 files changed, 93 insertions(+), 5 deletions(-)
>  
//snip

>  static krb5_error_code sss_krb5_prompter(krb5_context context, void
> *data,
>                                           const char *name, const char *banner,
>                                           int num_prompts, krb5_prompt prompts[])
>  {
>      int ret;
> +    size_t c;
>      struct krb5_req *kr = talloc_get_type(data, struct krb5_req);
>  
> +    DEBUG(SSSDBG_TRACE_ALL,
> +          "sss_krb5_prompter name [%s] banner [%s] num_prompts [%d]
> EINVAL.\n",
> +          name, banner, num_prompts);
> +
>      if (num_prompts != 0) {
>          DEBUG(SSSDBG_CRIT_FAILURE, "Cannot handle password prompts.\n");
> +        if (DEBUG_IS_SET(SSSDBG_TRACE_ALL)) {
> +            for (c = 0; c < num_prompts; c++) {
> +                DEBUG(SSSDBG_TRACE_ALL, "Prompt [%u][%s].\n", c, 
        
                                          ^^^
                                        type of "c" is size_t an therefore
                                        there should be "%zu". otherwise
                                        there is a warning on 64 bit platforms
  CC       src/providers/krb5/krb5_child-krb5_child.o
../sssd/src/providers/krb5/krb5_child.c: In function ‘sss_krb5_prompter’:
../sssd/src/providers/krb5/krb5_child.c:665:17: error: format ‘%u’ expects argument of
type ‘unsigned int’, but argument 6 has type ‘size_t {aka long unsigned int}’
[-Werror=format=]
                 DEBUG(SSSDBG_TRACE_ALL, "Prompt [%u][%s].\n", c,
                 ^~~~~
cc1: all warnings being treated as errors

LS

[Sumit Bose]

On Fri, Jun 10, 2016 at 02:28:16PM +0200, Lukas Slebodnik wrote:
>  On (30/05/16 17:07), Sumit Bose wrote:
>  >Hi,
>  >
>  >this patch is the SSSD part of the Authentication Indicator related
>  >changes in FreeIPA. The basic part is that now it is possible to
>  >authenticate at will with either password or 2FA as described on
>
> >https://fedorahosted.org/sssd/wiki/DesignDocs/PromptingForMultipleAuthenticationTypes.
>  >
>  >To test you need a FreeIPA server build from the FreeIPA master branch
>  >and optionally Nathaniel's '[PATCH 0093] Enable service authentication
>  >indicator management' which is currently under review for the kvno
>  >related test. Additionally you need MIT Kerberos packages which contain
>  >the fix for https://bugzilla.redhat.com//show_bug.cgi?id=1340304.
>  >
>  >Since this patch changes how libkrb5 gets the password for password
>  >authentication with newer version of MIT Kerberos it would be nice if
>  >someone can run some regression tests with the AD or plain KRB5
>  >auth_provider.
>  >
>  I can see failures in password change for AD provider.
>  I haven't tried KRB5 or IPA yet.
>   
Thank you for testing and reviewing. I guess it was a general issue, the
new patch should fix it.

>   
...
>  >+                DEBUG(SSSDBG_TRACE_ALL, "Prompt
> [%u][%s].\n", c,
>                                                     ^^^
>                                          type of "c" is size_t an therefore
>                                          there should be "%zu". otherwise
>                                          there is a warning on 64 bit platforms
>    CC       src/providers/krb5/krb5_child-krb5_child.o
>  ../sssd/src/providers/krb5/krb5_child.c: In function ‘sss_krb5_prompter’:
>  ../sssd/src/providers/krb5/krb5_child.c:665:17: error: format ‘%u’ expects argument of
> type ‘unsigned int’, but argument 6 has type ‘size_t {aka long unsigned int}’
> [-Werror=format=]
>                   DEBUG(SSSDBG_TRACE_ALL, "Prompt [%u][%s].\n", c,
>                   ^~~~~
>  cc1: all warnings being treated as errors 
Fixed as well. New version attached.

bye,
Sumit

[Sumit Bose]

and now with patch ...

On Mon, Jun 13, 2016 at 02:00:37PM +0200, Sumit Bose wrote:
>  On Fri, Jun 10, 2016 at 02:28:16PM +0200, Lukas Slebodnik wrote:
>  > On (30/05/16 17:07), Sumit Bose wrote:
>  > >Hi,
>  > >
>  > >this patch is the SSSD part of the Authentication Indicator related
>  > >changes in FreeIPA. The basic part is that now it is possible to
>  > >authenticate at will with either password or 2FA as described on
>  >
> >https://fedorahosted.org/sssd/wiki/DesignDocs/PromptingForMultipleAuthenticationTypes.
>  > >
>  > >To test you need a FreeIPA server build from the FreeIPA master branch
>  > >and optionally Nathaniel's '[PATCH 0093] Enable service authentication
>  > >indicator management' which is currently under review for the kvno
>  > >related test. Additionally you need MIT Kerberos packages which contain
>  > >the fix for https://bugzilla.redhat.com//show_bug.cgi?id=1340304.
>  > >
>  > >Since this patch changes how libkrb5 gets the password for password
>  > >authentication with newer version of MIT Kerberos it would be nice if
>  > >someone can run some regression tests with the AD or plain KRB5
>  > >auth_provider.
>  > >
>  > I can see failures in password change for AD provider.
>  > I haven't tried KRB5 or IPA yet.
>  > 
>  
>  Thank you for testing and reviewing. I guess it was a general issue, the
>  new patch should fix it.
>  
>  > 
>  ...
>  > >+                DEBUG(SSSDBG_TRACE_ALL, "Prompt [%u][%s].\n", c,
>  >                                                    ^^^
>  >                                         type of "c" is size_t an
> therefore
>  >                                         there should be "%zu". otherwise
>  >                                         there is a warning on 64 bit platforms
>  >   CC       src/providers/krb5/krb5_child-krb5_child.o
>  > ../sssd/src/providers/krb5/krb5_child.c: In function ‘sss_krb5_prompter’:
>  > ../sssd/src/providers/krb5/krb5_child.c:665:17: error: format ‘%u’ expects argument
> of type ‘unsigned int’, but argument 6 has type ‘size_t {aka long unsigned int}’
> [-Werror=format=]
>  >                  DEBUG(SSSDBG_TRACE_ALL, "Prompt [%u][%s].\n", c,
>  >                  ^~~~~
>  > cc1: all warnings being treated as errors
>  
>  Fixed as well. New version attached.
>  
>  bye,
>  Sumit

[Lukas Slebodnik]

On (13/06/16 14:01), Sumit Bose wrote:
> and now with patch ...
>
> On Mon, Jun 13, 2016 at 02:00:37PM +0200, Sumit Bose wrote:
> > On Fri, Jun 10, 2016 at 02:28:16PM +0200, Lukas Slebodnik wrote:
> > > On (30/05/16 17:07), Sumit Bose wrote:
> > > >Hi,
> > > >
> > > >this patch is the SSSD part of the Authentication Indicator related
> > > >changes in FreeIPA. The basic part is that now it is possible to
> > > >authenticate at will with either password or 2FA as described on
> > >
> >https://fedorahosted.org/sssd/wiki/DesignDocs/PromptingForMultipleAuthenticationTypes.
> > > >
> > > >To test you need a FreeIPA server build from the FreeIPA master branch
> > > >and optionally Nathaniel's '[PATCH 0093] Enable service
> authentication
> > > >indicator management' which is currently under review for the kvno
> > > >related test. Additionally you need MIT Kerberos packages which contain
> > > >the fix for https://bugzilla.redhat.com//show_bug.cgi?id=1340304.
> > > >
> > > >Since this patch changes how libkrb5 gets the password for password
> > > >authentication with newer version of MIT Kerberos it would be nice if
> > > >someone can run some regression tests with the AD or plain KRB5
> > > >auth_provider.
> > > >
> > > I can see failures in password change for AD provider.
> > > I haven't tried KRB5 or IPA yet.
> > > 
> > 
> > Thank you for testing and reviewing. I guess it was a general issue, the
> > new patch should fix it.
> > 
> > > 
> > ...
> > > >+                DEBUG(SSSDBG_TRACE_ALL, "Prompt [%u][%s].\n", c,
> > >                                                    ^^^
> > >                                         type of "c" is size_t an
> therefore
> > >                                         there should be "%zu".
> otherwise
> > >                                         there is a warning on 64 bit platforms
> > >   CC       src/providers/krb5/krb5_child-krb5_child.o
> > > ../sssd/src/providers/krb5/krb5_child.c: In function ‘sss_krb5_prompter’:
> > > ../sssd/src/providers/krb5/krb5_child.c:665:17: error: format ‘%u’ expects
> argument of type ‘unsigned int’, but argument 6 has type ‘size_t {aka long unsigned int}’
> [-Werror=format=]
> > >                  DEBUG(SSSDBG_TRACE_ALL, "Prompt [%u][%s].\n", c,
> > >                  ^~~~~
> > > cc1: all warnings being treated as errors
> > 
> > Fixed as well. New version attached.
> > 
> > bye,
> > Sumit 
> From 28b5cd4eb73cee9401eefd7d0232c5dc69e69f8c Mon Sep 17 00:00:00
> 2001
> From: Sumit Bose <sbose(a)redhat.com&gt;
> Date: Thu, 26 May 2016 13:20:59 +0200
> Subject: [PATCH] PAM/KRB5: optional otp and password prompting
>
> Depending on the available Kerberos pre-authentication methods pam_sss
> will prompt the user for a password, 2 authentication factors or both.
>
> Resolves https://fedorahosted.org/sssd/ticket/2988
> --- 
Bug with password change is fixed
There aren't any newly introduced isues reported by static analyzers.
All AD tests passed.

I didn't try OTP becuase I do not have such setup
and I need to work on other thing.
If someone could help I will appreciate it.

LS

[Nathaniel McCallum]

Patch WFM and looks clean. My only question is on the overall
prompting.

Currently we have:
  * 1FA:
    - "Password"
  * 2FA (required):
    - "First Factor"
    - "Second Factor"
  * 2FA (optional):
    - "First Factor or Password"
    - "Second Factor, press return for Password authentication"

This all seems a little verbose to me. It also seems like it reveals
technical details that don't matter and/or might confuse the user.

Is there a reason not to do the following?

  * 1FA:
    - "Password"
  * 2FA (required):
    - "Password"
    - "Token
Code"
  * 2FA (optional):
    - "Password"
    - "Token Code (optional)"

This seems to me simpler, shorter and more descriptive. Alternatively:

  * 1FA:
    - "Password"
  * 2FA (required):
    - "Password"
    -
"Second Factor"
  * 2FA (optional):
    - "Password"
    - "Second Factor
(optional)"

Whatever we decide to do, this patch has my ACK. So feel free to just
change the string and merge it.

On Mon, 2016-06-13 at 14:01 +0200, Sumit Bose wrote:
> and now with patch ...
> 
> On Mon, Jun 13, 2016 at 02:00:37PM +0200, Sumit Bose wrote:
> > 
> > On Fri, Jun 10, 2016 at 02:28:16PM +0200, Lukas Slebodnik wrote:
> > > 
> > > On (30/05/16 17:07), Sumit Bose wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > this patch is the SSSD part of the Authentication Indicator
> > > > related
> > > > changes in FreeIPA. The basic part is that now it is possible
> > > > to
> > > > authenticate at will with either password or 2FA as described
> > > > on
> > > > https://fedorahosted.org/sssd/wiki/DesignDocs/PromptingForMulti
> > > > pleAuthenticationTypes.
> > > > 
> > > > To test you need a FreeIPA server build from the FreeIPA master
> > > > branch
> > > > and optionally Nathaniel's '[PATCH 0093] Enable service
> > > > authentication
> > > > indicator management' which is currently under review for the
> > > > kvno
> > > > related test. Additionally you need MIT Kerberos packages which
> > > > contain
> > > > the fix for https://bugzilla.redhat.com//show_bug.cgi?id=134030
> > > > 4.
> > > > 
> > > > Since this patch changes how libkrb5 gets the password for
> > > > password
> > > > authentication with newer version of MIT Kerberos it would be
> > > > nice if
> > > > someone can run some regression tests with the AD or plain KRB5
> > > > auth_provider.
> > > > 
> > > I can see failures in password change for AD provider.
> > > I haven't tried KRB5 or IPA yet.
> > > 
> > 
> > Thank you for testing and reviewing. I guess it was a general
> > issue, the
> > new patch should fix it.
> > 
> > > 
> > > 
> > ...
> > > 
> > > > 
> > > > +                DEBUG(SSSDBG_TRACE_ALL, "Prompt
[%u][%s].\n",
> > > > c,
> > >                                                    ^^^
> > >                                         type of "c" is size_t an
> > > therefore
> > >                                         there should be "%zu".
> > > otherwise
> > >                                         there is a warning on 64
> > > bit platforms
> > >   CC       src/providers/krb5/krb5_child-krb5_child.o
> > > ../sssd/src/providers/krb5/krb5_child.c: In function
> > > ‘sss_krb5_prompter’:
> > > ../sssd/src/providers/krb5/krb5_child.c:665:17: error: format
> > > ‘%u’ expects argument of type ‘unsigned int’, but argument 6 has
> > > type ‘size_t {aka long unsigned int}’ [-Werror=format=]
> > >                  DEBUG(SSSDBG_TRACE_ALL, "Prompt [%u][%s].\n",
c,
> > >                  ^~~~~
> > > cc1: all warnings being treated as errors
> > 
> > Fixed as well. New version attached.
> > 
> > bye,
> > Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/[e-mail]
> ted.org

[Sumit Bose]

On Wed, Jul 06, 2016 at 09:03:45AM -0400, Nathaniel McCallum wrote:
>  Patch WFM and looks clean. My only question is on the overall 
Thank you very much for testing.

>  prompting.
>  
>  Currently we have:
>    * 1FA:
>      - "Password"
>    * 2FA (required):
>      - "First Factor"
>      - "Second Factor"
>    * 2FA (optional):
>      - "First Factor or Password"
>      - "Second Factor, press return for Password authentication"
>  
>  This all seems a little verbose to me. It also seems like it reveals
>  technical details that don't matter and/or might confuse the user.
>  
>  Is there a reason not to do the following?
>  
>    * 1FA:
>      - "Password"
>    * 2FA (required):
>      - "Password"
>      - "Token
>  Code"
>    * 2FA (optional):
>      - "Password"
>      - "Token Code (optional)"
>  
>  This seems to me simpler, shorter and more descriptive. Alternatively:
>  
>    * 1FA:
>      - "Password"
>    * 2FA (required):
>      - "Password"
>      -
>  "Second Factor"
>    * 2FA (optional):
>      - "Password"
>      - "Second Factor
>  (optional)"
>  
>  Whatever we decide to do, this patch has my ACK. So feel free to just
>  change the string and merge it. 
When the 2FA prompting was added in the previous release I think I asked
for alternative suggestions as well. My main reasons, especially with
the 'First Factor' prompt were first to make the user aware that
something new is happen. New in the sense that the user should be
prepared to enter the second factor in a new prompt and not put the
combined password in the 'Password' prompt as he was used to before.

And second I wasn't sure if users are aware that the first factor is
typically their long term password. Iirc the first factor with RSA
tokens are typically referred to as PIN and asking for a password might
confuse the users.

Maybe we should reach out to UX designers to see what they recommend?

bye,
Sumit

> 
> On Mon, 2016-06-13 at 14:01 +0200, Sumit Bose wrote:
> > and now with patch ...
> > 
> > On Mon, Jun 13, 2016 at 02:00:37PM +0200, Sumit Bose wrote:
> > > 
> > > On Fri, Jun 10, 2016 at 02:28:16PM +0200, Lukas Slebodnik wrote:
> > > > 
> > > > On (30/05/16 17:07), Sumit Bose wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > this patch is the SSSD part of the Authentication Indicator
> > > > > related
> > > > > changes in FreeIPA. The basic part is that now it is possible
> > > > > to
> > > > > authenticate at will with either password or 2FA as described
> > > > > on
> > > > > https://fedorahosted.org/sssd/wiki/DesignDocs/PromptingForMulti
> > > > > pleAuthenticationTypes.
> > > > > 
> > > > > To test you need a FreeIPA server build from the FreeIPA master
> > > > > branch
> > > > > and optionally Nathaniel's '[PATCH 0093] Enable service
> > > > > authentication
> > > > > indicator management' which is currently under review for
the
> > > > > kvno
> > > > > related test. Additionally you need MIT Kerberos packages which
> > > > > contain
> > > > > the fix for https://bugzilla.redhat.com//show_bug.cgi?id=134030
> > > > > 4.
> > > > > 
> > > > > Since this patch changes how libkrb5 gets the password for
> > > > > password
> > > > > authentication with newer version of MIT Kerberos it would be
> > > > > nice if
> > > > > someone can run some regression tests with the AD or plain KRB5
> > > > > auth_provider.
> > > > > 
> > > > I can see failures in password change for AD provider.
> > > > I haven't tried KRB5 or IPA yet.
> > > > 
> > > 
> > > Thank you for testing and reviewing. I guess it was a general
> > > issue, the
> > > new patch should fix it.
> > > 
> > > > 
> > > > 
> > > ...
> > > > 
> > > > > 
> > > > > +                DEBUG(SSSDBG_TRACE_ALL, "Prompt
[%u][%s].\n",
> > > > > c,
> > > >                                                    ^^^
> > > >                                         type of "c" is
size_t an
> > > > therefore
> > > >                                         there should be
"%zu".
> > > > otherwise
> > > >                                         there is a warning on 64
> > > > bit platforms
> > > >   CC       src/providers/krb5/krb5_child-krb5_child.o
> > > > ../sssd/src/providers/krb5/krb5_child.c: In function
> > > > ‘sss_krb5_prompter’:
> > > > ../sssd/src/providers/krb5/krb5_child.c:665:17: error: format
> > > > ‘%u’ expects argument of type ‘unsigned int’, but argument 6 has
> > > > type ‘size_t {aka long unsigned int}’ [-Werror=format=]
> > > >                  DEBUG(SSSDBG_TRACE_ALL, "Prompt
[%u][%s].\n", c,
> > > >                  ^~~~~
> > > > cc1: all warnings being treated as errors
> > > 
> > > Fixed as well. New version attached.
> > > 
> > > bye,
> > > Sumit
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel(a)lists.fedorahosted.org
> > https://lists.fedorahosted.org/admin/lists/[e-mail]
> > ted.org