[PATCH 1/2] Validate network interface name when parsing the kickstart (#1081982)

Anne Mulhern amulhern at redhat.com
Tue Sep 9 11:54:18 UTC 2014





----- Original Message -----
> From: "Anne Mulhern" <amulhern at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Monday, September 8, 2014 5:35:37 PM
> Subject: Re: [PATCH 1/2] Validate network interface name when parsing the	kickstart (#1081982)
> 
> 
> 
> 
> 
> ----- Original Message -----
> > From: "Martin Kolman" <mkolman at redhat.com>
> > To: anaconda-patches at lists.fedorahosted.org
> > Sent: Monday, September 8, 2014 1:49:42 PM
> > Subject: [PATCH 1/2] Validate network interface name when parsing the
> > 	kickstart (#1081982)
> > 
> > Resolves: rhbz#1081982
> > Signed-off-by: Martin Kolman <mkolman at redhat.com>
> > ---
> >  pykickstart/commands/network.py | 55
> >  +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> > 
> > diff --git a/pykickstart/commands/network.py
> > b/pykickstart/commands/network.py
> > index 7b1341c..44ddb8d 100644
> > --- a/pykickstart/commands/network.py
> > +++ b/pykickstart/commands/network.py
> > @@ -517,9 +517,64 @@ class RHEL6_Network(F9_Network):
> >          op.add_option("--bondopts", dest="bondopts")
> >          return op
> >  
> > +def validate_network_interface_name(name):
> > +    """Check if the given network interface name is valid, return an error
> > message
> > +    if an error is found or None if no errors are found
> > +
> 
> Could you give a link here to the specification of the network interface name
> format
> or else specify that format here?
> 
> > +    :param str name: name to validate
> > +    :returns: error message or None if no error is found
> > +    :rtype: str or None
> 
> This is very pedantic, but None is a value and _NoneType_ is its type.
> 
> > +    """
> > +    # (for reference see the NetworkManager source code:
> > +    #  NetworkManager/src/settings/plugins/ifcfg-rh/reader.c
> > +    #  and the make_vlan_setting function)
> > +
> > +    min_vlan_id = 0
> > +    max_vlan_id = 4095
> 
> The above are constants, so should be capitalized.
> 
> > +    vlan_id = None
> > +
> 
> For the condition below, partition is probably better:
> 
> (prefix, sep, vlan_id) = name.partition(".")
> if sep:
>   if prefix == "vlan":
>       return ...
>   try:
>       vlan_id = int(vlan_id)
>   except ...:
>       ...
> 
> That way, you have to do less indexing and you don't have to worry about len
> and max splits
> for split() function.

Except that, whoops, you don't want to set vlan_id in result of partition,
so code should actually be something like (with one further simplification):

(vlan, _dot, id_candidate) = name.partition(".")
if vlan == "vlan":
   vlan_id = int(id_candidate)
etc.

> 
> > +    # if it contains '.', vlan id should follow (eg 'ens7.171',
> > 'mydev.171')
> > +    split_name = name.split(".", 1)
> > +    if len(split_name) > 1:
> > +        # 'vlan' can't be followed by a '.'
> > +        if split_name[0] == "vlan":
> > +            return _("When using the <prefix>.<vlan id> interface name
> > notation, <prefix> can't be equal to 'vlan'.")
> > +
> > +        try:
> > +            vlan_id = int(split_name[1])
> > +        except ValueError:
> > +            return _("If network --interfacename contains a '.', valid
> > vlan
> > id should follow.")
> 
> Is vlang.123 an acceptable id? If so, then the block below should be an elif.
> If not,
> then the block below should probably be the first block, so that vlang.123
> gets thrown
> out at first check. Right now it gets accepted by first block but rejected by
> second
> block.
> 
> Should use partition below as well as strip() is not really what you want.
> 
> Something like:
> 
> (empty, sep, vlan_id) = name.partition("vlan")
> if empty == "":
>     try:
>         vlan_id = int(vlan_id)
>     except ....
>         ....
> 
> That way you avoid accepting "vlan123lvan" by bad coincidence.
> 

Don't want to set vlan_id in result of partition here, either, so need to make the
same change as above.

Also, I was probably carried away by my enthusiasm for partition() here, probably

(vlan, id_candidate) = (name[:4], name[4:])
if vlan == "vlan":
    try:
        vlan_id = int(id_candidate)
    etc.

would be just as good, maybe better.

> > +
> > +    # if it starts with 'vlan', vlan id should follow ('vlan171')
> > +    if name.startswith("vlan"):
> > +        try:
> > +            vlan_id = int(name.strip("vlan"))
> > +        except ValueError:
> > +            return _("If network --interfacename starts with 'vlan', valid
> > vlan id should follow.")
> > +
> > +    # check if the vlan id is in range
> > +    if vlan_id is not None:
> > +        if not(min_vlan_id <= vlan_id <= max_vlan_id):
> > +            return _("The vlan id out of the %d-%d vlan id range." %
> > (min_vlan_id, max_vlan_id))
> > +

Also, I think this code will perform incorrectly on the string "meh".

"meh" does not have a '.' so it does not meet first condition and it does not start
with "vlan", so it doesn't meet the second condition. This means that vlan_id is None
and we just end up returning None, which is wrong.

So, what we need above is something like:

if vlan_id is None:
   return <some appropriate error message>
else:
   # check if the vlan is is in range 
   etc.

> > +    # network interface name seems to be valid (no error found)
> > +    return None
> > +
> >  class RHEL7_Network(F20_Network):
> >      def _getParser(self):
> >          op = F20_Network._getParser(self)
> >          op.add_option("--interfacename", dest="interfacename",
> >          action="store",
> >                  default="")
> >          return op
> > +
> > +    def parse(self, args):
> > +        # call the overridden command to do it's job first
> > +        retval = F20_Network.parse(self, args)
> > +
> > +        # validate the network interface name
> > +        error_message =
> > validate_network_interface_name(retval.interfacename)
> > +        # something is wrong with the interface name
> > +        if error_message is not None:
> > +            raise
> > KickstartValueError(formatErrorMsg(self.lineno,msg=error_message))
> > +
> > +        return retval
> > --
> > 1.9.3
> > 
> > _______________________________________________
> > anaconda-patches mailing list
> > anaconda-patches at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> > 
> 
> Thanks for writing the tests to go along with, they make it a lot
> easier for me to think about the code.
> 
> - mulhern
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

- mulhern


More information about the anaconda-patches mailing list