ErrTruncated should not be returned by Msg.Unpack() just because TC=1

[markdingo]

Msg.Unpack() returns ErrTruncated when the TC header bit is set.

This is misleading because TC=1 is not an unpack error - it's merely a protocol signal between sender and receiver. Furthermore the error return forces the caller to assume that the message is malformed and cannot be used when in fact the message is perfectly fine.

Since TC is visible via Msg.Truncated it strikes me that Unpack() has no need to return an error at all as callers can determined TC for themselves. But this is an incompatible change so a hack might be to return a unique error for TC=1. Maybe:

ErrTruncatedBitSet error = &Error{err: "Unpack worked, but found TC=1"}

so clients can distinguish at least. Alternatively use a new error for all the genuinely borken messages:

ErrTruncated error = &Error{err: "Unpack worked, but found TC=1"}
ErrReallyTruncated error = &Error{err: "failed to unpack truncated message"}

[andrewtj]

I believe this behaviour came about in #281

[markdingo]

Yes, it does seem related. But it seems to me that there is a conflation between TC=1 and whether a packet is in error or not.

There is no correlation between TC=1 and whether a message is well formed. TC=1 is a common response setting, eg, DNS RRL uses it extensively as does Bonjour.

I think it's a mistake to assume that a TC=1 message is an error.

Leaving that aside, a caller still has no way to distinguish between a genuinely truncated message where bits have been dropped on the wire and one which just happens to arrive with TC=1. It would be nice to have some way of distinguishing between the two regardless of how one interprets TC=1.

[miekg]

I think we can distinguish between the two cases. But what is the use? You want to inspect the TC reply for bits that are useful? What about a signature in a message and 1/2 RRset? There is no indication you have the full RRset because of TC=1.

Is there RFC text on being able to use TC=1 messages?

[markdingo]

My particular use-case is Bonjour. RFC6762 has numerous examples where TC is set because Bonjour data can get very large. Generally the sender sends multiple packets with TC=1 on allbut the last message related to the same set of services.

In terms of code, I assumed that an error return from Unpack() means that the message could not be unpacked properly. Perhaps because the payload is corrupted in transit or bits were dropped during transmission.

My original code simply went:

if err := msg.Unpack(buffer); err != nil {
fmt.Println("Message discarded", err)
return err
}
.... do something useful with msg
fmt.Println("Message Processed")
return nil

But that code doesn't work if the sender sets TC=1 and in fact there is no code I can write that lets me know that I have a well-formed message that just happens to have TC=1 set.

As a work-around I have a crude heuristic whereby if msg.Truncated is set and there is at least one RR and the error return is ErrTruncated then I assume that the message is ok. But that's imperfect logic.

The modified code looks like this:

if err := msg.Unpack(buffer); err != nil {
if err == dns.ErrTruncated && dnsMsg.Truncated &&
(len(dnsMsg.Question)+len(dnsMsg.Answer)+len(dnsMsg.Ns)+len(dnsMsg.Extra)) > 0 {
err = nil // Message is probably ok
} else {
fmt.Println("Message discarded", err)
return err
}

[miekg]

Yes, you'll need to check for ErrTruncated, like https://github.com/miekg/exdns/blob/master/q/q.go#L361

no code I can write that lets me know that I have a well-formed message that just happens to have TC=1 set.

Where is it specified that TC=1 can be wellformed?

[gibson042]

@miekg https://tools.ietf.org/html/rfc2181#section-9

Where TC is set, the partial RRSet that would not completely fit may
be left in the response.

Not only that, but any section preceding the truncation point is complete (e.g., seeing an authority section record means I definitely have the full answer section).

[miekg]

That section lacks any 2119 language and even says you should ignore the response. I'm failing to see what you are trying to do here. There are 0 guarantees on TC=1 packet. You'll need to redo the query with TCP.

…

On 2 Dec 2016 5:32 pm, "Richard Gibson" ***@***.***> wrote: @miekg <https://github.com/miekg> https://tools.ietf.org/html/ rfc2181#section-9 Where TC is set, the partial RRSet that would not completely fit may be left in the response. Not only that, but any section preceding the truncation point is complete (e.g., seeing an authority section record means I definitely have the full answer section). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#423 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVkWzQcbO546xZ_AcoeLCnxSAK3E5Wcks5rEFYzgaJpZM4LCB5V> .

[markdingo]

As mentioned earlier, this ticket relates to RFC6762's use of DNS messages. (RFC6762 is also know as mDNS, Multicast DNS and formerly as Bonjour and Rendezvous and zeroconf).

The first thing to note is that mDNS does not fallback to TCP - in fact it has no TCP or DNSSEC support at all.

On closer examination it does seem that RFC6762 diverges from RFC2181 regarding TC=1. That's a bummer.

Unfortunately for mDNS, RFC2181 makes it fairly clear that a client should discard TC=1 responses and re-query: "When a DNS client receives a reply with TC set, it should ignore that response, and query again". I will note that it's a "should", not a "SHOULD" or a "MUST", but whateves.

RFC6762 however takes a completely different view of TC=1. In that RFC all TC=1 means is that the sender has put as many RRs that fit in the current message as possible and that there are more RRs coming in a subsequent message (they call them "Additional Known Answers" messages). This is stated in Section 19 "uses the TC bit in a query to indicate additional Known Answers". TC=1 is also extensively discussed in the text.

mDNS messages are often very large so the use of TC=1 with a full message is routine.

(By way of background, for those not familiar with mDNS, it's purpose is to broadcast services on a LAN using multicast and unicast DNS messages on port 5353. While RFC6762 does state that it uses standard DNS messages, it does diverge a bit as can be seen in Section 19 "Summary of Differences between Multicast DNS and Unicast DNS".)

So what this ticket really becomes is a question: does miekg/dns want to be usable for mDNS/RFC6762 traffic? If that answer is "no" then this ticket can be closed with that acknowledgement and maybe a note in the docs to that effect.

If the answer is a "maybe" or "yes" then we need to think about ways to accomodate the differences such as TC=1 semantics. The most obvious way to my mind would be a flag associated with Msg which indicates mDNS behaviour. Something like:

m := dns.Msg{}
m.Profile(dns.MulticastDNS)
err := m.Unpack(payload)
....etc

FWIW, I've been using miekg/dns in a mDNS application and it works just fine so obviously I'd like a way to bridge the differences without having to fork off a mDNS variant or some other ugliness.

[miekg]

Very interesting comment. Thank you.

The divergence within the RFCs is, I think, OK, because it is a different protocol (running on a different port).

In this regard mDNS goes even beyond dyn. update messages. Forking go dns to get mDNS semantics seems silly as 99.9% would be identical.

m.Profile could work. If a server sends a message, correctly filing with what will fit might even work in the general case (although it will have some cost). I'm a bit afraid of long term maintainability with if-elsing different protocol in the lib. But it is not every day someone invents a DNS like protocol.

Note that dns.Server has a Unsafe boolean which IIRC also came from some mDNS shenanigans.

IOW: I'm not opposed. We just need to find the right interface. m.Profile looks promising, but is mDNS a property of the packet or of the client/server?

[andrewtj]

I think ErrTruncated is only returned if there's no body after unpacking the message header (which seems a bit arbitrary) or if TC=1 (unless there's trailing junk which should probably be changed). ErrShortRead seems to be returned otherwise. So I think you already have the means to differentiate the two cases.

On mDNS, I haven't read the RFC for a while but if I recall correctly mDNS redefines the message header semantics (but not the format), snips a bit from the TTL for purposes I don't recall, and allows compression in some RRDatas that unicast DNS doesn't allow. Notably SRV targets can be compressed in mDNS which I'd argue is a requirement for an mDNS library since where there's mDNS there's almost certainly DNS-SD. So I'd say an mDNS flag probably belongs on Msg and that flag will need to be weaved through the packers to do the job properly.

[markdingo]

I think ErrTruncated is only returned if there's no body after unpacking the message header

I don't think that's true.

if you look at https://github.com/miekg/dns/blob/master/msg.go#L818 you see that ErrTruncated is set in the else part of off != len(msg) (which means a good message) and if dns.Truncated is set (ie TC=1). And that's it.

[andrewtj]

@MarkDAtEmu you've selectively quoted me.

[markdingo]

Forking go dns to get mDNS semantics seems silly as 99.9% would be identical.

Agreed. Currently I wrapper miekg/dns to create mDNS semantics and that worked fine until I hit TC=1.

m.Profile could work.

Sure. But any signalling works for me. A new function ensures that no existing code will mistakenly trigger it, is all I was really trying to convey. You might have a bigger vision in mind in terms of other edge-case management or profile issues.

I'm a bit afraid of long term maintainability with if-elsing different protocol in the lib. But it is not every day someone invents a DNS like protocol.

I understand that concern. In practice, I have found that mDNS is mostly a subset with about 3-4 true extensions, so the if/else flagging is pretty minimal as best I can tell. Furthermore, I think that mDNS is much more of a niche protocol than regular DNS so I don't think that it will change at anything like the same rate - particularly as it's meant to be embeddable into tiny devices such as printers and IOT-type things.

So all in all, I think that there won't be too much if-elsing and it should be pretty constrained. If you put something like m.Profile in place I should be able to offer code that helps.

I'll also add that good mDNS library support is very thin on the ground. Finding libraries that help are tough. So it's not like there are any other choices that developers can use. Put another way, if you can put an mDNS framework in place in which we can add the quirks needed without breaking regular DNS, then the library should appeal to more customers.

but is mDNS a property of the packet or of the client/server?

Functionally it's a client/server attribute, but I use my own transport management and just use the miekg/dns parts for message management and serialization. In fact I think I had to do that as the miekg client/server code doesn't handle multicast transport. You need to join multicast groups and other goop.

So I guess you could go down the path of setting the mDNS attribute in the client/server classes if they supported multicast transports. For my application though, that wouldn't be that helpful. I'd still want to be able to set it on a Msg basis as I also happen to use other transports to carry around the mDNS message but I still rely on Pack() and Unpack() as my serialisers.

[markdingo]

you've selectively quoted me

Sorry. I was only trying to be helpful. Perhaps the most important thing you said is:

I think you already have the means to differentiate the two cases.

Can you give a code fragment that shows that? That is a message that is truly truncated semantically and a message that merely has TC=1 set.

[andrewtj]

package main

import (
        "fmt"
        
        "github.com/miekg/dns"
)       

func main() {
        m := new(dns.Msg)
        m.Truncated = true
        m.SetQuestion("example.", dns.TypeA)
        b, err := m.Pack()
        if err != nil {
                panic(err)
        }               
        err = m.Unpack(b)
        fmt.Printf("%v\n", err)
        err = m.Unpack(b[:14])
        fmt.Printf("%v\n", err)
}

go run tc.go
dns: failed to unpack truncated message
dns: buffer size too small

The first error is ErrTruncated and the second is ErrShortRead.

[markdingo]

The first error is ErrTruncated and the second is ErrShortRead.

ErrShortRead only signals extreme truncation. The sort of wire truncation I'm talking about is the more typical MTU limitation, such as at 512 or 1440 bytes. In that case, both returns are ErrTruncated.
If you change b[:14] to b[:24] you get this:

 dns: failed to unpack truncated message
 dns: failed to unpack truncated message

Below is a more typical example of how ErrTruncated is ambiguous to an mDNS recipient:

package main

import (
        "fmt"
        "github.com/miekg/dns"
)

const domain = "example."

func main() {
        snd := new(dns.Msg)
        snd.SetQuestion(domain, dns.TypeTXT)
        var truncLen int
        for i := 0; i < 40; i++ {
                r := &dns.TXT{
                        Hdr: dns.RR_Header{Name: domain,
                                Rrtype: dns.TypeTXT, Class: dns.ClassINET},
                        Txt: []string{"A longish string that fills up the RR"},
                }
                snd.Answer = append(snd.Answer, r)

                if i == 10 { // Pick a convenient RR boundary length as the truncation point
                        truncLen = snd.Len()
                }

                if snd.Len() > 1000 { // Don't exceed our MTU size
                        ns := &dns.NS{
                                Hdr: dns.RR_Header{Name: domain,
                                        Rrtype: dns.TypeNS, Class: dns.ClassINET},
                                Ns: "ns1.example.com.",
                        }
                        snd.Ns = append(snd.Ns, ns)

                        snd.Truncated = true // Tell mDNS client there are more messages coming
                        break
                }
        }

        b, err := snd.Pack()
        if err != nil {
                panic(err)
        }

        //////////////////////////////////////////////////////////////////////
        // The message contains one Question, 'x' TXT Answer RRs and
        // one NS Ns RRs. Simulate reception of complete message
        // across the wire.
        //////////////////////////////////////////////////////////////////////

        rcv1 := new(dns.Msg)
        err1 := rcv1.Unpack(b)

        // Simulate reception of message truncated on an RR boundary
        // due to MTU limitations.

        rcv2 := new(dns.Msg)
        err2 := rcv2.Unpack(b[:truncLen])

        fmt.Println("rcv1", len(rcv1.Answer), len(rcv1.Ns), rcv1.Truncated, err1)
        fmt.Println("rcv2", len(rcv2.Answer), len(rcv2.Ns), rcv2.Truncated, err2)

        //////////////////////////////////////////////////////////////////////
        // In the rcv1 case the client should be a happy camper that
        // simply waits for the subsequent messages. In the rcv2 case
        // the client should re-issue the query and log an error about
        // truncated transport issues.
        //
        // But, how does the client differentiate between rcv1 and
        // rcv2? It can't. And yet it has lost 10 or so RRs in rcv2
        // without knowing it.
        //////////////////////////////////////////////////////////////////////
}

This generates:

rcv1 18 1 true dns: failed to unpack truncated message
rcv2 11 0 true dns: failed to unpack truncated message

[andrewtj]

Okay I'm with you now. Sorry for the noise.

[miekg]

[ Quoting <notifications@github.com> in "Re: [miekg/dns] ErrTruncated should..." ]

> but is mDNS a property of the packet or of the client/server? Functionally it's a client/server attribute, but I use my own transport management and just use the miekg/dns parts for message management and serialization. In fact I think I had to do that as the miekg client/server code doesn't handle multicast transport. You need to join multicast groups and other goop. So I guess you could go down the path of setting the mDNS attribute in the client/server classes if they supported multicast transports. For my application though, that wouldn't be that helpful. I'd still want to be able to set it on a Msg basis as I also happen to use other transports to carry around the mDNS message but I still rely on Pack() and Unpack() as my serialisers.

Yes, we should see what minimal changes we would need to make this work. Just wondering here, would an abstraction like http.Transport be of use here? /Miek

…

-- Miek Gieben

[markdingo]

Just wondering here, would an abstraction like http.Transport be of use here?

Possibly. But I don't think you'd want to tie mDNS message semantics to the transport. For example, what if message construction differs?

One example is the CacheFlushBit that can be set in the message (RFC6762 10.2). Currently you might implement that as:

m := new(dns.Msg)
m.CacheFlush(true)

rr := new(dns.A)
m.Answer = append(m.Answer, rr)

transport.Exchange(m)

How would you tie that to the transport? Something like this?

m := new(dns.Msg)

rr := new(dns.A)
m.Answer = append(m.Answer, rr)

transport.CacheFlush(true)
transport.Exchange(m)

where transport.CacheFlush() only exists for the mDNS transport?

It's possible, but it gets ugly quickly as some of the mDNS semantics are in the Msg and some in the RRs.

An abstract transport is not a bad idea as multicast is quite different from unicast, but I think that's orthogonal to the mDNS message semantics. In the future DNS will likely be carried over HTTPS so then you have to go down the path of putting mDNS semantics into that transport too. Ug.

Having said all that, m.CacheFlush(true) is an ugly solution too as it allows unicast DNS applications to mistakenly set mDNS bits with unknown and undetectable outcomes.

It's too late now, but Msg probably should have been an interface - just as you did with RR. Or perhaps a new Message interface is the way to go?

If you were to go down that path, I imagine it might look something like this:

package dns

import (
        "fmt"
)

type commonMessage interface { // Composition only - not for export.                                                    
        Id() uint16
        Truncated() bool
        Len() int
        SetMTU(int)
        // AppendQuestion(*RR)                                                                                          
        // AppendAnswer(*RR)                                                                                            

        Pack() ([]byte, error) // Implemented by specialization                                                         
        Unpack([]byte) error   // Implemented by specialization                                                         
}

type concreteCommonMessage struct { // Implements commonMessage                                                         
        id        uint16
        truncated bool
}

func (cc *concreteCommonMessage) Id() uint16 {
        return cc.id
}

func (cc *concreteCommonMessage) Truncated() bool {
        return cc.truncated
}

func (cc *concreteCommonMessage) Len() int {
        return 0
}

func (cc *concreteCommonMessage) SetMTU(int) {
}

//////////////////////////////////////////////////////////////////////                                                  

type UnicastMessage interface { // Compose unicast as common+specialization
        commonMessage

        Authoritative() bool
        AuthenticatedData() bool
        //      CheckingDisabled() bool                                                                                 
        //      RecursionDesired() bool                                                                                 
        //      ...                                                                                                     
}

type concreteUnicastMessage struct { // Implements unicast specialization                                               
        concreteCommonMessage

        authoritative     bool
        authenticatedData bool
        // ...                                                                                                          
}

func (cu *concreteUnicastMessage) Authoritative() bool {
        return cu.authoritative
}

func (cu *concreteUnicastMessage) AuthenticatedData() bool {
        return cu.authenticatedData
}

func (cu *concreteUnicastMessage) Pack() ([]byte, error) {  // Implements common interface 
        return []byte{}, nil
}

func (cu *concreteUnicastMessage) Unpack(buffer []byte) error { // Implements common interface                          
        return nil
}

//////////////////////////////////////////////////////////////////////                                                  

type MulticastMessage interface { // Compose multicast as common+specialization                                         
        commonMessage

        CacheFlush() bool
        UnicastResponse() bool
}

type concreteMulticastMessage struct { // Implements multicast specialization                                           
        concreteCommonMessage

        cacheFlush      bool
        unicastResponse bool
}

func (cm *concreteMulticastMessage) CacheFlush() bool {
        return cm.cacheFlush
}

func (cm *concreteMulticastMessage) UnicastResponse() bool {
        return cm.unicastResponse
}

func (cu *concreteMulticastMessage) Pack() ([]byte, error) {  // Implements common interface 
        return []byte{}, nil
}

func (cu *concreteMulticastMessage) Unpack(buffer []byte) error { // Implements common interface                        
        return nil
}

//////////////////////////////////////////////////////////////////////                                                  

func NewUnicastMessage() UnicastMessage {
        m := new(concreteUnicastMessage)
        return m
}

func NewMulticastMessage() MulticastMessage {
	m := new(concreteMulticastMessage)
        return m
}

//////////////////////////////////////////////////////////////////////                                                  

type transport struct { // IRL this would be an interface too                                                           
}

func NewTransport() *transport {
        t := new(transport)
        return t
}

func (t *transport) Exchange(common commonMessage) {
        fmt.Print("Common message with Pack() but also access to specialized...")
        common.Pack() // Actually calls concrete specialization

        switch m := common.(type) {
        case UnicastMessage:
                fmt.Println("unicast Authoritative() if needed", m.Authoritative())
        case MulticastMessage:
                fmt.Println("multicast CacheFlush() if needed", m.CacheFlush())
        }
}

which is then used something like this:

package main

import (
        "./dns"
        "fmt"
)

//////////////////////////////////////////////////////////////////////                                                  

func main() {
        trans := dns.NewTransport()
        uni := dns.NewUnicastMessage()
        multi := dns.NewMulticastMessage()

        myProcessUni(uni) // Application specific processing                                                            
        myProcessMulti(multi)

        trans.Exchange(uni) // Generic package processing                                                               
        trans.Exchange(multi)
}

func myProcessUni(m dns.UnicastMessage) {
        fmt.Println("Unicast message with common Id + specialized Authoritative()", m.Id(), m.Authoritative())
}

func myProcessMulti(m dns.MulticastMessage) {
        fmt.Println("multicast message with common Id + specialized CacheFlush()", m.Id(), m.CacheFlush())
}

[miekg]

Think we should aim for the most minimal change that will make things work correctly. It that fails or becomes to ugly we should see what more we can do (including a dns.Transport).

So to take that route: what does a typical mDNS program need to do and what can be moved to the dns package in a sane way (or what override do we need to provide). Some functions could become variables that you can overrule for instance...?

[markdingo]

what does a typical mDNS program need to do and what can be moved to the dns package

Let me chew on that a bit.

Todate my mDNS wrapper has been able to do everything on top of miekg/dns - albeit some things are a bit kludgey such as the embedded Class flags. The ErrTruncated was the first semantic difference that I couldn't reliably work around.

Most of the mDNS differences are in the transport rather than the message format so I don't expect many variants. I think my preference would be some sort of mDNS profile setting in Msg. That way we can add extras as needed without changing any other APIs.

The main thing is that due to the open nature of Msg{} you can easily construct mDNS messages that don't make sense - such as setting Id and adding Ns RRs. But we'll have to worry about that another time.

[miekg]

[ Quoting <notifications@github.com> in "Re: [miekg/dns] ErrTruncated should..." ]

> what does a typical mDNS program need to do and what can be moved to the dns package Let me chew on that a bit.

Ack.

Todate my mDNS wrapper has been able to do everything on top of miekg/dns - albeit some things are a bit kludgey such as the embedded Class flags. The ErrTruncated was the first semantic difference that I couldn't reliably work around.

Yes, ErrTruncated (and that handling) will need some deep fixes (and docs!).

Most of the mDNS differences are in the transport rather than the message format so I don't expect many variants. I think my preference would be some sort of mDNS profile setting in Msg. That way we can add extras as needed without changing any other APIs. The main thing is that due to the open nature of Msg{} you can easily construct mDNS messages that don't make sense - such as setting Id and adding Ns RRs. But we'll have to worry about that another time.

This library does not care too much about that. If you want to construct something whacky there isn't much holding you back. Great for testing I would say :) /Miek

…

-- Miek Gieben

[sayotte]

Not sniping, but @MarkDAtEmu you made a slight mis-statement that I think complicates things a bit:

type MulticastMessage interface { // Compose multicast as common+specialization                                         
        commonMessage

        CacheFlush() bool
        UnicastResponse() bool
}

Both of those properties are set on a per-record basis: cache-flush for RRs, unicast-response for QRs. They're not message-wide, and in fact I don't think there're any modifications to the message header at all for mDNS. So, I don't think we need to think super hard about how to change that.

I'm not very familiar with the miekg/dns code / API so I can't yet comment on where/what we'd need to add or change things. But this list (cut/pasted selectively from the summary of differences from DNS in RFC-6762) strikes me as the things that are likely to cause issues in basic encoding/decoding of messages:

• allows name compression in rdata for SRV and other record types
• allows larger UDP packets
• allows more than one question in a query message
• uses the TC bit in a query to indicate additional Known Answers
• defines a unicast-response bit in the rrclass of query questions
• defines a cache-flush bit in the rrclass of response records

Additionally, as I mentioned over in Issue #436, Bonjour / RFC-6763 different format for the RDATA section of TXT records. Since they have the same Type code it'd be impossible for the code to "know" that they should be decoded differently without a hint. That's fine if the outer caller knows to reinterpret the RDATA, but I think we're discussing right now how/where to provide a hint anyway. So maybe there's a convenience that can be added there.

[miekg]

allows name compression in rdata for SRV and other record types

not a problem

allows larger UDP packets

how, by default? Or uses EDNS0? If not EDNS0 this will be hard to implement.

allows more than one question in a query message

This is a slice (to allow multiple) but there is code enforcing there is a max of one IIRC

uses the TC bit in a query to indicate additional Known Answers

That is this discussion

defines a unicast-response bit in the rrclass of query questions

Just a function on a RR, so meh

defines a cache-flush bit in the rrclass of response records

Just a function on a RR, so meh

[miekg]

Open for a long time, closing as wont-fix for the time being.