[jitsi-dev] [jitsi] Improved X509CertificatePanel displaying certificate chain (#44)


#1

Changes the X509CertificatePanel to display all certificates (sent by server) in a tree view offering the user to select each certificate to be displayed.
Also makes all certificate text selectable (it is now an editor pane).

Screen shots has previously been discussed in this thread:
http://lists.jitsi.org/pipermail/dev/2014-May/020793.html

Note/question:
The resource strings (i18n) for the headings should no longer contain tags like "<html><b>" etc, should that be updated via translate.jitsi.org and not in the resource_xx.properties?
You can merge this Pull Request by running:

  git pull https://github.com/netmackan/jitsi certificate-panel-4

Or you can view, comment on it, or merge it online at:

  https://github.com/jitsi/jitsi/pull/44

-- Commit Summary --

  * Initial plumbing for improved certificate panel.
  * Improved X509CertificatePanel with support for displaying multiple certificates.
  * Fixing formatting issue.
  * Missed one i18n string.

-- File Changes --

    M resources/languages/resources.properties (13)
    M src/net/java/sip/communicator/plugin/desktoputil/VerifyCertificateDialogImpl.java (43)
    M src/net/java/sip/communicator/plugin/desktoputil/ViewCertificateFrame.java (15)
    M src/net/java/sip/communicator/plugin/desktoputil/X509CertificatePanel.java (494)

-- Patch Links --

https://github.com/jitsi/jitsi/pull/44.patch
https://github.com/jitsi/jitsi/pull/44.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44


#2

@@ -8,6 +8,7 @@

import java.awt.*;
import java.security.cert.*;
+import java.util.Arrays;

Wildcard import if it doesn't cause conflicts.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44/files#r15732889


#3

+ */
+ public X509CertificatePanel(java.util.List<X509Certificate> certificates)
+ {
+ setLayout(new BorderLayout(5, 5));
+
+ // Certificate chain list
+ TransparentPanel topPanel = new TransparentPanel(new BorderLayout());
+ topPanel.add(new JLabel("<html><body><b>"
+ + R.getI18NString("service.gui.CERT_INFO_CHAIN")
+ + "</b></body></html>"), BorderLayout.NORTH);
+
+ DefaultMutableTreeNode top = new DefaultMutableTreeNode();
+ DefaultMutableTreeNode previous = top;
+ ListIterator<X509Certificate> it = certificates.listIterator(
+ certificates.size());
+ while (it.hasPrevious()) {

{ on new line please

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44/files#r15732893


#4

- Insets valueInsets = new Insets(2,10,0,0);
- Insets titleInsets = new Insets(10,5,0,0);
+ /**
+ * Constructs a X509 certificate panel.
+ *
+ * @param certificates <tt>X509Certificate</tt> objects
+ */
+ public X509CertificatePanel(java.util.List<X509Certificate> certificates)

Why do you use a List<> here? Keeping everything as the array it was from the beginning seems enough to me?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44/files#r15732904


#5

+ DefaultMutableTreeNode previous = top;
+ ListIterator<X509Certificate> it = certificates.listIterator(
+ certificates.size());
+ while (it.hasPrevious()) {
+ X509Certificate cert = it.previous();
+ DefaultMutableTreeNode next = new DefaultMutableTreeNode(cert);
+ previous.add(next);
+ previous = next;
+ }
+ JTree tree = new JTree(top);
+ tree.setBorder(new BevelBorder(BevelBorder.LOWERED));
+ tree.setRootVisible(false);
+ tree.setExpandsSelectedPaths(true);
+ tree.getSelectionModel().setSelectionMode(
+ TreeSelectionModel.SINGLE_TREE_SELECTION);
+ tree.setCellRenderer(new DefaultTreeCellRenderer() {

{ new line

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44/files#r15732905


#6

Yes, the translations need to be updated via Pootle (and wasn't a good idea to have it there to start with, but different subject).

I'll merge this when you have fixed the { formattings and maybe removed the List (unless this is necessary and I haven't seen why from flying through).

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44#issuecomment-50981760


#7

- Insets valueInsets = new Insets(2,10,0,0);
- Insets titleInsets = new Insets(10,5,0,0);
+ /**
+ * Constructs a X509 certificate panel.
+ *
+ * @param certificates <tt>X509Certificate</tt> objects
+ */
+ public X509CertificatePanel(java.util.List<X509Certificate> certificates)

Previously it was only one constructor taking a single X509Certificate object. So no array as far as I can see unless you refer to the array passed to the ViewCertificateFrame. However, that array is of the Certificate type and might (in theory at least) contain any implementation of Certificate, not necessarily instanceof X509Certificate.
Anyway, I changed so the X509Certificate panel uses an array type instead of the collection now.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44/files#r15734199


#8

I had a second look and got to the conclusion that the most clean solution would be to expand the scope of the X509CertificatePanel to be able to display any Certificate instances. That also removed some duplicated code for handling the non-X.509 case in different places.

This pull request has been updated.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44#issuecomment-51325695


#9

Merged #44.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44#event-149801572


#10

I indend to start fixing the translations for the strings that should no longer should contain html tags using:
1) mkdir orig mine
2) Download each xx.po from translate.jitsi.org

$ curl --remote-name-all http://translate.jitsi.org/export/sip_communicator/{sq,ar,ast,ba,bg,nl,fi,el,he,hi,id,ga,is,it,ja,zh_CN,hr,lv,lt,mk,ml,pl,pt,ro,ru,gd,sr,si,sk,sl,es,sv,cs,tr,de,uk.po}.po

3) cp orig/*.po mine/

sed -r -i "/<html><b>Issued To<\/b><\/html>/I{n; s/<html><b>(.*)<\/b><\/html>/\1/}" mine/*.po
sed -r -i "/<html><b>Issued By<\/b><\/html>/I{n; s/<html><b>(.*)<\/b><\/html>/\1/}" mine/*.po
sed -r -i "/<html><b>Validity<\/b><\/html>/I{n; s/<html><b>(.*)<\/b><\/html>/\1/}" mine/*.po
sed -r -i "/<html><b>Fingerprints<\/b><\/html>/I{n; s/<html><b>(.*)<\/b><\/html>/\1/}" mine/*.po
sed -r -i "/<html><b>Certificate Info<\/b><\/html>/I{n; s/<html><b>(.*)<\/b><\/html>/\1/}" mine/*.po
sed -r -i "/<html><b>Public Key Info<\/b><\/html>/I{n; s/<html><b>(.*)<\/b><\/html>/\1/}" mine/*.po

4) diff -ur orig/ mine/ | less

5) Translate to properties files and test with Jitsi
For each orig/*.po: po2prop --personality=mozilla -t resources.properties mine/xx.po mine/resources_xx.properties

    for file in mine/*.po ; do po2prop --personality=mozilla -t resources.properties $file `echo $file | sed 's/mine\/\(.*\)\.po$/mine\/resources_\1\.properties/'` ; done

6) Upload each new po

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44#issuecomment-51714223


#11

I guess step 4 is a visual inspection? Go for it!

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44#issuecomment-51718267


#12

Yes, exactly. I was just scrolling through the changes to see that it looked about right.
For step 5 I just checked a few of the different languages.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/44#issuecomment-51721055