Updated webrev looks good.
--Sean
On 11/16/18 10:31 AM, Seán Coffey wrote:
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
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
Thanks for the comments Weijun.
As per other review thread, I'm now recording all properties set via
Security.setProperty(String, String).
regards,
Sean.
On 14/11/2018 01:11, Weijun Wang wrote:
Confused. Aren't all Security properties security-related? This is not about
normal system
On 11/13/18 8:11 PM, Weijun Wang wrote:
Confused. Aren't all Security properties security-related? This is not about
normal system properties.
Although probably not that common, an application could create their own
security properties, ex: Security.setProperty("security.myPassword",
Confused. Aren't all Security properties security-related? This is not about
normal system properties.
And the method name in the latest webrev is "isSecurityProperty" without the
"JDK" word. I assume this means you don't care about the difference between SE
properties and JDK properties.
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
Erik,
comments inline..
On 08/11/18 15:12, Erik Gahlin wrote:
Hi Sean,
I think we could still call the event
"jdk.SecurityPropertyModification", but add a @Description that
explains that events are only emitted for the JDK due to security
concerns. If we at a later stage want to include
Hi Sean,
I think we could still call the event
"jdk.SecurityPropertyModification", but add a @Description that explains
that events are only emitted for the JDK due to security concerns. If we
at a later stage want to include user events, we could add those and
remove the @Description,
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
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
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
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.
-
Thanks Sean.
Some feedback on the code in the security libraries.
- We should use camel case naming convention for variables (not underscore).
- 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
As per request from Erik, I separated the tests out into individual ones
to test the JFR and Logger functionality. I introduced a new separate
test for the CertificateChainEvent event also. Originally this was
wrapped into the TLSHandshakeEvent test.
Thanks to Erik for extra refactoring and
> On 29 Jun 2018, at 17:34, Seán Coffey wrote:
>
> I've introduced a new test helper class in the jdk/test/lib/jfr directory to
> help with the dual test nature of the new tests. It's helped alot with test
> code duplication.
>
My thinking was to put things like the certificates in a
I've introduced a new test helper class in the jdk/test/lib/jfr
directory to help with the dual test nature of the new tests. It's
helped alot with test code duplication.
Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's
fits. The output is displayed in units like "KiB" -
Comments inline.
On 28/06/2018 17:20, Erik Gahlin wrote:
It's sufficient if an event object escapes to another method
(regardless if JFR is enabled or not).
Some more feedback:
Rename event jdk.CertChain to jdk.CertificateChain
Rename event jdk.X509Cert to jdk.X509Certificate
Rename field
It's sufficient if an event object escapes to another method (regardless
if JFR is enabled or not).
Some more feedback:
Rename event jdk.CertChain to jdk.CertificateChain
Rename event jdk.X509Cert to jdk.X509Certificate
Rename field certChain to certificateChain.
Rename field serialNum to
Looks fine to me. Thanks!
Xuelei
On 6/28/2018 5:28 AM, Seán Coffey wrote:
Thanks for reviewing Xuelei,
I do acknowledge that the new TLS v1.3 code has greatly improved the
logging output for related operations. I think the main drive with this
enhancement is to use the new JFR API to
Thanks for the update Erik. By default I'm proposing that the new JFR
Events and Logger be disabled. As a result the event class shouldn't
escape. If performance metrics highlight an issue, we should revisit.
regards,
Sean.
On 27/06/2018 20:57, Erik Gahlin wrote:
On 2018-06-27 21:14, Seán
Thanks for reviewing Xuelei,
I do acknowledge that the new TLS v1.3 code has greatly improved the
logging output for related operations. I think the main drive with this
enhancement is to use the new JFR API to capture interesting events. We
can revisit the Logger requirements if there's a
Finished.java
-
In each handshake, Finished messages are delivered twice. One from
client, and the other one from the server side. Catch Finished message
and use the final one of them should be sufficient.
In the Finished.java implementation, the signal of the final Finished
KeyUtil.java
167 public static String getKeyAlgorithm(Key key) {
I may suggest remove this method and use Key.getAlgorithm() directly.
The KeyUtil.getKeyAlgorithm() is incomplete, and may need additional
maintenance if new key algorithms are added in the future. For example,
in
On 27/06/2018 19:57, Xuelei Fan wrote:
Hi Sean,
I may reply in several replies.
PKIXMasterCertPathValidator.java
+ CertChainEvent cce = new CertChainEvent();
+ if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+ String c =
Hi Sean,
I may reply in several replies.
PKIXMasterCertPathValidator.java
+ CertChainEvent cce = new CertChainEvent();
+ if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+ String c = reversedCertList.stream()
+ .map(x ->
Erik,
I rebased the patch with TLS v1.3 integration today. I hadn't realized
how much the handshaker code had changed. Hopefully, I'll get a review
from security dev team on that front.
Regards the JFR semantics, I believe the edits should match majority of
requests . I still have a
Many thanks for the review comments Erik. Replies inline.
On 24/06/2018 14:21, Erik Gahlin wrote:
Hi Sean,
Some of the changes in the webrev belongs to JDK-8203629 and should be
removed for clarity.
Some initial comments:
default.jfc, profile.jfr:
The events should not have
Hi Sean,
Some of the changes in the webrev belongs to JDK-8203629 and should be
removed for clarity.
Some initial comments:
default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The purpose of
the control attribute is so to provide parameterized configuration of
28 matches
Mail list logo
