Store passwords in user.db configs #251

lukebarnard1 · 2016-10-25T16:42:35Z

Send !storepass server.name password to have your virtual IRC client's password stored encrypted in the db.

The passwords are encrypted and decrypted using a PEM key stored in a file specified in the config file (See passwordEncryptionKeyPath).

The passwords are then used in place of the server password set in the config file, whether it is set or not.

This has been tested to work whilst bridging with freenode, and as expected, when !nick and !storepass are issued prior to the bridge restarting (or the IRC client reconnecting), NickServ asks for identification (if the nick is registered) but then immediately resolves (having received PASS with the recently decrypted password).

The passwords are encrypted and decrypted using a PEM key stored in a file specified in the config file (See passwordEncryptionKeyPath). The passwords are then used in place of the server password set in the config file. This has been tested to work with freenode, and as expected, when !nick and !storepass are issued prior to the bridge restarting (or the IRC client reconnecting), NickServ asks foridentification (if the nick is registered) but then imediately resolves (having received PASS with the recently decrypted password).

kegsay · 2016-10-26T09:13:37Z

config.sample.yaml

+  # made for the corresponding virtual IRC user of the matrix user that has issued
+  # !storepass, they will connect with the password that has been persisted, if
+  # there is one.
+  passwordEncryptionKeyPath: "passkey.pem"


kegsay · 2016-10-26T09:14:33Z

lib/DataStore.js

+
+    if (pkeyPath) {
+        try {
+            this._privateKey = fs.readFileSync(pkeyPath, "utf8").toString();


Can you please default-initialise this._privateKey to null.

kegsay · 2016-10-26T09:15:54Z

config.sample.yaml

+  # WARNING - STORING PASSWORDS ENCRYPTED IS PRECAUTIONARY TO MAKE COMPROMISE LESS
+  #           LIKELY. THIS MEASURE IS ONLY AS SECURE AS THE KEY FILE, BELOW.
+  #
+  # The path to the RSA PEM-formatted key to use when encrypting and decrypting


You don't mention if this should be the private or public key.

You don't mention if this is allowed to have a password on it.

kegsay · 2016-10-26T09:19:13Z

config.sample.yaml

@@ -328,3 +328,14 @@ ircService:
    # allotted time period, the provisioning request will fail.
    # Default: 300 seconds (5 mins)
    requestTimeoutSeconds: 300
+
+  # WARNING - STORING PASSWORDS ENCRYPTED IS PRECAUTIONARY TO MAKE COMPROMISE LESS
+  #           LIKELY. THIS MEASURE IS ONLY AS SECURE AS THE KEY FILE, BELOW.


I would tweak the wording on this. You want to tell them:

This is only "data at rest" encryption, the passwords are completely retrievable with the key (rather than being stored as a hash) because the bridge needs to send the password in the plain.

Said encryption is only as secure as the key file, so there should be restrictions on who can read the PEM file on the machine.

kegsay · 2016-10-26T09:19:55Z

lib/DataStore.js

+        if (!this._privateKey && !forcePlaintextStorage) {
+            throw new Error(
+                'WARNING: Store plaintext passwords at your own ' +
+                'risk; use -f to force'


kegsay · 2016-10-26T09:21:42Z

lib/bridge/IrcBridge.js

+                `is provided to the bridge. See config 'passwordEncryptionKeyPath'.`
+            );
+        }
+        ircClientConfig.setPrivateKey(this.getStore()._privateKey);


Please don't gut-wrench private member variables.

kegsay · 2016-10-26T09:23:32Z

lib/bridge/IrcBridge.js

+    this._dataStore = new DataStore(
+        this._bridge.getUserStore(),
+        this._bridge.getRoomStore(),
+        pkeyPath


In-line please.

kegsay · 2016-10-26T09:25:45Z

lib/bridge/MatrixHandler.js

+
+        try {
+            let force = args[1] === '-f';
+            yield this.ircBridge.getStore().storePass(userId, domain, args[0], force);


Right now I see why you mentioned -f before. Why is -f A Thing? This should not be an option for random Matrix users.

kegsay · 2016-10-26T09:31:32Z

lib/models/IrcClientConfig.js

+        }
+        if (typeof privateKey !== 'string') {
+            let actual = typeof privateKey;
+            throw new Error("Private key must be an RSA PEM-formatted string, not " + actual);


This all feels like needless boilerplate. We load the PEM key from file in DataStore, why aren't we just checking the types there? Why do we pass the private key through as a member variable to IrcClientConfig? Why is it even an option to store passwords unencrypted? It means you get these scenarios where you have no idea what _config.password is (unencrypted or encrypted) and you just make more work for yourself.

kegsay · 2016-10-26T09:37:00Z

Broadly speaking this is looking good, but the form of the API is questionable.

I don't like how we pass around the private key between DataStore and IrcClientConfigs, and do some checks in each: just choose one and stick with it (probably DataStore).
I don't like how users can choose to not encrypt their passwords: it's unthinkable that we should persist any password in the clear on the database.
I don't like how the config has to "redact" the password field - it shouldn't be stored in _config in the clear. getPassword should decrypt the encrypted form and return that and that's it.

lukebarnard1 · 2016-10-26T09:59:55Z

On your three points:

If DataStore is the only thing to ever store the private key, we would need some way to get the privateKey from there into calls to IrcClientConfig.getPassword(privateKey) because that is how the IrcBridge class gets the passwords. To be honest, I just couldn't see a clearer way of doing it architecturally, any input would be appreciated. To clarify, the problem with storing privateKey in DataStore is that the IrcBridge would have to inspect the DataStore every time it calls getPassword.
Fair enough, I'll remove the whole '-f' thing and replace 'use -f' notice with 'no private key loaded'.
I think it's perfectly fine to store the encrypted password in _config and I don't understand why we wouldn't redact the encrypted form. (Also, I'm assuming 'in the clear' means plaintext as opposed to encrypted?)

kegsay · 2016-10-26T14:16:04Z

Well it doesn't help you appear to be trying to reuse getPassword when it is currently being used for the bot password: https://github.com/matrix-org/matrix-appservice-irc/blob/master/lib/irc/IrcServer.js#L94 - but of course this will break when you add encryption and expect that field to be encrypted.

The private key is loaded from file when the DataStore starts up. When you hit storePass you give the plaintext and behind the scenes it does the necessary magic to plop it into the DB encrypted. This feels right: nothing else should really give a damn about this.

Can we do the same for getPassword? That would imply we have the client config store the unencrypted form inside itself, since it doesn't know about the encryption at all. When the bridge tries to load the IrcClientConfig for a user, the DataStore magically decrypts entries. This would also mean my first point about it breaking for the bot user is invalid: since it'll still be the unencrypted form. WDYT?

lukebarnard1 · 2016-10-27T12:32:05Z

That sounds reasonable, WFM.

Passwords are kept in config objects unencrypted; passwords are encrypted when the config itself is stored in the database and dencrypted when the config is retrieved from the database.

kegsay · 2016-10-27T14:51:45Z

lib/DataStore.js

+            log.info(`Private key loaded from ${pkeyPath} - IRC password encryption enabled.`);
+        }
+        catch (err) {
+            log.error('Could not load private key ${err.message}.');


Feels like this should be fatal.

Then you can remove :473 guard.

But what if someone wants to use the bridge without bothering with generating a PEM key?

I don't follow. This block only executes if (pkeyPath). So if you want to use the bridge without bothering to generate a PEM key, then don't fill in that config field?

But then how can I remove the guard on line 473? If the private key is null, surely we just want to throw an error?

kegsay · 2016-10-27T14:52:02Z

config.sample.yaml

+  # `!storepass server.name passw0rd. When a connection is made for the corresponding
+  # virtual IRC user of the matrix user that has issued !storepass, they will connect
+  # with the (decrypted) password that has been persisted, if there is one.
+  passwordEncryptionKeyPath: "passkey.pem"


kegsay · 2016-10-27T14:55:03Z

lib/bridge/MatrixHandler.js

+        let notice;
+
+        try {
+            yield this.ircBridge.getStore().storePass(userId, domain, args[0]);


Are spaces allowed in NickServ passwords? If so, then args[0] ain't gonna cut it.

Or rather, it will cut it 😆

kegsay · 2016-10-27T14:55:59Z

lib/models/IrcClientConfig.js

+
+        // The PEM string used to encrypt passwords when setPassword is
+        // called and decrypt when getPassword is called.
+        this._privateKey = null;


Never used.

kegsay · 2016-10-27T14:56:24Z

lib/models/IrcClientConfig.js

@@ -42,7 +47,7 @@ class IrcClientConfig {
    }

    getPassword() {
-        return this._config.password;
+       return this._config.password;


3 space indent?

kegsay · 2016-10-27T15:06:17Z

lib/DataStore.js

+    if (pkeyPath) {
+        try {
+            this._privateKey = fs.readFileSync(pkeyPath, "utf8").toString();
+            log.info(`Private key loaded from ${pkeyPath} - IRC password encryption enabled.`);


We should probably assert that the key is usable first so we can fail fast if _privateKey isn't a PEM file. Try encrypting/decrypting a test string in this constructor.

kegsay · 2016-11-04T11:31:57Z

config.sample.yaml

@@ -328,3 +328,13 @@ ircService:
    # allotted time period, the provisioning request will fail.
    # Default: 300 seconds (5 mins)
    requestTimeoutSeconds: 300
+
+  # WARNING - STORED PASSWORDS ENCRYPTED ARE COMPLETELY RETRIEVABLE WITH THE KEY.
+  #           THE BRIDGE NEEDS TO SEND PASSWORDS TO THE IRC SERVERS AS PLAINTEXTS.


Maybe rephrase to:

WARNING: The bridge needs to send plaintext passwords to the IRC server, it cannot send a password hash. As a result, passwords (NOT hashes) are stored encrypted in the database.

I'm not sure it's worth mentioning hashes at all. The PEM key requirement is clearly for not making hashes. How about:

WARNING: The bridge needs to send plaintext passwords to the IRC server, and so passwords are stored encrypted in the database.

?

kegsay · 2016-11-04T11:35:42Z

config.sample.yaml

+  # for storage in the database. Passwords are stored by using the admin room command
+  # `!storepass server.name passw0rd. When a connection is made for the corresponding
+  # virtual IRC user of the matrix user that has issued !storepass, they will connect
+  # with the (decrypted) password that has been persisted, if there is one.


When a connection is made for the corresponding virtual IRC user of the matrix user that has issued !storepass, they will connect with the (decrypted) password that has been persisted, if there is one.

Maybe rephrase to:

When a connection is made to IRC on behalf of the Matrix user, this password will be sent as the server password (PASS command).

kegsay · 2016-11-04T11:37:17Z

lib/DataStore.js

+                this._privateKey,
+                new Buffer(clientConfig.getPassword(), 'base64')
+            ).toString();
+            decryptedPass = decryptedPass.split(' ')[1];


Uhh what? Explanation please.

It's to remove the salt and extract the real password

Can you put that as a comment then please :)

kegsay · 2016-11-04T11:50:01Z

lib/bridge/MatrixHandler.js

+            let pass = args.join('');
+            yield this.ircBridge.getStore().storePass(userId, domain, pass);
+            notice = new MatrixAction(
+                "notice", `Successfully stored password`


We should mention that this only applies on reconnects and that they still need to poke NickServ initially. Can we name the domain as well please (since the server is often implicit, they may not have typed it in).

Successfully stored password on ${domain}. When you next reconnect to ${domain}, this password will be automatically sent in a PASS command which most IRC networks will use as your NickServ password. This means you will not need to talk to NickServ. This does NOT apply to your currently active connection: you still need to talk to NickServ one last time to authenticate your current connection if you haven't already.

We should probably also allow them to remove their stored password.

kegsay · 2016-11-04T11:52:02Z

Other than wordings on things, this is looking good.

I did have a thought though: we should probably allow people to remove their stored password somehow.

lukebarnard1 · 2016-11-04T12:44:20Z

Add !removepass command to unset the password
Handle empty password as an error case

kegsay · 2016-11-04T13:06:53Z

Should an empty pass be treated as an error or should we display the usage (similar to !nick)?

…that domain

lukebarnard1 · 2016-11-04T15:17:43Z

I went for the latter as it does indicate that the user doesn't really understand the command.

kegsay · 2016-11-09T11:43:59Z

lib/bridge/MatrixHandler.js

+                notice = new MatrixAction(
+                    "notice",
+                    "Format: '!storepass password' " +
+                    "or '!storepass irc.server.name password'\n"


Explanation of what this does would be good. I would move the blurb you have on success ("When you next reconnect...") to a var and then use that here in addition to on success.

kegsay · 2016-11-09T11:44:49Z

lib/bridge/MatrixHandler.js

+        yield this.ircBridge.sendMatrixAction(adminRoom, botUser, notice, req);
+        return;
+    }
+    else if (cmd === "!removepass") {


Both this and !storepass need to be added to !help.

kegsay · 2016-11-09T11:45:58Z

After you add more words to the commands, LGTM!

Conflicts: lib/bridge/MatrixHandler.js

3v1n0 · 2018-12-21T02:44:12Z

I'm wondering... Couldn't be the matrix protocol to be updated so that the key is encrypted using some runtime data that is valid for this user only?

I'm not into the deep of matrix, but having the service to require a matrix user pass whenever the nick pass should be used could be a way. Indeed this wouldn't make auto-connection work in case until the user has approved this, but it would make possible to trust the system whoever is hosting it.

kegsay · 2018-12-22T17:16:01Z

Sadly I don't think what you want is technically feasible. It always bugged me that we were/are persisting passwords on the bridge since it slaps a big target on our back, but you must understand the requirements here.

The bridge has to send the user/password to the target IRC server. Even if you only ever gave the bridge an encrypted password which you had the key for, then did some dance to decrypt it on one of your clients, it still wouldn't improve trust. There would be nothing to stop the bridge from retrieving the decrypted password from your client then doing bad things with it, in addition to logging you in to IRC. Simply put, the bridge has to have the password in the plain somewhere so it can do the necessary operation.

Luke Barnard added 2 commits October 24, 2016 15:21

[wip] storepass logic

0a52e8c

lukebarnard1 assigned kegsay Oct 25, 2016

kegsay reviewed Oct 26, 2016

View reviewed changes

Luke Barnard added 4 commits October 27, 2016 13:59

IrcClientConfig does not know about encryption

696ea5e

Passwords are kept in config objects unencrypted; passwords are encrypted when the config itself is stored in the database and dencrypted when the config is retrieved from the database.

Fix, use the cright config object before storing

881f25b

Fixes; make it work; less whitespace

6eb61d7

Remove setPrivateKey

9ac2b2b

kegsay reviewed Oct 27, 2016

View reviewed changes

Luke Barnard added 4 commits October 28, 2016 15:41

Fatal if key path specified but not found

832c83a

NL at EOL

908e3d9

Allow passwords with spaces

8c0c99d

Remove unused variable

ad9424f

Luke Barnard added 2 commits November 3, 2016 16:27

Fail on startup if private key is invalid

233b195

Add a salt to the passwords stored in the db

27b1ef8

kegsay reviewed Nov 4, 2016

View reviewed changes

Luke Barnard added 3 commits November 4, 2016 12:47

Handle empty password as error

9250e2a

removepass command

3d9444a

Update pkey config desc

5d12dc6

Luke Barnard added 5 commits November 4, 2016 13:18

Update stored password message, and log error stack

72f20f9

Throw early when the user does not have an IRC client configured for …

d0e6ec7

…that domain

Update error message

2b1f1f4

Comment password extraction

3c87db4

Send format response when password length 0

e5c4ce7

kegsay reviewed Nov 9, 2016

View reviewed changes

Luke Barnard added 3 commits November 10, 2016 16:16

Explanation in format notice

a9df84e

update help message

c569dd8

Merge branch 'develop' into luke/feature-nickserv-password

b18f83c

Conflicts: lib/bridge/MatrixHandler.js

lukebarnard1 merged commit 739b5fa into develop Nov 10, 2016

ara4n mentioned this pull request Jan 11, 2017

The official network wishlist for matrix.org / Element #208

Open

3v1n0 unassigned kegsay Dec 21, 2018

Store passwords in user.db configs #251

Store passwords in user.db configs #251

Uh oh!

Conversation

lukebarnard1 commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kegsay commented Oct 26, 2016

Uh oh!

lukebarnard1 commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kegsay commented Oct 26, 2016

Uh oh!

lukebarnard1 commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukebarnard1 Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kegsay commented Nov 4, 2016

lukebarnard1 commented Oct 26, 2016 •

edited

Loading

lukebarnard1 Nov 4, 2016 •

edited

Loading

lukebarnard1 commented Nov 4, 2016 •

edited

Loading