Repository: rthalley/dnspython

Issue:
TSIG RR cannot be read without validating it
**Motivation**
I'm processing raw DNS messages from files using `dns.message.from_wire()` and inspecting them for various properties. Turns out that messages signed with TSIG cannot be parsed and manipulated without providing valid keyring. Any such attempt raises exception:
```
  File "/home/pspacek/w/pkg/python-dns/wire_read_udp.py", line 16, in main
    msg = dns.message.from_wire(msg_wire, ignore_trailing=True)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/dns/message.py", line 1367, in from_wire
    m = reader.read()
        ^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/dns/message.py", line 1264, in read
    self._get_section(MessageSection.ADDITIONAL, adcount)
  File "/usr/lib/python3.12/site-packages/dns/message.py", line 1196, in _get_section
    raise UnknownTSIGKey("got signed message without keyring")
dns.message.UnknownTSIGKey: got signed message without keyring
```

**Describe the solution you'd like.**
I think `message.from_wire()` could accept a special `keyring` value which would ignore TSIG validation and just return the parsed Message object. Maybe `False` could be a good choice, but I don't care much.

Discussion:
@rthalley: Have you tried setting `continue_on_error` to `True` in `dns.message.from_wire()`?@pspacek: You are totally right, it works! :facepalm: I don't know how I missed that. Sorry for the noise!@pspacek: Aha! I retract my retraction :trollface: 

`from_wire(continue_on_error=True)` has significant limitation - it does not show the TSIG RR in the parsed message.

```
id 12336
opcode UPDATE
rcode NOERROR
flags 
;ZONE
example.nil. IN SOA
;PREREQ
example.nil. NONE TYPE0
;UPDATE
;ADDITIONAL
```

msg.errors contains object which says:
Error at offset 123: got signed message without keyring

What I would like to see is parsed message with TSIG RR which I can inspect myself without validating it.@rthalley: Fair enough, I like your suggestion about a keychain of `False` meaning "do not validate the TSIG if present"@rthalley: Also, dnspython does not currently print TSIGs in `to_text()` regardless, but with the proposed change the message's `tsig`
field will be a TSIG rrset and you could print it yourself if you wanted to.  I'm a little hesitant to change `to_text()` lest it break something not expecting a TSIG at the end.@pspacek: Thank you. I need to experiment with the code a bit. Two things which prevent me from acking it right away:
- what if the message has more than one TSIG
- to_text() does not seem like an interface where adding a new RR (or anything, really) should be surprising - but I'm just ruminating out loud@rthalley: More than one TSIG is forbidden by the RFC; dnspython will detect it and complain.  If the goal is printing really odd messages, then the "annotate" concept that lived on a branch for a while but wasn't merged is better.  I agree with you re to_text() in theory.@pspacek: Well if I got penny for every RFC violation I have seen I would be retired by now :-) But this is again the eternal question if dnspython should be high level library will safety belts etc. or allow low-level manipulation. As usual I don't have the answer.@rthalley: Dnspython has always been both things :)   The thing I struggle with is what to put in and what to leave out.@rthalley: I merged it but if you need more we can consider more changes.

Solution patch:
diff --git a/dns/message.py b/dns/message.py
index f322895..f320f82 100644
--- a/dns/message.py
+++ b/dns/message.py
@@ -1202,9 +1202,9 @@ class _WireReader:
                 if rdtype == dns.rdatatype.OPT:
                     self.message.opt = dns.rrset.from_rdata(name, ttl, rd)
                 elif rdtype == dns.rdatatype.TSIG:
-                    if self.keyring is None:
+                    if self.keyring is None or self.keyring is True:
                         raise UnknownTSIGKey("got signed message without keyring")
-                    if isinstance(self.keyring, dict):
+                    elif isinstance(self.keyring, dict):
                         key = self.keyring.get(absolute_name)
                         if isinstance(key, bytes):
                             key = dns.tsig.Key(absolute_name, key, rd.algorithm)
@@ -1214,18 +1214,19 @@ class _WireReader:
                         key = self.keyring
                     if key is None:
                         raise UnknownTSIGKey(f"key '{name}' unknown")
-                    self.message.keyring = key
-                    self.message.tsig_ctx = dns.tsig.validate(
-                        self.parser.wire,
-                        key,
-                        absolute_name,
-                        rd,
-                        int(time.time()),
-                        self.message.request_mac,
-                        rr_start,
-                        self.message.tsig_ctx,
-                        self.multi,
-                    )
+                    if key:
+                        self.message.keyring = key
+                        self.message.tsig_ctx = dns.tsig.validate(
+                            self.parser.wire,
+                            key,
+                            absolute_name,
+                            rd,
+                            int(time.time()),
+                            self.message.request_mac,
+                            rr_start,
+                            self.message.tsig_ctx,
+                            self.multi,
+                        )
                     self.message.tsig = dns.rrset.from_rdata(absolute_name, 0, rd)
                 else:
                     rrset = self.message.find_rrset(
@@ -1301,8 +1302,10 @@ def from_wire(
 ) -> Message:
     """Convert a DNS wire format message into a message object.
 
-    *keyring*, a ``dns.tsig.Key`` or ``dict``, the key or keyring to use if the message
-    is signed.
+    *keyring*, a ``dns.tsig.Key``, ``dict``, ``bool``, or ``None``, the key or keyring
+    to use if the message is signed.  If ``None`` or ``True``, then trying to decode
+    a message with a TSIG will fail as it cannot be validated.  If ``False``, then
+    TSIG validation is disabled.
 
     *request_mac*, a ``bytes`` or ``None``.  If the message is a response to a
     TSIG-signed request, *request_mac* should be set to the MAC of that request.
diff --git a/dns/rdtypes/txtbase.py b/dns/rdtypes/txtbase.py
index 3e35b14..73db6d9 100644
--- a/dns/rdtypes/txtbase.py
+++ b/dns/rdtypes/txtbase.py
@@ -50,6 +50,8 @@ class TXTBase(dns.rdata.Rdata):
         self.strings: Tuple[bytes] = self._as_tuple(
             strings, lambda x: self._as_bytes(x, True, 255)
         )
+        if len(self.strings) == 0:
+            raise ValueError("the list of strings must not be empty")
 
     def to_text(
         self,


Test patch:
diff --git a/tests/test_rdata.py b/tests/test_rdata.py
index 38ff831..c63dc18 100644
--- a/tests/test_rdata.py
+++ b/tests/test_rdata.py
@@ -672,6 +672,10 @@ class RdataTestCase(unittest.TestCase):
         with self.assertRaises(dns.exception.SyntaxError):
             dns.rdata.from_text("in", "txt", "")
 
+    def test_empty_TXT_wire(self):
+        with self.assertRaises(dns.exception.FormError):
+            dns.rdata.from_wire(dns.rdataclass.IN, dns.rdatatype.TXT, b"", 0, 0)
+
     def test_too_long_TXT(self):
         # hit too long
         with self.assertRaises(dns.exception.SyntaxError):
diff --git a/tests/test_tsig.py b/tests/test_tsig.py
index 61e67df..5d7ceba 100644
--- a/tests/test_tsig.py
+++ b/tests/test_tsig.py
@@ -1,15 +1,15 @@
 # Copyright (C) Dnspython Contributors, see LICENSE for text of ISC license
 
+import base64
+import time
 import unittest
 from unittest.mock import Mock
-import time
-import base64
 
+import dns.message
 import dns.rcode
+import dns.rdtypes.ANY.TKEY
 import dns.tsig
 import dns.tsigkeyring
-import dns.message
-import dns.rdtypes.ANY.TKEY
 
 keyring = dns.tsigkeyring.from_text({"keyname.": "NjHwPsMKjdN++dOfE5iAiQ=="})
 
@@ -179,6 +179,28 @@ class TSIGTestCase(unittest.TestCase):
         # not raising is passing
         dns.message.from_wire(w, keyring)
 
+    def test_signature_is_invalid(self):
+        m = dns.message.make_query("example", "a")
+        m.use_tsig(keyring, keyname)
+        w = m.to_wire()
+        b = bytearray(w)
+        # corrupt hash
+        b[-7] = (b[-7] + 1) & 0xFF
+        w = bytes(b)
+        with self.assertRaises(dns.tsig.BadSignature):
+            dns.message.from_wire(w, keyring)
+
+    def test_signature_is_invalid_and_ignored(self):
+        m = dns.message.make_query("example", "a")
+        m.use_tsig(keyring, keyname)
+        w = m.to_wire()
+        b = bytearray(w)
+        # corrupt hash
+        b[-7] = (b[-7] + 1) & 0xFF
+        w = bytes(b)
+        m2 = dns.message.from_wire(w, False)
+        self.assertIsNotNone(m2.tsig)
+
     def test_validate_with_bad_keyring(self):
         m = dns.message.make_query("example", "a")
         m.use_tsig(keyring, keyname)