jackc / pgconn

MIT License
182 stars 87 forks source link

add GSSAPI authentication #107

Closed otan closed 2 years ago

otan commented 2 years ago

This commit adds the GSSAPI authentication to pgx. This roughly follows the lib/pq implementation:

Depends on https://github.com/jackc/pgproto3/pull/27/. Refs #1166.

otan commented 2 years ago

marking as draft until initial feedback.

the bit that's annoying is adding testing - are you ok with me adding krb5 to the testing CI script @jackc ?

jackc commented 2 years ago

I know very little about GSS authentication so I don't have the expertise to comment much on the actual implementation of the protocol but here the things I did notice.

Definitely agree with the separate module to avoid dependency bloat. I don't understand multiple modules in the same repo well enough to be sure of all the implications. The one clear thing I got from https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories is that multi-module repos are tricky. How do they work when the parent is v2+? e.g. pgconn is moving back to the main pgx repo for pgx v5. Not sure if there are any issues with that. Maybe auth/krb should be a separate repo. That could also allow it to be used unmodified with pgx v4 and the future v5.

Regarding testing, is there a downside to adding krb5 to the test script? I guess it would bloat the test dependencies?

I try to keep pgx / pgconn working on the versions of Go supported by the Go team. I might make an exception with 1.18 due to its significant new features, but as far as I can tell none of the code is using them. Can we rollback the go.mod version to at least 1.17?

jackc commented 2 years ago

Oh, and thanks for working on this! It will be a great addition, but it wasn't something I could take on myself.

otan commented 2 years ago

Definitely agree with the separate module to avoid dependency bloat. Maybe auth/krb should be a separate repo. That could also allow it to be used unmodified with pgx v4 and the future v5.

gotcha, i'll do that in the next pass

Regarding testing, is there a downside to adding krb5 to the test script? I guess it would bloat the test dependencies?

I would need to add a docker suite to test it - the environment is pretty finicky to setup. is SASL tested here too?

I try to keep pgx / pgconn working on the versions of Go supported by the Go team. I might make an exception with 1.18 due to its significant new features, but as far as I can tell none of the code is using them. Can we rollback the go.mod version to at least 1.17?

my go get was very aggressive, i can bring it right back down in the next pass

Oh, and thanks for working on this! It will be a great addition, but it wasn't something I could take on myself.

:)


i think i need https://github.com/jackc/pgproto3/pull/27 merged before continuing.

jackc commented 2 years ago

I would need to add a docker suite to test it - the environment is pretty finicky to setup. is SASL tested here too?

SASL is tested here directly (see TestConnect). But it doesn't require any extra dependencies or setup since it is all builtin to PostgreSQL. Regarding adding Docker and friends to CI: I don't have anything against Docker, but I only have limited experience with it and most of that was 5+ years ago -- so it's not something I would be able to easily maintain if any future changes were required. If it is the only way to properly test it then add it anyway I guess. Though maybe this is a moot point now since the implementation is in a separate repo. Maybe the CI and tests for GSSAPI could live in that repo.

i think i need https://github.com/jackc/pgproto3/pull/27 merged before continuing.

Merged.

otan commented 2 years ago

Maybe the CI and tests for GSSAPI could live in that repo

i can do that :) but i guess there's not much here for testing then, heh.

otan commented 2 years ago

should be good to review

jackc commented 2 years ago

LGTM! Thanks!