[sssd PR#328][opened] KRB5: Return invalid credentials internally when attempting to renew an expired TGT

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/328
Author: jhrozek
 Title: #328: KRB5: Return invalid credentials internally when attempting to renew an
expired TGT
Action: opened

PR body:
"""
Since 1.14.2 and in particular commit
d3348f49260998880bb7cd3b2fb72d562b1b7a64 we return ERR_NETWORK_IO for any
krb5_child operations that receive KRB5KRB_AP_ERR_TKT_EXPIRED from libkrb5.

However, when the action that krb5_child performs is ticket renewal and the
ticket is totally expired, this can send the SSSD into offline mode.

Instead, this patch converts the KRB5KRB_AP_ERR_TKT_EXPIRED code into
sssd-internal ERR_CREDS_EXPIRED which map_krb5_error() won't map anymore.

The effect on the deamon is that just the single renewal fails, but the
failover code is not called and therefore sssd doesn't switch into offline
mode.

Resolves: https://pagure.io/SSSD/sssd/issue/3406
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/328/head:pr328
git checkout pr328

[sssd PR#328][comment] KRB5: Return invalid credentials internally when attempting to renew an expired TGT

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/328
Title: #328: KRB5: Return invalid credentials internally when attempting to renew an
expired TGT

lslebodn commented:
"""
On (20/07/17 00:59), Jakub Hrozek wrote:
> Since 1.14.2 and in particular commit
> d3348f49260998880bb7cd3b2fb72d562b1b7a64 we return ERR_NETWORK_IO for any
> krb5_child operations that receive KRB5KRB_AP_ERR_TKT_EXPIRED from libkrb5.
>
> However, when the action that krb5_child performs is ticket renewal and the
> ticket is totally expired, this can send the SSSD into offline mode.
>
> Instead, this patch converts the KRB5KRB_AP_ERR_TKT_EXPIRED code into
> sssd-internal ERR_CREDS_EXPIRED which map_krb5_error() won't map anymore.
>
> The effect on the deamon is that just the single renewal fails, but the
> failover code is not called and therefore sssd doesn't switch into offline
> mode.
>  
LGTM,

I will try to run some downstream tests.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/328#issuecomment-316639067

[sssd PR#328][comment] KRB5: Return invalid credentials internally when attempting to renew an expired TGT

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/328
Title: #328: KRB5: Return invalid credentials internally when attempting to renew an
expired TGT

jhrozek commented:
"""
On Thu, Jul 20, 2017 at 01:47:49AM -0700, lslebodn wrote:
>  On (20/07/17 00:59), Jakub Hrozek wrote:
>  >Since 1.14.2 and in particular commit
>  >d3348f49260998880bb7cd3b2fb72d562b1b7a64 we return ERR_NETWORK_IO for any
>  >krb5_child operations that receive KRB5KRB_AP_ERR_TKT_EXPIRED from libkrb5.
>  >
>  >However, when the action that krb5_child performs is ticket renewal and the
>  >ticket is totally expired, this can send the SSSD into offline mode.
>  >
>  >Instead, this patch converts the KRB5KRB_AP_ERR_TKT_EXPIRED code into
>  >sssd-internal ERR_CREDS_EXPIRED which map_krb5_error() won't map anymore.
>  >
>  >The effect on the deamon is that just the single renewal fails, but the
>  >failover code is not called and therefore sssd doesn't switch into offline
>  >mode.
>  >
>  LGTM,
>  
>  I will try to run some downstream tests. 
Thank you. I'm also interested if this feels like too much of a hack.
During development, I was wondering whether we should add a new return
code instead of abusing ERR_CREDS_EXPIRED, but I think it's fine for
effectivelly handling a failure case..

"""

See the full comment at https://github.com/SSSD/sssd/pull/328#issuecomment-316640068

[sssd PR#328][comment] KRB5: Return invalid credentials internally when attempting to renew an expired TGT

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/328
Title: #328: KRB5: Return invalid credentials internally when attempting to renew an
expired TGT

lslebodn commented:
"""
On (20/07/17 01:51), Jakub Hrozek wrote:
> On Thu, Jul 20, 2017 at 01:47:49AM -0700, lslebodn wrote:
> > On (20/07/17 00:59), Jakub Hrozek wrote:
> > >Since 1.14.2 and in particular commit
> > >d3348f49260998880bb7cd3b2fb72d562b1b7a64 we return ERR_NETWORK_IO for any
> > >krb5_child operations that receive KRB5KRB_AP_ERR_TKT_EXPIRED from libkrb5.
> > >
> > >However, when the action that krb5_child performs is ticket renewal and the
> > >ticket is totally expired, this can send the SSSD into offline mode.
> > >
> > >Instead, this patch converts the KRB5KRB_AP_ERR_TKT_EXPIRED code into
> > >sssd-internal ERR_CREDS_EXPIRED which map_krb5_error() won't map anymore.
> > >
> > >The effect on the deamon is that just the single renewal fails, but the
> > >failover code is not called and therefore sssd doesn't switch into offline
> > >mode.
> > >
> > LGTM,
> > 
> > I will try to run some downstream tests.
>
> Thank you. I'm also interested if this feels like too much of a hack.
> During development, I was wondering whether we should add a new return
> code instead of abusing ERR_CREDS_EXPIRED, but I think it's fine for
> effectivelly handling a failure case..
>  
I ran krb5 related downstream tests and I could not find any failure.
So +1

BTW have you tried test-cases for https://pagure.io/SSSD/sssd/issue/3174
Because I could not find automated test for it. (at least quick git grep
didn't reveal anything.)

BTW new return code might help but IMHO it would require bigger change.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/328#issuecomment-317389131

[sssd PR#328][comment] KRB5: Return invalid credentials internally when attempting to renew an expired TGT

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/328
Title: #328: KRB5: Return invalid credentials internally when attempting to renew an
expired TGT

jhrozek commented:
"""
Yes, I tested that we switch to offline mode with a clock skew so bad it triggers a
libkrb5 error:
```
(Mon Jul 24 19:36:04 2017) [[sssd[krb5_child[11924]]]] [map_krb5_error] (0x0020): 1657:
[-1765328352][Ticket expired]
(Mon Jul 24 19:36:04 2017) [[sssd[krb5_child[11924]]]] [k5c_send_data] (0x0200): Received
error code 1432158229
(Mon Jul 24 19:36:04 2017) [[sssd[krb5_child[11924]]]] [pack_response_packet] (0x2000):
response packet size: [4]
(Mon Jul 24 19:36:04 2017) [[sssd[krb5_child[11924]]]] [k5c_send_data] (0x4000): Response
sent.
(Mon Jul 24 19:36:04 2017) [[sssd[krb5_child[11924]]]] [main] (0x0400): krb5_child
completed successfully
(Mon Jul 24 19:36:04 2017) [sssd[be[ipa.test]]] [read_pipe_handler] (0x0400): EOF
received, client finished
(Mon Jul 24 19:36:04 2017) [sssd[be[ipa.test]]] [_be_fo_set_port_status] (0x8000): Setting
status: PORT_NOT_WORKING. Called from: /sssd/src/providers/krb5/krb5_auth.c:
krb5_auth_done: 986
(Mon Jul 24 19:36:04 2017) [sssd[be[ipa.test]]] [fo_set_port_status] (0x0100): Marking
port 0 of server 'unidirect.ipa.test' as 'not working'
(Mon Jul 24 19:36:04 2017) [sssd[be[ipa.test]]] [fo_set_port_status] (0x0400): Marking
port 0 of duplicate server 'unidirect.ipa.test' as 'not working'
```

There is no automated test yet. We really need to merge the pam_wrapper integration --
once we have that, it should be trivial to expand the KDC tests we already have because of
KCM to also call into a pam_wrapper conversation to trigger a krb5_auth request. But
I'm not sure it would be easy to simulate a clock skew without mocking quite a few
interfaces or true multi-host tests.

If you're OK with pushing the patches, please add the Accepted label..
"""

See the full comment at https://github.com/SSSD/sssd/pull/328#issuecomment-317532064

[sssd PR#328][comment] KRB5: Return invalid credentials internally when attempting to renew an expired TGT

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/328
Title: #328: KRB5: Return invalid credentials internally when attempting to renew an
expired TGT

lslebodn commented:
"""
master:
* 865cbab7db1458422033bbd19197516110b9deca

sssd-1-14:
* b74f6711ac629643bb3d0d603c705a6f542e3b2b
"""

See the full comment at https://github.com/SSSD/sssd/pull/328#issuecomment-317539251

[sssd PR#328][closed] KRB5: Return invalid credentials internally when attempting to renew an expired TGT

[lslebodn]

   URL: https://github.com/SSSD/sssd/pull/328
Author: jhrozek
 Title: #328: KRB5: Return invalid credentials internally when attempting to renew an
expired TGT
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/328/head:pr328
git checkout pr328

[sssd PR#328][+Pushed] KRB5: Return invalid credentials internally when attempting to renew an expired TGT

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/328
Title: #328: KRB5: Return invalid credentials internally when attempting to renew an
expired TGT

Label: +Pushed

[sssd PR#328][comment] KRB5: Return invalid credentials internally when attempting to renew an expired TGT

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/328
Title: #328: KRB5: Return invalid credentials internally when attempting to renew an
expired TGT

lslebodn commented:
"""
On (24/07/17 19:40), Jakub Hrozek wrote:
> Yes, I tested that we switch to offline mode with a clock skew so bad
> it triggers a libkrb5 error:
> ```
> (Mon Jul 24 19:36:04 2017) [[sssd[krb5_child[11924]]]] [map_krb5_error] (0x0020): 1657:
> [-1765328352][Ticket expired]
> (Mon Jul 24 19:36:04 2017) [[sssd[krb5_child[11924]]]] [k5c_send_data] (0x0200): Received
> error code 1432158229
> (Mon Jul 24 19:36:04 2017) [[sssd[krb5_child[11924]]]] [pack_response_packet] (0x2000):
> response packet size: [4]
> (Mon Jul 24 19:36:04 2017) [[sssd[krb5_child[11924]]]] [k5c_send_data] (0x4000): Response
> sent.
> (Mon Jul 24 19:36:04 2017) [[sssd[krb5_child[11924]]]] [main] (0x0400): krb5_child
> completed successfully
> (Mon Jul 24 19:36:04 2017) [sssd[be[ipa.test]]] [read_pipe_handler] (0x0400): EOF
> received, client finished
> (Mon Jul 24 19:36:04 2017) [sssd[be[ipa.test]]] [_be_fo_set_port_status] (0x8000): Setting
> status: PORT_NOT_WORKING. Called from: /sssd/src/providers/krb5/krb5_auth.c:
> krb5_auth_done: 986
> (Mon Jul 24 19:36:04 2017) [sssd[be[ipa.test]]] [fo_set_port_status] (0x0100): Marking
> port 0 of server 'unidirect.ipa.test' as 'not working'
> (Mon Jul 24 19:36:04 2017) [sssd[be[ipa.test]]] [fo_set_port_status] (0x0400): Marking
> port 0 of duplicate server 'unidirect.ipa.test' as 'not working'
> ```
>
> There is no automated test yet. We really need to merge the pam_wrapper integration
Do you mean patches which require changes? :-)
https://lists.fedorahosted.org/archives/list/[e-mail]...

> -- once we have that, it should be trivial to expand the KDC tests we
> already have because of KCM to also call into a pam_wrapper conversation to trigger a
> krb5_auth request. But I'm not sure it would be easy to simulate a clock skew without
> mocking quite a few interfaces or true multi-host tests.
>  
Agree

> If you're OK with pushing the patches, please add the Accepted
> label..
>  
I was jut waiting for confirmation about original ticket with clock skew.

And patches were already pushed

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/328#issuecomment-317555632