Skip to content

Commit

Permalink
feat(cf): Cloudfoundry API endpoint server certificate now validated …
Browse files Browse the repository at this point in the history 
…by default (#3737)

* Highlighting and added more logging for a situation where the oAuth2 token endpoint is misconfigured and the token is refreshed. A user wouldn't have much insight as why the request didn't fail but returned an unexpected empty list back.
Could be fixed gracefully by using okhttp3 authenticator or maybe using the Cloud Foundry Java Client.

The HTTPS endpoint for a CloudFoundry account's apiHost field now has its server certificate validated by default.

BREAKING CHANGE: The previous default behaviour used to be skipping the certificate validation. The change might break environments where the certificate or the certificate of its issuers are not in the JVM's trust store.
Skipping the certificate validation for CloudFoundry accounts can be enabled with this `hal` command: `hal config provider cloudfoundry account edit ACCOUNT_NAME --skip-ssl-validation=true`
  • Loading branch information
Pierre Delagrave authored and Jon Schneider committed Jun 3, 2019
1 parent 60ee344 commit bf873b9
Show file tree
Hide file tree
Showing 14 changed files with 76 additions and 69 deletions.
2 changes: 2 additions & 0 deletions ...n/java/com/netflix/spinnaker/clouddriver/cloudfoundry/client/CloudFoundryClientUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ static <T> Optional<T> safelyCall(RetrofitCallable<T> r) throws CloudFoundryApiE
return Optional.of(r.call());
} catch (RetrofitError retrofitError) {
if (retrofitError.getResponse() != null && retrofitError.getResponse().getStatus() == 404) {
// FIXME: The oauthInterceptor could use a misconfigured endpoint and return 404 and
// this code would mask the issue.
return Optional.empty();
} else {
ErrorDescription errorDescription =
Expand Down
79 changes: 48 additions & 31 deletions ...in/java/com/netflix/spinnaker/clouddriver/cloudfoundry/client/HttpCloudFoundryClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,15 @@
import javax.net.ssl.SSLContext;
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;
import lombok.extern.slf4j.Slf4j;
import okio.Buffer;
import okio.BufferedSource;
import retrofit.RequestInterceptor;
import retrofit.RestAdapter;
import retrofit.client.OkClient;
import retrofit.converter.JacksonConverter;

@Slf4j
public class HttpCloudFoundryClient implements CloudFoundryClient {
private final String apiHost;
private final String user;
Expand Down Expand Up @@ -149,31 +151,15 @@ public HttpCloudFoundryClient(
String metricsUri,
String apiHost,
String user,
String password) {
String password,
boolean skipSslValidation) {
this.apiHost = apiHost;
this.user = user;
this.password = password;

this.okHttpClient = new OkHttpClient();
okHttpClient.interceptors().add(this::createRetryInterceptor);

okHttpClient.setHostnameVerifier((s, sslSession) -> true);

TrustManager[] trustAllCerts =
new TrustManager[] {
new X509TrustManager() {
@Override
public void checkClientTrusted(X509Certificate[] x509Certificates, String s) {}
this.okHttpClient = createHttpClient(skipSslValidation);

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String s) {}

@Override
public X509Certificate[] getAcceptedIssuers() {
return new X509Certificate[0];
}
}
};
okHttpClient.interceptors().add(this::createRetryInterceptor);

ObjectMapper mapper = new ObjectMapper();
mapper.setPropertyNamingStrategy(PropertyNamingStrategy.SNAKE_CASE);
Expand All @@ -183,16 +169,6 @@ public X509Certificate[] getAcceptedIssuers() {

this.jacksonConverter = new JacksonConverter(mapper);

SSLContext sslContext;
try {
sslContext = SSLContext.getInstance("SSL");
sslContext.init(null, trustAllCerts, new SecureRandom());
} catch (KeyManagementException | NoSuchAlgorithmException e) {
throw new RuntimeException(e);
}

okHttpClient.setSslSocketFactory(sslContext.getSocketFactory());

this.uaaService =
new RestAdapter.Builder()
.setEndpoint("https://" + apiHost.replaceAll("^api\\.", "login."))
Expand All @@ -218,6 +194,42 @@ public X509Certificate[] getAcceptedIssuers() {
this.serviceKeys = new ServiceKeys(createService(ServiceKeyService.class), spaces);
}

private static OkHttpClient createHttpClient(boolean skipSslValidation) {
OkHttpClient client = new OkHttpClient();

if (skipSslValidation) {
client.setHostnameVerifier((s, sslSession) -> true);

TrustManager[] trustAllCerts =
new TrustManager[] {
new X509TrustManager() {
@Override
public void checkClientTrusted(X509Certificate[] x509Certificates, String s) {}

@Override
public void checkServerTrusted(X509Certificate[] x509Certificates, String s) {}

@Override
public X509Certificate[] getAcceptedIssuers() {
return new X509Certificate[0];
}
}
};

SSLContext sslContext;
try {
sslContext = SSLContext.getInstance("SSL");
sslContext.init(null, trustAllCerts, new SecureRandom());
} catch (KeyManagementException | NoSuchAlgorithmException e) {
throw new RuntimeException(e);
}

client.setSslSocketFactory(sslContext.getSocketFactory());
}

return client;
}

private void refreshTokenIfNecessary() {
long currentExpiration = tokenExpirationNs.get();
long now = System.nanoTime();
Expand All @@ -228,7 +240,12 @@ private void refreshTokenIfNecessary() {
}

private void refreshToken() {
token = uaaService.passwordToken("password", user, password, "cf", "");
try {
token = uaaService.passwordToken("password", user, password, "cf", "");
} catch (Exception e) {
log.warn("Failed to obtain a token", e);
throw e;
}
tokenExpirationNs.addAndGet(Duration.ofSeconds(token.getExpiresIn()).toNanos());
}

Expand Down
1 change: 1 addition & 0 deletions ...etflix/spinnaker/clouddriver/cloudfoundry/config/CloudFoundryConfigurationProperties.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,6 @@ public static class ManagedAccount {
private String user;
private String password;
private String environment;
private boolean skipSslValidation;
}
}
37 changes: 11 additions & 26 deletions ...java/com/netflix/spinnaker/clouddriver/cloudfoundry/security/CloudFoundryCredentials.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,11 @@

package com.netflix.spinnaker.clouddriver.cloudfoundry.security;

import static java.util.Collections.emptyList;
import static java.util.Collections.singletonMap;
import static java.util.stream.Collectors.toList;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.netflix.spinnaker.clouddriver.cloudfoundry.client.CloudFoundryApiException;
import com.netflix.spinnaker.clouddriver.cloudfoundry.client.CloudFoundryClient;
import com.netflix.spinnaker.clouddriver.cloudfoundry.client.HttpCloudFoundryClient;
import com.netflix.spinnaker.clouddriver.security.AccountCredentials;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.*;
import javax.annotation.Nullable;
import lombok.Getter;
import lombok.extern.slf4j.Slf4j;
Expand All @@ -54,6 +44,7 @@ public class CloudFoundryCredentials implements AccountCredentials<CloudFoundryC
private final String cloudProvider = "cloudfoundry";

@Deprecated private final List<String> requiredGroupMembership = Collections.emptyList();
private final boolean skipSslValidation;

private CloudFoundryClient credentials;

Expand All @@ -64,20 +55,23 @@ public CloudFoundryCredentials(
String apiHost,
String userName,
String password,
String environment) {
String environment,
boolean skipSslValidation) {
this.name = name;
this.appsManagerUri = appsManagerUri;
this.metricsUri = metricsUri;
this.apiHost = apiHost;
this.userName = userName;
this.password = password;
this.environment = Optional.ofNullable(environment).orElse("dev");
this.skipSslValidation = skipSslValidation;
}

public CloudFoundryClient getCredentials() {
if (this.credentials == null) {
this.credentials =
new HttpCloudFoundryClient(name, appsManagerUri, metricsUri, apiHost, userName, password);
new HttpCloudFoundryClient(
name, appsManagerUri, metricsUri, apiHost, userName, password, skipSslValidation);
}
return credentials;
}
Expand All @@ -86,17 +80,6 @@ public CloudFoundryClient getClient() {
return getCredentials();
}

public Collection<Map<String, String>> getRegions() {
try {
return getCredentials().getSpaces().all().stream()
.map(space -> singletonMap("name", space.getRegion()))
.collect(toList());
} catch (CloudFoundryApiException e) {
log.warn("Unable to determine regions for Cloud Foundry account " + name, e);
return emptyList();
}
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
Expand All @@ -109,11 +92,13 @@ public boolean equals(Object o) {
&& Objects.equals(metricsUri, that.metricsUri)
&& Objects.equals(userName, that.userName)
&& Objects.equals(password, that.password)
&& Objects.equals(environment, that.environment);
&& Objects.equals(environment, that.environment)
&& Objects.equals(skipSslValidation, that.skipSslValidation);
}

@Override
public int hashCode() {
return Objects.hash(name, appsManagerUri, metricsUri, userName, password, environment);
return Objects.hash(
name, appsManagerUri, metricsUri, userName, password, environment, skipSslValidation);
}
}
3 changes: 2 additions & 1 deletion ...flix/spinnaker/clouddriver/cloudfoundry/security/CloudFoundryCredentialsSynchronizer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ private List<String> synchronizeRepository(
managedAccount.getApi(),
managedAccount.getUser(),
managedAccount.getPassword(),
managedAccount.getEnvironment());
managedAccount.getEnvironment(),
managedAccount.isSkipSslValidation());

AccountCredentials existingCredentials =
accountCredentialsRepository.getOne(credentials.getName());
Expand Down
3 changes: 2 additions & 1 deletion ...src/test/java/com/netflix/spinnaker/clouddriver/cloudfoundry/client/ApplicationsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ void errorHandling() {
"some-metrics-uri",
"api.run.pivotal.io",
"baduser",
"badpassword");
"badpassword",
false);

assertThatThrownBy(() -> client.getApplications().all())
.isInstanceOf(CloudFoundryApiException.class);
Expand Down
6 changes: 3 additions & 3 deletions ...ava/com/netflix/spinnaker/clouddriver/cloudfoundry/client/HttpCloudFoundryClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ void createRetryInterceptorShouldRetryOnInternalServerErrorsThenTimeOut() {

HttpCloudFoundryClient cloudFoundryClient =
new HttpCloudFoundryClient(
"account", "appsManUri", "metricsUri", "host", "user", "password");
"account", "appsManUri", "metricsUri", "host", "user", "password", false);
Response response = cloudFoundryClient.createRetryInterceptor(chain);

try {
Expand Down Expand Up @@ -82,7 +82,7 @@ void createRetryInterceptorShouldNotRefreshTokenOnBadCredentials() {

HttpCloudFoundryClient cloudFoundryClient =
new HttpCloudFoundryClient(
"account", "appsManUri", "metricsUri", "host", "user", "password");
"account", "appsManUri", "metricsUri", "host", "user", "password", false);
Response response = cloudFoundryClient.createRetryInterceptor(chain);

try {
Expand Down Expand Up @@ -111,7 +111,7 @@ void createRetryInterceptorShouldReturnOnEverythingElse() {

HttpCloudFoundryClient cloudFoundryClient =
new HttpCloudFoundryClient(
"account", "appsManUri", "metricsUri", "host", "user", "password");
"account", "appsManUri", "metricsUri", "host", "user", "password", false);
Response response = cloudFoundryClient.createRetryInterceptor(chain);

try {
Expand Down
2 changes: 1 addition & 1 deletion ...ver/cloudfoundry/deploy/converters/AbstractLoadBalancersAtomicOperationConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class AbstractLoadBalancersAtomicOperationConverterTest {
}

private final CloudFoundryCredentials cloudFoundryCredentials =
new CloudFoundryCredentials("test", "", "", "", "", "", "") {
new CloudFoundryCredentials("test", "", "", "", "", "", "", false) {
public CloudFoundryClient getClient() {
return cloudFoundryClient;
}
Expand Down
2 changes: 1 addition & 1 deletion ...udfoundry/deploy/converters/CreateCloudFoundryServiceKeyAtomicOperationConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class CreateCloudFoundryServiceKeyAtomicOperationConverterTest {
}

private final CloudFoundryCredentials cloudFoundryCredentials =
new CloudFoundryCredentials("my-account", "", "", "", "", "", "") {
new CloudFoundryCredentials("my-account", "", "", "", "", "", "", false) {
public CloudFoundryClient getClient() {
return cloudFoundryClient;
}
Expand Down
2 changes: 1 addition & 1 deletion ...udfoundry/deploy/converters/DeleteCloudFoundryServiceKeyAtomicOperationConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class DeleteCloudFoundryServiceKeyAtomicOperationConverterTest {
}

private final CloudFoundryCredentials cloudFoundryCredentials =
new CloudFoundryCredentials("my-account", "", "", "", "", "", "") {
new CloudFoundryCredentials("my-account", "", "", "", "", "", "", false) {
public CloudFoundryClient getClient() {
return cloudFoundryClient;
}
Expand Down
2 changes: 1 addition & 1 deletion ...dfoundry/deploy/converters/DeployCloudFoundryServerGroupAtomicOperationConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ private static CloudFoundryCredentials createCredentials(String name) {
.build()));
}

return new CloudFoundryCredentials(name, "", "", "", "", "", "") {
return new CloudFoundryCredentials(name, "", "", "", "", "", "", false) {
public CloudFoundryClient getClient() {
return cloudFoundryClient;
}
Expand Down
2 changes: 1 addition & 1 deletion ...cloudfoundry/deploy/converters/DeployCloudFoundryServiceAtomicOperationConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class DeployCloudFoundryServiceAtomicOperationConverterTest {
}

private final CloudFoundryCredentials cloudFoundryCredentials =
new CloudFoundryCredentials("test", "", "", "", "", "", "") {
new CloudFoundryCredentials("test", "", "", "", "", "", "", false) {
public CloudFoundryClient getClient() {
return cloudFoundryClient;
}
Expand Down
2 changes: 1 addition & 1 deletion ...udfoundry/deploy/converters/ScaleCloudFoundryServerGroupAtomicOperationConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class ScaleCloudFoundryServerGroupAtomicOperationConverterTest {
}

private final CloudFoundryCredentials cloudFoundryCredentials =
new CloudFoundryCredentials("test", "", "", "", "", "", "") {
new CloudFoundryCredentials("test", "", "", "", "", "", "", false) {
public CloudFoundryClient getClient() {
return cloudFoundryClient;
}
Expand Down
2 changes: 1 addition & 1 deletion .../spinnaker/clouddriver/cloudfoundry/security/CloudFoundryCredentialsSynchronizerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ private CloudFoundryConfigurationProperties.ManagedAccount createAccount(String

private CloudFoundryCredentials createCredentials(String name) {
return new CloudFoundryCredentials(
name, null, null, "api." + name, "user-" + name, "pwd-" + name, null);
name, null, null, "api." + name, "user-" + name, "pwd-" + name, null, false);
}

private void loadProviderFromRepository() {
Expand Down

0 comments on commit bf873b9

Please sign in to comment.