Milter-greylist

Hello all,

   I am trying to make milter-greylist work with Sun/Oracle Messaging
Server (part of Oracle unified Communications Suite now), and there
is a problem which I've touched on recently - its partial milter-API
implementation. While extending and porting some rulesets from our
Sendmail-based relays, I found that the "domain" keyword only has
the bracketed IP-quad as the submitter host's name, like "[1.2.3.4]".

   Since milter-greylist does use DNS a lot anyway (RBL, SPF, etc.)
I wonder if it is possible to add a re-request into DNS for such
botched remote client names? Perhaps there is already a keyword to
enable such behavior?

   Also, are there any configuration patterns to enable DNS-based
tests that the remote host's HELO/EHLO name matches the textual
name in the DNS PTR entry for its IP address, and that this name
from DNS PTR resolves back to this IP address (or includes it
among multiple values) - i.e. what I believe Sendmail does when
estimating address "forgery"?

   I tried to print in milter-greylist's "msg" clause the values
of "sendmail macros" listed in different articles and blogs, and
found that if_addr, client_name, client_ptr are not defined; the
helo is defined to whatever the remote host wrote about itself,
client_addr is defined to the IP address (no brackets), and I did
not find a macro which would contain the domain name (%d in milter
greylist formatting), which is the IP in brackets.

Thanks for any ideas,
//Jim Klimov

On 2013-08-05 03:14, Jim Klimov wrote:
> Hello all,
>
> I am trying to make milter-greylist work with Sun/Oracle Messaging
> Server (part of Oracle unified Communications Suite now), and there
> is a problem which I've touched on recently - its partial milter-API
> implementation. While extending and porting some rulesets from our
> Sendmail-based relays, I found that the "domain" keyword only has
> the bracketed IP-quad as the submitter host's name, like "[1.2.3.4]".
>
> Since milter-greylist does use DNS a lot anyway (RBL, SPF, etc.)
> I wonder if it is possible to add a re-request into DNS for such
> botched remote client names? Perhaps there is already a keyword to
> enable such behavior?

To answer my own question, I did not find any relevant code in the
project that would do just that name resolution; but code in mx.c
was quite useful to make my own DNS queries. I attach the patch
which works for me in limited testing at least, may be quite noisy
in logs if debug is enabled.

I hope the list members can review this code for apparent errors
at least... but again - I have a test-case where it just works :)

It might make sense to enable this code-path with a config-file
option (i.e. for concerns about thread-unsafe resolvers), but I did
not get that far and I myself likely won't do it, either.

> Also, are there any configuration patterns to enable DNS-based
> tests that the remote host's HELO/EHLO name matches the textual
> name in the DNS PTR entry for its IP address, and that this name
> from DNS PTR resolves back to this IP address (or includes it
> among multiple values) - i.e. what I believe Sendmail does when
> estimating address "forgery"?

Now that I found how to do the DNS queries, making some logic to
detect such forgery (as a possibly new rule keyword in the overall
project structure) seems more feasible :) Though this, to be done
properly and prettily (with keywords, parsing, etc.) sounds too
complex for me to complete in my limited time.

Hope this helps, at least,
//Jim Klimov

On 2013-08-05 06:20, Jim Klimov wrote:
> I hope the list members can review this code for apparent errors
> at least... but again - I have a test-case where it just works :)

Got two cases now :)

For a host with a PTR name entry in DNS, this name is resolved and
used in "domain" ACLs. For a host without a PTR entry, the bracketed
quad is retained and is used in "domain" ACLs.

Also, one of my older systems complained about the patch's syntax -
the definition line
     int goodQuad = 0;
had to be moved a dozen lines up to other such definitions in the
block.

HTH,
//Jim Klimov

On Mon, Aug 05, 2013 at 03:14:21AM +0200, Jim Klimov wrote:
>    Since milter-greylist does use DNS a lot anyway (RBL, SPF, etc.)
> I wonder if it is possible to add a re-request into DNS for such
> botched remote client names? Perhaps there is already a keyword to
> enable such behavior?

No, there is not. Contribution is welcome.
I wonder if we want a new ACL clause for that, or a global switch.

>    Also, are there any configuration patterns to enable DNS-based
> tests that the remote host's HELO/EHLO name matches the textual
> name in the DNS PTR entry for its IP address, and that this name
> from DNS PTR resolves back to this IP address (or includes it
> among multiple values) - i.e. what I believe Sendmail does when
> estimating address "forgery"?

I think this may work (not tested)
racl blacklist not helo "%d" msg "HELO does not match reverse DNS"

-- 
Emmanuel Dreyfus
manu@...

Interesting off-topic came up today... I wonder if name resolution
(via res_nquery()) can fall-back to file-based nsswitch as well, or
if it just resolves its host's own name, at least on Solaris?

Today there is a problem with LAN DNS (not available), and the internet
DNS apparently does and should not know names for private IP addresses.
Still, I see the host's own names resolved (yes, not calling greylist
for $self at all - is on the menu):

Aug  5 13:41:59 ucs milter-greylist: [ID 471652 mail.debug] Incoming 
connection from host '[10.0.16.60]'
Aug  5 13:41:59 ucs milter-greylist: [ID 308029 mail.debug] Got an 
unresolved host name [10.0.16.60], will try to resolve
Aug  5 13:41:59 ucs milter-greylist: [ID 682236 mail.debug] Requesting 
PTR entry for 60.16.0.10.in-addr.arpa.
Aug  5 13:41:59 ucs milter-greylist: [ID 646443 mail.debug] Got name 
'ucs.domain.com' (1)
Aug  5 13:41:59 ucs milter-greylist: [ID 779078 mail.debug] User 
jimklimov@... authenticated, bypassing greylisting
Aug  5 13:41:59 ucs milter-greylist: [ID 703198 mail.debug] 
0MR100I02XLZW500: addr = ucs.domain.com[10.0.16.60], from = 
<jimklimov@...>, rcpt = <jim@...>


# nslookup 60.16.0.10.in-addr.arpa. 8.8.8.8
Server:         8.8.8.8
Address:        8.8.8.8#53

** server can't find 60.16.0.10.in-addr.arpa.: NXDOMAIN


# nslookup 10.0.16.60 8.8.8.8
Server:         8.8.8.8
Address:        8.8.8.8#53

Non-authoritative answer:
60.16.0.10.in-addr.arpa name = ucs.domain.com.

Authoritative answers can be found from:

# nslookup -q=ptr 60.16.0.10.in-addr.arpa. 8.8.8.8
Server:         8.8.8.8
Address:        8.8.8.8#53

Non-authoritative answer:
60.16.0.10.in-addr.arpa name = ucs.domain.com.

Authoritative answers can be found from:


I am pretty sure that the name-serivce clients were restarted recently
and the name should not be cached from previous DNS replies... Still,
interesting :)

Other names, including local zones on same machine also with entries
in the /etc/hosts file, are not resolved this way...


# nslookup -q=ptr 61.16.0.10.in-addr.arpa. 8.8.8.8
Server:         8.8.8.8
Address:        8.8.8.8#53

** server can't find 61.16.0.10.in-addr.arpa.: NXDOMAIN


So... here's a random bit of experience to contemplate ;)

HTH,
//Jim

On Mon, Aug 05, 2013 at 11:55:46AM +0200, Jim Klimov wrote:
> Interesting off-topic came up today... I wonder if name resolution
> (via res_nquery()) can fall-back to file-based nsswitch as well, or
> if it just resolves its host's own name, at least on Solaris?

res_* does not use the nss_* framework (it's true for the way back).
nss_* provides hostname resolution for gethostbyname/addr.


> 
> Today there is a problem with LAN DNS (not available), and the internet
> DNS apparently does and should not know names for private IP addresses.
> Still, I see the host's own names resolved (yes, not calling greylist
> for $self at all - is on the menu):
> 
> Aug  5 13:41:59 ucs milter-greylist: [ID 471652 mail.debug] Incoming 
> connection from host '[10.0.16.60]'
> Aug  5 13:41:59 ucs milter-greylist: [ID 308029 mail.debug] Got an 
> unresolved host name [10.0.16.60], will try to resolve
> Aug  5 13:41:59 ucs milter-greylist: [ID 682236 mail.debug] Requesting 
> PTR entry for 60.16.0.10.in-addr.arpa.
> Aug  5 13:41:59 ucs milter-greylist: [ID 646443 mail.debug] Got name 
> 'ucs.domain.com' (1)
> Aug  5 13:41:59 ucs milter-greylist: [ID 779078 mail.debug] User 
> jimklimov@... authenticated, bypassing greylisting
> Aug  5 13:41:59 ucs milter-greylist: [ID 703198 mail.debug] 
> 0MR100I02XLZW500: addr = ucs.domain.com[10.0.16.60], from = 
> <jimklimov@...>, rcpt = <jim@...>
> 
> 
> # nslookup 60.16.0.10.in-addr.arpa. 8.8.8.8
> Server:         8.8.8.8
> Address:        8.8.8.8#53
> 
> ** server can't find 60.16.0.10.in-addr.arpa.: NXDOMAIN

Not a PTR query?

> 
> 
> # nslookup 10.0.16.60 8.8.8.8
> Server:         8.8.8.8
> Address:        8.8.8.8#53
> 
> Non-authoritative answer:
> 60.16.0.10.in-addr.arpa name = ucs.domain.com.
> 
> Authoritative answers can be found from:
> 
> # nslookup -q=ptr 60.16.0.10.in-addr.arpa. 8.8.8.8
> Server:         8.8.8.8
> Address:        8.8.8.8#53
> 
> Non-authoritative answer:
> 60.16.0.10.in-addr.arpa name = ucs.domain.com.
> 
> Authoritative answers can be found from:
> 
> 
> I am pretty sure that the name-serivce clients were restarted recently
> and the name should not be cached from previous DNS replies... Still,
> interesting :)

Use a tracing tool make a look behind the scenes ...

truss -f nslookup 10.0.16.60 8.8.8.8

> 
> Other names, including local zones on same machine also with entries
> in the /etc/hosts file, are not resolved this way...
> 
> 
> # nslookup -q=ptr 61.16.0.10.in-addr.arpa. 8.8.8.8
> Server:         8.8.8.8
> Address:        8.8.8.8#53
> 
> ** server can't find 61.16.0.10.in-addr.arpa.: NXDOMAIN
> 
> 
> So... here's a random bit of experience to contemplate ;)

I'am not aware of any newer frameworks in Solaris which may hook
in resolver calls. Maybe a Bind extension or functionality?

Johann

On Mon, Aug 05, 2013 at 01:10:22PM +0200, Johann Klasek wrote:
> On Mon, Aug 05, 2013 at 11:55:46AM +0200, Jim Klimov wrote:
> > Interesting off-topic came up today... I wonder if name resolution
> > (via res_nquery()) can fall-back to file-based nsswitch as well, or
> > if it just resolves its host's own name, at least on Solaris?
> 
> res_* does not use the nss_* framework (it's true for the way back).
> nss_* provides hostname resolution for gethostbyname/addr.

Forgot about this: Solaris FAQ mentions it:

 4.2) What is /etc/nsswitch.conf? 

 [..]
 Terminology: Sun worried over the term "resolver", which technically
 means any "get info" routine (getpwent(3), gethostbyname(3), etc), but
 is also specifically attached to the DNS resolver. Therefore they used
 the term "source" to mean the things after the colon
 (files/DNS/NIS/NIS+) and "database" to mean the thing before the colon
 (passwd/group/hosts/services/netgroup etc).

 A complete discussion can be found in nsswitch.conf(4). 

[..]
> > and the name should not be cached from previous DNS replies... Still,
> > interesting :)
> 
> Use a tracing tool make a look behind the scenes ...
> 
> truss -f nslookup 10.0.16.60 8.8.8.8

Probably the requests go into some door_* call ...


Johann

On Mon, Aug 05, 2013 at 03:14:21AM +0200, Jim Klimov wrote:
> Hello all,
> 
>    I am trying to make milter-greylist work with Sun/Oracle Messaging
> Server (part of Oracle unified Communications Suite now), and there
> is a problem which I've touched on recently - its partial milter-API
> implementation. While extending and porting some rulesets from our
> Sendmail-based relays, I found that the "domain" keyword only has
> the bracketed IP-quad as the submitter host's name, like "[1.2.3.4]".
> 
>    Since milter-greylist does use DNS a lot anyway (RBL, SPF, etc.)
> I wonder if it is possible to add a re-request into DNS for such
> botched remote client names? Perhaps there is already a keyword to
> enable such behavior?
> 
>    Also, are there any configuration patterns to enable DNS-based
> tests that the remote host's HELO/EHLO name matches the textual
> name in the DNS PTR entry for its IP address, and that this name
> from DNS PTR resolves back to this IP address (or includes it
> among multiple values) - i.e. what I believe Sendmail does when
> estimating address "forgery"?

Sendmial provides macro client_resolve, I use it like that:

sm_macro "maybe_forged" "{client_resolve}" "FORGED"
racl greylist sm_macro "maybe_forged" delay 1h autowhite 3d


>    I tried to print in milter-greylist's "msg" clause the values
> of "sendmail macros" listed in different articles and blogs, and
> found that if_addr, client_name, client_ptr are not defined; the
> helo is defined to whatever the remote host wrote about itself,
> client_addr is defined to the IP address (no brackets), and I did
> not find a macro which would contain the domain name (%d in milter
> greylist formatting), which is the IP in brackets.

The real problem has it root in how Solaris (back into acient days)
handles IP to hostname mapping: Even a valid mapping from ip to hostname
exists, if the hostname does not exist or does not map back to the
originated IP the name is not taken! Gave me headache in all the
IP translation stuff for years ...
Solaris calls this kind of double-reverse check "security" (other call
it paranoid, e.g. TCP wrappers package). ;)

I didn't find a way to circumvent this behavior (in Solaris,
except with something like a dynamic library hook or similar).

Johann

Hello all,
   As a follow-up on the recent post about un-bracketing unresolved names,
I'd like to submit a rework of that code and a new patch for review.
* milter-greylist-4.4.3-resolveBrackets.patch

This updated patch moves the code to construct an ip-addr.arpa
name for DNS PTR lookups from a provided IP address string into
a separate function (not published into headers at the moment).

The routine uses a caller-provided buffer to store the resulting
string, so I hope this is thread-safe the way I understand it
(not much, really). Review and constructive criticism welcome ;)

Beside keeping the code cleaner, this separation is intended to
help if a DNS forgery/mismatch routine (keyword?) were to be
implemented. I am not sure I'll get around to coding it, though.
Also note that the code was not yet really tested with "clear"
IP address strings (with only dotted-quads without brackets),
though it should work.

I thought about also moving the DNS lookup into a function, but
it is relatively compact, and with parsing of output structure -
pretty unique anyway (forgery detection would be different).

The updated patch also includes keywords unbracket and nounbracket
to enable or disable this functionality via config-file. The default
is to do resolutions of bad names, and "nounbracket" can be used by
customers with bad DNS resolvers to achieve legacy behaviour as it
works (or doesn't) today. If it is deemed that legacy should be
default for backwards compatibility, the default value can be set
in code, and the other flag variable is available and tested to
enable these name resolutions :)

Man page also updated; default config samples, readmes, etc. - no.
BTW, I believe this is a very old timestamp in the manpage file:
.TH "greylist.conf" "5" "May 10, 2005" "" ""


* milter-greylist-4.4.3-override-packaging-strings.patch

This allows a package maintainer to override the package email and
url contacts, the name and version, add a version suffix and build
a new name+version+suffix string. Also this adds output of these
values into help/error-help text so that the version can be easily
reviewed (i.e. a locally maintained build number can go into this,
or a date-time-stamp, to identify the resulting binaries; or really
for distro-maintenance).

Probably the most popular override for private distros would be
   --with-package-version-suffix="-MyOrg"
which would be appended to PACKAGE_VERSION and PACKAGE_STRING based
on current release's "upstream" version.

Tested to compile, work, and also influence the RPM spec-file.


* milter-greylist-4.4.3-jimbuild-Makefile.patch

This is basically what I posted around the 4.4.2 timeframe - the
improved Makefile.in which makes possible to build this package
over NFS using parallel and sequential builds for different steps.
Since over the past months there are no reports of this slightly
complicated logic breaking anyone's builds, and there is at least
my report that it allows to build the project at least on my farm,
I think it may be added to the dev-branch of code, and sometime
in the future might even make it into releases ;)



** I also use the patch to fix the chown errors for inet sockets,
but it is not mine and IIRC it was added to current trunk...


PS: Also, I've previously posted a patch to use the client_addr
macro instead of if_addr if the former is not available; maybe
called milter-greylist-4.4.3-augment-if_addr-by-client_addr.diff
I now believe that given the macros' different content and purpose,
that patch would be misleading and should not be included.
Absent input is absent, after all; it should not be replaced by
an arbitrary string just because we have it - even if this is
only used for cosmetic outputs right now ;)

HTH,
//Jim Klimov

On 2013-08-05 10:54, Emmanuel Dreyfus wrote:
> racl blacklist not helo "%d" msg "HELO does not match reverse DNS"

Just tested - this does not work, the %d is not expanded in this
context. It does match literally "HELO %d" though :)

//Jim

On 2013-08-05 15:19, Johann Klasek wrote:
> Sendmial provides macro client_resolve, I use it like that:
>
> sm_macro "maybe_forged" "{client_resolve}" "FORGED"
> racl greylist sm_macro "maybe_forged" delay 1h autowhite 3d

Unfortunately, the CommSuite Messaging Server with which I am 
integrating now, does not define this macro. But thanks for
suggestion, it might be useful on our other Sendmail relays.

> The real problem has it root in how Solaris (back into acient days)
> handles IP to hostname mapping: Even a valid mapping from ip to hostname
> exists, if the hostname does not exist or does not map back to the
> originated IP the name is not taken! Gave me headache in all the
> IP translation stuff for years ...
> Solaris calls this kind of double-reverse check "security" (other call
> it paranoid, e.g. TCP wrappers package). ;)

I am not sure I ever saw such behavior, but I've only dealt
with it since late Solaris 7 - mostly 8-10 and open descendants.

While this may have been a problem for dynamic protocols like
DHCP or TFTP/BOOTP with sanity checks for the picked network
config of the host itself (net-booting is tricky with non-default
subnets, etc), I don't think similar checks were a system default
for testing names of remote hosts, nor for static config of the
local host...

> I didn't find a way to circumvent this behavior (in Solaris,
> except with something like a dynamic library hook or similar).

Thanks,
//Jim

Jim Klimov <jimklimov@...> wrote:

> Just tested - this does not work, the %d is not expanded in this
> context. It does match literally "HELO %d" though :)

That is a bug and it sould be easy to fix, feel free to send a patch.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Jim Klimov <jimklimov@...> wrote:

> * milter-greylist-4.4.3-jimbuild-Makefile.patch
> 
> This is basically what I posted around the 4.4.2 timeframe - the
> improved Makefile.in which makes possible to build this package
> over NFS using parallel and sequential builds for different steps.
> Since over the past months there are no reports of this slightly
> complicated logic breaking anyone's builds, and there is at least
> my report that it allows to build the project at least on my farm,
> I think it may be added to the dev-branch of code, and sometime
> in the future might even make it into releases ;)

Could you cleanup commented commands in that one? For instance:
+#TOUCH=                @TOUCH@
+TOUCH=         touch

I think you can skip autoconf here, as touch(1) is required by SUSv2
http://pubs.opengroup.org/onlinepubs/007904875/utilities/touch.html

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Jim Klimov <jimklimov@...> wrote:

> * milter-greylist-4.4.3-override-packaging-strings.patch

I am fine with the autoconf part, but you use it when displaying usage.
Why not add the new information after current output of milter-greylist
-r ? It looks made for it.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Jim Klimov <jimklimov@...> wrote:

> The updated patch also includes keywords unbracket and nounbracket
> to enable or disable this functionality via config-file. 

I am not sure I understand correctly: unbracket is the default and
should do the current behavior, right? Why add a new option.

Anyway, I am not sure it is on purpose, but current behavior is
inconditionnaly altered: if  priv->priv_hostname[0] == '[' ) then it
will get resolved

A note on style: milter-greylist never published coding style guidelines
mandate no more than 80 char/line, and no spaces inside parenthesis.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On 2013-08-10 18:52, manu@... wrote:
> Jim Klimov <jimklimov@... <mailto:jimklimov%40cos.ru>> wrote:
>
>  > * milter-greylist-4.4.3-override-packaging-strings.patch
>
> I am fine with the autoconf part, but you use it when displaying usage.
> Why not add the new information after current output of milter-greylist
> -r ? It looks made for it.

Hmm... indeed, it looks like the proper place. I did not realize
that there was a method to show the detailed release information.

Still, the convention I see in many programs is to show the version
and distro information as part of the help message, and/or as a
separate "-V|--version" method. So I'd argue this output should be
added to both locations in the code :)

Also note that the hacked autoconf part can also influence the RPM
spec-file, and maybe some other locations (depending on how they can
receive the overridden variables - so far I tested the plain variables
in the shell-script context of the generated configure script, and
the config.h undef's and re-define's. If there are other avenues of
the value propagation - they might be or not be handled by this fix.

//Jim Klimov

On 2013-08-10 18:52, manu@... wrote:
> Jim Klimov <jimklimov@... <mailto:jimklimov%40cos.ru>> wrote:
>
>  > The updated patch also includes keywords unbracket and nounbracket
>  > to enable or disable this functionality via config-file.
>
> I am not sure I understand correctly: unbracket is the default and
> should do the current behavior, right? Why add a new option.

Basically, because a distro maintainer or you as the vanilla tarball
maker is free to pick the hardcoded default (retain current behavior
and leave bracketed IP-quads "as is", or use new behavior and try to
resolve these). Whichever the default is, the end-user has a keyword
to explicitly enable or disable this logic; latter mostly reserved
for cases of "thread-unsafe" DNS resolvers and somesuch.

> Anyway, I am not sure it is on purpose, but current behavior is
> inconditionnaly altered: if priv->priv_hostname[0] == '[' ) then it
> will get resolved

No, at least in the second version of the patch that I posted (which
introduces the tunable). If the hostname starts with the bracket, we
evaluate the tunable flag and debug-log either that we will resolve
the bad name or skip it (and in case of enabled debug, we do log
something of this either way). If the unbracketing is disabled, we
skip the routine and goto next logic. Yes, maybe this is how goto's
are deemed not stylish... but this was not the first one in codebase
so I thought it's okay ;)

> A note on style: milter-greylist never published coding style guidelines
> mandate no more than 80 char/line, and no spaces inside parenthesis.

I tried to adhere to this myself, but I guess a few lines slipped
past, mostly due to long strings or tabbing... as an important
factor: what indentation to the unpublished guidelines prefer?
4-space tabs? 8? Another? ;)

It may also be that this code should be broken into more separate
routines (as some classics put it, if the function is over 100 lines
or needs more than three tabs - split it!) but I am not quite ready
to undertake this here... except if to move the whole new code into
a function and only invoke it to verify the priv_hostname. Hmmm...
this would likely also let remove the unstylish goto's ;)

Would you require the style and/or structure changes like this
before accepting the patch?

Thanks,
//Jim

On 2013-08-10 18:52, manu@... wrote:
> Jim Klimov <jimklimov@... <mailto:jimklimov%40cos.ru>> wrote:
>
>  > * milter-greylist-4.4.3-jimbuild-Makefile.patch
>  >
>  > This is basically what I posted around the 4.4.2 timeframe - the
>  > improved Makefile.in which makes possible to build this package
>  > over NFS using parallel and sequential builds for different steps.
>  > Since over the past months there are no reports of this slightly
>  > complicated logic breaking anyone's builds, and there is at least
>  > my report that it allows to build the project at least on my farm,
>  > I think it may be added to the dev-branch of code, and sometime
>  > in the future might even make it into releases ;)
>
> Could you cleanup commented commands in that one? For instance:
> +#TOUCH= @TOUCH@
> +TOUCH= touch
>
> I think you can skip autoconf here, as touch(1) is required by SUSv2
> http://pubs.opengroup.org/onlinepubs/007904875/utilities/touch.html

I guess you're right, sorry for keeping the commented part.
For some reason, $TOUCH did not resolve on one of the build systems
at some point (while worked for others) so I quickly commented it
away and forgot to remove.

Cleaned-up version attached.

//Jim

On 2013-08-10 19:58, Jim Klimov wrote:
> On 2013-08-10 18:52, manu@... <mailto:manu%40netbsd.org> wrote:
>  > Jim Klimov <jimklimov@... <mailto:jimklimov%40cos.ru>
> <mailto:jimklimov%40cos.ru>> wrote:
>  >
>  > > * milter-greylist-4.4.3-override-packaging-strings.patch
>  >
>  > I am fine with the autoconf part, but you use it when displaying usage.
>  > Why not add the new information after current output of milter-greylist
>  > -r ? It looks made for it.
>
> Hmm... indeed, it looks like the proper place. I did not realize
> that there was a method to show the detailed release information.
>
> Still, the convention I see in many programs is to show the version
> and distro information as part of the help message, and/or as a
> separate "-V|--version" method. So I'd argue this output should be
> added to both locations in the code :)

Added to both locations, broke the lines to fit into 80 chars, though
some existing examples nearby break this code of honour ;)
Also fixed the help text by making the "-p socket" optional as it is.

//Jim

Manu, can you clarify something?

The priv_hostname is defined to be char[ADDRLEN+1], where
ADDRLEN is the name of local user part plus the domain part
and some optional characters, amounting to 324 bytes (dump.h)

Why ADDRLEN+1 and not the NS_MAXDNAME+1 ?

I assume your choice is bigger and will replace the NS_MAXDNAME
sizes in my code to match this, so that we don't happen to trim
anything unintentionally... but the "why?" remains ;)

//Jim

Jim Klimov <jimklimov@...> wrote:

> I tried to adhere to this myself, but I guess a few lines slipped
> past, mostly due to long strings or tabbing... as an important
> factor: what indentation to the unpublished guidelines prefer?
> 4-space tabs? 8? Another? ;)

The Unpublished Rules say sthat tabs are used as 8 spaces :-)
 
> Would you require the style and/or structure changes like this
> before accepting the patch?

Well, since I will do it if you do not, I of course prefer if you do. It
would also be nice to move the thing that happen after if (
priv->priv_hostname[0] == '[' ) to a separate function, for the sake of
readability.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Jim Klimov <jimklimov@...> wrote:

> I guess you're right, sorry for keeping the commented part.
> For some reason, $TOUCH did not resolve on one of the build systems
> at some point (while worked for others) so I quickly commented it
> away and forgot to remove.

I committed it, with the configure test after all. Perhaps it could be
useful to someone that wants to add wrappers? I do not know, but we
already do it for true and test, which are likely to be shell built-in
anyway...

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Jim Klimov <jimklimov@...> wrote:

> Why ADDRLEN+1 and not the NS_MAXDNAME+1 ?

Because it was introduced at a time milter-greylist did not do any DNS
lookup on its own, and therefore it did not include the header that
would have given it NS_MAXDNAME. I guess NS_MAXDNAME makes more sense
now we have it at hand.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Jim Klimov <jimklimov@...> wrote:

> Still, the convention I see in many programs is to show the version
> and distro information as part of the help message, and/or as a
> separate "-V|--version" method. So I'd argue this output should be
> added to both locations in the code :)

Well, why not? Anyone else has opinion on this?

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Okay, so here goes the third version of the un-bracketer :)

There was quite a bit of indentation, so formatting may still be a bit
ugly, especially around the debug-printers; but at least it should all
fit into 80 chars now, sometimes just barely missing the mark ;)

I've also moved the whole routine into a separate function as proposed
earlier, so now we have a generic DNS PTR resolver that can be fed both
bracketed and "clear" dot-quad IP numbers (resolution of "clear" IPs
was still not tested though; resolution of bracketed strings works as
it did before the code-move). If the resolution fails at any step, the
original hostname (IP number, bracketed or clear) is copied into the
target storage string.

I am not sure how much this routine would be usable in the forgery
tests I had in mind (compare the HELO string, reverse name for the IP
address and the forward IPs for these names, and "pass" the test if
these match "strictly" or at least are in the same domain "loosely").
The problem is that while preparing this code, I've read somewhere
that in general, several PTR entries for one IP number are allowed
(even though few programs implement and handle this). For the forgery
test to be correct, this should iterate for all PTR entries and their
forward A/CNAME->A resolutions. The function I made so far only returns
the last resolved PTR entry (though it should debug-log them all if
it ever encounters several replies to one IP.in-addr.arpa. request).


On 2013-08-11 02:06, Emmanuel Dreyfus wrote:
> Jim Klimov <jimklimov@...> wrote:
>
>> I tried to adhere to this myself, but I guess a few lines slipped
>> past, mostly due to long strings or tabbing... as an important
>> factor: what indentation to the unpublished guidelines prefer?
>> 4-space tabs? 8? Another? ;)
>
> The Unpublished Rules says that tabs are used as 8 spaces :-)
>
>> Would you require the style and/or structure changes like this
>> before accepting the patch?
>
> Well, since I will do it if you do not, I of course prefer if you do.

Done ... hopefully ... ummm... acceptably now :)
As I said, some indentations remained "compressed" into 4 spaces,
but mostly 8-long tabs are used.

> It would also be nice to move the thing that happen after if (
> priv->priv_hostname[0] == '[' ) to a separate function, for the sake of
> readability.

Done, there's even more sake's involved :)

 >  > Why ADDRLEN+1 and not the NS_MAXDNAME+1 ?
 >
 > Because it was introduced at a time milter-greylist did not do any DNS
 > lookup on its own, and therefore it did not include the header that
 > would have given it NS_MAXDNAME. I guess NS_MAXDNAME makes more sense
 > now we have it at hand.

To remain compatible with current code, left ADDRLEN+1 in my additions,
and commented on that discrepancy. Changes to the more reasonable value
should be done carefully by inspecting all locations, and not just some
point-and-should that I did initially. Not that we're likely to overflow
either one with real names - but some spammer-squatters might like to
crash SMTP filters by forging max-sized names and hunting for such
one-off discrepancies, I suppose...

HTH,
//Jim Klimov

Jim Klimov <jimklimov@...> wrote:

> Added to both locations, broke the lines to fit into 80 chars, though
> some existing examples nearby break this code of honour ;)

The fact that your neighbor breaks the law is not a good reason for
breaking it yourself :-)

I checked that patch in.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On 2013-08-12 09:30, Emmanuel Dreyfus wrote:
> On Mon, Aug 12, 2013 at 09:14:08AM +0200, Jim Klimov wrote:
>> So, this multiple DNS PTR is possible in theory, happens in practice,
>
> Do we want to take care of all SPF corner cases? That defense is
> already weak, I am not sure it is worth investing on it.

This would not be even SPF defense, but rather classic (though IMHO
not yet obsolete) "DNS Sanity Check" - that the registered "reverse"
(PTR) and "forward" (A) entries for the connecting IP address match
(according to some definition of "match" - are identical, are in same
textual 2/3-level domain, etc.) A further check would be to involve
the HELO name comparison into this as well.

This all established relative validity of the sending host itself -
that it is properly administered by people with access to relevant
forward and reverse DNS zones. Over a decade I had about 5 or so
valid senders who had not any control over their reverse DNS (such
as having a "dial-up" consumer address, though static over time)
and did need to communicate with our domains, so they got into our
white-lists. Nowadays such hosts are accepted via SPF, if that is
set up properly for their domains; and other "forged" sources might
fall into a lengthy greylist (if logic to detect them does appear).

The envelope/email FROM domain does not come into consideration for
DNS forgery tests, unlike SPF, since one host with its (ideally one)
domain name all-around can be a relay for dozens of different hosted
domains.

//Jim Klimov

Jim Klimov <jimklimov@...> wrote:

> Okay, so here goes the third version of the un-bracketer :)

I was reading it and it seemed a bit overkill to me to do the 
a.b.c.d -> d.c.b.a.in-addr.arpa transformation on your own. That adds
many lines and still fail to support IPv6. Is your system able to use
getnameinfo(), which is part of POSIX1?

Here is a sample proram to test, with both IPv4 or IPv6 addresses. It
even works if you supply a DNS address.

#include <stdio.h>
#include <err.h>
#include <sysexits.h>
#include <netdb.h>

int
main(argc, argv)
        int argc;
        char **argv;
{
        char *addr;
        struct addrinfo *res;
        char hbuf[NI_MAXHOST];
        int rc;

        if (argc != 2)
                errx(EX_USAGE, "%s IP", argv[0]);

        addr = argv[1];

        rc = getaddrinfo(addr, NULL, NULL, &res);
        if (rc != 0)
                errx(EX_OSERR, "failed parsing %s: %s", 
                     addr, gai_strerror(rc));           
           
        rc = getnameinfo(res->ai_addr, res->ai_addrlen,
                         hbuf, sizeof(hbuf), NULL, 0, NI_NAMEREQD);
        if (rc != 0)
                errx(EX_OSERR, "failed resolving %s: %s", 
                     addr, gai_strerror(rc));           

        freeaddrinfo(res);

        printf("%s => %s\n", addr, hbuf);

        return 0;
}


-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On Wed, Aug 14, 2013 at 07:18:41AM +0200, manu@... wrote:
> Jim Klimov <jimklimov@...> wrote:
> 
> > Okay, so here goes the third version of the un-bracketer :)
> 
> I was reading it and it seemed a bit overkill to me to do the 
> a.b.c.d -> d.c.b.a.in-addr.arpa transformation on your own. That adds
> many lines and still fail to support IPv6. Is your system able to use
> getnameinfo(), which is part of POSIX1?

That's what I am wondering about too. But as mentioned in a posting
before, the below solution under Solaris may suffer from the resolve-
double-checking behavior. However, maybe this can be neglected in favour of
not to reinvent the wheel and code stability ...

> Here is a sample proram to test, with both IPv4 or IPv6 addresses. It
> even works if you supply a DNS address.

To get to program (at least) on Solaris 8 to work:
(BSD calls err(), errx() do not exist - at least on old Solaris versions)

To build with "gcc -o getaddrinfo -lsocket -lnsl getaddrinfo.c"

--- lnx/getaddrinfo.c   Wed Aug 14 17:04:14 2013
+++ sol/getaddrinfo.c   Wed Aug 14 17:46:26 2013
@@ -1,6 +1,9 @@
 
 #include <stdio.h>
-#include <err.h>
+/* #include <sys/err.h>
+*/
+#include <sys/socket.h>
+
 #include <sysexits.h>
 #include <netdb.h>
 
@@ -14,21 +17,27 @@
         char hbuf[NI_MAXHOST];
         int rc;
 
-        if (argc != 2)
-                errx(EX_USAGE, "%s IP", argv[0]);
+        if (argc != 2) {
+                fprintf(stderr,"%s IP\n", argv[0]);
+                exit(EX_USAGE);
+       }
 
         addr = argv[1];
 
         rc = getaddrinfo(addr, NULL, NULL, &res);
-        if (rc != 0)
-                errx(EX_OSERR, "failed parsing %s: %s",
+        if (rc != 0) {
+                fprintf(stderr, "failed parsing %s: %s\n",
                      addr, gai_strerror(rc));
+                exit(EX_OSERR);
+       }
 
         rc = getnameinfo(res->ai_addr, res->ai_addrlen,
                          hbuf, sizeof(hbuf), NULL, 0, NI_NAMEREQD);
-        if (rc != 0)
-                errx(EX_OSERR, "failed resolving %s: %s",
+        if (rc != 0) {
+                fprintf(stderr, "failed resolving %s: %s\n",
                      addr, gai_strerror(rc));
+                exit(EX_OSERR);
+       }
 
         freeaddrinfo(res);
 


Johann E. K.

Jim Klimov <jimklimov@...> wrote:

> > I was reading it and it seemed a bit overkill to me to do the
> > a.b.c.d -> d.c.b.a.in-addr.arpa transformation on your own. That adds
> > many lines and still fail to support IPv6. Is your system able to use
> > getnameinfo(), which is part of POSIX1?
> 
> Well, we are only as crafty as the tools we know about ;)

Here is a patch that does it using getaddrinfo/getnameinfo
http://ftp.espci.fr/shadow/manu/unbracket.patch

Please tell me if it fits your needs.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On 2013-08-14 18:08, Johann Klasek wrote:
> On Wed, Aug 14, 2013 at 07:18:41AM +0200, manu@...
> <mailto:manu%40netbsd.org> wrote:
>  > Jim Klimov <jimklimov@... <mailto:jimklimov%40cos.ru>> wrote:
>  >
>  > > Okay, so here goes the third version of the un-bracketer :)
>  >
>  > I was reading it and it seemed a bit overkill to me to do the
>  > a.b.c.d -> d.c.b.a.in-addr.arpa transformation on your own. That adds
>  > many lines and still fail to support IPv6. Is your system able to use
>  > getnameinfo(), which is part of POSIX1?
>
> That's what I am wondering about too. But as mentioned in a posting
> before, the below solution under Solaris may suffer from the resolve-
> double-checking behavior. However, maybe this can be neglected in favour of
> not to reinvent the wheel and code stability ...
>
>  > Here is a sample proram to test, with both IPv4 or IPv6 addresses. It
>  > even works if you supply a DNS address.
>
> To get to program (at least) on Solaris 8 to work:
> (BSD calls err(), errx() do not exist - at least on old Solaris versions)
>
> To build with "gcc -o getaddrinfo -lsocket -lnsl getaddrinfo.c"

I used -lresolv -lsocket instead, but yes, errx() is absent and can be
replaced by a macro to do the error reporting (yes, there is more to
a "correct" output, but for a trivial case like this - the short macro
suffices):

# define errx(E, FMT...) fprintf(stderr, FMT)

I did not encounter the problems you've posted about in the past weeks
about a Solaris resolver problem, and we use a range from Solaris 8 to
10, and via OpenSolaris SXCE to current illumos-based OSes. Then again,
I am not a programmer but a sysadmin, and maybe the problems you see as
a programmer are hidden from me by programs and tools which take these
"inconvenient features" into account and work around them - so that's
why I never saw them...

As for reinventing the wheel - I do not object. I posted about some of
my reservations - whether the standard way would allow us to detect
forgery mismatches in case of multi-DNS-PTR entries; if "yes" - I can
have no objections at all :)

//Jim

On 2013-08-15 06:52, manu@... wrote:
> Here is a patch that does it using getaddrinfo/getnameinfo
> http://ftp.espci.fr/shadow/manu/unbracket.patch
>
> Please tell me if it fits your needs.

I did not compile and test it yet, but from code review:

# This is in wrong order by a couple of lines - you should first
set the priv->priv_hostname value with strncpy(), then fix it :)

@@ -375,8 +376,11 @@
         priv->priv_buflen = 0;
         priv->priv_max_elapsed = 0;
         priv->priv_last_whitelist = EXF_NONE;

+       if ((priv->priv_hostname[0] = '[') && conf.c_unbracket)
+               (void)resolve_bracketed(priv->priv_hostname);
+
         strncpy(priv->priv_hostname, hostname, ADDRLEN);
         priv->priv_hostname[ADDRLEN] = '\0';

         if (addr != NULL) {


# Also, this is geared towards exactly resolving a bracketed name.
My code could also do resolution in the (theoretical) case that a
numeric IP address was passed without brackets. Then again, this
routine is called "unbracketing" for a reason ;)

# In the routine itself you have an ifdef... I think it warrants
an #else to report that unbracketing was needed and requested, but
the program was configured and built without proper DNS support.

# Mine also had some more debugging info about the process (that
the unbracketing was called, that it failed or succeeded, that there
were several DNS PTR hits and we only picked one, etc.) which the
current implementation lacks. I found it convenient to have while
debugging the configuration...

Overall, this is more compact and simple - less prone to breakage
due to our own neglect. If the concerns about debug-logging and
about the order of operations are addressed, and this does also
work equivalently to mine, I'd be okay with this change ;)

I hope to test it tonight sometime...

//Jim

On 2013-08-15 06:52, manu@... wrote:
> Jim Klimov <jimklimov@... <mailto:jimklimov%40cos.ru>> wrote:
>
>  > > I was reading it and it seemed a bit overkill to me to do the
>  > > a.b.c.d -> d.c.b.a.in-addr.arpa transformation on your own. That adds
>  > > many lines and still fail to support IPv6. Is your system able to use
>  > > getnameinfo(), which is part of POSIX1?
>  >
>  > Well, we are only as crafty as the tools we know about ;)
>
> Here is a patch that does it using getaddrinfo/getnameinfo
> http://ftp.espci.fr/shadow/manu/unbracket.patch
>
> Please tell me if it fits your needs.

Addon to previous review: I do not quite see how this routine does
NOT change the original string in addr in case of any lookup errors?
Did I miss something (i.e. maybe getnameinfo() does not modify it
upon failure - always on all platforms)?

//Jim

Jim Klimov <jimklimov@...> wrote:

> # This is in wrong order by a couple of lines - you should first
> set the priv->priv_hostname value with strncpy(), then fix it :)

Right.

> # Also, this is geared towards exactly resolving a bracketed name.
> My code could also do resolution in the (theoretical) case that a
> numeric IP address was passed without brackets. Then again, this
> routine is called "unbracketing" for a reason ;)

I prefer to address real problems rather than potential ones.
 
> # In the routine itself you have an ifdef... I think it warrants
> an #else to report that unbracketing was needed and requested, but
> the program was configured and built without proper DNS support.

Updated version: http://ftp.espci.fr/shadow/manu/unbracket2.patch

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Jim Klimov <jimklimov@...> wrote:
 
> Addon to previous review: I do not quite see how this routine does
> NOT change the original string in addr in case of any lookup errors?
> Did I miss something (i.e. maybe getnameinfo() does not modify it
> upon failure - always on all platforms)?

Good question. Anyone has an experience of that?

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On 2013-08-15 16:14, Emmanuel Dreyfus wrote:
> Jim Klimov <jimklimov@...> wrote:
>
>> Addon to previous review: I do not quite see how this routine does
>> NOT change the original string in addr in case of any lookup errors?
>> Did I miss something (i.e. maybe getnameinfo() does not modify it
>> upon failure - always on all platforms)?
>
> Good question. Anyone has an experience of that?
>



Well, the one experience I had with my code led to separate char[]
arrays and inspection of the return code - and if was a success,
I copied the string returned by the lookup into the priv_hostname.

//Jim

Hi,

>>>>> On Thu, 15 Aug 2013 16:14:51 +0200
>>>>> manu@... said:

> Jim Klimov <jimklimov@...> wrote:
>
> Addon to previous review: I do not quite see how this routine does
> NOT change the original string in addr in case of any lookup errors?
> Did I miss something (i.e. maybe getnameinfo() does not modify it
> upon failure - always on all platforms)?

manu> Good question. Anyone has an experience of that?

If NI_NAMEREQD is not specified, getnameinfo(3) copies a result on
success, and copies a numeric form of the node's address on fail.
AFAIK, if NI_NAMEREQD is specified, getnameinfo(3) on BSDs copies a
result only on success, and doesn't touch addr on fail.  However, I'm
not sure if there is such implementation that changes addr on fail.
POSIX says only on success:

    Upon successful completion, getnameinfo() shall return the node
    and service names, if requested, in the buffers provided. The
    returned names are always null-terminated strings.

Sincerely,

--
Hajimu UMEMOTO
ume@...  ume@{,jp.}FreeBSD.org
http://www.mahoroba.org/~ume/

Hajimu UMEMOTO <ume@...> wrote:

>  However, I'm
> not sure if there is such implementation that changes addr on fail.
> POSIX says only on success:
> 
>     Upon successful completion, getnameinfo() shall return the node
>     and service names, if requested, in the buffers provided. The
>     returned names are always null-terminated strings.

Thank you for digging that out. Let us start with assuming the standard
are honoured, we will add workaround later if they are not.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On 2013-08-16 02:15, Emmanuel Dreyfus wrote:
> Hajimu UMEMOTO <ume@...> wrote:
>
>>   However, I'm
>> not sure if there is such implementation that changes addr on fail.
>> POSIX says only on success:
>>
>>      Upon successful completion, getnameinfo() shall return the node
>>      and service names, if requested, in the buffers provided. The
>>      returned names are always null-terminated strings.
>
> Thank you for digging that out. Let us start with assuming the standard
> are honoured, we will add workaround later if they are not.

The way I read it, the standard only determines the actions about
success, and not about failure - which is implementation-dependent
(replace the original string with the IP address number, or don't
touch the original string at all). Given that the milter-greylist
rulesets may depend on bracketed names (as mine do, to delay the
message until a later retry might have successful DNS resolution),
I think it would be portable to ensure that we do the same thing
on all platforms - by using the getnameinfo() with a temporary copy
of the string, inspecting the return code, and copying the string
into original buffer if it resolved successfully. This way we won't
have to guess later on about esoteric (or popular) platforms which
might have private quirks. This only costs a few lines of code and
about 300 bytes of temporary buffer space ;)

//Jim

Jim Klimov <jimklimov@...> wrote:

> The way I read it, the standard only determines the actions about
> success, and not about failure 

If it is not broken, do not fix it. The situation you think about is
usually worded in standards as "the buffer content in case of failure is
undefined", which is not the case here. Moreover, I have to think hard
on how this could be implemented with a dirty output buffer on failure.

Let us first find an instance where there is a problem here, then we
will fix it. I doubt we will encounter such a situation.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On Thu, Aug 15, 2013 at 04:14:49PM +0200, manu@... wrote:
> Jim Klimov <jimklimov@...> wrote:
> 
> > # This is in wrong order by a couple of lines - you should first
> > set the priv->priv_hostname value with strncpy(), then fix it :)
> 
> Right.
> 
> > # Also, this is geared towards exactly resolving a bracketed name.
> > My code could also do resolution in the (theoretical) case that a
> > numeric IP address was passed without brackets. Then again, this
> > routine is called "unbracketing" for a reason ;)
> 
> I prefer to address real problems rather than potential ones.
>  
> > # In the routine itself you have an ifdef... I think it warrants
> > an #else to report that unbracketing was needed and requested, but
> > the program was configured and built without proper DNS support.
> 
> Updated version: http://ftp.espci.fr/shadow/manu/unbracket2.patch

Just want to note, that the bracketed version sendmail provides may look like

[IPv6:2a01:1b0:7999:446:0:2:8ed:2c78]

This is the form sendmail falls back to a hostname if the ip addr does not resolve.
In a MG log it looks like 

Aug 16 06:11:23 neelix milter-greylist: r7G4BN1J030057: skipping greylist because this is the default action, (from=<XXXXXX@...>, rcpt=<xxxxxxx@...>, addr=[IPv6:2a01:1b0:7999:446:0:2:8ed:2c78][2a01:1b0:7999:446:0:2:8ed:2c78]) ACL 614

as opposed to 

Aug 16 08:06:07 neelix milter-greylist: r7G667ba013248: skipping greylist because this is the default action, (from==netbsd.org@...>, rcpt=<xxx@...>, addr=mail.NetBSD.org[2001:4f8:3:7::25]) ACL 614


It would be interessting, what Oracle CommunicationServer defines ...

Maybe the function resolve_bracketed() should handle this case too.


Johann

On 2013-08-16 10:52, Johann Klasek wrote:
> It would be interessting, what Oracle CommunicationServer defines ...

I am afraid I can not answer that responsibly - there is no IPv6
anywhere I have access too. So far it is only a theoretical construct
for us ;)

> Maybe the function resolve_bracketed() should handle this case too.

Possibly, at least that was one of the rationales to replace my
homebrew implementation with a concise standard function ;)

//Jim

Jim Klimov <jimklimov@...> wrote:

>         rc = getnameinfo(res->ai_addr, res->ai_addrlen,
> -                        addr, IPADDRSTRLEN, NULL, 0, NI_NAMEREQD);
> +                        abuf, IPADDRSTRLEN, NULL, 0, NI_NAMEREQD);
> -       if (rc != 0)
> +       if (rc != 0) {
>                 mg_log(LOG_INFO, "failed resolving %s: %s",
>                        addr, gai_strerror(rc));
> +       } else {
> +              strncpy(addr, abuf, IPADDRSTRLEN);
> +              addr[IPADDRSTRLEN] = '\0';
> +       }

Ok, I added that.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Johann Klasek <johann@...> wrote:

> Just want to note, that the bracketed version sendmail provides may look like
> 
> [IPv6:2a01:1b0:7999:446:0:2:8ed:2c78]

You mean we first have to strip "[" or "[IPv6:" ? 

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On Fri, Aug 16, 2013 at 03:15:48PM +0200, manu@... wrote:
> Johann Klasek <johann@...> wrote:
> 
> > Just want to note, that the bracketed version sendmail provides may look like
> > 
> > [IPv6:2a01:1b0:7999:446:0:2:8ed:2c78]
> 
> You mean we first have to strip "[" or "[IPv6:" ? 

Right, just in case of "[IPv6:" the starting pointer of the string to
unbracket can be advanced to the position beyond the ":" ... However, this
would make this function more general and removes any fear about how sendmail
passes bakc bracketed IPs. ;)


Johann

Hi,

>>>>> On Fri, 16 Aug 2013 15:15:48 +0200
>>>>> manu@... said:

manu> Johann Klasek <johann@...> wrote:

> Just want to note, that the bracketed version sendmail provides may look like
> 
> [IPv6:2a01:1b0:7999:446:0:2:8ed:2c78]

manu> You mean we first have to strip "[" or "[IPv6:" ? 

JFYI, RFC 5321 defines address-literal as follows:

address-literal  = "[" ( IPv4-address-literal /
                   IPv6-address-literal /
                   General-address-literal ) "]"
                   ; See Section 4.1.3

IPv4-address-literal  = Snum 3("."  Snum)

IPv6-address-literal  = "IPv6:" IPv6-addr

General-address-literal  = Standardized-tag ":" 1*dcontent

Standardized-tag  = Ldh-str
                    ; Standardized-tag MUST be specified in a
                    ; Standards-Track RFC and registered with IANA

IPv6-addr      = IPv6-full / IPv6-comp / IPv6v4-full / IPv6v4-comp

IPv6-hex       = 1*4HEXDIG

IPv6-full      = IPv6-hex 7(":" IPv6-hex)

IPv6-comp      = [IPv6-hex *5(":" IPv6-hex)] "::"
                 [IPv6-hex *5(":" IPv6-hex)]
                 ; The "::" represents at least 2 16-bit groups of
                 ; zeros.  No more than 6 groups in addition to the
                 ; "::" may be present.

IPv6v4-full    = IPv6-hex 5(":" IPv6-hex) ":" IPv4-address-literal

IPv6v4-comp    = [IPv6-hex *3(":" IPv6-hex)] "::"
                 [IPv6-hex *3(":" IPv6-hex) ":"]
                 IPv4-address-literal
                 ; The "::" represents at least 2 16-bit groups of
                 ; zeros.  No more than 4 groups in addition to the
                 ; "::" and IPv4-address-literal may be present.

Sincerely,

--
Hajimu UMEMOTO
ume@...  ume@{,jp.}FreeBSD.org
http://www.mahoroba.org/~ume/

Hajimu UMEMOTO <ume@...> wrote:

> manu> You mean we first have to strip "[" or "[IPv6:" ? 
> 
> JFYI, RFC 5321 defines address-literal as follows:
> 
> address-literal  = "[" ( IPv4-address-literal /
>                    IPv6-address-literal /
>                    General-address-literal ) "]"
>                    ; See Section 4.1.3
> 
> IPv4-address-literal  = Snum 3("."  Snum)
> 
> IPv6-address-literal  = "IPv6:" IPv6-addr

I tried the test program published earlier on NetBSD, "::1" is resolved
to "localhost", "IPv6:::1" is not. I guess IPv6: shall be stripped.

I get it working with this:

        if (strncmp(addr, "[IPv6:", 6) == 0) {
                (void)strncpy(abuf, addr + 6, len - 7);
                abuf[len - 7] = '\0';
        } else {        /* Strip leading '[' and trailing ']' */
                (void)strncpy(abuf, addr + 1, len - 2);
                abuf[len - 2] = '\0';
        }

I wonder if this is a but in NetBSD getnameinfo(3)

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

Hi,

>>>>> On Fri, 16 Aug 2013 18:40:56 +0200
>>>>> manu@... said:

manu> I tried the test program published earlier on NetBSD, "::1" is resolved
manu> to "localhost", "IPv6:::1" is not. I guess IPv6: shall be stripped.

manu> I get it working with this:

manu>         if (strncmp(addr, "[IPv6:", 6) == 0) {
manu>                 (void)strncpy(abuf, addr + 6, len - 7);
manu>                 abuf[len - 7] = '\0';
manu>         } else {        /* Strip leading '[' and trailing ']' */
manu>                 (void)strncpy(abuf, addr + 1, len - 2);
manu>                 abuf[len - 2] = '\0';
manu>         }

manu> I wonder if this is a but in NetBSD getnameinfo(3)

The notation of `[IPv6:IPv6-addr]' is just SMTP things, and
getaddrinfo(3) and getnameinfo(3) don't handle it.  So, application
has to strip '[IPv6:' and `]'.

Sincerely,

--
Hajimu UMEMOTO
ume@...  ume@{,jp.}FreeBSD.org
http://www.mahoroba.org/~ume/

On Fri, Aug 16, 2013 at 06:40:56PM +0200, manu@... wrote:
> Hajimu UMEMOTO <ume@...> wrote:
> 
> > manu> You mean we first have to strip "[" or "[IPv6:" ? 
> > 
> > JFYI, RFC 5321 defines address-literal as follows:
> > 
> > address-literal  = "[" ( IPv4-address-literal /
> >                    IPv6-address-literal /
> >                    General-address-literal ) "]"
> >                    ; See Section 4.1.3
> > 
> > IPv4-address-literal  = Snum 3("."  Snum)
> > 
> > IPv6-address-literal  = "IPv6:" IPv6-addr
> 
> I tried the test program published earlier on NetBSD, "::1" is resolved
> to "localhost", "IPv6:::1" is not. I guess IPv6: shall be stripped.

Got the same result on Solaris 8, Linux Fedora 15. In my opinion this is ok
(see below).

> 
> I get it working with this:
> 
>         if (strncmp(addr, "[IPv6:", 6) == 0) {
>                 (void)strncpy(abuf, addr + 6, len - 7);
>                 abuf[len - 7] = '\0';
>         } else {        /* Strip leading '[' and trailing ']' */
>                 (void)strncpy(abuf, addr + 1, len - 2);
>                 abuf[len - 2] = '\0';
>         }

Looks ok to me.

> I wonder if this is a but in NetBSD getnameinfo(3)

A bug?
No, getnameinfo() has no sense about brackated addresses at all, which is used in
RFC5321 (SMTP) context only.

Sendmail log contains always [IPv6:] style IPv6 addresses, even if they
are not resolveable:

Resolveable:
relay=mail.NetBSD.org [IPv6:2001:4f8:3:7::25]

Not resolveable:
relay=[IPv6:2a01:1b0:7999:446:0:2:8ed:2c78]

In the latter case sendmail sets the macros like this

client_name: [IPv6:2001:41d0:8:452c::1]
client_addr: IPv6:2001:41d0:8:452c::1

(always in compressed representation)

I found it interesting that even client_addr is a typical resolver
information and not directly related to SMTP, the IPv6 prefix is
preserved ...
(just to note for the ruleset hackers out there).




Johann

Hajimu UMEMOTO <ume@...> wrote:

> The notation of `[IPv6:IPv6-addr]' is just SMTP things, and
> getaddrinfo(3) and getnameinfo(3) don't handle it.  So, application
> has to strip '[IPv6:' and `]'.

Right, I fixed it.

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...