Skip to content

Support fetch external resources over HTTP#1007

Merged
tlimoncelli merged 2 commits intoDNSControl:masterfrom
xddxdd:pr-fetch
Jan 6, 2021
Merged

Support fetch external resources over HTTP#1007
tlimoncelli merged 2 commits intoDNSControl:masterfrom
xddxdd:pr-fetch

Conversation

@xddxdd
Copy link
Copy Markdown
Contributor

@xddxdd xddxdd commented Dec 26, 2020

This PR adds Fetch API to dnscontrol, mentioned in issue #994, via this extension for otto https://github.com/deoxxa/ottoext.

In addition, a PANIC(reason) function is added to abort dnscontrol execution with an exit code, in case an error happened.

And a FETCH() wrapper function is added to automatically call PANIC() if a network error occurred.

Fixes #994

Example: successful request

dnscontrol.js:

var REG_NONE = NewRegistrar('none', 'NONE');
var DNS_BIND = NewDnsProvider('bind', 'BIND');

D('example.com', REG_NONE, DnsProvider(DNS_BIND), [
  A('@', '1.2.3.4'),
]);

console.log('Hello World');
FETCH('https://example.com').then(function(r) {
  return r.text();
}).then(function(t) {
  // Do something with response body
  D_EXTEND('example.com', [
    TXT('@', t.slice(0, 100)),
  ]);
});
console.log('Goodbye');

Generated zone:

$TTL 300
; generated with dnscontrol 2020-12-27T00:57:20+08:00
@                IN SOA   DEFAULT_NOT_SET. DEFAULT_NOT_SET. 2020122601 3600 600 604800 1440
                 IN A     1.2.3.4
                 IN TXT   "<!doctype html>\010<html>\010<head>\010    <title>Example Domain</title>\010\010    <meta charset=\"utf-8\" />\010    <m"
Example: failed request

dnscontrol.js:

var REG_NONE = NewRegistrar('none', 'NONE');
var DNS_BIND = NewDnsProvider('bind', 'BIND');

D('example.com', REG_NONE, DnsProvider(DNS_BIND), [
  A('@', '1.2.3.4'),
]);

console.log('Hello World');
// Example of failure: invalid URL
FETCH('httttttps://example.com').then(function(r) {
  return r.text();
}).then(function(t) {
  // Do something with response body
  D_EXTEND('example.com', [
    TXT('@', t.slice(0, 100)),
  ]);
});
console.log('Goodbye');

Dnscontrol output:

Hello World
Goodbye
Error: Get "httttttps://example.com": unsupported protocol scheme "httttttps"

@xddxdd
Copy link
Copy Markdown
Contributor Author

xddxdd commented Dec 26, 2020

I'm unsure if I should add unit tests for the fetch functionality, since I didn't find individual tests for basic functions like A(). Please let me know if a unit test for this is necessary.

Copy link
Copy Markdown
Contributor

@tlimoncelli tlimoncelli left a comment

Choose a reason for hiding this comment

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

A. It is fabulous that you can add such a big feature in so few lines of code. Good job!

I have 2 concerns:

  1. There have been no updates to fknsrs.biz/p/ottoext in years. Is it abandoned or just very stable?
  2. This will encourage people to make dnsconfig.js configs that break when a third-party dependency is broken or down. I think that the docs should include a warning against complexity.

B. Please add docs (docs/_functions/global/FETCH.md)

C. Unit tests would be great:

I'm unsure if I should add unit tests for the fetch functionality, since I didn't find individual tests for basic functions like A(). Please let me know if a unit test for this is necessary.

My knowledge of Javascript is limited. If you can help me make unit tests, I would appreciate it!

Comment thread pkg/js/js.go
currentDirectory = filepath.Dir(file)

vm := otto.New()
l := loop.New(vm)
Copy link
Copy Markdown
Contributor

@tlimoncelli tlimoncelli Dec 28, 2020

Choose a reason for hiding this comment

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

Please add comments explaining these timers.

@xddxdd xddxdd force-pushed the pr-fetch branch 2 times, most recently from 9d592d8 to 8b30a48 Compare January 1, 2021 07:49
@xddxdd
Copy link
Copy Markdown
Contributor Author

xddxdd commented Jan 1, 2021

There have been no updates to fknsrs.biz/p/ottoext in years. Is it abandoned or just very stable?

The project seems abandoned, but from the testing the functionalities are stable. Nevertheless I can take over and maintain it.

For example, I just forked that project and added the functionality to specify request headers, and updated this PR to point to my fork. Please let me know if it's acceptable to you.

Please add docs (docs/_functions/global/FETCH.md)

I've added docs for the two functions I added, FETCH and PANIC.

This will encourage people to make dnsconfig.js configs that break when a third-party dependency is broken or down. I think that the docs should include a warning against complexity.

A warning is included in the docs for FETCH function.

Unit tests would be great:

On a second thought, the ottoext project already includes its own unit tests, so I doubt if adding separate unit tests for FETCH is necessary. If needed, I can reuse the unit tests from ottoext.

@tlimoncelli
Copy link
Copy Markdown
Contributor

Yes, using your fork is good.

I don't see the new docs. Did you "git add" them? (or maybe it's a problem on my side?)

@xddxdd
Copy link
Copy Markdown
Contributor Author

xddxdd commented Jan 4, 2021

Oh wait, I forgot to push the doc changes to remote.

Should be fixed now.

Comment thread pkg/js/js.go Outdated
@xddxdd
Copy link
Copy Markdown
Contributor Author

xddxdd commented Jan 4, 2021

Speaking of security, I just realized that a switch is needed to explicitly enable this functionality, along with some additional warnings.

Or, in the case where DnsControl runs in a CI, based on data from a git repo that accepts and tests PRs, I can easily turn the CI into a DDoS machine with some while true do fetch(victim) thing.

@xddxdd xddxdd changed the title Support fetch external resources over HTTP [WIP] Support fetch external resources over HTTP Jan 4, 2021
@tlimoncelli
Copy link
Copy Markdown
Contributor

Good point. It would be good to enable this only with a flag.

add PANIC() and error-handled FETCH()
@xddxdd
Copy link
Copy Markdown
Contributor Author

xddxdd commented Jan 5, 2021

A flag --allow-fetch is added to enable fetch. Otherwise dnscontrol will exit with the following error if fetch is used:

executing dnsconfig.js: ReferenceError: 'fetch' is not defined

@xddxdd xddxdd changed the title [WIP] Support fetch external resources over HTTP Support fetch external resources over HTTP Jan 5, 2021
Copy link
Copy Markdown
Contributor

@tlimoncelli tlimoncelli left a comment

Choose a reason for hiding this comment

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

LGTM. Ready for merge?

@xddxdd
Copy link
Copy Markdown
Contributor Author

xddxdd commented Jan 6, 2021

Sure, good to go.

@tlimoncelli tlimoncelli merged commit 6efedd6 into DNSControl:master Jan 6, 2021
@tlimoncelli
Copy link
Copy Markdown
Contributor

Thanks!

P.S. I'd love to hear about how people use this feature!

@patschi
Copy link
Copy Markdown
Contributor

patschi commented Jan 8, 2021

Sorry for being late to the party: A usecase I can think of is querying the DKIM public key from my mailserver setup (using mailcow) from the API and add the records to the DNS zone. However this requires authenticating on the API via POST parameters, but when I see it correct the current fetch() implementation isn't capable of POST'ing values on the same request?

@xddxdd
Copy link
Copy Markdown
Contributor Author

xddxdd commented Jan 9, 2021

@patschi Technically POSTing parameters is supported, but for now you'll have to handcraft the request body. For example, both snippers below POST "a=12345" and "b=54321".

This is the easier way of form-urlencoded, useful to pass a pair of API key/secret along:

FETCH('http://localhost/test.php', {
  headers: {
    "Content-Type": "application/x-www-form-urlencoded",
  },
  method: "POST",
  body: 'a=12345&b=54321',
}).then(function(r) {
  return r.text();
}).then(function(t) {
  console.log(t);
});

This is the more complicated way of creating a multipart form-data.

data = '--------------------------2c3e2cab183d59b3' + "\n"
     + 'Content-Disposition: form-data; name="a"' + "\n"
     + '' + "\n"
     + '12345' + "\n"
     + '--------------------------2c3e2cab183d59b3' + "\n"
     + 'Content-Disposition: form-data; name="b"' + "\n"
     + '' + "\n"
     + '54321' + "\n"
     + '--------------------------2c3e2cab183d59b3--' + "\n";

FETCH('http://localhost/test.php', {
  headers: {
    "Content-Type": "multipart/form-data; boundary=------------------------2c3e2cab183d59b3",
  },
  method: "POST",
  body: data,
}).then(function(r) {
  return r.text();
}).then(function(t) {
  console.log(t);
});

@xddxdd xddxdd deleted the pr-fetch branch November 14, 2022 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Idea: support fetch external resources over HTTP

3 participants