Add preliminary TLS1.3 PSK-mode implementation. #24

rfk · 2019-01-18T01:25:55Z

Fixes #24

(This currently targets #23 but I will rebase against master once that's merged)

This adds actually-appears-to-be-functioning code to implement the TLS1.3 PSK mode. It needs a lot of tests, but I have confirmed that it interoperates successfully with an existing TLS library (tlslite-ng) so I have some base level of confidence that it's doing the right thing in the happy case.

@vladikoff @eoger please try building against this branch and check if things work OK. I will rebase this branch as I add additional testcases and cleanups.

Things still to do:

Implement the basics of the "alert" protocol, for cleanly closing on crypto errors etc.
Just, like, lots more tests in general.
More thorough interop tests with tlslite-ng.
~~Look in the NSS codebase for specific testcases to duplicate here.~~

vladikoff · 2019-01-18T19:38:05Z

~~First look at this, we cannot make this work yet:~~

vladikoff · 2019-01-18T19:58:40Z

Ok we got it to work, it was a problem with psk

rfk · 2019-01-22T04:19:47Z

@vladikoff @eoger thanks for the fixes; heads-up that I'm going to port some of them to master and squash the others with my latest work.

eoger · 2019-01-23T01:54:57Z

Could we rebase this on master?

rfk · 2019-01-23T02:04:00Z

Rebased.

martinthomson

Since I was looking, I spent a little time on this. Hopefully some of my suggestions result in more tests being added :)

martinthomson · 2019-01-25T03:21:33Z

src/utils.js

+
+  seek(pos) {
+    this._pos = pos;
+    assert(this._pos <= this.length(), 'do not seek past end of buffer');


Shouldn't you assert on < 0 as well? And why not check the input before applying it?

martinthomson · 2019-01-25T03:22:02Z

src/utils.js

+    this._pos = 0;
+  }
+
+  resize(size) {


If you can't shrink, then grow() might be the name you want.

martinthomson · 2019-01-25T03:22:53Z

src/utils.js

+    return slice;
+  }
+
+  readRemainingBytes() {


In TLS, this is probably a hazard. You should always know how many bytes you are reading.

👍 ; from a quick grep it looks like this is actually a leftover that is no longer used, so I guess I refactored away whatever hazard was using it...

martinthomson · 2019-01-25T03:28:25Z

src/utils.js

+  _readVector(length, cb) {
+    const limit = this.tell() + length;
+    // Keep calling the callback until we've consumed the expected number of bytes.
+    // It can return `true` to indicate we should stop iterating and skip the remainder.


I would disable this feature. The callback should be run once for each item in the list until it is done.

One thing I would do with the callback is pass it a different instance of this object that offers a view onto the same buffer, but with a the shorter length. Then they can't read past "length" by design and you don't have to worry about inner reads going outside of the space that is available.

martinthomson · 2019-01-25T03:28:41Z

src/utils.js

+  //
+
+  _readVector(length, cb) {
+    const limit = this.tell() + length;


assert that limit <= this.length()

martinthomson · 2019-01-25T03:32:40Z

src/utils.js

  for (let i = 0; i < v1.length; i++) {
-    mismatch &= v1[i] !== v2[i];
+    mismatch |= v1[i] ^ v2[i];


As we discussed, crypto.subtle.verify might be what you want here.

martinthomson · 2019-01-25T03:47:03Z

test/tlsconnection.js

+    // XXX TODO: feed it the pre-generated ServerHello.
+  });
+});
+


Some tests to consider, though I'm guessing that you have some of these already:

test reading and writing more thoroughly, including short reads and long reads

skipping records fails

damage the ciphertext and the recipient explodes

skipping handshake messages fails

missing extensions means failure

extra extensions (in ClientHello are ignored, in ServerHello generate alerts)

test that writing blocks/fails prior to the handshake being complete (you could write at the server in 0.5-RTT, but please disable that and test that it is disabled)

test that you ignore ChangeCipherSpec (or that you choke cleanly) and limit it to during the handshake if you do accept it, if you do ignore it, the server should probably emit one if the client sends a non-zero session ID

test that you are actually encrypting (yep, that's somewhat tricky, but it's pretty important)

test without a mocked RNG and see that you get different ClientHello/ServerHello when you run the handshake multiple times

check that reordered extensions still work fine

add dummy versions, ciphersuites, and PSK modes to extensions and see that these are ignored

add a non-zero compression mode and the server should explode

validate that the version in the record header is correct (TLS 1.0 I think is right: 0x0301)

validate the version in the ServerHello is TLS 1.2 (0x0303)

handle padded records properly

abort if a record only contains zeros (i.e., it is over-padded)

abort if a handshake record is empty (i.e., it is properly padded with a handshake type, but no data)

abort if an alert of any kind is received

you could write at the server in 0.5-RTT

To clarify, do you mean the server sending application data after it has sent its Finished, but before it has received Finished from the client in return?

Right. In TLS 1.3, the server can send before it receives the Finished from the client, but that is generally considered risky in mutually authenticated modes. For your purposes, you want to wait until you have verified the peer's Finished, in both directions.

rfk · 2019-01-25T06:33:00Z

Things still to do:
More thorough interop tests with tlslite-ng.

The latest push includes both (tlslite-ng => fxa-pairing-channel) and (fxa-pairing-channel => tlslite-ng) test vectors and they appear to be passing \o/

Look in the NSS codebase for specific testcases to duplicate here.

I can honestly say I looked, but also am not short on testcases to add as evidenced by comment above...

vladikoff · 2019-01-25T19:50:14Z

src/states.js

+  async sendApplicationData(bytes) {
+    assert(false, 'uninitialized state');
+  }
+  async sendAlertMessage(err) {


is this sendAlertMessage used? I see _sendAlertMessage elsewhere?

vladikoff · 2019-01-25T19:52:38Z

src/messages.js

+  }
+
+  static _read(buf) {
+    // The legacy_version field may indiciate an earlier version of TLS


typo: indiciate -> indicate

vladikoff · 2019-01-25T19:53:29Z

src/messages.js

+            break;
+          case EXTENSION_TYPE.PRE_SHARED_KEY:
+            // https://tools.ietf.org/html/rfc8446#section-4.2.11
+            // The extension data contains an `OfferredPsks` struct:


typo, per spec OfferredPsks --> OfferedPsks?

vladikoff · 2019-01-25T19:54:07Z

src/states.js

+    throw this.error;
+  }
+  async handleError(err) {
+    // We've alreaded errored out the connection, don't try to do it again.


already ?

vladikoff · 2019-01-25T19:54:19Z

src/states.js

+    alertIf(! bytesAreEqual(msg.verifyData, this._expectedServerFinishedMAC), ALERT_DESCRIPTION.DECRYPT_ERROR);
+    // Send our own Finished message in return.
+    // This must be encrypted with the handshake traffic key,
+    // but must not appear in the transscript used to calculate the application keys.


transcript

rfk · 2019-02-02T23:06:54Z

src/keyschedule.js

+    this.extBinderKey = await this.deriveSecret('ext binder', EMPTY);
+  }
+
+  async addECDHE(ecdhe) {


I would prefer to make the caller pass in an explicit "I don't have an ECDHE output" indicator, seems safer to me. I think I'll make this a more explicit check for null rather than just falsiness.

Sounds good!

vladikoff · 2019-01-25T20:08:19Z

src/messages.js

+  }
+
+  static fromBytes(bytes) {
+    // Each handshake messages has a type and length prefix, per


messages -> message ?

vladikoff · 2019-01-25T20:09:21Z

src/messages.js

+  }
+
+  write(buf) {
+    // Each handshake messages has a type and length prefix, per


messages -> message?

vladikoff · 2019-01-25T20:21:29Z

src/recordlayer.js

+    // * xor with the provided iv
+    const nonce = new Uint8Array(IV_LENGTH);
+    // Our sequence numbers are always less than 2^24, so fit in a Uint32.
+    (new DataView(nonce.buffer)).setUint32(IV_LENGTH - 4, this.seqnum);


new DataView(nonce.buffer).setUint32(IV_LENGTH - 4, this.seqnum); ? remove extra (?

vladikoff · 2019-01-25T20:23:37Z

src/recordlayer.js

+// The main RecordLayer class.
+
+export class RecordLayer {
+  constructor(sendCallback) {


do you want to assert that sendCallback is set?

rfk · 2019-02-06T10:02:29Z

By way of a brief update here, I'm working my way through the recommended testcases and expect to have them wrapped by in the next day or two. I also managed to get the test runner to emit coverage reports and run them automatically on CircleCI, latest one from the build output here:

https://87-160715051-gh.circle-artifacts.com/0/home/circleci/project/coverage/Firefox%2064.0.0%20%28Linux%200.0.0%29/src/index.html

This does a pretty good job of highlighting the error cases that are not yet covered by tests!

@vladikoff, whenever you have a few moments, I'd appreciate your feedback on this commit which adds the test coverage reporting. I had to add a third build configuration with babel-plugin-instanbul in order to get coverage reporting working with the combination of async/await, babel and karma, but maybe you know of a better approach?

rfk · 2019-02-08T05:03:18Z

Ugh, this one test "FxAccountsPairingChannel a Client and Server connected together should communicate successfully if they have the same PSK FAILED" is perma-failing due to a timeout, I wonder if there's some timing issue in setting up the websocket listeners or something, because it passes reliably on my machine...

rfk · 2019-02-11T08:04:18Z

src/utils.js

+  // Using `value instanceof Uint8Array` seems to fail in Firefox chrome code
+  // for inscrutable reasons, so we do a less direct check.
+  assert(ArrayBuffer.isView(value), msg);
+  assert(value.BYTES_PER_ELEMENT === 1, msg);


@vladikoff do either or both of these pass in Firefox chrome code, or do we need to keep looking for a suitable approach here?

rfk · 2019-02-11T08:40:13Z

@martinthomson I've pushed a bunch of tests for the client side of the handshake, and I'm pretty happy with the level of test coverage overall; if you're interested you can view the built coverage report here:

https://94-160715051-gh.circle-artifacts.com/0/home/circleci/project/coverage/Firefox%2064.0.0%20%28Linux%200.0.0%29/src/index.html

I don't have anything else I plan to add here in response to your first round of comments.

martinthomson

I'm going to try to review all the TLS stuff here, and not a lot more, but I do have a few comments outside of that.

Running npm install;npm test produces changes to checked-in code. That's not cool. I don't pretend to know how to manage this stuff, but I find the fact that you have build output in the tree to be unhygienic (for this exact reason).

The changes to package-lock.json seem to be the addition of an optional key to most of the dev dependencies, which might be a cleanup performed by npm 6.5.0 (if you are running an earlier version, that is).

This is part1. I need to continue this later. I haven't really started looking at the tests, so it might take a few more hours.

martinthomson · 2019-02-12T00:02:40Z

src/alerts.js

+    switch (description) {
+      case ALERT_DESCRIPTION.CLOSE_NOTIFY:
+      case ALERT_DESCRIPTION.USER_CANCELLED:
+        this.level = ALERT_LEVEL.WARNING;


Do you use the user_cancelled alert? Because if you don't, then it might pay not to have the case here. TLS 1.3 treats all alerts as fatal other than close_notify, so this might not work out anyway.

Put differently: if you aren't testing this, remove it.

I'm not using it, and only included it because it was mentioned alongside close_notify in [1]. I'll be very happy to remove it.

[1] https://tools.ietf.org/html/rfc8446#section-6.1

martinthomson · 2019-02-12T00:05:06Z

src/crypto.js

+export async function decrypt(key, iv, ciphertext, additionalData) {
+  let plaintext;
+  try {
+    plaintext = await crypto.subtle.decrypt({


If you put the return inside the try, would that mean you have to deal with warnings about the function not returning?

Nope, I think I might have just had it outside for some console.log debugging at some point

src/crypto.js

+export async function hkdfExpand(prk, info, length) {
+  // Ref https://tools.ietf.org/html/rfc5869#section-2.3
+  const N = Math.ceil(length / HASH_LENGTH);
+  if (N >= 255) {


martinthomson · 2019-02-12T00:11:11Z

src/index.js

@@ -72,7 +72,7 @@ export class InsecurePairingChannel extends EventTarget {
          const pskId = utf8ToBytes(channelId);
          const connection = await ConnectionClass.create(psk, pskId, data => {
            // To send data over the websocket, it needs to be encoded as a safe string.
-            socket.send(bytesToHex(data));
+            socket.send(bytesToBase64url(data));


I'm not sure about this assertion regarding strings. They take ABV: https://html.spec.whatwg.org/multipage/web-sockets.html#dom-websocket-send

This is an implementation detail of the server we're talking to, rather than of websockets in general. (The comment could be worded more clearly in that regard).

martinthomson · 2019-02-12T00:15:43Z

src/keyschedule.js

+const STAGE_HANDSHAKE_SECRET = 2;
+const STAGE_MASTER_SECRET = 3;
+
+export class KeySchedule {


It would be good to have a comment explaining the state machine: UNINITIALIZED --addPSK--> EARLY_SECRET --addECDHE--> HANDSHAKE_SECRET --finalize--> MASTER_SECRET.

martinthomson · 2019-02-12T02:00:37Z

src/utils.js

+  }
+
+  writeVector8(cb) {
+    return this._writeVector(Math.pow(2, 8), this.writeUint8.bind(this), cb);


is len => this.writeUint8(len) better than using bind()?

martinthomson · 2019-02-12T02:02:05Z

test/FxAccountsPairingChannel.js

-      const client = await FxAccountsPairingChannel.InsecurePairingChannel.connect(CHANNEL_SERVER, channelId, channelKey);
-      client.addEventListener('message', ({detail: {data}}) => {
-        CLIENT_RECV.push(data.msg);
+      const client = await FxAccountsPairingChannel.PairingChannel.connect(CHANNEL_SERVER, channelId, channelKey);


You run against a network-based service in test? That seems brittle. Can you run this server locally? Or at least a cut-down version of the same.

martinthomson · 2019-02-12T02:04:24Z

test/helpers.js

+    const tampered = bytes.slice();
+    const i = Math.floor(Math.random() * (tampered.byteLength - start) + start);
+    tampered[i] += 1 + Math.floor(Math.random() * 255);
+    return tampered;


The problem with this function is that if it does trigger an error, then you won't ever know what conditions produced that error. If you are going to fuzz, record the way in which your tampering was applied so that you can reproduce problems easily.

martinthomson · 2019-02-12T02:07:41Z

test/helpers.js

+    const buf = new BufferWriter();
+    buf.writeUint8(1);
+    buf.writeVector24(buf => {
+      buf.writeUint16(typeof opts.version !== 'undefined' ? opts.version : 0x0303);


You use typeof opts.x !== 'undefined' ? opts.x : blah here, but the simpler opts.x || blah above. Any particular reason?

IIRC the simpler opts.x || default form is used when the default is falsy, and hence there's no reason for the caller to want to pass in an explicit falsy value. Probably cleaner to just use typeof opts.x !== 'undefined' consistently throughout.

martinthomson · 2019-02-12T02:08:34Z

test/helpers.js

+  signClientHelloMessage: async function (clientHello, psk) {
+    const keyschedule = new KeySchedule();
+    await keyschedule.addPSK(psk);
+    const binder = await keyschedule.calculateFinishedMAC(keyschedule.extBinderKey, clientHello.slice(0, -35));


-35 = magic number

rfk · 2019-02-12T02:52:51Z

src/states.js

+        this.conn._closeForRecv();
+        break;
+      default:
+        return await this.handleError(err);


Note to self: this won't actually throw the error, but it should!

Maybe you should name the function error when you have it throw.

yep; in my head it's something like handleAndRethrowError to make it super clear what will happen.

martinthomson

Looking good.

It's tedious going through all these tests, but I'm fairly happy with the coverage you are getting. I see some unreachable code that is not tested, but the rest seems fairly thorough.

I have a few suggestions for more tests, a couple of minor problems, and probably a need for another round before I'm completely happy with this. But this is a very competent effort. A good example of how to do this right.

martinthomson · 2019-02-12T03:50:36Z

src/messages.js

+    // We should not receive any encrypted extensions,
+    // since we do not advertize any in the ClientHello.
+    buf.readVector16(buf => {
+      if (buf.length() !== 0) {


The coverage report here seems to suggest that this test always passes - as in, the callback doesn't get called if the buffer is empty. You can remove the check in that case.

martinthomson · 2019-02-12T03:51:48Z

test/vectors/generate_test_vectors.py

@@ -0,0 +1,374 @@
+#


I didn't review this bit.

martinthomson · 2019-02-12T03:52:32Z

test/utils.js

+    assert.ok(! bytesAreEqual(arrayToBytes([1, 2, 3]), arrayToBytes([1, 2, 4])));
+    assert.ok(! bytesAreEqual(arrayToBytes([1, 2, 3]), arrayToBytes([1, 2, 3, 4])));
+    assert.ok(! bytesAreEqual(arrayToBytes([1, 2, 3, 4]), arrayToBytes([1, 2, 3])));
+  });


What about null or invalid inputs?

martinthomson · 2019-02-12T03:54:39Z

test/utils.js

+        assert.equal(contentsBuf.length(), 2);
+        contentsBuf.readUint24();
+      });
+    }, TLSError, /DECODE_ERROR/);


Is a regex necessary? Can this be a more specific test?

This is mostly just using the existing convenience API of assert.throws; I could instead check for a specific numeric value of the alert description, if that seems better?

martinthomson · 2019-02-12T03:58:44Z

test/recordlayer.js

+
+  it('prevents wrapping of the sequence number', async () => {
+    await cs.setKey(zeros(32), ['encrypt', 'decrypt']);
+    cs.seqnum = Math.pow(2, 24) - 1;


Magic number. Make a constant or share the one you have.

martinthomson · 2019-02-12T05:23:45Z

test/tlsconnection.js

+              }, TLSError, 'CLOSE_NOTIFY');
+            });
+
+            describe('is able to send an explicit close in return, and then', () => {


You don't seem to have a separate case for server.close() without having first received a close.

martinthomson · 2019-02-12T05:24:30Z

test/tlsconnection.js

+              }, TLSError, 'CLOSE_NOTIFY');
+            });
+
+            describe('is able to send an explicit close in return, and then', () => {


can you test for sending the close without having received the close first?

martinthomson · 2019-02-12T05:26:44Z

test/tlsconnection.js

+      content: await testHelpers.makeServerHelloMessage({
+        extensions: [
+          testHelpers.makePreSharedKeyExtension(1)
+        ]


Maybe add version: 0x0304 to this, maybe as a supplementary test, so that you catch the case where the version field in the SH is used to drive version negotiation. (Not that your code is at all vulnerable to this currently, but it's a good paranoia check.)

To be clear, the handshake should fail if SH.version is 0x0304, right?

Correct. Fail.

martinthomson · 2019-02-12T05:28:40Z

test/tlsconnection.js

+    }, TLSError, 'DECODE_ERROR');
+  });
+
+  it('errors if ServerHello and EncryptedExtensions appear in the same record', async () => {


Do you have a test for a partial record being present after the ServerHello or Finished? I see that you have the code for checking this, but I can't find the test.

No, I don't believe I do - perhaps that check is being exercised accidentally by some other test. I'll add one.

martinthomson · 2019-02-12T05:31:36Z

test/tlsconnection.js

+      server._keyschedule.serverApplicationTrafficSecret,
+      client._keyschedule.serverApplicationTrafficSecret
+    ));
+  });


Can you add tests here for the promise I talked about.

Also, can you add a test that includes calls to send(). It would be good if you could also test that you can close in one direction then continue sending and receiving in the other direction.

vladikoff · 2019-02-12T16:39:43Z

Running npm install;npm test produces changes to checked-in code. That's not cool

In my experience, at this point npm install cannot be trusted to properly use package-lock, it just keeps fetching new things. We should strictly instruct to use the npm ci command, that one should never produce changes to the checked-in code.

vladikoff · 2019-02-13T12:03:28Z

src/extensions.js

+//
+// Extension parsing.
+//
+// This file contains some helpers reading/writing the various kinds


some helpers reading/writing -> some helpers for reading/writing ?

also this comment didn't make it to the final bundle, need to regenerate the build

rfk · 2019-02-13T12:03:39Z

Thanks for the detailed feedback @martinthomson, I've updated based on your review with the following key points:

Extension parsing has been moved into a separate file and helper classes. This involved refactoring a little bit of the logic of what validation checks live where, but I think the end result is much more easily understood in aggregate.
Addition on the connected promise to resolve/reject on handshake completion, and blocking of sends until it resolves.
Addition of an explicit TLSCloseNotify exception to make it easier to detect close, and some modifications of the websocket wrapper to deal with it appropriately.
More careful handling of change_cipher_spec, and support for cleanly ignoring NewSessionTicket.
Split CipherState into independent DecryptionStateandEncryptionState` classes
The various extra tests you suggested

The diff from the version you reviewed is here, and the latest coverage report is here for completeness.

vladikoff

I got into a Error: TLS Alert: DECODE_ERROR (50) with bce404a545165a791ebcd7d1ca984bd9861120ee, trying to it track down, probably incompat with the Firefox build.

vladikoff · 2019-02-13T12:25:12Z

.circleci/config.yml

@@ -21,6 +21,9 @@ jobs:
            npm install


let's try to use npm ci here

vladikoff · 2019-02-13T12:25:31Z

src/extensions.js

+//
+// Extension parsing.
+//
+// This file contains some helpers reading/writing the various kinds


also this comment didn't make it to the final bundle, need to regenerate the build

vladikoff · 2019-02-13T12:27:06Z

README.md

@@ -8,19 +8,15 @@ It will be used by the Firefox Accounts pairing flow, with one side
 of the channel being web content from https://accounts.firefox.com and


let's add a doc about running npm ci and npm run build to ensure the build is consistent and no new sub-deps show up. Maybe we can do it as a separate PR.

Maybe this should be a pre-commit hook or something? (TBH it kind of weirds me out checking in the build output on every commit, and I wonder if we can only do it on releases or something; but if we're going to do it we should be able to automate it)

pre-commit hooks are local only, you want something in CI ideally

vladikoff · 2019-02-13T12:34:29Z

src/states.js

+  async sendApplicationData(bytes) {
+    throw this.error;
+  }
+  async sendAlertMessage(err) {


is this still used , needed ? I think we use _sendAlertMessage in other places

vladikoff · 2019-02-13T12:36:36Z

src/index.js

+      if (! this._peerClosed) {
+        this.dispatchEvent(new CustomEvent('error', {
+          detail: {
+            error: new Error('WebSocket unexepectedly closed'),


unexepectedly --> unexpectedly

vladikoff · 2019-02-13T12:37:48Z

test/recordlayer.js

+        assert.equal(rl._recvDecryptState, null);
+      });
+
+      it('will send encypted handshake records', async () => {


encrypted

vladikoff · 2019-02-13T12:37:51Z

test/recordlayer.js

+        assert.equal(type, 22);
+      });
+
+      it('will send encypted application data records', async () => {


encrypted

vladikoff · 2019-02-13T12:38:16Z

src/recordlayer.js

+    if (this._recvError !== null) {
+      throw this._recvError;
+    }
+    // For simplicity, we assume that the given data conatins exactly one record.


contains ?

vladikoff · 2019-02-13T12:38:38Z

src/states.js

+    const keyschedule = this.conn._keyschedule;
+    const transcript = keyschedule.getTranscript();
+    // Calculate size occupied by the PSK binders.
+    let pskBindersSize = 2; // Vector16 represntation overhead.


representation ?

vladikoff · 2019-02-13T12:39:15Z

test/tlsconnection.js

+    // keyshares, optional extensions, and other exciting features that we do not support.
+    const data = await server.recv(TEST_VECTORS.EXTENDED_CLIENT_HELLO);
+    assert.equal(data, null);
+    // It sends ServerHello, ChangeCipherSpec, EncyptedExtensions+Finished in response.


EncryptedExtensions ?

martinthomson

I'm done with this.

Thanks for catching a few extra things with the new code.

martinthomson · 2019-02-14T03:35:03Z

src/keyschedule.js

+//       |
+//       | addECDHE()
+//       v
+//   HANDSHAKE_SECRET


This makes me think. Can you null-out the old secrets as you go, so that we get something like post-compromise security?

martinthomson · 2019-02-14T03:40:20Z

src/recordlayer.js

    const buf = this._pendingRecordBuf;
-    const type = this._pendingRecordType;
+    let type = this._pendingRecordType;
    if (! type) {
      return;


Might pay to assert that buf.byteLength === 0 here too.

martinthomson · 2019-02-14T03:42:14Z

src/recordlayer.js

+    //
+    // The additional data for the decryption is the `TLSCiphertext` record
+    // header, which is a fixed size and at the start of the buffer.
+    buf.seek(0);


This breaks encapsulation a little, but I think that it's OK.

martinthomson · 2019-02-14T03:43:21Z

src/states.js

-        this.conn._closeForRecv();
-        break;
+        this.conn._closeForRecv(alert);
+        throw alert;


I'm not so sure about throwing when a close alert is received. It's not an error condition. Is the problem here that you don't have an alternative means of signaling a close from the peer?

Basically, yes. I think a better approach is probably to shuffle the API to use an EventEmitter style, so it can emit a close event in this case, which is basically what the higher-level PairingChannel abstraction does anyway. But it was a bigger refactor than I wanted to take on inline in this PR.

martinthomson · 2019-02-14T03:46:50Z

src/states.js

+}
+
+// A base class for states that occur in the middle of the handshake
+// (that is, between ClientHello and Finished).  These states may receive


CCS appears between ClientHello and Finished. CCS also happens between CCS and EncryptedExtensions, if you request - as you do - compatibility mode.

I wonder if you shouldn't have an CLIENT_WAIT_CCS state immediately after handling the ServerHello then.

martinthomson · 2019-02-14T04:04:47Z

test/tlsconnection.js

+    const CLIENT_SENT = [], SERVER_SENT = [];
+    const server = await ServerConnection.create(TEST_VECTORS.PSK, TEST_VECTORS.PSK_ID, data => SERVER_SENT.push(data));
+    const client = await ClientConnection.create(TEST_VECTORS.PSK, TEST_VECTORS.PSK_ID, data => CLIENT_SENT.push(data));
+    await server.recv(CLIENT_SENT[0]);


martinthomson · 2019-02-14T04:04:56Z

test/tlsconnection.js

+    const server = await ServerConnection.create(TEST_VECTORS.PSK, TEST_VECTORS.PSK_ID, data => SERVER_SENT.push(data));
+    const client = await ClientConnection.create(TEST_VECTORS.PSK, TEST_VECTORS.PSK_ID, data => CLIENT_SENT.push(data));
+    await server.recv(CLIENT_SENT[0]);
+    await client.recv(SERVER_SENT[0]);


martinthomson · 2019-02-14T04:05:22Z

test/tlsconnection.js

+    const client = await ClientConnection.create(TEST_VECTORS.PSK, TEST_VECTORS.PSK_ID, data => CLIENT_SENT.push(data));
+    await server.recv(CLIENT_SENT[0]);
+    await client.recv(SERVER_SENT[0]);
+    await client.recv(SERVER_SENT[1]);


// EE+Finished

I'll add comments, but for completeness, this is ChangeCipherSpec...

martinthomson · 2019-02-14T04:05:37Z

test/tlsconnection.js

+    await server.recv(CLIENT_SENT[0]);
+    await client.recv(SERVER_SENT[0]);
+    await client.recv(SERVER_SENT[1]);
+    await client.recv(SERVER_SENT[2]);


?? Finished?

NSS bundles everything together, so I'm unused to multiple messages.

...and this is EE+Finished.

martinthomson · 2019-02-14T04:06:05Z

test/tlsconnection.js

+    await client.recv(SERVER_SENT[0]);
+    await client.recv(SERVER_SENT[1]);
+    await client.recv(SERVER_SENT[2]);
+    await server.recv(CLIENT_SENT[1]);


// Client Finished

rfk · 2019-02-14T04:27:00Z

Thanks so much @martinthomson!

vladikoff

I've used the latest SHA to produce a firefox build + content server, and everything seems to be working!
🎉 🎉 🎉 🎉 🎉 🎉

One nit: on a Mac I did get a diff with a line ending and a js-e file created due to sed substitution we are doing:

Fix leftover review comments from #24

rfk mentioned this pull request Jan 18, 2019

Add basic TLS1.3-PSK logic #20

Closed

rfk mentioned this pull request Jan 22, 2019

Some build fixes #25

Merged

rfk force-pushed the tls13-psk-mode branch from dae32ce to ce358f1 Compare January 22, 2019 10:55

rfk force-pushed the tls13-psk-mode branch from ce358f1 to 8b8b18b Compare January 23, 2019 02:03

rfk force-pushed the tls13-psk-mode branch 2 times, most recently from 705de41 to 7fa784c Compare January 24, 2019 05:27

eoger changed the base branch from trim-down-insecure-code to master January 24, 2019 17:06

martinthomson reviewed Jan 25, 2019

View reviewed changes

rfk force-pushed the tls13-psk-mode branch 3 times, most recently from e3d28a2 to 9d7f09b Compare January 25, 2019 06:32

vladikoff reviewed Jan 25, 2019

View reviewed changes

shane-tomlinson added train-131 waffle:active labels Feb 4, 2019

vladikoff assigned rfk Feb 4, 2019

rfk force-pushed the tls13-psk-mode branch from 56a0601 to 43d42c4 Compare February 8, 2019 04:44

rfk commented Feb 11, 2019

View reviewed changes

shane-tomlinson added the P1 label Feb 11, 2019

martinthomson reviewed Feb 12, 2019

View reviewed changes

rfk commented Feb 12, 2019

View reviewed changes

martinthomson reviewed Feb 12, 2019

View reviewed changes

vladikoff mentioned this pull request Feb 13, 2019

fix(pairing): conver to Uint8Array for pairing channel compatibility mozilla/fxa-content-server#6958

Merged

rfk force-pushed the tls13-psk-mode branch from 172b345 to bce404a Compare February 13, 2019 11:56

vladikoff reviewed Feb 13, 2019

View reviewed changes

rfk force-pushed the tls13-psk-mode branch from dae9d28 to fbde0b4 Compare February 13, 2019 22:05

martinthomson approved these changes Feb 14, 2019

View reviewed changes

vladikoff approved these changes Feb 14, 2019

View reviewed changes

Add initial TLS1.3 PSK-mode implementation.

fea659d

rfk force-pushed the tls13-psk-mode branch from fbde0b4 to fea659d Compare February 14, 2019 20:57

rfk merged commit 57146da into master Feb 14, 2019

rfk added a commit that referenced this pull request Feb 20, 2019

Fix leftover review comments from #24

7494f67

rfk mentioned this pull request Feb 20, 2019

Fix leftover review comments from #24 #33

Merged

rfk added a commit that referenced this pull request Feb 20, 2019

Merge pull request #33 from mozilla/mt-review-fixups; r=vladikoff

0a5dcc5

Fix leftover review comments from #24

		@@ -8,19 +8,15 @@ It will be used by the Firefox Accounts pairing flow, with one side
		of the channel being web content from https://accounts.firefox.com and

Add preliminary TLS1.3 PSK-mode implementation. #24

Add preliminary TLS1.3 PSK-mode implementation. #24

Conversation

rfk commented Jan 18, 2019 • edited Loading

vladikoff commented Jan 18, 2019 • edited Loading

vladikoff commented Jan 18, 2019

rfk commented Jan 22, 2019

eoger commented Jan 23, 2019

rfk commented Jan 23, 2019

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Jan 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Feb 6, 2019

rfk commented Feb 8, 2019

Choose a reason for hiding this comment

rfk commented Feb 11, 2019

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

rfk Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladikoff commented Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Feb 13, 2019

vladikoff left a comment

rfk commented Jan 18, 2019 •

edited

Loading

vladikoff commented Jan 18, 2019 •

edited

Loading

rfk Feb 12, 2019 •

edited

Loading

rfk Feb 12, 2019 •

edited

Loading

vladikoff commented Feb 12, 2019 •

edited

Loading