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 config setting auth_methods #220

Merged
merged 2 commits into from
Dec 24, 2020

Conversation

straight-shoota
Copy link
Contributor

This adds a auth_methods config setting which allows to configure which auth methods the client should accept.

The settings value is a list of comma separated values. The default is scram-sha-256,md5. cleartext needs to be explicitly enabled.

Is there any place where documentation can be added?

I didn't do anything about OK authentication frame. A safe default should also fail if the server just passes you through without a credential check.
But this is more complicated because it is fine with SSL client certificate for example.
So it needs more work. For now I'm just posting this patch.

Superseeds #218
cf crystal-lang/crystal-db#141

Copy link
Owner

@will will left a comment

Choose a reason for hiding this comment

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

Overall this is good, and meets what I was looking for in having cleartext be opt-in. Thank you

Is there any place where documentation can be added?

The only place right now is the ever growing and not well organized readme. I suppose we should eventually build some better doc thing sooner or later.

anything about OK authentication frame.

Yeah this is a little more complex. For the reason you say and also, if I remember correctly, if you're connecting over like a local unix socket, and pg is doing ident/peer auth, you just get OK and that is legitimately ok. I think we can probably leave this one be for now, but we can wait a bit to see if there are any counter examples.

But again, overall looks good :) Let me know when you think it's ready ready.

src/pq/conninfo.cr Show resolved Hide resolved
@straight-shoota
Copy link
Contributor Author

I think this is ready to merge. As you said, further improvements can follow later.

@will
Copy link
Owner

will commented Dec 23, 2020

I think this is ready to merge. As you said, further improvements can follow later.

Sounds good, I'll do this tomorrow.

@will will merged commit f8058a5 into will:master Dec 24, 2020
@will
Copy link
Owner

will commented Dec 24, 2020

Thank you! Going to wrap up a few other things then do a release soon.

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