|
Replies:
1
-
Last Post:
Sep 8, 2009 1:14 AM
by: mph
|
|
|
Posts:
110
From:
US
Registered:
4/16/08
|
|
|
|
[nwam-discuss] SMF and IPFilter related code review comments:
paul-01...05 and tony-01...15
Posted:
Sep 3, 2009 9:27 AM
|
|
Paul and Tony,
Apologize for the delay in getting to your code review comments.
The webrev for these changes against the current nwam gate is at
http://zhadum.east/export/ws/am223141/temp/nwam1-smf-review/webrev/
The original webrev that went out for code review is at
http://cr.opensolaris.org/~mph/nwam1_cr/
> paul-01 s/IPSec/IPsec/
corrected
> paul-02 net-loc'265: do_nis(): Keep in mind that the ypservers, if
> listed by name, need to be in /etc/hosts. NIS looks explicitly there
> and does not so regular nameservice lookup. Not sure if this is really
> the place to address it, but there probably should be some warning
> when the user is doing manual config and uses a hostname.
Changed the library such that only ip addresses are allowed for
nis-nameservice-servers property.
> paul-03 net-loc'399: It would probably be good to not use the default
> nsswitch.nis file. Especially if you have both dns and NIS enabled. In
> general, one only needs NIS for passwd and automount (and possibly
> printers). At the very least, it would be good to have hosts and
> ipnodes list dns first, if available.
That part of the code is not relevant anymore. The validation function
in the library updates the nameservice-config-file. If only one
nameservice is specified and nameservice-config-file is not given, then
nameservice-config-file is updated to point to the respective nsswitch
file. If more than one nameservice is specified, then
nameservice-config-file is mandatory. The location is not validated if
nameservice-config-file is missing in this case.
> paul-04 net-nwam:revert_to_legacy_loc() disables IPsec policy and
> IPfilter policy, changes some properties, then re-enables them. This
> leaves open a window of opportunity where there is no network security
> policy, which is a bad thing. Can't you change the config file
> locations in the SMF properties and do a refresh/restart instead
> without first disabling? Or refresh/restart/innocuous enable in case
> the service wasn't started before?
corrected this to do the same as do_sec() in net-loc. The services are
not disabled at first. They are disabled only if the Legacy location
doesn't have the *-config-file property.
> tony-01 The integration of 6855845, Allow Property Modification in SMF
> profiles, into b119 should be sufficient for NWAM phase next to
> quickly update location specific values and simplify logic in various
> services and their inter-relationships.
> tony-02 Is there a reason why /etc/svc/volatile is used instead of
> /var/run or a non-persistent pg?
Both are tmpfs and non-persistent. We chose /etc/svc/volatile. dladm
is also in /etc/svc/volatile.
> tony-03 ipf, ipf6 files: Comments on line 3 and 16 suggest these files
> aren't the final version.
Removed the comments suggesting that the files are not final.
> tony-04 usr/src/cmd/svc/milestone/network-service.xml - No update?
> However, are dependencies (e.g. NIS dependencies) and comments still
> accurate if the service is now only responsible for updating DNS
> information?
Yes. If nwam is enabled, the net-svc script exits. The net-loc script
sets up the remaining configuration depending on the location that's
being activated. If nwam is not enabled, the net-svc script proceeds as
before. Some tasks from network/service was spun-off into
network/netmask and network/ipqos services.
> tony-05 network-physical.xml'41,87 - It's not clear from the
> design spec that there will be additional network/physical instances.
> Clarification please?
The history seems to have lost the comment that went with this
particular change. I think Renee has the answers. Will chat with her
early next week.
> tony-06 network-physical.xml'74 - unnecessary diff?
hmm ... strangely I don't see it. Maybe spaces changed to tabs!
> tony-07 network-location.xml'100-113 manifest-import is defined as
> a dependent but the comment states it should be a dependency. Has
> testing been done to verify correct behavior?
Corrected. the "dependency" tag should be used in the xml and has
already been fixed in the project gate
> tony-08 svc method scripts: These files have similar
> implementation to figure out whether a service is enabled. It's
> probably better to have define a single service_is_enabled() in
> net_include.sh that these three files can consume.
I've moved the function to determine whether a service is enabled or not
into net_include.sh, calling it service_is_enabled() and return 0 if
enabled, 1 otherwise. Also, moved the get_loc_prop() function into
net_include.sh, calling it nwam_get_loc_prop().
> tony-09 create/revert_legacy_loc doesn't correctly save/restore
> custom file. User may have a policy other than custom but has a valid
> configuration file. The current code doesn't correctly save the
> configuration file if policy is something other than 'custom' thus
> can't properly restore it.
If the policy is not custom, then when creating the Legacy location we
save the value of the policy. In the ipfilter-config-file property,
this value is preceded by a '/' since the property requres that the file
be absolute. For any location, the user specifies the config file and
net-loc only changes the policy (to "custom") and the
custom_policy_file. When the Legacy location is installed (on disabling
nwam), the policy is set back to what is was. If the policy was not
custom, then the '/' is removed from the ipfilter-config-file and set to
policy.
If Host-based firewall provided with an export-like function, then it
will be much easier for nwam. We could simply "export" the ipfilter
configuration and save that configuration file in the
ipfilter-config-file property. When installing the Legacy location,
nwam could simply import the exported file and the configuration would
be back to what is was. Is there something like this in the works?
> tony-10 net-loc'501 - clearing custom policy_file is probably
> unnecessary since policy is set to 'none'.
removed the emptying of the custom_policy_file. Also, in net-nwam:do_sec()
> tony-11 net-loc'502 - refresh_ipf also needs to be set here to
> make the changes effective.
Rather than setting refresh_ipf, I've explicitly refreshed ipfilter.
That's because if refresh_ipf is set, ipfilter service remains online
(even though the policy is "none"). Not setting the refresh_ipf changes
the ipfilter service to disabled. This makes it less confusing for
users to see if ipfilter is on by simplying doing "svcs ipfilter".
> tony-12 net-loc'stop_svc() - in all uses of this function, no
> configuration change was made to the target service so a refresh isn't
> necessary.
removed "svcadm refresh" from stop_svc().
> tony-13 net-loc'refresh_svc() - The name seems incomplete as the
> function really is refreshing/starting the service with the updated
> configuration. Perhaps run_svc?
I renamed this to start_svc() and collapsed refresh_svc() and
restart_svc(). start_svc() refreshes the service and then either
restarts or temporarily enables the service depending on whether the
service is already running or not.
> tony-15 I have another concern regarding location-based IPfilter
> configuration. The current implementation requires users to supply a
> customized ipf rules for every location if they want to configure
> IPfilter. This essentially undo the functionality of Host-based
> firewall which simplified IPfilter configuration. We ought to allow
> users to configure location with the option of using their existing
> IPFilter configuration, in fact, I'd argued that option should be the
> default.
Changes made to the system when a particular NCP or location is active
is not persistent and it lost when the system is rebooted, NWAM
restarted or location changed. For example, if a user plumbs an
interface with ifconfig, then it is not plumbed the next time the system
is rebooted. As for the Host-based firewall, the changes are also
temporary, in effect until the location is changed. By the way, how do
users access the new firewall configuration? Is there a GUI or CLI? One
way I see using it would be to have an option to save the configured
IPFilter settings (using an export-like function) and then setting the
active location's ipfilter-config-file to point to that. If there's a
GUI, then a button to "save config to current location" would achieve this.
Thanks,
Anurag
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
Posts:
542
From:
Registered:
5/11/05
|
|
|
|
Re: [nwam-discuss] SMF and IPFilter related code review comments:
paul-01...05 and tony-01...15
Posted:
Sep 8, 2009 1:14 AM
in response to: anurag_m
|
|
On Thu, 03 Sep 2009 12:27:37 -0400
Anurag Maskey <Anurag dot Maskey at Sun dot COM> wrote:
> Paul and Tony,
>
> Apologize for the delay in getting to your code review comments.
>
> The webrev for these changes against the current nwam gate is at
>
> http://zhadum.east/export/ws/am223141/temp/nwam1-smf-review/webrev/
I'm not the SMF and/or IPFilter gurus but to my eyes these changes
looked good.
mph
>
> The original webrev that went out for code review is at
>
> http://cr.opensolaris.org/~mph/nwam1_cr/
>
> > paul-01 s/IPSec/IPsec/
> corrected
>
> > paul-02 net-loc'265: do_nis(): Keep in mind that the ypservers, if
> > listed by name, need to be in /etc/hosts. NIS looks explicitly there
> > and does not so regular nameservice lookup. Not sure if this is really
> > the place to address it, but there probably should be some warning
> > when the user is doing manual config and uses a hostname.
> Changed the library such that only ip addresses are allowed for
> nis-nameservice-servers property.
>
> > paul-03 net-loc'399: It would probably be good to not use the default
> > nsswitch.nis file. Especially if you have both dns and NIS enabled. In
> > general, one only needs NIS for passwd and automount (and possibly
> > printers). At the very least, it would be good to have hosts and
> > ipnodes list dns first, if available.
> That part of the code is not relevant anymore. The validation function
> in the library updates the nameservice-config-file. If only one
> nameservice is specified and nameservice-config-file is not given, then
> nameservice-config-file is updated to point to the respective nsswitch
> file. If more than one nameservice is specified, then
> nameservice-config-file is mandatory. The location is not validated if
> nameservice-config-file is missing in this case.
>
> > paul-04 net-nwam:revert_to_legacy_loc() disables IPsec policy and
> > IPfilter policy, changes some properties, then re-enables them. This
> > leaves open a window of opportunity where there is no network security
> > policy, which is a bad thing. Can't you change the config file
> > locations in the SMF properties and do a refresh/restart instead
> > without first disabling? Or refresh/restart/innocuous enable in case
> > the service wasn't started before?
> corrected this to do the same as do_sec() in net-loc. The services are
> not disabled at first. They are disabled only if the Legacy location
> doesn't have the *-config-file property.
>
> > tony-01 The integration of 6855845, Allow Property Modification in SMF
> > profiles, into b119 should be sufficient for NWAM phase next to
> > quickly update location specific values and simplify logic in various
> > services and their inter-relationships.
>
> > tony-02 Is there a reason why /etc/svc/volatile is used instead of
> > /var/run or a non-persistent pg?
> Both are tmpfs and non-persistent. We chose /etc/svc/volatile. dladm
> is also in /etc/svc/volatile.
>
> > tony-03 ipf, ipf6 files: Comments on line 3 and 16 suggest these files
> > aren't the final version.
> Removed the comments suggesting that the files are not final.
>
> > tony-04 usr/src/cmd/svc/milestone/network-service.xml - No update?
> > However, are dependencies (e.g. NIS dependencies) and comments still
> > accurate if the service is now only responsible for updating DNS
> > information?
> Yes. If nwam is enabled, the net-svc script exits. The net-loc script
> sets up the remaining configuration depending on the location that's
> being activated. If nwam is not enabled, the net-svc script proceeds as
> before. Some tasks from network/service was spun-off into
> network/netmask and network/ipqos services.
>
> > tony-05 network-physical.xml'41,87 - It's not clear from the
> > design spec that there will be additional network/physical instances.
> > Clarification please?
> The history seems to have lost the comment that went with this
> particular change. I think Renee has the answers. Will chat with her
> early next week.
>
> > tony-06 network-physical.xml'74 - unnecessary diff?
> hmm ... strangely I don't see it. Maybe spaces changed to tabs!
>
> > tony-07 network-location.xml'100-113 manifest-import is defined as
> > a dependent but the comment states it should be a dependency. Has
> > testing been done to verify correct behavior?
> Corrected. the "dependency" tag should be used in the xml and has
> already been fixed in the project gate
>
> > tony-08 svc method scripts: These files have similar
> > implementation to figure out whether a service is enabled. It's
> > probably better to have define a single service_is_enabled() in
> > net_include.sh that these three files can consume.
> I've moved the function to determine whether a service is enabled or not
> into net_include.sh, calling it service_is_enabled() and return 0 if
> enabled, 1 otherwise. Also, moved the get_loc_prop() function into
> net_include.sh, calling it nwam_get_loc_prop().
>
> > tony-09 create/revert_legacy_loc doesn't correctly save/restore
> > custom file. User may have a policy other than custom but has a valid
> > configuration file. The current code doesn't correctly save the
> > configuration file if policy is something other than 'custom' thus
> > can't properly restore it.
> If the policy is not custom, then when creating the Legacy location we
> save the value of the policy. In the ipfilter-config-file property,
> this value is preceded by a '/' since the property requres that the file
> be absolute. For any location, the user specifies the config file and
> net-loc only changes the policy (to "custom") and the
> custom_policy_file. When the Legacy location is installed (on disabling
> nwam), the policy is set back to what is was. If the policy was not
> custom, then the '/' is removed from the ipfilter-config-file and set to
> policy.
>
> If Host-based firewall provided with an export-like function, then it
> will be much easier for nwam. We could simply "export" the ipfilter
> configuration and save that configuration file in the
> ipfilter-config-file property. When installing the Legacy location,
> nwam could simply import the exported file and the configuration would
> be back to what is was. Is there something like this in the works?
>
> > tony-10 net-loc'501 - clearing custom policy_file is probably
> > unnecessary since policy is set to 'none'.
> removed the emptying of the custom_policy_file. Also, in net-nwam:do_sec()
>
> > tony-11 net-loc'502 - refresh_ipf also needs to be set here to
> > make the changes effective.
> Rather than setting refresh_ipf, I've explicitly refreshed ipfilter.
> That's because if refresh_ipf is set, ipfilter service remains online
> (even though the policy is "none"). Not setting the refresh_ipf changes
> the ipfilter service to disabled. This makes it less confusing for
> users to see if ipfilter is on by simplying doing "svcs ipfilter".
>
> > tony-12 net-loc'stop_svc() - in all uses of this function, no
> > configuration change was made to the target service so a refresh isn't
> > necessary.
> removed "svcadm refresh" from stop_svc().
>
> > tony-13 net-loc'refresh_svc() - The name seems incomplete as the
> > function really is refreshing/starting the service with the updated
> > configuration. Perhaps run_svc?
> I renamed this to start_svc() and collapsed refresh_svc() and
> restart_svc(). start_svc() refreshes the service and then either
> restarts or temporarily enables the service depending on whether the
> service is already running or not.
>
> > tony-15 I have another concern regarding location-based IPfilter
> > configuration. The current implementation requires users to supply a
> > customized ipf rules for every location if they want to configure
> > IPfilter. This essentially undo the functionality of Host-based
> > firewall which simplified IPfilter configuration. We ought to allow
> > users to configure location with the option of using their existing
> > IPFilter configuration, in fact, I'd argued that option should be the
> > default.
> Changes made to the system when a particular NCP or location is active
> is not persistent and it lost when the system is rebooted, NWAM
> restarted or location changed. For example, if a user plumbs an
> interface with ifconfig, then it is not plumbed the next time the system
> is rebooted. As for the Host-based firewall, the changes are also
> temporary, in effect until the location is changed. By the way, how do
> users access the new firewall configuration? Is there a GUI or CLI? One
> way I see using it would be to have an option to save the configured
> IPFilter settings (using an export-like function) and then setting the
> active location's ipfilter-config-file to point to that. If there's a
> GUI, then a button to "save config to current location" would achieve this.
>
> Thanks,
> Anurag
>
> _______________________________________________
> nwam-discuss mailing list
> nwam-discuss at opensolaris dot org
> http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
_______________________________________________
nwam-discuss mailing list
nwam-discuss at opensolaris dot org
http://mail.opensolaris.org/mailman/listinfo/nwam-discuss
|
|
|
|
|
