Update TLS connection to optionally output TLS keys to file

[cconstab]

Is your feature request related to a problem? Please describe. Update TLS connection to optionally output TLS keys to file. This allows you to "see" inside the TLS packets using WireShark and diagnose issues.

Describe the solution you'd like Open to suggestions on implementation but it would be nice just to include a dev low level library that includes this feature. So testing can be done by including a dev library in pubspec.yaml. This would dump the TLS keys in the directory where the binary is being run.

The additional lines of code required to do this are :-

In at_lookup_impl.dart

      var secConConnect = SecurityContext();
       var keyfile = File('keysfile');
       secConConnect.setTrustedCertificates('caroot/rootcacert.pem');
      var secureSocket = await SecureSocket.connect(host, int.parse(port), context: secConConnect, keyLog: (line) => keyfile.writeAsStringSync(line, mode: FileMode.append));

And in monitor_client.dart

var secConConnect = SecurityContext();
       var keyfile = File('keysfile');
       secConConnect.setTrustedCertificates('caroot/rootcacert.pem');
      var secureSocket = await SecureSocket.connect(host, int.parse(port), context: secConConnect, keyLog: (line) => keyfile.writeAsStringSync(line, mode: FileMode.append));

replacing the secureSocket connection with no SecurityContent()

It would be nice to abstract the SecureSocket.connect so only one change would effect both lines of code and then that abstraction could be used in the secondary server code as well perhaps.

Describe alternatives you've considered I did consider pushing all the way through via command line options or by adding a method options but that I think holds the danger of leaving it in place before going to a prod build.. But open to them or other ideas..

Additional context Screen shot of the resulting Wireshark diagnostics

[VJag]

Flowchart for proposed changes

flowchart TD
    A[Start] --> B[CreateSecureSocket]
    B --> C[Read preference]
    C --> D{decryptPackets?}
    D -->|No| E[Create secure socket without security context]
    D -->|Yes| F[Create secure socket with security context]
    E --> G[End]
    F --> G

[murali-shris]

   class SecureSocketUtil {

            SecureSocket createSecureSocket(host, port) {

                if (config.decryptPackets) {
                        var secConConnect = SecurityContext();
                        var keyfile = File('keysfile');
                        secConConnect.setTrustedCertificates('caroot/rootcacert.pem');
                        return await SecureSocket.connect(host, int.parse(port), context: secConConnect, keyLog: (line) => keyfile.writeAsStringSync(line, mode: FileMode.append));

                } else {
                    return await SecureSocket.connect(host, int.parse(port);
                }

            }
      }

[murali-shris]

https://api.flutter.dev/flutter/dart-io/SecureSocket/connect.html

[murali-shris]

files to modify on client side at_client/lib/src/manager/monitor.dart at_lookup/lib/src/at_lookup_impl.dart

[murali-shris]

wireshark TLS decryption https://wiki.wireshark.org/TLS#tls-decryption

[srieteja]

The above-listed PRs contain implementation for creating sockets with security context and changes necessary to support this new implementation. What needs to be worked on further: Unit tests in at_client_sdk need minor changes to the way they mock MonitorOutboundConnectionFactory class as this class uses a new implementation to create secure sockets.

@srieteja

[cconstab]

When testing this I only see one TLS connection dumping the keys .. The monitor connection.. We need to dump keys for all files. The test for the rootcacerts file also does not error if the file is not there. Plus if the files is not there for the keys it does not get opend (it is being opened in append mode)..

It seems that the monitor connections transfer the options but the atlookup connections do not and hence do not drop the keys.

I locally changed that check to false and I see all the session keys..

Will put more info in to the ticket asap.. just late /tired now @srieteja

[cconstab]

Ok I have spotted the problem in at_libraraies and posted a branch with a 'fix'

@srieteja see what you think

I am not too sure how the monitor connection gets picked up ?? As I see nowhere in monitor_client.dart where the code uses the TLS dumping socket connection , does that happen somewhere else now ??

the branch is 'tlsdump` I tested sshnp's using

dependency_overrides:
  at_lookup:
      git:
         url: https://github.com/atsign-foundation/at_libraries.git
         path: at_lookup
         ref: tlsdump

[srieteja]

The monitor uses the SecureSocketUtil in 'at_client/lib/src/manager/monitor.dart'. This was a more feasible place to do this as we needed access to the AtClientPreferences. @cconstab

[cconstab]

Is my fix ok? @srieteja if so I will raise a PR

[srieteja]

@cconstab I was able to understand the problem but I was unable the understand the fix. But I tried running a sample code with your branch and I could see the difference. I got tls keys from two sessions with the fix. It does seem good to me, I will try understanding how that is happening in the meantime.

[cconstab]

Yup it's just a single line change to remove the bool. The rest is just editor noise

[srieteja]

Yes. The thing that is bugging me is that even though the false statement was removed, the default value for decryptPackets is false in SecureSocketConfig class. I'm just trying to understand how removing the false statement is affecting the functioning.

[srieteja]

@cconstab I just debugged it and understood your fix and was able to observe the bug. Also if it's okay with you I would like to push a commit into your branch resolving the case with rootcerts availability check.

[cconstab]

That's great thanks

[srieteja]

When testing this I only see one TLS connection dumping the keys .. The monitor connection.. We need to dump keys for all files. The test for the rootcacerts file also does not error if the file is not there. Plus if the files is not there for the keys it does not get opend (it is being opened in append mode)..

@cconstab sorry for the delay, I forgot to push the change into the branch. I pushed a fix to throw an error when the certs don't exist. Reg the other thing I used append mode instead of write as write would overwrite data from other connections in the same session, and append does create a new file when a file does not already exist. If that sounds good to you, we can go ahead and open up a pr.

[cconstab]

That sounds great please open the PR

[gkc]

I think this has been completed but waiting for @cconstab to approve the PR

[cconstab]

Ok I have tested this an approved the PR ... Thanks folks!

[cconstab]

Once PR is merged and published, we can close this ticket