diff --git a/cloudplatform/connectivity-oauth/pom.xml b/cloudplatform/connectivity-oauth/pom.xml index 4335d9e8e..2402a84eb 100644 --- a/cloudplatform/connectivity-oauth/pom.xml +++ b/cloudplatform/connectivity-oauth/pom.xml @@ -135,10 +135,6 @@ org.apache.httpcomponents.core5 httpcore5 - - org.apache.commons - commons-lang3 - com.google.guava guava diff --git a/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliers.java b/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliers.java index 2e112c0b5..869176d6b 100644 --- a/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliers.java +++ b/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliers.java @@ -267,6 +267,8 @@ private void attachClientKeyStore( @Nonnull final OAuth2Options.Builder optionsB { final KeyStore maybeClientStore = getClientKeyStore(); if( maybeClientStore != null ) { + // note: in case the KS is loaded from ZTIS, the KS used for token retrieval and the KS registered here for mTLS to the target system may diverge + // Token retrieval supports certificate rotation in place, but mTLS to the target system requires re-loading the destination instead. optionsBuilder.withClientKeyStore(maybeClientStore); } } @@ -275,8 +277,8 @@ private void attachClientKeyStore( @Nonnull final OAuth2Options.Builder optionsB private KeyStore getClientKeyStore() { final ClientIdentity clientIdentity = getClientIdentity(); - if( clientIdentity instanceof ZtisClientIdentity ) { - return ((ZtisClientIdentity) clientIdentity).getKeyStore(); + if( clientIdentity instanceof ZtisClientIdentity ztisClientIdentity ) { + return ztisClientIdentity.getKeyStore(); } if( !(clientIdentity instanceof ClientCertificate) ) { return null; diff --git a/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultOAuth2PropertySupplier.java b/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultOAuth2PropertySupplier.java index 2daa1f50c..7367ab8e2 100644 --- a/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultOAuth2PropertySupplier.java +++ b/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultOAuth2PropertySupplier.java @@ -2,7 +2,6 @@ import java.net.URI; import java.net.URISyntaxException; -import java.security.KeyStore; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -173,14 +172,7 @@ private ZtisClientIdentity getZtisIdentity( @Nonnull final String clientid ) } final ZeroTrustIdentityService ztis = ZeroTrustIdentityService.getInstance(); - final KeyStore keyStore; - try { - keyStore = ztis.getOrCreateKeyStore(); - } - catch( final Exception e ) { - throw new CloudPlatformException("Failed to load X509 certificate for credential type X509_ATTESTED.", e); - } - return new ZtisClientIdentity(clientid, keyStore); + return new ZtisClientIdentity(clientid, ztis::getOrCreateKeyStore); } @Nonnull diff --git a/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2Service.java b/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2Service.java index 7dec1c128..a07af3fa2 100644 --- a/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2Service.java +++ b/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2Service.java @@ -97,6 +97,11 @@ class OAuth2Service OAuth2TokenService getTokenService( @Nullable final String tenantId ) { final CacheKey key = CacheKey.fromIds(tenantId, null).append(identity); + // ZTIS certificates rotate at runtime, thus we explicitly add the current KeyStore as cache key + // once the certificate rotates, this produces a cache miss, ensuring we construct a new HTTP client with a new certificate + if( identity instanceof final ZtisClientIdentity ztisIdentity ) { + key.append(ztisIdentity.getKeyStore()); + } return tokenServiceCache.get(key, this::createTokenService); } diff --git a/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/SecurityLibWorkarounds.java b/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/SecurityLibWorkarounds.java index 1ff70dff9..409bc8d0b 100644 --- a/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/SecurityLibWorkarounds.java +++ b/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/SecurityLibWorkarounds.java @@ -1,19 +1,16 @@ package com.sap.cloud.sdk.cloudplatform.connectivity; -import static com.sap.cloud.sdk.cloudplatform.connectivity.DestinationKeyStoreComparator.resolveCertificatesOnly; -import static com.sap.cloud.sdk.cloudplatform.connectivity.DestinationKeyStoreComparator.resolveKeyStoreHashCode; - import java.security.KeyStore; +import java.util.function.Supplier; import javax.annotation.Nonnull; -import org.apache.commons.lang3.builder.EqualsBuilder; -import org.apache.commons.lang3.builder.HashCodeBuilder; - +import com.sap.cloud.sdk.cloudplatform.exception.CloudPlatformException; import com.sap.cloud.security.config.ClientIdentity; -import lombok.AllArgsConstructor; -import lombok.Getter; +import lombok.EqualsAndHashCode; +import lombok.ToString; +import lombok.Value; final class SecurityLibWorkarounds { @@ -22,14 +19,18 @@ private SecurityLibWorkarounds() throw new IllegalStateException("This utility class should never be instantiated."); } - @Getter - @AllArgsConstructor + @Value static class ZtisClientIdentity implements ClientIdentity { @Nonnull - private final String id; + String id; + + // Exclude certificates from equals & hash code since they rotate dynamically at runtime + // Instead, the OAuth2Service cache explicitly checks for outdated KeyStores @Nonnull - private final KeyStore keyStore; + @EqualsAndHashCode.Exclude + @ToString.Exclude + Supplier keyStoreSource; @Override public boolean isCertificateBased() @@ -37,29 +38,17 @@ public boolean isCertificateBased() return true; } - // The identity will be used as cache key, so it's important we correctly implement equals/hashCode - @Override - public boolean equals( final Object obj ) + @Nonnull + KeyStore getKeyStore() { - if( this == obj ) { - return true; + try { + return keyStoreSource.get(); } - - if( obj == null || getClass() != obj.getClass() ) { - return false; + catch( final Exception e ) { + throw new CloudPlatformException( + "Failed to load X509 certificate for credential type X509_ATTESTED.", + e); } - - final ZtisClientIdentity that = (ZtisClientIdentity) obj; - return new EqualsBuilder() - .append(id, that.id) - .append(resolveCertificatesOnly(keyStore), resolveCertificatesOnly(that.keyStore)) - .isEquals(); - } - - @Override - public int hashCode() - { - return new HashCodeBuilder(41, 71).append(id).append(resolveKeyStoreHashCode(keyStore)).build(); } } } diff --git a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliersTest.java b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliersTest.java index ae9da1462..f7be0e7a8 100644 --- a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliersTest.java +++ b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliersTest.java @@ -757,8 +757,12 @@ void testMutualTlsWithZeroTrustIdentityService() final OAuth2PropertySupplier sut = IDENTITY_AUTHENTICATION.resolve(options); assertThat(sut).isNotNull(); + assertThat(sut.getClientIdentity()).isInstanceOf(SecurityLibWorkarounds.ZtisClientIdentity.class); - assertThatThrownBy(sut::getClientIdentity) + final var identity = (SecurityLibWorkarounds.ZtisClientIdentity) sut.getClientIdentity(); + assertThat(identity.getId()).isEqualTo("ias-client-id"); + + assertThatThrownBy(identity::getKeyStore) .isInstanceOf(CloudPlatformException.class) .describedAs("We are not mocking the ZTIS service here so this should fail") .hasRootCauseInstanceOf(ServiceBindingAccessException.class); diff --git a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultOAuth2PropertySupplierTest.java b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultOAuth2PropertySupplierTest.java index d1dff916e..860c666dd 100644 --- a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultOAuth2PropertySupplierTest.java +++ b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultOAuth2PropertySupplierTest.java @@ -206,9 +206,15 @@ void testCredentialTypeX509_ATTESTED() sut = new DefaultOAuth2PropertySupplier(options); assertThat(sut.getCredentialType()).isEqualTo(CredentialType.X509_ATTESTED); - assertThatThrownBy(sut::getClientIdentity) + assertThat(sut).isNotNull(); + assertThat(sut.getClientIdentity()).isInstanceOf(SecurityLibWorkarounds.ZtisClientIdentity.class); + + final var identity = (SecurityLibWorkarounds.ZtisClientIdentity) sut.getClientIdentity(); + assertThat(identity.getId()).isEqualTo("id"); + + assertThatThrownBy(identity::getKeyStore) .isInstanceOf(CloudPlatformException.class) - .describedAs("We are not mocking the Zero Trust Identity Service here, so this should be a failure") + .describedAs("We are not mocking the ZTIS service here so this should fail") .hasRootCauseInstanceOf(ServiceBindingAccessException.class); } diff --git a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClient5OAuth2TokenServiceTest.java b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClient5OAuth2TokenServiceTest.java index 115cb6310..dcb5fb00d 100644 --- a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClient5OAuth2TokenServiceTest.java +++ b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClient5OAuth2TokenServiceTest.java @@ -159,7 +159,7 @@ void testCreateHttpClientWithCertificateBasedClientIdentity() void testCreateHttpClientWithZtisClientIdentity() { final KeyStore keyStore = createEmptyKeyStore(); - final ClientIdentity ztisIdentity = new ZtisClientIdentity("ztis-client-id", keyStore); + final ClientIdentity ztisIdentity = new ZtisClientIdentity("ztis-client-id", () -> keyStore); // ZtisClientIdentity is certificate-based but doesn't implement certificate methods properly // This should fail with certificate validation error @@ -175,7 +175,7 @@ void testCreateHttpClientWithZtisClientIdentityAndExplicitKeyStore() { final KeyStore embeddedKeyStore = createEmptyKeyStore(); final KeyStore explicitKeyStore = createEmptyKeyStore(); - final ClientIdentity ztisIdentity = new ZtisClientIdentity("ztis-client-id", embeddedKeyStore); + final ClientIdentity ztisIdentity = new ZtisClientIdentity("ztis-client-id", () -> embeddedKeyStore); final CloseableHttpClient result = HttpClient5OAuth2TokenService.createHttpClient(ztisIdentity, explicitKeyStore); diff --git a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2IntegrationTest.java b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2IntegrationTest.java index 5c37e53ae..026a2e8ba 100644 --- a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2IntegrationTest.java +++ b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2IntegrationTest.java @@ -216,7 +216,7 @@ void testIasFlowWithZeroTrustAndSubscriberTenant() { final KeyStore ks = KeyStore.getInstance("JKS"); ks.load(null, null); - final ClientIdentity identity = new SecurityLibWorkarounds.ZtisClientIdentity("myClientId", ks); + final ClientIdentity identity = new SecurityLibWorkarounds.ZtisClientIdentity("myClientId", () -> ks); stubFor( post("/oauth2/token") diff --git a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceTest.java b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceTest.java index 9ddd50ac0..28007bbae 100644 --- a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceTest.java +++ b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceTest.java @@ -18,6 +18,7 @@ import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; import java.io.IOException; import java.net.URI; @@ -28,6 +29,7 @@ import java.time.Duration; import java.util.List; import java.util.Map; +import java.util.function.Supplier; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -401,15 +403,47 @@ void testZeroTrustClientIdentity() { final KeyStore ks = KeyStore.getInstance("JKS"); ks.load(null, null); - ClientIdentity identity = new ZtisClientIdentity("id", ks); + ClientIdentity identity = new ZtisClientIdentity("id", () -> ks); OAuth2Service service = OAuth2Service.builder().withTokenUri(SERVER_1.baseUrl()).withIdentity(identity).build(); final OAuth2TokenService result = service.getTokenService(null); assertThat(result).isSameAs(service.getTokenService(null)); - identity = new ZtisClientIdentity("other-id", ks); + identity = new ZtisClientIdentity("other-id", () -> ks); service = OAuth2Service.builder().withTokenUri(SERVER_1.baseUrl()).withIdentity(identity).build(); assertThat(result).isNotSameAs(service.getTokenService(null)); } + + @Test + @SneakyThrows + void testZeroTrustCertificateRotationCausesCacheMiss() + { + // we need to use actual KeyStores here because the code will build an HTTP Client and mocks don't suffice + final KeyStore ks1 = KeyStore.getInstance("JKS"); + final KeyStore ks2 = KeyStore.getInstance("JKS"); + ks1.load(null, null); + ks2.load(null, null); + + assertThat(ks1).describedAs("Sanity check: objects should be different").isNotSameAs(ks2).isNotEqualTo(ks2); + + @SuppressWarnings( "unchecked" ) + final Supplier mockZtis = mock(Supplier.class); + when(mockZtis.get()).thenReturn(ks1); + + final ZtisClientIdentity identity = new ZtisClientIdentity("id", mockZtis); + + final OAuth2Service service = + OAuth2Service.builder().withTokenUri(SERVER_1.baseUrl()).withIdentity(identity).build(); + + // Before rotation: same KeyStore → cache hit + final OAuth2TokenService tokenService1 = service.getTokenService(null); + assertThat(tokenService1).isSameAs(service.getTokenService(null)); + + when(mockZtis.get()).thenReturn(ks2); + + // After rotation: different KeyStore → cache miss → new token service with new certificate + final OAuth2TokenService tokenService2 = service.getTokenService(null); + assertThat(tokenService2).isNotSameAs(tokenService1); + } } diff --git a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/SecurityLibWorkaroundsTest.java b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/SecurityLibWorkaroundsTest.java index 0899cd4c0..a5693cfd2 100644 --- a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/SecurityLibWorkaroundsTest.java +++ b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/SecurityLibWorkaroundsTest.java @@ -1,13 +1,17 @@ package com.sap.cloud.sdk.cloudplatform.connectivity; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; import java.security.KeyStore; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; import org.junit.jupiter.api.Test; import com.sap.cloud.sdk.cloudplatform.connectivity.SecurityLibWorkarounds.ZtisClientIdentity; +import com.sap.cloud.sdk.cloudplatform.exception.CloudPlatformException; import com.sap.cloud.security.config.CredentialType; class SecurityLibWorkaroundsTest @@ -15,12 +19,58 @@ class SecurityLibWorkaroundsTest @Test void testZtisClientIdentityImplementsEquals() { - final ZtisClientIdentity sut = new ZtisClientIdentity("id", mock(KeyStore.class)); + // equals/hashCode are based only on id, not on the keyStoreSource (which rotates dynamically) + final ZtisClientIdentity sut = new ZtisClientIdentity("id", () -> mock(KeyStore.class)); - assertThat(sut).isEqualTo(new ZtisClientIdentity("id", mock(KeyStore.class))); - assertThat(sut).hasSameHashCodeAs(new ZtisClientIdentity("id", mock(KeyStore.class))); - assertThat(sut).isNotEqualTo(new ZtisClientIdentity("id2", mock(KeyStore.class))); - assertThat(sut).doesNotHaveSameHashCodeAs(new ZtisClientIdentity("id2", mock(KeyStore.class))); + assertThat(sut).isEqualTo(new ZtisClientIdentity("id", () -> mock(KeyStore.class))); + assertThat(sut).hasSameHashCodeAs(new ZtisClientIdentity("id", () -> mock(KeyStore.class))); + assertThat(sut).isNotEqualTo(new ZtisClientIdentity("id2", () -> mock(KeyStore.class))); + assertThat(sut).doesNotHaveSameHashCodeAs(new ZtisClientIdentity("id2", () -> mock(KeyStore.class))); + } + + @Test + void testZtisClientIdentityGetKeyStore() + { + final KeyStore expectedKeyStore = mock(KeyStore.class); + final ZtisClientIdentity sut = new ZtisClientIdentity("id", () -> expectedKeyStore); + + assertThat(sut.getKeyStore()).isSameAs(expectedKeyStore); + } + + @Test + void testZtisClientIdentityGetKeyStoreWrapsException() + { + final RuntimeException cause = new RuntimeException("ZTIS failure"); + final ZtisClientIdentity sut = new ZtisClientIdentity("id", () -> { + throw cause; + }); + + assertThatThrownBy(sut::getKeyStore) + .isInstanceOf(CloudPlatformException.class) + .hasMessageContaining("X509_ATTESTED") + .hasCause(cause); + } + + @Test + void testZtisClientIdentitySupplierCalledLazily() + { + final AtomicInteger callCount = new AtomicInteger(0); + final KeyStore ks = mock(KeyStore.class); + final Supplier supplier = () -> { + callCount.incrementAndGet(); + return ks; + }; + + // Construction must NOT call the supplier + final ZtisClientIdentity sut = new ZtisClientIdentity("id", supplier); + assertThat(callCount.get()).isZero(); + + // Each call to getKeyStore() invokes the supplier + sut.getKeyStore(); + assertThat(callCount.get()).isEqualTo(1); + + sut.getKeyStore(); + assertThat(callCount.get()).isEqualTo(2); } @Test diff --git a/release_notes.md b/release_notes.md index ddb8d96e2..c5bdfc685 100644 --- a/release_notes.md +++ b/release_notes.md @@ -16,7 +16,7 @@ ### 📈 Improvements -- +- Improved the support for the credential type `X509_ATTESTED`. `HttpDestination` objects created via `ServiceBindingDestinationLoader` no longer need to be re-created for the rotation of certificates to take effect. ### 🐛 Fixed Issues