RFR: 8148188: Enhance the security libraries to record events of interest
Xuelei Fan
xuelei.fan at oracle.com
Thu Jun 28 12:42:21 UTC 2018
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 capture interesting events. We
> can revisit the Logger requirements if there's a strong opinion from
> users. By default, both the new Logger and JFR output will be switched
> off. I've just made an edit to the JFR jfc files to disable these events.
>
> "I may suggest remove this method and use Key.getAlgorithm() directly."
> - Yes, great idea. Done.
>
> Thanks for feedback on Finished.java edits. I did question to myself
> whether fewer edits were sufficient given the client/server nature of
> the handshake.
>
> I've refreshed the webrev :
>
> http://cr.openjdk.java.net/~coffeys/webrev.8148188.v3/webrev/
>
> some jfr side edits also :
>
> * Change label from "X.509 Certificate" to "X509 Certificate" - JFR test
> fails with "." usage
> * Move the instance field variable name in CertChainEvent to "certChain"
> - JFR tests discourage use of "ID" in "certIDs"
>
> regards,
> Sean.
>
>
> On 27/06/2018 21:11, Xuelei Fan wrote:
>> 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
>> message is the handshakeFinished field is set to true.
>>
>> Please move line 582:
>> 582 recordEvent(chc.conContext.conSession);
>> to line 558:
>> 556 // handshake context cleanup.
>> 557 chc.handshakeFinished = true;
>> 558
>>
>> Please move line 632:
>> 632 recordEvent(shc.conContext.conSession);
>> to line 608:
>> 606 // handshake context cleanup.
>> 607 shc.handshakeFinished = true;
>> 608
>>
>> Please remove line 838:
>> 838 recordEvent(shc.conContext.conSession);
>>
>> Please remove line 984:
>> 984 recordEvent(chc.conContext.conSession);
>>
>> No more comment.
>>
>> Thanks,
>> Xuelei
>>
>> On 6/27/2018 11:57 AM, Xuelei Fan wrote:
>>> 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 -> x.getSerialNumber().toString(16))
>>> + .collect(Collectors.joining(", "));
>>> + EventHelper.commitCertChainEvent(cce, c);
>>> + }
>>>
>>> No matter the event or the JFR mechanism is enabled or not, the above
>>> code will create a new instance. Is the return value of
>>> cce.isEnabled() dynamically changed or static?
>>>
>>> Is there a need to support both logging and JFR? I'm new to record
>>> events. I did not get the point to use both.
>>>
>>> Thanks,
>>> Xuelei
>>>
>>> On 6/26/2018 3:18 PM, Seán Coffey wrote:
>>>> 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 preference for the test infra design
>>>> for these new logger/JFR tests used in version 1 of webrev. I think
>>>> it makes sense to keep the test functionality together - no sense in
>>>> separating them out into different components IMO. Also, some of the
>>>> edits to the JFR testing were made to test for the new dual
>>>> log/record functionality. I might catch up with you tomorrow to see
>>>> what the best arrangement would be.
>>>>
>>>> http://cr.openjdk.java.net/~coffeys/webrev.8148188.v2/webrev/
>>>>
>>>> regards,
>>>> Sean.
>>>>
>>>>
>>>> On 25/06/2018 21:22, Seán Coffey wrote:
>>>>> 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 control="enable-exceptions". The
>>>>>> purpose of the control attribute is so to provide parameterized
>>>>>> configuration of events for JMC. As it is now, the security
>>>>>> events will be enabled when a user turns on the exception events.
>>>>> Makes sense. I'll remove that parameter.
>>>>>>
>>>>>> X509CertEvent:
>>>>>> You should use milliseconds since epoch to represent a date
>>>>>> instead of a string value, i.e.
>>>>>>
>>>>>> @Label("Valid From")
>>>>>> @Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
>>>>>> public long validFrom;
>>>>>>
>>>>>> @Label("Valid Until")
>>>>>> @Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
>>>>>> public long validUntil;
>>>>>>
>>>>> The CertificateValidity class operates on Date Object values. I'll
>>>>> work with the Date.getTime() method then (and update the Logger
>>>>> formatting)
>>>>>> This following annotation adds little value
>>>>>>
>>>>>> @Description("Details of Security Property")
>>>>>>
>>>>>> I would either remove the annotation, or provide information that
>>>>>> helps a user understand the event. For instance, what is X509, and
>>>>>> what kind of certificates are we talking about?
>>>>> Yes - that looks like the wrong Description. I'll review all of
>>>>> these fields.
>>>>>>
>>>>>> @Category("Java Application")
>>>>>>
>>>>>> I'm a bit worried that we will pollute the "Java Application"
>>>>>> namespace if we add lots of JDK events in that category. We may
>>>>>> want to add a new top level category "Java Development Kit",
>>>>>> analogous to the "Java Virtual Machine" category, and put all
>>>>>> security related events in subcategory, perhaps called "Security".
>>>>> Yes - Open to suggestions. "Security" sounds like a good suggestion.
>>>>>>
>>>>>> @Label("X509Cert")
>>>>>>
>>>>>> The label should be human readable name, with spaces, title cased
>>>>>> etc. I would recommend "X.509 Certificate". In general, avoid
>>>>>> abbreviations like "certs" and instead use labels such as
>>>>>> "Certificate Chain". The label should be short and not a full
>>>>>> sentence.
>>>>>>
>>>>>> For details see,
>>>>>> https://docs.oracle.com/javase/10/docs/api/jdk/jfr/Label.html
>>>>>>
>>>>>> I think it would be good to separate testing of JFR and logging
>>>>>> into different files / tests. I would prefer that the test name is
>>>>>> the same as the event prefixed with "Test", i.e
>>>>>> TestX509CertificateEvent, as this is the pattern used by other JFR
>>>>>> tests.
>>>>>>
>>>>> I'll take a look at that pattern again. I've separated out the
>>>>> current tests into an (a) outer test to analyze the logger output
>>>>> and (b) the inner test which checks for JFR correctness. I did
>>>>> include extra logic to make sure that the EventHelper configuration
>>>>> was working correctly. "Events.assertField" looks very helpful.
>>>>> Thanks for the pointer.
>>>>>
>>>>> Let me take on board the suggestions below and get a new webrev out
>>>>> on Tuesday.
>>>>>
>>>>> regards,
>>>>> Sean.
>>>>>
>>>>>> I reworked one of the tests to how I like to see it:
>>>>>>
>>>>>> /*
>>>>>> * @test
>>>>>> * @key jfr
>>>>>> * @library /test/lib
>>>>>> * @run main/othervm jdk.jfr.event.security.TestX509CertificateEvent
>>>>>> */
>>>>>> public class TestX509CertificateEvent {
>>>>>>
>>>>>> private static final String CERTIFICATE_1 = "...";
>>>>>> private static final String CERTIFICATE_2 = "...";
>>>>>>
>>>>>> public static void main(String... args) throws
>>>>>> CertificateException {
>>>>>>
>>>>>> Recording r = new Recording();
>>>>>> r.enable(EventNames.X509Certificate).withoutStackTrace();
>>>>>> r.start();
>>>>>>
>>>>>> CertificateFactory cf =
>>>>>> CertificateFactory.getInstance("X.509");
>>>>>> cf.generateCertificate(new
>>>>>> ByteArrayInputStream(CERTIFICATE_1.getBytes()));
>>>>>> cf.generateCertificate(new
>>>>>> ByteArrayInputStream(CERTIFICATE_2.getBytes()));
>>>>>>
>>>>>> // Make sure only one event per certificate
>>>>>> cf.generateCertificate(new
>>>>>> ByteArrayInputStream(CERTIFICATE_1.getBytes()));
>>>>>> cf.generateCertificate(new
>>>>>> ByteArrayInputStream(CERTIFICATE_2.getBytes()));
>>>>>>
>>>>>> r.stop();
>>>>>>
>>>>>> List<RecordedEvent> events = Events.fromRecording(r);
>>>>>> Asserts.assertEquals(events.size(), 2, "Expected two
>>>>>> X.509 Certificate events");
>>>>>>
>>>>>> assertEvent(events, "1000", "SHA256withRSA",
>>>>>> "CN=SSLCertificate, O=SomeCompany",
>>>>>> "CN=Intermediate CA Cert, O=SomeCompany",
>>>>>> "RSA", 2048);
>>>>>> assertEvent(events, "1000", "SHA256withRSA",
>>>>>> "CN=SSLCertificate, O=SomeCompany",
>>>>>> "CN=Intermediate CA Cert, O=SomeCompany",
>>>>>> "RSA", 2048);
>>>>>> }
>>>>>>
>>>>>> private static void assertEvent(List<RecordedEvent> events,
>>>>>> String certID, String algId, String subject,
>>>>>> String issuer, String keyType, int length) throws
>>>>>> Exception {
>>>>>>
>>>>>> for (RecordedEvent e : events) {
>>>>>> if (e.getString("serialNumber").equals(certID)) {
>>>>>> Events.assertField(e, "algId").equal(algId);
>>>>>> ...
>>>>>> return;
>>>>>> }
>>>>>> }
>>>>>> System.out.println(events);
>>>>>> throw new Exception("Could not find event with Cert ID: "
>>>>>> + certID);
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> The reworked example uses the Events.assertField method, which
>>>>>> will give context if the assertion fails. Keeping the test simple,
>>>>>> means it can be analyzed quickly if it fails in testing. There is
>>>>>> no new test framework to learn, or methods to search for, and it
>>>>>> is usually not hard to determine if the failure is product, test
>>>>>> or infrastructure related, and what component (team) should be
>>>>>> assigned the issue. The use of EventNames.X509Certificate means
>>>>>> all occurrences of the event can be tracked done in an IDE using
>>>>>> find by reference.
>>>>>>
>>>>>> Thanks
>>>>>> Erik
>>>>>>
>>>>>>> Following on from the recent JDK-8203629 code review, I'd like to
>>>>>>> propose enhancements on how we can record events in security
>>>>>>> libs. The introduction of the JFR libraries can give us much
>>>>>>> better ways of examining JDK actions. For the initial phase, I'm
>>>>>>> looking to enhance some key security library events in JDK 11 so
>>>>>>> that they can be either recorded to JFR, logged to a traditional
>>>>>>> logger, or both.
>>>>>>>
>>>>>>> Examples of how useful JFR recordings could be can be seen here :
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~coffeys/event_snaps/X509Event_1.png
>>>>>>> http://cr.openjdk.java.net/~coffeys/event_snaps/securityProp_1.png
>>>>>>> http://cr.openjdk.java.net/~coffeys/event_snaps/securityProp_2.png
>>>>>>> http://cr.openjdk.java.net/~coffeys/event_snaps/TLSEvent_1.png
>>>>>>>
>>>>>>> securityProp_2.png gives an example of how the JFR recording can
>>>>>>> be queried to quickly locate events of interest (in this case,
>>>>>>> code setting the jdk.tls.* Security properties). I still need to
>>>>>>> clean up the TLSEvents testcase to improve test coverage and hope
>>>>>>> to do that in coming days.
>>>>>>>
>>>>>>> JBS record :
>>>>>>> * https://bugs.openjdk.java.net/browse/JDK-8148188
>>>>>>>
>>>>>>> webrev :
>>>>>>> http://cr.openjdk.java.net/~coffeys/webrev.8148188.v1/webrev/
>>>>>>>
>>>>>>
>>>>>
>>>>
>
More information about the core-libs-dev
mailing list