Pass Handshaker instance to handshakeFailed & handshakeCompleted in SessionListener

[lhotari]

My use case is about doing custom logging to a database when a DTLS handshake fails. The requirement is to be able to associate the handshake failures to a PSK identity.

The PSK identity wrapped in a PreSharedKeyIdentity instance is set to DTLSSession.setPeerIdentity.
I'd like to look this up when the handshake fails.

The current challenge is the SessionListener interface. (Thanks for previous changes related to #716 which made it possible to register custom SessionListeners). The handshakeFailed method doesn't pass the Handshaker instance to the handshakeFailed method call. If it was passed, I'd expect it would be possible to find the PSK identity by calling ((PreSharedKeyIdentity)handshaker.getSession().getPeerIdentity()).getIdentity() .

requested change in SessionListener interface:

	/**
	 * Indicates that a handshake has been completed.
	 * 
	 * In particular, this means that both peers have received the other
	 * peer's <em>FINISHED</em> messages.
	 * 
	 * @param handshaker the handshaker used to establish the session
	 */
	void handshakeCompleted(Handshaker handshaker);

	/**
	 * Indicates that a handshake has failed.
	 * 
	 * @param handshaker the handshaker used to establish the session
	 * @param error the error occurred during the handshake
	 */
	void handshakeFailed(Handshaker handshaker, Throwable error);

The current handshakeStarted and sessionEstablished methods already have the Handshaker handshaker parameter so this change would also improve the consistency of the SessionListener interface.

[boaks]

FMPOV, it makes sense. I try to include it in 2.0.0-M12.

[lhotari]

Looks like there is another challenge for my use case of logging the invalid PSK attempts to a database.

The SessionListener interface's handshakeFailed method doesn't get called in that case:
https://github.com/eclipse/californium/blob/0fcca599600538250c25fafd7c580e92941bd8ff/scandium-core/src/main/java/org/eclipse/californium/scandium/DTLSConnector.java#L1948-L1954

I wonder what would be the way to proceed. Could I add another issue as a feature request for having some listener method called when the PSK is invalid?

[sbernard31]

This make senses to me to call handshakeFailed even if we don't send an alert because in that case the handshake has failed anyway.

[lhotari]

Even if calling handshakeFailed would be added in the UNKNOWN_PSK_IDENTITY case, it looks like it's not possible to get hold of an recognized PSK identity without parsing an exception message.
https://github.com/eclipse/californium/blob/ca5a7b2df4fbe2cb0df850e0bd39b4e8c212ce0d/scandium-core/src/main/java/org/eclipse/californium/scandium/dtls/ServerHandshaker.java#L997-L1005
The PSK identity isn't set to DTLSSession.setPeerIdentity unless the PSK is known.
There might be other ways to solve the case of an unknown PSK identity. For that case, it would be useful to have the peer address passed in a call to
https://github.com/eclipse/californium/blob/ab7454b679756a6cd70b67d524cfc929dca79f24/scandium-core/src/main/java/org/eclipse/californium/scandium/dtls/pskstore/PskStore.java#L61
The reason why that would be useful is that it would be possible to log failed authentication attempts based on the peer address in the PskStore implementation.

My actual use case is to be able to detect when the PSK secret is incorrect. For that I'm expecting that the original approach using the handshakeFailed event + DTLSSession.getPeerIdentity would be a working solution.

[boaks]

Please remind, that PR #605 introduced the "discard" instead of "terminate". And in issue #606 there is still a discussion about the best practice there. I'm not sure, if calling handshakeFailed instead of "discard" or "terminate" will comply to all there requirements here (even, if they are not that clear).

[sbernard31]

It seems to me that "discard or terminate" is about behavior expose to foreign peer.
"Raising a handshakeFailed" is about what we are doing internally.
I think that those 2 questions could/should be uncorrelated.

Creating a specific HanshakeException (like UnknowPskException with necessary getter) could be a way.

[boaks]

ASFAIK:

"discard": simply ignores the record (but potentially leave the session/handshake in a unexpected state)

"terminate": remove connection and session, call handshakeFailed, and send alerts to the other peer.
Some stuff depends on the situation, there seems to be more than one terminate, the one for terminateOngoingHandshake does different things for connection with or without established session, and the code in "Connection.terminateOngoingHandshake" does the same as the code in "Connection.handshakeFailed".
So, maybe a cleanup now, would be better than an additional quick fix :-).

"handshakeFailed": call all handler, and execute, what they do. At least the Connection handler will modify the connection and with that, the further processing is changed.

FMPOV, it seems to be too complex for too easy answers.

[lhotari]

I'm linking one more issue to this discussion, #343 . It's slightly related to the handshakeFailed method. As mentioned in discussion of #335, which is linked to #343, handshake timeouts aren't detected when earlyStopRetransmission = true. The session listener interface method "handshakeFailed" never gets called. As a workaround I'm using earlyStopRetransmission = false to be able to detect handshake failures and log them to a database. This works, but I'm wonderting what the consequences are of using earlyStopRetransmission = false.

[boaks]

After reanalyzing, I think a solution would be to add the PreSharedKey identity to the ServerHandshaker. Together with using earlyStopRetransmission = false, should work for 2.0.0-M12.

[boaks]

earlyStopRetransmission = false is a initial recommendation out of RFC6347. But some implementations are broken and don't work with retransmissions. Therefore sbernard31 discussed it with the ietf core/tls mailing list and so the behavior was changed.

If you want to use earlyStopRetransmission = false, just check your clients, if they could process that proper.

[boaks]

Alternatively, the already available clientKeyExchange could be exposed public.
Then check for specific subclass PSKClientKeyExchange and read the identifier there.

[boaks]

It's merged. If possible, give us please a feedback, if it works for you.

[lhotari]

Thanks @boaks . The change LGTM. Do you also publish snapshot releases to some repository? That would make my testing a bit easier.

[boaks]

The build has completed and the result should be in

https://repo.eclipse.org/content/repositories/californium-snapshots
and
https://repo.eclipse.org/content/repositories/californium/

[lhotari]

@boaks Thank you. I tested with 2.0.0-SNAPSHOT. I can confirm that these changes work for me.

[boaks]

Great news! So, please close this issue. I hope we can release 2.0.0-M12 this week.

[lhotari]

I'm looking forward to that. Thanks for your help @boaks

[boaks]

Just to mention:
With the merge of PR #782, all stale handshakes are no failed by a timeout, not longer depending on the earlyStopRetransmission flag.
May be you can spend time into check, if this solution causes any trouble for you.