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 sasl SCRAM-SHA-256 authentication mechanism #412

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

benbenw
Copy link
Contributor

@benbenw benbenw commented Sep 10, 2019

support SASL SCRAM-SHA-256 as an authentication mechanism.

SCRAM-SHA-256-PLUS (added in Postgresql 11) is not supported.

The scram part is provided by om.ongres.scram:client (BSD 2-Clause "Simplified" License), this is a new dependency.
This library also provides the scram support in pgjdbc.

Please review carefully, I'm not intimate with the vertx-pg-client source code.

@vietj
Copy link
Member

vietj commented Sep 14, 2019

can you sign the ECA @benbenw ?

@benbenw
Copy link
Contributor Author

benbenw commented Sep 14, 2019

@vietj done

@benbenw
Copy link
Contributor Author

benbenw commented Sep 15, 2019

@vietj just added a test based on org.testcontainers (already used in the mysql driver)

the sals scram connection is now tested with the test suite

@benbenw
Copy link
Contributor Author

benbenw commented Sep 15, 2019

travis test failed on postgresql 9

sasl scram is available starting with postgresql 10

will ignore the test if postgresql < v10

@benbenw
Copy link
Contributor Author

benbenw commented Sep 26, 2019

@vietj do you anything more for this PR ?

pom.xml Outdated Show resolved Hide resolved
vertx-mysql-client/pom.xml Outdated Show resolved Hide resolved
@BillyYccc
Copy link
Member

@vietj I think we need to file a CQ for the new dependency first.

@vietj
Copy link
Member

vietj commented Sep 30, 2019

can we test scram without testcontainer for now ?

I will make the CQ for both though

if we are going to use testcontainer for PG I would like rather a separate PR that migrates the project to use testcontainer for PG and a different one for scram.

@vietj
Copy link
Member

vietj commented Sep 30, 2019

actually we already have testcontainer CQ for postgresql

@benbenw
Copy link
Contributor Author

benbenw commented Sep 30, 2019

@vietj

if we are going to use testcontainer for PG I would like rather a separate PR that migrates the project to use testcontainer for PG and a different one for scram.

I have a branch that migrates most of the pg tests to test containers. I was waiting for this PR (scran) to be merged before pushing that branch.
If it's easier for you I can re-order my PRs to push the test migration first and then rebase this PR on top.

@vietj
Copy link
Member

vietj commented Sep 30, 2019

@benbenw can you make a separate PR for testcontainer that we merge first and then we finish review and merge this PR for scram ?

@benbenw
Copy link
Contributor Author

benbenw commented Sep 30, 2019

@benbenw can you make a separate PR for testcontainer that we merge first and then we finish review and merge this PR for scram ?

Yep ! Will do.

@benbenw
Copy link
Contributor Author

benbenw commented Sep 30, 2019

I've created PR #431
This one will require a rebase as soon as the #431 is merged

@benbenw
Copy link
Contributor Author

benbenw commented Oct 1, 2019

@vietj
this PR was rebased

@vietj
Copy link
Member

vietj commented Oct 1, 2019

allright, we need to wait for the library CQ to be resolved.

@BillyYccc
Copy link
Member

Indeed it would be good if you add something about the newly supported authentication method and the dependency used in the documentation.

@benbenw
Copy link
Contributor Author

benbenw commented Oct 2, 2019

minimalist doc added

@vietj
Copy link
Member

vietj commented Nov 17, 2019

sorry this has been pending for long time but we cannot yet use the scram library...

@benbenw
Copy link
Contributor Author

benbenw commented Nov 17, 2019

@vietj no pb
the CQ for the scram lib is still pending or it has been refused ?

@vietj
Copy link
Member

vietj commented Nov 17, 2019

it is still pending

@vietj
Copy link
Member

vietj commented Nov 17, 2019

it seems actually resolved but it is not clear to me, you can see the CQ here https://dev.eclipse.org/ipzilla/process_bug.cgi . I asked about it in the comments.

@vietj
Copy link
Member

vietj commented Dec 13, 2019

this PR is good to go, would you main providing it for master instead ?

@vietj
Copy link
Member

vietj commented Dec 15, 2019

ping @benbenw

@benbenw
Copy link
Contributor Author

benbenw commented Dec 15, 2019

Will rebase on master.
Give me a few days

SCRAM-SHA-256-PLUS (added in Postgresql 11) is not supported.

The scram part is provided by om.ongres.scram:client (BSD 2-Clause
"Simplified" License)

the sasl test is ignored for postgresql version < 10


Signed-off-by: zorglub <b.wiart@ubik-ingenierie.com>
@benbenw benbenw changed the base branch from 3.8 to master December 16, 2019 10:04
@benbenw
Copy link
Contributor Author

benbenw commented Dec 16, 2019

@vietj The PR was rebased.
Could you force a Travis build before merging ?

@vietj
Copy link
Member

vietj commented Jan 6, 2020

I can't find a way to do it @benbenw

@vietj
Copy link
Member

vietj commented Jan 6, 2020

I will checkout and test it myself

@vietj vietj added this to the 4.0.0 milestone Jan 6, 2020
@vietj vietj merged commit d96e53f into eclipse-vertx:master Jan 6, 2020
@vietj
Copy link
Member

vietj commented Jan 6, 2020

thanks for the contribution @benbenw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants