Introduce dtls connection id. #857

boaks · 2019-01-25T19:52:47Z

Add also Bytes for byte array based ids. Redesign connection store. Use
connection id as identifier to support address changes.

Signed-off-by: Achim Kraus [email protected]

boaks · 2019-01-28T17:47:04Z

I moved some lines into much smaller PR and marked them with "merged soon".

sbernard31 · 2019-02-04T16:07:28Z

Should I review this one ? or should we plan more split ?

boaks · 2019-02-04T16:27:52Z

At least I rebase and retest it ... split may be ... so review not today :-)

boaks · 2019-02-06T06:44:28Z

I extended my "integration tests" in branch "wip_add_dtls_cid", commit 887de52 with resumption handshakes (currently not pushed to the remote repo) and adjusted some code. There is still more work to be done ... I guess, this PR and PR #868 will be adapted again over the weekend and next Monday.

boaks · 2019-02-11T17:22:58Z

If possible PR #868 this week, and this PR next week :-).
There are only very small part (e.g. rename of Record length to fragmentLength), which may be split. In my experience, that doesn't pay-off.

boaks · 2019-02-18T08:18:40Z

It would great, if this PR could be merged this week.

sbernard31 · 2019-02-19T10:59:54Z

scandium-core/src/main/java/org/eclipse/californium/scandium/DTLSConnector.java

+			if (cidLength != null && cidLength > 0 && cidLength != connectionStore.getConnectionIdLength()) {
+				throw new IllegalArgumentException("Connection store must use the same cid length! " + cidLength
+						+ " != " + connectionStore.getConnectionIdLength());
+			}


If I well understand, to set ConnectionIdLength, I need to set it in DtlsConnectorConfig ? but also in connectionStore (in case of custom store) ?

Should we avoid to set this information twice ?
I see cidlenght is only needed to newConnectionId which is only called by put which is only called by DTLSconnector ?

Maybe we can change the API with something like : boolean put(Connection connection, integer cidlenght);

but also in connectionStore (in case of custom store) ?

I assume, there will be only rare cases, when a custom store is used. But if, the same cid length must be used as for the Record.fromByteArray. I would prefer to keep the configuration values in the DtlsConnectorConfig(at least as leading definition ).

So together:

configuration value in DtlsConnectorConfig

rarely use custom store

I think, add it to the javadoc of the DTLSConnector constructor will do it best.

Just in case there is misunderstanding, I would keep it DtlsconnectorConfig but remove it from connectionStore.

Remove what? The method getConnectionIdLength? Or the field cidLength?
My assumption was, that a custom connection store is rarely used, so I don't think, that spending time into that will payout.

like express above :

I see cidlenght is only needed to newConnectionId which is only called by put which is only called by DTLSconnector ?

Maybe we can change the API with something like : boolean put(Connection connection, integer cidlenght);

The constructor also calls newConnectionId, if a ClientSessionCache is restored.
If you really feel, it's worth, then introduce the DtlsConnectorConfig into the parameters of the constructor and replace the others. But please, shift that into an other PR :-).

I missed it was used in the constructor too.
So not so easy to change it.

sbernard31 · 2019-02-19T11:12:29Z

scandium-core/src/main/java/org/eclipse/californium/scandium/DTLSConnector.java

@@ -869,6 +932,12 @@ private void processRecord(Record record, Connection connection) {
 			LOGGER.info("Unexpected error occurred while processing record from peer [{}]",
 					record.getPeerAddress(), e);
 			terminateConnection(connection, e, AlertLevel.FATAL, AlertDescription.INTERNAL_ERROR);
+		} catch (GeneralSecurityException e) {
+			LOGGER.info("error occurred while processing record from peer [{}]",
+					record.getPeerAddress(), e);


I don't really know when those 2 exceptions are raised but is it normal we don't terminate the connection ?

That's the receiving part, so I would not terminate the connection, because this may by used as attack vector. That's somehow the opposite of the catch RuntimeException in receiveNextDatagramFromNetwork. But this PR is more about introducing the cid, than the cleanup of exception handling. I tried to keep it as I found it, may be it gets cleaned up later in an other PR, if we find that it must be improved.

sbernard31 · 2019-02-19T13:17:13Z

scandium-core/src/main/java/org/eclipse/californium/scandium/dtls/Record.java

@@ -73,6 +73,8 @@
 	public static final int SEQUENCE_NUMBER_BITS = 48;

 	public static final int LENGTH_BITS = 16;
+


I don't see it in github but in eclipse I see unwanted whitespaces ?

sbernard31 · 2019-02-19T14:57:05Z

scandium-core/src/main/java/org/eclipse/californium/scandium/dtls/Record.java

-		this.sequenceNumber = sequenceNumber;
-		if (session != null && session.getWriteState() != null && epoch > 0) {
-			fragmentBytes = encryptFragment(fragment.toByteArray());
+		if (this.sequenceNumber != sequenceNumber) {


You faced situation where this.sequenceNumber == sequenceNumber ?

No, I haven't seen it.

Ok so you add this test just in case ?

Yes, in the case, that some calls it without changing it. FMPOV, protecting such a method, which may require more resources, is not a bad practice.

sbernard31 · 2019-02-19T15:12:05Z

scandium-core/src/main/java/org/eclipse/californium/scandium/dtls/Record.java

+				byte[] byteArray = fragment.toByteArray();
+				byteArray = applyInnerTypeAndPadding(byteArray);
+				fragmentLength = byteArray.length;
+				fragmentBytes = encryptFragment(byteArray);


it seems pretty much the same idea that setFragment.
I don't know if this is worthy to factorize this code ?
(I don't like so much those setters which are doing more than it looks like :))

FMPOV, I don't like a setter, which then have something "always call additionally" in the javadoc.
Before the change, encryptFragment was called, now with the new record type, the inner type and padding must also be applied. The grants before this change was, that the new sequence number also triggers a new encryption, because the explicit nonce is changed. Now the same grants as before are kept.

So should we go for rename the setter to update?

FMPOV, I don't like a setter, which then have something "always call additionally" in the javadoc.

It's not better I agree.

So you're right it's just a naming issue, update sounds good.

But you didn't answer to the first question : it seems pretty much the same idea that setFragment.
I don't know if this is worthy to factorize this code ?

worthy to factorize this code

If I would guess, what you have in mind ...
Do you want the two 4-lines snippet to be merged into a single function?

byte[] byteArray = fragment.toByteArray(); // the current length of the unprotected message // this value is needed to generate the additional data when using AEAD fragmentLength = byteArray.length;

Verifying the comment, I found that it's not (longer?) valid. I would prefer to clean it up with the commit of the wip adding more cipher suites.

See e4df69e

I see, this would be changed in next PR

sbernard31 · 2019-02-19T15:26:01Z

scandium-core/src/main/java/org/eclipse/californium/scandium/DTLSConnector.java

+					return;
+				} else {
+					record.setSession(session);
+					record.getFragment();


A bit disturbed about a "getter call" to execute some inner calculation.

This PR doesn't change that.
The new record_type with the "encrypted inner type" just requires, that it is called before that real type is used to determine the record type for processing.

This PR doesn't change that.

I agree. Here this is just more visible as we just call a getter without really use the result.

The new record_type with the "encrypted inner type" just requires, that it is called before that real type is used to determine the record type for processing.

If we don't want do change the idea (I mean a getter which do some processing), maybe this would be more consistent to just "extract the fragment" on getType() ?

If you check, where "getType()" is called, you will see the answer.
The inner/outer is a little shitty, but for now, "it works".
I added to comment to give a hint about the intention.

sbernard31 · 2019-02-19T16:00:15Z

scandium-core/src/main/java/org/eclipse/californium/scandium/dtls/ConnectionIdExtension.java

+		int length = id.length();
+		writer.write(1 + length, LENGTH_BITS);
+		writer.write(length, CID_FIELD_LENGTH_BITS);
+		writer.writeBytes(id.getBytes());


This is an arbitrary way to encode CID ?
I mean there is nothing which say that first byte is the length and next byte are CID ?
We could have used fixed size CID right ?

I mean there is nothing which say that first byte is the length and next byte are CID ?

The draft encodes the cid different in the hello extension and the new record type.
In the extension with leading uint8 length, and opaque bytes, in the new record type only the opaque bytes. That's specified by the draft.

Anyway the draft allows you to be more flexible as my current implementation. The draft specifies the cid similar to the coap-token. If one peer send the cid in the hello extension, then the other, if agreed, sent that cid in the records of the next epoch. If a peer uses different sized cids, it must do it in a proper way, so that the peer will be able to read them back.

Ok I missed the meaning of https://tools.ietf.org/html/rfc5246#section-4.3 notation

sbernard31 · 2019-02-19T16:19:47Z

scandium-core/src/main/java/org/eclipse/californium/scandium/dtls/ConnectionIdExtension.java

+					"Connection id length too large! 255 max, but has " + (extensionData.length - 1),
+					new AlertMessage(AlertLevel.FATAL, AlertDescription.ILLEGAL_PARAMETER, peerAddress));
+		}
+		int len = extensionData[0];


This is the extension length not the CID length ? right ?

That's the cid length.
The extension length is encoded as uint16 and is decoded in
HelloExtensions.fromByteArray.

I get it now.

boaks · 2019-02-20T15:11:41Z

Let me propose to shift the renames (set => update) to a later PR, which only contains that renames.

Improve dtls communication for environments with unaware address changes (e.g. NATs). Signed-off-by: Achim Kraus <[email protected]>

Signed-off-by: Achim Kraus <[email protected]>

sbernard31 · 2019-02-22T09:23:36Z

Let me propose to shift the renames (set => update) to a later PR, which only contains that renames.

👍
(you can merge it)

boaks · 2019-02-22T09:27:31Z

Some cosmetic.

boaks force-pushed the add_connection_id branch from 8a25154 to 88d7496 Compare January 27, 2019 15:52

boaks mentioned this pull request Jan 28, 2019

Add Bytes. #859

Merged

boaks force-pushed the add_connection_id branch 5 times, most recently from f026b4e to 0baafdf Compare February 3, 2019 13:44

boaks force-pushed the add_connection_id branch 2 times, most recently from 14e2243 to c715147 Compare February 4, 2019 19:20

boaks force-pushed the add_connection_id branch from c715147 to fb5af41 Compare February 10, 2019 18:21

boaks mentioned this pull request Feb 10, 2019

Redesign connection store #868

Merged

boaks force-pushed the add_connection_id branch from fb5af41 to 1dfd856 Compare February 11, 2019 09:30

boaks force-pushed the add_connection_id branch 7 times, most recently from a008bd6 to 5256485 Compare February 18, 2019 07:45

boaks added the merged soon - If no objection label Feb 18, 2019

boaks force-pushed the add_connection_id branch from cb3b001 to 704985d Compare February 18, 2019 09:45

sbernard31 reviewed Feb 19, 2019

View reviewed changes

boaks force-pushed the add_connection_id branch 2 times, most recently from 3b2b612 to 685b55e Compare February 20, 2019 10:27

Achim Kraus added 2 commits February 22, 2019 10:21

Introduce dtls connection id.

f25b1eb

Improve dtls communication for environments with unaware address changes (e.g. NATs). Signed-off-by: Achim Kraus <[email protected]>

Add unit tests with cid handshakes.

aa34d08

Signed-off-by: Achim Kraus <[email protected]>

boaks force-pushed the add_connection_id branch from 685b55e to aa34d08 Compare February 22, 2019 09:27

boaks merged commit 8d7b41f into eclipse-californium:2.0.x Feb 22, 2019

boaks deleted the add_connection_id branch April 3, 2019 14:28

		@@ -73,6 +73,8 @@
		public static final int SEQUENCE_NUMBER_BITS = 48;

		public static final int LENGTH_BITS = 16;

Introduce dtls connection id. #857

Introduce dtls connection id. #857

Conversation

boaks commented Jan 25, 2019

boaks commented Jan 28, 2019

sbernard31 commented Feb 4, 2019

boaks commented Feb 4, 2019

boaks commented Feb 6, 2019

boaks commented Feb 11, 2019

boaks commented Feb 18, 2019

Choose a reason for hiding this comment

boaks Feb 19, 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

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

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

boaks Feb 19, 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

boaks commented Feb 20, 2019

sbernard31 commented Feb 22, 2019

boaks commented Feb 22, 2019

boaks Feb 19, 2019 •

edited

Loading

boaks Feb 19, 2019 •

edited

Loading