[fix][broker] Fix token expiration

[nodece]

Signed-off-by: Zixuan Liu nodeces@gmail.com

Motivation

When token expiration, the broker requests the client to refresh the token, then the broker performs org.apache.pulsar.broker.service.ServerCnx#doAuthentication when the broker receives the auth response, which uses org.apache.pulsar.broker.authentication.AuthenticationState#authenticate to authentication, but the org.apache.pulsar.broker.authentication.AuthenticationProviderToken.TokenAuthenticationState#authenticate doesn't do anything, this cause a loop to refresh the token.

Right now the token is only validated in the TokenAuthenticationState constructor, we need to add a check to the authenticate method.

Modifications

• Add check the token expiration

Documentation

• doc-not-needed
  Fix

[nodece]

/pulsarbot rerun-failure-checks

[codelipenghui]

Looks the issue is introduced by #14044?

I just checked the branch-2.10

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

Lines 333 to 348 in 7c88bd1

	@Override
	public AuthData authenticate(AuthData authData) throws AuthenticationException {
	String token = new String(authData.getBytes(), UTF_8);
	this.jwt = provider.authenticateToken(token);
	this.authenticationDataSource = new AuthenticationDataCommand(token, remoteAddress, sslSession);
	if (jwt.getBody().getExpiration() != null) {
	this.expiration = jwt.getBody().getExpiration().getTime();
	} else {
	// Disable expiration
	this.expiration = Long.MAX_VALUE;
	}
	// There's no additional auth stage required
	return null;
	}

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

Lines 197 to 236 in 7c88bd1

	private Jwt<?, Claims> authenticateToken(final String token) throws AuthenticationException {
	try {
	Jwt<?, Claims> jwt = parser.parseClaimsJws(token);
	if (audienceClaim != null) {
	Object object = jwt.getBody().get(audienceClaim);
	if (object == null) {
	throw new JwtException("Found null Audience in token, for claimed field: " + audienceClaim);
	}
	if (object instanceof List) {
	List<String> audiences = (List<String>) object;
	// audience not contains this broker, throw exception.
	if (audiences.stream().noneMatch(audienceInToken -> audienceInToken.equals(audience))) {
	throw new AuthenticationException("Audiences in token: [" + String.join(", ", audiences)
	+ "] not contains this broker: " + audience);
	}
	} else if (object instanceof String) {
	if (!object.equals(audience)) {
	throw new AuthenticationException("Audiences in token: [" + object
	+ "] not contains this broker: " + audience);
	}
	} else {
	// should not reach here.
	throw new AuthenticationException("Audiences in token is not in expected format: " + object);
	}
	}
	if (jwt.getBody().getExpiration() != null) {
	expiringTokenMinutesMetrics.observe(
	(double) (jwt.getBody().getExpiration().getTime() - new Date().getTime()) / (60 * 1000));
	}
	return jwt;
	} catch (JwtException e) {
	if (e instanceof ExpiredJwtException) {
	expiredTokenMetrics.inc();
	}
	throw new AuthenticationException("Failed to authentication token: " + e.getMessage());
	}
	}

[nodece]

@codelipenghui @RobertIndie Thanks for your review! These request changes have been fixed, could you review this PR again?

[nodece]

/pulsarbot rerun-failure-checks

[nodece]

@codelipenghui The authenticate logic has been update.

[michaeljmarshall]

LGTM. Great catch @nodece! I'm glad we didn't cherry pick #14044 when it was merged since it would be live now.

For additional context, I had mistakenly approved this specific change, and here is a link to my justification: #14044 (comment). This change technically means that clients authenticate the token twice on the initial connection, but only once on token refreshes.