RFR 8165996: PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8165996: PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite

Martin Balao
Hi,

I'd like to propose a fix for JDK-8165996 - PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite [1].

Webrev01:


Kind regards,
Martin.-

--
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8165996: PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite

Weijun Wang
Hi Martin

I'm just starting to read this patch. Two questions:

1. Is there a webpage on configDir using sql:/?

2. Your test hardcoded nssLibraryDirectory to be "/lib64". It would need to be changed to either those inclosed the repository (macOS and Windows) or in the system (others). Is there a version requirement?

3. The test contains a lot of binary data. Can you describe more clearly on which is from where? Especially key4Content and cert9Content? In fact, can they be recreated from the existing file based db inside test/jdk/sun/security/pkcs11/nss/db? If yes, the test will be much shorter. Please at least use multiple lines for the 2 keys.

Thanks
Max

> On Nov 29, 2017, at 10:11 PM, Martin Balao <[hidden email]> wrote:
>
> Hi,
>
> I'd like to propose a fix for JDK-8165996 - PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite [1].
>
> Webrev01:
>
>  * http://cr.openjdk.java.net/~akasko/mbalao/8165996.webrev.01/ (browse online)
>  * http://cr.openjdk.java.net/~akasko/mbalao/8165996.webrev.01.zip (download)
>
> Kind regards,
> Martin.-
>
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8165996

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8165996: PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite

Weijun Wang
Hi Martin

I've made some change and post a new webrev at

  http://cr.openjdk.java.net/~weijun/8165996/webrev.00/

The src part is unchanged. Major changes to test are:

1. PKCS11Test.getNSSLibDir() is used to get the NSS lib dir. Honestly this is my 1st time touching NSS so hopefully it's not wrong.

2. I didn't used your private key and certs. Instead, an internal class CertAndKeyGen is used.

3. I've saved "key4.db" and "cert9.db" as real files inside nss/sqlite. I know binary files are extremely unwelcome in an open source project, but maybe this time this is acceptable. We already have nss/db and nss/sqlite is certainly not worse, and maybe we can write more test using this backend later.

4. I also moved "nssdbsqlite" from /tmp to the current working directory. For jtreg, cwd is always empty and will be cleaned/retained after a test run. More importantly, no two test runs will use the same cwd.

So nothing really changed. I still need to read about sql:/ to understand the src fix.

Thanks
Max

> On Dec 8, 2017, at 2:33 PM, Weijun Wang <[hidden email]> wrote:
>
> Hi Martin
>
> I'm just starting to read this patch. Two questions:
>
> 1. Is there a webpage on configDir using sql:/?
>
> 2. Your test hardcoded nssLibraryDirectory to be "/lib64". It would need to be changed to either those inclosed the repository (macOS and Windows) or in the system (others). Is there a version requirement?
>
> 3. The test contains a lot of binary data. Can you describe more clearly on which is from where? Especially key4Content and cert9Content? In fact, can they be recreated from the existing file based db inside test/jdk/sun/security/pkcs11/nss/db? If yes, the test will be much shorter. Please at least use multiple lines for the 2 keys.
>
> Thanks
> Max
>
>> On Nov 29, 2017, at 10:11 PM, Martin Balao <[hidden email]> wrote:
>>
>> Hi,
>>
>> I'd like to propose a fix for JDK-8165996 - PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite [1].
>>
>> Webrev01:
>>
>> * http://cr.openjdk.java.net/~akasko/mbalao/8165996.webrev.01/ (browse online)
>> * http://cr.openjdk.java.net/~akasko/mbalao/8165996.webrev.01.zip (download)
>>
>> Kind regards,
>> Martin.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8165996
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8165996: PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite

Weijun Wang


> On Dec 8, 2017, at 4:55 PM, Weijun Wang <[hidden email]> wrote:
>
> Hi Martin
>
> I've made some change and post a new webrev at
>
>  http://cr.openjdk.java.net/~weijun/8165996/webrev.00/

More change in the same URL.

- key4.db and cert9.db are saved in Secmod.

- I modified the existing PKCS11Test/SecmodTest to load sqlite based dbs.

Thanks
Max

>
> The src part is unchanged. Major changes to test are:
>
> 1. PKCS11Test.getNSSLibDir() is used to get the NSS lib dir. Honestly this is my 1st time touching NSS so hopefully it's not wrong.
>
> 2. I didn't used your private key and certs. Instead, an internal class CertAndKeyGen is used.
>
> 3. I've saved "key4.db" and "cert9.db" as real files inside nss/sqlite. I know binary files are extremely unwelcome in an open source project, but maybe this time this is acceptable. We already have nss/db and nss/sqlite is certainly not worse, and maybe we can write more test using this backend later.
>
> 4. I also moved "nssdbsqlite" from /tmp to the current working directory. For jtreg, cwd is always empty and will be cleaned/retained after a test run. More importantly, no two test runs will use the same cwd.
>
> So nothing really changed. I still need to read about sql:/ to understand the src fix.
>
> Thanks
> Max
>
>> On Dec 8, 2017, at 2:33 PM, Weijun Wang <[hidden email]> wrote:
>>
>> Hi Martin
>>
>> I'm just starting to read this patch. Two questions:
>>
>> 1. Is there a webpage on configDir using sql:/?
>>
>> 2. Your test hardcoded nssLibraryDirectory to be "/lib64". It would need to be changed to either those inclosed the repository (macOS and Windows) or in the system (others). Is there a version requirement?
>>
>> 3. The test contains a lot of binary data. Can you describe more clearly on which is from where? Especially key4Content and cert9Content? In fact, can they be recreated from the existing file based db inside test/jdk/sun/security/pkcs11/nss/db? If yes, the test will be much shorter. Please at least use multiple lines for the 2 keys.
>>
>> Thanks
>> Max
>>
>>> On Nov 29, 2017, at 10:11 PM, Martin Balao <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> I'd like to propose a fix for JDK-8165996 - PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite [1].
>>>
>>> Webrev01:
>>>
>>> * http://cr.openjdk.java.net/~akasko/mbalao/8165996.webrev.01/ (browse online)
>>> * http://cr.openjdk.java.net/~akasko/mbalao/8165996.webrev.01.zip (download)
>>>
>>> Kind regards,
>>> Martin.-
>>>
>>> --
>>> [1] - https://bugs.openjdk.java.net/browse/JDK-8165996
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8165996: PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite

Weijun Wang
Hi Martin

Your src change looks fine, and if you think my test update is good, I can push the changeset.

Still, I need one confirmation. The modutil man page has "modutil supports two types of databases: the legacy security
databases (cert8.db, key3.db, and secmod.db) and new SQLite databases (cert9.db, key4.db, and pkcs11.txt)". So it looks like the pkcs11.txt file is optional?

Thanks
Max


> On Dec 11, 2017, at 11:16 AM, Weijun Wang <[hidden email]> wrote:
>
>
>
>> On Dec 8, 2017, at 4:55 PM, Weijun Wang <[hidden email]> wrote:
>>
>> Hi Martin
>>
>> I've made some change and post a new webrev at
>>
>> http://cr.openjdk.java.net/~weijun/8165996/webrev.00/
>
> More change in the same URL.
>
> - key4.db and cert9.db are saved in Secmod.
>
> - I modified the existing PKCS11Test/SecmodTest to load sqlite based dbs.
>
> Thanks
> Max
>
>>
>> The src part is unchanged. Major changes to test are:
>>
>> 1. PKCS11Test.getNSSLibDir() is used to get the NSS lib dir. Honestly this is my 1st time touching NSS so hopefully it's not wrong.
>>
>> 2. I didn't used your private key and certs. Instead, an internal class CertAndKeyGen is used.
>>
>> 3. I've saved "key4.db" and "cert9.db" as real files inside nss/sqlite. I know binary files are extremely unwelcome in an open source project, but maybe this time this is acceptable. We already have nss/db and nss/sqlite is certainly not worse, and maybe we can write more test using this backend later.
>>
>> 4. I also moved "nssdbsqlite" from /tmp to the current working directory. For jtreg, cwd is always empty and will be cleaned/retained after a test run. More importantly, no two test runs will use the same cwd.
>>
>> So nothing really changed. I still need to read about sql:/ to understand the src fix.
>>
>> Thanks
>> Max
>>
>>> On Dec 8, 2017, at 2:33 PM, Weijun Wang <[hidden email]> wrote:
>>>
>>> Hi Martin
>>>
>>> I'm just starting to read this patch. Two questions:
>>>
>>> 1. Is there a webpage on configDir using sql:/?
>>>
>>> 2. Your test hardcoded nssLibraryDirectory to be "/lib64". It would need to be changed to either those inclosed the repository (macOS and Windows) or in the system (others). Is there a version requirement?
>>>
>>> 3. The test contains a lot of binary data. Can you describe more clearly on which is from where? Especially key4Content and cert9Content? In fact, can they be recreated from the existing file based db inside test/jdk/sun/security/pkcs11/nss/db? If yes, the test will be much shorter. Please at least use multiple lines for the 2 keys.
>>>
>>> Thanks
>>> Max
>>>
>>>> On Nov 29, 2017, at 10:11 PM, Martin Balao <[hidden email]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> I'd like to propose a fix for JDK-8165996 - PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite [1].
>>>>
>>>> Webrev01:
>>>>
>>>> * http://cr.openjdk.java.net/~akasko/mbalao/8165996.webrev.01/ (browse online)
>>>> * http://cr.openjdk.java.net/~akasko/mbalao/8165996.webrev.01.zip (download)
>>>>
>>>> Kind regards,
>>>> Martin.-
>>>>
>>>> --
>>>> [1] - https://bugs.openjdk.java.net/browse/JDK-8165996
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8165996: PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite

Martin Balao
Hi Max,

Thanks for your time and review.

Test refactorings look good to me :-)

In regard to pkcs11.txt, we are currently using the cfg file to store configuration information (as before). I suggest to keep using it to leverage on previous work. The only change we are actually doing to configuration information is the "sql:/" prefix on the db path (to indicate a sqllite db).

Kind regards,
Martin.-


On Mon, Dec 11, 2017 at 11:40 AM, Weijun Wang <[hidden email]> wrote:
Hi Martin

Your src change looks fine, and if you think my test update is good, I can push the changeset.

Still, I need one confirmation. The modutil man page has "modutil supports two types of databases: the legacy security
databases (cert8.db, key3.db, and secmod.db) and new SQLite databases (cert9.db, key4.db, and pkcs11.txt)". So it looks like the pkcs11.txt file is optional?

Thanks
Max


> On Dec 11, 2017, at 11:16 AM, Weijun Wang <[hidden email]> wrote:
>
>
>
>> On Dec 8, 2017, at 4:55 PM, Weijun Wang <[hidden email]> wrote:
>>
>> Hi Martin
>>
>> I've made some change and post a new webrev at
>>
>> http://cr.openjdk.java.net/~weijun/8165996/webrev.00/
>
> More change in the same URL.
>
> - key4.db and cert9.db are saved in Secmod.
>
> - I modified the existing PKCS11Test/SecmodTest to load sqlite based dbs.
>
> Thanks
> Max
>
>>
>> The src part is unchanged. Major changes to test are:
>>
>> 1. PKCS11Test.getNSSLibDir() is used to get the NSS lib dir. Honestly this is my 1st time touching NSS so hopefully it's not wrong.
>>
>> 2. I didn't used your private key and certs. Instead, an internal class CertAndKeyGen is used.
>>
>> 3. I've saved "key4.db" and "cert9.db" as real files inside nss/sqlite. I know binary files are extremely unwelcome in an open source project, but maybe this time this is acceptable. We already have nss/db and nss/sqlite is certainly not worse, and maybe we can write more test using this backend later.
>>
>> 4. I also moved "nssdbsqlite" from /tmp to the current working directory. For jtreg, cwd is always empty and will be cleaned/retained after a test run. More importantly, no two test runs will use the same cwd.
>>
>> So nothing really changed. I still need to read about sql:/ to understand the src fix.
>>
>> Thanks
>> Max
>>
>>> On Dec 8, 2017, at 2:33 PM, Weijun Wang <[hidden email]> wrote:
>>>
>>> Hi Martin
>>>
>>> I'm just starting to read this patch. Two questions:
>>>
>>> 1. Is there a webpage on configDir using sql:/?
>>>
>>> 2. Your test hardcoded nssLibraryDirectory to be "/lib64". It would need to be changed to either those inclosed the repository (macOS and Windows) or in the system (others). Is there a version requirement?
>>>
>>> 3. The test contains a lot of binary data. Can you describe more clearly on which is from where? Especially key4Content and cert9Content? In fact, can they be recreated from the existing file based db inside test/jdk/sun/security/pkcs11/nss/db? If yes, the test will be much shorter. Please at least use multiple lines for the 2 keys.
>>>
>>> Thanks
>>> Max
>>>
>>>> On Nov 29, 2017, at 10:11 PM, Martin Balao <[hidden email]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> I'd like to propose a fix for JDK-8165996 - PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite [1].
>>>>
>>>> Webrev01:
>>>>
>>>> * http://cr.openjdk.java.net/~akasko/mbalao/8165996.webrev.01/ (browse online)
>>>> * http://cr.openjdk.java.net/~akasko/mbalao/8165996.webrev.01.zip (download)
>>>>
>>>> Kind regards,
>>>> Martin.-
>>>>
>>>> --
>>>> [1] - https://bugs.openjdk.java.net/browse/JDK-8165996
>>>
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: RFR 8165996: PKCS11 using NSS throws an error regarding secmod.db when NSS uses sqlite

Weijun Wang
http://hg.openjdk.java.net/jdk/jdk/rev/55b9b1e184c6

> On Dec 13, 2017, at 1:17 AM, Martin Balao <[hidden email]> wrote:
>
> Hi Max,
>
> Thanks for your time and review.
>
> Test refactorings look good to me :-)
>
> In regard to pkcs11.txt, we are currently using the cfg file to store configuration information (as before). I suggest to keep using it to leverage on previous work. The only change we are actually doing to configuration information is the "sql:/" prefix on the db path (to indicate a sqllite db).
>
> Kind regards,
> Martin.-