passwordsync/passhook/passhook.cpp | 143 ++++++++++++++++---------------------
passwordsync/passsync/syncserv.cpp | 50 +++++++++---
passwordsync/passsync/syncserv.h | 2
3 files changed, 100 insertions(+), 95 deletions(-)
New commits:
commit fb017b47317eb759da2625b3c2c798554b7de8ba
Author: Noriko Hosoi <nhosoi(a)redhat.com>
Date: Mon May 13 13:31:11 2013 -0700
Ticket #47351 - Passsync loops when updating password of locked user
Bug description: If a password of a user is updated/reset, whose
account is disabled/inactivated on both AD and DS, the password
update is endlessly repeated on AD and DS.
Fix description: A method CanBind in syncserv is used to determine
modify password is needed in the first round as well as to check
the modification was successful in the second round. The following
modification in SyncPassword invokes the server side's WinSync
plugin to send the modify back, and SyncPassword is invoked as the
second round. If the return code from CanBind is not LDAP_INVALID_
CREDENTIALS (e.g., LDAP_UNWILLING_TO_PERFORM for the inactivated
account), the second round CanBind wound not return LDAP_SUCCESS
even if the password is correctly updated. That's said, if Can-
Bind returns any error other than LDAP_INVALID_CREDENTIALS, we
should defer the password update.
Note: Changes in passhook.cpp are all indentation fix.
https://fedorahosted.org/389/ticket/47351
Reviewed by Rich (Thank you!!)
diff --git a/passwordsync/passhook/passhook.cpp b/passwordsync/passhook/passhook.cpp
index 49a0a9f..3218881 100644
--- a/passwordsync/passhook/passhook.cpp
+++ b/passwordsync/passhook/passhook.cpp
@@ -79,20 +79,20 @@ NTSTATUS NTAPI PasswordChangeNotify(PUNICODE_STRING UserName, ULONG
RelativeId,
// Fill in the password change struct
if (newPassInfo->username && newPassInfo->password) {
- _snprintf(newPassInfo->username, (UserName->Length / 2),
"%S", UserName->Buffer);
+ _snprintf(newPassInfo->username, (UserName->Length / 2), "%S",
UserName->Buffer);
newPassInfo->username[UserName->Length / 2] = '\0';
if (Password != NULL) {
- _snprintf(newPassInfo->password, (Password->Length / 2),
"%S", Password->Buffer);
+ _snprintf(newPassInfo->password, (Password->Length / 2), "%S",
Password->Buffer);
newPassInfo->password[Password->Length / 2] = '\0';
} else {
newPassInfo->password[0] = '\0';
}
// Backoff
- newPassInfo->backoffCount = 0;
+ newPassInfo->backoffCount = 0;
- // Load time
- time(&(newPassInfo->atTime));
+ // Load time
+ time(&(newPassInfo->atTime));
} else {
// Memory error. Free everything we allocated.
free(newPassInfo->username);
@@ -114,12 +114,12 @@ NTSTATUS NTAPI PasswordChangeNotify(PUNICODE_STRING UserName, ULONG
RelativeId,
// If we got the mutex, log the error, otherwise it's not safe to log
if (waitRes == WAIT_OBJECT_0) {
- outLog.open("passhook.log", ios::out | ios::app);
+ outLog.open("passhook.log", ios::out | ios::app);
- if(outLog.is_open()) {
- timeStamp(&outLog);
- outLog << "Failed to start thread. Aborting change for
" << newPassInfo->username << endl;
- }
+ if(outLog.is_open()) {
+ timeStamp(&outLog);
+ outLog << "Failed to start thread. Aborting change for " <<
newPassInfo->username << endl;
+ }
outLog.close();
@@ -147,16 +147,13 @@ BOOL NTAPI InitializeChangeNotify()
// check if logging is enabled
RegOpenKey(HKEY_LOCAL_MACHINE, "SOFTWARE\\PasswordSync", ®Key);
- buffSize = PASSHAND_BUF_SIZE;
- if(RegQueryValueEx(regKey, "Log Level", NULL, &type, (unsigned
char*)regBuff, &buffSize) == ERROR_SUCCESS)
- {
- logLevel = (unsigned long)atoi(regBuff);
- }
- else
- {
- logLevel = 0;
- }
- RegCloseKey(regKey);
+ buffSize = PASSHAND_BUF_SIZE;
+ if(RegQueryValueEx(regKey, "Log Level", NULL, &type, (unsigned
char*)regBuff, &buffSize) == ERROR_SUCCESS) {
+ logLevel = (unsigned long)atoi(regBuff);
+ } else {
+ logLevel = 0;
+ }
+ RegCloseKey(regKey);
// Create mutex for passhook data file and log file access
passhookMutexHandle = CreateMutex(NULL, FALSE, PASSHOOK_MUTEX_NAME);
@@ -179,92 +176,80 @@ BOOL NTAPI InitializeChangeNotify()
DWORD WINAPI SavePasshookChange( LPVOID passinfo )
{
PASS_INFO *newPassInfo = NULL;
- PASS_INFO_LIST passInfoList;
- HANDLE passhookEventHandle = OpenEvent(EVENT_MODIFY_STATE, FALSE,
PASSHAND_EVENT_NAME);
+ PASS_INFO_LIST passInfoList;
+ HANDLE passhookEventHandle = OpenEvent(EVENT_MODIFY_STATE, FALSE, PASSHAND_EVENT_NAME);
fstream outLog;
if ((newPassInfo = (PASS_INFO *)passinfo) == NULL) {
goto exit;
}
- // Acquire the mutex for passhook.dat. This mutex also guarantees
+ // Acquire the mutex for passhook.dat. This mutex also guarantees
// that we can write to outLog safely.
- WaitForSingleObject(passhookMutexHandle, INFINITE);
+ WaitForSingleObject(passhookMutexHandle, INFINITE);
// Open the log file if logging is enabled
- if(logLevel > 0)
- {
- outLog.open("passhook.log", ios::out | ios::app);
- }
-
- if(outLog.is_open())
- {
- timeStamp(&outLog);
- outLog << "user " << newPassInfo->username
<< " password changed" << endl;
- //outLog << "user " << newPassInfo->username
<< " password changed to " << newPassInfo->passname <<
endl;
- }
-
- // loadSet allocates memory for the usernames and password. We need to be
- // sure to free it by calling clearSet.
- if(loadSet(&passInfoList, "passhook.dat") == 0)
- {
- if(outLog.is_open())
- {
- timeStamp(&outLog);
- outLog << passInfoList.size() << " entries
loaded from file" << endl;
- }
- }
- else
- {
- if(outLog.is_open())
- {
- timeStamp(&outLog);
- outLog << "failed to load entries from file"
<< endl;
- }
- }
+ if(logLevel > 0) {
+ outLog.open("passhook.log", ios::out | ios::app);
+ }
+
+ if(outLog.is_open()) {
+ timeStamp(&outLog);
+ outLog << "user " << newPassInfo->username << "
password changed" << endl;
+ //outLog << "user " << newPassInfo->username << "
password changed to " << newPassInfo->passname << endl;
+ }
+
+ // loadSet allocates memory for the usernames and password. We need to be
+ // sure to free it by calling clearSet.
+ if(loadSet(&passInfoList, "passhook.dat") == 0) {
+ if(outLog.is_open()) {
+ timeStamp(&outLog);
+ outLog << passInfoList.size() << " entries loaded from file"
<< endl;
+ }
+ } else {
+ if(outLog.is_open()) {
+ timeStamp(&outLog);
+ outLog << "failed to load entries from file" << endl;
+ }
+ }
// Add the new change to the list
- passInfoList.push_back(*newPassInfo);
-
- // Save the list to disk
- if(saveSet(&passInfoList, "passhook.dat") == 0)
- {
- if(outLog.is_open())
- {
- timeStamp(&outLog);
- outLog << passInfoList.size() << " entries saved
to file" << endl;
- }
- }
- else
- {
+ passInfoList.push_back(*newPassInfo);
+
+ // Save the list to disk
+ if(saveSet(&passInfoList, "passhook.dat") == 0) {
+ if(outLog.is_open()) {
+ timeStamp(&outLog);
+ outLog << passInfoList.size() << " entries saved to file"
<< endl;
+ }
+ } else {
// We always want to log this error condition
- if(!outLog.is_open())
- {
+ if(!outLog.is_open()) {
// We need to open the log since debug logging is turned off
outLog.open("passhook.log", ios::out | ios::app);
}
- timeStamp(&outLog);
- outLog << "failed to save entries to file" <<
endl;
- }
+ timeStamp(&outLog);
+ outLog << "failed to save entries to file" << endl;
+ }
// Close the log file before we release the mutex.
outLog.close();
- // Release the mutex for passhook.dat
- ReleaseMutex(passhookMutexHandle);
+ // Release the mutex for passhook.dat
+ ReleaseMutex(passhookMutexHandle);
- // We need to call clearSet so memory gets free'd
- clearSet(&passInfoList);
+ // We need to call clearSet so memory gets free'd
+ clearSet(&passInfoList);
exit:
// Free the passed in struct from the heap
free(newPassInfo);
- if (passhookEventHandle != NULL) {
- SetEvent(passhookEventHandle);
+ if (passhookEventHandle != NULL) {
+ SetEvent(passhookEventHandle);
CloseHandle(passhookEventHandle);
- }
+ }
return 0;
}
diff --git a/passwordsync/passsync/syncserv.cpp b/passwordsync/passsync/syncserv.cpp
index 86081ba..17a47a2 100644
--- a/passwordsync/passsync/syncserv.cpp
+++ b/passwordsync/passsync/syncserv.cpp
@@ -414,6 +414,7 @@ int PassSyncService::SyncPasswords()
{
while((dn = GetDN()) != NULL)
{
+ int bindRC = LDAP_SUCCESS;
if(FutureOccurrence(currentPassInfo))
{
if(logLevel > 0)
@@ -427,14 +428,40 @@ int PassSyncService::SyncPasswords()
timeStamp(&outLog);
outLog << "Multiple results not allowed: " <<
currentPassInfo->username << endl;
}
- else if(CanBind(dn, currentPassInfo->password))
+ else if(LDAP_SUCCESS == (bindRC = CanBind(dn, currentPassInfo->password)))
{
- if(logLevel > 0)
- {
+ if(logLevel > 0) {
timeStamp(&outLog);
outLog << "Password match, no modify performed: " <<
currentPassInfo->username << endl;
}
}
+ else if(LDAP_INVALID_CREDENTIALS != bindRC)
+ {
+ // password check failure.
+ /*
+ * CanBind is used to determine modify password is needed in the
+ * first round as well as to check the modification was successful
+ * in the second round. The following modification invokes the
+ * server side's WinSync plugin to send the modify back, and this
+ * code is invoked as the second round. If the return code from
+ * CanBind is not LDAP_INVALID_CREDENTIALS (e.g., LDAP_UNWILLING_
+ * TO_PERFORM for the inactivated account), the second round Can-
+ * Bind wound not return LDAP_SUCCESS even if the password is
+ * correctly updated. That's said, if CanBind returns any error
+ * other than LDAP_INVALID_CREDENTIALS, we should defer the pass-
+ * word update.
+ */
+ timeStamp(&outLog);
+ outLog << "Checking password failed for remote entry: " << dn
<< endl;
+ // defer this change for later
+ timeStamp(&outLog);
+ outLog << "Deferring password change for " <<
currentPassInfo->username << endl;
+ currentPassInfo++;
+ // free dn
+ ldap_memfree(dn);
+ dn = NULL;
+ continue;
+ }
else if(ModifyPassword(dn, currentPassInfo->password) != 0)
{
// log modify failure.
@@ -505,7 +532,7 @@ exit:
// ****************************************************************
int PassSyncService::Connect(LDAP** connection, char* dn, char* auth)
{
- int result = 0;
+ int result = LDAP_SUCCESS;
*connection = ldapssl_init(ldapHostName, atoi(ldapHostPort), 1);
@@ -530,7 +557,7 @@ int PassSyncService::Connect(LDAP** connection, char* dn, char* auth)
outLog << "\t" << lastLdapError << ": " <<
ldap_err2string(lastLdapError) << endl;
}
- result = -1;
+ result = lastLdapError;
goto exit;
}
@@ -696,19 +723,12 @@ bool PassSyncService::MultipleResults()
// ****************************************************************
// PassSyncService::CanBind
// ****************************************************************
-bool PassSyncService::CanBind(char* dn, char* password)
+int PassSyncService::CanBind(char* dn, char* password)
{
- bool result;
+ int result = LDAP_SUCCESS;
LDAP* tempConnection = NULL;
- if(Connect(&tempConnection, dn, password) == 0)
- {
- result = true;
- }
- else
- {
- result = false;
- }
+ result = Connect(&tempConnection, dn, password);
if(tempConnection != NULL)
{
diff --git a/passwordsync/passsync/syncserv.h b/passwordsync/passsync/syncserv.h
index 60b1bcf..1dcbb53 100644
--- a/passwordsync/passsync/syncserv.h
+++ b/passwordsync/passsync/syncserv.h
@@ -73,7 +73,7 @@ private:
bool FutureOccurrence(PASS_INFO_LIST_ITERATOR &startingPassInfo);
bool MultipleResults();
- bool CanBind(char* dn, char* password);
+ int CanBind(char* dn, char* password);
unsigned long BackoffTime(int backoff);
void UpdateBackoff();