Re: New external_acl helper squid_kerb_ldap

"Amos Jeffries" <squid3_at_treenet.co.nz> wrote in message
news:90351e19016110c5e9fd1699187b55f6_at_mail.treenet.co.nz...
> On Sun, 30 May 2010 16:15:14 +0100, "Markus Moeller"
> <huaraz_at_moeller.plus.com> wrote:
>> Hi,
>>
>> I have converted my helper to kerberos_ldap_group ( not sure if that
> is
>> the best name) and created a patch for inclusion into the head revision.
>
>> Please review and let me know any feedback.
>>
>>
>> Thank you
>> Markus
>
> Hi Markus,
>
> * No need to include the COPYING file when bundled, Squid gets
> distributed with a version in the base directory.

OK I removed it

> * Also seems to have pulled in "negotiate_kerberos_auth-logging.patch"
> and "negotiate_kerberos_auth-new.cc"
>

YES as I think their are not required just a left over

>
> support.h:
> * config.h will pull in the compat/* files. no need to include them
> explicitly now. it will also pull in several of the standard headers.

OK I removed them

> * not sure about that LogTime definition going in here when its not
> inline. Can it be moved to a .cc?
>

YES I did create another support_log.cc

> kerberos_ldap_group.cc and support_*.cc:
> * needs to include config.h as the first include, this will pull in the
> basic headers for types etc for portability: unistd.h, stdlib.h,
> sys/time.h, etc
>

OK. Done

> * ALL system headers used need to be wrapped with HAVE_* even if standard
> headers, and go below the local file includes. If a .h file needs some
> definition to build, the system file needs to be pulled into there instead
> of the .cc.
>

OK Done

> Also; why are the support_*.cc files all in helpers/external_acl/
> directly?
>

Apologies a wrong copy in my source.

> support_ldap.cc:
> * sub-Copyright for The OpenLDAP Foundation will have to be added to the
> Squid combined CREDITS copyright file.
>

I updated CREDITS.

> support_netbios.cc:
> * I thought NetBOIS was officially declared dead by MS? can that be
> dropped?
>

This is for cases when I get a user name of NETBIOS-DOMAIN\user (e.g. NTLM
auth) so I can transform it into user_at_KERBEROS-DOMAIN. Unfortunately the
NETBIOS-DOMAIN and KERBEROS-DOMAIN names are not always the same.

> support_resolv.cc:
> * RFC1035 text is bundled with Squid in docs/rfc/, IMO a simple reference
> to the RFC number and section numbers should be sufficient for the
> documentation.

OK. I did not see the rfc directory in my rsync copy. This was more for my
own guidance

> * RFC 2782 can be pulled into doc/rfc/ and added to that
> doc/rfc/1-index.txt as a separate patch instead of quoting large portions.

OK. as above

> * There are rfc1035 type definitions and packers/unpackers are provided
> through $(COMPAT_LIB) for now. see include/rfc1035.h if you actually need
> them. Though from a quick look you seem to be using the resolver library
> API not packet-level details from RFC 1035.

I did not know that they were available as I created the module independant
of squid libraries. I will check how to use them.

> * NP: long-term plan is to have a libdns in Squid you can link and call,
> but that is not ready yet, so overlooking the manual resolving this file
> does for now.
>

So you will include resolving SRV records ?

> * Some good news; since we imported the auth helper I've cleaned up
> Squids' API for getaddrinfo/freeaddrinfo/gai_strerror. You can use the
> POSIX names now instead of the xgetaddrinfo variants.
>

Excellent

>
> Amos
>

I think we have to change the config.test. It really does not cover cases
where the header files are not in the default place (.e.g. if krb5-config
points to a newer library)

Thank you very much for the quick and detailed reply

Markus