Skip to content

fix ZTIS certificate caching #1135

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.

Sign up for GitHub

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

Open
mpern wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix ZTIS certificate caching #1135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions cloudplatform/connectivity-oauth/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,16 @@
<scope>runtime</scope>
</dependency>
<!-- scope "test" -->
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcprov-jdk18on</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcpkix-jdk18on</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.sap.cloud.environment.servicebinding.api</groupId>
<artifactId>java-access-api</artifactId>
Expand Down
114 changes: 114 additions & 0 deletions ...rc/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/SecurityLibWorkaroundsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,30 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;

import java.math.BigInteger;
import java.security.KeyPair;
import java.security.KeyPairGenerator;
import java.security.KeyStore;
import java.security.Provider;
import java.security.Security;
import java.security.cert.Certificate;
import java.util.Date;

import org.bouncycastle.asn1.ASN1ObjectIdentifier;
import org.bouncycastle.asn1.x500.X500Name;
import org.bouncycastle.asn1.x509.BasicConstraints;
import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter;
import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder;
import org.bouncycastle.jce.provider.BouncyCastleProvider;
import org.bouncycastle.operator.ContentSigner;
import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder;
import org.junit.jupiter.api.Test;

import com.sap.cloud.sdk.cloudplatform.connectivity.SecurityLibWorkarounds.ZtisClientIdentity;
import com.sap.cloud.security.config.CredentialType;

import lombok.SneakyThrows;

class SecurityLibWorkaroundsTest
{
@Test
Expand All @@ -23,6 +40,103 @@ void testZtisClientIdentityImplementsEquals()
assertThat(sut).doesNotHaveSameHashCodeAs(new ZtisClientIdentity("id2", mock(KeyStore.class)));
}

@SneakyThrows
@Test
void testZtisClientIdentityEqualityWithRealCertificates()
{
// Two keystores loaded with different certificates must be considered unequal.
// This is what drives cache-key differentiation in OAuth2Service.tokenServiceCache
// when the SVID cert rotates.
final KeyPair keyPair = generateKeyPair();
final Certificate certA = generateCertificate(keyPair, "CN=cert-a");
final Certificate certB = generateCertificate(keyPair, "CN=cert-b");

final KeyStore ksA = buildKeyStore(keyPair, certA);
final KeyStore ksB = buildKeyStore(keyPair, certB);
final KeyStore ksSameAsA = buildKeyStore(keyPair, certA);

final ZtisClientIdentity identityA = new ZtisClientIdentity("client", ksA);
final ZtisClientIdentity identityB = new ZtisClientIdentity("client", ksB);
final ZtisClientIdentity identitySameAsA = new ZtisClientIdentity("client", ksSameAsA);

// Different cert → unequal: a new tokenServiceCache entry would be created after rotation
assertThat(identityA).isNotEqualTo(identityB);
assertThat(identityA).doesNotHaveSameHashCodeAs(identityB);

// Same cert content → equal: tokenServiceCache hit, HttpClient is reused (expected behaviour)
assertThat(identityA).isEqualTo(identitySameAsA);
assertThat(identityA).hasSameHashCodeAs(identitySameAsA);
}

@SneakyThrows
@Test
void testOAuth2ServiceTokenCacheKeyChangesWhenCertRotates()
{
// This test documents the self-healing behaviour of OAuth2Service.tokenServiceCache:
// when the SVID cert rotates, getZtisIdentity() produces a new ZtisClientIdentity with a
// different certificate, which hashes to a different cache key, causing tokenServiceCache
// to create a new OAuth2TokenService (and thus a new HttpClient with a fresh SSLContext).
//
// PRECONDITION for this to work: tryGetDestination() (and therefore getZtisIdentity())
// must be called again after cert rotation. If the HttpDestination is cached at a higher
// level and re-used indefinitely, the ZtisClientIdentity inside OAuth2Service is never
// refreshed and the cache key never changes.
final KeyPair keyPair = generateKeyPair();
final Certificate certBeforeRotation = generateCertificate(keyPair, "CN=before-rotation");
final Certificate certAfterRotation = generateCertificate(keyPair, "CN=after-rotation");

final KeyStore ksBefore = buildKeyStore(keyPair, certBeforeRotation);
final KeyStore ksAfter = buildKeyStore(keyPair, certAfterRotation);

final ZtisClientIdentity identityBefore = new ZtisClientIdentity("client", ksBefore);
final ZtisClientIdentity identityAfter = new ZtisClientIdentity("client", ksAfter);

// Simulate OAuth2Service.getTokenService() cache key computation
final com.sap.cloud.sdk.cloudplatform.cache.CacheKey keyBefore =
com.sap.cloud.sdk.cloudplatform.cache.CacheKey.fromIds(null, null).append(identityBefore);
final com.sap.cloud.sdk.cloudplatform.cache.CacheKey keyAfter =
com.sap.cloud.sdk.cloudplatform.cache.CacheKey.fromIds(null, null).append(identityAfter);

// Cache keys must differ → cache miss → new HttpClient built with rotated cert
assertThat(keyBefore).isNotEqualTo(keyAfter);
}

@SneakyThrows
private static KeyPair generateKeyPair()
{
final KeyPairGenerator kpg = KeyPairGenerator.getInstance("RSA");
kpg.initialize(2048);
return kpg.generateKeyPair();
}

@SneakyThrows
private static Certificate generateCertificate( final KeyPair keyPair, final String subject )
{
final long now = System.currentTimeMillis();
final X500Name name = new X500Name(subject);
final BigInteger serial = new BigInteger(Long.toString(now));
final Date startDate = new Date(now);
final Date endDate = new Date(now + 3_600_000L);

final JcaX509v3CertificateBuilder certBuilder =
new JcaX509v3CertificateBuilder(name, serial, startDate, endDate, name, keyPair.getPublic());
certBuilder.addExtension(new ASN1ObjectIdentifier("2.5.29.19"), true, new BasicConstraints(true));

final Provider prov = new BouncyCastleProvider();
Security.addProvider(prov);
final ContentSigner contentSigner = new JcaContentSignerBuilder("SHA256WithRSA").build(keyPair.getPrivate());
return new JcaX509CertificateConverter().setProvider(prov).getCertificate(certBuilder.build(contentSigner));
}

@SneakyThrows
private static KeyStore buildKeyStore( final KeyPair keyPair, final Certificate cert )
{
final KeyStore ks = KeyStore.getInstance("JKS");
ks.load(null);
ks.setKeyEntry("spiffe", keyPair.getPrivate(), new char[0], new Certificate[] { cert });
return ks;
}

@Test
void testGetCredentialType()
{
Expand Down
10 changes: 10 additions & 0 deletions cloudplatform/connectivity-ztis/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,16 @@
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcprov-jdk18on</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcpkix-jdk18on</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
Expand Down
10 changes: 8 additions & 2 deletions .../src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ZeroTrustIdentityService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ public class ZeroTrustIdentityService
private static final String DEFAULT_SOCKET_PATH = "unix:///tmp/spire-agent/public/api.sock";
private static final String SOCKET_ENVIRONMENT_VARIABLE = "SPIFFE_ENDPOINT_SOCKET";
private static final Duration DEFAULT_SOCKET_TIMEOUT = Duration.ofSeconds(10);
// Invalidate the cached KeyStore this long before the SVID expires to tolerate slow SPIRE rotation
static final Duration SVID_EXPIRY_SAFETY_MARGIN = Duration.ofDays(1);
@Getter
private static final ZeroTrustIdentityService instance = new ZeroTrustIdentityService();
private final Lazy<X509Source> source = Lazy.of(this::initX509Source);
Expand Down Expand Up @@ -223,8 +225,12 @@ KeyStore loadKeyStore( @Nonnull final X509Svid svid )

boolean isKeyStoreCached( @Nonnull final X509Svid svid )
{
// X509Svid does implement equals, so we don't have to manually compare the certificates
return keyStoreCache != null && svid.equals(keyStoreCache.svid());
if( keyStoreCache == null || !svid.getSpiffeId().equals(keyStoreCache.svid().getSpiffeId()) ) {
return false;
}
// Treat as not cached if the SVID is already expired or expires within the safety margin.
final Instant expiryWithMargin = svid.getLeaf().getNotAfter().toInstant().minus(SVID_EXPIRY_SAFETY_MARGIN);
return Instant.now().isBefore(expiryWithMargin);
}

@RequiredArgsConstructor
Expand Down
Loading
Loading