Repository: faucetsdn/chewie

Issue:
Exception handling with MessageParser
There are times we raise exceptions during packet parsing which should result in the processed packet being dropped, and Chewie can safely continue processing other packets.

Exceptions (and example of when raised) include:
- KeyError: unknown EAP type.
- UnicodeDecodeError: Illegal bytes in the EAP.identity field.
- ValueError: unknown EAP code.
- struct.error: the payload doesn't contain enough bytes to unpack.

Should all of these be caught by the caller, or a new exception created (e.g. InvalidPacketException)?
A new exception can still contain the original reason.

Problem if we catch them all, the same exception could be raised somewhere else, that we might not expect (e.g. KeyError & ValueError are reasonably common errors for other parts of python to raise with) and it will basically be swallowed.

Does anyone have any thoughts on this?

Discussion:

Patch:
diff --git a/chewie/auth_8021x.py b/chewie/auth_8021x.py
index 8ae4947..0de562f 100644
--- a/chewie/auth_8021x.py
+++ b/chewie/auth_8021x.py
@@ -1,5 +1,7 @@
 import struct
 
+from chewie.utils import MessageParseError
+
 
 def hwaddr_to_string(hwaddr):
     return ":".join(["%02x" % x for x in hwaddr])
@@ -22,9 +24,15 @@ class Auth8021x:
             packed_message (bytes):
         Returns:
             Auth8021x
+        Raises:
+            MessageParseException: if packed_message cannot be parsed successfully.
         """
-        version, packet_type, length = struct.unpack("!BBH",
-                                                     packed_message[:AUTH_8021X_HEADER_LENGTH])
+        try:
+            version, packet_type, length = struct.unpack("!BBH",
+                                                         packed_message[:AUTH_8021X_HEADER_LENGTH])
+        except struct.error as exception:
+            raise MessageParseError(message="Auth8021x unable to parse first 4 bytes",
+                                    original_error=exception) from exception
         data = packed_message[AUTH_8021X_HEADER_LENGTH:AUTH_8021X_HEADER_LENGTH+length]
         return cls(version, packet_type, data)
 
diff --git a/chewie/chewie.py b/chewie/chewie.py
index ccb2a66..580fbc8 100644
--- a/chewie/chewie.py
+++ b/chewie/chewie.py
@@ -11,7 +11,8 @@ from chewie.eap_state_machine import FullEAPStateMachine
 from chewie.radius_lifecycle import RadiusLifecycle
 from chewie.message_parser import MessageParser, MessagePacker
 from chewie.event import EventMessageReceived, EventPortStatusChange
-from chewie.utils import get_logger
+from chewie.utils import get_logger, MessageParseError
+
 
 def unpack_byte_string(byte_string):
     """unpacks a byte string"""
@@ -174,7 +175,16 @@ class Chewie:
             self.logger.info("waiting for eap.")
             packed_message = self.eap_socket.receive()
             self.logger.info("Received packed_message: %s", str(packed_message))
-            eap, dst_mac = MessageParser.ethernet_parse(packed_message)
+            try:
+                eap, dst_mac = MessageParser.ethernet_parse(packed_message)
+            except MessageParseError as exception:
+                self.logger.info(
+                    "MessageParser.ethernet_parse threw exception.\n"
+                    " packed_message: '%s'.\n"
+                    " exception: '%s'.",
+                    packed_message,
+                    exception)
+                continue
             self.send_eap_to_state_machine(eap, dst_mac)
 
     def send_eap_to_state_machine(self, eap, dst_mac):
@@ -199,8 +209,17 @@ class Chewie:
             sleep(0)
             self.logger.info("waiting for radius.")
             packed_message = self.radius_socket.receive()
-            radius = MessageParser.radius_parse(packed_message, self.radius_secret,
-                                                self.radius_lifecycle)
+            try:
+                radius = MessageParser.radius_parse(packed_message, self.radius_secret,
+                                                    self.radius_lifecycle)
+            except MessageParseError as exception:
+                self.logger.info(
+                    "MessageParser.radius_parse threw exception.\n"
+                    " packed_message: '%s'.\n"
+                    " exception: '%s'.",
+                    packed_message,
+                    exception)
+                continue
             self.send_radius_to_state_machine(radius)
             self.logger.info("Received RADIUS message: %s", radius)
 
diff --git a/chewie/eap.py b/chewie/eap.py
index d7d30cd..948cd06 100644
--- a/chewie/eap.py
+++ b/chewie/eap.py
@@ -2,6 +2,8 @@
 
 import struct
 
+from chewie.utils import MessageParseError
+
 EAP_HEADER_LENGTH = 1 + 1 + 2
 EAP_TYPE_LENGTH = 1
 
@@ -27,18 +29,40 @@ class Eap:
 
     @staticmethod
     def parse(packed_message):
-        code, packet_id, length = struct.unpack("!BBH", packed_message[:EAP_HEADER_LENGTH])
+        """
+
+        Args:
+            packed_message:
+        Returns:
+            Eap*** object.
+        Raises:
+            MessageParseError if packed_message cannot be parsed.
+        """
+        try:
+            code, packet_id, length = struct.unpack("!BBH", packed_message[:EAP_HEADER_LENGTH])
+        except struct.error as exception:
+            raise MessageParseError("unable to unpack EAP header (4 bytes)") from exception
+
         if code in (Eap.REQUEST, Eap.RESPONSE):
-            packet_type, = struct.unpack("!B",
-                                         packed_message[EAP_HEADER_LENGTH :
-                                                        EAP_HEADER_LENGTH + EAP_TYPE_LENGTH])
+            try:
+                packet_type, = struct.unpack("!B",
+                                             packed_message[EAP_HEADER_LENGTH :
+                                                            EAP_HEADER_LENGTH + EAP_TYPE_LENGTH])
+            except struct.error as exception:
+                raise MessageParseError("EAP unable to unpack packet_type byte") \
+                    from exception
+
             data = packed_message[EAP_HEADER_LENGTH+EAP_TYPE_LENGTH:length]
-            return PARSERS[packet_type](code, packet_id, data)
+            try:
+                return PARSERS[packet_type](code, packet_id, data)
+            except KeyError as exception:
+                raise MessageParseError("EAP packet_type: %s not supported" % packet_type) \
+                    from exception
         elif code == Eap.SUCCESS:
             return EapSuccess(packet_id)
         elif code == Eap.FAILURE:
             return EapFailure(packet_id)
-        raise ValueError("Got Eap packet with bad code: %s" % packed_message)
+        raise MessageParseError("Got Eap packet with bad code: %s" % packed_message)
 
     def pack(self, packed_body):
         header = struct.pack("!BBHB", self.code, self.packet_id,
@@ -64,7 +88,17 @@ class EapIdentity(Eap):
 
     @classmethod
     def parse(cls, code, packet_id, packed_message):
-        return cls(code, packet_id, packed_message.decode())
+        """
+        Returns:
+            EapIdentity.
+        Raises:
+            MessageParseError if packed message cannot be decoded.
+        """
+        try:
+            identity = packed_message.decode()
+        except UnicodeDecodeError as exception:
+            raise MessageParseError("%s unable to decode identity" % cls.__name__) from exception
+        return cls(code, packet_id, identity)
 
     def pack(self):
         packed_identity = self.identity.encode()
@@ -87,7 +121,17 @@ class EapMd5Challenge(Eap):
 
     @classmethod
     def parse(cls, code, packet_id, packed_message):
-        value_length, = struct.unpack("!B", packed_message[:1])
+        """
+        Returns:
+            EapMd5Challenge.
+        Raises:
+            MessageParseError if cannot unpack packed_message
+        """
+        try:
+            value_length, = struct.unpack("!B", packed_message[:1])
+        except struct.error as exception:
+            raise MessageParseError("%s unable to unpack first byte" % cls.__name__) \
+                from exception
         challenge = packed_message[1:1+value_length]
         extra_data = packed_message[1+value_length:]
         return cls(code, packet_id, challenge, extra_data)
@@ -145,8 +189,17 @@ class EapLegacyNak(Eap):
 
     @classmethod
     def parse(cls, code, packet_id, packed_msg):
+        """
+        Returns:
+            EapLegacyNak.
+        Raises:
+            MessageParseError if cannot unpack packed_message
+        """
         value_len = len(packed_msg)
-        desired_auth_types = struct.unpack("!%ds" % value_len, packed_msg)
+        try:
+            desired_auth_types = struct.unpack("!%ds" % value_len, packed_msg)
+        except struct.error as exception:
+            raise MessageParseError("%s unable to unpack." % cls.__name__) from exception
         return cls(code, packet_id, desired_auth_types)
 
     def pack(self):
@@ -170,11 +223,20 @@ class EapTLSBase(Eap):
 
     @classmethod
     def parse(cls, code, packet_id, packed_msg):
+        """
+        Returns:
+            A child of EapTLSBase e.g. EapTLS, EAPTTLS.
+        Raises:
+            MessageParseError if cannot unpack packed_message
+        """
         value_len = len(packed_msg)
         fmt_str = "!B"
         if value_len > 1:
             fmt_str += "%ds" % (value_len - 1)
-        unpacked = struct.unpack(fmt_str, packed_msg)
+        try:
+            unpacked = struct.unpack(fmt_str, packed_msg)
+        except struct.error as exception:
+            raise MessageParseError("%s unable to unpack" % cls.__name__) from exception
         extra_data = b""
         if value_len > 1:
             flags, extra_data = unpacked
diff --git a/chewie/ethernet_packet.py b/chewie/ethernet_packet.py
index fa7f3d4..3a477eb 100644
--- a/chewie/ethernet_packet.py
+++ b/chewie/ethernet_packet.py
@@ -1,5 +1,6 @@
 import struct
 from chewie.mac_address import MacAddress
+from chewie.utils import MessageParseError
 
 ETHERNET_HEADER_LENGTH = 6 + 6 + 2
 
@@ -19,9 +20,15 @@ class EthernetPacket:
             packed_message (bytes):
         Returns:
             EthernetPacket
+        Raises:
+            MessageParseError: if packed_message cannot be successfully parsed.
         """
-        dst_mac, src_mac, ethertype = struct.unpack("!6s6sH",
-                                                    packed_message[:ETHERNET_HEADER_LENGTH])
+        try:
+            dst_mac, src_mac, ethertype = struct.unpack("!6s6sH",
+                                                        packed_message[:ETHERNET_HEADER_LENGTH])
+        except struct.error as exception:
+            raise MessageParseError(message="Unable to parse Ethernet header (14bytes)",
+                                    original_error=exception) from exception
         data = packed_message[ETHERNET_HEADER_LENGTH:]
         return cls(MacAddress(dst_mac), MacAddress(src_mac), ethertype, data)
 
diff --git a/chewie/message_parser.py b/chewie/message_parser.py
index ea02373..08a73a8 100644
--- a/chewie/message_parser.py
+++ b/chewie/message_parser.py
@@ -8,6 +8,7 @@ from chewie.ethernet_packet import EthernetPacket
 from chewie.auth_8021x import Auth8021x
 from chewie.eap import Eap, EapIdentity, EapMd5Challenge, EapSuccess, EapFailure, EapLegacyNak, \
     EapTTLS, EapTLS, PARSERS_TYPES
+from chewie.utils import MessageParseError
 
 
 class EapMessage:
@@ -124,7 +125,12 @@ AUTH_8021X_MESSAGES = {
 class MessageParser:
     @staticmethod
     def one_x_parse(data, src_mac):
-        """Parses the 1x header (version and packet type) part of the packet, and the payload."""
+        """Parses the 1x header (version and packet type) part of the packet, and the payload.
+        Args:
+            data:
+            src_mac (MacAddress): source mac address of the data packet.
+        Raises:
+            MessageParseError: the data cannot be parsed."""
         auth_8021x = Auth8021x.parse(data)
         if auth_8021x.packet_type == 0:
             return MessageParser.eap_parse(auth_8021x.data, src_mac)
@@ -132,11 +138,16 @@ class MessageParser:
             return EapolStartMessage.build(src_mac)
         elif auth_8021x.packet_type == 2:
             return EapolLogoffMessage.build(src_mac)
-        raise ValueError("802.1x has bad type, expected 0: %s" % auth_8021x)
+        raise MessageParseError("802.1x has bad type, expected 0: %s" % auth_8021x)
 
     @staticmethod
     def eap_parse(data, src_mac):
-        """Parses the actual EAP payload"""
+        """Parses the actual EAP payload
+        Args:
+            data:
+            src_mac (MacAddress): source mac address of the data packet
+        Raises:
+            MessageParseError: the data cannot be parsed."""
         eap = Eap.parse(data)
         if isinstance(eap, tuple(PARSERS_TYPES.values())):
             return EAP_MESSAGES[eap.PACKET_TYPE].build(src_mac, eap)
@@ -145,21 +156,32 @@ class MessageParser:
         elif isinstance(eap, EapFailure):
             return FailureMessage.build(src_mac, eap)
         else:
-            raise ValueError("Got bad Eap packet: %s" % eap)
+            raise MessageParseError("Got bad Eap packet: %s" % eap)
 
     @staticmethod
     def ethernet_parse(packed_message):
-        """Parses the ethernet header part, and payload"""
+        """Parses the ethernet header part, and payload
+        Args:
+            packed_message:
+        Returns:
+            ***Message & destination mac address.
+        Raises:
+            MessageParseError: the packed_message cannot be parsed."""
         ethernet_packet = EthernetPacket.parse(packed_message)
         if ethernet_packet.ethertype != 0x888e:
-            raise ValueError("Ethernet packet with bad ethertype received: %s" % ethernet_packet)
+            raise MessageParseError("Ethernet packet with bad ethertype received: %s" %
+                                    ethernet_packet)
 
         return MessageParser.one_x_parse(ethernet_packet.data, ethernet_packet.src_mac), \
-               ethernet_packet.dst_mac
+            ethernet_packet.dst_mac
 
     @staticmethod
     def radius_parse(packed_message, secret, radius_lifecycle):
-        """Parses a RADIUS packet"""
+        """Parses a RADIUS packet
+        Returns:
+            RadiusPacket
+        Raises:
+            MessageParseError: the packed_message cannot be parsed"""
         parsed_radius = Radius.parse(packed_message, secret,
                                      radius_lifecycle=radius_lifecycle)
         return parsed_radius
@@ -207,7 +229,7 @@ class MessagePacker:
         attr_list.append(CallingStationId.create(str(src_mac)))
 
         if nas_port:
-            attr_list.append(NASPort.create( nas_port))
+            attr_list.append(NASPort.create(nas_port))
 
         attr_list.extend(extra_attributes)
 
diff --git a/chewie/radius.py b/chewie/radius.py
index 4f1ba1d..31370e5 100644
--- a/chewie/radius.py
+++ b/chewie/radius.py
@@ -7,7 +7,7 @@ import struct
 
 from chewie.radius_attributes import ATTRIBUTE_TYPES, Attribute, MessageAuthenticator
 from chewie.radius_datatypes import Concat
-
+from chewie.utils import MessageParseError
 
 RADIUS_HEADER_LENGTH = 1 + 1 + 2 + 16
 
@@ -48,18 +48,34 @@ class Radius:
         Returns:
             RadiusPacket - RadiusAccessChallenge/RadiusAccessRequest/
                             RadiusAccessAccept/RadiusAccessFailure
+        Raises:
+            MessageParseError: if packed_message cannot be parsed
         """
-        code, packet_id, length, response_authenticator = struct.unpack("!BBH16s",
-                                                                        packed_message[:RADIUS_HEADER_LENGTH])
+        try:
+            code, packet_id, length, response_authenticator = struct.unpack("!BBH16s",
+                                                                            packed_message[:RADIUS_HEADER_LENGTH])
+        except struct.error as exception:
+            raise MessageParseError('Unable to unpack first 20 bytes of RADIUS header') \
+                from exception
+
         response_authenticator = binascii.hexlify(response_authenticator)
         if code in PACKET_TYPE_PARSERS.keys():
             radius_packet = PACKET_TYPE_PARSERS[code](packet_id, response_authenticator,
                                                       RadiusAttributesList.parse(
                                                           packed_message[RADIUS_HEADER_LENGTH:]))
-            request_authenticator = radius_lifecycle.packet_id_to_request_authenticator[packet_id]
-            return radius_packet.validate_packet(secret,
-                                                 request_authenticator=request_authenticator)
-        raise ValueError("Unable to parse radius code: %d" % code)
+            try:
+                request_authenticator = radius_lifecycle.packet_id_to_request_authenticator[packet_id]
+            except KeyError as exception:
+                raise MessageParseError('Unknown RAIDUS packet_id: %s' % packet_id,) \
+                    from exception
+            try:
+                return radius_packet.validate_packet(secret,
+                                                     request_authenticator=request_authenticator)
+            except (InvalidMessageAuthenticatorError,
+                    InvalidResponseAuthenticatorError) as exception:
+                raise MessageParseError("Unable to parse Radius packet") \
+                    from exception
+        raise MessageParseError("Unable to parse radius code: %d" % code)
 
     def pack(self):
         pass
@@ -143,7 +159,7 @@ class RadiusPacket(Radius):
         radius_packet.authenticator = request_authenticator
         radius_packet.pack()
         calculated_response_authenticator = binascii.hexlify(hashlib.md5(radius_packet.packed +
-                                                        bytearray(secret, 'utf-8')).digest())
+                                                             bytearray(secret, 'utf-8')).digest())
         if calculated_response_authenticator != response_authenticator:
             raise InvalidResponseAuthenticatorError(
                 "Original ResponseAuthenticator: '%s', does not match calculated: '%s' %s" % (
@@ -195,6 +211,16 @@ class RadiusAttributesList:
 
     @classmethod
     def parse(cls, attributes_data):
+        """
+
+        Args:
+            attributes_data:
+
+        Returns:
+            RadiusAttributeList
+        Raises:
+            MessageParseError: if unable to parse an attribute's data.
+        """
         attributes = []
         attributes_to_concat = {}
         cls.extract_attributes(attributes_data, attributes, attributes_to_concat)
@@ -213,6 +239,9 @@ class RadiusAttributesList:
             attributes_to_concat (dict): attribute - position.
         Returns:
             attributes (list)
+        Raises:
+            MessageParseError: RadiusAttribute.parse will raise error
+            if it cannot parse the attribute's data
         """
         # Join Attributes that's datatype is Concat into one attribute.
         concatenated_attributes = []
@@ -241,20 +270,30 @@ class RadiusAttributesList:
             attributes_data (): data to extract from (input).
             attributes: attributes extracted (output variable).
             attributes_to_concat (dict): (output variable).
+        Raises:
+            MessageParseError: RadiusAttribute.parse will raise error
+            if it cannot parse the attribute's data
         """
         total_length = len(attributes_data)
         pos = 0
         index = -1
         last_attribute = -1
         while pos < total_length:
-            type_, attr_length = struct.unpack("!BB",
-                                               attributes_data[pos:pos + Attribute.HEADER_SIZE])
+            try:
+                type_, attr_length = struct.unpack("!BB",
+                                                   attributes_data[pos:pos + Attribute.HEADER_SIZE])
+            except struct.error as exception:
+                raise MessageParseError('Unable to unpack first 2 bytes of attribute header')\
+                    from exception
             data = attributes_data[pos + Attribute.HEADER_SIZE: pos + attr_length]
             pos += attr_length
 
             packed_value = data[:attr_length - Attribute.HEADER_SIZE]
-
-            attribute = ATTRIBUTE_TYPES[type_].parse(packed_value)
+            try:
+                attribute = ATTRIBUTE_TYPES[type_].parse(packed_value)
+            except KeyError as exception:
+                raise MessageParseError('Cannot find parser for RADIUS attribute %s' %
+                                        type_) from exception
             # keep track of where the concated AVP should be in the attributes list.
             # required so the hashing gives correct hash.
             if attribute.DATA_TYPE != Concat or last_attribute != attribute.TYPE:
diff --git a/chewie/radius_attributes.py b/chewie/radius_attributes.py
index 63e420a..c5658f7 100644
--- a/chewie/radius_attributes.py
+++ b/chewie/radius_attributes.py
@@ -39,6 +39,8 @@ class Attribute():
             packed_value (bytes): pre-packed value
         Returns:
             Attribute subclass.
+        Raises:
+            MessageParseError: if unable to parse the packed_value into the appropriate datatype.
         """
         return cls(cls.DATA_TYPE.parse(packed_value))
 
diff --git a/chewie/radius_datatypes.py b/chewie/radius_datatypes.py
index 6bf2134..ea0bd07 100644
--- a/chewie/radius_datatypes.py
+++ b/chewie/radius_datatypes.py
@@ -3,6 +3,7 @@ import math
 import struct
 
 import chewie.message_parser
+from chewie.utils import MessageParseError
 
 
 class DataType():
@@ -68,14 +69,19 @@ class Integer(DataType):
         if raw_data:
             try:
                 bytes_data = raw_data.to_bytes(self.MAX_DATA_LENGTH, "big")
-            except OverflowError:
-                raise ValueError("Integer must be >= 0  and <= 2^32-1, was %d" % raw_data)
+            except OverflowError as exception:
+                raise ValueError("Integer must be >= 0  and <= 2^32-1, was %d" %
+                                                raw_data)
         self.bytes_data = bytes_data
 
     @classmethod
     def parse(cls, packed_value):
-        cls.is_valid_length(packed_value)
-        return cls(bytes_data=struct.unpack("!4s", packed_value)[0])
+
+        try:
+            cls.is_valid_length(packed_value)
+            return cls(bytes_data=struct.unpack("!4s", packed_value)[0])
+        except (ValueError, struct.error)as exception:
+            raise MessageParseError("%s unable to unpack." % cls.__name__) from exception
 
     def pack(self, attribute_type):
         return struct.pack("!4s", self.bytes_data)
@@ -102,8 +108,11 @@ class Enum(DataType):
 
     @classmethod
     def parse(cls, packed_value):
-        cls.is_valid_length(packed_value)
-        return cls(bytes_data=struct.unpack("!4s", packed_value)[0])
+        try:
+            cls.is_valid_length(packed_value)
+            return cls(bytes_data=struct.unpack("!4s", packed_value)[0])
+        except (ValueError, struct.error)as exception:
+            raise MessageParseError("%s unable to unpack." % cls.__name__) from exception
 
     def pack(self, attribute_type):
         return struct.pack("!4s", self.bytes_data)
@@ -126,8 +135,11 @@ class Text(DataType):
 
     @classmethod
     def parse(cls, packed_value):
-        cls.is_valid_length(packed_value)
-        return cls(struct.unpack("!%ds" % len(packed_value), packed_value)[0])
+        try:
+            cls.is_valid_length(packed_value)
+            return cls(struct.unpack("!%ds" % len(packed_value), packed_value)[0])
+        except (ValueError, struct.error) as exception:
+            raise MessageParseError("%s unable to unpack." % cls.__name__) from exception
 
     def pack(self, attribute_type):
         return struct.pack("!%ds" % len(self.bytes_data), self.bytes_data)
@@ -154,8 +166,11 @@ class String(DataType):
 
     @classmethod
     def parse(cls, packed_value):
-        cls.is_valid_length(packed_value)
-        return cls(struct.unpack("!%ds" % len(packed_value), packed_value)[0])
+        try:
+            cls.is_valid_length(packed_value)
+            return cls(struct.unpack("!%ds" % len(packed_value), packed_value)[0])
+        except (ValueError, struct.error) as exception:
+            raise MessageParseError("%s unable to unpack." % cls.__name__) from exception
 
     def pack(self, attribute_type):
         return struct.pack("!%ds" % len(self.bytes_data), self.bytes_data)
@@ -186,7 +201,10 @@ class Concat(DataType):
         # Packing is (generally) for packets going to the radius server.
         #
         # Therefore we error out if length is too long (you are not allowed to have AVP that are too long)
-        return cls(struct.unpack("!%ds" % len(packed_value), packed_value)[0])
+        try:
+            return cls(struct.unpack("!%ds" % len(packed_value), packed_value)[0])
+        except struct.error as exception:
+            raise MessageParseError("%s unable to unpack." % cls.__name__) from exception
 
     def pack(self, attribute_type):
 
@@ -235,10 +253,13 @@ class Vsa(DataType):
 
     @classmethod
     def parse(cls, packed_value):
-        cls.is_valid_length(packed_value)
         # TODO Vsa.parse does not currently separate the vendor-id from the vsa-data
         # we could do that at some point (e.g. if we wanted to use Vendor-Specific)
-        return cls(struct.unpack("!%ds" % len(packed_value), packed_value)[0])
+        try:
+            cls.is_valid_length(packed_value)
+            return cls(struct.unpack("!%ds" % len(packed_value), packed_value)[0])
+        except (ValueError, struct.error) as exception:
+            raise MessageParseError("%s unable to unpack." % cls.__name__) from exception
 
     def pack(self, attribute_type):
         return struct.pack("!%ds" % (self.data_length()), self.bytes_data)
diff --git a/chewie/utils.py b/chewie/utils.py
index f65bfba..5704bf5 100644
--- a/chewie/utils.py
+++ b/chewie/utils.py
@@ -15,3 +15,8 @@ def log_method(method):
         self.logger.info('Entering %s' % method.__name__)
         return method(self, *args, **kwargs)
     return wrapped
+
+
+class MessageParseError(Exception):
+    """Error for when parsing cannot be successfully completed."""
+    pass


Test patch:
diff --git a/test/test_message_parser.py b/test/test_message_parser.py
index 938925a..d8705c5 100644
--- a/test/test_message_parser.py
+++ b/test/test_message_parser.py
@@ -1,18 +1,20 @@
 
 # pylint: disable=missing-docstring
-
+import struct
 import unittest
 from chewie.message_parser import MessageParser, MessagePacker, IdentityMessage, \
     Md5ChallengeMessage, TtlsMessage, \
     LegacyNakMessage, TlsMessage
-from chewie.message_parser import EapolStartMessage, EapolLogoffMessage, SuccessMessage, FailureMessage
+from chewie.message_parser import EapolStartMessage, EapolLogoffMessage,\
+    SuccessMessage, FailureMessage
 from chewie.mac_address import MacAddress
 from chewie.eap import Eap
 from chewie.radius_attributes import State, CalledStationId, NASPortType
+from chewie.utils import MessageParseError
 
 
 class MessageParserTestCase(unittest.TestCase):
-    def test_identity_request_message_parses(self):
+    def test_identity_request_message_parses(self):  # pylint: disable=invalid-name
         packed_message = bytes.fromhex("0180c2000003001906eab88c888e010000050101000501000000")
         message = MessageParser.ethernet_parse(packed_message)[0]
         self.assertEqual(MacAddress.from_string("00:19:06:ea:b8:8c"), message.src_mac)
@@ -20,7 +22,7 @@ class MessageParserTestCase(unittest.TestCase):
         self.assertEqual(Eap.REQUEST, message.code)
         self.assertEqual("", message.identity)
 
-    def test_identity_response_message_parses(self):
+    def test_identity_response_message_parses(self):  # pylint: disable=invalid-name
         packed_message = bytes.fromhex(
             "0180c2000003001422e9545e888e0100001102000011014a6f686e2e4d63477569726b")
         message = MessageParser.ethernet_parse(packed_message)[0]
@@ -29,7 +31,7 @@ class MessageParserTestCase(unittest.TestCase):
         self.assertEqual(Eap.RESPONSE, message.code)
         self.assertEqual("John.McGuirk", message.identity)
 
-    def test_md5_challenge_request_message_parses(self):
+    def test_md5_challenge_request_message_parses(self):  # pylint: disable=invalid-name
         packed_message = bytes.fromhex(
             "0180c2000003001906eab88c888e01000016010100160410824788d693e2adac6ce15641418228cf0000")
         message = MessageParser.ethernet_parse(packed_message)[0]
@@ -39,7 +41,7 @@ class MessageParserTestCase(unittest.TestCase):
         self.assertEqual(bytes.fromhex("824788d693e2adac6ce15641418228cf"), message.challenge)
         self.assertEqual(b"", message.extra_data)
 
-    def test_md5_challenge_response_message_parses(self):
+    def test_md5_challenge_response_message_parses(self):  # pylint: disable=invalid-name
         packed_message = bytes.fromhex(
             "0180c2000003001422e9545e888e010000220201002204103a535f0ee8c6b34fe714aa7dad9a0e154a6f686e2e4d63477569726b")  # pylint: disable=line-too-long
         message = MessageParser.ethernet_parse(packed_message)[0]
@@ -49,7 +51,7 @@ class MessageParserTestCase(unittest.TestCase):
         self.assertEqual(bytes.fromhex("3a535f0ee8c6b34fe714aa7dad9a0e15"), message.challenge)
         self.assertEqual(b"John.McGuirk", message.extra_data)
 
-    def test_identity_request_message_packs(self):
+    def test_identity_request_message_packs(self):  # pylint: disable=invalid-name
         expected_packed_message = bytes.fromhex(
             "0180c2000003001906eab88c888e010000050101000501")
         message = IdentityMessage(src_mac=MacAddress.from_string("00:19:06:ea:b8:8c"), message_id=1,
@@ -59,7 +61,7 @@ class MessageParserTestCase(unittest.TestCase):
                                                      MacAddress.from_string("01:80:c2:00:00:03"))
         self.assertEqual(expected_packed_message, packed_message)
 
-    def test_identity_response_message_packs(self):
+    def test_identity_response_message_packs(self):  # pylint: disable=invalid-name
         expected_packed_message = bytes.fromhex(
             "0180c2000003001422e9545e888e0100001102000011014a6f686e2e4d63477569726b")
         message = IdentityMessage(src_mac=MacAddress.from_string("00:14:22:e9:54:5e"),
@@ -69,7 +71,7 @@ class MessageParserTestCase(unittest.TestCase):
                                                      MacAddress.from_string("01:80:c2:00:00:03"))
         self.assertEqual(expected_packed_message, packed_message)
 
-    def test_md5_challenge_request_message_packs(self):
+    def test_md5_challenge_request_message_packs(self):  # pylint: disable=invalid-name
         expected_packed_message = bytes.fromhex(
             "0180c2000003001906eab88c888e01000016010100160410824788d693e2adac6ce15641418228cf")
         message = Md5ChallengeMessage(src_mac=MacAddress.from_string("00:19:06:ea:b8:8c"),
@@ -83,7 +85,7 @@ class MessageParserTestCase(unittest.TestCase):
                                                      MacAddress.from_string("01:80:c2:00:00:03"))
         self.assertEqual(expected_packed_message, packed_message)
 
-    def test_md5_challenge_response_message_packs(self):
+    def test_md5_challenge_response_message_packs(self):  # pylint: disable=invalid-name
         expected_packed_message = bytes.fromhex(
             "0180c2000003001422e9545e888e010000220201002204103a535f0ee8c6b34fe714aa7dad9a0e154a6f686e2e4d63477569726b")  # pylint: disable=line-too-long
         message = Md5ChallengeMessage(src_mac=MacAddress.from_string("00:14:22:e9:54:5e"),
@@ -111,7 +113,7 @@ class MessageParserTestCase(unittest.TestCase):
                                                      MacAddress.from_string("01:80:c2:00:00:03"))
         self.assertEqual(expected_packed_message, packed_message)
 
-    def test_eapol_logoff_message_parses(self):
+    def test_eapol_logoff_message_parses(self):  # pylint: disable=invalid-name
         packed_message = bytes.fromhex("0180c2000003001906eab88c888e01020000")
         message = MessageParser.ethernet_parse(packed_message)[0]
         self.assertEqual(MacAddress.from_string("00:19:06:ea:b8:8c"), message.src_mac)
@@ -178,7 +180,7 @@ class MessageParserTestCase(unittest.TestCase):
 
     def test_tls_message_parses(self):
         packed_message = bytes.fromhex("000000111101444444444444888e"
-                                       "010000b2026900b20d0016030100a7010000a303038c8007fa4ffe8f11fbe62debce4a1385e70be51efe77b105d205d2dc9ae815a5000038c02cc030009fcca9cca8ccaac02bc02f009ec024c028006bc023c0270067c00ac0140039c009c0130033009d009c003d003c0035002f00ff01000042000b000403000102000a000a0008001d0017001900180016000000170000000d0020001e060106020603050105020503040104020403030103020303020102020203")
+                                       "010000b2026900b20d0016030100a7010000a303038c8007fa4ffe8f11fbe62debce4a1385e70be51efe77b105d205d2dc9ae815a5000038c02cc030009fcca9cca8ccaac02bc02f009ec024c028006bc023c0270067c00ac0140039c009c0130033009d009c003d003c0035002f00ff01000042000b000403000102000a000a0008001d0017001900180016000000170000000d0020001e060106020603050105020503040104020403030103020303020102020203")  # pylint: disable=line-too-long
         message = MessageParser.ethernet_parse(packed_message)[0]
         self.assertEqual(MacAddress.from_string("44:44:44:44:44:44"), message.src_mac)
         self.assertEqual(105, message.message_id)
@@ -187,10 +189,10 @@ class MessageParserTestCase(unittest.TestCase):
 
     def test_tls_message_packs(self):
         expected_packed_message = bytes.fromhex("000000111101444444444444888e"
-                                                "010000b2026900b20d0016030100a7010000a303038c8007fa4ffe8f11fbe62debce4a1385e70be51efe77b105d205d2dc9ae815a5000038c02cc030009fcca9cca8ccaac02bc02f009ec024c028006bc023c0270067c00ac0140039c009c0130033009d009c003d003c0035002f00ff01000042000b000403000102000a000a0008001d0017001900180016000000170000000d0020001e060106020603050105020503040104020403030103020303020102020203")
+                                                "010000b2026900b20d0016030100a7010000a303038c8007fa4ffe8f11fbe62debce4a1385e70be51efe77b105d205d2dc9ae815a5000038c02cc030009fcca9cca8ccaac02bc02f009ec024c028006bc023c0270067c00ac0140039c009c0130033009d009c003d003c0035002f00ff01000042000b000403000102000a000a0008001d0017001900180016000000170000000d0020001e060106020603050105020503040104020403030103020303020102020203")  # pylint: disable=line-too-long
         message = TlsMessage(src_mac=MacAddress.from_string("44:44:44:44:44:44"),
-                              message_id=105, code=Eap.RESPONSE,
-                              flags=0x00, extra_data=bytes.fromhex('16030100a7010000a303038c8007fa4ffe8f11fbe62debce4a1385e70be51efe77b105d205d2dc9ae815a5000038c02cc030009fcca9cca8ccaac02bc02f009ec024c028006bc023c0270067c00ac0140039c009c0130033009d009c003d003c0035002f00ff01000042000b000403000102000a000a0008001d0017001900180016000000170000000d0020001e060106020603050105020503040104020403030103020303020102020203'))
+                             message_id=105, code=Eap.RESPONSE,
+                             flags=0x00, extra_data=bytes.fromhex('16030100a7010000a303038c8007fa4ffe8f11fbe62debce4a1385e70be51efe77b105d205d2dc9ae815a5000038c02cc030009fcca9cca8ccaac02bc02f009ec024c028006bc023c0270067c00ac0140039c009c0130033009d009c003d003c0035002f00ff01000042000b000403000102000a000a0008001d0017001900180016000000170000000d0020001e060106020603050105020503040104020403030103020303020102020203'))  # pylint: disable=line-too-long
         packed_message = MessagePacker.ethernet_pack(message,
                                                      MacAddress.from_string("44:44:44:44:44:44"),
                                                      MacAddress.from_string("00:00:00:11:11:01"))
@@ -216,7 +218,7 @@ class MessageParserTestCase(unittest.TestCase):
                                                      MacAddress.from_string("01:80:c2:00:00:03"))
         self.assertEqual(expected_packed_message, packed_message)
 
-    def test_radius_with_extra_attributes_packs(self):
+    def test_radius_with_extra_attributes_packs(self):  # pylint: disable=invalid-name
 
         packed_message = bytes.fromhex("010a0073"
                                        "be5df1f3b3366c69b977e56a7da47cba"
@@ -278,3 +280,33 @@ class MessageParserTestCase(unittest.TestCase):
         packed_radius = MessagePacker.radius_pack(eap_message, src_mac, username, radius_packet_id,
                                                   request_authenticator, state, secret)
         self.assertEqual(packed_message, packed_radius)
+
+    def test_unicode_decode_error_converts_to_message_parse_error(self):  # pylint: disable=invalid-name
+        data = bytes.fromhex("0163bf130103bf1301")
+        try:
+            MessageParser.eap_parse(data, MacAddress.from_string("00:00:00:12:34:56"))
+        except MessageParseError as exception:
+            self.assertIsInstance(exception.__cause__, UnicodeDecodeError)
+            return
+        self.fail()
+
+    def test_struct_unpack_error_converts_to_message_parse_error(self):  # pylint: disable=invalid-name
+        data = bytes.fromhex("01001000")
+        try:
+            MessageParser.eap_parse(data, MacAddress.from_string("00:00:00:12:34:56"))
+        except MessageParseError as exception:
+            self.assertIsInstance(exception.__cause__, struct.error)
+            return
+        self.fail()
+
+    def test_bad_eap_code(self):
+        data = bytes.fromhex("001F1101")
+        self.assertRaises(MessageParseError,
+                          MessageParser.eap_parse,
+                          data, MacAddress.from_string("00:00:00:12:34:56"))
+
+    def test_bad_packet_type(self):
+        data = bytes.fromhex("02a02a02070207")
+        self.assertRaises(MessageParseError,
+                          MessageParser.eap_parse,
+                          data, MacAddress.from_string("00:00:00:12:34:56"))
diff --git a/test/test_radius.py b/test/test_radius.py
index a39017a..73b5fdb 100644
--- a/test/test_radius.py
+++ b/test/test_radius.py
@@ -13,6 +13,7 @@ from chewie.radius_attributes import UserName, ServiceType, FramedMTU, CalledSta
     AcctSessionId, NASPortType, ConnectInfo, EAPMessage, MessageAuthenticator, State,\
     VendorSpecific, CallingStationId
 from chewie.radius_datatypes import Vsa, String, Enum, Text, Integer, Concat
+from chewie.utils import MessageParseError
 
 
 class RadiusTestCase(unittest.TestCase):
@@ -89,22 +90,29 @@ class RadiusTestCase(unittest.TestCase):
         packed_message = bytes.fromhex(
             "0b00005056d9280d3e4fed327eb31cf1823f8c244f1801020016041074d3db089b727d9cc5774599e4a32a295012982a0ba06d3557f0dbc8ba6e823822f1181219ddf6d119dff272fa26666666666666")
 
-        self.assertRaises(InvalidResponseAuthenticatorError, Radius.parse,
-                          packed_message, secret="SECRET",
+        try:
+            Radius.parse(packed_message, secret="SECRET",
                           radius_lifecycle=namedtuple('RadiusLifecycle', 'packet_id_to_request_authenticator')({
                             0: bytes.fromhex("982a0ba06d3557f0dbc8ba6e823822f1")
                           }))
+            self.fail()
+        except MessageParseError as exception:
+            self.assertIsInstance(exception.__cause__, InvalidResponseAuthenticatorError)
 
         # the original response authenticator does not match the computed one
         #  because the message authenticator was 'corrupted'
         packed_message = bytes.fromhex(
             "0b00005056d9280d3e4fed327eb31cf1823f8c244f1801020016041074d3db089b727d9cc5774599e4a32a29501266666666666666666666666666666666181219ddf6d119dff272fa2fe16c34990c7d")
 
-        self.assertRaises(InvalidResponseAuthenticatorError, Radius.parse, packed_message,
+        try:
+            Radius.parse(packed_message,
                           secret="SECRET",
                           radius_lifecycle=namedtuple('RadiusLifecycle', 'packet_id_to_request_authenticator')({
                             0: bytes.fromhex("982a0ba06d3557f0dbc8ba6e823822f1")
                           }))
+            self.fail()
+        except MessageParseError as exception:
+            self.assertIsInstance(exception.__cause__, InvalidResponseAuthenticatorError)
 
         # TODO How can we test that response authenticator is correct
         #  but message authenticator is not?
@@ -239,30 +247,30 @@ class RadiusTestCase(unittest.TestCase):
 
     def test_parse_illegal_radius_datatype_lengths(self):
 
-        self.assertRaises(ValueError, Integer.parse, (1500).to_bytes(3, byteorder='big'))
-        self.assertRaises(ValueError, Integer.parse, bytes.fromhex("01234567890123456789"))
+        self.assertRaises(MessageParseError, Integer.parse, (1500).to_bytes(3, byteorder='big'))
+        self.assertRaises(MessageParseError, Integer.parse, bytes.fromhex("01234567890123456789"))
 
-        self.assertRaises(ValueError, Enum.parse, bytes.fromhex("000002"))
-        self.assertRaises(ValueError, Enum.parse, bytes.fromhex("01234567890123456789"))
+        self.assertRaises(MessageParseError, Enum.parse, bytes.fromhex("000002"))
+        self.assertRaises(MessageParseError, Enum.parse, bytes.fromhex("01234567890123456789"))
 
-        self.assertRaises(ValueError, Text.parse, "".encode())
-        self.assertRaises(ValueError, Text.parse,
+        self.assertRaises(MessageParseError, Text.parse, "".encode())
+        self.assertRaises(MessageParseError, Text.parse,
                           "260CharacterLengthUserName260CharacterLengthUserName"
                           "260CharacterLengthUserName260CharacterLengthUserName"
                           "260CharacterLengthUserName260CharacterLengthUserName"
                           "260CharacterLengthUserName260CharacterLengthUserName"
                           "260CharacterLengthUserName260CharacterLengthUserName".encode())
 
-        self.assertRaises(ValueError, String.parse, "".encode())
-        self.assertRaises(ValueError, String.parse, "260CharacterLengthState260CharacterLengthState"
+        self.assertRaises(MessageParseError, String.parse, "".encode())
+        self.assertRaises(MessageParseError, String.parse, "260CharacterLengthState260CharacterLengthState"
                                                     "260CharacterLengthState260CharacterLengthState"
                                                     "260CharacterLengthState260CharacterLengthState"
                                                     "260CharacterLengthState260CharacterLengthState"
                                                     "260CharacterLengthState260CharacterLengthState"
                                                     "260CharacterLengthState260Char".encode())
 
-        self.assertRaises(ValueError, Vsa.parse, "abcd".encode())
-        self.assertRaises(ValueError, Vsa.parse,
+        self.assertRaises(MessageParseError, Vsa.parse, "abcd".encode())
+        self.assertRaises(MessageParseError, Vsa.parse,
                           "270CharacterLengthVSAAttribute270CharacterLengthVSAAttribute"
                           "270CharacterLengthVSAAttribute270CharacterLengthVSAAttribute"
                           "270CharacterLengthVSAAttribute270CharacterLengthVSAAttribute"