Check the protocol state machine

claucece commented 6 years ago

Why

As we are currently implementing the revision number 2 of the OTRv4 specification, we need to include a consistent way of using the protocol state machine.

Reference

Please, refer to the "The protocol state machine" section of the OTRv4 spec.

Tasks

[x] Check that all of the states are correctly set.
[x] Check that all protocol events match the otrv4 spec.
[x] Check the receiving and sending of TLV type 1.
[x] Check issue otrv4/otrv4#106 in the OTRv4 specification.

Open questions

None.

claucece commented 6 years ago

@deniscostadsc I saw you assigned this. Thanks for that. Do you have any question around this issue?

deniscostadsc commented 6 years ago

@claucece hi. Yeah I had questions, but there is nothing more to check on out-of-order... :+1:

:heart:

deniscostadsc commented 6 years ago

Check that all of the states are correctly set. Check that all protocol events match the otrv4 spec.

Does this mean all transitions should be checked?

claucece commented 6 years ago

Does this mean all transitions should be checked?

Already answered but just for the record, yes. :)

deniscostadsc commented 6 years ago

Check the receiving and sending of TLV type 1.

Sending TLV type 1, in all states, transition state machine to START state. This is pretty clear on the Sate Machine section[1]. But is not clear for me if receiving TLV type 1, transition state machine to FINISHED state no matter which state it was before[2]. It's clear that happens when the participant receives a TLV type 1 when in ENCRYPTED state.

That stated, how should state machine bahave when receiving TLV type 1 in all state? Not sure this question make sense.

Should I add test (or check if there are tests) for every STATE sending and receiving TLV type 1?

[1] - https://github.com/otrv4/otrv4/blob/master/otrv4.md#protocol-states [2] - https://github.com/otrv4/otrv4/blob/master/otrv4.md#user-requests-to-end-an-otr-conversation

claucece commented 6 years ago

But is not clear for me if receiving TLV type 1, transition state machine to FINISHED state no matter which state it was before.

I'm just answering for documentation, as we already talked about this :) A TLV type 1 is sent attached to a data message, and it is only sent over ENCRYPTED_MESSAGES state, as well as received. If it is received in any other state (prob because an out-of-order happened), as data message is either ignored or queued for later encryption. The protocol is not clear around this.

Should I add test (or check if there are tests) for every STATE sending and receiving TLV type 1?

No. But we should definitely check the queuing over issue #79

deniscostadsc commented 6 years ago

I'll replicate here all points on otrv4/otrv4#106, in order to check them on libotr-ng's code.

[x] On "Protocol states" section, what happens when a TLV type 1 (Disconnected) message is received? This box suggests that we transition to START, but also to FINISHED.
[x] On "Protocol events" section, who initializes the protocol with the allowed versions?
[x] On "Protocol events" > "Query Messages" section, why is it required that users can choose which versions to support?
[x] On "Protocol events" > "Query Messages" section, some information from the version string section is duplicated here. Consider reusing / referencing it.
[x] On "Protocol events" section, what happens if a single message contains multiple valid query strings and whitespace tags?
[x] On "Protocol events" > "Receiving plaintext without the whitespace tag" section, this section header should also exclude query messages.
[x] On "Protocol events" > "Sending an encrypted data message" section, "The ENCRYPTED_MESSAGES state is the only state where a participant is allowed to send encrypted data messages" except when sending offline messages.
[x] On "Protocol events" > "Receiving an encrypted data message" section, if possible, include a nested diagram showing the nested structure of a message. This should help to clarify the protocol. Something like this: ["?OTR" [ message [ TLV ] ] ]
[x] On "Protocol events" > "Receiving an encrypted data message" section, are there any TLV types other than 1 (Disconnected) that can be nested inside an encrypted message in this way?
[x] On "Protocol events" > "Receiving an encrypted data message" section, if a message is received, it is valid, and it can be decrypted, then the spec says to send a heartbeat if no message has been sent in some configurable length of time. Shouldn't this heartbeat happen all the time (not just after receiving a message)?
[x] On "Protocol events" > "Receiving an Error Message" section, the last part here about OTRv3 is not clear.
[x] On "Protocol events" > "Receiving a TLV type 1 (Disconnected) Message" section, does this refer to receiving a TLV type 1 message outside of an encrypted data message? If so, how does this reaction differ from the one described in the "Receiving an encrypted data message" section?
[x] On "Considerations for networks that allow multiple devices", "Doing so allows establishing an OTR channel at the same time with multiple devices from the other participant". Should this be "establishing multiple OTR channels"?

deniscostadsc commented 6 years ago

On "Protocol states" section, what happens when a TLV type 1 (Disconnected) message is received? This box suggests that we transition to START, but also to FINISHED.

We already saw this. When TLV type 1 is received the state machine transition to FINISHED.

deniscostadsc commented 6 years ago

On "Protocol events" section, who initializes the protocol with the allowed versions? On "Protocol events" > "Query Messages" section, why is it required that users can choose which versions to support? On "Protocol events" > "Query Messages" section, some information from the version string section is duplicated here. Consider reusing / referencing it.

From what I understood here, those points are related only to allowed version. So they are not applicable to this issue.

deniscostadsc commented 6 years ago

On "Protocol events" section, what happens if a single message contains multiple valid query strings and whitespace tags?

Based on this comment, I think there is nothing to check here.

deniscostadsc commented 6 years ago

On "Protocol events" > "Receiving plaintext without the whitespace tag" section, this section header should also exclude query messages.

I think this item is not applicable to this issue.

deniscostadsc commented 6 years ago

On "Protocol events" > "Sending an encrypted data message" section, "The ENCRYPTED_MESSAGES state is the only state where a participant is allowed to send encrypted data messages" except when sending offline messages.

This is only a matter of possible misunderstanding.

On "Protocol events" > "Receiving an encrypted data message" section, if possible, include a nested diagram showing the nested structure of a message. This should help to clarify the protocol. Something like this: ["?OTR" [ message [ TLV ] ] ]

This will not interfere on the state machine behavior.

deniscostadsc commented 6 years ago

On "Protocol events" > "Receiving an encrypted data message" section, are there any TLV types other than 1 (Disconnected) that can be nested inside an encrypted message in this way?

@claucece, should I create tests that send different TLVs in order to test this?

deniscostadsc commented 6 years ago

On "Protocol events" > "Receiving an encrypted data message" section, if a message is received, it is valid, and it can be decrypted, then the spec says to send a heartbeat if no message has been sent in some configurable length of time. Shouldn't this heartbeat happen all the time (not just after receiving a message)?

This won't change state machine behavior.

deniscostadsc commented 6 years ago

On "Protocol events" > "Receiving an Error Message" section, the last part here about OTRv3 is not clear.

Not applicable to this issue.

deniscostadsc commented 6 years ago

On "Protocol events" > "Receiving a TLV type 1 (Disconnected) Message" section, does this refer to receiving a TLV type 1 message outside of an encrypted data message? If so, how does this reaction differ from the one described in the "Receiving an encrypted data message" section?

I already discussed this with @claucece, and a correct implementation of the protocol won't send TLV type 1 outside ENCRYPTED state.

deniscostadsc commented 6 years ago

On "Considerations for networks that allow multiple devices", "Doing so allows establishing an OTR channel at the same time with multiple devices from the other participant". Should this be "establishing multiple OTR channels"?

Not sure how this implicates this issue. From what understood, The state machine won't change its behavior if the client is communicating with different devices. But, I'm not sure. Can you help me on this @claucece.

deniscostadsc commented 6 years ago

@claucece, are you satisfied with this point:

Check that all protocol events match the otrv4 spec.

I'm not sure what else should I verify. You can describe here what else is missing and I'll work on it.

olabini commented 6 years ago

About TLVs - I need to clarify a few things:

ANY TLV can be sent in any Data Message. In fact, there is no other way for TLVs to be sent, and in theory, any combination of TLVs can come in the same Data Message.
A Data Message is always encrypted. If the receiver is not in the encrypted state, it is impossible for the receiver to decrypt the Data Message. So if that happens, the receiver has no way of knowing that the Data Message contains TLVs.

olabini commented 6 years ago

About multiple channels - if I understand this point correctly, the idea is that it is exactly in the protocol state machine that we need to keep track of when messages are received for other OTR channels, for example.

olabini commented 6 years ago

I don't really understand the heartbeat comment.

claucece commented 6 years ago

Hey, thanks @deniscostadsc and @olabini for looking into this ;)

Ok, for clarification. Denis copied the questions sent by Nik around the protocol state machine (stated on issue ##106 of the OTRv4 spec). Some of these questions don't really apply to the library as they were more around wording around the spec. But some of them do.

About multiple channels:

On "Considerations for networks that allow multiple devices", "Doing so allows establishing an OTR channel at the same time with multiple devices from the other participant". Should this be "establishing multiple OTR channels"?

Not sure how this implicates this issue. From what understood, The state machine won't change its behavior if the client is communicating with different devices. But, I'm not sure. Can you help me on this @claucece.

About multiple channels - if I understand this point correctly, the idea is that it is exactly in the protocol state machine that we need to keep track of when messages are received for other OTR channels, for example.

This section refers to section "Considerations for Networks that allow Multiple Devices". Previously, on the spec it was stated that we could establish one OTR channel at the same time with multiple devices from the other participant, when actually you establish multiple OTR channels. This can be checked over the library by for example, starting OTR conversations from different devices (instance tags), and actually seeing how multiple OTR channels start. Check, for example, this test: https://github.com/otrv4/libotr-ng/blob/master/src/test/test_api.c#L861

About heartbeat messages:

On "Protocol events" > "Receiving an encrypted data message" section, if a message is received, it is valid, and it can be decrypted, then the spec says to send a heartbeat if no message has been sent in some configurable length of time. Shouldn't this heartbeat happen all the time (not just after receiving a message)?

This won't change state machine behavior.

I don't really understand the heartbeat comment.

Well, we have our own issue to actually check heartbeat messages (as the current implementation does not work with any client), in #49 . The idea is that, if you haven't sent a message to a participant on some time, you send a heartbeat. But you can send them everywhere not only when you receive one data message from the other participant.

claucece commented 6 years ago

On "Protocol events" > "Sending an encrypted data message" section, "The ENCRYPTED_MESSAGES state is the only state where a participant is allowed to send encrypted data messages" except when sending offline messages.

This is only a matter of possible misunderstanding.

Mmm.. actually, no. When attaching a data message to the Non-Interactive auth (as per #68), you send an encrypted message on state START.

On "Protocol events" > "Receiving an encrypted data message" section, are there any TLV types other than 1 (Disconnected) that can be nested inside an encrypted message in this way?

@claucece, should I create tests that send different TLVs in order to test this?

Mmm.. we already have a bunch of them. ;) Check this one, for example, https://github.com/otrv4/libotr-ng/blob/master/src/test/test_api.c#L985

Thanks, @olabini for the TLV clarification. There is a diagram of how that looks on the spec:

    ["?OTR" || protocol version || message type || sender's instance_tag ||receiver's instance tag ||
    flags || previous chain message number || ratchet id || message id || public ECDH key ||
    public DH key || nonce || enc(plaintext message || TLV) || authenticator ||
    old MAC keys to be revealed ]

claucece commented 6 years ago

@claucece, are you satisfied with this point: Check that all protocol events match the otrv4 spec. I'm not sure what else should I verify. You can describe here what else is missing and I'll work on it.

Mmm, we can check on Monday. Wanna pair?

claucece commented 6 years ago

On "Protocol events" section, who initializes the protocol with the allowed versions? On "Protocol events" > "Query Messages" section, why is it required that users can choose which versions to support? On "Protocol events" > "Query Messages" section, some information from the version string section is duplicated here. Consider reusing / referencing it.

From what I understood here, those points are related only to allowed version. So they are not applicable to this issue.

Yes. But we can check, if you want @deniscostadsc , how versions are set (over the query messages, the user profile, etc) ;)

deniscostadsc commented 6 years ago

About multiple channels:

On "Considerations for networks that allow multiple devices", "Doing so allows establishing an OTR channel at the same time with multiple devices from the other participant". Should this be "establishing multiple OTR channels"?

Not sure how this implicates this issue. From what understood, The state machine won't change its behavior if the client is communicating with different devices. But, I'm not sure. Can you help me on this @claucece.

About multiple channels - if I understand this point correctly, the idea is that it is exactly in the protocol state machine that we need to keep track of when messages are received for other OTR channels, for example.

This section refers to section "Considerations for Networks that allow Multiple Devices". Previously, on the spec it was stated that we could establish one OTR channel at the same time with multiple devices from the other participant, when actually you establish multiple OTR channels. This can be checked over the library by for example, starting OTR conversations from different devices (instance tags), and actually seeing how multiple OTR channels start. Check, for example, this test: https://github.com/otrv4/libotr-ng/blob/master/src/test/test_api.c#L861

You are totally right. And it tests the state machine in this situation. So, I think I can check this item.

deniscostadsc commented 6 years ago

On "Protocol events" > "Receiving an encrypted data message" section, if a message is received, it is valid, and it can be decrypted, then the spec says to send a heartbeat if no message has been sent in some configurable length of time. Shouldn't this heartbeat happen all the time (not just after receiving a message)?

This won't change state machine behavior.

I don't really understand the heartbeat comment.

Well, we have our own issue to actually check heartbeat messages (as the current implementation does not work with any client), in #49 . The idea is that, if you haven't sent a message to a participant on some time, you send a heartbeat. But you can send them everywhere not only when you receive one data message from the other participant.

Then I'll keep this item checked.

deniscostadsc commented 6 years ago

On "Protocol events" > "Receiving an encrypted data message" section, are there any TLV types other than 1 (Disconnected) that can be nested inside an encrypted message in this way?

@claucece, should I create tests that send different TLVs in order to test this?

Mmm.. we already have a bunch of them. ;) Check this one, for example, https://github.com/otrv4/libotr-ng/blob/master/src/test/test_api.c#L985

Ok I'll check this item.

olabini commented 6 years ago

OK, cool. I do think that the heartbeat stuff needs some more consideration. Maybe I'm misunderstanding, but it sounds like you're describing something that is different from how previous OTR versions function. (Fundamentally, we NEVER send messages without interaction from the user of the library - so, a heartbeat will happen when we receive a message, and we haven't sent a message for N s where N is the timeout)

claucece commented 6 years ago

OK, cool. I do think that the heartbeat stuff needs some more consideration. Maybe I'm misunderstanding, but it sounds like you're describing something that is different from how previous OTR versions function. (Fundamentally, we NEVER send messages without interaction from the user of the library - so, a heartbeat will happen when we receive a message, and we haven't sent a message for N s where N is the timeout)

Yes.. we have an issue for the heartbeat as the current implementation is just not working. It was more a matter of wording on the spec.

olabini commented 6 years ago

Ah, ok - I see.

claucece commented 6 years ago

@juniorz for Q/A

otrv4 / libotr-ng