I've renamed the 'peerCertificateId' variable and label in
TLSHandshakeEvent to 'certificateId'. This allows the event data to be
co-displayed in JMC views which share the same type of data
(@CertificateId). I've uploaded an example screenshot [1]
I've also made an adjustment to the TestModuleEvents test to disregard
the jdk.proxy1 module for test purposes.
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v10/webrev/
[1]
http://cr.openjdk.java.net/~coffeys/jmc_screenshots/shared_selection_certificateId.png/Screenshot%20from%202018-11-16%2015%3a28%3a59.png
Regards,
Sean.
On 14/11/18 21:09, Seán Coffey wrote:
Hi Sean,
comments inline..
On 13/11/2018 18:53, Sean Mullan wrote:
Looking good, a couple of comments/questions:
* src/java.base/share/classes/java/security/Security.java
The isJdkSecurityProperty method could return false positives, for
example there may be a non-JDK property starting with "security.". I
was thinking it would be better to put all the JDK property names in
a HashSet which is populated by the static initialize() method, and
only if event logging is enabled. Then setProperty can just check if
the property name is in this set.
after further discussion, we've decided to log all properties operated
on via Security.setProperty(String, String). It think it makes sense.
The contents of a JFR recording should always be treated as sensitive.
Keep in mind also that these new JFR Events will be off by default.
* src/java.base/share/classes/sun/security/x509/X509CertImpl.java
Good points here. I've taken on board your suggestion to move this
code logic to the sun/security/provider/X509Factory.java class as per
your later email suggestion.
45 l.addExpected("SunJSSE Test Serivce");
Is that a typo? "Serivce"
That's a typo in the test certificate details. We should fix it up via
another bug record.
* test/jdk/jdk/security/logging/TestX509ValidationLog.java
54: remove blank line
* test/lib/jdk/test/lib/security/SSLSocketTest.java
Why is this file included? It looks like a duplicate of
SSLSocketTemplate.
Yes - it's almost identical to SSLSocketTemplate except that
SSLSocketTemplate doesn't belong to a package. I had a hard time
incorporating its use into the jtreg tests via the @lib syntax. I
think it's best if we open another JBS issue to migrate other uses of
SSLSocketTemplate to jdk.test.lib.security.SSLSocketTemplate
I've cleaned up the various typo/syntax issues that you've highlighted
also.
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v9/webrev/index.html
regards,
Sean.
The rest of my comments are mostly minor stuff, and nits:
*
src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java
245 if(xve.shouldCommit() || EventHelper.isLoggingSecurity()) {
add space before "(xve"
* src/java.base/share/classes/sun/security/ssl/Finished.java
1097 }
indent
1098 if(event.shouldCommit()) {
add space before "(event"
* src/java.base/share/classes/sun/security/x509/X509CertImpl.java
update copyright
* src/java.base/share/classes/jdk/internal/event/EventHelper.java
35 * A helper class to have events logged to a JDK Event Logger
Add '.' at end of sentence.
* src/java.base/share/classes/jdk/internal/event/TLSHandshakeEvent.java
38: remove blank line
*
src/java.base/share/classes/jdk/internal/event/X509ValidationEvent.java
30 * used in X509 cert path validation
Add '.' at end of sentence.
* src/jdk.jfr/share/classes/jdk/jfr/events/X509ValidationEvent.java
47: remove blank line
* test/jdk/jdk/security/logging/TestTLSHandshakeLog.java
--Sean
On 11/6/18 3:46 AM, Seán Coffey wrote:
With JDK-8203629 now pushed, I've re-based my patch on latest
jdk/jdk code.
Some modifications also made based on off-thread suggestions :
src/java.base/share/classes/java/security/Security.java
* Only record JDK security properties for now (i.e. those in
java.security conf file)
- we can consider a new event to record non-JDK security events
if demand comes about
- new event renamed to *Jdk*SecurityPropertyModificationEvent
src/java.base/share/classes/sun/security/x509/X509CertImpl.java
* Use hashCode() equality test for storing certs in List.
Tests updated to capture these changes
src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java
* Verify that self signed certs exercise the modified code paths
I've added new test functionality to test use of self signed cert.
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/
Regards,
Sean.
On 17/10/18 11:25, Seán Coffey wrote:
I'd like to re-boot this review. I've been working with Erik off
thread who's been helping with code and test design.
Some of the main changes worthy of highlighting :
* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name :
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the
@Relational JFR annotation.
* Keep calls for JFR event operations local to call site to
optimize jvm compilation
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/
This code is dependent on the JDK-8203629 enhancement being
integrated.
Regards,
Sean.
On 10/07/18 13:50, Seán Coffey wrote:
Erik,
After some trial edits, I'm not so sure if moving the event &
logger commit code into the class where it's used works too well
after all.
In the code you suggested, there's an assumption that calls such
as EventHelper.certificateChain(..) are low cost. While that might
be the case here, it's something we can't always assume. It's
called once for the JFR operation and once for the Logger
operation. That pattern multiplies itself further in other Events.
i.e. set up and assign the variables for a JFR event without
knowing whether they'll be needed again for the Logger call. We
could work around it by setting up some local variables and
re-using them in the Logger code but then, we're back to where we
were. The current EventHelper design eliminates this problem and
also helps to abstract the recording/logging functionality away
from the functionality of the class itself.
It also becomes a problem where we record events in multiple
different classes (e.g. SecurityPropertyEvent). While we could
leave the code in EventHelper for this case, we then have a mixed
design pattern.
Are you ok with leaving as is for now? A future wish might be one
where JFR would handle Logger via its own framework/API in a
future JDK release.
I've removed the variable names using underscore. Also optimized
some variable assignments in X509Impl.commitEvent(..)
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/
regards,
Sean.
On 09/07/2018 18:01, Seán Coffey wrote:
Erik,
Thanks for reviewing. Comments inline..
On 09/07/18 17:21, Erik Gahlin wrote:
Thanks Sean.
Some feedback on the code in the security libraries.
- We should use camel case naming convention for variables (not
underscore).
Sure. I see two offending variable names which I'll fix up.
- Looking at sun/security/ssl/Finished.java,
I wonder if it wouldn't be less code and more easy to read, if
we would commit the event in a local private function and then
use the EventHelper class for common functionality.
I'm fine with your suggested approach. I figured that tucking the
recording/logging logic away in a helper class also had benefits
but I'll re-factor to your suggested style unless I hear objections.
regards,
Sean.
