[PATCHES] Make nss responder aware of views and overrides

[Sumit Bose]

Hi,

this patches make the nss responder view aware. A number of new sysdb
calls is added as drop in replacements from existing ones. I preferred
this instead of modifying the existing ones to not break their usage in
other places. But I think they are a good targets, together with the
other user and group related sysdb calls, for a refactoring in an
upcoming version.

bye,
Sumit

[Sumit Bose]

On Tue, Oct 14, 2014 at 05:57:36PM +0200, Sumit Bose wrote:
> Hi,
>
> this patches make the nss responder view aware. A number of new sysdb
> calls is added as drop in replacements from existing ones. I preferred
> this instead of modifying the existing ones to not break their usage in
> other places. But I think they are a good targets, together with the
> other user and group related sysdb calls, for a refactoring in an
> upcoming version.
>
> bye,
> Sumit

Please find attached a new version with some minor fixes in fill_pwent()
and fill_grent() with respect to fully qualified names and an additional
patch with the "changes" for the pam responder.

bye,
Sumit

[Pavel Březina]

On 10/15/2014 01:08 PM, Sumit Bose wrote:
> On Tue, Oct 14, 2014 at 05:57:36PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > this patches make the nss responder view aware. A number of new sysdb
> > calls is added as drop in replacements from existing ones. I preferred
> > this instead of modifying the existing ones to not break their usage in
> > other places. But I think they are a good targets, together with the
> > other user and group related sysdb calls, for a refactoring in an
> > upcoming version.
> >
> > bye,
> > Sumit
>
> Please find attached a new version with some minor fixes in fill_pwent()
> and fill_grent() with respect to fully qualified names and an additional
> patch with the "changes" for the pam responder.
>
> bye,
> Sumit

Patch #4 sysdb: add overide lookup calls won't apply to current master. 
But the rebase was trivial so it didn't stop me from reviewing.

Patch #1: confdb: add has_views and view_name to sss_domain_info

ACK

Patch #2: new_subdomain: copy view data from parent

ACK

Patch #3: sysdb: add view data to domains
> if (ret == ENOENT || view_name == NULL
>        || (view_name != NULL
>            && strcmp(view_name, SYSDB_DEFAULT_VIEW_NAME) == 0)) {

view_name != NULL condition can be skipped

And the whole branching in sysdb_master_domain_update() is quite complex 
and difficult to understand. Can you add a comment to the top of each 
branch that says when the branch will be executed?

Patch #4: sysdb: add overide lookup calls

ACK

Patch #5: sysdb: add sysdb_getpwnam/uid_with_views()
> /* TODO: check if EOK is returned even if orig_obj is empty, i.e nothgin
> * found. */

This is strange todo to leave in the code. Can you do it? :-)

Patch #6: sysdb: add sss_view_ldb_msg_find_element/attr_as_string/uint64

ACK

Patch #7:  nss: add view support for getpwnam/getpwuid requests

I think you can use sss_view_ldb* functions from the previous patch, 
can't you?

Patch #8: sysdb: add sysdb_initgroups_with_views()

ACK

Patch #9: nss: add view support to initgroups request

ACK

Patch #10: sysdb: add sysdb_getgrnam_with_views and 
sysdb_getgrgid_with_views
> /* TODO: check if EOK is returned even if orig_obj is empty, i.e nothing
> * found. */

Again.

Patch #11: nss: add view support for getgr* requests

Coverity warning:

nsssrv_cmd.c:2856: value_overwrite: Value "22" is overwritten with value 
from "sss_packet_set_size(packet, 0UL)".
nsssrv_cmd.c:2810: assigned_value: Value "22" is assigned to "ret" here, 
but that stored value is not used before it is overwritten.

Patch #12: sid2name: return name without views applied
> if (apply_no_view) {
>    orig_name = ldb_msg_find_attr_as_string(msg,
>                                            ORIGINALAD_PREFIX SYSDB_NAME,
>                                            NULL);
> }
> if (orig_name == NULL) {
>    orig_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
> }

The name of the variable 'apply_no_view' suggests that if true, no 
override should be applied. Thus if false, shouldn't we use 
sss_view_ldb* to get overriden value if possible?

Patch #13: pam: make pam responder aware if views

ACK

I was using these patches for quite some time during my work on IFP and 
sudo responder part of the views. All known issues seems to be resolved. 
Functional ACK.

[Sumit Bose]

On Fri, Oct 17, 2014 at 02:05:44PM +0200, Pavel Březina wrote:
> On 10/15/2014 01:08 PM, Sumit Bose wrote:
> > On Tue, Oct 14, 2014 at 05:57:36PM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patches make the nss responder view aware. A number of new sysdb
> > > calls is added as drop in replacements from existing ones. I preferred
> > > this instead of modifying the existing ones to not break their usage in
> > > other places. But I think they are a good targets, together with the
> > > other user and group related sysdb calls, for a refactoring in an
> > > upcoming version.
> > >
> > > bye,
> > > Sumit
> >
> > Please find attached a new version with some minor fixes in fill_pwent()
> > and fill_grent() with respect to fully qualified names and an additional
> > patch with the "changes" for the pam responder.
> >
> > bye,
> > Sumit
>
> Patch #4 sysdb: add overide lookup calls won't apply to current master. But
> the rebase was trivial so it didn't stop me from reviewing.

Thank you very much for the review.
> Patch #1: confdb: add has_views and view_name to sss_domain_info
>
> ACK
>
> Patch #2: new_subdomain: copy view data from parent
>
> ACK
>
> Patch #3: sysdb: add view data to domains
> > if (ret == ENOENT || view_name == NULL
> >        || (view_name != NULL
> >            && strcmp(view_name, SYSDB_DEFAULT_VIEW_NAME) == 0)) {
>
> view_name != NULL condition can be skipped

dropped
> And the whole branching in sysdb_master_domain_update() is quite complex and
> difficult to understand. Can you add a comment to the top of each branch
> that says when the branch will be executed?

done
> Patch #4: sysdb: add overide lookup calls
>
> ACK
>
> Patch #5: sysdb: add sysdb_getpwnam/uid_with_views()
> > /* TODO: check if EOK is returned even if orig_obj is empty, i.e nothgin
> > * found. */
>
> This is strange todo to leave in the code. Can you do it? :-)

removed
> Patch #6: sysdb: add sss_view_ldb_msg_find_element/attr_as_string/uint64
>
> ACK
>
> Patch #7:  nss: add view support for getpwnam/getpwuid requests
>
> I think you can use sss_view_ldb* functions from the previous patch, can't
> you?

yes, except for name and gid where some special handling is needed. For
the primary gid I assume that a server-side override overrides a
client-side value given by the override_gid option.
> Patch #8: sysdb: add sysdb_initgroups_with_views()
>
> ACK
>
> Patch #9: nss: add view support to initgroups request
>
> ACK
>
> Patch #10: sysdb: add sysdb_getgrnam_with_views and
> sysdb_getgrgid_with_views
> > /* TODO: check if EOK is returned even if orig_obj is empty, i.e nothing
> > * found. */
>
> Again.

removed
> Patch #11: nss: add view support for getgr* requests
>
> Coverity warning:
>
> nsssrv_cmd.c:2856: value_overwrite: Value "22" is overwritten with value
> from "sss_packet_set_size(packet, 0UL)".
> nsssrv_cmd.c:2810: assigned_value: Value "22" is assigned to "ret" here, but
> that stored value is not used before it is overwritten.

fixed
> Patch #12: sid2name: return name without views applied
> > if (apply_no_view) {
> >    orig_name = ldb_msg_find_attr_as_string(msg,
> >                                            ORIGINALAD_PREFIX SYSDB_NAME,
> >                                            NULL);
> > }
> > if (orig_name == NULL) {
> >    orig_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
> > }
>
> The name of the variable 'apply_no_view' suggests that if true, no override
> should be applied. Thus if false, shouldn't we use sss_view_ldb* to get
> overriden value if possible?

yes, although I used ldb_msg_find_attr_as_string and look for the
override value directly to make to more clear what is happening and
since override names are un-qualified they need some special
handling.
> Patch #13: pam: make pam responder aware if views
>
> ACK
>
> I was using these patches for quite some time during my work on IFP and sudo
> responder part of the views. All known issues seems to be resolved.
> Functional ACK.

New version attached.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 10/17/2014 03:40 PM, Sumit Bose wrote:
> On Fri, Oct 17, 2014 at 02:05:44PM +0200, Pavel Březina wrote:
> > On 10/15/2014 01:08 PM, Sumit Bose wrote:
> > > On Tue, Oct 14, 2014 at 05:57:36PM +0200, Sumit Bose wrote:
> > > > Hi,
> > > >
> > > > this patches make the nss responder view aware. A number of new sysdb
> > > > calls is added as drop in replacements from existing ones. I preferred
> > > > this instead of modifying the existing ones to not break their usage in
> > > > other places. But I think they are a good targets, together with the
> > > > other user and group related sysdb calls, for a refactoring in an
> > > > upcoming version.
> > > >
> > > > bye,
> > > > Sumit
> > >
> > > Please find attached a new version with some minor fixes in fill_pwent()
> > > and fill_grent() with respect to fully qualified names and an additional
> > > patch with the "changes" for the pam responder.
> > >
> > > bye,
> > > Sumit
> >
> > Patch #4 sysdb: add overide lookup calls won't apply to current master. But
> > the rebase was trivial so it didn't stop me from reviewing.
>
> Thank you very much for the review.
> > Patch #1: confdb: add has_views and view_name to sss_domain_info

ACK
> > Patch #2: new_subdomain: copy view data from parent

ACK
> > Patch #3: sysdb: add view data to domains

ACK
> > Patch #4: sysdb: add overide lookup calls

ACK
> > Patch #5: sysdb: add sysdb_getpwnam/uid_with_views()

ACK
> > Patch #6: sysdb: add sss_view_ldb_msg_find_element/attr_as_string/uint64

ACK
> > Patch #7:  nss: add view support for getpwnam/getpwuid requests
> >
> > I think you can use sss_view_ldb* functions from the previous patch, can't
> > you?
>
> yes, except for name and gid where some special handling is needed. For
> the primary gid I assume that a server-side override overrides a
> client-side value given by the override_gid option.

ACK
> > Patch #8: sysdb: add sysdb_initgroups_with_views()

ACK
> > Patch #9: nss: add view support to initgroups request

ACK
> > Patch #10: sysdb: add sysdb_getgrnam_with_views and
> > sysdb_getgrgid_with_views

ACK
> > Patch #11: nss: add view support for getgr* requests

ACK
> > Patch #12: sid2name: return name without views applied
> > > if (apply_no_view) {
> > >    orig_name = ldb_msg_find_attr_as_string(msg,
> > >                                            ORIGINALAD_PREFIX SYSDB_NAME,
> > >                                            NULL);
> > > }
> > > if (orig_name == NULL) {
> > >    orig_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
> > > }
> >
> > The name of the variable 'apply_no_view' suggests that if true, no override
> > should be applied. Thus if false, shouldn't we use sss_view_ldb* to get
> > overriden value if possible?
>
> yes, although I used ldb_msg_find_attr_as_string and look for the
> override value directly to make to more clear what is happening and
> since override names are un-qualified they need some special
> handling.

Ok. ACK
> > Patch #13: pam: make pam responder aware if views

ACK

Patch #14: sysdb: add sysdb_enumpw/grent_with_views()

ACK

Patch #15: nss: make enumeration requests aware of views

ACK

Full ACK to all patches.

[Jakub Hrozek]

On Mon, Oct 20, 2014 at 02:52:47PM +0200, Pavel Březina wrote:
> Full ACK to all patches.

awesome! Let the push begin!

[Jakub Hrozek]

On Mon, Oct 20, 2014 at 02:52:47PM +0200, Pavel Březina wrote:
> Full ACK to all patches.

I ran the patches through Coverity and didn't find any issues. Pushed to
master:
 * 396fb27b17d66261e2d15146a7c925be8d637226
 * 4777af0b8f9a3f418a54f0d4bf7eb72b896dabb5
 * c2788a00c49b14fc56690af93dc1ac60d6ee6c70
 * a983272f1afa8dbae3ecd4425b04649601732a71
 * afbe298d8d99c037056c1d3878812d98783309b0
 * d2f4551519698809e73a029c49599e1f67e6bdd4
 * f1d5f72459ec7d776e66c4516da2c1b9c6c1a84d
 * 908ee7aa8f046ae7f066d80b787cd380d61af619
 * e6b476c9749737f0979fe6460f0d6ced08351db3
 * ba88f3617e5a56bba19a0d65d35069d8e4d0c89c
 * 89b065cb85f57e80760ce4d4b1215b533e249e92
 * d70023a7fa95c8c12683de965a76ec38a6234ae5
 * 9f734d4c122e37cc3080974342ed9586d05d5f83
 * c9589c42bcdcc864c6becda3e6c04b890ee81b0c
 * e7cc651468ab8b1462a6a39e712e7b8d36a3a166

[Sumit Bose]

On Wed, Oct 15, 2014 at 01:08:29PM +0200, Sumit Bose wrote:
> On Tue, Oct 14, 2014 at 05:57:36PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > this patches make the nss responder view aware. A number of new sysdb
> > calls is added as drop in replacements from existing ones. I preferred
> > this instead of modifying the existing ones to not break their usage in
> > other places. But I think they are a good targets, together with the
> > other user and group related sysdb calls, for a refactoring in an
> > upcoming version.
> >
> > bye,
> > Sumit
>
> Please find attached a new version with some minor fixes in fill_pwent()
> and fill_grent() with respect to fully qualified names and an additional
> patch with the "changes" for the pam responder.

This new version contains fixes for memory leaks and unused return
values.

There are two new patches which make the enumeration requests view aware
as well.

bye,
Sumit