Skip to content

Comments

Fixed some PostgreSQL issues#770

Merged
jrossi merged 1 commit intoossec:masterfrom
danthedaniel:master
Mar 12, 2016
Merged

Fixed some PostgreSQL issues#770
jrossi merged 1 commit intoossec:masterfrom
danthedaniel:master

Conversation

@danthedaniel
Copy link

As for what this fixes:

  • The user column needs to be escaped else Postgres will think it's the USER keyword. This was acknowledged in the schema file but not in the alert insertion statement in the C code.
  • The schema file has some conflicting index names so I prefixed each with the name of the table it's for
  • Many alerts do not match the schema defined, as they don't provide a user or src_ip/dst_ip, so I removed the NOT NULL constraints from those columns on the alerts table.

There should also probably be a prompt in the install.sh script to ask users if they want Postgres or MySQL support in the application.

I didn't want to mess around with that behemoth of a script though.


This change is Review on Reviewable

@danthedaniel
Copy link
Author

I should also add that these changes were made with PostgreSQL 9.4 in mind.

@jrossi
Copy link
Member

jrossi commented Mar 12, 2016

:lgtm:


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

jrossi added a commit that referenced this pull request Mar 12, 2016
@jrossi jrossi merged commit 3044432 into ossec:master Mar 12, 2016
@atomicturtle
Copy link
Member

We might need to revert this one, it is breaking mysql compatibility on :
"alert(server_id,rule_id,level,timestamp,location_id,src_ip,src_port,dst_ip,dst_port,alertid,"user",full_log) "

"user" is not valid syntax

@danthedaniel
Copy link
Author

Should add a check for MySQL/PGSQL and use different query strings

@danthedaniel
Copy link
Author

Made a new PR:

#804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants