[xDS] A65 mTLS credentials in bootstrap (part 2)

[Zgoda91]

implementing gRFC A65 grpc/proposal/pull/372

This change contains:

1. Updated TlsChannelCredentials instantiation process, which involves reading trust certificates from paths provided in the bootstrap configuration file.
2. Trust certificates polling mechanism is going to be closed once given TlsChannelCredentials are no longer used (as per [xDS] A65 mTLS credentials in bootstrap (part1) #12350)

[Zgoda91]

@ejona86 Could you please review this PR when you get a chance? Thanks!

[ejona86]

One quick/important comment. But I'll need to look over this a bit more before merging.

Based on Eric's comment.

[Zgoda91]

@ejona86 - PR ready for the third round. Mind that there is a part 1 implemented separately here for visiblity

[kannanjgithub]

The grfc says implementations should be able to reuse the FileWatcherCertificateProvider. It is a bit more work though - we could do similar to CertProvidersslContextProvider's usage here. (You can see it getting used during XdsClientServerSecurityTest)

1. passing the plugin name as file_watcher and
2. Make this class (TlsXdsCredentialsProvider) implement Watcher interface for the Watcher parameter that receives certificate updates
3. Add the returned Handle to the list of Closeables in ResourceAllocationChannelProvider.
4. This will require making CertificateProviderStore.Handle public.

@ejona86 WDYT?

[ejona86]

You're reading C++-isms, which ignore everything that isn't C++. Java has two sets of classes: FileWatcherCertificateProvider and AdvancedTlsX509TrustManager/KeyManager. C++ only has one. I have no desire to use FileWatcherCertificateProvider and the abomination of plumbing to make that work. If anything, we should delete parts of FileWatcherCertificateProvider to use more of AdvancedTlsX509TrustManager/KeyManager (which were created later). The closest code in Java to FileWatcherCertificateProvider for this purpose is AdvancedTlsX509TrustManager/KeyManager.

[ejona86]

Please avoid the rebases until the reviewers agree it is a good time to rebase. I'd much rather have conflicts and be able to know what the new changes are than have to re-review the entire PR.

[ejona86]

Put this in XDS. We don't want a new public type to workaround our internals.

[ejona86]

All of this plumbing change should go back to the old way. We need to parse early so that it is actually validated. Instead, we communicate through the ChannelCredentials. (I'll make a comment elsewhere.)

[ejona86]

Instead of creating the ChannelCredentials now, which is necessary because your ResourceAllocatingChannelCredentials API can only release the resources once, we want to share the credentials and keep track of how many usages there are.

public final class ResourceAllocatingChannelCredentials extends ChannelCredentials {
  public static ChannelCredentials create(
      ChannelCredentials channelCreds, Supplier<Closeable> resource) {
    return new ResourceAllocatingChannelCredentials(channelCreds, resource);
  }

  private final ChannelCredentials channelCreds;
  private final Closeable resourceReleaser;
  private final int refCount;

  // constructor...

  public synchronized ChannelCredentials acquireChannelCredentials() {
    if (refCount++ == 0) {
      this.resourceReleaser = resource.get();
    }
    return channelCreds;
  }

  public synchronized releaseChannelCredentials() {
    if (--refCount == 0) {
      resourceReleaser.close();
      resourceReleaser = null;
    }
    assert refCount >= 0;
  }

  @Override
  public ChannelCredentials withoutBearerTokens() {
    // Could be implemented, but unnecessary. Would need to return a ResourceAllocatingChannelCredentials or similar class
    throw new UnsupportedOperationException();
  }
}

Unfortunately, the error handling in resource.get() gets ugly; AdvancedTlsx509TrustManager.updateTrustCredentials() throws. To limp along, we should probably let that throw, and if it throws we return a FailingXdsTransport here.

That's not really what we want, as we don't want a transient failure to become permanent, but it seems really hard to "correctly," so I think we should accept something lesser for the while.