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 support for SCRAM-SHA-256 authentication. #608

Closed
wants to merge 1 commit into from

Conversation

hlinnaka
Copy link

@hlinnaka hlinnaka commented May 3, 2017

Upcoming PostgreSQL version 10 adds SCRAM-SHA-256 authentication. Implement
it in the driver.

This includes a built-in implementation of SASLPrep.

Upcoming PostgreSQL version 10 adds SCRAM-SHA-256 authentication. Implement
it in the driver.

This includes a built-in implementation of SASLPrep.
@johto
Copy link
Contributor

johto commented May 5, 2017

1 commit with 1,337 additions and 0 deletions 🤔

Slightly surprised how much code went into this (considering how much cryptography Go has in its standard library), but it looks like a lot of it is just magic tables.

I can review this work this weekend, but I'd appreciate if someone else did that as well.

@cbandy
Copy link
Contributor

cbandy commented May 5, 2017

I'll devote some time this weekend as well, though probably not enough for a complete review.

I'm not sure if/how we should consider external dependencies. A quick search pointed me to the following:

So I have an idea of how to approach this, @hlinnaka, is this a port of some other implementation?

@hlinnaka
Copy link
Author

hlinnaka commented May 6, 2017 via email

@johto
Copy link
Contributor

johto commented May 6, 2017

My health unfortunately won't let me spend any time on this this weekend. I'll try and organize some time some other weekend, but I can't promise anything.

@SamWhited
Copy link

SamWhited commented Jul 9, 2017

Ah, I saw issue #16257 earlier, when I googled around, but I didn't notice there was a SCRAM-SHA-256 implementation included in that.

Fair warning: my library doesn't support the server side of SASL yet and the API is probably going to have to undergo at least one breaking change to add that. (EDIT: Added the server side) API feedback and thoughts would be welcomed.

Looking at that implementation, it doesn't do SASLprep (yet).

This is trivial to add, I just didn't have an RFC 7613 implementation yet when I started on the SASL implementation. The credentials API (where this change would be introduced) probably also will be part of the change when adding server support though. Having it as an option doesn't make a lot of sense since credentials may be just about anything, not just usernames and passwords.

PRECIS isn't identical to SASLprep, although it's close. I'm not sure what exactly the differences are.

PRECIS attempted to be as backwards compatible as possible and, in the particular case of SASLprep, most likely won't cause any problems as long as you re-normalize your data. See: RFC 7613 §6

@lafriks
Copy link

lafriks commented Nov 17, 2017

Any update on this?

@ghost
Copy link

ghost commented Nov 17, 2017

Also curious on updates as they are needed for continued usage of Gitea.

@williambanks
Copy link

Any chance of an ETA on merging this PR?

@onemanstartup
Copy link

Any progress?

@dnp1
Copy link

dnp1 commented Mar 22, 2018

Also interested on this. How can I help this to be merged?

@maddyblue
Copy link
Collaborator

I haven't done a review of the PR, but at a minimum it needs a test that connects to a real postgres server using this method. My read of the tests suggests this is missing.

@niski84
Copy link

niski84 commented Sep 13, 2018

@johto Hi, we very much need this feature for our Sept release. Is there any possible way to get this test case working so we can get this feature from last year merged?

@maddyblue
Copy link
Collaborator

This also needs a rebase onto master.

@ptman
Copy link

ptman commented Apr 16, 2019

maybe the tests could be used for the merged scram auth?

@knz
Copy link

knz commented Apr 16, 2020

This PR has been superseded by #833 and can be closed.

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.