search

Solutions-Nitriques / anti_brute_force

Secure your Symphony CMS login page against brute force attacks
http://symphonyextensions.com/extensions/anti_brute_force/
Other
10 stars 12 forks source link

Error with INSERT (which should be UPDATE) #8

Closed michael-e closed 11 years ago

michael-e commented 11 years ago

I am running a MySQL replicate, which (by design of the MySQL binlog, obviously) completely breaks on database errors — those errors may be less critical when Symphony deals with the main database.

I noticed a problem with the registerFailure function. My replicate breaks with the following error as soon as failures are actually registered (private data are masked by xxx):

Last_SQL_Error: Error 'Duplicate entry '93.xxx.xxx.xxx' for key 'PRIMARY'' on query. Default database: 'symphony'. Query: 'INSERT INTO sym_anti_brute_force
    (`IP`, `LastAttempt`, `Username`, `FailedCount`, `UA`, `Source`, `Hash`)
    VALUES
    ('93.xxx.xxx.xxx', NOW(),        'xxxxxxxxxx', 1,            '','Anti Brute Force', UUID())'

So it looks as if the check you implemented to decide whether to UPDATE or INSERT does not work as expected.

Wouldn't it be cooler anyway to either use ON DUPLICATE KEY UPDATE or maybe Symphony's database insert method? (Im am not sure about the latter, I haven't checked if it will it work properly with custom tables or not.)

nitriques commented 11 years ago

Hi @michael-e

I never worked with a replicated mySQL DB. So I will try to answer to your questions with my little knowledge of how replicated DB works.

The thing is, I do not understand why the code won't simply update the DB. In the registerFailure function, I first check to see if there is already a record for that particular ip (see https://github.com/Solutions-Nitriques/anti_brute_force/blob/master/lib/class.ABF.php#L198). If there is not, INSERT is used. If there is a record for that IP address, then UPDATE is called.

Using Symphony insert/update method won't help since they are only wrappers for creating the actual SQL string.

We could use ON DUPLICATE KEY UPDATE and remove the if clause in the code, but I do not think it will solve your problem, since there is already a check for preventing duplicate primary keys. But I could be neater code :)

The thing is, in replicated DB scheme, I think that primary keys MUST be unique ACROSS all servers. Am I right ? If so, that's the problem. Instead of using a 'normative' primary key (in this case, the IP), maybe we should use a 'generic' primary key. An auto-increment number might be the solution.

I really encourage you to fork and play with it. Since I don't have a replicated DB, I can't test things for you. But I will be available for helping you, answering questions you might have about this extension.

nitriques commented 11 years ago

@michael-e Please fork the dev branch :)

michael-e commented 11 years ago

Sorry, I don't know when I will find the time. :-(

The thing is, in replicated DB scheme, I think that primary keys MUST be unique ACROSS all servers.

No, it uses the same keys. The replicate is a 1:1 copy.

I am rather sure that for some reason the check doesn't work reliably. The MySQL replicate shouldn't produce errors if there are no errors in the main DB (using the same query). So I am rather sure that using ON DUPLICATE KEY UPDATE would solve my problem, and — as you said — it would mean cleaner code.

BTW: The extension is pretty useful to me, so I hope that we (resp. me myself) can fix this.

nitriques commented 11 years ago

@michael-e

Reading mySQL's doc http://dev.mysql.com/doc/refman/5.0/en/insert-on-duplicate.html it seems like the ON DUPLICATE KEY UPDATE would fail just as my if is failling.

Do you allow Symphony to talk to BOTH of the mySQL instances ? If so, there's the problem. The SELECT is done on one DB and the INSERT into the other one...

Can you try to make only one DB responding to those queries ? If not, try the ON DUPLICATE KEY UPDATE and if it works, I will do the change in the code.

Hopes this fixes your issue. And thanks for finding this extensions useful: Brute force attacks happends everyday for some on my sites.

michael-e commented 11 years ago

No, Symphony only talks to database "A". Database "B" is simply a slave, performing the same queries using the binary log of database A, so keeping in sync automatically. (You may wonder what is the purpose of having such a "mirror" — in my case it is used for backups only. It can be locked for several minutes to create dumps without affecting the live system. As soon as the lock is released, it will parse the binlog and catch up to the main DB.)

When I find the time, I will do some tests. Thank you!

nitriques commented 11 years ago

Ok thanks for the explanation ! But I fear that the 'ON DUPLICATE KEY UPDATE' will still fail then...

I would be curious to look at the fact that the primary key is a varchar... maybe we should use a char (no variable length). Maybe your error was due to the fact that there was a similar IP (let's say 10.0.0.1) in the DB and that a new record (let's say 10.0.0.10) was conflicting the select.... I think this is the only time I used a varchar primary key..

Have you ever experienced some thing like that ?

michael-e commented 11 years ago

But I fear that the 'ON DUPLICATE KEY UPDATE' will still fail then

So do I, after examining your code. It shouldn't make any difference.

(let's say 10.0.0.1) … Have you ever experienced some thing like that ?

No, I haven't.

Still I wonder why it happened several times, and only with this extension. Before hacking around in the code I should try and nail down how it is triggered. I will try and watch the database replicate status to find out when it breaks.

michael-e commented 11 years ago

That's what I found googling around: http://mark.koli.ch/2008/10/mysql-duplicate-entry-error-when-handling-varchar-primary-keys.html – but this shouldn't be a problem here, since we are only saving IP addresses (which can't be case sensitive by design).

nitriques commented 11 years ago

Hum this puzzles me a lot. Does the error happens when the replication to DB B is done or when the value is inserted into DB A ?

michael-e commented 11 years ago

The error happened when DB B — the replicate — executed the binary log af DB A. So theoretically it should have occured in DB A as well. But I haven't found it in the Symphony logs. Maybe I overlooked it?

At the moment the slave DB (B) is running fine. If it breaks again, I will search all the available logs to find the reason.

nitriques commented 11 years ago

At least people getting this error are not typical anonymous users of the website... Do not hesitate to contact me again if you have more debug info on this. I will close this issue for now as there is nothing we can do.

michael-e commented 11 years ago

Agreed, and thanks for your time!

nitriques commented 11 years ago

You're welcome !