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

Allow empty password in presto-jdbc #8566

Closed
xerial opened this Issue Jul 20, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@xerial
Member

xerial commented Jul 20, 2017

JDBC password should be allowed to be empty:

super("password", NOT_REQUIRED, ALLOWED, STRING_CONVERTER);

But this code wrongly forces password presence check:

String password = PASSWORD.getValue(properties).orElse("");

Background:
presto-jdbc before 0.180 has ignored JDBC password property, so we are using username property for authentication (e.g., by embedding some API keys), so we have been using presto-jdbc with SSL + username (api key) + empty password.

@xerial xerial added the bug label Jul 20, 2017

@xerial xerial closed this Jul 20, 2017

@xerial

This comment has been minimized.

Show comment
Hide comment
@xerial

xerial Jul 20, 2017

Member

The code itself seems allowing empty password. I'll check the actual behavior in detail.

Member

xerial commented Jul 20, 2017

The code itself seems allowing empty password. I'll check the actual behavior in detail.

@xerial

This comment has been minimized.

Show comment
Hide comment
@xerial

xerial Jul 20, 2017

Member

I confirmed this problem is reproducible with presto-jdbc 0.181:

java.sql.SQLException: Connection property 'password' value is empty
	at com.facebook.presto.jdbc.AbstractConnectionProperty.getValue(AbstractConnectionProperty.java:104)
	at com.facebook.presto.jdbc.AbstractConnectionProperty.validate(AbstractConnectionProperty.java:123)
	at com.facebook.presto.jdbc.PrestoDriverUri.validateConnectionProperties(PrestoDriverUri.java:297)
	at com.facebook.presto.jdbc.PrestoDriverUri.<init>(PrestoDriverUri.java:89)
	at com.facebook.presto.jdbc.PrestoDriverUri.<init>(PrestoDriverUri.java:79)
	at com.facebook.presto.jdbc.PrestoDriver.connect(PrestoDriver.java:86)
	at java.sql.DriverManager.getConnection(DriverManager.java:664)
	at java.sql.DriverManager.getConnection(DriverManager.java:247)
	at Sample.main(Sample.java:12)

The source code is here: https://gist.github.com/xerial/de55b08607df73fcec36867c2408ae09#file-sample-java

Even if I use ?SSL=false (or true) URL parameter, the same error is shown.

Member

xerial commented Jul 20, 2017

I confirmed this problem is reproducible with presto-jdbc 0.181:

java.sql.SQLException: Connection property 'password' value is empty
	at com.facebook.presto.jdbc.AbstractConnectionProperty.getValue(AbstractConnectionProperty.java:104)
	at com.facebook.presto.jdbc.AbstractConnectionProperty.validate(AbstractConnectionProperty.java:123)
	at com.facebook.presto.jdbc.PrestoDriverUri.validateConnectionProperties(PrestoDriverUri.java:297)
	at com.facebook.presto.jdbc.PrestoDriverUri.<init>(PrestoDriverUri.java:89)
	at com.facebook.presto.jdbc.PrestoDriverUri.<init>(PrestoDriverUri.java:79)
	at com.facebook.presto.jdbc.PrestoDriver.connect(PrestoDriver.java:86)
	at java.sql.DriverManager.getConnection(DriverManager.java:664)
	at java.sql.DriverManager.getConnection(DriverManager.java:247)
	at Sample.main(Sample.java:12)

The source code is here: https://gist.github.com/xerial/de55b08607df73fcec36867c2408ae09#file-sample-java

Even if I use ?SSL=false (or true) URL parameter, the same error is shown.

@xerial xerial reopened this Jul 20, 2017

@electrum

This comment has been minimized.

Show comment
Hide comment
@electrum

electrum Jul 21, 2017

Member

Thanks, I see the problem here. We don't allow you to specify a property as empty, even if it's optional. For example, you can't say ?SSL=&user=test, even though SSL is optional. But, password is special because DriverManager sets it automatically. We can probably just skip the special check for empty and allow the normal validation to handle it.

Member

electrum commented Jul 21, 2017

Thanks, I see the problem here. We don't allow you to specify a property as empty, even if it's optional. For example, you can't say ?SSL=&user=test, even though SSL is optional. But, password is special because DriverManager sets it automatically. We can probably just skip the special check for empty and allow the normal validation to handle it.

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