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

Introduce support for dynamic credentials #273

Open
mp911de opened this issue Aug 16, 2022 · 9 comments
Open

Introduce support for dynamic credentials #273

mp911de opened this issue Aug 16, 2022 · 9 comments
Labels
for: team-attention An issue we need to discuss as a team to make progress type: enhancement A general enhancement

Comments

@mp911de
Copy link
Member

mp911de commented Aug 16, 2022

Right now, we support static credentials provided through ConnectionFactoryOptions. It would be neat to support credential rotation (or dynamic credentials) through a Publisher<? extends Credential>. Credential could be a marker interface with a concrete UserPasswordCredential implementation as a general case. Driver implementations could ship with their own extensions for other authentication types.

Dynamic credentials require programmatic configuration and cannot be parsed from a connection URL.

@mp911de mp911de added the for: team-attention An issue we need to discuss as a team to make progress label Aug 16, 2022
@calebcodesgud
Copy link

This would greatly help us, we're unable to extend the underlying oracle connection factory we're using to allow for new credential retrieval. Our current work around is setting the connection pool to be of fixed size and hope we don't lose connections...

@Michael-A-McMahon
Copy link
Contributor

One idea is to specify that a Credential is mutable, as this would allow security sensitive values can be cleared from memory. I've explored with this idea in the code that follows.

  /**
   * Credentials used to authenticate with a database. Credential objects are
   * mutable. Mutability allows any security sensitive values retained by a
   * {@code Credential} to be cleared from memory. Drivers MUST NOT retain a
   * reference to a {@code Credential} object after consuming it for database
   * authentication.
   */
  interface Credential {
  }

Consistent with the existing PASSWORD option, CharSequence might work well for storing a password as a UserPasswordCredential:

  interface UserPasswordCredential extends Credential {
    String user();
    CharSequence password();
  }

Would we want to have a factory for creating Credential objects?

  /** Factory for creating {@link Credential} objects */
  public static class Credentials {

    /**
     * Returns a new {@link AsyncLock2.UserPasswordCredential}
     * @param user Username. Not null.
     * @param password Password. Not null.
     */
    public static UserPasswordCredential createUserPasswordCredential(
      String user, CharSequence password) {

      record UserPasswordCredentialImpl(String user, CharSequence password)
        implements UserPasswordCredential { }

      return new UserPasswordCredentialImpl(user, password);
    }

  }

With an Option constant declared as ...

  public static final Option<Publisher<? extends Credential>> CREDENTIAL_PUBLISHER =
    Option.valueOf("credentialPublisher");

... we can have user code like this:

  static char[] getPassword() {
    // Example code below. Real code could obtain a password from the file
    // system, user input, etc
    return "the password".toCharArray();
  }

 /**
   * Publishes the password and clears it from memory after
   * consumption or cancellation by a downstream subscriber.
   */
  static Publisher<CharSequence> createPasswordPublisher() {
    return Mono.using(
      // On each subscribe, return a password char array
      () -> getPassword(),
      // Map the char[] to Publisher<CharSequence>
      password -> Mono.just(CharBuffer.wrap(password)),
      // Clear the char[] after emitting onComplete/onError
      password -> Arrays.fill(password, (char)0));
  }

  /**
   * Publishes a database connection authenticated by a
   * {@link UserPasswordCredential}
   */
  public static Publisher<? extends Connection> connect() {
    return ConnectionFactories.get(ConnectionFactoryOptions.parse(
      "r2dbc:driver:@host:port/database")
      .mutate()
      .option(
        CREDENTIAL_PUBLISHER,
        Mono.from(createPasswordPublisher())
          .map(password ->
            Credentials.createUserPasswordCredential("TheUser", password)))
        .build())
      .create();
  }

@mp911de mp911de added the type: enhancement A general enhancement label Aug 17, 2022
calebcodesgud added a commit to calebcodesgud/r2dbc-spi that referenced this issue Sep 7, 2022
calebcodesgud added a commit to calebcodesgud/r2dbc-spi that referenced this issue Sep 7, 2022
Signed-off-by: Caleb Woy [email protected]

[resolves r2dbc#273]

Signed-off-by: calebcodesgud <[email protected]>
calebcodesgud added a commit to calebcodesgud/r2dbc-spi that referenced this issue Sep 8, 2022
calebcodesgud added a commit to calebcodesgud/r2dbc-spi that referenced this issue Sep 8, 2022
…alImpl package-private.

[resolves r2dbc#273]

Signed-off-by: calebcodesgud <[email protected]>
@barathnagarajan
Copy link

@calebcodesgud , could you please help me with user code for this? Should I have a class extending Subscriber ? What would be my logic on overriding subscribe method?

@calebcodesgud
Copy link

@barathnagarajan It's not implemented yet, watch the status of this PR
#274

@bfkelsey
Copy link

Any chance to support this faster than the stated 2-3 years (from the above MR). This feature would allow using IAM auth with an AWS RDS database.

https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.Connecting.Java.html

@calebcodesgud
Copy link

@bfkelsey I too would love if this was merged faster... In absence of consensus on that, one work around that seems to work for me (with an RDS database) is to just set your connection pool to a fixed size where min = max. Obviously, this wastes connections, but if you can scale your application layer horizontally it isn't terrible...?

@mp911de
Copy link
Member Author

mp911de commented Oct 18, 2023

The problem here is that there's quite some tension. Username/password authentication seems a general theme for a certain group of databases. But there's more! Some authentication schemes just want a username, some use TLS authentication, some use challenge-response mechanism.

As we observe the space, instead of defining a hard-to-change API with the specification covering a subset of databases and authentication mechanisms, it would be much nicer to be able to specify a Supplier<String> username or the like for the already existing config properties.

@Michael-A-McMahon let me know what you think about it. While we initially started with a different mindset, it seems to be the least invasive change and so far, we don't have much what would justify another spec release.

@Michael-A-McMahon
Copy link
Contributor

I like the idea of allowing Supplier<String> for the existing USER Option.

If the process of supplying a username requires network I/O, then it could be useful to support also Publisher<String> for USER. For PASSWORD we might support Supplier<CharSequence> and Publisher<CharSequence>.

For other credential types, drivers can define their own extended options. For example, Oracle now supports OAUTH access tokens, so we might add an option named ACCESS_TOKEN which can be set to a Supplier<AccessToken> or Publisher<AccessToken>.

@calebcodesgud
Copy link

This would satisfy my requirements too. I appreciate the continued interest and support in getting this solved properly. I can close my previous PR or one of you can reject it, either is fine 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-attention An issue we need to discuss as a team to make progress type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants