[SSSD] [PATCHES][PRELIMINARY] Support the 'services' NSS map in SSSD

Stephen Gallagher sgallagh at redhat.com
Wed Jan 25 18:16:44 UTC 2012


On Wed, 2012-01-25 at 18:31 +0100, Jan Zelený wrote:
> > On Tue, 2012-01-24 at 16:48 -0500, Stephen Gallagher wrote:
> > > On Tue, 2012-01-24 at 11:19 -0500, Stephen Gallagher wrote:
> > > > On Tue, 2012-01-24 at 16:03 +0100, Jakub Hrozek wrote:
> > > > > On Mon, Jan 23, 2012 at 10:12:15PM -0500, Stephen Gallagher wrote:
> > > > > > On Sat, 2012-01-21 at 16:54 -0500, Stephen Gallagher wrote:
> > > > > > > These patches add support for getservbyname() and getservbyport()
> > > > > > > to the NSS responder. They do not yet support enumeration of
> > > > > > > services through setservent(), getservent() and endservent().
> > > > > > > These patches also include a reference implementation using the
> > > > > > > proxy ID provider (which I used to test and validate the
> > > > > > > functionality).
> > > > > > > 
> > > > > > > Work still to come: enumeration support and LDAP provider
> > > > > > > implementation.
> > > > > > > 
> > > > > > > Patch 0001: Make the sized_string structure available elsewhere
> > > > > > > in the source.
> > > > > > > 
> > > > > > > Patch 0002: Add support for strtouint16 (needed for managing the
> > > > > > > port number)
> > > > > > > 
> > > > > > > Patch 0003: Make add_string() and add_ulong() available to all
> > > > > > > sysdb files.
> > > > > > > 
> > > > > > > Patch 0004: Add routines to save, delete and modify service
> > > > > > > entries in the sysdb. (Also includes some very rudimentary unit
> > > > > > > tests)
> > > > > > > 
> > > > > > > Patch 0005: Add LDB indexes for the port and protocol arguments,
> > > > > > > since we'll be searching on them.
> > > > > > > 
> > > > > > > Patch 0006: Add support to the NSS sss_client to look up
> > > > > > > getservbyname() and getservbyport().
> > > > > > > 
> > > > > > > Patch 0007: Add support for services in dp requests
> > > > > > > 
> > > > > > > Patch 0008: Add routines to the negative cache to handle services
> > > > > > > 
> > > > > > > Patch 0009: The big one. This patch adds a new feature to the NSS
> > > > > > > responder to handle looking up services by name and by port
> > > > > > > (optionally filtering by protocol).
> > > > > > > 
> > > > > > > Important note: I did not use the existing check_cache()
> > > > > > > functionality here. I changed the cache check and lookup logic
> > > > > > > to match that which was proposed in
> > > > > > > https://fedorahosted.org/sssd/ticket/1126 (to deal with multiple
> > > > > > > domains more efficiently). I would like this very carefully
> > > > > > > reviewed (by two engineers) because I think our long-term plan
> > > > > > > is going to be to migrate the existing NSS responders over to
> > > > > > > this style.
> > > > > > > 
> > > > > > > /* Provider Lookup Logic:
> > > > > > >  * Iterate through the available caches. If the cached entry is
> > > > > > >  * present and not expired, return it immediately(*). If it is
> > > > > > >  * present and expired, add it to a list of domains eligible to
> > > > > > >  * be checked. If it is in the negative cache, skip over it and
> > > > > > >  * do not add it to the eligible domain list.
> > > > > > >  *
> > > > > > >  * Once we have searched all of the caches, if the entry has not
> > > > > > >  * been determined to be available, search all domains in order
> > > > > > >  * to see if any of them contain the requested entry.
> > > > > > >  *
> > > > > > >  * (*) Optionally perform a midpoint cache refresh if
> > > > > > >  appropriate. */
> > > > > > > 
> > > > > > > See the functions getserv_send() and lookup_service_*() for more
> > > > > > > details.
> > > > > > > 
> > > > > > > 
> > > > > > > Patch 0010: This is the reference implementation I used for
> > > > > > > testing purposes. I tested against proxy_lib_name = files (and
> > > > > > > changed the 'services' line in my /etc/nsswitch.conf to contain
> > > > > > > only 'sss')
> > > > > > 
> > > > > > New round of patches. The first seven patches are unchanged from
> > > > > > the first pass (though offset by three, since the first three
> > > > > > patches have been pushed). The eighth patch is just Jakub's acked
> > > > > > patch for refactoring the dp_req routines from "[PATCH] DP:
> > > > > > Refactor responder_dp_req so it's reusable by other responders" in
> > > > > > the proper order to ensure it doesn't get lost/put in the wrong
> > > > > > place. (Though I'm pretty sure it will apply cleanly anywhere
> > > > > > after 0006).
> > > > > > 
> > > > > > Patch 0009: Add support to sss_client to make enumeration requests
> > > > > > for services
> > > > > > 
> > > > > > Patch 0010: Add sysdb routines for requesting the complete list of
> > > > > > cached services.
> > > > > > 
> > > > > > Patch 0011: Another big one. Adds support for enumerating services
> > > > > > to the NSS provider.
> > > > > > 
> > > > > > Patch 0012: The proxy provider implementation of the enumeration
> > > > > > logic. Useful for testing purposes (I tested against the 'files'
> > > > > > NSS library).
> > > > > > 
> > > > > > There is a known issue with the proxy provider support that I'm
> > > > > > going to work on in a later patch. Specifically, when we get back
> > > > > > the list of available services from the proxy provider, we get
> > > > > > services that have multiple available protocols back as separate
> > > > > > entries. This causes the second value to overwrite the primary
> > > > > > value in sysdb_store_service(). I'm going to add the ability to
> > > > > > merge them, but since the LDAP provider (when finished) won't
> > > > > > suffer this problem (since LDAP lookups will return all protocols
> > > > > > in the same request), I'm not stressing over it right now. Once
> > > > > > the code is in, I'll file a bug so we don't forget about it.
> > > > > 
> > > > > Patch 0001: SYSDB: Add sysdb routines for manipulating service
> > > > > entries
> > > > > 
> > > > > I agree with Simo that proto?sanitized_proto:"*" is confusing. I
> > > > > think it would be more readable to set sanitized_proto to NULL on
> > > > > definition and just use sanitized_proto in the ternary operator.
> > > > 
> > > > Whoops. I forgot to include Simo's review comment. Fixed.
> > > > 
> > > > > Why the cast of "int proto" in sysdb_getservbyport() to "unsigned
> > > > > int" ? Would it make sense to add a check if proto >= 0 ? (I can't
> > > > > help wondering why the glibc interface uses int and not unsigned int
> > > > > in the first place..)
> > > > 
> > > > I assume you meant "int port". I cast it because (as you observe) glibc
> > > > uses 'int' instead of 'uint16_t' which is what it SHOULD be doing.
> > > > 
> > > > I added a check.
> > > > 
> > > > > I noticed we use the same construct with sysdb_transaction_start()
> > > > > and bool in_transaction all around the code. I think a macro that
> > > > > would wrap the start, commit and cancel might be nicer and save us
> > > > > from some bugs in the future. Not as part of this patch, of course.
> > > > 
> > > > We can think about it. Please file a ticket.
> > > > 
> > > > > Can you add a DEBUG() message fot the case sysdb_store_service() is
> > > > > deleting a corrupt entry?
> > > > 
> > > > Yeah, good idea.
> > > > 
> > > > > The loop after sysdb_getservbyname() in sysdb_store_service() always
> > > > > references res->msgs[0], shouldn't that be res->msgs[i] ?
> > > > 
> > > > Thanks, bad copy-paste
> > > > 
> > > > > In sysdb_svc_add() isn't it safer to use "if (aliases &&
> > > > > aliases[0])"? I think the caller might appreciate the simplicity of
> > > > > passing NULL for no
> > > > 
> > > > Good idea. Fixed
> > > > 
> > > > > aliases. The same applies for aliases and protocols in
> > > > > sysdb_svc_update().
> > > > 
> > > > For aliases, yes. There MUST be at least one protocol. I've added an
> > > > EINVAL check at the start of the function.
> > > > 
> > > > > In sysdb_svc_delete() you forgot to set in_transaction to false after
> > > > > sysdb_transaction_commit().
> > > > 
> > > > Good catch. That macro is sounding better :)
> > > > 
> > > > > Patch 0002: NSS: Add client support for services (non-enumeration)
> > > > > Ack
> > > > > 
> > > > > Patch 0003: NSS: Add client support for services (non-enumeration)
> > > > > Can you #define some SERVICES_METADATA_COUNT to avoid using the
> > > > > constant 8 in the functions?
> > > > > Otherwise looks OK
> > > > 
> > > > Done. I also made the same change to patch 0009
> > > > 
> > > > > Patch 0004: DP: Add support for services in dp requests
> > > > > Ack
> > > > > 
> > > > > Patch 0005: NSS: Add negative cache routines for services
> > > > > Ack
> > > > > 
> > > > > The rest of the patches is TBR, it might take time to review the big
> > > > > ones, so I decided to send the review in parts.
> > > > 
> > > > Since the full patchset is enormous, I'm only attaching the current
> > > > updates. Anyone who wants the full set of patches can get them from my
> > > > personal git repo with the following commands:
> > > > 
> > > > git remote add sgallagh \
> > > > git://fedorapeople.org/home/fedora/jhrozek/public_git/sssd.git
> > > > 
> > > > git remote update
> > > > 
> > > > git format-patch -M -C --patience --full-index \
> > > > 6e4efa5a0960a83fef6a654152edfc618bcebece..HEAD
> > > > 
> > > > 
> > > > I'll continue to update the format-patch command in subsequent
> > > > revisions.
> > > 
> > > Just a heads-up. I'm reworking the "extra" field in the backend so that
> > > it's parsed by be_get_account_info() instead of in the providers itself
> > > (so they'll be able to get breq->extra_value instead of parsing
> > > breq->filter_value).
> > > 
> > > This is going to require adjustment for the proxy provider.
> > 
> > Ok, resending all patches (tarballed this time) just to keep things
> > clean.
> > 
> > I added a new patch to the series (now Patch 0001) that adds support for
> > splitting the extra data in the be_get_account_info() routine instead of
> > requiring the provider plugins to do it themselves. This adds a new
> > attribute to the be_acct_req structure for the extra data.
> > 
> > Patches 0002-0006 are unchanged this round.
> > 
> > Patch 0007 was updated to correct some DEBUG log glitches:
> >  * %lu instead of %ul
> >  * Wrap the SVC_*_CASED macros in parens so they will work inside other
> > ternary operations.
> > 
> > It also fixes a much more serious issue where it was not properly
> > detecting the length of the protocol and was thus failing.
> > 
> > Patch 0008 was updated to properly convert the port value back to the
> > network format which is expected by the loaded NSS library. (The NSS
> > Responder serves the data provider a port in the native format).
> > 
> > All the rest are unchanged from the last submission. Sorry for the
> > churn.
> 
> 
> #0001:
> Ack.
> 
> #0002:
> - make sysdb_svc_add and sysdb_svc_update static -> there is a 
> sysdb_store_service as an interface for them. Also consider making 
> sysdb_svc_dn and sysdb_svc_remove_alias making static - I'm not sure about 
> those.
> - if we are to be really defensive (as discussed on IRC), I'd change the 
> approach if detecting more services with the same name being already stored in 
> sysdb - perhaps delete them all and store the new one instead of returning 
> EINVAL?
> - in sysdb_svc_delete: just in case both port and name is given, I'd try to 
> delete service by name and in case of failure fall back to the port (i.e. not 
> making them mutually exclusive).
> 
> #0003:
> Ack
> 
> #0004:
> - just a reminder that there is that segfault inducing error, other than that 
> the patch is fine, so conditional Ack
> 
> #0005:
> Ack
> 
> #0006:
> - I think you should be freeing service_and_protocol variables after they are 
> used. They are allocated on top of negative cache, so they could potentially 
> live pretty long and take a lot of space
> - I also think you should be checking the above mentioned variable for NULL 
> and returning ENOMEM if the operation failed. Otherwise I see potential 
> problems in utf8 library which would could receive NULL on input. And if 
> nothing else, you would be returning EINVAL from sss_ncache_set_service_int 
> instead of ENOMEM ;-)
> 
> 
> That's all I've got right now. I will continue with the review tomorrow.


I forgot to include the patches. Fixed.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: servicesreview.tar.bz2
Type: application/x-bzip-compressed-tar
Size: 30246 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120125/bcc43b74/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120125/bcc43b74/attachment.sig>


More information about the sssd-devel mailing list