Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 0 additions & 4 deletions cloudplatform/connectivity-oauth/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,6 @@
<groupId>org.apache.httpcomponents.core5</groupId>
<artifactId>httpcore5</artifactId>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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
{
Expand All @@ -22,44 +19,36 @@ 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<KeyStore> keyStoreSource;

@Override
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();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<KeyStore> 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);
}
}
Original file line number Diff line number Diff line change
@@ -1,26 +1,76 @@
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
{
@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<KeyStore> 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
Expand Down
2 changes: 1 addition & 1 deletion release_notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Loading