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

fix: issuedAt now validated with leeway in Oauth2ExpirationIssuedAtValidationRule #3728

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public class Oauth2ServiceConfiguration {
private String publicCertificateAlias;
private String providerAudience;
private int notBeforeValidationLeeway;
private int issuedAtValidationLeeway;
private String endpointAudience;

private Long tokenExpiration;
Expand Down Expand Up @@ -77,6 +78,10 @@ public int getNotBeforeValidationLeeway() {
return notBeforeValidationLeeway;
}

public int getIssuedAtValidationLeeway() {
richardtreier marked this conversation as resolved.
Show resolved Hide resolved
return issuedAtValidationLeeway;
}

public String getEndpointAudience() {
return endpointAudience;
}
Expand Down Expand Up @@ -146,6 +151,11 @@ public Builder notBeforeValidationLeeway(int notBeforeValidationLeeway) {
return this;
}

public Builder issuedAtValidationLeeway(int issuedAtValidationLeeway) {
configuration.issuedAtValidationLeeway = issuedAtValidationLeeway;
return this;
}

public Builder endpointAudience(String endpointAudience) {
configuration.endpointAudience = endpointAudience;
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,10 @@ public class Oauth2ServiceExtension implements ServiceExtension {
private static final String TOKEN_EXPIRATION = "edc.oauth.token.expiration"; // in minutes
@Setting
private static final String CLIENT_ID = "edc.oauth.client.id";
@Setting
@Setting(value = "Leeway in seconds for validating the not before (nbf) claim in the token")
richardtreier marked this conversation as resolved.
Show resolved Hide resolved
private static final String NOT_BEFORE_LEEWAY = "edc.oauth.validation.nbf.leeway";
@Setting(value = "Leeway in seconds for validating the issuedAt claim in the token")
richardtreier marked this conversation as resolved.
Show resolved Hide resolved
private static final String ISSUED_AT_LEEWAY = "edc.oauth.validation.issued.at.leeway";
private IdentityProviderKeyResolver providerKeyResolver;

@Inject
Expand Down Expand Up @@ -167,8 +169,22 @@ private Oauth2ServiceConfiguration createConfig(ServiceExtensionContext context)
.privateKeyResolver(privateKeyResolver)
.certificateResolver(certificateResolver)
.notBeforeValidationLeeway(context.getSetting(NOT_BEFORE_LEEWAY, 10))
.issuedAtValidationLeeway(getIssuedAtValidationLeeway(context))
.tokenExpiration(TimeUnit.MINUTES.toSeconds(tokenExpiration))
.build();
}

private int getIssuedAtValidationLeeway(ServiceExtensionContext context) {
if (!context.getConfig().hasKey(ISSUED_AT_LEEWAY)) {
String message = "No value was configured for '%s'.".formatted(ISSUED_AT_LEEWAY) +
richardtreier marked this conversation as resolved.
Show resolved Hide resolved
" Please consider configuring a leeway of 2-5s. The check 'iat <= now' can" +
" cause errors in production due to the comparison of second-rounded and" +
" nanosecond-precise dates, because the rounding and rounding direction are" +
" implementation specific, platform specific, but also within spec for JWT.";
context.getMonitor().info(message);
}

return context.getSetting(ISSUED_AT_LEEWAY, 0);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,11 @@
public class Oauth2ExpirationIssuedAtValidationRule implements TokenValidationRule {

private final Clock clock;
private final int issuedAtValidationLeeway;

public Oauth2ExpirationIssuedAtValidationRule(Clock clock) {
public Oauth2ExpirationIssuedAtValidationRule(Clock clock, int issuedAtValidationLeeway) {
richardtreier marked this conversation as resolved.
Show resolved Hide resolved
this.clock = clock;
this.issuedAtValidationLeeway = issuedAtValidationLeeway;
}

@Override
Expand All @@ -51,7 +53,7 @@ public Result<Void> checkRule(@NotNull ClaimToken toVerify, @Nullable Map<String
if (issuedAt != null) {
if (issuedAt.isAfter(expires)) {
return Result.failure("Issued at (iat) claim is after expiration time (exp) claim in token");
} else if (now.isBefore(issuedAt)) {
} else if (now.plusSeconds(issuedAtValidationLeeway).isBefore(issuedAt)) {
return Result.failure("Current date/time before issued at (iat) claim in token");
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ public class Oauth2ValidationRulesRegistryImpl extends TokenValidationRulesRegis
public Oauth2ValidationRulesRegistryImpl(Oauth2ServiceConfiguration configuration, Clock clock) {
this.addRule(new Oauth2AudienceValidationRule(configuration.getEndpointAudience()));
this.addRule(new Oauth2NotBeforeValidationRule(clock, configuration.getNotBeforeValidationLeeway()));
this.addRule(new Oauth2ExpirationIssuedAtValidationRule(clock));
this.addRule(new Oauth2ExpirationIssuedAtValidationRule(clock, configuration.getIssuedAtValidationLeeway()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package org.eclipse.edc.iam.oauth2;

import org.eclipse.edc.junit.extensions.DependencyInjectionExtension;
import org.eclipse.edc.spi.monitor.Monitor;
import org.eclipse.edc.spi.security.CertificateResolver;
import org.eclipse.edc.spi.security.PrivateKeyResolver;
import org.eclipse.edc.spi.system.ServiceExtensionContext;
Expand All @@ -29,6 +30,7 @@
import java.util.Map;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.contains;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
Expand All @@ -49,24 +51,75 @@ void setup(ServiceExtensionContext context) {
}

@Test
void verifyExtensionWithCertificateAlias(Oauth2ServiceExtension extension, ServiceExtensionContext context) throws CertificateEncodingException {
void verifyExtensionWithCertificateAlias(Oauth2ServiceExtension extension, ServiceExtensionContext context) {
var config = spy(ConfigFactory.fromMap(Map.of(
"edc.oauth.client.id", "id",
"edc.oauth.token.url", "url",
"edc.oauth.certificate.alias", "alias",
"edc.oauth.private.key.alias", "p_alias")));
when(context.getConfig(any())).thenReturn(config);
var certificate = mock(X509Certificate.class);
var privateKey = mock(PrivateKey.class);
when(privateKey.getAlgorithm()).thenReturn("RSA");
when(certificate.getEncoded()).thenReturn(new byte[] {});
when(certificateResolver.resolveCertificate("alias")).thenReturn(certificate);
when(privateKeyResolver.resolvePrivateKey("p_alias", PrivateKey.class)).thenReturn(privateKey);
mockCertificate("alias");
mockRsaPrivateKey("p_alias");

when(context.getConfig(any())).thenReturn(config);
extension.initialize(context);

verify(config, times(1)).getString("edc.oauth.certificate.alias");
verify(config, never()).getString("edc.oauth.public.key.alias");
}

@Test
void leewayWarningLoggedWhenLeewayUnconfigured(Oauth2ServiceExtension extension, ServiceExtensionContext context) {
var config = spy(ConfigFactory.fromMap(Map.of(
"edc.oauth.client.id", "id",
"edc.oauth.token.url", "url",
"edc.oauth.certificate.alias", "alias",
"edc.oauth.private.key.alias", "p_alias")));
mockCertificate("alias");
mockRsaPrivateKey("p_alias");

var monitor = mock(Monitor.class);
when(context.getMonitor()).thenReturn(monitor);
when(context.getConfig(any())).thenReturn(config);
extension.initialize(context);

var message = "No value was configured for 'edc.oauth.validation.issued.at.leeway'.";
verify(monitor, times(1)).info(contains(message));
}

@Test
void leewayNoWarningWhenLeewayConfigured(Oauth2ServiceExtension extension, ServiceExtensionContext context) {
var config = spy(ConfigFactory.fromMap(Map.of(
"edc.oauth.client.id", "id",
"edc.oauth.token.url", "url",
"edc.oauth.certificate.alias", "alias",
"edc.oauth.private.key.alias", "p_alias",
"edc.oauth.validation.issued.at.leeway", "5")));
mockCertificate("alias");
mockRsaPrivateKey("p_alias");

var monitor = mock(Monitor.class);
when(context.getMonitor()).thenReturn(monitor);
when(context.getConfig(any())).thenReturn(config);
extension.initialize(context);

var message = "No value was configured for 'edc.oauth.validation.issued.at.leeway'.";
verify(monitor, never()).info(contains(message));
}

private void mockRsaPrivateKey(String alias) {
var privateKey = mock(PrivateKey.class);
when(privateKey.getAlgorithm()).thenReturn("RSA");
when(privateKeyResolver.resolvePrivateKey(alias, PrivateKey.class)).thenReturn(privateKey);
}

private void mockCertificate(String alias) {
try {
var certificate = mock(X509Certificate.class);
when(certificate.getEncoded()).thenReturn(new byte[] {});
when(certificateResolver.resolveCertificate(alias)).thenReturn(certificate);
} catch (CertificateEncodingException e) {
// Should never happen, it's a checked exception in the way of mocking
throw new RuntimeException(e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class Oauth2ExpirationIssuedAtValidationRuleTest {

private final Instant now = Instant.now().truncatedTo(ChronoUnit.SECONDS);
private final Clock clock = Clock.fixed(now, UTC);
private final TokenValidationRule rule = new Oauth2ExpirationIssuedAtValidationRule(clock);
private final TokenValidationRule rule = new Oauth2ExpirationIssuedAtValidationRule(clock, 0);

@Test
void validationOk() {
Expand Down Expand Up @@ -96,5 +96,87 @@ void validationKoBecauseIssuedAtInFuture() {
assertThat(result.getFailureMessages()).hasSize(1).contains("Current date/time before issued at (iat) claim in token");
}

@Test
void validationKoBecauseIssuedAtInFutureOutsideLeeway() {
var rule = new Oauth2ExpirationIssuedAtValidationRule(clock, 5);

var token = ClaimToken.Builder.newInstance()
.claim(EXPIRATION_TIME, Date.from(now.plusSeconds(60)))
.claim(ISSUED_AT, Date.from(now.plusSeconds(10)))
.build();

var result = rule.checkRule(token, emptyMap());

assertThat(result.succeeded()).isFalse();
assertThat(result.getFailureMessages()).hasSize(1).contains("Current date/time before issued at (iat) claim in token");
}

@Test
void validationOkBecauseIssuedAtInFutureButWithinLeeway() {
var rule = new Oauth2ExpirationIssuedAtValidationRule(clock, 20);

var token = ClaimToken.Builder.newInstance()
.claim(EXPIRATION_TIME, Date.from(now.plusSeconds(60)))
.claim(ISSUED_AT, Date.from(now.plusSeconds(10)))
.build();

var result = rule.checkRule(token, emptyMap());

assertThat(result.succeeded()).isTrue();
}

/**
* Demonstrates situation where rounded dates in the JWT token cause validation failures
* <br>
* Rounding of dates in JWT is within spec and the direction of rounding is platform-dependant.
*/
@Test
void validationKoWithRoundedIssuedAtAndNoLeeway() {
// time skew: tokens have dates rounded up to the second
var issuedAt = Instant.now().truncatedTo(ChronoUnit.SECONDS);
var expiresAt = issuedAt.plusSeconds(60);

// time skew: the connector is still in the previous second, with unrounded dates
var now = issuedAt.minus(250, ChronoUnit.MILLIS);

var clock = Clock.fixed(now, UTC);
var rule = new Oauth2ExpirationIssuedAtValidationRule(clock, 0);

var token = ClaimToken.Builder.newInstance()
.claim(EXPIRATION_TIME, Date.from(expiresAt))
.claim(ISSUED_AT, Date.from(issuedAt))
.build();

var result = rule.checkRule(token, emptyMap());

assertThat(result.succeeded()).isFalse();
assertThat(result.getFailureMessages()).hasSize(1).contains("Current date/time before issued at (iat) claim in token");
}

/**
* Regression test against clock skew and platform-dependant rounding of dates, solved with a 2s leeway.
* <br>
* Rounding of dates in JWT is within spec and the direction of rounding is platform-dependant.
*/
@Test
void validationOkWithRoundedIssuedAtAndMinimalLeeway() {
// time skew: tokens have dates rounded up to the second
var issuedAt = Instant.now().truncatedTo(ChronoUnit.SECONDS);
var expiresAt = issuedAt.plusSeconds(60);

// time skew: the connector is still in the previous second, with unrounded dates
var now = issuedAt.minus(250, ChronoUnit.MILLIS);

var clock = Clock.fixed(now, UTC);
var rule = new Oauth2ExpirationIssuedAtValidationRule(clock, 2);

var token = ClaimToken.Builder.newInstance()
.claim(EXPIRATION_TIME, Date.from(expiresAt))
.claim(ISSUED_AT, Date.from(issuedAt))
.build();

var result = rule.checkRule(token, emptyMap());

assertThat(result.succeeded()).isTrue();
}
}