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 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f6e2b48
adding /wsapi/realm_info?realm=foo.com
seanmonstar Aug 29, 2013
2dda3ca
realm support in watch()
seanmonstar Aug 29, 2013
7d81daa
turns realm off by default on rp example
seanmonstar Sep 5, 2013
8c7b0d9
keep list of sites logged into with realm
seanmonstar Sep 5, 2013
a76701f
forcing browserid-realm files to be https
seanmonstar Sep 9, 2013
a38a69f
logout fix
seanmonstar Sep 11, 2013
527c4f5
dont use request
seanmonstar Sep 19, 2013
28152c7
be sure to JSON.parse if options.json
seanmonstar Sep 19, 2013
bc5c8a9
watch realm state
seanmonstar Sep 23, 2013
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
View file
Open in desktop
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(\?.*)?$/;
Copy link

Choose a reason for hiding this comment

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

do we need to allow the extra .* on the end? Does it give us anything?

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?

Copy link

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])?

Copy link

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)

Copy link

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
View file
Open in desktop
@@ -0,0 +1,3 @@
{
"realm": ["http://localhost:10001", "http://127.0.0.1:10001"]
}
1 change: 1 addition & 0 deletions example/rp/index.html
View file
Open in desktop
Expand Up @@ -179,6 +179,7 @@ <h2>readiness</h2>

navigator.id.watch({
loggedInUser: (storage.loggedInUser === 'null') ? null : storage.loggedInUser,
realm: 'http://localhost:10001',
Copy link

Choose a reason for hiding this comment

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

must a site specify a realm? If not, can we make this optional or create a new example page for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A site doesn't have to. I was trying to figure out how to make this optional. Perhaps if the page is loaded with ?realm=1 ?

onready: function () {
loggit("onready");
var txt = serial++ + ' navigator.id ready at ' + (new Date).toString();
Expand Down
43 changes: 43 additions & 0 deletions lib/realm.js
View file
Open in desktop
@@ -0,0 +1,43 @@
/* 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 request = require('./request');
const logger = require('./logging').logger;
const util = require('util');
const validate = require('./validate');

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

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

exports.checkSupport = function realmSupport(realm, callback) {
request(realm + WELL_KNOWN_PATH, { json: true }, function onRequest(err, res, body) {
Copy link

Choose a reason for hiding this comment

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

Must realms be defined at an https endpoint like /.well-known/browserid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, http and https are both valid. The protocol is required in the listed realm argument (wsapi/realm_info validates its a proper origin).

Copy link

Choose a reason for hiding this comment

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

I admit, this makes me uncomfortable - is there any way that defining a realm file over http can be used to hijack a user? @fmarier or @warner, can we get your opinions here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I could have missed something, but here's my reasoning as to why we should allow http as well.

First, we allow RPs to use Persona for login even over http. Their sites don't need to be secured with HTTPS. Of course, there's risk for the sites, that an attacker could adjust DNS to make the user log into the wrong site, or an attack could capture the assertion as the site POSTs it to their own server for verification, and login as the user. Still, we let the sites accept those risks, instead of demanding everyone who wants to use Persona needs to setup HTTPS.

Likewise, the realm is simply a group of sites that a user is logging in to. An attacker could hijack DNS to an http site, and declare a different realm than normal. Or they could serve up their own browserid-realm file, which could include a different list of sites. The user isn't automatically logged into any of those sites at that time. The user then needs to visit one of those sites, and that site (whether normally or by the attacker) would need to declare being part of the hijacked realm.

In the end, the worst that could happen is a user is logged into a realm or site that isn't really part of the realm.

Copy link
Member

Choose a reason for hiding this comment

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

@seanmonstar If we want https and http for the /.well-known/browserid-realm URL I'll need to tweak my proxy config change (I'd assumed this was https only like /.well-known/browserid

if (err) {
logger.debug(err);
} else if (!isJsonType(res)) {
// 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;
} else if (!(body && body.realm)) {
err = 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;
Copy link

Choose a reason for hiding this comment

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

Should there be some sort of error surfaced here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a filter on the array provided by a support document. If a value in the array isn't a proper origin, it gets dumped.

So, { realm: ['foo.com', 'http://bar.com', 'baz'] } becomes { realm: ['http://bar.com'] }. Our client side doesn't care if there's errors or if it's malformed, simply if the declared realm is in the support document.

Copy link

Choose a reason for hiding this comment

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

ok, that makes sense.

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

29 changes: 29 additions & 0 deletions lib/request.js
View file
Open in desktop
@@ -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.

Copy link

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;
Copy link

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);
};

2 changes: 2 additions & 0 deletions lib/validate.js
View file
Open in desktop
Expand Up @@ -143,3 +143,5 @@ module.exports = function (params) {
next();
};
};

module.exports.is = types;
28 changes: 28 additions & 0 deletions lib/wsapi/realm_info.js
View file
Open in desktop
@@ -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: 'origin'
};
exports.i18n = false;

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

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 = [];
Copy link

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);
});
};
67 changes: 59 additions & 8 deletions lockdown.json
View file
Open in desktop
Expand Up @@ -14,13 +14,22 @@
"ansi": {
"0.1.2": "2627e29498f06e2a1c2ece9c21e28fd494430827"
},
"asn1": {
"0.1.11": "559be18376d08a4ec4dbe80877d27818639b2df7"
},
"assert-plus": {
"0.1.2": "d93ffdbb67ac5507779be316a7d65146417beef8"
},
"async": {
"0.1.22": "0fc1aaa088a0e3ef0ebe2d8831bab0dcf8845061",
"0.2.9": "df63060fbf3d33286a76aaf6d55a2986d9ff8619"
},
"aws-lib": {
"0.0.5": "971b8995078d83c80f2372f134c496e71b293a46"
},
"aws-sign": {
"0.3.0": "3d81ca69b474b1e16518728b51c24ff0bbedc6e9"
},
"awsbox": {
"0.4.5": "027a9998955f09f3d6b0a10d38ea37ae381b6358"
},
Expand All @@ -33,6 +42,9 @@
"bindings": {
"1.0.0": "c3ccde60e9de6807c6f1aa4ef4843af29191c828"
},
"boom": {
Copy link

Choose a reason for hiding this comment

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

I like boom, can we get rid of our homegrown error code library and use it instead?

"0.4.2": "7a636e9ded4efcefb19cef4947a3c67dfaee911b"
},
"browserify": {
"1.13.5": "b5f0a160733779d27547885dfb598a65ef6fdaad"
},
Expand Down Expand Up @@ -63,6 +75,9 @@
"0.5.1": "7d0023eaeb154e8ee9fce75dcb923d0ed1667774",
"0.6.0-1": "6dbb68ceb8bc60f2b313dcc5ce1599f06d19e67a"
},
"combined-stream": {
"0.0.4": "2d1a43347dbe9515a4a2796732e5b88473840b22"
},
"commander": {
"1.1.1": "50d1651868ae60eccff0a2d9f34595376bc6b041"
},
Expand Down Expand Up @@ -96,13 +111,19 @@
"convict": {
"0.0.6": "960495df513baf3e5c20b970268fd86ff1ac79b2"
},
"cookie-jar": {
"0.3.0": "bc9a27d4e2b97e186cd57c9e2063cb99fa68cccc"
},
"cookies": {
"0.2.1": "fee635ef023704893ac30387899f3dc448a52840",
"0.3.6": "1b5e4bd66fc732ea2e8b5087a8fb3718b4ec8597"
},
"cover": {
"0.2.8": "91946a9806e992f98d374cecd642fce9b96f5237"
},
"cryptiles": {
"0.2.2": "ed91ff1f17ad13d3748288594f8a48a0d26f325c"
},
"crypto-browserify": {
"0.2.3": "c98141505d90e31a1e456cb97343dc3b0f4a1a2a"
},
Expand All @@ -115,6 +136,9 @@
"css-stringify": {
"1.0.5": "b0d042946db2953bb9d292900a6cb5f6d0122031"
},
"ctype": {
"0.5.2": "fe8091d468a373a0b0c9ff8bbfb3425c00973a1d"
},
"cycle": {
"1.0.2": "269aca6f1b8d2baeefc8ccbc888b459f322c4e60"
},
Expand All @@ -124,6 +148,9 @@
"debug": {
"0.7.2": "056692c86670977f115de82917918b8e8b9a10f0"
},
"delayed-stream": {
"0.0.5": "d4b1f43a93e8296dfe02694f4680bc37a313c73f"
},
"deputy": {
"0.0.4": "edc00a9ef5c53527c405328534c99795ada41cbf"
},
Expand Down Expand Up @@ -158,6 +185,12 @@
"eyes": {
"0.1.8": "62cf120234c683785d902348a800ef3e0cc20bc0"
},
"forever-agent": {
"0.5.0": "0c1647a74f3af12d76a07a99490ade7c7249c8f0"
},
"form-data": {
"0.1.1": "0d5f2805647b45533ba10bc8a59cf17d1efa5f12"
},
"fresh": {
"0.1.0": "03e4b0178424e4c2d5d19a54d8814cdc97934850"
},
Expand All @@ -173,27 +206,34 @@
},
"graceful-fs": {
"1.1.14": "07078db5f6377f6321fceaaedf497de124dc9465",
"1.2.1": "b35cc6e623576fc2a278cba12c00dda6a1758d2d",
"1.2.3": "15a4806a57547cb2d2dbf27f42e89a8c3451b364",
"2.0.0": "c9a206f6f5f4b94e1046dfaaccfe9e12d0ab8cef"
},
"hashish": {
"0.0.4": "6d60bc6ffaf711b6afd60e426d077988014e6554"
},
"hawk": {
"1.0.0": "b90bb169807285411da7ffcb8dd2598502d3b52d"
},
"hoek": {
"0.9.1": "3d322462badf07716ea7eb85baf88079cddce505"
},
"htmlparser": {
"1.7.6": "6a263c7ee5930f3e5c56fa564011f99e49f80d34"
},
"http-browserify": {
"0.1.1": "d9d82735a5f85f950761ac3909ba9485ec0af4f1"
},
"http-signature": {
"0.10.0": "1494e4f5000a83c0f11bcc12d6007c530cb99582"
},
"i18n-abide": {
"0.0.8": "a7610465be083a21e1c2f52e2fd78503ee90dd29"
},
"iconv-lite": {
"0.2.7": "45be2390d27af4b7613aac4ee4d957e3f4cbdb54"
},
"inherits": {
"1.0.0": "38e1975285bf1f7ba9c84da102bb12771322ac48",
"2.0.0": "76c81b3b1c10ddee3a60bf2c247162bc369f8ba8"
},
"irc": {
Expand All @@ -208,8 +248,10 @@
"jshint": {
"0.9.1": "ff32ec7f09f84001f7498eeafd63c9e4fbb2dc0e"
},
"json-stringify-safe": {
"5.0.0": "4c1f228b5050837eba9d21f50c2e6e320624566e"
},
"jsxgettext": {
"0.0.4": "7dc442c1102ba73edfaa7fd60703ab1d03234194",
"0.1.3": "db9dd0bedd531606cdd5fd978dd75d383fd99327"
},
"jwcrypto": {
Expand All @@ -229,10 +271,8 @@
"mailcomposer": {
"0.1.33": "69f6e924c55804f17c1bfea6c3f2f51049650820"
},
"mersenne": {
"0.0.3": "c39a3d45fee6091189ccd329107312ea8fe14d8a"
},
"mime": {
"1.2.11": "58203eed86e3a5ef17aed2b7d9ebd47f0a60dd10",
"1.2.6": "b1f86c768c025fa87b48075f1709f28aeaf20365",
"1.2.9": "009cd40867bd35de521b3b966f04e2f8d4d13d09"
},
Expand All @@ -249,7 +289,6 @@
"0.3.5": "de3e5f8961c88c787ee1368df849ac4413eca8d7"
},
"monocle": {
"0.1.46": "32ff9137fd3ee31d2d69a29db2f8a69389cd9860",
"0.1.50": "9a7cbd0ccc10de95fd78a04b9beb2482ae4940b7"
},
"mysql": {
Expand All @@ -264,6 +303,9 @@
"node-statsd": {
"0.0.2": "ba96c26d4ec22b4f9501bb332fdf740db5a40ee7"
},
"node-uuid": {
"1.4.1": "39aef510e5889a3dca9c895b506c73aae1bac048"
},
"nodemailer": {
"0.3.21": "d9637a1d585f36fd30362c3036ce0692656fe771"
},
Expand All @@ -280,6 +322,9 @@
"nub": {
"0.0.0": "b369bd32bdde66af59605c3b0520bc219dccc04f"
},
"oauth-sign": {
Copy link

Choose a reason for hiding this comment

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

eh?

"0.3.0": "cb540f93bb2b22a7d5941691a288d60e8ea9386e"
},
"optimist": {
"0.2.6": "c15b750c98274ea175d241b745edf4ddc88f177b",
"0.2.8": "e981ab7e268b457948593b55674c099a815cac31",
Expand Down Expand Up @@ -324,6 +369,7 @@
"1.1.1": "75c97c5446fa1146c1d250c47ca3629fb9a2e764"
},
"request": {
"2.27.0": "dfb1a224dd3a5a9bade4337012503d710e538668",
"2.9.203": "6c1711a5407fb94a114219563e44145bcbf4723a"
},
"resolve": {
Expand Down Expand Up @@ -352,8 +398,10 @@
"slide": {
"1.1.4": "2b23f1949b369ed61d22bd6570ff0320302fc8df"
},
"sntp": {
Copy link

Choose a reason for hiding this comment

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

eheh?

"0.2.4": "fb885f18b0f3aad189f824862536bceeec750900"
},
"source-map": {
"0.1.22": "425906162f81bf110552ccc9931dba079e9f1341",
"0.1.25": "5851545c1f4a40243829065c20e6f40b023fba1a"
},
"stack-trace": {
Expand All @@ -371,6 +419,9 @@
"traverse": {
"0.6.3": "a053ffa1b6179b9240ea16d74bfd604bd6b6e41b"
},
"tunnel-agent": {
Copy link

Choose a reason for hiding this comment

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

and finally, eheheh? Are these all required by 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.

yea they are... i can switch away from request, but its the most popular and tested library.

"0.3.0": "ad681b68f5321ad2827c4cfb1b7d5df2cfe942ee"
},
"ua-parser": {
"0.2.4": "c25ef577be95350d0fe1d3361597282da4e9ba71"
},
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -35,6 +35,7 @@
"mkdirp": "0.3.0",
"optimist": "0.2.8",
"postprocess": "0.2.4",
"request": "2.27.0",
"rimraf": "2.0.2",
"semver": "1.0.12",
"temp": "0.4.0",
Expand Down
19 changes: 19 additions & 0 deletions resources/static/common/js/network.js
View file
Open in desktop
Expand Up @@ -525,6 +525,25 @@ BrowserID.Network = (function() {
});
},

/**
* Get infornation about a browserid-realm.
* @method realmInfo
* @param {string} realm - Hostname of realm to check.
* @param {function} onComplete - Called with an object on success,
* contained these properties:
* realm: array of hostnames
* @param {function} onFailure - Called on XHR failure.
*/
realmInfo: function realmInfo(realm, onComplete, onFailure) {
Copy link

Choose a reason for hiding this comment

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

Is this tested via the tests for getSilentAssertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, getSilentAssertion calls User.isRealmValid, which calls network.realmInfo(realm).

get({
url: "/wsapi/realm_info?realm=" + encodeURIComponent(realm),
success: function realmInfo_sucess(data, textStatus, xhr) {
complete(onComplete, data);
},
error: onFailure
});
},

/**
* Remove an email address from the current user.
* @method removeEmail
Expand Down