Skip to content

Support for SCRAM-SHA-256 SASL authentication #89

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

Merged
merged 8 commits into from
Jan 11, 2021
Merged

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented Mar 11, 2020

  • PostgreSQL supports SCRAM-SHA-256 authentication since version 11. This is preliminary support for that authentication type.

gwynne added 4 commits March 11, 2020 14:03
…ypes 10 (SASL mechanisms), 11 (SASL continue), and 12 (SASL final). Add more specific errors for types 2(Kerberos), 7(GSSAPI), 8(GSSAPI), 9(SSPI), and 6(obsolete SCM).
…56 and SCRAM-SHA-256-PLUS per RFC 7677 et al. Things that are still missing: Channel binding support (Postgres DOES use this), authorization names (Postgres does not use these), proper username and password normalization, RFC-compliant validation of nonces, and determining whether the Hi() function can be replaced with PBKDF2
@gwynne gwynne added enhancement New feature or request semver-patch labels Mar 11, 2020
@gwynne gwynne requested a review from tanner0101 March 11, 2020 19:10
@gwynne gwynne self-assigned this Mar 11, 2020
@gwynne gwynne requested a review from calebkleveter March 24, 2020 22:29
Copy link
Member

@calebkleveter calebkleveter left a comment

Choose a reason for hiding this comment

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

This generally looks good to me. I think it's safe to assume you know what you're doing so I don't have to read the docs for the Postgres authentication protocol.

One question though, should there be tests for this?

@gwynne
Copy link
Member Author

gwynne commented Mar 25, 2020

One question though, should there be tests for this?

@calebkleveter The Postgres-specific side of it is implicitly tested whenever the test suite is run against a database configured for scram-sha-256 auth, a job for a Docker Compose manifest or similar. But yes, there certainly should be unit tests for the other bits. Some categories that'd make good test suites that come to mind:

  • Tests for the generic SASL authentication manager (such as by implementing a dummy mechanism to put the system through its paces)
  • Tests for the SCRAM_SHA256 mechanism in terms of it as a SASLMechanism.
  • Tests for the SCRAMMessageParser, which is currently a bit of a hacked-up mess with several fail-closed (auth fails when it shouldn't rather than vice versa) corner cases.
  • Possibly, separate tests for the GS2Header parsing code, depending how later refinements to the overall implementation go.
  • Tests for the Hi() function, which is effectively most of a PBKDF2 implementation and should at least be verified against that algorithm's test vectors.
  • Last but not least, tests to verify the behavior of the additional as-yet-unimplemented SCRAM_SHA256_PLUS mechanism, particularly to verify it performs channel bonding correctly, since that's the only reason it exists.

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

Huge +1 to this ❤️

Can we configure the DB image we use in CI to test this?

It could also be nice to add a psql-sasl image to the docker-compose file:

For easy testing locally.

@gwynne gwynne force-pushed the scram-sha-256-support branch 3 times, most recently from a832b1a to 8a784dc Compare October 12, 2020 02:24
…bos disabled to cut down on the excessive number of checks generated by the test matrices (72 instead of 234).
@gwynne gwynne force-pushed the scram-sha-256-support branch from 8a784dc to 2cf6b5a Compare October 12, 2020 07:39
@gwynne
Copy link
Member Author

gwynne commented Oct 12, 2020

@tanner0101 Test workflow has been updated to test all three auth methods (trust, MD5, SCRAM-SHA-256) for all supported PostgreSQL versions (and lots of Swift versions...), docker-compose.yml now permits specifying an auth method via env var. I'd like to get this in ASAP; more Postgres setups are starting to default to SCRAM-SHA-256, and Postgres itself has made it the default for the "next version after 13" release (see https://www.postgresql.org/docs/devel/runtime-config-connection.html#RUNTIME-CONFIG-CONNECTION-AUTHENTICATION, at the time of this writing).

@bridger
Copy link

bridger commented Jan 11, 2021

Thank you for implementing this! I was getting my dev environment set up on a new machine and sure enough, postgresql has started defaulting to scram-sha-256. That means it was rejecting any connections from postgres-nio. I'm happy to see the fix is ready to go! 🌟

For other people who might run into this, the error you'll get is protocol error: Unkonwn authentication request type: 10, which means postgres-nio is connecting with md5 but scram-sha-256 is required. You can go back to md5 by modifying pg_hba.conf and changing the lines that look something like local all all scram-sha-265 to have md5 instead.

Copy link
Member

@Joannis Joannis left a comment

Choose a reason for hiding this comment

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

A lot of commented code could be removed, but it's non-critical for sure. LGTM

@gwynne gwynne merged commit 072b685 into master Jan 11, 2021
@gwynne gwynne deleted the scram-sha-256-support branch January 11, 2021 01:41
@tanner0101
Copy link
Member

These changes are now available in 1.4.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants