Exception setting shared_secret on PostgreSQL

[itmattersnottome]

Laraguard version: 2.0.2 Laravel version: 6.18.31 PostgreSQL version: 12.3 PHP version: 7.4.7

Having set up laraguard as described in the README, an exception is thrown when calling the route associated with prepareTwoFactor().

A stacktrace is attached: 2020-07-31 11-22-00 stacktrace.txt

The relevant part is where Illuminate will throw an exception with the following message:

SQLSTATE[22021]: Character not in repertoire: 7 ERROR:  invalid byte sequence for encoding "UTF8": 0x9f (SQL: update "two_factor_authentications" set "shared_secret" = X[S�
��� ���"|>�}��, "updated_at" = 2020-07-31 09:03:05 where "id" = 1)

The characters following "0x" and the actual byte sequence will vary from request to request because the shared secret will be generated anew.

On PostgreSQL, the column type for "shared_secret" is "bytea". It seems that laraguard does not handle encoding the value for the database correctly.

The PostgreSQL server's encoding as indicated by "show server_encoding;" is "UTF8". The charset for the DB driver in config/database.php is "utf8".

[DarkGhostHunter]

Welp, I can't test for PostgreSQL as of right now, so I don't know how to fix it considering it defines the migration as binary.

Everytime a new Shared Secret is created, it's done by this method, and later saved by this other one in the model.

Can you check if there is any PostgreSQL options for alternative handling of binary data in the docs?

[itmattersnottome]

Sorry, there isn't, to my knowledge.

I mean, it comes down to Laravel's asinine decision to treat MySQL's BLOB and PostgreSQL's BYTEA as equivalent when they just aren't. It's dumb and you shouldn't have to deal with the fallout, but here we are. Not the first ones, either.

So I got two ideas.

You could include extra code to test for whether the connection is a PostgreSQL one and react to that:

if (DB::connection() instanceof \Illuminate\Database\PostgresConnection) {
    $this->attributes["shared_secret"] = "\\x" . bin2hex(Base32::decodeUpper($value));
} else {
    $this->attributes["shared_secret"] = Base32::decodeUpper($value);
}

I guess that's kinda horrible in its own right because now you have essentially stored different formats across different RDBMS.

The second idea would be to just store the Base32 representation on all DB drivers, i. e. without decoding. Then you can change the migration from binary() to string().

[DarkGhostHunter]

I think going for BASE32 is the best approach, but it will be BC. If there is a fix, it will be on the next version.

Damn binary strings.

[nathan-io]

We had the same issue, this fix worked in our case.

I looked into this and thought that the underlying issue was solved in Laravel 7.6.0 (see this commit) but apparently not, because we experienced the issue in a more recent version.

[Zer0xFF]

I have the same issue except with MS SQL, my fix was to just use Base32 as is.

and convert it to binary when needed, all required binary access is done through getBinarySecret() which makes the changes needed are minimal. if you're happy with this solution, use can use migration to avoid breaking changes, aka introduce a new shared_secret_base32 decode existing shared_secret to it.

[DarkGhostHunter]

Next version will use text as shared secret, as I don't have any guarantee it won't be under 255 characters. No more binary columns, so no more errors as these.

[DarkGhostHunter]

I ended using VARCHAR.

https://github.com/DarkGhostHunter/Laraguard/releases/tag/v3.0.0