Avoid DNS TXT record resolutions for hostnames with pseudo-TLDs. #447

isislovecruft · 2015-05-30T23:27:59Z

ADD function hasPseudoTLD() to shell/server/pre-meteor.js which
checks if an hostname has any of the major pseudo-TLDs.
CHANGE Meteor.startup() function in shell/server/pre-meteor.js to
avoid attempting to resolve DNS TXT records for requests with
hostnames which have a pseudo-TLD.

* ADD function `hasPseudoTLD()` to shell/server/pre-meteor.js which checks if an hostname has any of the major pseudo-TLDs. * CHANGE `Meteor.startup()` function in shell/server/pre-meteor.js to avoid attempting to resolve DNS TXT records for requests with hostnames which have a pseudo-TLD.

garply · 2015-05-30T23:28:00Z

Hey admins! Can one of you let me know if it's safe to build this pull request? See https://alpha.sandstorm.io/grain/kB7bofATTL2jGuKSFwmqdA/GithubPRCommands for a list of commands.

isislovecruft · 2015-05-30T23:34:54Z

I forgot to mention that this fixes part of issue #434.

paulproteus · 2015-05-31T00:07:13Z

Garply, ok to test

paulproteus · 2015-05-31T00:19:56Z

shell/server/pre-meteor.js

@@ -43,6 +43,12 @@ function isSandstormShell(hostname) {
  return (hostname === HOSTNAME || (DDP_HOSTNAME && hostname === DDP_HOSTNAME));
 }

+function hasPseudoTLD(hostname) {
+  // Returns true if this hostname ends in one of the major pseudo-TLDs.
+  let tld = hostname.split(".").pop();


I think this should be var tld.

Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let it looks like let is an ECMAScript 6 feature, which I wish we could use yet, but I guess we can't yet.

@jparyani and @kentonv and others who might care, I'd be curious to hear if you think we should consider using https://github.com/mquandalle/meteor-harmony .

It looks like we can opt in on a file-by-file basis, by choosing a filename ending in .next.js, which seems straightforward.

isislovecruft · 2015-05-31T00:23:02Z

Hrm, it seems to be mad at the let tld = […] syntax. I can't use let? Should I change that to var?

paulproteus · 2015-05-31T00:23:33Z

shell/server/pre-meteor.js

+function hasPseudoTLD(hostname) {
+  // Returns true if this hostname ends in one of the major pseudo-TLDs.
+  let tld = hostname.split(".").pop();
+  return (tld === ("onion" || "i2p" || "gnu"));


In my testing, this seems to use short-circuit evaluation of ||.

I think this would be cleaner if the function has a list called

var pseudoTlds = ['onion', 'i2p', 'gnu'];

and then you check for membership by e.g.

pseudoTlds.indexOf(tld) !== -1

I forget how to do stuff like this all the time in Javascript, so here is my interactive session where I verified the above:

➜ ~ js > 'x' || 'y' 'x' > var x = ['a', 'b']; undefined > x.indexOf('a'); 0 > x.indexOf('z'); -1

Note that elsewhere in the codebase, we seem to use _.contains(array, member), based on this grep:

$ git grep _.contains shell/packages/sandstorm-accounts-oauth/oauth_server.js: if (!_.contains(Accounts.oauth.serviceNames(), result.serviceName)) { shell/packages/sandstorm-accounts-packages/accounts.js: if (_.contains(services, serviceName)) { shell/packages/sandstorm-accounts-packages/accounts.js: if (!_.contains(services, serviceName)) { shell/packages/sandstorm-accounts-ui/accounts_ui.js: if (!_.contains(VALID_KEYS, key)) shell/packages/sandstorm-accounts-ui/accounts_ui.js: if (_.contains([ shell/packages/sandstorm-accounts-ui/login_buttons_dropdown.js: return _.contains( shell/packages/sandstorm-accounts-ui/login_buttons_session.js: if (!_.contains(VALID_KEYS, key)) shell/packages/sandstorm-accounts-ui/login_buttons_session.js: if (_.contains(['errorMessage', 'infoMessage'], key)) shell/packages/sandstorm-accounts-ui/login_buttons_single.js: if (_.contains(_.pluck(getLoginServices(), "name"), attemptInfo.type)) shell/server/proxy.js: !(_.contains(["GET", "HEAD", "POST", "PUT", "DELETE"], requestedMethod))) {

We do also use indexOf based on a similar grep, so if you have no preference, I would pick the JS native indexOf.

Perhaps it's a nitpick, but I was concerned that using indexOf() would return true for an hostname like storm.onion.example.com. The same should be true for _.contains.

Oh, interesting, I see now.

Your way is almost fine, but I think the short-circuit evaluation is going to be bad news; I think you're ||-ing the TLD names, not ||-ing the result of comparing your desired string against the TLD names.

https://epeli.github.io/underscore.string/#endswith-string-ends-position-gt-boolean supposedly exists.

I don't know if you're familiar with the Sandstorm dev flow, so here's how I am checking how we'd use that:

$ cd ~/projects/sandstorm/shell $ ./run-dev.sh # lots of printouts while stuff compiles # slow last line while it fuzz tests capnp I couldn't find an initscript for Sandstorm. Please pass the directory where Sandstorm is installed as an argument to this script. $ ➜ ./run-dev.sh /opt/sandstorm Please shut down your Sandstorm front-end: sudo /opt/sandstorm/sandstorm stop-fe $ sudo /opt/sandstorm/sandstorm stop-fe $ ./run-dev.sh /opt/sandstorm # more printout => App running at: http://local.sandstorm.io:6080

In a different terminal:

$ cd ~/projects/sandstorm/shell $ meteor shell # some printout > _.endsWith('a') TypeError: Object function (obj) { if (obj instanceof _) return obj; if (!(this instanceof _)) return new _(obj); this._wrapped = obj; } has no method 'endsWith' at repl:1:4 at /home/paulproteus/projects/sandstorm/shell/.meteor/local/build/programs/server/shell-server.js:243:23

OK, so I guess we have to do something like

meteor add underscorestring:underscore.string

per https://github.com/epeli/underscore.string . That seems like a lot of work for one endsWith() function, but it could be a reasonable idea.

Just to demonstrate the above more clearly:

$ js > ('x' === ('y' || 'x')) false >

@paulproteus Yeah, sorry, you're totally right about the short-circuit evaluation on the || operator… ffffuuuuuu, I hate Javascript. I've added a patch to this branch which changes this function to use a mix between your suggested way and the original patch:

var tld = hostname.split(".").pop(); var pseudoTLDs = ["onion", "i2p", "gnu"]; return (_.contains(pseudoTLDs, tld));

paulproteus · 2015-05-31T00:26:23Z

This code looks great, modulo the concerns that the autobuild found (which I commented on).

Also hi @isislovecruft ! Great to see you here. I'm that Asheesh person, in case you forgot the nick<->name mapping.

I'll be really excited if Sandstorm ends up useful for Tor. Perhaps I'll see you at Debconf or CCC camp this year.

paulproteus · 2015-05-31T00:34:53Z

shell/server/pre-meteor.js

+      if (hasPseudoTLD(hostname)) {
+        return;
+      } else {
+        publicIdPromise = lookupPublicIdFromDns(hostname);


I have a feeling that the logic here, where for certain "TLDs" we carefully do not do the DNS lookup, would be better in the lookupPublicIdFromDns function, though @kentonv or @jparyani might have another opinion.

It seems to me that it's the lookupPublicIdFromDns function that is the one whose actions are being adjusted.

Yes, I originally put the changes in lookupPublicIdFromDns() and returned a new Error there. However, I changed it because I didn't want the error handling in Meteor.start() to do something like return HTTP 500 with a message like "Hey, Sandstorm server here, and we're responding to tell you that we're pretending not to know what blahblahblahblahblah.onion is!"

I agree that the logic ought to be inside lookupPublicIdFromDns.

I'm not sure I understand what you mean about the error message. I think the goal here should be to produce the same error page that you saw before, but without doing any DNS query. If we want to also change the error page to leak less information, that seems like a separate issue from the DNS lookup.

@paulproteus

* CHANGE `let tld = […]` to `var tld = […]`. * CHANGE `hasPseudoTLD()` function to take @paulproteus' advice into account, specifically, use UnderscoreJS `_.contains()` method to test for pseudo-TLDs.

isislovecruft · 2015-05-31T02:50:44Z

shell/server/pre-meteor.js

+      // returning any result which might indicate that there's actually a
+      // server listening here.
+      if (hasPseudoTLD(hostname)) {
+        return;


I'm not sure if this is correct here… would Meteor prefer getting a new Promise that eventually returns null/undefined?

Unfortunately, returning here will cause no response to ever be sent for this HTTP request, so the user will be left hanging.

You probably want to do something like:

publicIdPromise = Promise.reject(new Error("Pseudo-TLDs have no DNS records."))

Here's an article about ES6 promises, if you want to learn more about them:

http://www.html5rocks.com/en/tutorials/es6/promises/

paulproteus · 2015-09-15T00:24:01Z

Hi @isislovecruft !

Is this still something you're interested in seeing? There's now some merge conflicts, which might make it tough to revive this pull request.

If I don't hear from you by a week from now then I might close this for now, but I'd be very happy to see this pull request get revived.

If you want to see this get merged, but don't have time to finish it yourself, then let me know and I'll see what I can do. No promises about timeline but I like supporting y'all's needs.

kentonv · 2015-09-18T23:32:58Z

If we close this we should file a bug in its place. This is something we can and should fix when someone gets a free moment.

zenhack · 2020-04-15T23:03:41Z

At this point the code this touches has been completely re-written. The issue still exists, but we'd need to make the change in gateway-router.js now. I think closing makes sense.

Re: opening a bug, is that not just #434?

ocdtrekkie · 2020-04-15T23:13:44Z

Since Asheesh said he'd probably close this if there was no response in a week, and that was in 2015, I will close this.

isislovecruft mentioned this pull request May 30, 2015

Sandstorm tries to resolve DNS for Tor .onion domains #434

Open

paulproteus reviewed May 31, 2015
View reviewed changes

Change let → var and use _.contains() in hasPseudoTLD().

70fe8f6

* CHANGE `let tld = […]` to `var tld = […]`. * CHANGE `hasPseudoTLD()` function to take @paulproteus' advice into account, specifically, use UnderscoreJS `_.contains()` method to test for pseudo-TLDs.

isislovecruft reviewed May 31, 2015
View reviewed changes

paulproteus mentioned this pull request Nov 4, 2015

option to make Sandstorm accessible through Tor with a .onion address iflowfor8hours/sandcastle#14

Open

ocdtrekkie added the install-config Installation/configuration issues label Apr 1, 2020

ocdtrekkie closed this Apr 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid DNS TXT record resolutions for hostnames with pseudo-TLDs. #447

Avoid DNS TXT record resolutions for hostnames with pseudo-TLDs. #447

isislovecruft commented May 30, 2015

garply commented May 30, 2015

isislovecruft commented May 30, 2015

paulproteus commented May 31, 2015

paulproteus May 31, 2015

paulproteus May 31, 2015

paulproteus May 31, 2015

isislovecruft commented May 31, 2015

paulproteus May 31, 2015

paulproteus May 31, 2015

paulproteus May 31, 2015

isislovecruft May 31, 2015

paulproteus May 31, 2015

paulproteus May 31, 2015

isislovecruft May 31, 2015

paulproteus commented May 31, 2015

paulproteus May 31, 2015

isislovecruft May 31, 2015

kentonv May 31, 2015

isislovecruft May 31, 2015

kentonv May 31, 2015

paulproteus commented Sep 15, 2015

kentonv commented Sep 18, 2015

zenhack commented Apr 15, 2020

ocdtrekkie commented Apr 15, 2020

Avoid DNS TXT record resolutions for hostnames with pseudo-TLDs. #447

Avoid DNS TXT record resolutions for hostnames with pseudo-TLDs. #447

Conversation

isislovecruft commented May 30, 2015

garply commented May 30, 2015

isislovecruft commented May 30, 2015

paulproteus commented May 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isislovecruft commented May 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulproteus commented May 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulproteus commented Sep 15, 2015

kentonv commented Sep 18, 2015

zenhack commented Apr 15, 2020

ocdtrekkie commented Apr 15, 2020