Skip to content

Add support for SCRAM-SHA-256 authentication.#608

Closed
hlinnaka wants to merge 1 commit intolib:masterfrom
hlinnaka:scram-auth
Closed

Add support for SCRAM-SHA-256 authentication.#608
hlinnaka wants to merge 1 commit intolib:masterfrom
hlinnaka:scram-auth

Conversation

@hlinnaka
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

hlinnaka commented May 6, 2017 via email

@johto
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

lafriks commented Nov 17, 2017

Any update on this?

@ghost
Copy link
Copy Markdown

ghost commented Nov 17, 2017

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

@williambanks
Copy link
Copy Markdown

Any chance of an ETA on merging this PR?

@onemanstartup
Copy link
Copy Markdown

Any progress?

@dnp1
Copy link
Copy Markdown

dnp1 commented Mar 22, 2018

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

@madelynnblue
Copy link
Copy Markdown
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
Copy Markdown

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?

@madelynnblue
Copy link
Copy Markdown
Collaborator

This also needs a rebase onto master.

@ptman
Copy link
Copy Markdown

ptman commented Apr 16, 2019

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

@knz
Copy link
Copy Markdown

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.