Thank you Paul for your comments. I have accepted most of your comments. Please see in-line. Have also updated the webrev at
http://cr.opensolaris.org/~girishmg/tcpsig/ thanks ~Girish Paul Wernau wrote: > > ----- > ipsecconf.c > > 840: s/alogrithm/algorithm/ > > 3688: I know you're probably using the text from above, but this error > message is misleading. In the bypass policy case, it is obvious that > putting an algorithm in a bypass rule is wrong. In the TCP signature > case, it is not self evident. > > How about "tcp signature algorithm found in combination with ipsec > action, not allowed. > > Nit - be consistent with capitalization of tcp in user visible > messages. I prefer the TCP to be capitalized. (Users have scripts > that grep for things and consistency is helpful.) > > 4861: s/and negotiate/or negotiate/ as any one of these parameters > will invalidate rule. Also, ulp should really say ulp/proto since > these are synonyms in the parser. Accepted. > > ---- > ipseckey.c > > 896: Can you expand this comment to include the bounds. How long can > this string be, is it variable length, etc. > > 912: s/a auth string/an auth string/ Accepted. > > 930: You detect a value is not a printable character, then you try to > print it as a string. This can potentially crash. Doesn't this emit > ugly garbage at the very least? Try entering in > "[CTRL-V][Backspace][CTRL-V]C" and see what happens. Actually was allowing all the ASCII characters. Now have restricted to printable ASCII characters (codes 33 - 126) as specified in RFC. > > 3060: s/as SPI do no matter/as the SPI value does not matter/ > ---- > > keysock.c: > > 2355: Please top post a small comment about the context and usage of > this function. (i.e. explain that it is stripping out keying material > and it is used for TCP signature's keysock reply) > > 2457: ISTR that ASSERT(0) is frowned upon. I don't have anything to > back that up, though. > ---- > > ipsec_impl.h > > 231: throw a comment at the end of the line for consistency Accepted. > > > ---- > > inet/tcp.h: 749 > tcp.c: 655, 13456 > > tcp_opt_sig_digest and TCP_SIG_OK use a magic 16. This should be a > defined constant or use TCP_OPT_SIG_LEN - 2. It is probably worth > explaining the -2 if you do that. > > tcp.c > > 16224: nit. It is really a bit and not a boolean :) Accepted. > > 24054-24075: again, the 20, 18, etc should really be using defined > constants. (TCP_MIN_HEADER_LENGTH?) Am now using TCPOPT_SIG_LEN as those numbers refer to it. > > ----- > > pfkeyv2.h > > Where did you come up with these numbers? Definitely run them by > danmcd first (the author of pfkey v2 RFC). I'm especially concerned > about SADB_AALG_MD5 being 1. > > See openbsd header file for instance... > > http://fxr.watson.org/fxr/source/net/pfkeyv2.h?v=OPENBSD#L275 Will talk with Dan. > > > --- > ipsec_util.c > Makefile.files > ip.h > ip.c > spd.c > spdsock.c > ip_stack.h > tcp_sig.c > pfpolicy.h > netinet/tcp.h > > No comments from me. > > ---- > > -Paul _______________________________________________ networking-discuss mailing list [email protected]
