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.