better handling of HTTP and JSON errors on network fetches, added http-forcing behavior for webfinger client and sector URL service

pull/820/merge
Justin Richer 2015-06-23 22:21:18 -04:00
parent 9ae92b983a
commit f4a1b27e2e
6 changed files with 84 additions and 32 deletions

View File

@ -29,7 +29,6 @@ import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import com.google.common.collect.Iterables;
import org.apache.http.client.HttpClient;
import org.apache.http.client.config.RequestConfig;
import org.apache.http.impl.client.HttpClientBuilder;
@ -57,10 +56,11 @@ import org.springframework.security.web.authentication.AbstractAuthenticationPro
import org.springframework.security.web.authentication.AuthenticationSuccessHandler;
import org.springframework.util.LinkedMultiValueMap;
import org.springframework.util.MultiValueMap;
import org.springframework.web.client.HttpClientErrorException;
import org.springframework.web.client.RestClientException;
import org.springframework.web.client.RestTemplate;
import com.google.common.base.Strings;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
@ -407,15 +407,13 @@ public class OIDCAuthenticationFilter extends AbstractAuthenticationProcessingFi
try {
jsonString = restTemplate.postForObject(serverConfig.getTokenEndpointUri(), form, String.class);
} catch (HttpClientErrorException httpClientErrorException) {
} catch (RestClientException e) {
// Handle error
logger.error("Token Endpoint error response: "
+ httpClientErrorException.getStatusText() + " : "
+ httpClientErrorException.getMessage());
logger.error("Token Endpoint error response: " + e.getMessage());
throw new AuthenticationServiceException("Unable to obtain Access Token: " + httpClientErrorException.getMessage());
throw new AuthenticationServiceException("Unable to obtain Access Token: " + e.getMessage());
}
logger.debug("from TokenEndpoint jsonString = " + jsonString);

View File

@ -114,11 +114,8 @@ public class DynamicServerConfigurationService implements ServerConfigurationSer
}
return servers.get(issuer);
} catch (UncheckedExecutionException ue) {
logger.warn("Couldn't load configuration for " + issuer, ue);
return null;
} catch (ExecutionException e) {
logger.warn("Couldn't load configuration for " + issuer, e);
} catch (UncheckedExecutionException | ExecutionException e) {
logger.warn("Couldn't load configuration for " + issuer + ": " + e);
return null;
}

View File

@ -36,6 +36,38 @@ import com.google.common.collect.Sets;
*/
public class HybridIssuerService implements IssuerService {
/**
* @return
* @see org.mitre.openid.connect.client.service.impl.ThirdPartyIssuerService#getAccountChooserUrl()
*/
public String getAccountChooserUrl() {
return thirdPartyIssuerService.getAccountChooserUrl();
}
/**
* @param accountChooserUrl
* @see org.mitre.openid.connect.client.service.impl.ThirdPartyIssuerService#setAccountChooserUrl(java.lang.String)
*/
public void setAccountChooserUrl(String accountChooserUrl) {
thirdPartyIssuerService.setAccountChooserUrl(accountChooserUrl);
}
/**
* @return
* @see org.mitre.openid.connect.client.service.impl.WebfingerIssuerService#isForceHttps()
*/
public boolean isForceHttps() {
return webfingerIssuerService.isForceHttps();
}
/**
* @param forceHttps
* @see org.mitre.openid.connect.client.service.impl.WebfingerIssuerService#setForceHttps(boolean)
*/
public void setForceHttps(boolean forceHttps) {
webfingerIssuerService.setForceHttps(forceHttps);
}
private ThirdPartyIssuerService thirdPartyIssuerService = new ThirdPartyIssuerService();
private WebfingerIssuerService webfingerIssuerService = new WebfingerIssuerService();

View File

@ -77,6 +77,11 @@ public class WebfingerIssuerService implements IssuerService {
* URL of the page to forward to if no identifier is given.
*/
private String loginPageUrl;
/**
* Strict enfocement of "https"
*/
private boolean forceHttps = true;
public WebfingerIssuerService() {
issuers = CacheBuilder.newBuilder().build(new WebfingerIssuerFetcher());
@ -102,7 +107,7 @@ public class WebfingerIssuerService implements IssuerService {
return new IssuerServiceResponse(issuer, identifier, null);
} catch (UncheckedExecutionException | ExecutionException e) {
logger.warn("Issue fetching issuer for user input: " + identifier, e.getMessage());
logger.warn("Issue fetching issuer for user input: " + identifier + ": " + e.getMessage());
return null;
}
@ -169,6 +174,20 @@ public class WebfingerIssuerService implements IssuerService {
this.blacklist = blacklist;
}
/**
* @return the forceHttps
*/
public boolean isForceHttps() {
return forceHttps;
}
/**
* @param forceHttps the forceHttps to set
*/
public void setForceHttps(boolean forceHttps) {
this.forceHttps = forceHttps;
}
/**
* @author jricher
*
@ -188,9 +207,16 @@ public class WebfingerIssuerService implements IssuerService {
// preserving http scheme is strictly for demo system use only.
String scheme = key.getScheme();
if (!Strings.isNullOrEmpty(scheme) && scheme.equals("http")) {
scheme = "http://"; // add on colon and slashes.
logger.warn("Webfinger endpoint MUST use the https URI scheme.");
if (!Strings.isNullOrEmpty(scheme)) {
if (scheme.equals("http")) {
if (forceHttps) {
throw new IllegalArgumentException("Scheme must start with htps");
} else {
logger.warn("Webfinger endpoint MUST use the https URI scheme, overriding by configuration");
scheme = "http://"; // add on colon and slashes.
}
}
} else {
scheme = "https://";
}

View File

@ -82,11 +82,8 @@ public class JWKSetCacheService {
public JWTSigningAndValidationService getValidator(String jwksUri) {
try {
return validators.get(jwksUri);
} catch (UncheckedExecutionException ue) {
logger.warn("Couldn't load JWK Set from " + jwksUri, ue);
return null;
} catch (ExecutionException e) {
logger.warn("Couldn't load JWK Set from " + jwksUri, e);
} catch (UncheckedExecutionException | ExecutionException e) {
logger.warn("Couldn't load JWK Set from " + jwksUri + ": " + e.getMessage());
return null;
}
}
@ -94,11 +91,8 @@ public class JWKSetCacheService {
public JWTEncryptionAndDecryptionService getEncrypter(String jwksUri) {
try {
return encrypters.get(jwksUri);
} catch (UncheckedExecutionException ue) {
logger.warn("Couldn't load JWK Set from " + jwksUri, ue);
return null;
} catch (ExecutionException e) {
logger.warn("Couldn't load JWK Set from " + jwksUri, e);
} catch (UncheckedExecutionException | ExecutionException e) {
logger.warn("Couldn't load JWK Set from " + jwksUri + ": " + e.getMessage());
return null;
}
}
@ -117,7 +111,6 @@ public class JWKSetCacheService {
*/
@Override
public JWTSigningAndValidationService load(String key) throws Exception {
String jsonString = restTemplate.getForObject(key, String.class);
JWKSet jwkSet = JWKSet.parse(jsonString);
@ -126,7 +119,6 @@ public class JWKSetCacheService {
JWTSigningAndValidationService service = new DefaultJWTSigningAndValidationService(keyStore);
return service;
}
}

View File

@ -36,6 +36,7 @@ import org.mitre.oauth2.repository.OAuth2ClientRepository;
import org.mitre.oauth2.repository.OAuth2TokenRepository;
import org.mitre.oauth2.service.ClientDetailsEntityService;
import org.mitre.oauth2.service.SystemScopeService;
import org.mitre.openid.connect.config.ConfigurationPropertiesBean;
import org.mitre.openid.connect.model.WhitelistedSite;
import org.mitre.openid.connect.service.ApprovedSiteService;
import org.mitre.openid.connect.service.BlacklistedSiteService;
@ -54,6 +55,7 @@ import com.google.common.base.Strings;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.util.concurrent.UncheckedExecutionException;
import com.google.gson.JsonElement;
import com.google.gson.JsonParser;
@ -85,6 +87,9 @@ public class DefaultOAuth2ClientDetailsEntityService implements ClientDetailsEnt
@Autowired
private StatsService statsService;
@Autowired
private ConfigurationPropertiesBean config;
// map of sector URI -> list of redirect URIs
private LoadingCache<String, List<String>> sectorRedirects = CacheBuilder.newBuilder()
@ -167,8 +172,8 @@ public class DefaultOAuth2ClientDetailsEntityService implements ClientDetailsEnt
}
}
} catch (ExecutionException e) {
throw new IllegalArgumentException("Unable to load sector identifier URI: " + client.getSectorIdentifierUri());
} catch (UncheckedExecutionException | ExecutionException e) {
throw new IllegalArgumentException("Unable to load sector identifier URI " + client.getSectorIdentifierUri() + ": " + e.getMessage());
}
}
}
@ -321,7 +326,9 @@ public class DefaultOAuth2ClientDetailsEntityService implements ClientDetailsEnt
public List<String> load(String key) throws Exception {
if (!key.startsWith("https")) {
// TODO: this should optionally throw an error (#506)
if (config.isForceHttps()) {
throw new IllegalArgumentException("Sector identifier must start with https: " + key);
}
logger.error("Sector identifier doesn't start with https, loading anyway...");
}
@ -339,7 +346,7 @@ public class DefaultOAuth2ClientDetailsEntityService implements ClientDetailsEnt
return redirectUris;
} else {
return null;
throw new IllegalArgumentException("JSON Format Error");
}
}