Skip to content

Commit 31c2e97

Browse files
hhundhhund
committed
refactored code, improved oidc / jwks handling (use='sig', min RSA)
1 parent bdeddf3 commit 31c2e97

8 files changed

Lines changed: 198 additions & 102 deletions

File tree

‎dsf-bpe/dsf-bpe-server/src/main/java/dev/dsf/bpe/client/oidc/OidcClientJersey.java‎

Lines changed: 7 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -169,22 +169,6 @@ public DecodedJWT getAccessTokenDecoded(OidcConfiguration configuration, Jwks jw
169169
}
170170
}
171171

172-
/**
173-
* Does not verify if the access token is expired. Supported algorithms: RS256, RS384, RS512, ES256, ES384 and
174-
* ES512.
175-
*
176-
* @param accessToken
177-
* not <code>null</code>
178-
* @param jwks
179-
* not <code>null</code>
180-
* @return decoded access token
181-
* @throws OidcClientException
182-
* if verification fails, the public key to verify is unknown or a unsupported signature algorithm was
183-
* used.
184-
*
185-
* @see DecodedJWT#getExpiresAt()
186-
* @see DecodedJWT#getExpiresAtAsInstant()
187-
*/
188172
private DecodedJWT verifyAndDecodeAccessToken(String accessToken, Jwks jwks) throws OidcClientException
189173
{
190174
try
@@ -196,14 +180,15 @@ private DecodedJWT verifyAndDecodeAccessToken(String accessToken, Jwks jwks) thr
196180
throw new OidcClientException("Access token has no kid property");
197181

198182
Optional<JwksKey> key = jwks.getKey(keyId);
199-
if (key.isEmpty())
200-
throw new OidcClientException("Access token key with kid '" + keyId + "' not in JWKS");
183+
if (key.isEmpty() || !key.get().use().equals("sig"))
184+
throw new OidcClientException("Access token key with kid '" + keyId + "' and use 'sig' not in JWKS");
201185

202-
Optional<Algorithm> algorithm = key.map(JwksKey::toAlgorithm);
186+
Optional<Algorithm> algorithm = key.flatMap(JwksKey::toAlgorithm);
203187
if (key.isEmpty())
188+
{
204189
throw new OidcClientException("Access token key with kid '" + keyId
205-
+ "' has unsupported type (kty) / algorithm (alg) in JWKS '" + key.get().kty() + "' / '"
206-
+ key.get().alg() + "'");
190+
+ "' has unsupported type (kty) / algorithm (alg) / key-size in JWKS");
191+
}
207192

208193
try
209194
{
@@ -231,7 +216,7 @@ else if (requiredAudiences.size() > 1)
231216
}
232217
catch (TokenExpiredException e)
233218
{
234-
throw new OidcClientException("JWT verification failed: claim missing", e);
219+
throw new OidcClientException("JWT verification failed: token expired", e);
235220
}
236221
catch (IncorrectClaimException e)
237222
{

‎dsf-bpe/dsf-bpe-server/src/main/java/dev/dsf/bpe/client/oidc/OidcClientWithCache.java‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ private static final record CacheEntry<T>(ZonedDateTime timeout, T resource)
3232
{
3333
}
3434

35-
private final Duration cacheTimeoutconfigurationResource;
35+
private final Duration cacheTimeoutConfigurationResource;
3636
private final Duration cacheTimeoutJwksResource;
3737
private final Duration cacheTimeoutAccessTokenBeforeExpiration;
3838
private final OidcClientWithDecodedJwt delegate;
@@ -42,7 +42,7 @@ private static final record CacheEntry<T>(ZonedDateTime timeout, T resource)
4242
private CacheEntry<DecodedJWT> accessTokenCache;
4343

4444
/**
45-
* @param cacheTimeoutconfigurationResource
45+
* @param cacheTimeoutConfigurationResource
4646
* not <code>null</code>, not negative
4747
* @param cacheTimeoutJwksResource
4848
* not <code>null</code>, not negative
@@ -51,13 +51,13 @@ private static final record CacheEntry<T>(ZonedDateTime timeout, T resource)
5151
* @param delegate
5252
* not <code>null</code>
5353
*/
54-
public OidcClientWithCache(Duration cacheTimeoutconfigurationResource, Duration cacheTimeoutJwksResource,
54+
public OidcClientWithCache(Duration cacheTimeoutConfigurationResource, Duration cacheTimeoutJwksResource,
5555
Duration cacheTimeoutAccessTokenBeforeExpiration, OidcClientWithDecodedJwt delegate)
5656
{
57-
this.cacheTimeoutconfigurationResource = Objects.requireNonNull(cacheTimeoutconfigurationResource,
58-
"cacheTimeoutconfigurationResource");
59-
if (cacheTimeoutconfigurationResource.isNegative())
60-
throw new IllegalArgumentException("cacheTimeoutconfigurationResource negative");
57+
this.cacheTimeoutConfigurationResource = Objects.requireNonNull(cacheTimeoutConfigurationResource,
58+
"cacheTimeoutConfigurationResource");
59+
if (cacheTimeoutConfigurationResource.isNegative())
60+
throw new IllegalArgumentException("cacheTimeoutConfigurationResource negative");
6161

6262
this.cacheTimeoutJwksResource = Objects.requireNonNull(cacheTimeoutJwksResource, "cacheTimeoutJwksResource");
6363
if (cacheTimeoutJwksResource.isNegative())
@@ -73,13 +73,13 @@ public OidcClientWithCache(Duration cacheTimeoutconfigurationResource, Duration
7373
@Override
7474
public Configuration getConfiguration() throws OidcClientException
7575
{
76-
if (configurationCache != null && configurationCache.timeout.isBefore(ZonedDateTime.now()))
76+
if (configurationCache != null && configurationCache.timeout.isAfter(ZonedDateTime.now()))
7777
return configurationCache.resource;
7878
else
7979
{
8080
Configuration configuration = delegate.getConfiguration();
8181
configurationCache = new CacheEntry<Configuration>(
82-
ZonedDateTime.now().plus(cacheTimeoutconfigurationResource), configuration);
82+
ZonedDateTime.now().plus(cacheTimeoutConfigurationResource), configuration);
8383
return configuration;
8484
}
8585
}
@@ -89,7 +89,7 @@ public Jwks getJwks() throws OidcClientException
8989
{
9090
Configuration configuration = getConfiguration();
9191

92-
if (jwksCache != null && jwksCache.timeout.isBefore(ZonedDateTime.now()))
92+
if (jwksCache != null && jwksCache.timeout.isAfter(ZonedDateTime.now()))
9393
return jwksCache.resource;
9494
else
9595
{

‎dsf-common/dsf-common-jetty/src/main/java/dev/dsf/common/auth/DsfOpenIdLoginService.java‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,12 @@ public class DsfOpenIdLoginService extends OpenIdLoginService
3333
{
3434
private static final Logger logger = LoggerFactory.getLogger(DsfOpenIdLoginService.class);
3535

36-
private final OpenIdConfiguration configuration;
3736
private final LoginService loginService;
3837

3938
public DsfOpenIdLoginService(OpenIdConfiguration configuration, LoginService loginService)
4039
{
4140
super(configuration, loginService);
4241

43-
this.configuration = configuration;
4442
this.loginService = loginService;
4543
}
4644

@@ -49,17 +47,6 @@ public UserIdentity login(String identifier, Object credentials, Request request
4947
Function<Boolean, Session> getOrCreateSession)
5048
{
5149
OpenIdCredentials openIdCredentials = (OpenIdCredentials) credentials;
52-
try
53-
{
54-
openIdCredentials.redeemAuthCode(configuration);
55-
}
56-
catch (Exception e)
57-
{
58-
logger.debug("Unable to redeem auth code", e);
59-
logger.warn("Unable to redeem auth code: {} - {}", e.getClass().getName(), e.getMessage());
60-
61-
return null;
62-
}
6350

6451
return loginService.login(openIdCredentials.getUserId(), credentials, request, getOrCreateSession);
6552
}

‎dsf-common/dsf-common-jetty/src/main/java/dev/dsf/common/config/AbstractJettyConfig.java‎

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,14 @@ public abstract class AbstractJettyConfig extends AbstractCertificateConfig
162162
@Value("${dev.dsf.server.auth.oidc.provider.discovery.path:/.well-known/openid-configuration}")
163163
private String oidcProviderDiscoveryPath;
164164

165+
@Documentation(description = "OIDC provider client cache timeout of the 'openid-configuration' discovery resource")
166+
@Value("${dev.dsf.server.auth.oidc.provider.client.cache.timeout.configuration.resource:PT1H}")
167+
private String oidcProviderClientCacheConfigurationResourceTimeout;
168+
169+
@Documentation(description = "OIDC provider client cache timeout of the jwks resource")
170+
@Value("${dev.dsf.server.auth.oidc.provider.client.cache.timeout.jwks.resource:PT1H}")
171+
private String oidcProviderClientCacheJwksResourceTimeout;
172+
165173
@Documentation(description = "OIDC provider client connect timeout")
166174
@Value("${dev.dsf.server.auth.oidc.provider.client.timeout.connect:PT5S}")
167175
private String oidcProviderClientTimeoutConnect;
@@ -407,24 +415,44 @@ public BaseOidcClient baseOidcClient()
407415
char[] keyStorePassword = UUID.randomUUID().toString().toCharArray();
408416
KeyStore keyStore = oidcProviderClientKeyStore(keyStorePassword);
409417

410-
return new BaseOidcClientWithCache(new BaseOidcClientJersey(oidcProviderRealmBaseUrl, oidcProviderDiscoveryPath,
411-
trustStore, keyStore, keyStore == null ? null : keyStorePassword, proxyUrl, proxyUsername,
412-
proxyPassword, buildInfoReader().getUserAgentValue(), oidcProviderClientTimeoutConnect(),
413-
oidcProviderClientTimeoutRead(), false));
418+
return new BaseOidcClientWithCache(getOidcProviderClientCacheConfigurationResourceTimeout(),
419+
getOidcProviderClientCacheJwksResourceTimeout(),
420+
new BaseOidcClientJersey(oidcProviderRealmBaseUrl, oidcProviderDiscoveryPath, trustStore, keyStore,
421+
keyStore == null ? null : keyStorePassword, proxyUrl, proxyUsername, proxyPassword,
422+
buildInfoReader().getUserAgentValue(), oidcProviderClientTimeoutConnect(),
423+
oidcProviderClientTimeoutRead(), false));
424+
}
425+
426+
private Duration getOidcProviderClientCacheConfigurationResourceTimeout()
427+
{
428+
return assertPositive(Duration.parse(oidcProviderClientCacheConfigurationResourceTimeout));
429+
}
430+
431+
private Duration getOidcProviderClientCacheJwksResourceTimeout()
432+
{
433+
return assertPositive(Duration.parse(oidcProviderClientCacheJwksResourceTimeout));
414434
}
415435

416436
@Bean
417437
@Lazy
418438
public Duration oidcProviderClientTimeoutRead()
419439
{
420-
return Duration.parse(oidcProviderClientTimeoutRead);
440+
return assertPositive(Duration.parse(oidcProviderClientTimeoutRead));
421441
}
422442

423443
@Bean
424444
@Lazy
425445
public Duration oidcProviderClientTimeoutConnect()
426446
{
427-
return Duration.parse(oidcProviderClientTimeoutConnect);
447+
return assertPositive(Duration.parse(oidcProviderClientTimeoutConnect));
448+
}
449+
450+
private Duration assertPositive(Duration duration)
451+
{
452+
if (duration != null && duration.isNegative())
453+
throw new IllegalArgumentException("configured duration is negative");
454+
else
455+
return duration;
428456
}
429457

430458
private Proxy oidcClientProxy()

‎dsf-common/dsf-common-oidc/src/main/java/dev/dsf/common/oidc/BaseOidcClientWithCache.java‎

Lines changed: 30 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,51 +15,65 @@
1515
*/
1616
package dev.dsf.common.oidc;
1717

18+
import java.time.Duration;
19+
import java.time.ZonedDateTime;
1820
import java.util.Objects;
1921
import java.util.concurrent.atomic.AtomicReference;
2022
import java.util.function.Supplier;
2123

2224
public class BaseOidcClientWithCache implements BaseOidcClient
2325
{
24-
private final BaseOidcClient delegate;
26+
private static final record CacheEntry<T>(ZonedDateTime timeout, T resource)
27+
{
28+
}
29+
30+
private final Duration cacheTimeoutConfigurationResource;
31+
private final Duration cacheTimeoutJwksResource;
32+
33+
private final AtomicReference<CacheEntry<OidcConfiguration>> configurationCache = new AtomicReference<>();
34+
private final AtomicReference<CacheEntry<Jwks>> jwksCache = new AtomicReference<>();
2535

26-
private final AtomicReference<OidcConfiguration> oidcConfiguration = new AtomicReference<>();
27-
private final AtomicReference<Jwks> jwks = new AtomicReference<>();
36+
private final BaseOidcClient delegate;
2837

2938
/**
39+
* @param cacheTimeoutconfigurationResource
40+
* not <code>null</code>
41+
* @param cacheTimeoutJwksResource
42+
* not <code>null</code>
3043
* @param delegate
3144
* not <code>null</code>
3245
*/
33-
public BaseOidcClientWithCache(BaseOidcClient delegate)
46+
public BaseOidcClientWithCache(Duration cacheTimeoutconfigurationResource, Duration cacheTimeoutJwksResource,
47+
BaseOidcClient delegate)
3448
{
49+
this.cacheTimeoutConfigurationResource = Objects.requireNonNull(cacheTimeoutconfigurationResource,
50+
"cacheTimeoutconfigurationResource");
51+
this.cacheTimeoutJwksResource = Objects.requireNonNull(cacheTimeoutJwksResource, "cacheTimeoutJwksResource");
3552
this.delegate = Objects.requireNonNull(delegate, "delegate");
3653
}
3754

3855
@Override
3956
public OidcConfiguration getConfiguration() throws OidcClientException
4057
{
41-
return getOrSet(oidcConfiguration, delegate::getConfiguration);
58+
return getOrSet(configurationCache, cacheTimeoutConfigurationResource, delegate::getConfiguration);
4259
}
4360

44-
private <T> T getOrSet(AtomicReference<T> cache, Supplier<T> supplier)
61+
private <T> T getOrSet(AtomicReference<CacheEntry<T>> cache, Duration timeout, Supplier<T> supplier)
4562
{
46-
T cached = cache.get();
47-
if (cached == null)
63+
CacheEntry<T> cached = cache.get();
64+
if (cached != null && cached.timeout.isAfter(ZonedDateTime.now()))
65+
return cached.resource;
66+
else
4867
{
49-
T value = supplier.get();
50-
if (cache.compareAndSet(cached, value))
51-
return value;
52-
else
53-
return cache.get();
68+
cache.compareAndSet(cached, new CacheEntry<>(ZonedDateTime.now().plus(timeout), supplier.get()));
69+
return cache.get().resource;
5470
}
55-
else
56-
return cached;
5771
}
5872

5973
@Override
6074
public Jwks getJwks() throws OidcClientException
6175
{
62-
return getOrSet(jwks, () -> delegate.getJwks(getConfiguration()));
76+
return getOrSet(jwksCache, cacheTimeoutJwksResource, () -> delegate.getJwks(getConfiguration()));
6377
}
6478

6579
@Override

0 commit comments

Comments
 (0)
⚡