-
Notifications
You must be signed in to change notification settings - Fork 119
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
feat: add support for Postgres IAM authentication in JDBC and R2DBC connectors #490
Conversation
a289613
to
d8db5d2
Compare
core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java
Outdated
Show resolved
Hide resolved
/** | ||
* Name of system property that can specify an alternative credential factory. | ||
*/ | ||
String TOKEN_SOURCE_FACTORY_PROPERTY = "cloudSql.socketFactory.TokenSourceFactory"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a credential socket factory right? Can we get a token source from the credentials?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I'm saying is we already have a property for setting the user credentials, can we reuse the user credentials instead of trying to ask for the token source from somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the Credential Factory returns credentials that are wrapped in a HttpCredentialsAdapter
and there's no way to get credentials back from that, because they're private https://github.com/googleapis/google-auth-library-java/blob/master/oauth2_http/java/com/google/auth/http/HttpCredentialsAdapter.java#L64
We could change the Credential Factory to return a GoogleCredentials
object and then convert that to the HttpCredentialsAdapter
later, but I'm hesitant to change the existing interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a DPE owned library - can we PR a getter so we can have access to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java
Outdated
Show resolved
Hide resolved
4732aa0
to
6f8f197
Compare
…java Co-authored-by: Kurtis Van Gent <31518063+kurtisvg@users.noreply.github.com>
Are there any security implications to disabling sslmode? |
@meltsufin No - there are two potential layers of encryption - at the connection level, and at the database protocol level. Disabling at the database level is fine because the connection level is always persistent. |
Change Description
Adds IAM Auth support. Some notes to keep in mind:
sslmode
needs to be disabled or the driver will hang indefinitely.Checklist
bug/issue
before writing your code! That way we can discuss the change, evaluate
designs, and agree on the general idea.
Relevant issues: