SPARK-1934, SPARK-1939 Certificate extensions. #351

Alameyo · 2017-06-27T22:12:31Z

No description provided.

Alameyo · 2017-06-28T07:42:02Z

I added new class translating OIDs. I could use just resources but i think that give some place to expand it in the future with things like getFatherOID(), getChildren() if I would need that. I couldn't extract some extensions. In this case such extension goes to TextArea unsupported extensions where it's OID and if possible it's description goes but not value. I think it's quite impossible to get all extensions/it's values as some can be custom in use by only particular companies (like Microsoft).

I am not sure yet if extensions 2.5.29.9 and 2.5.29.36 works fine, in the worst case it can be dropped to unsupported extensions.

Also sometimes is shown here error with missing labels on left but it was resolved in other PR so to prevent conflicts I don't solve such things again.

guusdk · 2017-06-30T14:21:08Z

I cannot open the "advanced" window any more. When I click on the button (in the login screen), this exception is being logged:

Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
	at org.jivesoftware.sparkimpl.certificates.CertificateModel.extensionExtractHandler(CertificateModel.java:291)
	at org.jivesoftware.sparkimpl.certificates.CertificateModel.setupExtensions(CertificateModel.java:189)
	at org.jivesoftware.sparkimpl.certificates.CertificateModel.<init>(CertificateModel.java:172)
	at org.jivesoftware.sparkimpl.certificates.CertificateModel.<init>(CertificateModel.java:133)
	at org.jivesoftware.sparkimpl.certificates.CertificateController.<init>(CertificateController.java:88)
	at org.jivesoftware.spark.ui.login.CertificatesManagerSettingsPanel.<init>(CertificatesManagerSettingsPanel.java:74)
	at org.jivesoftware.spark.ui.login.LoginSettingDialog.<init>(LoginSettingDialog.java:58)
	at org.jivesoftware.LoginDialog$LoginPanel.actionPerformed(LoginDialog.java:737)

Alameyo · 2017-06-30T15:56:57Z

Ok, seems I missed some situations when some values in extensions can be null. I will add some more checks for null values and I will add catching NullPointerException as it seems I cannot predict every possible situations for every extension.

Alameyo · 2017-07-01T19:07:45Z

I added some nullchecks and just in case I catch now NullPointerException. I also added here SPARK-1955 which is information about criticality of extensions.

…actHandler. Also added information about criticality of extension.

guusdk

Now it works! :)

Something is a little odd: when I look at one certificate, and use my mouse wheel to scroll, it is much less sensitive than in other parts of Spark: I have to scroll a lot to move the scrollbar just a bit. It is very different from the overview table, for example. It's not a big deal, but if you can easily fix this, it would be good.

Although it is working as intended, I don't like how the User Interface is evolving. The certificate detail window is now becoming one big blob of text input fields. We should make this prettier, if we have time. Please create an issue for this, but don't work on it unless we have extra time left.

guusdk · 2017-07-03T09:10:36Z

core/src/main/java/org/jivesoftware/sparkimpl/certificates/CertificateModel.java

+	private String policyMappingsExtension; // OID 2.5.29.33
+	private String authorityKeyIdentifierExtension; // OID 2.5.29.35
+	private String policyConstraintsExtension; // OID 2.5.29.36
+	private String extendedKeyUsageExtension; // OID 2.5.29.37


You are keeping a lot of duplicate data in each instance of the Certificate Model. It's not wrong but you will see that this quickly results in weird bugs (where one "duplicate" field has different content as compared to a field that should have the same values, but does not). I advice that you don't store duplicates at all.

guusdk · 2017-07-03T09:12:11Z

core/src/main/java/org/jivesoftware/sparkimpl/certificates/CertificateModel.java

+	private void extensionExtractHandler(X509Certificate cert, String oid, boolean critical) {
+		try {
+			ASN1Primitive primitive;
+			if (oid.equals("2.5.29.9")) {


Try to use constants, instead of string values for OIDs. Bouncy Castle has nice ones:

final Extension result = new JcaX509CertificateHolder( certificate ).getExtensions() .getExtension( Extension.authorityKeyIdentifier );

guusdk · 2017-07-03T09:13:17Z

core/src/main/java/org/jivesoftware/sparkimpl/certificates/CertificateModel.java

+				try {
+					primitive = JcaX509ExtensionUtils.parseExtensionValue(cert.getExtensionValue(oid));
+					SubjectDirectoryAttributes sub = SubjectDirectoryAttributes.getInstance(primitive);
+					subjectDirectoryAttributesExtension = Res.getString("cert.is.critical") + critical + "\n";


Here, you're composing the text that the end user sees, correct? Should you do that in the Model (instead of the View)?

I'm not sure: perhaps you need a "certificate-extension" view?

This CertificateModel is made mostly to prepare text to show in view windows. Model of certificate for other use is mostly covered by X509Certificate class which I might to extend in future but I don't feel that I need that now.

guusdk · 2017-07-03T09:17:07Z

core/src/main/java/org/jivesoftware/sparkimpl/certificates/CertificateModel.java

+		} catch (NullPointerException e) {
+			Log.error("Couldn't extract " + oid + ": " + OIDTranslator.getDescription(oid) + "extension.", e);
+			addToUnsupported(critical, oid);
+		}


This is getting a very, very long method, which will be hard to maintain. Can we improve on the code style here?

Alameyo · 2017-07-04T11:06:33Z

I also deleted one constructor, that with pure String arguments as I don't think I will need it.

guusdk · 2017-07-04T11:31:52Z

That's certainly an improvement to the code, thanks! The scroll issue, and the issue of the UI becoming very very messy remains. Can you improve on that, please?

guusdk · 2017-07-07T11:24:05Z

I'm merging this, after @Alameyo took a blood-oath promising that the remaining issues, captured in SPARK-1958, would be resolved later.

Alameyo requested a review from guusdk June 27, 2017 22:12

Alameyo force-pushed the extensions_extraction branch from 68a872b to 052b9bc Compare June 30, 2017 09:45

Extensions extraction

a2d9f70

Alameyo force-pushed the extensions_extraction branch from 052b9bc to a2d9f70 Compare June 30, 2017 10:15

Added more null checks, caching NullPointerException in extensionExtr…

1a58bc8

…actHandler. Also added information about criticality of extension.

Alameyo force-pushed the extensions_extraction branch from 60ddbd0 to 1a58bc8 Compare July 1, 2017 19:13

guusdk reviewed Jul 3, 2017

View reviewed changes

guusdk requested changes Jul 3, 2017

View reviewed changes

Cleaning code, removed constructor without certificate.

4d7cf01

guusdk merged commit ef8e73f into igniterealtime:master Jul 7, 2017

Alameyo deleted the extensions_extraction branch July 9, 2017 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPARK-1934, SPARK-1939 Certificate extensions. #351

SPARK-1934, SPARK-1939 Certificate extensions. #351

Alameyo commented Jun 27, 2017

Alameyo commented Jun 28, 2017 •

edited

Loading

guusdk commented Jun 30, 2017

Alameyo commented Jun 30, 2017

Alameyo commented Jul 1, 2017 •

edited

Loading

guusdk left a comment

guusdk Jul 3, 2017

guusdk Jul 3, 2017

guusdk Jul 3, 2017

guusdk Jul 3, 2017

Alameyo Jul 3, 2017

guusdk Jul 3, 2017

Alameyo commented Jul 4, 2017 •

edited

Loading

guusdk commented Jul 4, 2017

guusdk commented Jul 7, 2017

SPARK-1934, SPARK-1939 Certificate extensions. #351

SPARK-1934, SPARK-1939 Certificate extensions. #351

Conversation

Alameyo commented Jun 27, 2017

Alameyo commented Jun 28, 2017 • edited Loading

guusdk commented Jun 30, 2017

Alameyo commented Jun 30, 2017

Alameyo commented Jul 1, 2017 • edited Loading

guusdk left a comment

Choose a reason for hiding this comment

guusdk Jul 3, 2017

Choose a reason for hiding this comment

guusdk Jul 3, 2017

Choose a reason for hiding this comment

guusdk Jul 3, 2017

Choose a reason for hiding this comment

guusdk Jul 3, 2017

Choose a reason for hiding this comment

Alameyo Jul 3, 2017

Choose a reason for hiding this comment

guusdk Jul 3, 2017

Choose a reason for hiding this comment

Alameyo commented Jul 4, 2017 • edited Loading

guusdk commented Jul 4, 2017

guusdk commented Jul 7, 2017

Alameyo commented Jun 28, 2017 •

edited

Loading

Alameyo commented Jul 1, 2017 •

edited

Loading

Alameyo commented Jul 4, 2017 •

edited

Loading