Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use golang.org/x/crypto/cryptobyte to unpack DNS records #1507

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Commits on Nov 6, 2023

  1. Use golang.org/x/crypto/cryptobyte to unpack DNS records

    This substantially simplifies the machinery and state tracking around
    DNS record unpacking. Gone are the `off int` arguments! Gone are the
    manual slice length checks!
    
    Care has been taken to maintain existing behaviour, including quirks,
    with the only notable intentional change being to UnpackDomainName
    where we now strictly enforce that pointers point backwards into the
    message.
    tmthrgd committed Nov 6, 2023
    Configuration menu
    Copy the full SHA
    a4fd336 View commit details
    Browse the repository at this point in the history

Commits on Nov 7, 2023

  1. Eliminate dnsString type

    Instead of using a custom type we're just going to pass the full msg
    byte-slice down the call stack. This makes this substantially simpler
    and allows for some nice optimisations and cleanups.
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    00fe85c View commit details
    Browse the repository at this point in the history
  2. Use cryptobyte.String to unpack EDNS0 and SVCB values

    This unpacking is more strict than previous. It now validates that no
    trailing data exists.
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    a205faf View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    3e37cee View commit details
    Browse the repository at this point in the history
  4. Avoid *cryptobyte.String allocation in UnpackDomainName

    This is ugly, but there doesn't really seem to be a way to get around
    that. The previous code was visually elegant but allocated far too much.
    This code is efficient but visually ugly. *sigh*
    
    I'm not sure if `msg = &b` causing b to escape is a missed optimisation
    in the compiler or not, but with go1.21 that's how it is.
    
    Also adjust our end-of-name check slightly.
    
    name                                   old time/op    new time/op    delta
    UnpackDomainName-12                       152ns ± 6%     148ns ±13%     ~     (p=0.353 n=10+10)
    UnpackDomainNameUnprintable-12            120ns ±12%     122ns ± 9%     ~     (p=0.616 n=10+10)
    UnpackDomainNameLongest-12                618ns ±11%     558ns ± 9%   -9.83%  (p=0.000 n=10+9)
    UnpackDomainNameLongestUnprintable-12    1.60µs ±16%    1.55µs ±13%     ~     (p=0.447 n=10+9)
    UnpackDomainNamePointers-12               302ns ±17%     149ns ± 6%  -50.51%  (p=0.000 n=10+10)
    
    name                                   old alloc/op   new alloc/op   delta
    UnpackDomainName-12                       64.0B ± 0%     64.0B ± 0%     ~     (all equal)
    UnpackDomainNameUnprintable-12            48.0B ± 0%     48.0B ± 0%     ~     (all equal)
    UnpackDomainNameLongest-12                 256B ± 0%      256B ± 0%     ~     (all equal)
    UnpackDomainNameLongestUnprintable-12    1.02kB ± 0%    1.02kB ± 0%     ~     (all equal)
    UnpackDomainNamePointers-12                120B ± 0%       48B ± 0%  -60.00%  (p=0.000 n=10+10)
    
    name                                   old allocs/op  new allocs/op  delta
    UnpackDomainName-12                        1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    UnpackDomainNameUnprintable-12             1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    UnpackDomainNameLongest-12                 1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    UnpackDomainNameLongestUnprintable-12      1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    UnpackDomainNamePointers-12                4.00 ± 0%      1.00 ± 0%  -75.00%  (p=0.000 n=10+10)
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    ed4bfbb View commit details
    Browse the repository at this point in the history
  5. Refactor DNS message unpacking behaviour

    We keep some of the existing weird behaviour (mostly of not erroring
    when we should), but refactor it so it's far more obvious and things
    are cleaner.
    
    We leave a number of TODOs to address some of the weirder choices. They
    would also allow for some other improvements to the code if we can make
    those changes.
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    a286fbc View commit details
    Browse the repository at this point in the history
  6. Return final offset when UnpackRR(WithHeader) errors

    This was to maintain existing undocumented behaviour, but this makes
    more sense.
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    293f1d3 View commit details
    Browse the repository at this point in the history
  7. Fix TestTruncatedMsg logic

    This logic in this test case was wrong which caused strange behaviour
    when re-working the domain name unpacking code.
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    785960e View commit details
    Browse the repository at this point in the history
  8. Replace errUnpack(Signed)Overflow with ErrBuf

    This is the more correct error to be returning.
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    857cbcf View commit details
    Browse the repository at this point in the history
  9. Delete TestTruncatedMsg

    Actually no matter what I seem to do this test is terribly broken. I
    can't be bothered wrestling with it any longer. Goodbye.
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    acaf1b4 View commit details
    Browse the repository at this point in the history
  10. Allow unpackCounted to return partial results

    Now that TestTruncatedMsg is gone, we can implement the sane behaviour
    here.
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    1b5a3fa View commit details
    Browse the repository at this point in the history
  11. Avoid passing *cryptobyte.String through an interface

    In some places this results in more readable code, in other places less.
    What we gain though is the same number of allocations that we started
    with. In other words, we haven't made things worse!
    
    Without this change, every time we pass *cryptobyte.String into an
    interface it is forced onto the heap and we get a nasty allocation.
    
    It does require us to check that all the data has been consumed at
    the end of each `unpack([]byte) error` call instead of having a
    single check. Ultimately this isn't too bad.
    
    We could change unpack to return whatever is left over. We don't
    do this for semantic reasons. unpack is required to consume the
    entirety of data (as we know it's length ahead of time). Asking
    it to return whatever is left for the sole purpose of checking
    it's nothing is silly. In some places we pass a pointer or do
    return the rest. In these cases, it's because we don't know
    ahead of time how long something will be.
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    c4db081 View commit details
    Browse the repository at this point in the history
  12. Remove header count updates in (*Msg).unpack

    These never actually did anything as dh is a struct and we don't consult
    it again.
    tmthrgd committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    1150c4f View commit details
    Browse the repository at this point in the history

Commits on Nov 8, 2023

  1. Un-genericfy unpackCounted

    While it's nice having this as a single function rather than one for
    Question and one for RR, using generics and passing the unpack function
    causes the *cryptobyte.String to escape. That leaves us with some
    rather ugly workarounds. Instead just bite the bullet and write it
    twice. If we chose to remove the msg.Empty() check in the future, then
    this doesn't end up too bad at all.
    tmthrgd committed Nov 8, 2023
    Configuration menu
    Copy the full SHA
    f4aaceb View commit details
    Browse the repository at this point in the history
  2. Introduce offset helper

    This helper validates that the offset can be calculated correctly.
    
    We have to restrict msgBuf in unpackRRWithHeader as previously we would
    sometimes computing invalid offsets in unpackDomainName. This helper is
    designed to ensure we don't make this mistake again.
    tmthrgd committed Nov 8, 2023
    Configuration menu
    Copy the full SHA
    689bd5d View commit details
    Browse the repository at this point in the history
  3. Fix pointer offset check in unpackDomainName

    We need to compare against the start of the pointer not the end of it.
    
    While we're here, improve TestUnpackDomainName and ensure it's testing
    UnpackDomainName properly.
    tmthrgd committed Nov 8, 2023
    Configuration menu
    Copy the full SHA
    390ac04 View commit details
    Browse the repository at this point in the history
  4. Remove _ = s from generated unpack code

    This isn't needed now that we always check s.Empty().
    tmthrgd committed Nov 8, 2023
    Configuration menu
    Copy the full SHA
    73318a7 View commit details
    Browse the repository at this point in the history
  5. Avoid spilling RR_Header to heap in unpacking happy path

    Returning &h (*RR_Header) as an RR causes h to spill to the heap and
    allocate. Given this is only needed for a specific error and not in the
    happy path, we instead create a local copy of h and return a pointer to
    that instead. That eliminates the allocation on success.
    
    While we're here, remove the new(RR_Header) return in UnpackRR. This
    doesn't make any sense as it contains no information. We already return
    nil in the error case above.
    
    It shows a nice improvement for several of the benchmarks.
    
    name                                   old time/op    new time/op    delta
    UnpackDomainName-12                       153ns ±14%     148ns ± 6%     ~     (p=0.268 n=10+9)
    UnpackDomainNameUnprintable-12            124ns ± 4%     123ns ± 6%     ~     (p=0.591 n=10+10)
    UnpackDomainNameLongest-12                581ns ±12%     561ns ±15%     ~     (p=0.190 n=10+10)
    UnpackDomainNameLongestUnprintable-12    1.76µs ±24%    1.66µs ±20%     ~     (p=0.393 n=10+10)
    UnpackDomainNamePointers-12               146ns ± 5%     146ns ± 3%     ~     (p=0.730 n=9+9)
    UnpackA-12                                194ns ±15%     135ns ±12%  -30.14%  (p=0.000 n=10+10)
    UnpackMX-12                               200ns ±15%     140ns ±10%  -29.96%  (p=0.000 n=10+10)
    UnpackAAAA-12                             199ns ±15%     153ns ±14%  -22.88%  (p=0.000 n=10+10)
    UnpackMsg-12                             1.48µs ±16%    1.26µs ±13%  -14.68%  (p=0.001 n=10+10)
    UnpackString/Escaped-12                  84.8ns ± 7%    79.6ns ± 6%   -6.12%  (p=0.003 n=9+10)
    UnpackString/Unescaped-12                46.5ns ±15%    47.8ns ±14%     ~     (p=0.353 n=10+10)
    Serve-12                                 58.3µs ± 2%    57.8µs ± 3%     ~     (p=0.065 n=9+10)
    Serve6-12                                61.4µs ±10%    61.1µs ± 8%     ~     (p=0.796 n=10+10)
    ServeCompress-12                         59.1µs ± 2%    58.6µs ± 2%     ~     (p=0.113 n=9+10)
    
    name                                   old alloc/op   new alloc/op   delta
    UnpackDomainName-12                       64.0B ± 0%     64.0B ± 0%     ~     (all equal)
    UnpackDomainNameUnprintable-12            48.0B ± 0%     48.0B ± 0%     ~     (all equal)
    UnpackDomainNameLongest-12                 256B ± 0%      256B ± 0%     ~     (all equal)
    UnpackDomainNameLongestUnprintable-12    1.02kB ± 0%    1.02kB ± 0%     ~     (all equal)
    UnpackDomainNamePointers-12               48.0B ± 0%     48.0B ± 0%     ~     (all equal)
    UnpackA-12                                 100B ± 0%       68B ± 0%  -32.00%  (p=0.000 n=10+10)
    UnpackMX-12                                100B ± 0%       68B ± 0%  -32.00%  (p=0.000 n=10+10)
    UnpackAAAA-12                              112B ± 0%       80B ± 0%  -28.57%  (p=0.000 n=10+10)
    UnpackMsg-12                               584B ± 0%      520B ± 0%  -10.96%  (p=0.000 n=10+10)
    UnpackString/Escaped-12                   32.0B ± 0%     32.0B ± 0%     ~     (all equal)
    UnpackString/Unescaped-12                 24.0B ± 0%     24.0B ± 0%     ~     (all equal)
    Serve-12                                 3.53kB ± 0%    3.50kB ± 0%   -0.91%  (p=0.000 n=10+10)
    Serve6-12                                3.37kB ± 0%    3.34kB ± 0%   -0.95%  (p=0.000 n=10+8)
    ServeCompress-12                         3.74kB ± 0%    3.71kB ± 0%   -0.87%  (p=0.000 n=10+10)
    
    name                                   old allocs/op  new allocs/op  delta
    UnpackDomainName-12                        1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    UnpackDomainNameUnprintable-12             1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    UnpackDomainNameLongest-12                 1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    UnpackDomainNameLongestUnprintable-12      1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    UnpackDomainNamePointers-12                1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    UnpackA-12                                 3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.000 n=10+10)
    UnpackMX-12                                3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.000 n=10+10)
    UnpackAAAA-12                              3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.000 n=10+10)
    UnpackMsg-12                               12.0 ± 0%      10.0 ± 0%  -16.67%  (p=0.000 n=10+10)
    UnpackString/Escaped-12                    1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    UnpackString/Unescaped-12                  1.00 ± 0%      1.00 ± 0%     ~     (all equal)
    Serve-12                                   55.0 ± 0%      54.0 ± 0%   -1.82%  (p=0.000 n=10+10)
    Serve6-12                                  52.0 ± 0%      51.0 ± 0%   -1.92%  (p=0.000 n=10+10)
    ServeCompress-12                           57.0 ± 0%      56.0 ± 0%   -1.75%  (p=0.000 n=10+10)
    tmthrgd committed Nov 8, 2023
    Configuration menu
    Copy the full SHA
    6b408b5 View commit details
    Browse the repository at this point in the history
  6. Rename msg *cryptobyte.String when not specifically referring to the …

    …message
    
    Semantically this makes more sense. We also rename a few variables here
    and there to be more consistent.
    tmthrgd committed Nov 8, 2023
    Configuration menu
    Copy the full SHA
    198f1a8 View commit details
    Browse the repository at this point in the history
  7. Be consistent in how we format EDNS0 methods

    Also remove some pointless `err != nil` checks.
    tmthrgd committed Nov 8, 2023
    Configuration menu
    Copy the full SHA
    f13deab View commit details
    Browse the repository at this point in the history
  8. Configuration menu
    Copy the full SHA
    c74fce5 View commit details
    Browse the repository at this point in the history
  9. Use semantically sensible error messages when unpacking

    This fixes a CoreDNS test failure
    (github.com/coredns/coredns/plugin/pkg/proxy.TestCoreDNSOverflow) that
    was testing behaviour that is sensitive to the specific error message
    returned when unpacking overflowing messages or that it is ErrBuf.
    
    It also ensures we correctly distinguish between invalid record data
    and invalid message framing. Ideally the `unpack(...) error` methods
    (on RR, EDNS0 and SVCBKeyValue) would actually be `unpack(...) bool`
    and we'd allow ourselves more freedom on how we report unpacking
    failures.
    tmthrgd committed Nov 8, 2023
    Configuration menu
    Copy the full SHA
    0f65f64 View commit details
    Browse the repository at this point in the history
  10. Report unpacking success rather than error for EDNS0 and SVCBKeyValue

    In a small number of instances we lose some error information with this
    change. I don't think it's particularly valuable information and this
    approach is far simpler, far less vague about what errors are
    appropriate and match how crypto/tls uses cryptobyte.String to unmarshal
    handshake messages.
    tmthrgd committed Nov 8, 2023
    Configuration menu
    Copy the full SHA
    c347f2f View commit details
    Browse the repository at this point in the history
  11. Report unpacking success rather than error for unpackMsgHdr

    This code only ever returns a single error value.
    tmthrgd committed Nov 8, 2023
    Configuration menu
    Copy the full SHA
    9b4c3a9 View commit details
    Browse the repository at this point in the history
  12. Configuration menu
    Copy the full SHA
    69b9deb View commit details
    Browse the repository at this point in the history

Commits on Nov 17, 2023

  1. Address review comments

    tmthrgd committed Nov 17, 2023
    Configuration menu
    Copy the full SHA
    8b86ec4 View commit details
    Browse the repository at this point in the history