Dave, See my responses inline and the updated webrev at:
http://cr.opensolaris.org/~tonyn/firewall13Jan2009-inc/ http://cr.opensolaris.org/~tonyn/firewall13Jan2009/ Thanks much, tony David Powell wrote: > Tony Nguyen wrote: >> Dave, >> >> The incremental and revised full webrev are respectively at: >> >> http://cr.opensolaris.org/~tonyn/firewallDec112008-inc/ >> http://cr.opensolaris.org/~tonyn/firewallDec112008/ >> >> Some more closures to prior discussion below. >> >> The only portion of the code left to be reviewed are the ipf rule >> generation. > > I haven't been over everything with a fine-tooth comb yet, but this > is the bulk of the remainder: > > servinfo.c: > > Since the error messages from this command could end up in a log > file, they should be internationalized. The normal output is only > ever consumed programmatically, so it isn't necessary to i18n it. > > That said, you have several output formats, many of which look like > they are intended for humans. This is great for debugging, but you > don't need the variety, I'd try to keep it to one format. I eliminated the human friendly output. The two remaining outputs are still applicable to programming consumers. > > servinfo.c`usage: > > Open paren should be on a new line. done > > servinfo.c`svc_getrpcinfo: > > Where does the number 12 (as in buf[12]) come from? > > Instead of mallocing a struct rpcent, just put one on the stack > that you point rpc to if you can't resolve the rpc. done > > Though it is unlikely you'll run up against LINE_MAX, your code > could theoretically emit truncated results if strlcat was unable to > append the entire proto or port to 'line'. That could in turn > cause the firewall to malfunction in an insecure fashion. You > should be tracking the size of line and ensure there's sufficient > space *before* attempting to append the next port or protocol. > done > servinfo.c`uaddr2port: > > I believe the universal address you get from rpcb_getmaps is > provided by the server. Though the server should be our rpcbind, I > don't think it's safe to assume the format of the string, > especially since universal addresses aren't limited to the formats > you list. This function needs to extract port numbers in a more > generic, robust way. Yes, the server is rpcbind but universal address doesn't have standardized formats at the moment. There is a draft rfc at http://www.ietf.org/internet-drafts/draft-ietf-nfsv4-rpc-netid-05.txt Since universal addresses have the form: h1.h2.h3.h4.p1.p2 x1:x2:x3:x4:x5:x6:x7:x8.p1.p2 ::1.p1.p2 How about getting the last two octets? Do you see an alternative? > > ipfd.c: > > max_scf_value_size should be called max_scf_name_size. done > ipfd.c`ipfilter_update: > > This has three callers, all of whom emit a similar error message > and exit. Perhaps this could be put in ipfilter_update. done > Additionally, they all determine failure based on the result of > ipfilter_update, which in turn only looks at whether fork and exec > succeeded. Since the fw_update subcommand of > /lib/svc/method/ipfilter can fail, shouldn't that be communicated > back to the caller? Agreed. A failed fw_update subcommand indicates result in an error message and return value. > > ipfd.c`repository_event_wait: > > You can get rid of the fmri length check entirely. Whatever result > you are getting from one scf function should be valid for passing > to another. done > > ipfilter.xml: > > Some of your <description>s are too verbose. They should only a > sentence or two, and contain no line breaks. The pre-formatted > content you currently have will be mangled by consumers and should > be removed. e.g. for the 'apply_to' and 'exceptions' properties I > would say something like: > > The host and network IPs, network interfaces, and ippools to deny > if the policy is set to deny, or accept if the policy is set to > accept. I thought examples would make it easier for users to set property values. If the content will be mangled, then users will need to use the manpage. I'll make the suggested change. > You need to have <common_name>s in addition to <description>s. > common_names for properties are added. I didn't add common_names for each property values as they don't seem necessary. > The <description> for 'custom' should read: > > Apply the custom ipfilter configuration stored in a custom file > (custom file property must be set). done > > The <description> for 'custom_policy_file' should read: > > The file containing a custom ipfilter configuration to use if a > custom policy is in force. done > The <description> 'open_ports' should read: > > A set of ports to leave open regardless of firewall policy. > done > global.xml: > > Since <common_name>s are used as labels, don't punctuate them with > periods. done > When a property refers to itself, use "this property", not > "nameofthispg/nameofthisproperty". The latter requires > re-localization if the property changes, and is unnecessarily > unfriendly in an environment where the <common_name>s are used > instead of the SMF property names. done > > ipf_include.sh: > > Instead of: > > command > [ $? -ne 0 ] && return 1 > > (and similar constructs), you could just do: > > command || return 1 done > > Instead of elaborate counting logic to limit the number of > iterations of your while loops, just use 'for i in `seq 1 3`; > do...'. done > > Since get_policy is only used with IPF_FMRI explictly, I would have > a separate function or inline the act of getting the default > configuration. It doesn't make sense to force every call to > get_policy to test for IPF_FMRI. > A new function is created to get global default policy. > get_exceptions: are there any callers who pass IPF_FMRI? > There are two cases where callers passes an IPF_FMRI: 1) create_global_rules() 2) generate_rules() when a service's policy is 'use_global' in which case replaces the service's fmri with IPF_FMRI and call get_exceptions() and get_apply2_list() to obtain global default policy. I'll have a function that returns the pg for a given FMRI as you suggested. > I recommend creating a function that returns the property group for > an FMRI, rather than having 'if IPF_FMRI do_something else > do_something' in each of your property-fetching functions. > done > Though the abstraction around inst_ipf_file and inst_nat_file is > nice, the fact that you almost always call both and each call needs > to recompute the FMRI path isn't. I recommend having callers use > fmri_to_file and provide suffix variables that they could use > instead. e.g. file=`fmri_to_file`; my_file=${file}${my_suffix} Yes, callers who need both ipf and nat files will use fmri_to_file() and add the suffixes. I'd like to keep the inst_ipf_file() and inst_nat_file for callers who only uses ipf file, e.g. custom ipf methods. > service_is_online should use svcprop instead of parsing the output > of svcs. > A new function service_check_state() will verify the desired service state. > I also don't understand why you're rejecting services that are in > transition, since you wait for the service to change and a > transition will inevitably occur if that wait isn't to be in vain. > The intention was to get a more stable state, after transitions completed but not indefinitely. Perhaps, waiting for restarter/next_state to become "none" which indicates no further transition before getting the service's state is a better implementation. > Same comments apply to service_is_maintenance. These two should be > a single function that takes the name of the desired state as an > argument. > replaced by service_check_state(). > value_is_interface should return 0 or 1, and that should just be > tested by the shell. It doesn't make sense to test one exit status > only to convert it to a string which you then convert back to an > exit status. > done > get_IP could just do something like: > > echo "$1" | sed -n -e 's,^pool:\(.*\),pool/\1,p' \ > -e 's,^host:\(.*\),\1,p' \ > -e 's,^network:\(.*\),\1,p' > done > In general, code that does > > foo=`echo something | sed something` > echo "$foo" > > could just do > > echo something | sed something > done > validate_interface could just do 'dladm show-dev $1 > /dev/null > 2>&1' and test the result. > dladm show-dev is obsoleted. get_interface now validates network interface using ifconfig. > While the abstractions for check_ipf_syntax and check_nat_syntax > are nice, there's no need to explicitly return 0 if the return > value is 0. Just call the program you're calling and let your > caller get the exit code of what you're calling. > done > In general, boolean functions should communicate their result > though their exit code, not through echoing 'true' or 'false'. > done > tuple_get_port does the work of splitting a port range into pieces, > but then throws the result away. If it is doing so to test the > format of the input, then you shouldn't be doing it again later. > > port_get_lower/power_get_higher could be simpler: > > All callers call both, so you're unnecessarily performing all > your sanity testing twice and processing twice. Make a single > function that sets/returns both lport and hport. > > Perform a single test for well-formedness (assuming > tuple_get_port hasn't done so already): > > echo $1 | grep '^[0-9]{1,5}-[0-9]{1,5}$' > > Then just emit them in the correct order: > > echo $1 | ( IFS=- read a b ; [ $a -lt $b ] && echo $a $b || echo $b > $a ) > I've simplified these functions and tuple_get_port() now check and return the port(s). > ipf_get_lock is racy. A simpler and more robust lock file can be > created by creating a lockfile.$newpid that contains $newpid and > attempting to ln $IPF_LOCK to your lockfile. > > Why does ipf_get_lock fail after 5 seconds? You're blocking on a > lock, and you know the process that holds the lock is live. Either > it will drop the lock, or it will die. Either way you eventually > get the lock. The modified lock creation uses mkdir and no longer fail after 5 seconds. > custom_set_symlink/IPFILCONF: > > You should put all dynamically created files and symlinks under > /var, leaving /etc alone. This is for two reasons: > > 1) Modifying files under /etc will frustrate users who strive to > have a read-only root filesystem. If the configuration of the > system isn't changing, the contents of /etc shouldn't be > changing either. > > 2) Customers with pre-existing configurations will be putting > them in /etc. In other words, your dynamically generated > files conflict with a part of the filesystem namespace people > are already using. Specifically, the #1 custom file people > will be supplying is /etc/ipf/ipf.conf. Selecting/testing a > non-custom configuration shouldn't override that > configuration. > > Instead, you should generate your configuration files into > VAR_IPF_DIR, and change the ipfilter service to read its > configuration from there. Those can then symlink back to /etc if > the customer has put files there. Done. Global ipf files, IPFILCONF, and IPFILOVRCONF are now in /var/tmp/ipf. Custom policy will make IPFILCONF a symlink to the custom rule file. > > replace_file: > > This is always given a regular "new" file; your handling of the > new-is-link case is unnecessary (and doesn't look correct either). > > If the "orig" file is a symlink, I wouldn't bother creating a > backup. Moreover, since in all cases "orig" should be a generated > file, I don't see why storing a backup is that worthwhile. > Finally, if copying "new" to "orig" fails, it means you won't be > loading the correct rules, meaning the caller should fail, too. done > > generate rules: > > Shouldn't the "none" policy specify that packets should be to > ${ip}? done > When the policy is "use_global", why do you generate a per-service > copy of the global rules? Why not just fall back to the global > definitions? Good catch. It makes perfect sense and reduces the number ipf rules. > > This function does essentially the same thing 4 times, yet has 4 > copies of the code to do it. And the same idiom appears in the > global rule generation routines. > Simplified and reduced duplication. > process_service: > > `something > /dev/null` is a strange construct; I'm not sure what > you're trying to do, but I know that it will cause some versions of > ksh93 to crash. I'm trying to exec another script in a subshell but don't want the output. Would ( exec $cmd $args >/dev/null ) be better? > create_services_rules: > > When you return from the loop, you're not removing the lock file. It should continue rather than "return" if processing failed for a service. The returns are removed.
