feat: allow querying with RecordType labels #5

SgtPooki · 2024-03-20T21:27:49Z

feat: support records as numeric or string label
chore: remove unnecessary toType method
test: add basic useRecordTypeValue tests
fix: cache key uses RecordType only

This helps enable users to resolve the issues brought up in ipfs/helia#474 and ipfs/service-worker-gateway#130

SgtPooki

self review

SgtPooki · 2024-03-20T21:30:12Z

src/utils/cache.ts

+  /**
+   * We index the cache on 'domain-{@link RecordType}` instead of
+   * 'domain-{@link RecordType}|{@link RecordTypeLabel}' to ensure that we don't
+   * cache the same answers separately for RecordType.A and RecordTypeLabel.A.
+   *
+   * This means, if you query a resolver with `useRecordTypeValue=true`, and
+   * they return an empty answer, that empty answer will be cached, and a
+   * subsequent call with `useRecordTypeValue=false` would need to be paired
+   * with `cached: false` to avoid getting the empty answer back when using a
+   * {@link RecordTypeLabel}.
+   *
+   * NOTE: this will resolve/obfuscate the issue where dns resolvers return
+   * different answers depending on the value of the "type" field in the query.
+   * But give the user the ability to retry with a different type if they want
+   */
+  private getKey (domain: string, type: RecordType | RecordTypeLabel): string {
+    return `${domain.toLowerCase()}-${convertType(type, true)}`
+  }


Are we good with this? We could also check the cache for either cached type in dns.ts and then convert it, so we're only caching exactly what the response is.

i.e.

let cached if (options.cached !== false) { cached = this.cache.get(domain, types) ?? this.cache.get(domain, getTypes(types, !options.useRecordTypeValue)) }

SgtPooki · 2024-03-20T21:31:00Z

src/utils/get-types.ts

+    const reverseMap = {
+      [RecordTypeLabel.A]: RecordType.A,
+      [RecordType.A]: RecordTypeLabel.A,
+      [RecordTypeLabel.CNAME]: RecordType.CNAME,
+      [RecordType.CNAME]: RecordTypeLabel.CNAME,
+      [RecordTypeLabel.TXT]: RecordType.TXT,
+      [RecordType.TXT]: RecordTypeLabel.TXT,
+      [RecordTypeLabel.AAAA]: RecordType.AAAA,
+      [RecordType.AAAA]: RecordTypeLabel.AAAA
+    }


if this is defined outside of the function, we get Cannot read properties of undefined (reading 'A')

SgtPooki · 2024-03-20T21:31:22Z

src/utils/get-types.ts

+
+export function convertType (type: RecordType | RecordTypeLabel, useRecordTypeValue?: true): RecordType
+export function convertType (type: RecordType | RecordTypeLabel, useRecordTypeValue?: false): RecordTypeLabel
+export function convertType (type: RecordType | RecordTypeLabel, useRecordTypeValue?: boolean): RecordType | RecordTypeLabel


typescript doesn't see the implementation's definition with function overrides, so we have to define this one too

SgtPooki · 2024-03-20T21:31:27Z

src/utils/get-types.ts

+
+export function getTypes (types?: (RecordType | RecordTypeLabel) | Array<RecordType | RecordTypeLabel>, useRecordTypeValue?: true): RecordType[]
+export function getTypes (types?: (RecordType | RecordTypeLabel) | Array<RecordType | RecordTypeLabel>, useRecordTypeValue?: false): RecordTypeLabel[]
+export function getTypes (types?: (RecordType | RecordTypeLabel) | Array<RecordType | RecordTypeLabel>, useRecordTypeValue?: boolean): Array<RecordType | RecordTypeLabel>


typescript doesn't see the implementation's definition with function overrides, so we have to define this one too

src/resolvers/default.ts

src/index.ts

SgtPooki · 2024-03-20T21:46:11Z

src/dns.ts

@@ -44,12 +46,16 @@ export class DNS implements DNSInterface {
   * Any new responses will be added to the cache for subsequent requests.
   */
  async query (domain: string, options: QueryOptions = {}): Promise<DNSResponse> {
-    const types = getTypes(options.types)
+    options.useRecordTypeValue = options.useRecordTypeValue ?? this.useRecordTypeValue
+    const types = getTypes(options.types, options.useRecordTypeValue)
    const cached = options.cached !== false ? this.cache.get(domain, types) : undefined


see #5 (comment)

Maybe:

Suggested change

const cached = options.cached !== false ? this.cache.get(domain, types) : undefined

let cached

if (options.cached !== false) {

cached = this.cache.get(domain, types) ?? this.cache.get(domain, getTypes(types, !options.useRecordTypeValue))

}

SgtPooki · 2024-03-20T22:21:50Z

src/dns.ts

+        result.Answer = result.Answer
+          .map((answer) => {
+            // convert type to either RecordType or RecordTypeLabel
+            answer.type = convertType(answer.type, options.useRecordTypeValue)
+
+            return answer
+          })
+
        for (const answer of result.Answer) {
          this.cache.add(domain, answer)
        }


If we implement #5 (comment), we should change this to

Suggested change

result.Answer = result.Answer

.map((answer) => {

// convert type to either RecordType or RecordTypeLabel

answer.type = convertType(answer.type, options.useRecordTypeValue)

return answer

})

for (const answer of result.Answer) {

this.cache.add(domain, answer)

}

for (const answer of result.Answer) {

this.cache.add(domain, answer)

}

result.Answer = result.Answer

.map((answer) => {

// convert type to either RecordType or RecordTypeLabel

answer.type = convertType(answer.type, options.useRecordTypeValue)

return answer

})

achingbrain · 2024-04-03T09:06:59Z

I think this fix may be a lot more complicated than it needs to be.

If the specs say we can send text or numeric values to dns-json-over-https endpoints, but the endpoints only really support text values, then lets just map the numeric values to text before sending?

We are still compliant with the spec then, and we don't have to shift the number-or-text decision on to the user.

SgtPooki · 2024-04-04T00:48:47Z

I think this fix may be a lot more complicated than it needs to be.

If the specs say we can send text or numeric values to dns-json-over-https endpoints, but the endpoints only really support text values, then lets just map the numeric values to text before sending?

We are still compliant with the spec then, and we don't have to shift the number-or-text decision on to the user.

Isn't it possible that a server won't support one or the other? we could easily take the stand that we don't want to support dns resolvers that don't accept either text or numeric, but the code is here and gives users the ability to pass the values they need for the resolvers they're using

achingbrain · 2024-04-04T11:37:26Z

Isn't it possible that a server won't support one or the other?

Ha, well, servers should implement the spec and the spec says they should support both.

the code is here and gives users the ability to pass the values they need for the resolvers they're using

As implemented the code lets users switch between numbers and strings for each request - do we need this flexibility? My gut feeling is no, a server will support one or the other.

Or the server will support strings and might support numbers. Given that strings are easier to reason about I think we should just convert the enums to strings instead of numbers - if we still see significant breakage in the wild then ok, let's be more flexible.

SgtPooki · 2024-04-04T14:22:25Z

Ha, well, servers should implement the spec and the spec says they should support both.

I couldn't find in the RFC (1035) where it explicitly says that value and label should be accepted.

google says it can be either, and cloudflare says it can be either (though DoH and DoT don't specify any difference AFAICT), but RFC1035(https://datatracker.ietf.org/doc/html/rfc1035) says:

TYPE two octets containing one of the RR TYPE codes.

If QTYPE can only be two octets, then it can't ever be "TXT," right? Also, the RFC explicitly states that it must be a "RR TYPE code." Does that mean either representation as represented in the RFC, or the on-the-wire TYPE value? It's not 100% clear either way.

a two octet code which specifies the type of the query.

            The values for this field include all codes valid for a
            TYPE field, together with some more general codes which
            can match more than one type of RR.

and there is this:

Name servers and resolvers must compare labels in a case-insensitive manner (i.e., A=a), assuming ASCII with zero parity. Non-alphabetic codes must match exactly.

but it doesn't state that those labels must be accepted.

It doesn't seem like RFC1035 specifies that label/mnemonic strings MUST be accepted. It seems to imply that only numeric values should be accepted, but I believe it's talking about the records on the wire.

Overall, it's ambiguous to me, but maybe I'm missing something. However, I am sure we will eventually run into issues with some dns provider or another that implements the spec wrong.. Most IETF specs are not human-readable, IMHO, and therefore easily misinterpreted.

With all that said, I think if we use just numbers, we can move ahead, since the label/mnemonic representation usually seems like higher-level syntax. I will update this PR with a change to the enum values

SgtPooki added 4 commits March 20, 2024 12:17

feat: support records as numeric or string label

5981a87

chore: remove unnecessary toType method

da87ac4

test: add basic useRecordTypeValue tests

062f96b

fix: cache key uses RecordType only

b5f05b5

SgtPooki commented Mar 20, 2024

View reviewed changes

SgtPooki added 3 commits March 20, 2024 14:47

fix: default resolver only compares RecordType

6db53bd

chore: jsdoc update from pr commentUpdate src/index.ts

7bfebab

chore: ensure resolvers respect useRecordTypeValue

4429d5a

SgtPooki mentioned this pull request Mar 20, 2024

DoH queries with numeric type fail ipfs/helia#474

Closed

SgtPooki commented Mar 20, 2024

View reviewed changes

SgtPooki requested a review from achingbrain April 2, 2024 19:49

SgtPooki self-assigned this Apr 2, 2024

This was referenced Apr 5, 2024

fix: use string enum for RecordType [WIP] #6

Closed

fix: cloudflare dns only supports string type #7

Merged

SgtPooki closed this Apr 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow querying with RecordType labels #5

feat: allow querying with RecordType labels #5

SgtPooki commented Mar 20, 2024

SgtPooki left a comment

SgtPooki Mar 20, 2024

SgtPooki Mar 20, 2024

SgtPooki Mar 20, 2024

SgtPooki Mar 20, 2024

SgtPooki Mar 20, 2024

SgtPooki Mar 20, 2024

achingbrain commented Apr 3, 2024 •

edited

Loading

SgtPooki commented Apr 4, 2024

achingbrain commented Apr 4, 2024

SgtPooki commented Apr 4, 2024

-    const cached = options.cached !== false ? this.cache.get(domain, types) : undefined
+    let cached
+    if (options.cached !== false) {
+      cached = this.cache.get(domain, types) ?? this.cache.get(domain, getTypes(types, !options.useRecordTypeValue))
+    }

feat: allow querying with RecordType labels #5

feat: allow querying with RecordType labels #5

Conversation

SgtPooki commented Mar 20, 2024

SgtPooki left a comment

Choose a reason for hiding this comment

SgtPooki Mar 20, 2024

Choose a reason for hiding this comment

SgtPooki Mar 20, 2024

Choose a reason for hiding this comment

SgtPooki Mar 20, 2024

Choose a reason for hiding this comment

SgtPooki Mar 20, 2024

Choose a reason for hiding this comment

SgtPooki Mar 20, 2024

Choose a reason for hiding this comment

SgtPooki Mar 20, 2024

Choose a reason for hiding this comment

achingbrain commented Apr 3, 2024 • edited Loading

SgtPooki commented Apr 4, 2024

achingbrain commented Apr 4, 2024

SgtPooki commented Apr 4, 2024

achingbrain commented Apr 3, 2024 •

edited

Loading