Expose updated certificate verification function in JSS

RESOLVED FIXED

Status

JSS
Library
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Elio Maldonado, Assigned: Christina Fu)

Tracking

Details

Attachments

(4 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

2 years ago
Blocks: 1307859
(Reporter)

Comment 1

2 years ago
Created attachment 8798643 [details] [diff] [review]
Expose updated certificate verification function

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

Comment 2

a year ago
jss-VerifyCertificate.patch will need additional updates.
(Reporter)

Comment 3

a year ago
Created attachment 8835046 [details] [diff] [review]
Expose updated certificate verification function

The one that I tested.
Attachment #8798643 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Assignee: glenbeasley → cfu
(Reporter)

Comment 4

a year ago
Created attachment 8838784 [details] [diff] [review]
Expose updated certificate verification function
Attachment #8835046 - Attachment is obsolete: true
Attachment #8838784 - Flags: review?(jmagne)
(Reporter)

Comment 5

a year ago
Created attachment 8838968 [details]
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.
(Reporter)

Comment 6

a year ago
Created attachment 8838969 [details]
the test log
(Reporter)

Comment 7

a year ago
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.
(Reporter)

Comment 8

a year ago
(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.
(Reporter)

Comment 9

a year ago
Created attachment 8839223 [details] [diff] [review]
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.
Attachment #8839223 - Flags: review?(jmagne)
(Reporter)

Comment 10

a year ago
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.
Attachment #8839223 - Attachment description: address compiler warnings - only a subset → Expose updated certificate verification function and address compiler warnings - only a subset
(Reporter)

Updated

a year ago
Attachment #8838784 - Attachment is obsolete: true
Attachment #8838784 - Flags: review?(jmagne)
(Reporter)

Comment 11

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

the same as before, renamed as it was intended
Attachment #8839223 - Attachment is obsolete: true
Attachment #8839223 - Flags: review?(jmagne)
Attachment #8839269 - Flags: review?(jmagne)
(Reporter)

Comment 12

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

The original one without any compiler warnings clean-up.
Attachment #8835046 - Attachment is obsolete: false
Attachment #8835046 - Flags: review?(mharmsen)
(Reporter)

Comment 13

a year ago
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.
Attachment #8839269 - Flags: review?(jmagne)
(Reporter)

Updated

a year ago
Attachment #8839269 - Attachment is obsolete: true
(Reporter)

Comment 14

a year ago
Created attachment 8839295 [details] [diff] [review]
jss address a subset of the compiler warnings

This is the supplementary patch I originally intended to attach.
Attachment #8839295 - Flags: review?(mharmsen)

Comment 15

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

Matches downstream patch very well.
Attachment #8835046 - Flags: review?(mharmsen) → review+
(Reporter)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED

Comment 17

a year ago
(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.
(Reporter)

Comment 18

a year ago
(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.
(Reporter)

Updated

a year ago
Attachment #8839295 - Flags: review?(mharmsen)
(Reporter)

Updated

a year ago
No longer blocks: 1307859
You need to log in before you can comment on or make changes to this bug.