Re: [PATCH 1/2] crypt32/tests: testing priorities and flags of stores in a collection; and whether certs are saved in registry endpoint

[Jacek Caban]

Hi Donat,


Sorry it took a while, I wanted to do some testing myself before 
reviewing. Patches look mostly good to me, I have a few comments.


On 10/28/16 5:15 PM, Donat Enikeev wrote:
> Superceeds: https://source.winehq.org/patches/data/127162
>
> v1:
> Test of priorities and flags of stores in a collection
> v2:
> Test whether certificates are actually saved in the registry for the most common pathes and one todo for HLCU\My saved in the AppData
>
> Signed-off-by: Donat Enikeev <donat(a)enikeev.net>
> ---
>   dlls/crypt32/tests/Makefile.in |   2 +-
>   dlls/crypt32/tests/store.c     | 288 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 289 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/crypt32/tests/Makefile.in b/dlls/crypt32/tests/Makefile.in
> index d84ca17..d96ad1c 100644
> --- a/dlls/crypt32/tests/Makefile.in
> +++ b/dlls/crypt32/tests/Makefile.in
> @@ -1,5 +1,5 @@
>   TESTDLL   = crypt32.dll
> -IMPORTS   = crypt32 advapi32
> +IMPORTS   = crypt32 advapi32 user32 shlwapi shell32
>
> C_SRCS = \
>   base64.c \
> diff --git a/dlls/crypt32/tests/store.c b/dlls/crypt32/tests/store.c
> index e811fbe..7fedaa4 100644
> --- a/dlls/crypt32/tests/store.c
> +++ b/dlls/crypt32/tests/store.c
> @@ -23,6 +23,9 @@
>
> #include <windef.h>
>   #include <winbase.h>
> +#include <winuser.h>
> +#include <shlobj.h>
> +#include <shlwapi.h>
>   #include <winreg.h>
>   #include <winerror.h>
>   #include <wincrypt.h>
> @@ -347,6 +350,287 @@ static const BYTE serializedStoreWithCert[] = {
>    0x08,0x30,0x06,0x01,0x01,0xff,0x02,0x01,0x01,0x00,0x00,0x00,0x00,0x00,0x00,
>    0x00,0x00,0x00,0x00,0x00,0x00 };
>
> +static const struct
> +{
> +    HKEY key;
> +    DWORD cert_store;
> +    BOOL todo_appdata_file;

Please remove todo_ part of the name.
> +    WCHAR store_name[MAX_PATH];

MAX_PATH does not make much sense. I believe that hardcoding the size to 
a small number would be better here. If there is an overflow, compiler 
will warn about that anyway.
> +    LPCWSTR base_reg_path;

const WCHAR * instead of LPCWSTR in new code would be slightly preferable.
> +} reg_store_saved_certs[] = {
> +    { HKEY_LOCAL_MACHINE, CERT_SYSTEM_STORE_LOCAL_MACHINE, FALSE,
> +        {'R','O','O','T',0}, CERT_LOCAL_MACHINE_SYSTEM_STORE_REGPATH },
> +    { HKEY_LOCAL_MACHINE, CERT_SYSTEM_STORE_LOCAL_MACHINE, FALSE,
> +        {'M','Y',0}, CERT_LOCAL_MACHINE_SYSTEM_STORE_REGPATH },
> +    { HKEY_LOCAL_MACHINE, CERT_SYSTEM_STORE_LOCAL_MACHINE, FALSE,
> +        {'C','A',0}, CERT_LOCAL_MACHINE_SYSTEM_STORE_REGPATH },
> +    /* Adding to HKCU\Root triggers safety warning dialog at least on Win764 */
> +    /* { HKEY_CURRENT_USER, CERT_SYSTEM_STORE_CURRENT_USER,
> +        {'R','O','O','T',0}, CERT_LOCAL_MACHINE_SYSTEM_STORE_REGPATH }, */
> +    { HKEY_CURRENT_USER, CERT_SYSTEM_STORE_CURRENT_USER, TRUE,
> +        {'M','Y',0}, CERT_LOCAL_MACHINE_SYSTEM_STORE_REGPATH },
> +    { HKEY_CURRENT_USER, CERT_SYSTEM_STORE_CURRENT_USER, FALSE,
> +        {'C','A',0}, CERT_LOCAL_MACHINE_SYSTEM_STORE_REGPATH }
> +};
> +
> +/* Testing whether system stores are available for adding new certs
> + * and checking directly in the registry whether they are actually saved or deleted.
> + * However, by the moment Windows treats HKCU\My (at least) as a special case,
> + * and uses AppData directory for storing certs, not registry
> + * @see additionally testRegStore() verifying a content of read/saved certs
> + */
> +static void testRegStoreSavedCerts(void)
> +{
> +    static const WCHAR fmt[] =
> +        { '%','s','\\','%','s','\\','%','s','\\','%','s',0},
> +    ms_certs[] =
> +        { 'M','i','c','r','o','s','o','f','t','\\','S','y','s','t','e','m','C','e','r','t','i','f','i','c','a','t','e','s',0},
> +    certs[] =
> +        {'C','e','r','t','i','f','i','c','a','t','e','s',0},
> +    bigCert_hash[] = {
> +        '6','E','3','0','9','0','7','1','5','F','D','9','2','3',
> +        '5','6','E','B','A','E','2','5','4','0','E','6','2','2',
> +        'D','A','1','9','2','6','0','2','A','6','0','8',0};
> +    PCCERT_CONTEXT cert1, cert2;
> +    HCERTSTORE store;
> +    HANDLE cert_file;
> +    HRESULT pathres;
> +    WCHAR key_name[MAX_PATH], appdata_path[MAX_PATH];
> +    HKEY key;
> +    BOOL ret;
> +    DWORD res,i;
> +
> +    for (i = 0; i < sizeof(reg_store_saved_certs) / sizeof(reg_store_saved_certs[0]); i++)
> +    {
> +        store = CertOpenStore(CERT_STORE_PROV_SYSTEM_REGISTRY_W,0,0,
> +            reg_store_saved_certs[i].cert_store, reg_store_saved_certs[i].store_name);
> +        if (!store)
> +        {
> +            win_skip("Insufficient privileges for the test %d, skipping\n", i);
> +            continue;

This should be skip() instead of win_skip(). Once Wine will implement 
access rights, it will hit that skip as well. I think that this skip is 
valid only in local machine store, current user should always have 
sufficient rights, it should be expressed here to make tests more 
strict. Also, testing GetLastError() in this branch would be nice.

Thanks,
Jacek