1307993 - Expose updated certificate verification function in JSS

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect)

Comment 1

[Elio Maldonado]

Attachment: Expose updated certificate verification function

Christina's patch updated for current sources, not ready yet for review.

Comment 2

[Elio Maldonado]

jss-VerifyCertificate.patch will need additional updates.

Comment 3

[Elio Maldonado]

Attachment: Expose updated certificate verification function

The one that I tested.

Comment 4

[Elio Maldonado]

Attachment: Expose updated certificate verification function

Comment 5

[Elio Maldonado]

Attachment: the build log

After I had submitted the patch for review I noticed many warnings. This is the build log which I should discuss with Jack, the reviewer, and if needed also with Christina, the patch original author, as part of the review process.

Comment 6

[Elio Maldonado]

Attachment: the test log

Comment 7

[Elio Maldonado]

Comment on attachment 8838784 [details] [diff] [review]
Expose updated certificate verification function

Review of attachment 8838784 [details] [diff] [review]:
-----------------------------------------------------------------

Some concerns below.

::: lib/jss.def
@@ +125,5 @@
>  Java_org_mozilla_jss_pkcs11_PK11Store_deletePrivateKey;
>  Java_org_mozilla_jss_pkcs11_PK11Store_importPrivateKey;
>  Java_org_mozilla_jss_pkcs11_PK11Store_putCertsInVector;
>  Java_org_mozilla_jss_pkcs11_PK11Store_putKeysInVector;
> +Java_org_mozilla_jss_pkcs11_PK11Store_putSymKeysInVector;

worth examining, will explain later

@@ +215,5 @@
>  Java_org_mozilla_jss_pkcs11_PK11PubKey_fromSPKI;
>  Java_org_mozilla_jss_provider_java_security_JSSKeyStoreSpi_engineGetKey;
>  Java_org_mozilla_jss_provider_java_security_JSSKeyStoreSpi_engineIsCertificateEntry;
>  Java_org_mozilla_jss_provider_java_security_JSSKeyStoreSpi_engineSetKeyEntryNative;
> +Java_org_mozilla_jss_CryptoManager_initializeAllNative2;

worth examining, will explain later

@@ +292,5 @@
>  Java_org_mozilla_jss_pkcs11_PK11Token_needsLogin;
>  ;+    local:
>  ;+       *;
>  ;+};
> +;+JSS_4.2.6 {     # JSS 4.2.6 release

This JSS_4.2.6 release entry appears after the JSS_4.3 one which is odd. I understand that 4.2.6 release was a Red Hat release which Mozilla never made. 
Still we must be careful with things like this. See this guideline
https://wiki.mozilla.org/NSS/Release_Management#Verify_correctness
Following the instruction given there
$ hg diff -r JSS_4_3_2_RTM -r default | lsdiff |grep \.def$
b/lib/jss.def
tells me this file file should be examined. That's why I bring that up.
Should JSS be bound by NSPR and NSS guiedines? I can't answer that with certainty. Let's discuss this either here or off-line.

Comment 8

[Elio Maldonado]

(In reply to Elio Maldonado from comment #7)

> $ hg diff -r JSS_4_3_2_RTM -r default | lsdiff |grep \.def$
> b/lib/jss.def
> tells me this file file should be examined. That's why I bring that up.
> Should JSS be bound by NSPR and NSS guiedines? I can't answer that with
> certainty. Let's discuss this either here or off-line.

I can answer my own question. The rule makes sense for NSS but no so much for JSS. The reason being is that in NSS we declare some functions in public headers that are for the sole use of NSS and not to be used my client code. A functions is not really public unless it is added to nss.def (or ssl.def, until.def, and so on) For JSS the C code is for JNI so the java code can access the new functions and as far as I know no other code except JSS would be having access to what's exported by jas4.so. It's fine to leave the patch as is it. Only jss.def is involved, if this turns out to be incorrect we can always revise our decision at a later time.

Comment 9

[Elio Maldonado]

Attachment: Expose updated certificate verification function and address compiler warnings - only a subset

supplementary patch to address some of the compiler warnings, it should be applied after the previous one has been applied. I'm picking only the low hanging fruit and refraining from dealing with other warnings that are more difficult and doing so would be risky. Keeping it separate for now so you can concentrate on each part at a time and I'll a merge them into the main patch if you so request.

Comment 10

[Elio Maldonado]

Comment on attachment 8839223 [details] [diff] [review]
Expose updated certificate verification function and address compiler warnings - only a subset

I misspoke earlier, this is actually the the main patch with warning fixes merged into it.

Comment 11

[Elio Maldonado]

Attachment: Expose updated certificate verification function and address compiler warnings - only a subsetjss-VerifyCertificate.patch

the same as before, renamed as it was intended

Comment 12

[Elio Maldonado]

Comment on attachment 8835046 [details] [diff] [review]
Expose updated certificate verification function

The original one without any compiler warnings clean-up.

Comment 13

[Elio Maldonado]

Comment on attachment 8839269 [details] [diff] [review]
Expose updated certificate verification function and address compiler warnings - only a subsetjss-VerifyCertificate.patch

Has to much stuff in it.

Comment 14

[Elio Maldonado]

Attachment: jss address a subset of the compiler warnings

This is the supplementary patch I originally intended to attach.

Comment 15

[Matthew Harmsen]

Comment on attachment 8835046 [details] [diff] [review]
Expose updated certificate verification function

Matches downstream patch very well.

Comment 16

[Elio Maldonado]

Pushed: https://hg.mozilla.org/projects/jss/rev/aba466cb400d85d3bba54f4d5259a9ff7d639059

Comment 17

[Matthew Harmsen]

(In reply to Matthew Harmsen from comment #15)
> Comment on attachment 8835046 [details] [diff] [review]
> Expose updated certificate verification function
> 
> Matches downstream patch very well.

jss-VerifyCertificate.patch

NOTE: I have not yet reviewed the second patch in this bug.

Comment 18

[Elio Maldonado]

(In reply to Matthew Harmsen from comment #17)
> (In reply to Matthew Harmsen from comment #15)
> 
> NOTE: I have not yet reviewed the second patch in this bug.

After a discussion with Matt we agreed that it is best to postpone review if this patch until we reach Bug 1308008 where it fits better.