Re: [PATCH] Some tests for lsa

[Bruno Jesus]

On Sun, Feb 5, 2017 at 5:57 PM, Julius Schwartzenberg
 wrote:
> dlls/secur32/tests/Makefile.in |   1 +
>  dlls/secur32/tests/lsa.c       | 215 +++++++++++++++++++++++++++++++++++++++++
>  include/ntsecapi.h             |  38 ++++++++
>  3 files changed, 254 insertions(+)
>  create mode 100644 dlls/secur32/tests/lsa.c

Hi, thanks for the patch. Unfortunately as is it cannot be used.
Somethings are stated below but mostly you have to check the compiler
warnings and guarantee there is none. Also, although very minor, git
apply complains about 7 lines adding trailing whitespaces.
> diff --git a/dlls/secur32/tests/Makefile.in b/dlls/secur32/tests/Makefile.in
> index 089d1c3..d9f140a 100644
> --- a/dlls/secur32/tests/Makefile.in
> +++ b/dlls/secur32/tests/Makefile.in
> @@ -2,6 +2,7 @@ TESTDLL   = secur32.dll
>  IMPORTS   = secur32 crypt32 advapi32 ws2_32
>
> C_SRCS = \
> -
>   lsa.c \
>   main.c \
>   negotiate.c \
>   ntlm.c \
>
> diff --git a/dlls/secur32/tests/lsa.c b/dlls/secur32/tests/lsa.c
> new file mode 100644
> index 0000000..6fa1564
> --- /dev/null
> +++ b/dlls/secur32/tests/lsa.c
> @@ -0,0 +1,215 @@
> +/*
> -
> - Lsa tests

The copyright line is missing, example:

Copyright 2017 Julius Schwartzenberg
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> - */
> -
>
> +#include <stdio.h>
> +#include <windows.h>
> +#include <ntsecapi.h>
> +#define SECURITY_WIN32
> +#include <ntstatus.h>
> +#include <winternl.h>

Compiling the patch generates dozens of warnings due to duplicated
definitions, please review the headers in use.
> +#include "wine/test.h"
> -
>
> +static HMODULE secdll;

This variable is basically not used anywhere, you don't load the DLL
so there is no need to free it.
> +char *package_name_kerberos = MICROSOFT_KERBEROS_NAME_A;

Is this really necessary? Can't you use the define directly?
> +static void test_LsaConnectUntrusted_null_handle(void)
> +{
> - NTSTATUS status;
> -
> - status = LsaConnectUntrusted(NULL);
> -
> - todo_wine {
> -
>    ok(status == STATUS_ACCESS_VIOLATION || status == RPC_NT_NULL_REF_POINTER,
> -
>       "LsaConnectUntrusted(NULL) returned 0x%x\n", status);
> - }
>
> +}
> -
>
> +static void test_LsaConnectUntrsuted_correct_handle(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> -
> - status = LsaConnectUntrusted(&lsa_handle);
> -
> - ok(status == STATUS_SUCCESS, "LsaConnectUntrusted(valid_handle) returned 0x%x\n", status);
>
> +}

You are leaking the handle, according to MSDN you need to call
LsaDeregisterLogonProcess to close it. This applies to every function
that calls this function.
> +static void test_LsaLookupAuthenticationPackage_null_handle(void)
> +{
> - NTSTATUS status;
> - LSA_STRING package_name;
> - ULONG auth_package;
> - package_name.Buffer = package_name_kerberos;
> - package_name.Length = (USHORT)strlen(package_name.Buffer);

If you use the #define you can use sizeof(MICROSOFT_KERBEROS_NAME_A) -
1 instead of strlen since the buffer is constant.
> - status = LsaLookupAuthenticationPackage(NULL, &package_name, &auth_package);
> -
> - todo_wine {
> -
>    ok(status == STATUS_INVALID_HANDLE || status == STATUS_INVALID_CONNECTION,
> -
>       "LsaLookupAuthenticationPackage(NULL, \"Kerberos\", ...) returned 0x%x\n", status);
> - }

No need for a block in the todo_wine since it is only one ok call.
> +}
> -
>
> +static void test_LsaLookupAuthenticationPackage_correct_handle(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> - LSA_STRING package_name;
> - ULONG auth_package;
> - package_name.Buffer = package_name_kerberos;
> - package_name.Length = (USHORT)strlen(package_name.Buffer);
> -
> - LsaConnectUntrusted(&lsa_handle);
> - status = LsaLookupAuthenticationPackage(lsa_handle, &package_name, &auth_package);
> -
> - ok(status == STATUS_SUCCESS,
> -
>  "LsaLookupAuthenticationPackage(valid_handle, \"Kerberos\", ...) returned 0x%x\n", status);

Is it possible to test that auth_package somehow?
> +}
> -
>
> +static void test_LsaLookupAuthenticationPackage_correct_handle_wrong_name(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> - LSA_STRING package_name;
> - ULONG auth_package;
> - package_name.Buffer = "this is probably not a valid authentication package name";
> - package_name.Length = (USHORT)strlen(package_name.Buffer);
> -
> - LsaConnectUntrusted(&lsa_handle);
> - status = LsaLookupAuthenticationPackage(lsa_handle, &package_name, &auth_package);
> -
> - todo_wine {
> -
>    ok(status == STATUS_NO_SUCH_PACKAGE,
> -
>       "LsaLookupAuthenticationPackage(valid_handle, invalid, ...) returned 0x%x\n", status);
> - }
>
> +}
> -
>
> +static void test_LsaCallAuthenticationPackage_invalid_handle(void)
> +{
> - NTSTATUS status;
> - ULONG submit_buffer_length = 0;
> -
> - VOID *protocol_return_buffer;
> - ULONG *return_buffer_length;
> - PNTSTATUS protocol_status;
> -
> - status = LsaCallAuthenticationPackage(
> -
>    NULL, 0xdeadbeef, NULL, submit_buffer_length,
> -
>    protocol_return_buffer, return_buffer_length, protocol_status
> -
>    );
> -
> - todo_wine {
> -
>    ok(status == STATUS_INVALID_HANDLE || status == STATUS_INVALID_CONNECTION,
> -
>       "LsaCallAuthenticationPackage(NULL, ...) returned 0x%x\n", status);
> - }
>
> +}
> -
>
> +static void test_LsaCallAuthenticationPackage_invalid_package(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> - ULONG submit_buffer_length = 0;
> -
> - VOID *protocol_return_buffer;
> - ULONG *return_buffer_length;
> - PNTSTATUS protocol_status;
> -
> - LsaConnectUntrusted(&lsa_handle);
> - status = LsaCallAuthenticationPackage(
> -
>    lsa_handle, 0xdeadbeef, NULL, submit_buffer_length,
> -
>    protocol_return_buffer, return_buffer_length, protocol_status
> -
>    );

Bad parameters passed to the function, see compiler warnings.
> -
> - todo_wine {
> -
>    ok(status == STATUS_NO_SUCH_PACKAGE,
> -
>       "LsaCallAuthenticationPackage(valid_handle, invalid, ...) returned 0x%x\n", status);
> - }
>
> +}
> -
>
> +static void test_LsaCallAuthenticationPackage_null_submit_buffer(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> - LSA_STRING package_name;
> - ULONG auth_package;
> - package_name.Buffer = package_name_kerberos;
> - package_name.Length = (USHORT)strlen(package_name.Buffer);
> - ULONG submit_buffer_length = 0;

Very specific compiler warning that you can't declare variables after code.
> -
> - VOID     *protocol_return_buffer;
> - ULONG    *return_buffer_length;
> - PNTSTATUS protocol_status;
> -
> - LsaConnectUntrusted(&lsa_handle);
> - LsaLookupAuthenticationPackage(lsa_handle, &package_name, &auth_package);
> - status = LsaCallAuthenticationPackage(
> -
>    lsa_handle, auth_package, NULL, submit_buffer_length,
> -
>    protocol_return_buffer, return_buffer_length, protocol_status
> -
>    );

Bad parameters passed to the function, see compiler warnings.
> - todo_wine {
> -
>    ok(status == STATUS_INVALID_PARAMETER,
> -
>       "LsaCallAuthenticationPackage(...) returned 0x%x\n", status);
> - }
>
> +}
> -
>
> +static void test_LsaCallAuthenticationPackage_correct_submit_buffer(void)
> +{
> - NTSTATUS status;
> - HANDLE lsa_handle;
> - LSA_STRING package_name;
> - ULONG auth_package;
> - package_name.Buffer = package_name_kerberos;
> - package_name.Length = (USHORT)strlen(package_name.Buffer);
> - PKERB_RETRIEVE_TKT_REQUEST protocol_submit_buffer;
> -
> - VOID *protocol_return_buffer;
> - ULONG *return_buffer_length;
> - PNTSTATUS protocol_status;
> -
> - LsaConnectUntrusted(&lsa_handle);
> - LsaLookupAuthenticationPackage(lsa_handle, &package_name, &auth_package);
> -
> - protocol_submit_buffer = LocalAlloc(LMEM_ZEROINIT, sizeof(KERB_RETRIEVE_TKT_REQUEST));
> - protocol_submit_buffer->MessageType = KerbRetrieveEncodedTicketMessage;
> -
> - status = LsaCallAuthenticationPackage(
> -
>    lsa_handle, auth_package, protocol_submit_buffer, sizeof(KERB_RETRIEVE_TKT_REQUEST),
> -
>    protocol_return_buffer, return_buffer_length, protocol_status
> -
>    );
> -
> - ok(status == STATUS_SUCCESS, "LsaCallAuthenticationPackage(...) returned 0x%x\n", status);
>
> +}
> -
>
> +START_TEST(lsa)
> +{
> - test_LsaConnectUntrusted_null_handle();
> - test_LsaConnectUntrsuted_correct_handle();

Untrusted misspelling.
> - test_LsaLookupAuthenticationPackage_null_handle();
> - test_LsaLookupAuthenticationPackage_correct_handle();
> - test_LsaLookupAuthenticationPackage_correct_handle_wrong_name();
> - test_LsaCallAuthenticationPackage_invalid_handle();
> - test_LsaCallAuthenticationPackage_invalid_package();
> - test_LsaCallAuthenticationPackage_null_submit_buffer();
> - test_LsaCallAuthenticationPackage_correct_submit_buffer();
> -
> - if(secdll)
> -
>    FreeLibrary(secdll);

As I said earlier this is not required.
> +}
> diff --git a/include/ntsecapi.h b/include/ntsecapi.h
> index 2bb3d31..439f67e 100644
> --- a/include/ntsecapi.h
> +++ b/include/ntsecapi.h
> @@ -387,6 +387,44 @@ NTSTATUS WINAPI LsaSetTrustedDomainInformation(LSA_HANDLE,PSID,TRUSTED_INFORMATI
>  NTSTATUS WINAPI LsaStorePrivateData(LSA_HANDLE,PLSA_UNICODE_STRING,PLSA_UNICODE_STRING);
>  NTSTATUS WINAPI LsaUnregisterPolicyChangeNotification(POLICY_NOTIFICATION_INFORMATION_CLASS,HANDLE);
>
> +#ifndef MICROSOFT_KERBEROS_NAME_A
> -
>
> +#define MICROSOFT_KERBEROS_NAME_A "Kerberos"
> +#define MICROSOFT_KERBEROS_NAME_W L"Kerberos"

You can't use L to define WCHAR data.
> +#ifdef WIN32_CHICAGO

Where is this from? Not a single Wine header has that name.
> +#define MICROSOFT_KERBEROS_NAME MICROSOFT_KERBEROS_NAME_A
> +#else
> +#define MICROSOFT_KERBEROS_NAME MICROSOFT_KERBEROS_NAME_W
> +#endif
> +#endif
> -
>
> +typedef enum _KERB_PROTOCOL_MESSAGE_TYPE {
> - KerbDebugRequestMessage = 0,KerbQueryTicketCacheMessage,KerbChangeMachinePasswordMessage,KerbVerifyPacMessage,KerbRetrieveTicketMessage,
> - KerbUpdateAddressesMessage,KerbPurgeTicketCacheMessage,KerbChangePasswordMessage,KerbRetrieveEncodedTicketMessage,KerbDecryptDataMessage,
> - KerbAddBindingCacheEntryMessage,KerbSetPasswordMessage,KerbSetPasswordExMessage,KerbVerifyCredentialsMessage,KerbQueryTicketCacheExMessage,
> - KerbPurgeTicketCacheExMessage,KerbRefreshSmartcardCredentialsMessage,KerbAddExtraCredentialsMessage,KerbQuerySupplementalCredentialsMessage,
> - KerbTransferCredentialsMessage,KerbQueryTicketCacheEx2Message

These lines are too long, try to keep around 100 columns.
> +} KERB_PROTOCOL_MESSAGE_TYPE,*PKERB_PROTOCOL_MESSAGE_TYPE;
> -
>
> +#ifndef __SECHANDLE_DEFINED__
> - typedef struct _SecHandle {
> -
>    ULONG_PTR dwLower;
> -
>    ULONG_PTR dwUpper;
> - } SecHandle,*PSecHandle;
> -
>
> +#define __SECHANDLE_DEFINED__
> +#endif
> -
>
> +typedef struct _KERB_RETRIEVE_TKT_REQUEST {
> - KERB_PROTOCOL_MESSAGE_TYPE MessageType;
> - LUID LogonId;
> - UNICODE_STRING TargetName;
> - ULONG TicketFlags;
> - ULONG CacheOptions;
> - LONG EncryptionType;
> - SecHandle CredentialsHandle;
>
> +} KERB_RETRIEVE_TKT_REQUEST,*PKERB_RETRIEVE_TKT_REQUEST;
> -
>
> #ifdef __cplusplus
>  } /* extern "C" */
> #endif /* defined(__cplusplus) */
>
> 1.9.1

[PATCH] Some tests for lsa

[Julius Schwartzenberg]

Hi Bruno,

Bruno Jesus wrote:
> On Sun, Feb 5, 2017 at 5:57 PM, Julius Schwartzenberg
>  wrote:
> > dlls/secur32/tests/Makefile.in |   1 +
> >  dlls/secur32/tests/lsa.c       | 215 +++++++++++++++++++++++++++++++++++++++++
> >  include/ntsecapi.h             |  38 ++++++++
> >  3 files changed, 254 insertions(+)
> >  create mode 100644 dlls/secur32/tests/lsa.c
>
> Hi, thanks for the patch. Unfortunately as is it cannot be used.
> Somethings are stated below but mostly you have to check the compiler
> warnings and guarantee there is none. Also, although very minor, git
> apply complains about 7 lines adding trailing whitespaces.

Thanks a lot for the review! I hope to send a better version soon. I
just wasn't sure whether wine-patches was already appropriate for
non-final patches or not. It seems nice that Marvin runs the tests already.
Let me know if I'd better send it differently before the review process
is finished.
> > -
> > - Lsa tests
>
> The copyright line is missing, example:
>
> Copyright 2017 Julius Schwartzenberg

I wasn't sure whether this was important, but I can add that.
> > +static void test_LsaLookupAuthenticationPackage_correct_handle(void)
> > +{
> > - NTSTATUS status;
> > - HANDLE lsa_handle;
> > - LSA_STRING package_name;
> > - ULONG auth_package;
> > - package_name.Buffer = package_name_kerberos;
> > - package_name.Length = (USHORT)strlen(package_name.Buffer);
> > -
> > - LsaConnectUntrusted(&lsa_handle);
> > - status = LsaLookupAuthenticationPackage(lsa_handle, &package_name, &auth_package);
> > -
> > - ok(status == STATUS_SUCCESS,
> > -
> >  "LsaLookupAuthenticationPackage(valid_handle, \"Kerberos\", ...) returned 0x%x\n", status);
>
> Is it possible to test that auth_package somehow?

Possibly yeah. I haven't figured out much yet. I just started to write a
bunch of tests some time ago to see whether I could figure things out. I
mailed it now because it seemed better to submit it after the 2.0
release. Would it make sense to add this test in a second patch?

These tests so far are really really basic and it seemed to make sense
to me to add more tests in later patches. (Or first solve some todo_wines?)
> > diff --git a/include/ntsecapi.h b/include/ntsecapi.h
> > index 2bb3d31..439f67e 100644
> > --- a/include/ntsecapi.h
> > +++ b/include/ntsecapi.h
> > @@ -387,6 +387,44 @@ NTSTATUS WINAPI LsaSetTrustedDomainInformation(LSA_HANDLE,PSID,TRUSTED_INFORMATI
> >  NTSTATUS WINAPI LsaStorePrivateData(LSA_HANDLE,PLSA_UNICODE_STRING,PLSA_UNICODE_STRING);
> >  NTSTATUS WINAPI LsaUnregisterPolicyChangeNotification(POLICY_NOTIFICATION_INFORMATION_CLASS,HANDLE);
> >
> > +#ifndef MICROSOFT_KERBEROS_NAME_A
> > -
> >
> > +#define MICROSOFT_KERBEROS_NAME_A "Kerberos"
> > +#define MICROSOFT_KERBEROS_NAME_W L"Kerberos"
>
> You can't use L to define WCHAR data.
> > +#ifdef WIN32_CHICAGO
>
> Where is this from? Not a single Wine header has that name.

I copy pasted the headers from MinGW. On IRC I was told that is ok
because the file starts with "This file has no copyright assigned and is
placed in the Public Domain.".

I didn't consider any technical issues though. How should MinGW/Wine
header differences be dealt with?

Thanks again for the feedback! I hope I can send a better patch soon.

Best regards,
Julius