At this moment we will support only asterisk, designating "all services".
https://fedorahosted.org/sssd/ticket/1360
Thanks Jan
On Thu, May 31, 2012 at 09:17:18PM +0200, Jan Zeleny wrote:
At this moment we will support only asterisk, designating "all services".
https://fedorahosted.org/sssd/ticket/1360
Thanks Jan
Nack, you need to initialize services to NULL, otherwise if any operation before the strdup failed, you would free random pointer.
You can also use sizeof(ALL_SERVICES)-1 and avoid defining ALL_SERVICES_LEN (and be safe if ALL_SERVICES changed, not that it's likely).
Does the SELinux feature work at all without the patch? If not, we should consider moving the ticket to 1.8
On Thu, May 31, 2012 at 09:17:18PM +0200, Jan Zeleny wrote:
At this moment we will support only asterisk, designating "all services".
https://fedorahosted.org/sssd/ticket/1360
Thanks Jan
Nack, you need to initialize services to NULL, otherwise if any operation before the strdup failed, you would free random pointer.
Good catch, fixed.
You can also use sizeof(ALL_SERVICES)-1 and avoid defining ALL_SERVICES_LEN (and be safe if ALL_SERVICES changed, not that it's likely).
As you have said, the constant is not likely to be changed. The next planned change is not to use the constant at all and rather dynamically fill this in by PAM responder. Hence I think leaving this is ok.
Does the SELinux feature work at all without the patch? If not, we should consider moving the ticket to 1.8
The original documentation for the feature was incorrect so it didn't work (or rather it did work but pam_selinux got all confused). This time I tested the feature and it is working. There are still some rough edges but I suppose they are a part of intended behaviour of pam_selinux.
To sum up, yes, this should be also backported to 1.8. Patch attached and tested.
Thanks Jan
On Thu, Jun 07, 2012 at 11:47:35AM +0200, Jan Zelený wrote:
On Thu, May 31, 2012 at 09:17:18PM +0200, Jan Zeleny wrote:
At this moment we will support only asterisk, designating "all services".
https://fedorahosted.org/sssd/ticket/1360
Thanks Jan
Nack, you need to initialize services to NULL, otherwise if any operation before the strdup failed, you would free random pointer.
Good catch, fixed.
You can also use sizeof(ALL_SERVICES)-1 and avoid defining ALL_SERVICES_LEN (and be safe if ALL_SERVICES changed, not that it's likely).
As you have said, the constant is not likely to be changed. The next planned change is not to use the constant at all and rather dynamically fill this in by PAM responder. Hence I think leaving this is ok.
Does the SELinux feature work at all without the patch? If not, we should consider moving the ticket to 1.8
The original documentation for the feature was incorrect so it didn't work (or rather it did work but pam_selinux got all confused). This time I tested the feature and it is working. There are still some rough edges but I suppose they are a part of intended behaviour of pam_selinux.
To sum up, yes, this should be also backported to 1.8. Patch attached and tested.
Sorry, one more thing I didn't notice before, can you move zeroing the errno right before sss_atomic_write_s() ?
On Thu, Jun 07, 2012 at 11:47:35AM +0200, Jan Zelený wrote:
On Thu, May 31, 2012 at 09:17:18PM +0200, Jan Zeleny wrote:
At this moment we will support only asterisk, designating "all services".
https://fedorahosted.org/sssd/ticket/1360
Thanks Jan
Nack, you need to initialize services to NULL, otherwise if any operation before the strdup failed, you would free random pointer.
Good catch, fixed.
You can also use sizeof(ALL_SERVICES)-1 and avoid defining ALL_SERVICES_LEN (and be safe if ALL_SERVICES changed, not that it's likely).
As you have said, the constant is not likely to be changed. The next planned change is not to use the constant at all and rather dynamically fill this in by PAM responder. Hence I think leaving this is ok.
Does the SELinux feature work at all without the patch? If not, we should consider moving the ticket to 1.8
The original documentation for the feature was incorrect so it didn't work (or rather it did work but pam_selinux got all confused). This time I tested the feature and it is working. There are still some rough edges but I suppose they are a part of intended behaviour of pam_selinux.
To sum up, yes, this should be also backported to 1.8. Patch attached and tested.
Sorry, one more thing I didn't notice before, can you move zeroing the errno right before sss_atomic_write_s() ?
Done in both patches.
Thanks Jan
On Thu, 2012-06-14 at 16:22 +0200, Jan Zelený wrote:
On Thu, Jun 07, 2012 at 11:47:35AM +0200, Jan Zelený wrote:
On Thu, May 31, 2012 at 09:17:18PM +0200, Jan Zeleny wrote:
At this moment we will support only asterisk, designating "all services".
https://fedorahosted.org/sssd/ticket/1360
Thanks Jan
Nack, you need to initialize services to NULL, otherwise if any operation before the strdup failed, you would free random pointer.
Good catch, fixed.
You can also use sizeof(ALL_SERVICES)-1 and avoid defining ALL_SERVICES_LEN (and be safe if ALL_SERVICES changed, not that it's likely).
As you have said, the constant is not likely to be changed. The next planned change is not to use the constant at all and rather dynamically fill this in by PAM responder. Hence I think leaving this is ok.
Does the SELinux feature work at all without the patch? If not, we should consider moving the ticket to 1.8
The original documentation for the feature was incorrect so it didn't work (or rather it did work but pam_selinux got all confused). This time I tested the feature and it is working. There are still some rough edges but I suppose they are a part of intended behaviour of pam_selinux.
To sum up, yes, this should be also backported to 1.8. Patch attached and tested.
Sorry, one more thing I didn't notice before, can you move zeroing the errno right before sss_atomic_write_s() ?
Done in both patches.
Ack
On Thu, 2012-06-14 at 11:13 -0400, Stephen Gallagher wrote:
On Thu, 2012-06-14 at 16:22 +0200, Jan Zelený wrote:
On Thu, Jun 07, 2012 at 11:47:35AM +0200, Jan Zelený wrote:
On Thu, May 31, 2012 at 09:17:18PM +0200, Jan Zeleny wrote:
At this moment we will support only asterisk, designating "all services".
https://fedorahosted.org/sssd/ticket/1360
Thanks Jan
Nack, you need to initialize services to NULL, otherwise if any operation before the strdup failed, you would free random pointer.
Good catch, fixed.
You can also use sizeof(ALL_SERVICES)-1 and avoid defining ALL_SERVICES_LEN (and be safe if ALL_SERVICES changed, not that it's likely).
As you have said, the constant is not likely to be changed. The next planned change is not to use the constant at all and rather dynamically fill this in by PAM responder. Hence I think leaving this is ok.
Does the SELinux feature work at all without the patch? If not, we should consider moving the ticket to 1.8
The original documentation for the feature was incorrect so it didn't work (or rather it did work but pam_selinux got all confused). This time I tested the feature and it is working. There are still some rough edges but I suppose they are a part of intended behaviour of pam_selinux.
To sum up, yes, this should be also backported to 1.8. Patch attached and tested.
Sorry, one more thing I didn't notice before, can you move zeroing the errno right before sss_atomic_write_s() ?
Done in both patches.
Ack
Pushed to master and sssd-1-8, respectively.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Stephen Gallagher