-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8262880: Add support for the NSS Key Log Format for SSL/TLS keys #2896
Conversation
Hi @SalusaSecondus, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user SalusaSecondus" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
/covered |
Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated! |
@SalusaSecondus The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
I can confirm that @SalusaSecondus is covered by the Amazon OCA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not good practice to leave secret information in debug log. Also, it may be not a good practice to introduce new logger format, including file and NSS format, into the SSLLogger. Someone also may want to introduce log format for MSS or XSS as well. Instead, please consider to make use of the features of Java Logger if you want to write the log to files, or use any special format.
/csr needed |
@XueleiFan has indicated that a compatibility and specification (CSR) request is needed for this pull request. |
I'm happy to create a CSR for this issue once the exact details of the option format have been figured out. |
I am not familiar with either the MSS or XSS log formats and would be interested to see them. The NSS format is a defacto industry standard and already supported by many libraries (both producers and consumers) and thus used widely in the security industry. Most other uses that I can find take this similar pattern of providing a file-name to the TLS logic and then getting key log (in this format) written to that file. I agree completely that logging secret information is dangerous and should almost never be done. That is why it has to be explicitly enabled (unlike most of the other |
@SalusaSecondus Unknown command |
@XueleiFan I'd really like to move this forward but I'm uncertain what changes you want me to make. This extra debugging information will be very valuable to those of us debugging Java TLS connections. |
I am also not comfortable adding this feature to the JDK, especially since every build of the JDK would by default have this feature enabled. Logging sensitive information to log files is not good security practice (there are many references I could cite). I also think it would be too easy to accidentally leave the system property enabled or forget to remove the file. |
I think that there might be some confusion around the sensitivity of the data being logged. The security impact of this data is almost identical to passing Similar to the existing feature, the data logged only impacts the exact connections that it is enabled for. (These are ephemeral session-specific secrets). I hope that this helps to ease some concerns and help explain why this exact feature is present in so many existing applications (including OpenSSL, BoringSSL, WolfSSL, s2n, Mozilla, and Chrome, among others). If you would like to try it with your current Chrome browser, just add |
Mailing list message from raell at web.de on security-dev: Hi all, I very much appreciate @SalusaSecondus' idea for providing a key logger. For, when working with networks, it's quite natural to analyze message flow To give an example: I'm running a https client based on SSLEngine and an Concerning the security issue: Of course, holding keys is always a risk. I would be very glad if there would be a chance to implement @SalusaSecondus' Regards, Ralph ? Gesendet:?Freitag, 19. M?rz 2021 um 13:48 Uhr
I am also not comfortable adding this feature to the JDK, especially since every build of the JDK would by default have this feature enabled. Logging sensitive information to log files is not good security practice (there are many references I could cite). I also think it would be too easy to accidentally leave the system property enabled or forget to remove the file. ------------- PR: https://git.openjdk.java.net/jdk/pull/2896 |
Mailing list message from Bernd Eckenfels on security-dev: Hello, I agree with the need for such a facility. Instead, or in addition to a key log a session handshake listener with access to the handshake result (and the master key) would also be useful for auditing, renegotiation limiting and key logging. It can even be used for things like priming introspecting firewalls. There is not yet a good official platform API with access to that kind of information (and proxying client Hello packages to get the offered ciphers is really ugly). With the API an Adapter which can write NSS Keylogs can be provided by users and you don?t have to worry about having debug logging Code expose secrets. Gruss -- Hi all, I very much appreciate @SalusaSecondus' idea for providing a key logger. For, when working with networks, it's quite natural to analyze message flow To give an example: I'm running a https client based on SSLEngine and an Concerning the security issue: Of course, holding keys is always a risk. I would be very glad if there would be a chance to implement @SalusaSecondus' Regards, Ralph Gesendet: Freitag, 19. M?rz 2021 um 13:48 Uhr
I am also not comfortable adding this feature to the JDK, especially since every build of the JDK would by default have this feature enabled. Logging sensitive information to log files is not good security practice (there are many references I could cite). I also think it would be too easy to accidentally leave the system property enabled or forget to remove the file. ------------- PR: https://git.openjdk.java.net/jdk/pull/2896 |
I agree with Bernd, an API gives us some more flexibility. I like the functionality provided by a key logging feature, but an API-based approach is more appropriate for the platform, and it more closely resembles what OpenSSL and GnuTLS are doing with callback registration. |
@SalusaSecondus This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@SalusaSecondus This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
This is my implementation for JDK-8262880 and enables creating of an SSL/TLS key log in the standardized NSS Key Log Format. This is supported by many TLS implementations and also by several parsers such as Wireshark. Supporting this will greatly ease in debugging TLS problems.
(Note: I am covered by the Amazon corporate contribution agreement).
Progress
Integration blocker
Issue
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2896/head:pull/2896
$ git checkout pull/2896