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

add GSSAPI authentication #107

Merged
merged 1 commit into from
Apr 15, 2022
Merged

add GSSAPI authentication #107

merged 1 commit into from
Apr 15, 2022

Conversation

otan
Copy link
Contributor

@otan otan commented Apr 10, 2022

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

Depends on jackc/pgproto3#27.
Refs #1166.

@otan
Copy link
Contributor Author

otan commented Apr 10, 2022

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 ?

go.mod Outdated Show resolved Hide resolved
@otan otan force-pushed the krb5 branch 2 times, most recently from eb5dfcd to e7b83b7 Compare April 11, 2022 05:25
@jackc
Copy link
Owner

jackc commented Apr 12, 2022

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
Copy link
Owner

jackc commented Apr 12, 2022

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

@otan
Copy link
Contributor Author

otan commented Apr 12, 2022

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 jackc/pgproto3#27 merged before continuing.

@jackc
Copy link
Owner

jackc commented Apr 12, 2022

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 jackc/pgproto3#27 merged before continuing.

Merged.

This commit adds the GSSAPI authentication to pgx. This roughly follows
the lib/pq implementation:
* We require registering a provider to avoid mass dependency inclusions
  that may not be desired (lib/pq#971).
* Requires the pgproto3 package be updated. I've included my custom fork
  for now.
@otan
Copy link
Contributor Author

otan commented Apr 12, 2022

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 otan marked this pull request as ready for review April 12, 2022 21:16
@otan
Copy link
Contributor Author

otan commented Apr 12, 2022

should be good to review

@jackc jackc merged commit 90ef5bb into jackc:master Apr 15, 2022
@jackc
Copy link
Owner

jackc commented Apr 15, 2022

LGTM! Thanks!

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.

2 participants