Added a new file "thalicryptomanager.js" which has an API to obtain the #41

Closed
wants to merge 32 commits into
from

Conversation

Projects
None yet
4 participants
@srichal
Contributor

srichal commented Jul 21, 2015

public-key-hash value. Also added new functionality to the "ThaliReplicationManager.prototype.start" function where the public-key-hash is obtained for the device and it is added to the DB if needed.

if (file) {
fs.unlinkSync(file);
}
fs.rmdirSync(fileLocation);
cb();

Normally, btw, using sync function is bad but in this case we are running a test framework which is inherently serial anyway, so it's o.k.

   

@srichal srichal added the in progress label Jul 21, 2015

thali/thalicryptomanager.js
+ Mobile.GetDocumentsPath(function (err, fileLocation) {
+ if (err) {
+ console.error("GetDocumentsPath err: ", err);
+ callback(null);

This comment has been minimized.

@yaronyg

yaronyg Jul 21, 2015

Member

We should follow the usual node.js convention of returning an error in the first argument and success information in the second argument. So we should be returning an error string in the first argument here not null.

@yaronyg

yaronyg Jul 21, 2015

Member

We should follow the usual node.js convention of returning an error in the first argument and success information in the second argument. So we should be returning an error string in the first argument here not null.

This comment has been minimized.

@mattpodwysocki

mattpodwysocki Jul 22, 2015

Contributor

Correct, it must be in the form of cb(err, ...args) where the error is always first even if null/undefined, meaning cb() is perfectly valid if there are no errors and no return value.

@mattpodwysocki

mattpodwysocki Jul 22, 2015

Contributor

Correct, it must be in the form of cb(err, ...args) where the error is always first even if null/undefined, meaning cb() is perfectly valid if there are no errors and no return value.

This comment has been minimized.

@srichal

srichal Jul 23, 2015

Contributor

Fixed as per comments.

@srichal

srichal Jul 23, 2015

Contributor

Fixed as per comments.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalicryptomanager.js
+ console.error("GetDocumentsPath err: ", err);
+ callback(null);
+ } else {
+ var file = fileLocation + pkcs12FileName;

This comment has been minimized.

@yaronyg

yaronyg Jul 21, 2015

Member

Typically just concatenating paths isn't safe across OS's. I think Node.js has a function somewhere in FS to handle safe file concatenation. Can you please find it and use it?

@yaronyg

yaronyg Jul 21, 2015

Member

Typically just concatenating paths isn't safe across OS's. I think Node.js has a function somewhere in FS to handle safe file concatenation. Can you please find it and use it?

This comment has been minimized.

This comment has been minimized.

@srichal

srichal Jul 23, 2015

Contributor

Fixed as per comments.

@srichal

srichal Jul 23, 2015

Contributor

Fixed as per comments.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalicryptomanager.js
+ fs.readFile(file, function (err, pkcs12Content) {
+ if (err) {
+ console.log('failed to read pkcs12 file - err: ', err);
+ callback(null);

This comment has been minimized.

@yaronyg

yaronyg Jul 21, 2015

Member

Same callback comment as above and again in line 42 and so on.

@yaronyg

yaronyg Jul 21, 2015

Member

Same callback comment as above and again in line 42 and so on.

This comment has been minimized.

@srichal

srichal Jul 23, 2015

Contributor

Fixed.

@srichal

srichal Jul 23, 2015

Contributor

Fixed.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalicryptomanager.js
+ console.log('successfully saved pkcs12Content');
+ // extract the public key
+ console.log('extracting publicKey from pkcs12Content');
+ var publicKey = crypto.pkcs12.extractPublicKey(password, pkcs12Content);

This comment has been minimized.

@yaronyg

yaronyg Jul 21, 2015

Member

I would restructure this code so there is one function that checks if the file exists and if not creates it and if so returns it. So in either case the function will return the file contents. Then I would have code that takes the returned value and gets the public key and generates the hash. that way we wouldn't have identical code in two places like we have in 39-46 and 65-72 and I think the code would be easier to read.

@yaronyg

yaronyg Jul 21, 2015

Member

I would restructure this code so there is one function that checks if the file exists and if not creates it and if so returns it. So in either case the function will return the file contents. Then I would have code that takes the returned value and gets the public key and generates the hash. that way we wouldn't have identical code in two places like we have in 39-46 and 65-72 and I think the code would be easier to read.

This comment has been minimized.

@srichal

srichal Jul 23, 2015

Contributor

Fixed as suggested.

@srichal

srichal Jul 23, 2015

Contributor

Fixed as suggested.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalicryptomanager.js
+ hash.update(publicKey); //already encoded to 'base64'
+ var fullSizeKeyHash = hash.digest('base64');
+ var slicedKeyIndex = fullSizeKeyHash.slice(0, hashSizeInBytes);
+ var keyIndexByteArray = new Buffer(slicedKeyIndex);

This comment has been minimized.

@yaronyg

yaronyg Jul 21, 2015

Member

Why did you go through all the trouble to create a Buffer when you end up only using the value as a string? In fact, when would it ever be used as a buffer?

@yaronyg

yaronyg Jul 21, 2015

Member

Why did you go through all the trouble to create a Buffer when you end up only using the value as a string? In fact, when would it ever be used as a buffer?

This comment has been minimized.

@srichal

srichal Jul 23, 2015

Contributor

Fixed by returning a string.

@srichal

srichal Jul 23, 2015

Contributor

Fixed by returning a string.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalicryptomanager.js
+var path = require('path');
+
+var pkcs12FileName = '/pkcs12.pfx';
+var password = 'password';

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

Please put in a comment specifying that we use a bogus password because using a password at all is bogus. Anyone who can get to the file can get to the app and thus can get the password. So we just include something because the API expects it. If and when mobile devices start having secure key stores that are tagged to app IDs then we can discuss using a real password.

@yaronyg

yaronyg Jul 23, 2015

Member

Please put in a comment specifying that we use a bogus password because using a password at all is bogus. Anyone who can get to the file can get to the app and thus can get the password. So we just include something because the API expects it. If and when mobile devices start having secure key stores that are tagged to app IDs then we can discuss using a real password.

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

I'll go file an issue that we really should be using the key stores on both iOS and Android. But keep in mind that this only provides a nominal security benefit since no secure hardware is being used. Instead what it does it store the keys in an encrypted file based off the device's key. This does provide some (tiny) level of protection for unencrypted devices. For devices that encrypt their drives it provides no meaningful protection. As such I think adding in support for local keystores is low priority.

@yaronyg

yaronyg Jul 23, 2015

Member

I'll go file an issue that we really should be using the key stores on both iOS and Android. But keep in mind that this only provides a nominal security benefit since no secure hardware is being used. Instead what it does it store the keys in an encrypted file based off the device's key. This does provide some (tiny) level of protection for unencrypted devices. For devices that encrypt their drives it provides no meaningful protection. As such I think adding in support for local keystores is low priority.

This comment has been minimized.

@srichal

srichal Jul 28, 2015

Contributor

Added the comment.

@srichal

srichal Jul 28, 2015

Contributor

Added the comment.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

I don't see a comment.

@yaronyg

yaronyg Jul 30, 2015

Member

I don't see a comment.

This comment has been minimized.

@srichal

srichal Aug 4, 2015

Contributor

The comment was added in a different place. Now moved it here.

@srichal

srichal Aug 4, 2015

Contributor

The comment was added in a different place. Now moved it here.

This comment has been minimized.

@yaronyg

yaronyg Aug 4, 2015

Member

👍

thali/thalicryptomanager.js
+module.exports = {
+
+/**
+* Creates a PKCS12 content, saves it to a file, extracts the public

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

This description is not correct. It would be better to use a correct description which is that this function looks for a pkcs12 file in a known location and will create it if it doesn't exist and then extract the public key and return its SHA256 hash.

@yaronyg

yaronyg Jul 23, 2015

Member

This description is not correct. It would be better to use a correct description which is that this function looks for a pkcs12 file in a known location and will create it if it doesn't exist and then extract the public key and return its SHA256 hash.

This comment has been minimized.

@srichal

srichal Jul 28, 2015

Contributor

Gave a correct/complete description.

@srichal

srichal Jul 28, 2015

Contributor

Gave a correct/complete description.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalicryptomanager.js
+ console.error('failed to extract public key');
+ callback('failed to extract publicKey');
+ }
+ console.log('generating SHA256 hash value');

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

Is this really necessary in the log? Please review your console.log statements and ask yourself "Will anyone benefit by adding all of this to the log?" The more stuff is in the log the harder it is to use for much of anything. We already have a work item to add some kind of log leveling but we aren't there yet.

@yaronyg

yaronyg Jul 23, 2015

Member

Is this really necessary in the log? Please review your console.log statements and ask yourself "Will anyone benefit by adding all of this to the log?" The more stuff is in the log the harder it is to use for much of anything. We already have a work item to add some kind of log leveling but we aren't there yet.

This comment has been minimized.

@srichal

srichal Jul 28, 2015

Contributor

Removed all the unnecessary log statements.

@srichal

srichal Jul 28, 2015

Contributor

Removed all the unnecessary log statements.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalicryptomanager.js
+
+/**
+* Creates a PKCS12 content, saves it to a file, extracts the public
+* key and returns it's SHA256 hash value.

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

Why doesn't the JSDoc specify the callback? See http://usejsdoc.org/tags-callback.html

@yaronyg

yaronyg Jul 23, 2015

Member

Why doesn't the JSDoc specify the callback? See http://usejsdoc.org/tags-callback.html

This comment has been minimized.

@srichal

srichal Jul 29, 2015

Contributor

Added the callback description here and other functions as necessary.

@srichal

srichal Jul 29, 2015

Contributor

Added the callback description here and other functions as necessary.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalireplicationmanager.js
@@ -38,36 +39,70 @@ ThaliReplicationManager.events = {
};
/**
-* Starts the Thali replication manager with the given device name and port number
-* @param {String} deviceName the device name to advertise.
+* Starts the Thali replication manager with the given port number and db name
* @param {Number} port the port number used for synchronization.

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

Where is the jsdoc for deviceName?

@yaronyg

yaronyg Jul 23, 2015

Member

Where is the jsdoc for deviceName?

This comment has been minimized.

@srichal

srichal Jul 29, 2015

Contributor

The deviceName parameter is not being passed to the thalireplicationmanager's start function anymore. The device-name/device-identity is already available in the thalireplicationmanager.

@srichal

srichal Jul 29, 2015

Contributor

The deviceName parameter is not being passed to the thalireplicationmanager's start function anymore. The device-name/device-identity is already available in the thalireplicationmanager.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalireplicationmanager.js
@@ -6,6 +6,7 @@ var inherits = require('util').inherits;
var net = require('net');
var multiplex = require('multiplex');
var validations = require('./validations');
+var addressPrefix = 'addressbook-'

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

Why is addressPrefix in the thalireplicationmanager? That is something that is specific to the postcardapp. It is not generic to Thali at all. It shouldn't be Thali_CordovaPlugin, it should be in postcardapp.

@yaronyg

yaronyg Jul 23, 2015

Member

Why is addressPrefix in the thalireplicationmanager? That is something that is specific to the postcardapp. It is not generic to Thali at all. It shouldn't be Thali_CordovaPlugin, it should be in postcardapp.

This comment has been minimized.

@srichal

srichal Jul 29, 2015

Contributor

Fixed as part of the redesign where the thalireplicationmanager does not add anything o the DB.

@srichal

srichal Jul 29, 2015

Contributor

Fixed as part of the redesign where the thalireplicationmanager does not add anything o the DB.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalireplicationmanager.js
+ return;
+ } else {
+ console.log('got a valid publicKeyHash');
+ currentAddrEntry = addressPrefix + publicKeyHash;

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

This should not be here per my previous comment. I think it makes sense for the cryptomanager to be here. I think it makes sense for the cryptomanager to be part of start up (after all, you can't do replication with an identity). I even think it makes sense for the cryptomanager to get the publickeyhash. But that is where it ends. That publickeyhash should be available as a property identifying the current device but that's it. It should be up to the postcardapp to then get that publickeyhash and use it for something.

Please propose a design for how we can expose the publickeyhash all the way to the postcardapp in a manner that makes logical sense.

@yaronyg

yaronyg Jul 23, 2015

Member

This should not be here per my previous comment. I think it makes sense for the cryptomanager to be here. I think it makes sense for the cryptomanager to be part of start up (after all, you can't do replication with an identity). I even think it makes sense for the cryptomanager to get the publickeyhash. But that is where it ends. That publickeyhash should be available as a property identifying the current device but that's it. It should be up to the postcardapp to then get that publickeyhash and use it for something.

Please propose a design for how we can expose the publickeyhash all the way to the postcardapp in a manner that makes logical sense.

This comment has been minimized.

@srichal

srichal Jul 29, 2015

Contributor

Completed the redesign as proposed.

@srichal

srichal Jul 29, 2015

Contributor

Completed the redesign as proposed.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalireplicationmanager.js
+ attachments: true
+ }).then(function (result) {
+ var currentAdddrFound = false;
+ result.rows.forEach(function(element) {

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

Again, this is postcardapp specific. It's not generic to Thali. I do believe that eventually Thali should have a central address book store and when we work on ACLs and identity exchange we will add one. But for now the thali replication manager can only handle one DB at a time so we agreed to put the addressbook entries in it but that should be an app level (e.g. postcardapp) consideration, not a Thali_CordovaPlugin consideration.

@yaronyg

yaronyg Jul 23, 2015

Member

Again, this is postcardapp specific. It's not generic to Thali. I do believe that eventually Thali should have a central address book store and when we work on ACLs and identity exchange we will add one. But for now the thali replication manager can only handle one DB at a time so we agreed to put the addressbook entries in it but that should be an app level (e.g. postcardapp) consideration, not a Thali_CordovaPlugin consideration.

This comment has been minimized.

@srichal

srichal Jul 29, 2015

Contributor

Fixed as part of the redesign.

@srichal

srichal Jul 29, 2015

Contributor

Fixed as part of the redesign.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalireplicationmanager.js
+ include_docs: true,
+ attachments: true
+ }).then(function (result) {
+ var currentAdddrFound = false;

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

There should be 2, not 3 ds in Addr. Actually it's generally not recommended to use abbreviations. Now that we have auto-complete in our IDEs it's better to just use the full word, e.g. currentAddressFound.

@yaronyg

yaronyg Jul 23, 2015

Member

There should be 2, not 3 ds in Addr. Actually it's generally not recommended to use abbreviations. Now that we have auto-complete in our IDEs it's better to just use the full word, e.g. currentAddressFound.

This comment has been minimized.

@srichal

srichal Jul 29, 2015

Contributor

I am not using abbreviations anymore.

@srichal

srichal Jul 29, 2015

Contributor

I am not using abbreviations anymore.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalireplicationmanager.js
+ if(!currentAdddrFound) {
+ console.log('did not find current device address, adding it now');
+ this._db.put({_id: currentAddrEntry, author: '', destination: '', content: ''});
+ //TODO: handle error for this db-put operation

This comment has been minimized.

@yaronyg

yaronyg Jul 23, 2015

Member

Please don't check into story main with a todo like this. Please add in the error handler!

@yaronyg

yaronyg Jul 23, 2015

Member

Please don't check into story main with a todo like this. Please add in the error handler!

This comment has been minimized.

@srichal

srichal Jul 29, 2015

Contributor

This entire DB operation has been moved to the app. I also handled the DB operation error there.

@srichal

srichal Jul 29, 2015

Contributor

This entire DB operation has been moved to the app. I also handled the DB operation error there.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

👍

thali/thalicryptomanager.js
+/**
+* Generates the SHA256 Hash of the input key and returns the first
+* 'hashSizeInBytes' bytes.
+* @param {String} publicKey the piblic key whose hash needs to be generated.

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

/piblic/public

@yaronyg

yaronyg Jul 30, 2015

Member

/piblic/public

This comment has been minimized.

@srichal

srichal Aug 4, 2015

Contributor

fixed.

@srichal

srichal Aug 4, 2015

Contributor

fixed.

This comment has been minimized.

@yaronyg

yaronyg Aug 4, 2015

Member

👍

thali/thalicryptomanager.js
+}
+
+/**
+* Generates the SHA256 Hash of the input key and returns the first

This comment has been minimized.

@yaronyg

yaronyg Jul 30, 2015

Member

In general you don't need to document internal functions and you especially don't need to document such a short function. Typically headers like this are only needed on public functions or on functions that do something really confusing and so could benefit from extra explanation. Remember, comments rot. So we only want to use them when the value they bring is greater than the cost of maintaining them.

@yaronyg

yaronyg Jul 30, 2015

Member

In general you don't need to document internal functions and you especially don't need to document such a short function. Typically headers like this are only needed on public functions or on functions that do something really confusing and so could benefit from extra explanation. Remember, comments rot. So we only want to use them when the value they bring is greater than the cost of maintaining them.

This comment has been minimized.

@srichal

srichal Aug 4, 2015

Contributor

Removed the documentation.

@srichal

srichal Aug 4, 2015

Contributor

Removed the documentation.

This comment has been minimized.

@yaronyg

yaronyg Aug 4, 2015

Member

👍

srichal and others added some commits Aug 4, 2015

Code review comments
I got rid of some extraneous comments and made it clearer what the hash function does. Note that these are "comments" so I haven't tested that they don't break something. :)
thali/thalireplicationmanager.js
@@ -7,6 +7,8 @@ var net = require('net');
var multiplex = require('multiplex');
var validations = require('./validations');
+var currentDeviceIdentity;

This comment has been minimized.

@yaronyg

yaronyg Aug 4, 2015

Member

Why introduce currentDeviceIdentity if we already have _deviceName?

@yaronyg

yaronyg Aug 4, 2015

Member

Why introduce currentDeviceIdentity if we already have _deviceName?

This comment has been minimized.

@srichal

srichal Aug 5, 2015

Contributor

Fixed.

@srichal

srichal Aug 5, 2015

Contributor

Fixed.

This comment has been minimized.

@yaronyg

yaronyg Aug 6, 2015

Member

👍

thali/thalireplicationmanager.js
@@ -66,8 +67,8 @@ ThaliReplicationManager.prototype.start = function (deviceName, port, dbName) {
this._emitter.addListener(NETWORK_CHANGED, networkChanged.bind(this));
this.emit(ThaliReplicationManager.events.STARTED);
}
- }.bind(this));
- }.bind(this));
+ }.bind(this)); //startBroadcasting

This comment has been minimized.

@yaronyg

yaronyg Aug 4, 2015

Member

Please remove the comment on line 70 and 71. This is just the kind of comment that rots really quickly as soon as there is an edit. We have IDEs to handle matching squiggly braces.

@yaronyg

yaronyg Aug 4, 2015

Member

Please remove the comment on line 70 and 71. This is just the kind of comment that rots really quickly as soon as there is an edit. We have IDEs to handle matching squiggly braces.

This comment has been minimized.

@srichal

srichal Aug 5, 2015

Contributor

Fixed.

@srichal

srichal Aug 5, 2015

Contributor

Fixed.

This comment has been minimized.

@yaronyg

yaronyg Aug 6, 2015

Member

👍

thali/thalireplicationmanager.js
+ if(currentDeviceIdentity) {
+ cb(null, currentDeviceIdentity);
+ } else {
+ var cryptomanager = require('./thalicryptomanager');

This comment has been minimized.

@yaronyg

yaronyg Aug 4, 2015

Member

Why are you requiring this down here rather than at the top of the file?

@yaronyg

yaronyg Aug 4, 2015

Member

Why are you requiring this down here rather than at the top of the file?

This comment has been minimized.

@srichal

srichal Aug 5, 2015

Contributor

Moved the getDeviceIdentity function to the top of the file.

@srichal

srichal Aug 5, 2015

Contributor

Moved the getDeviceIdentity function to the top of the file.

This comment has been minimized.

@yaronyg

yaronyg Aug 6, 2015

Member

I wasn't referring to the function definition. I was referring to your use of require('./thalicryptomanager'). Generally it is considered good practice to put require statements at the top of the file. See http://justbuildsomething.com/node-js-best-practices/#2 for an example why.

@yaronyg

yaronyg Aug 6, 2015

Member

I wasn't referring to the function definition. I was referring to your use of require('./thalicryptomanager'). Generally it is considered good practice to put require statements at the top of the file. See http://justbuildsomething.com/node-js-best-practices/#2 for an example why.

This comment has been minimized.

@srichal

srichal Aug 6, 2015

Contributor

I declared the "require" here as it is used only one time here. But you are right, it is better to put it at the top. I moved it now.

@srichal

srichal Aug 6, 2015

Contributor

I declared the "require" here as it is used only one time here. But you are right, it is better to put it at the top. I moved it now.

This comment has been minimized.

@yaronyg

yaronyg Aug 7, 2015

Member

👍

thali/thalireplicationmanager.js
+ cb(null, currentDeviceIdentity);
+ } else {
+ var cryptomanager = require('./thalicryptomanager');
+ // get the device public-key-hash

This comment has been minimized.

@yaronyg

yaronyg Aug 4, 2015

Member

Please remove this comment. When the function is called 'getPublicKeyHash' it's pretty clear what's going on.

@yaronyg

yaronyg Aug 4, 2015

Member

Please remove this comment. When the function is called 'getPublicKeyHash' it's pretty clear what's going on.

This comment has been minimized.

@srichal

srichal Aug 5, 2015

Contributor

Done.

@srichal

srichal Aug 5, 2015

Contributor

Done.

This comment has been minimized.

@yaronyg

yaronyg Aug 6, 2015

Member

👍

thali/thalicryptomanager.js
+ cb('failed to create pkcs12Content');
+ }
+ // write to the file
+ fs.writeFile(fileNameWithPath, pkcs12Content, function (err) {

This comment has been minimized.

@yaronyg

yaronyg Aug 4, 2015

Member

I believe this code suffers from a very nasty race condition! Imagine that two people call getPKCS12Content at the same time (this can easily happen if two calls to getDeviceIdentity are made) and the PKCS12 file doesn't exist yet. In that case both calls can get false on line 70 (since exists is an async function) and both calls can successfully get to line 92 and both will succeed on line 92 (since writeFile is async and explicitly overwrites if the file already exists) and so there will be two callbacks with two difference public keys for the same device!

Thankfully I think there is a pretty easy fix. If you pass in the "wx" flag to writeFile then it should fail if the file already exists. In that case exactly one of the two calls would succeed on the write, the other would get a file exists error.

@yaronyg

yaronyg Aug 4, 2015

Member

I believe this code suffers from a very nasty race condition! Imagine that two people call getPKCS12Content at the same time (this can easily happen if two calls to getDeviceIdentity are made) and the PKCS12 file doesn't exist yet. In that case both calls can get false on line 70 (since exists is an async function) and both calls can successfully get to line 92 and both will succeed on line 92 (since writeFile is async and explicitly overwrites if the file already exists) and so there will be two callbacks with two difference public keys for the same device!

Thankfully I think there is a pretty easy fix. If you pass in the "wx" flag to writeFile then it should fail if the file already exists. In that case exactly one of the two calls would succeed on the write, the other would get a file exists error.

This comment has been minimized.

@srichal

srichal Aug 5, 2015

Contributor

The fix will not work entirely... The 2nd call to writeFile will fail as you explained. But the 2nd guy who called getPKCS12Content will not get the content and thus will not get the public key hash.

This is better than having two different public keys for the same device but the 2nd guy will have to retry by calling getPublicKeyHash again.

@srichal

srichal Aug 5, 2015

Contributor

The fix will not work entirely... The 2nd call to writeFile will fail as you explained. But the 2nd guy who called getPKCS12Content will not get the content and thus will not get the public key hash.

This is better than having two different public keys for the same device but the 2nd guy will have to retry by calling getPublicKeyHash again.

This comment has been minimized.

@srichal

srichal Aug 5, 2015

Contributor

Please ignore my above comment. You already mentioned the issue below.

@srichal

srichal Aug 5, 2015

Contributor

Please ignore my above comment. You already mentioned the issue below.

This comment has been minimized.

@srichal

srichal Aug 5, 2015

Contributor

Fixed as suggested.

@srichal

srichal Aug 5, 2015

Contributor

Fixed as suggested.

This comment has been minimized.

@yaronyg

yaronyg Aug 6, 2015

Member

👍

thali/thalireplicationmanager.js
+ } else {
+ var cryptomanager = require('./thalicryptomanager');
+ // get the device public-key-hash
+ cryptomanager.getPublicKeyHash(function (err, publicKeyHash) {

This comment has been minimized.

@yaronyg

yaronyg Aug 4, 2015

Member

Assuming you make the change I talked about in the the crypto file to deal with the race condition we still have a problem with this code. Which is that if the function gets called twice when there is no existing PKCS12 file then one of the two calls can end up with an error because the underlying write failed. That sort of thing shouldn't be visible (or be a concern) for someone calling getDeviceIdentity.

The problem is that fixing this is tricky because it's possible that when the error is returned the PKCS12 file may have been created but currentDeviceIdentity hasn't been set yet.

There are several traditional ways to deal with this in Node.js land -

Promises - We could use promises to work around this by creating a promise to retrieve the public key from the file and then adding any cbs to a then on that promise. They will then automatically be run once the ID is returned. But I don't think it's worth adding the promises library (we are using lie) just to solve this one issue.

Flags - Another traditional approach is some kind of state flag. You set the flag initially to 'noDeviceIdentitySet' and then when the first call is made to getDeviceIdentity this will cause you to set the flag to 'gettingDeviceKey' and you will then make the call to getPublicKeyHash. Any calls to getDeviceIdentity while the flag is set to 'gettingDeviceKey' will just have their cb added to a cb array. When getPublicKeyHash returns then you would set the flag to 'deviceKeyAvailable' and you would call all the cbs in the arrays. Any future calls to getDeviceIdentity would just immediately return the value since the state is 'deviceKeyAvailable'.

Event Emitter - The third approach is to use an event emitter. This is a bit more complicated but basically you could attach the CBs as a listener to the event emitter and once the key is available then you would emit the event which would trigger the CBs. You would also need to check if the device identity is already available and skip the emitter all together in that case. Honestly I think this is more work than it's worth.

So my suggestion would be the flag approach.

@mattpodwysocki - What do you think is the best way to handle this race condition? Or is there no race condition and I'm just hallucinating?

@yaronyg

yaronyg Aug 4, 2015

Member

Assuming you make the change I talked about in the the crypto file to deal with the race condition we still have a problem with this code. Which is that if the function gets called twice when there is no existing PKCS12 file then one of the two calls can end up with an error because the underlying write failed. That sort of thing shouldn't be visible (or be a concern) for someone calling getDeviceIdentity.

The problem is that fixing this is tricky because it's possible that when the error is returned the PKCS12 file may have been created but currentDeviceIdentity hasn't been set yet.

There are several traditional ways to deal with this in Node.js land -

Promises - We could use promises to work around this by creating a promise to retrieve the public key from the file and then adding any cbs to a then on that promise. They will then automatically be run once the ID is returned. But I don't think it's worth adding the promises library (we are using lie) just to solve this one issue.

Flags - Another traditional approach is some kind of state flag. You set the flag initially to 'noDeviceIdentitySet' and then when the first call is made to getDeviceIdentity this will cause you to set the flag to 'gettingDeviceKey' and you will then make the call to getPublicKeyHash. Any calls to getDeviceIdentity while the flag is set to 'gettingDeviceKey' will just have their cb added to a cb array. When getPublicKeyHash returns then you would set the flag to 'deviceKeyAvailable' and you would call all the cbs in the arrays. Any future calls to getDeviceIdentity would just immediately return the value since the state is 'deviceKeyAvailable'.

Event Emitter - The third approach is to use an event emitter. This is a bit more complicated but basically you could attach the CBs as a listener to the event emitter and once the key is available then you would emit the event which would trigger the CBs. You would also need to check if the device identity is already available and skip the emitter all together in that case. Honestly I think this is more work than it's worth.

So my suggestion would be the flag approach.

@mattpodwysocki - What do you think is the best way to handle this race condition? Or is there no race condition and I'm just hallucinating?

This comment has been minimized.

@srichal

srichal Aug 5, 2015

Contributor

Fixed using flags.

@srichal

srichal Aug 5, 2015

Contributor

Fixed using flags.

This comment has been minimized.

@yaronyg

yaronyg Aug 6, 2015

Member

👍

this._dbName = dbName;
this._serverBridge = muxServerBridge.call(this, port);
this._serverBridge.listen(function () {
this._serverBridgePort = this._serverBridge.address().port;
- this._emitter.startBroadcasting(deviceName, this._serverBridgePort, function (err) {
+ this._emitter.startBroadcasting(this._deviceName, this._serverBridgePort, function (err) {

This comment has been minimized.

@yaronyg

yaronyg Aug 6, 2015

Member

I apologize if I'm missing something but I don't see a check anywhere here that this._deviceName is actually set. There seems to be an assumption that getDeviceIdentity will always be called before start. That seems like a dangerous assumption to make. If in fact we are making that assumption then I would suggest that code should be 'fail safe'. That is, either start should fail if getDeviceIdentity hasn't been called or we should build getDeviceIdentity into start just to make sure. The later seems preferable.

Sorry if I missed where deviceName was guaranteed to be set.

@yaronyg

yaronyg Aug 6, 2015

Member

I apologize if I'm missing something but I don't see a check anywhere here that this._deviceName is actually set. There seems to be an assumption that getDeviceIdentity will always be called before start. That seems like a dangerous assumption to make. If in fact we are making that assumption then I would suggest that code should be 'fail safe'. That is, either start should fail if getDeviceIdentity hasn't been called or we should build getDeviceIdentity into start just to make sure. The later seems preferable.

Sorry if I missed where deviceName was guaranteed to be set.

This comment has been minimized.

@srichal

srichal Aug 6, 2015

Contributor

Fixed by building getDeviceIdentity if user has not done it before calling start. This involved moving around the code as well as adding new functions.

@srichal

srichal Aug 6, 2015

Contributor

Fixed by building getDeviceIdentity if user has not done it before calling start. This involved moving around the code as well as adding new functions.

This comment has been minimized.

@yaronyg

yaronyg Aug 7, 2015

Member

👍

+*/
+ThaliReplicationManager.prototype.getDeviceIdentity = function (cb) {
+ if(this._deviceIdentityFlag == deviceIdentityFlag.deviceIdentityAvailable) {
+ cb(null, this._deviceName);

This comment has been minimized.

@yaronyg

yaronyg Aug 6, 2015

Member

It is generally considered best coding practice to avoid implementing the equivalent of case statements as nested if-then-elses. It is hard to read and keep track of state. A much cleaner solution, in my opinion, is to just use return statements. In other words

if (..deviceIdentityAvailable...) {
cb();
return;
}

if (...noDeviceIdentitySet...) {
stuff...
return;
}

if (...gettingDeviceIdentity...) {
blah...
return;
}

@yaronyg

yaronyg Aug 6, 2015

Member

It is generally considered best coding practice to avoid implementing the equivalent of case statements as nested if-then-elses. It is hard to read and keep track of state. A much cleaner solution, in my opinion, is to just use return statements. In other words

if (..deviceIdentityAvailable...) {
cb();
return;
}

if (...noDeviceIdentitySet...) {
stuff...
return;
}

if (...gettingDeviceIdentity...) {
blah...
return;
}

This comment has been minimized.

@srichal

srichal Aug 6, 2015

Contributor

Fixed. Will also use this format in the future.

@srichal

srichal Aug 6, 2015

Contributor

Fixed. Will also use this format in the future.

This comment has been minimized.

@yaronyg

yaronyg Aug 7, 2015

Member

👍

thali/thalireplicationmanager.js
@@ -112,6 +148,18 @@ ThaliReplicationManager.prototype.stop = function () {
});
};
+function informDeviceIdentityListeners(err, publicKeyHash) {

This comment has been minimized.

@yaronyg

yaronyg Aug 6, 2015

Member

Shouldn't this code be hanging out with the rest of the identity set code? It's odd to separate them. But I realize this was probably a consequence of my comment about moving the require not being clear enough. Sorry. But could you please re-unite informDeviceIdentityListeners and clearDeviceIdentityListeners with getDeviceIdentity?

@yaronyg

yaronyg Aug 6, 2015

Member

Shouldn't this code be hanging out with the rest of the identity set code? It's odd to separate them. But I realize this was probably a consequence of my comment about moving the require not being clear enough. Sorry. But could you please re-unite informDeviceIdentityListeners and clearDeviceIdentityListeners with getDeviceIdentity?

This comment has been minimized.

@srichal

srichal Aug 6, 2015

Contributor

Actually, Matt had all the external functions at the top followed by all the internal functions. So I was following that pattern. But I have reunited the functions now.

@srichal

srichal Aug 6, 2015

Contributor

Actually, Matt had all the external functions at the top followed by all the internal functions. So I was following that pattern. But I have reunited the functions now.

This comment has been minimized.

@yaronyg

yaronyg Aug 7, 2015

Member

👍

thali/thalireplicationmanager.js
+}
+
+function clearDeviceIdentityListeners() {
+ while(this._deviceIdentityListeners.length > 0) {

This comment has been minimized.

@yaronyg

yaronyg Aug 6, 2015

Member

Wouldn't it be easier to just set this._deviceIdentityListeners = []?

@yaronyg

yaronyg Aug 6, 2015

Member

Wouldn't it be easier to just set this._deviceIdentityListeners = []?

This comment has been minimized.

@srichal

srichal Aug 7, 2015

Contributor

Learned a new thing today :). Also removed the function as it only has one line now.

@srichal

srichal Aug 7, 2015

Contributor

Learned a new thing today :). Also removed the function as it only has one line now.

This comment has been minimized.

@yaronyg

yaronyg Aug 7, 2015

Member

👍

@@ -0,0 +1,3 @@
+# The folder where a good pkcs12 file will be saved.
+
+!.gitignore

This comment has been minimized.

@yaronyg

yaronyg Aug 7, 2015

Member

I'm confused. Why would you need to tell the system not to ignore .gitignore? It wouldn't ignore it anyway?

@yaronyg

yaronyg Aug 7, 2015

Member

I'm confused. Why would you need to tell the system not to ignore .gitignore? It wouldn't ignore it anyway?