Authenticator conceptually flawed

[jack-berg]

In reviewing #6952 I've identified a conceptual flaw in Authenticator.

To demonstrate, I've pushed a commit to revise our main Authenticator test code:

The workflow goes like this:

AtomicInteger authenticatorCallCount = new AtomicInteger();
Authenticator authenticator =
    () -> Collections.singletonMap("key", "value" + authenticatorCallCount.incrementAndGet());
TelemetryExporter<T> exporter =
    exporterBuilder()
        .setEndpoint(server.httpUri() + path)
        .setAuthenticator(() -> Collections.singletonMap("key", "value"))
        .setAuthenticator(authenticator)
        .build();

addHttpError(401);
assertThat(
        exporter
            .export(Collections.singletonList(generateFakeTelemetry()))
            .join(10, TimeUnit.SECONDS)
            .isSuccess())
    .isTrue();
assertThat(authenticatorCallCount.get()).isEqualTo(1);
assertThat(httpRequests)
    .element(0)
    .satisfies(req -> assertThat(req.headers().get("key")).isNull());
assertThat(httpRequests)
    .element(1)
    .satisfies(req -> assertThat(req.headers().get("key")).isEqualTo("value"));
    .satisfies(req -> assertThat(req.headers().get("key")).isEqualTo("value1"));

// If we make a followup request without a 401, authenticator is never invoked and its headers
// are not included
httpRequests.clear();
assertThat(
        exporter
            .export(Collections.singletonList(generateFakeTelemetry()))
            .join(10, TimeUnit.SECONDS)
            .isSuccess())
    .isTrue();
assertThat(authenticatorCallCount.get()).isEqualTo(1);
assertThat(httpRequests).hasSize(1);
assertThat(httpRequests)
    .element(0)
    .satisfies(req -> assertThat(req.headers().get("key")).isNull());

Notes:

• Authenticator is only called after a 401 response from the server.
• There's no internal caching of Authenticator's headers, so if a 401 response isn't received, the headers provided by Authenticator aren't included at all.
• A popular pattern for authentication is to make a network request for an access token which eventually expires. Include this access token on each request until the server indicates that the access token is invalid with 401. At this point, request a new access token and include it on subsequent requests. This is not possible because of bullet point two, and because there's no signal to the Authenticator on what prompted the call to getHeaders(). I.e. is this business as usual and we should just return the existing access token, or did we get a 401 back and we should try to refresh the token.
• As currently designed, I think users are better off calling setHeaders(Supplier<Map<String, String>>). There's no way for the supplier to be reactive to a 401 response from the server, which means it must rely on knowing when its access token expires and be able to refresh accordingly. However, at least the flow doesn't depend on every single export request getting a 401 just to add an access token in the next request and get a 200.

I'm really struggling to see the value in Authenticator at all at the moment. We could partially fix it by adjusting the contract to be Map<String, String> getHeaders(@Nullable HttpResponse), which on its face gives the implementation a way to react to the http resonse and only fetch an access token if its a 401. But because OkHttp's design for Authenticator means its not called until the client receives a challenge from the server, this would still mean that every export request gets a 401 followed by a 200.

It seems like the only way that Authenticator can be useful right now is by using it _in combination with setHeaders(Supplier<Map<String, String>>). Something like this:

    MyAuthenticator authenticator = new MyAuthenticator("username", "password");
    OtlpHttpSpanExporter.builder()
        .setEndpoint(...)
        .setAuthenticator(authenticator)
        .setHeaders(authenticator)
        .build();

  private static class MyAuthenticator implements Supplier<Map<String, String>>, Authenticator {
    private final String username;
    private final String password;
    private String token;

    private MyAuthenticator(String username, String password) {
      this.username = username;
      this.password = password;
    }

    private String refreshToken() {
      // ... fetch token from network location using username / password
    }

    private synchronized String getToken(boolean forceRefresh) {
      if (forceRefresh || token == null) {
        token = refreshToken();
      }
      return token;
    }

    @Override
    public Map<String, String> getHeaders() {
      return Collections.singletonMap("Authorization", "Bearer " + getToken(false));
    }

    @Override
    public Map<String, String> get() {
      return Collections.singletonMap("Authorization", "Bearer " + getToken(true));
    }
  }

Notes:

• MyAuthenticator provides initial headers for the request by implementing Supplier<Map<String, String> via get()
• MyAuthenticator responds to 401s by the server and fetches a new access token by implementing Authenticator via getHeader()
• MyAuthenticator retains internal state for the current access token, and refreshes it when Authenticator is called due to a challenge from the server

The problem with this design is that it relies on the underlying HTTP client to be able to call Authenticator only when it receives a challenge from the server (i.e. 401 response). OkHttp supports this. We can make the JDK HttpClient behave like this. I'm not sure if the upstream grpc client library has the configurability to do something like this.

[jack-berg]

cc @saxocellphone

[jack-berg]

@sfriberg - you contributed Authenticator originally. I wonder if you have any thoughts on this.

[saxocellphone]

So if I'm understanding correctly, current Authenticator doesn't provide extra value over setHeaders, and if I want to pass in an authentication header for grpc, I should just do something like

OtlpGrpcMetricExporterBuilder otlpGrpcMetricExporterBuilder = OtlpGrpcMetricExporter.builder().setHeaders(() -> {
  String token = GoogleCredentials.getApplicationDefault().refreshAccessToken().getTokenValue();
  return Map.of("custom-auth-header", token);
}).setEndpoint(collectorEndpoint);

following header convention here: https://grpc.io/docs/guides/auth/#with-server-authentication-ssltls-and-a-custom-header-with-token.

[jack-berg]

Yes, currently that's the case. But ideally the supplier is aware of the access token's TTL, and only refreshes when needed, instead of on every request.

Its possible we could enhance authenticator, but I'm struggling to imagine a situation where it would offer any benefit over the setHeaders(Supplier<Map<String, String>>). For the supplier approach to not work, you'd have to have an opaque access token (i.e. not something like a JWT where you can decode it and determine the expiration time). But even if the token is opaque, you can still take a naive approach and do something like "refresh if more than N seconds has passed".

Right now, I'm inclined to delete Authenticator. WDYT?

[saxocellphone]

Deleting it makes sense to me. But it'll be nice to document how to do authentication in the official documentation, given that the interest for authentication is increasing. When I was doing research, the Authenticator method only showed up in the release notes and github issues, and wasn't aware of the setHeaders method.