Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving PostgreSQL Support #1687

Merged
merged 7 commits into from
May 16, 2024
Merged

Conversation

RafBishopFox
Copy link
Collaborator

This PR addresses #1605 and improves PostgreSQL support in Sliver.

It seems that SQLite and PostgreSQL handle the zero GUID differently, so for PostgreSQL databases, we need to store NULL instead of the zero GUID. SQLite takes care of storing the zero GUID when NULL is provided as the value for a UUID column. For PostgreSQL and the current model, we need to be explicit, so some UUIDs have been changed to *uuid.UUID to signal to GORM that when we pass in a nil UUID, we want NULL and not uuid.Nil (the zero GUID).

I tested this change with both SQLite and PostgreSQL (14.11), and they handled the following tasks just fine:

  • Generating an http implant without an associated profile as described in Postgres FK constraint errors with fresh install #1605
  • Generating a beacon without an associated profile, tasking the beacon, and killing it
  • Generating an http implant with an associated profile and deleting the profile
  • Creating and deleting a job
  • Adding and removing loot

There may be other tasks which generate errors in PostgreSQL that I am not aware of, but I wanted to hit common use cases. Are there any other common tasks that I should test?

I tested database support in MySQL (8.0) and MariaDB (10.11), and I suggest we remove support for the mysql dialect. It is not compatible with our database model. MySQL does not have a uuid datatype and instead uses BINARY(16). We could either shift our current model to use BINARY(16) for all dialects or create hooks that translate uuid.UUID to BINARY(16) when mysql is the active dialect. MariaDB 10.7 gained the UUID datatype, but lacks support for array type which breaks the crackstation model. No one has asked for MySQL or MariaDB support publicly, so I am not sure the juice is worth the squeeze to support it.

Changing database migrations to occur in sequence instead of all at once for more accurate error reporting in the log
Changing HostUUID in the models.Host struct to be unique to fix a constraint issue
Changing models.HttpC2Header to contain pointers to ServerConfigIDs and ImplantConfigIDs so that NULL values can be inserted into the database when a type of config does not apply.
Changing implant generation without an associated profile to insert NULL into the database instead of the zero GUID
@RafBishopFox RafBishopFox requested a review from a team as a code owner May 14, 2024 14:35
Copy link
Member

@rkervella rkervella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change required.

server/db/sql.go Outdated Show resolved Hide resolved
@moloch-- moloch-- requested a review from rkervella May 14, 2024 16:51
@moloch-- moloch-- merged commit 130b5f9 into master May 16, 2024
5 checks passed
@RafBishopFox RafBishopFox deleted the v1.6.0/database-implant-table-fixes branch May 16, 2024 17:27
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