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

Support option to disable DNSSEC validation #38

Merged
merged 9 commits into from
Nov 23, 2022
Merged

Support option to disable DNSSEC validation #38

merged 9 commits into from
Nov 23, 2022

Conversation

gnarea
Copy link
Contributor

@gnarea gnarea commented Nov 23, 2022

This enables the cd flag when DNSSEC records are requested, but we want to disable DNSSEC validation on the resolver.

I created this test file to test the changes against real resolvers (but didn't commit it):

// pkg/dohdec/test/real.ava.js

import {DNSoverHTTPS} from '../lib/doh.js'
import {DNSoverTLS} from '../lib/index.js'
// eslint-disable-next-line node/no-missing-import
import test from 'ava'

test('DoH - no dnssec', async t => {
  const doh = new DNSoverHTTPS()
  const r = await doh.lookup('ietf.org', {
    json: false,
    dnssec: false,
  })
  t.truthy(r)

  t.false(r.answers.some(a => a.type === 'RRSIG'))
  t.false(r.flag_cd)
})

test('DoH - dnssec', async t => {
  const doh = new DNSoverHTTPS()
  const r = await doh.lookup('ietf.org', {
    json: false,
    dnssec: true,
  })
  t.truthy(r)

  t.true(r.answers.some(a => a.type === 'RRSIG'))
  t.false(r.flag_cd)
})

test('DoH - dnssec failed', async t => {
  const doh = new DNSoverHTTPS()
  const r = await doh.lookup('dnssec-failed.org', {
    json: false,
    dnssec: true,
  })
  t.truthy(r)

  t.is(r.rcode, 'SERVFAIL')
  t.is(r.answers.length, 0)
  t.false(r.flag_cd)
})

test('DoH - dnssec failed with cd=1', async t => {
  const doh = new DNSoverHTTPS()
  const r = await doh.lookup('dnssec-failed.org', {
    json: false,
    dnssec: true,
    dnssecCd: true,
  })
  t.truthy(r)

  t.is(r.rcode, 'NOERROR')
  t.true(r.answers.some(a => a.type === 'RRSIG'))
  t.true(r.flag_cd)
})

test('DoT - no dnssec', async t => {
  const dot = new DNSoverTLS()
  const r = await dot.lookup('ietf.org', {
    json: false,
    dnssec: false,
  })
  t.truthy(r)

  t.false(r.answers.some(a => a.type === 'RRSIG'))
  t.false(r.flag_cd)
})

test('DoT - dnssec', async t => {
  const dot = new DNSoverTLS()
  const r = await dot.lookup('ietf.org', {
    json: false,
    dnssec: true,
  })
  t.truthy(r)

  t.true(r.answers.some(a => a.type === 'RRSIG'))
  t.false(r.flag_cd)
})

test('DoT - dnssec failed', async t => {
  const dot = new DNSoverTLS()
  const r = await dot.lookup('dnssec-failed.org', {
    json: false,
    dnssec: true,
  })
  t.truthy(r)

  t.is(r.rcode, 'SERVFAIL')
  t.is(r.answers.length, 0)
  t.false(r.flag_cd)
})

Fixes #37

This enables the `cd` flag when DNSSEC records are requested, but we want to disable DNSSEC validation on the resolver.

Fixes #37
pkg/dohdec/types/dot.d.ts Outdated Show resolved Hide resolved
@gnarea
Copy link
Contributor Author

gnarea commented Nov 23, 2022

I spotted a couple of bugs whilst testing the PR but now everything looks good and I won't be making further changes until you provide some feedback.

gnarea added a commit to relaycorp/dnssec-js that referenced this pull request Nov 23, 2022
Copy link
Owner

@hildjj hildjj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty close. I had a few minor questions that aren't blockers.
I'd like two changes, please:

  • Add yourself to a new contributors section in the package.json
  • Add a corresponding CLI option

Only the first is mandatory, I can hack the CLI option in if you don't have time.

pkg/dohdec/test/fixtures/doh.ava.js.json Show resolved Hide resolved
pkg/dohdec/test/dnsUtils.ava.js Show resolved Hide resolved
pkg/dohdec/types/doh.d.ts Outdated Show resolved Hide resolved
pkg/dohdec-cli/lib/cli.js Outdated Show resolved Hide resolved
@hildjj
Copy link
Owner

hildjj commented Nov 23, 2022

OK, this looks ready to go to me. There's a solid 10% chance this is going to break GHA when it lands, because I hadn't updated the action definition to run on pull requests to main instead of master, but I'll deal with that in my update PR that will come after this. Anything else before I merge?

@gnarea
Copy link
Contributor Author

gnarea commented Nov 23, 2022

Thank you! I think we're good to go! 🚀

@hildjj hildjj merged commit 4356411 into hildjj:main Nov 23, 2022
kodiakhq bot pushed a commit to relaycorp/dnssec-js that referenced this pull request Nov 27, 2022
Fixes #54

- [x] Wait for hildjj/dohdec#38 to be released to NPM.
- [x] Resolver requirements: Retrieve RRSig records and return `Buffer | Message`. Preferably `cd` flag on, otherwise validation will still fail but for the wrong reason.
- [x] Security audit.
@gnarea
Copy link
Contributor Author

gnarea commented Sep 1, 2024

Hi @hildjj 👋🏾 Can you please release this change to NPM? I've been relying on the Git tag but that's now breaking CI for me. TIA! 🙇🏾

@hildjj
Copy link
Owner

hildjj commented Sep 2, 2024

I'll look at this first thing Tuesday. I think this is blocked on hildjj/mock-tls-server#2, so I either have to fix that or rewrite the tests.

@hildjj
Copy link
Owner

hildjj commented Sep 3, 2024

OK, I think I'm ready to cut a major release, but I'm too tired today to re-learn lerna. Release probably will happen tomorrow morning MDT.

@hildjj
Copy link
Owner

hildjj commented Sep 5, 2024

Version 6.0.0 just released. Please check to make sure I got it right... I had to do a bunch of manual hacking to get the release to publish from GHA with provenance information correct.

gnarea added a commit to relaycorp/dnssec-js that referenced this pull request Sep 5, 2024
@gnarea
Copy link
Contributor Author

gnarea commented Sep 5, 2024

Thanks @hildjj! I tried upgrading, but it looks like the changes I made to pkg/dohdec/types/doh.d.ts in this PR didn't make it to NPM:

src/integration_tests/retrieval.test.ts:38:9 - error TS2345: Argument of type '{ rrtype: "A" | "NS" | "MD" | "MF" | "CNAME" | "SOA" | "MB" | "MG" | "MR" | "NULL" | "WKS" | "PTR" | "HINFO" | "MINFO" | "MX" | "TXT" | "RP" | "AFSDB" | "X25" | "ISDN" | "RT" | "NSAP" | ... 66 more ... | "DLV"; json: false; decode: false; dnssec: true; dnssecCheckingDisabled: boolean; }' is not assignable to parameter of type 'string | DOH_LookupOptions | undefined'.
  Object literal may only specify known properties, and 'dnssecCheckingDisabled' does not exist in type 'DOH_LookupOptions'.

38         dnssecCheckingDisabled: true,
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 1 error in src/integration_tests/retrieval.test.ts:38

@hildjj
Copy link
Owner

hildjj commented Sep 5, 2024

The .d.ts files are generated, and aren't even checked into git anymore. I'll see if I can find the right place to change the source.

@hildjj
Copy link
Owner

hildjj commented Sep 5, 2024

It looks like you only added it on getJSON(), and didn't get it passed all the way through the types when you use lookup().

WillFix.

hildjj added a commit that referenced this pull request Sep 5, 2024
hildjj added a commit that referenced this pull request Sep 6, 2024
kodiakhq bot pushed a commit to relaycorp/dnssec-js that referenced this pull request Sep 6, 2024
See: hildjj/dohdec#38

This replaces the reference to the Git tag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support option to disable DNSSEC verification
2 participants