Ceri, Thanks for the comments.
Ceri Davies wrote: > On Sun, Oct 19, 2008 at 02:49:40AM -0700, Tony Nguyen wrote: >> This is for 6761070 PSARC 2008/580 Solaris host-based firewall >> <http://monaco.sfbay/detail.jsf?cr=6761070>* >> >> *The webrev and the new event message are respectively at: >> >> http://cr.opensolaris.org/~tonyn/firewall/ >> >> http://cr.opensolaris.org/~tonyn/firewall/8000-R4* >> >> *A quick summary of the changes: >> - firewall policy configuration for network services >> - generate IPfilter rules from firewall policy >> - network service monitoring daemon (refresh/restart actions) >> - additional maintenance cause, service request maintenance >> - IPfilter configuration migration > > Hi Tony, here are my comments. > > usr/src/cmd/ssh/etc/sshd: > > create_ipf_rules() is too simple. Multiple Port entries are > allowed in sshd_config, and they can also be specified in > ListenAddress entries. You're quite right, multiple port entries should be supported. On the other hand, I'd like to defer ListenAddress support for a future rfe since most users don't modify ListenAddress entries. Is that reasonable? > > usr/src/cnd/fs.d/nfs/svc/nfs-server: > > Typo at line 131, "mountd is an RPC daemond." << daemon is misspelt. fixed > Typo at line 166, "it if hasn't..." << IF it hasn't.. fixed > Slightly uncomfortable just sleeping for one second without > rechecking. Yes, I don't like the wait myself. However, in my testing, 1 second is sufficient so if mountd hasn't come up then there maybe a problem. In the case that mountd didn't start, the /usr/lib/servinfo will not return any port so we simply don't generate any rules for mountd. > usr/src/cmd/ipf/svc/ipfd.c > > Lines 537-543: That looks like a syslog entry that hasn't been > finished. Is this just going to write "fmri: svc://network/rpc/bind" > or whatever to a logfile? Won't that be confusing? Why not just > do nothing? Yes, it can be removed but may serve some debugging purposes. How about something like: /* * Service, instance, or pg deleted. */ syslog(LOG_DEBUG | LOG_DAEMON, "Deleted: %s", fmri); > > usr/src/cmd/ipf/svc/ipfilter > > Lines 81-83 now invoke "ipnat -CF" multiple times; is there any danger > of losing connectivity when ipnat -CF runs? If so, is there any way > to refactor this so that it only runs once? It's actually incorrect to invoke "ipnat -CF" as it flushes all active filters/session. I fixed the code such that ipnat -CF is called once before we start loading the nat rules. > > usr/src/cmd/lp/cmd/lpsched/print-svc > > Line 143: Again, are we sure this will catch everything? Is it > possible that this has come from a Listen directive, or that there > is more than one? I can't find official documentation but our implementation has interesting behavior. If "Port" directives are used, only one (the last directive) will be active. If Listen directives are used, multiple ports can be active but the "Port" directives are ignored. I modified the code to reflect the described behavior. > > usr/src/cmd/svc/shell/ipf_include.sh > > Line 779: Important typo, "service is either has"... > fixed > Line 856: I think this means "if firewall policy is 'none'", in which > case it should say that. fixed > > Lines 870 and 964: You have defined VAR_RUN_DIR so may as use it (or > remove the definition). definition removed. > > Line 1068: remove this? fixed > > Line 1087: "we update a service's ipf ruleset in the following > manner." fixed > > usr/src/cmd/xntpd/xntpd/xntp > > Line 11: Nitpicking, but s/these/this/ fixed > usr/src/cmd/ypcmd/yp.sh > > Line 35 is indented with spaces, surrounding ones use a tab. fixed > > Line 55: Suggest making this grep case insensitive, as I don't think > lower-case is mandatory in /etc/hosts. fixed > > Line 87: Can this at least be restricted to tcp or udp? > Nice catch, upd is sufficient. > Hope that wasn't too nitpicky, and thanks for including the pool: > functionality. Not at all. Your comments are greatly appreciated. -tony
