Re: New external_acl helper squid_kerb_ldap

On Mon, 31 May 2010 23:47:40 +0100, "Markus Moeller"
<huaraz_at_moeller.plus.com> wrote:
> "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.

Oh well. Okay. here to hoping :)

>
>> 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 ?

There is a TODO list entry to add them, yes. This is long-term and will
happen after this helper gets included.

>
>> * 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
>

Welcome. Thanks for the helper.

Amos
Received on Tue Jun 01 2010 - 00:15:51 MDT