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