Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

Realms #3854

Closed
wants to merge 9 commits into from
Closed

Realms #3854

Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion bin/proxy
Expand Up @@ -20,11 +20,12 @@ var addy = config.has('bind_to.host') ? config.get('bind_to.host') : "127.0.0.1"
forward.setTimeout(config.get('declaration_of_support_timeout_ms'));

const allowed = /^https:\/\/[a-zA-Z0-9\.\-_]+\/\.well-known\/browserid(\?.*)?$/;
const allowed_realm = /^https:\/\/[a-zA-Z0-9\.\-_:]+\/\.well-known\/browserid-realm$/;
const allowed_kpi = /^https:\/\/[a-zA-Z0-9\.\-_]+\/wsapi\/interaction_data(\?.*)?$/;

var server = http.createServer(function (req, res) {
var url = req.url;
if (!allowed.test(url) && !allowed_kpi.test(url) ) {
if (!allowed.test(url) && !allowed_realm.test(url) && !allowed_kpi.test(url) ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will this also require changes to our production proxy?

Choose a reason for hiding this comment

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

Possibly, @gene1wood or @jrgm?

Copy link
Member

Choose a reason for hiding this comment

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

@shane-tomlinson Will this change result in persona initiating outbound connections out to the internet to new destinations (destinations other than sites' well-known/.browserid files and identity bridge endpoints [yahoo, gmail, etc])?

Choose a reason for hiding this comment

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

@gene1wood - indeed, the new outbound request will search for realm files at <rp_location>/.well-known/browserid-realm

Copy link
Member

Choose a reason for hiding this comment

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

Proxy server support added here mozilla/identity-ops@e6e2931

When QA wants to see this code in stage they'll need to request that new proxy server AMIs be built with this new code in the process. (cc @jrgm)

Choose a reason for hiding this comment

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

Excellent, thanks @gene1wood

res.writeHead(400);
res.end('You can\'t get there from here');
return;
Expand Down
3 changes: 3 additions & 0 deletions example/rp/.well-known/browserid-realm
@@ -0,0 +1,3 @@
{
"realm": ["http://localhost:10001", "http://127.0.0.1:10001"]
}
2 changes: 1 addition & 1 deletion example/rp/index.html
Expand Up @@ -176,9 +176,9 @@ <h2>readiness</h2>
}
});
};

navigator.id.watch({
loggedInUser: (storage.loggedInUser === 'null') ? null : storage.loggedInUser,
realm: location.search ? location.search.match(/realm=([^&]+)/)[1] : undefined,
onready: function () {
loggit("onready");
var txt = serial++ + ' navigator.id ready at ' + (new Date).toString();
Expand Down
87 changes: 87 additions & 0 deletions lib/realm.js
@@ -0,0 +1,87 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

const fs = require('fs');

const request = require('./request');
const logger = require('./logging').logger;
const util = require('util');
const validate = require('./validate');

const WELL_KNOWN_PATH = '/.well-known/browserid-realm';

const SHIMMED_REALMS = {};

// Support for "shimmed realms" for local development.
// CSV values:
// <realm>|<browserid-realm filepath>,

Choose a reason for hiding this comment

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

This took me a while to figure out how to do from bash command line:

SHIMMED_REALMS=127.0.0.1\|example/rp/.well-known/browserid-realm npm start

Can you add a comment about how to make it so? This info should probably go into the front end wiki as well.

if (process.env.SHIMMED_REALMS) {
process.env.SHIMMED_REALMS.split(',').forEach(function(shim) {
var parts = shim.split('|');
SHIMMED_REALMS[parts[0]] = parts[1]; // realm name, filepath
logger.info("shimmed realm info for " + parts[0]);
});
}


function isJsonType(res) {
var contentType = res.headers['content-type'];
return contentType && contentType.indexOf('application/json') === 0;
}

function onRealmInfo(err, body, callback) {
if (err) {
// just pass err
} else if (!(body && body.realm)) {
err = new Error('missing realm json');

Choose a reason for hiding this comment

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

Would it make the code clearer to use these as guard clauses and immediately return:

if (err) return callback(err);
if (!(body && body.realm)) return callback(new Error('missing realm json'));
...

} else if (!util.isArray(body.realm)) {
err = new Error('realm property is not an array');
} else {
body.realm = body.realm.filter(function(value) {
try {
validate.is.origin(value);
return true;
} catch(err) {
return false;
}
});
}
callback(err, body);
}

function loadShimmedRealm(realm, callback) {
fs.readFile(SHIMMED_REALMS[realm], function(err, str) {
var body;
if (err) {
logger.debug(err); // io error?
} else {
try {
body = JSON.parse(str);
} catch (jsonErr) {
err = jsonErr;
}
}
onRealmInfo(err, body, callback);
});
}

exports.checkSupport = function realmSupport(realm, callback) {
if (SHIMMED_REALMS[realm]) {
loadShimmedRealm(realm, callback);
} else {
request('https://' + realm + WELL_KNOWN_PATH, { json: true }, function onRequest(err, res, body) {
if (err) {
logger.debug(err);
} else if (!isJsonType(res)) {

Choose a reason for hiding this comment

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

Same comment about guard clauses here. Secondly, if onRealmInfo just calls callback when there is error, can we just call callback directly from here?

// strict here so that file isn't added to site by mistake. admin
// must want this file to exist by explicitly setting headers
err = new Error("content-type was not application/json");
body = null;
}

onRealmInfo(err, body, callback);
});
}
};

29 changes: 29 additions & 0 deletions lib/request.js
@@ -0,0 +1,29 @@
const request = require('request');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i dearly wanted to fix up lib/primary.js to use request also, but it's off-topic of this feature.

Choose a reason for hiding this comment

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

Add an issue for it!

const config = require('./configuration');

const REQUEST_TIMEOUT = config.get('declaration_of_support_timeout_ms');
const HTTP_PROXY = config.has('http_proxy') ? config.get('http_proxy') : null;

module.exports = function browserid_request(url, options, callback) {
if (typeof options === 'function') {
callback = options;
options = {};
}

if (HTTP_PROXY) {
options.proxy = {
protocol: 'http:',
host: HTTP_PROXY.host,
port: HTTP_PROXY.port
};
}

if (!('timeout' in options)) {
options.timeout = REQUEST_TIMEOUT;
}

options.strictSSL = true;

Choose a reason for hiding this comment

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

Ah yes, it looks like declaration of support must be SSL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't have to be. This option is only used for HTTPS requests. But considering we pretty much always use an outgoing proxy, it's likely never actually needed. Still, doesn't hurt, and is safer.


request(url, options, callback);
};

6 changes: 6 additions & 0 deletions lib/validate.js
Expand Up @@ -18,6 +18,7 @@ httputils = require('./httputils.js'),
check = require('validator').check;

var hostnameRegex = /^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$|^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$/;
var hostRegex = /^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$|^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])(:\d{1,5})?$/;

Choose a reason for hiding this comment

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

Head explosion. What are the differences between this and hostnameRegex? Both allow IP addresses, both allow domain names. It looks like hostRegex allows a port, if that is so, can you add a comment?

Choose a reason for hiding this comment

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

That reminds me, is an RP able to specify the port the realm's support document is located on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I think about it, it doesn't really make sense, since https is required. I added this mostly to allow local testing, but now with shimmed realms, it's not needed. I'll just use the hostname instead.


var types = {
email: function(x) {
Expand Down Expand Up @@ -48,6 +49,9 @@ var types = {
hostname: function(x) {
check(x).is(hostnameRegex);
},
host: function(x) {
check(x).is(hostRegex);
},
origin: function(x) {
/* origin regex
/^ // beginning
Expand Down Expand Up @@ -143,3 +147,5 @@ module.exports = function (params) {
next();
};
};

module.exports.is = types;
28 changes: 28 additions & 0 deletions lib/wsapi/realm_info.js
@@ -0,0 +1,28 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
const realm = require('../realm');
const logger = require('../logging').logger;

exports.method = 'get';
exports.writes_db = false;
exports.authed = false;
exports.args = {
realm: 'host'
};
exports.i18n = false;

exports.process = function realm_info(req, res) {
var domain = req.params.realm.toLowerCase();
realm.checkSupport(domain, function onRealm(err, body) {

Choose a reason for hiding this comment

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

I'm not sure whether this comment is better here or in ../ream - it doesn't look like this information is cached, which could cause a heavy load on the realm host. Perhaps it is not needed at this point, but we should give it some thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, we could let squid handle caching this file, just as we do when requesting the IdP support document. possible @gene1wood ?

Copy link
Member

Choose a reason for hiding this comment

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

@seanmonstar The squid proxy server will respect whatever caching directives it receives from the target site in the HTTP headers that come with the /.well-known/browserid-realm document. If the target asserts long cache times, the squid proxy will respect that and not make an outbound request.

var json = {};
if (err) {
logger.info('"' + domain + '" realm support is misconfigured: ' + err);
json.realm = [];

Choose a reason for hiding this comment

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

IIUC, the intent here is to not block users from signing in to a site if the support document is misconfigured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct. i'll add a comment say as much

} else {
json.realm = body.realm;
}

res.json(json);
});
};