Re: [SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

[Lukas Slebodnik]

On (02/11/15 14:41), Pavel Reichl wrote:
>
>
>On 11/02/2015 01:02 PM, Sumit Bose wrote:
>>On Thu, Oct 29, 2015 at 03:59:40PM +0100, Lukas Slebodnik wrote:
>>>On (29/10/15 15:32), Pavel Reichl wrote:
Hello,

while I worked on tests for PAM responder Sumit proposed to use attached 
patch. QA have already kindly run their tests (successfully).

I'll do the review myself (But all opinions are welcomed for sure).

Thanks
>>>
From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 19 Oct 2015 13:10:51 -0400
Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
set PAM_SUCCESSS explicitly for a successful authentication and will
really return an error in all other cases.
---
src/providers/dp_pam_data_util.c | 4 
1 file changed, 4 insertions(+)

diff --git a/src/providers/dp_pam_data_util.c 
b/src/providers/dp_pam_data_util.c
index 
10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44
 100644
--- a/src/providers/dp_pam_data_util.c
+++ b/src/providers/dp_pam_data_util.c
@@ -22,6 +22,8 @@
 along with this program.  If not, see .
*/

+#include 
+
>>>We try to avoid including non-standar pam header files.
>>>Prefix "_" is Linux PAM specific.
>>>
>>
>>Fixed, new version attached.
>>
>>bye,
>>Sumit
>
>Thank you both. Code LGTM, ci passed with exception of rawhide where it seems 
>to be a problem of package dependency which is not relevant to this patch:
>http://sssd-ci.duckdns.org/logs/job/31/81/summary.html
>

All ad forest test passed.

master:
* d0d79b53a5a16831169a3d854fd59402a99a1dd6

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

[Lukas Slebodnik]

On (02/11/15 14:41), Pavel Reichl wrote:
>On 11/02/2015 01:02 PM, Sumit Bose wrote:
>>On Thu, Oct 29, 2015 at 03:59:40PM +0100, Lukas Slebodnik wrote:
>>>On (29/10/15 15:32), Pavel Reichl wrote:
Hello,

while I worked on tests for PAM responder Sumit proposed to use attached 
patch. QA have already kindly run their tests (successfully).

I'll do the review myself (But all opinions are welcomed for sure).

Thanks
>>>
From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 19 Oct 2015 13:10:51 -0400
Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
set PAM_SUCCESSS explicitly for a successful authentication and will
really return an error in all other cases.
---
src/providers/dp_pam_data_util.c | 4 
1 file changed, 4 insertions(+)

diff --git a/src/providers/dp_pam_data_util.c 
b/src/providers/dp_pam_data_util.c
index 
10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44
 100644
--- a/src/providers/dp_pam_data_util.c
+++ b/src/providers/dp_pam_data_util.c
@@ -22,6 +22,8 @@
 along with this program.  If not, see .
*/

+#include 
+
>>>We try to avoid including non-standar pam header files.
>>>Prefix "_" is Linux PAM specific.
>>>
>>
>>Fixed, new version attached.
>>
>>bye,
>>Sumit
>
>Thank you both. Code LGTM, ci passed with exception of rawhide where it seems 
>to be a problem of package dependency which is not relevant to this patch:
>http://sssd-ci.duckdns.org/logs/job/31/81/summary.html
>
>ACK
>
I would say you ack-ed your own patch because Sumit's patch is almost the same.
So it should not count.

I will run som authentication related test (with AD)
to be sure it does not break some corner case.
Even though it should not.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

[Pavel Reichl]

On 11/02/2015 01:02 PM, Sumit Bose wrote:

On Thu, Oct 29, 2015 at 03:59:40PM +0100, Lukas Slebodnik wrote:

On (29/10/15 15:32), Pavel Reichl wrote:

Hello,

while I worked on tests for PAM responder Sumit proposed to use attached patch. 
QA have already kindly run their tests (successfully).

I'll do the review myself (But all opinions are welcomed for sure).

Thanks


>From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001

From: Sumit Bose 
Date: Mon, 19 Oct 2015 13:10:51 -0400
Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
set PAM_SUCCESSS explicitly for a successful authentication and will
really return an error in all other cases.
---
src/providers/dp_pam_data_util.c | 4 
1 file changed, 4 insertions(+)

diff --git a/src/providers/dp_pam_data_util.c b/src/providers/dp_pam_data_util.c
index 
10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44
 100644
--- a/src/providers/dp_pam_data_util.c
+++ b/src/providers/dp_pam_data_util.c
@@ -22,6 +22,8 @@
 along with this program.  If not, see .
*/

+#include 
+

We try to avoid including non-standar pam header files.
Prefix "_" is Linux PAM specific.



Fixed, new version attached.

bye,
Sumit


Thank you both. Code LGTM, ci passed with exception of rawhide where it seems 
to be a problem of package dependency which is not relevant to this patch:
http://sssd-ci.duckdns.org/logs/job/31/81/summary.html

ACK





___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

[Sumit Bose]

On Mon, Nov 02, 2015 at 04:05:33PM +0100, Lukas Slebodnik wrote:
> On (02/11/15 14:41), Pavel Reichl wrote:
> >On 11/02/2015 01:02 PM, Sumit Bose wrote:
> >>On Thu, Oct 29, 2015 at 03:59:40PM +0100, Lukas Slebodnik wrote:
> >>>On (29/10/15 15:32), Pavel Reichl wrote:
> Hello,
> 
> while I worked on tests for PAM responder Sumit proposed to use attached 
> patch. QA have already kindly run their tests (successfully).
> 
> I'll do the review myself (But all opinions are welcomed for sure).
> 
> Thanks
> >>>
> From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Mon, 19 Oct 2015 13:10:51 -0400
> Subject: [PATCH] PAM: successful authentication sets explicitly 
> PAM_SUCCESSS
> 
> Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
> set PAM_SUCCESSS explicitly for a successful authentication and will
> really return an error in all other cases.
> ---
> src/providers/dp_pam_data_util.c | 4 
> 1 file changed, 4 insertions(+)
> 
> diff --git a/src/providers/dp_pam_data_util.c 
> b/src/providers/dp_pam_data_util.c
> index 
> 10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44
>  100644
> --- a/src/providers/dp_pam_data_util.c
> +++ b/src/providers/dp_pam_data_util.c
> @@ -22,6 +22,8 @@
>  along with this program.  If not, see .
> */
> 
> +#include 
> +
> >>>We try to avoid including non-standar pam header files.
> >>>Prefix "_" is Linux PAM specific.
> >>>
> >>
> >>Fixed, new version attached.
> >>
> >>bye,
> >>Sumit
> >
> >Thank you both. Code LGTM, ci passed with exception of rawhide where it 
> >seems to be a problem of package dependency which is not relevant to this 
> >patch:
> >http://sssd-ci.duckdns.org/logs/job/31/81/summary.html
> >
> >ACK
> >
> I would say you ack-ed your own patch because Sumit's patch is almost the 
> same.
> So it should not count.

no, I was mine all the way. Pavel just send it to the list after running
tests with our QE team. Sorry for the confusion.

bye,
Sumit

> 
> I will run som authentication related test (with AD)
> to be sure it does not break some corner case.
> Even though it should not.
> 
> LS
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

[Sumit Bose]

On Thu, Oct 29, 2015 at 03:59:40PM +0100, Lukas Slebodnik wrote:
> On (29/10/15 15:32), Pavel Reichl wrote:
> >Hello,
> >
> >while I worked on tests for PAM responder Sumit proposed to use attached 
> >patch. QA have already kindly run their tests (successfully).
> >
> >I'll do the review myself (But all opinions are welcomed for sure).
> >
> >Thanks
> 
> >From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001
> >From: Sumit Bose 
> >Date: Mon, 19 Oct 2015 13:10:51 -0400
> >Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS
> >
> >Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
> >set PAM_SUCCESSS explicitly for a successful authentication and will
> >really return an error in all other cases.
> >---
> > src/providers/dp_pam_data_util.c | 4 
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/src/providers/dp_pam_data_util.c 
> >b/src/providers/dp_pam_data_util.c
> >index 
> >10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44
> > 100644
> >--- a/src/providers/dp_pam_data_util.c
> >+++ b/src/providers/dp_pam_data_util.c
> >@@ -22,6 +22,8 @@
> > along with this program.  If not, see .
> > */
> > 
> >+#include 
> >+
> We try to avoid including non-standar pam header files.
> Prefix "_" is Linux PAM specific.
> 

Fixed, new version attached.

bye,
Sumit
From fe1fce0f767f07ce4568de3cb86fc8823e5d66ec Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 19 Oct 2015 13:10:51 -0400
Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
set PAM_SUCCESSS explicitly for a successful authentication and will
really return an error in all other cases.
---
 src/providers/dp_pam_data_util.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/providers/dp_pam_data_util.c b/src/providers/dp_pam_data_util.c
index 
10e91f5f7286db5e76ad98b6c7519f2482d006db..bed5db872d18e0c3ab13d7b7fd061749253cc72a
 100644
--- a/src/providers/dp_pam_data_util.c
+++ b/src/providers/dp_pam_data_util.c
@@ -22,6 +22,8 @@
 along with this program.  If not, see .
 */
 
+#include 
+
 #include "providers/data_provider.h"
 #include "util/sss_cli_cmd.h"
 
@@ -48,6 +50,8 @@ struct pam_data *create_pam_data(TALLOC_CTX *mem_ctx)
 goto failed;
 }
 
+pd->pam_status = PAM_SYSTEM_ERR;
+
 pd->authtok = sss_authtok_new(pd);
 if (pd->authtok == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
-- 
2.1.0

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

[Pavel Reichl]

Hello,

while I worked on tests for PAM responder Sumit proposed to use attached patch. 
QA have already kindly run their tests (successfully).

I'll do the review myself (But all opinions are welcomed for sure).

Thanks
>From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 19 Oct 2015 13:10:51 -0400
Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
set PAM_SUCCESSS explicitly for a successful authentication and will
really return an error in all other cases.
---
 src/providers/dp_pam_data_util.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/providers/dp_pam_data_util.c b/src/providers/dp_pam_data_util.c
index 10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44 100644
--- a/src/providers/dp_pam_data_util.c
+++ b/src/providers/dp_pam_data_util.c
@@ -22,6 +22,8 @@
 along with this program.  If not, see .
 */
 
+#include 
+
 #include "providers/data_provider.h"
 #include "util/sss_cli_cmd.h"
 
@@ -48,6 +50,8 @@ struct pam_data *create_pam_data(TALLOC_CTX *mem_ctx)
 goto failed;
 }
 
+pd->pam_status = PAM_SYSTEM_ERR;
+
 pd->authtok = sss_authtok_new(pd);
 if (pd->authtok == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
-- 
2.4.3

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS

[Lukas Slebodnik]

On (29/10/15 15:32), Pavel Reichl wrote:
>Hello,
>
>while I worked on tests for PAM responder Sumit proposed to use attached 
>patch. QA have already kindly run their tests (successfully).
>
>I'll do the review myself (But all opinions are welcomed for sure).
>
>Thanks

>From 55e487ac403f39e8b65e9373b06059399e74b792 Mon Sep 17 00:00:00 2001
>From: Sumit Bose 
>Date: Mon, 19 Oct 2015 13:10:51 -0400
>Subject: [PATCH] PAM: successful authentication sets explicitly PAM_SUCCESSS
>
>Set PAM_SYSTEM_ERR as default pam_status to ensure that we always must
>set PAM_SUCCESSS explicitly for a successful authentication and will
>really return an error in all other cases.
>---
> src/providers/dp_pam_data_util.c | 4 
> 1 file changed, 4 insertions(+)
>
>diff --git a/src/providers/dp_pam_data_util.c 
>b/src/providers/dp_pam_data_util.c
>index 
>10e91f5f7286db5e76ad98b6c7519f2482d006db..f471c7d02f5c86d1b1a45dd152c048de0e0bbb44
> 100644
>--- a/src/providers/dp_pam_data_util.c
>+++ b/src/providers/dp_pam_data_util.c
>@@ -22,6 +22,8 @@
> along with this program.  If not, see .
> */
> 
>+#include 
>+
We try to avoid including non-standar pam header files.
Prefix "_" is Linux PAM specific.

sh$ git grep pam_types | wc -l
0

sh$ git grep pam_modules .
src/external/pam.m4:AC_CHECK_HEADERS([security/pam_appl.h 
security/pam_modules.h],
src/providers/ad/ad_access.c:#include 
src/providers/ad/ad_gpo.c:#include 
src/providers/ad/ad_gpo_child.c:#include 
src/providers/data_provider_be.c:#include 
src/providers/ipa/ipa_access.c:#include 
src/providers/ipa/ipa_auth.c:#include 
src/providers/ipa/ipa_selinux.c:#include 
src/providers/krb5/krb5_auth.c:#include 
src/providers/krb5/krb5_child.c:#include 
src/providers/krb5/krb5_delayed_online_authentication.c:#include 

src/providers/krb5/krb5_renew_tgt.c:#include 
src/providers/krb5/krb5_wait_queue.c:#include 
src/providers/ldap/ldap_access.c:#include 
src/providers/ldap/ldap_auth.c:#include 
src/providers/ldap/sdap_access.c:#include 
src/providers/proxy/proxy.h:#include 
src/providers/proxy/proxy_child.c:#include 
src/providers/simple/simple_access.c:#include 
src/responder/pam/pam_LOCAL_domain.c:#include 
src/responder/pam/pamsrv_dp.c:#include 
src/sss_client/common.c:#include 
src/sss_client/pam_message.c:#include 
src/sss_client/pam_sss.c:#include 
src/tests/cmocka/test_krb5_wait_queue.c:#include 
src/tests/cmocka/test_pam_srv.c:#include 

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel