Migrate jDiameter to Netty

[deruelle]

Move all the IO layer to Netty.

• TCP
• TLS
• SCTP : we can use the Netty-2 branch of https://github.com/Restcomm/sctp/tree/netty-2 and corresponding release from CI Job https://mobicents.ci.cloudbees.com/job/Restcomm-SCTP-Netty/. this should also solve Support SCTP Multi-Homing #25

[jehanzebqayyum]

TCPClientConnection & TCPTransportClient are used by both diameter server and client.
To move to netty this should be refactored into server and client specific transport.
Since server will have both boss and worker group to handle multiple clients.
So server side NetworkGuard functionality will move into TCPServerConnection.

[jehanzebqayyum]

I went through the code and seems like changing to netty requires a bit of refactoring or may be i need more understanding of the code.

The problems i am facing specifically are:

1- Client and Server both are using same TCPClientConnection class which I want to separate. I will need some assistance how to do that.
In netty, Server will have 2 thread groups i.e. boss group which accepts clients and worker group which will serve each client. Whereas client will require only worker group.

2- I am trying to understand the purpose of NetworkGuard and when it is used?
Because TCPServerConnection (which i am planning to create) should have the functionality to handle multiple clients by default for server logic.

I am attaching a client side NettyTransportClient that i created for review.

There are other parts of transport which will benefit from refactoring e.g.

Connection Listener and Event logic is duplicated in all transports TCP, TLS etc.

Connection Listeners are blocking i.e. when an event e.g. message received is raised, all the connection listeners are called in blocking which blocks the worker thread instead it should be asynchronous and we can use e.g. guava event bus for handling events.

What are your thoughts?

NettyTransportClient.java.txt

[brainslog]

Hi @jehanzebqayyum, thanks for your interest in this contribution.

As per your queries, I will try to explain as I can.

1- Client and Server both are using same TCPClientConnection class which I want to separate.

In jDiameter it's a common pattern to have the basic/common functionality in the client side and the server extending it with additional features. In this case it's pretty much the same, except the naming might not be the most clear.

So, on the client side we have:

• TCPTransportClient - responsible for reading/writing bytes from/to TCP socket, and pass the message bytes to TCPClientConnection;
• TCPClientConnection - responsible for delivering the messages (already parsed as a Java object) to this connection' listeners. Other events on the connection are also notified to the listeners (connect, disconnect, errors).

These are the basics for a Diameter peer to operate, be it a server or a client, receive the bytes, transform them into a Diameter message and deliver it to the listeners. Also, always keep in mind that in Diameter both client or server might initiate the session with a message and even any of them may initiate the connection.

2- I am trying to understand the purpose of NetworkGuard and when it is used?
Because TCPServerConnection (which i am planning to create) should have the functionality to handle multiple clients by default for server logic.

So, in case of the server, we need to open a port for incoming connections. We have the following:

• NetworkGuard - responsible for opening a listening socket, waiting for and handling incoming connections, which ultimately will result in a new TCPClientConnection (which will have it's TCPTransportClient).

So, I think this should make it clear that there's no need for distinct TCPTransportClient or TCPTransportClient as the tasks they perform are quite delimited and common to any type of peer.
As for the TCPServerConnection, the functionality you seem to pretend to implement is partially what is actually being done at NetworkGuard.

Also should be taken into account the modularity of jDiameter, where all these components are changeable by custom implementations, thus the need of these separation of concerns.

I will review the NettyTransportClient.java you have attached, at first look it seems correct, but haven't yet had the time to fully review and test it, but wanted to try to answer your questions ASAP. Feel free to make any changes to it meanwhile, if you feel necessary and if this explanation might have caused any difference to it.

[jehanzebqayyum]

Thanks for the guidance. Base on the input i updated TCPClientConnection, TCPClientTransport (NettyTransportClient) and NetworkGuard. Attached updated.

A couple of more questions, hope you do not mind:
1- Should we be reusing the thread pool from IConcurrentFactory or instantiate new ones as defaults in netty's NioEventLoopGroups.
2- In NettyTransportClient orig/source address seems redundant since same channel cannot be used for bind and connect, i believe.

Meanwhile i will be testing the changes.

NettyTransportClient.java.txt
NetworkGuard.java.txt
TCPClientConnection.java.txt

[deruelle]

@jehanzebqayyum it may be easier for review if you fork the project, commit your changes in your own branch and push to your fork and do a pull request for review as described in our Open Source Playbook

[jehanzebqayyum]

Done. Please review the pull request and provide your comments.
Meanwhile i continue to work on my fork.

Best Regards,
Jehanzeb Qayyum
+971 52 898 2285
+92 3311337064

On Thu, May 12, 2016 at 12:57 AM, Jean Deruelle notifications@github.com
wrote:

@jehanzebqayyum https://github.com/jehanzebqayyum it may be easier for
review to fork the project, commit your changes in your own branch and do a
pull request for review as described in our Open Source Playbook
https://docs.google.com/document/d/1RZz2nd2ivCK_rg1vKX9ansgNF6NpK_PZl81GxZ2MSnM/edit#heading=h.mcjitemt6ng1

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#35 (comment)

[deruelle]

Thanks @jehanzebqayyum !

@brainslog will review.

[jehanzebqayyum]

Should TLS negotiation happen in connection establishment or CER/CEA?

Below is what i found out:
For RFC3588 compliant peers TLS (Transport Layer Security) may optionally
be negotiated. For RFC6733 compliant peers TLS negotiation may optionally
happen before the CER/CEA.

Secondly this is what i found for inband security id in RFC6733:

Inband-Security-Id AVP

The Inband-Security-Id AVP (AVP Code 299) is of type Unsigned32 and
is used in order to advertise support of the security portion of the
application. The use of this AVP in CER and CEA messages is NOT
RECOMMENDED. Instead, discovery of a Diameter entity's security
capabilities can be done either through static configuration or via
Diameter Peer Discovery.

Any comments?

Best Regards,
Jehanzeb Qayyum
+971 52 898 2285
+92 3311337064

On Mon, May 16, 2016 at 1:49 PM, Jean Deruelle notifications@github.com
wrote:

Thanks @jehanzebqayyum https://github.com/jehanzebqayyum !

@brainslog https://github.com/brainslog will review.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#35 (comment)

[brainslog]

@jehanzebqayyum, regarding TLS we are working in parallel on issue #14 . The idea will be to have both ways working. We surely want RFC 6733 behavior to be the default, but most implementations seem to still be using RFC 3588, so we also need to have it for interop.

Implementing RFC 3588 is likely (still) the most useful and the most challenging one, so that'd be a good start :)

[jehanzebqayyum]

@brainslog quick questions
why post sending CEA there is startTLS? i expect it only after post receiving CEA.
what is the purpose of checking EXPERIMENTAL_RESULT in TLS transport?

[brainslog]

@jehanzebqayyum, both server and client need to prepare for TLS after sending/receiving the CEA, and setup the HandshakeCompletedListener.

As for the check of EXPERIMENTAL_RESULT, it's likely just a "fallback" mechanism when no Result-Code is present. It can probably be safely discarded.

[deruelle]

@jehanzebqayyum where you able to move forward thanks to @brainslog comments ? Anything blocking ?

[jehanzebqayyum]

Yes i almost finished TLS implementation. Since there are no test case for
TLS. Can you point out for TLS sample config?

Thanks
On Jun 1, 2016 12:32 PM, "Jean Deruelle" notifications@github.com wrote:

@jehanzebqayyum https://github.com/jehanzebqayyum where you able to
move forward thanks to @brainslog https://github.com/brainslog comments
? Anything blocking ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AFNfSl8xXtsxykxl7C2xjzDT7I-rHHFDks5qHUOsgaJpZM4IVOfw
.

[jehanzebqayyum]

@brainslog https://github.com/brainslog where can i find sample TLS
client/server config?

Best Regards,
Jehanzeb Qayyum
+971 52 898 2285
+92 3311337064

On Wed, Jun 1, 2016 at 12:36 PM, jz jehanzeb.qayyum@gmail.com wrote:

Yes i almost finished TLS implementation. Since there are no test case for
TLS. Can you point out for TLS sample config?

Thanks
On Jun 1, 2016 12:32 PM, "Jean Deruelle" notifications@github.com wrote:

@jehanzebqayyum https://github.com/jehanzebqayyum where you able to
move forward thanks to @brainslog https://github.com/brainslog
comments ? Anything blocking ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AFNfSl8xXtsxykxl7C2xjzDT7I-rHHFDks5qHUOsgaJpZM4IVOfw
.

[brainslog]

@jehanzebqayyum, I don't think we have any ready, will need to prepare it. Or maybe @coolface88 who's working on #14 might have some already ? If not, I'll try to provide you with it by next week.

BTW, regarding TCP, I've made some comments to the PR #39 as it seems it has some functional issues.

[deruelle]

@jehanzebqayyum just checking if you have been able to move forward on that ?

[jehanzebqayyum]

​Well i am trying to resolve following issue with TLS implementation​.

io.netty.handler.codec.DecoderException:
javax.net.ssl.SSLHandshakeException: no cipher suites in common
at
io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:418)
at
io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:245)
at
io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:292)
at
io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:278)

Client config looks like this:

​Server config has following:

​ ​SSL context creation:

public static SslContext getSslContextForClient(Configuration config)
throws SSLException, Exception {
SslContext sslContext = SslContextBuilder.forClient()

.keyManager(getKeyManagerFactory(config)).trustManager(getTrustManagerFactory(config)).build();
return sslContext;
}

public static SslContext getSslContextForServer(Configuration config)
throws SSLException, Exception {
SslContext sslContext =
SslContextBuilder.forServer(getKeyManagerFactory(config))
.trustManager(getTrustManagerFactory(config)).build();
return sslContext;
}
​

On Fri, Jun 10, 2016 at 8:40 PM, Jean Deruelle notifications@github.com
wrote:

@jehanzebqayyum https://github.com/jehanzebqayyum just checking if you
have been able to move forward on that ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AFNfSvuvVy7m0NahDgSCN7Y39ngG3qHMks5qKZN0gaJpZM4IVOfw
.

[deruelle]

@jehanzebqayyum were you able to debug it ?
did you see http://stackoverflow.com/questions/9534118/ssl-tls-error-no-cipher-suites-in-common-with-netty-when-trying-to-establish#comment12130198_9534118 ?

try to make sure the key manager is not null http://stackoverflow.com/a/15144731/118052

[jehanzebqayyum]

Yes in fact the ssl debug log showed that the key i was using did not match
the TLS specs/supported cipher suites.

I used default jdk keytool option to generate key pair which resulted in
DSA algo.

I resolved it by specifying RSA as the keyalg.

And finally i was able to run a functional test
(AccSessionStatefulBasicFlowTest) successfully.

I will push my changes soon.

On Mon, Jun 13, 2016 at 5:23 PM, Jean Deruelle notifications@github.com
wrote:

@jehanzebqayyum https://github.com/jehanzebqayyum were you able to
debug it ?
did you see
http://stackoverflow.com/questions/9534118/ssl-tls-error-no-cipher-suites-in-common-with-netty-when-trying-to-establish#comment12130198_9534118
?

try to make sure the key manager is not null
http://stackoverflow.com/a/15144731/118052

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AFNfSnwvLi6GP-Qtka28eANGmB93gMV6ks5qLVnGgaJpZM4IVOfw
.

[jehanzebqayyum]

@brainslog did you check?

On Jun 13, 2016 10:58 PM, "jz" jehanzeb.qayyum@gmail.com wrote:

Yes in fact the ssl debug log showed that the key i was using did not
match the TLS specs/supported cipher suites.

I used default jdk keytool option to generate key pair which resulted in
DSA algo.

I resolved it by specifying RSA as the keyalg.

And finally i was able to run a functional test
(AccSessionStatefulBasicFlowTest) successfully.

I will push my changes soon.

On Mon, Jun 13, 2016 at 5:23 PM, Jean Deruelle notifications@github.com
wrote:

@jehanzebqayyum were you able to debug it ?
did you see
http://stackoverflow.com/questions/9534118/ssl-tls-error-no-cipher-suites-in-common-with-netty-when-trying-to-establish#comment12130198_9534118
?

try to make sure the key manager is not null
http://stackoverflow.com/a/15144731/118052

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[jehanzebqayyum]

hi @deruelle, are you referring Restcomm/sctp netty branch? I do not see netty-2.
The restcomm sctp netty branch is using older version of netty lib.
Considering that there is default support for SctpChannel in netty should we be using the restcomm sctp netty lib? Since we will not be using features such as fall back to TCP, Management etc from the lib. I feel going along the lines of existing implementations of netty tcp/tls in jdiameter will be much straightforward.

[deruelle]

@jehanzebqayyum since my comment the master branch is now netty enabled so you can use https://github.com/RestComm/sctp/tree/master.
Let's go with the pure netty approach then for now. I would like @vetss to comment on that too to see if we are missing on any restcomm sctp features if we go with the pure netty based approach but he is on vacations so please proceed with pure netty approach for now.

[vetss]

I have created an issue that explain details for switching to new SCTP lib versions
#60