Remove Xstream dependency from eureka-client #1074
Comments
|
I am voting for this as well because of future compatibility regarding reflection issue |
|
Plus JDK 11 issues: Which relates to this mess: x-stream/xstream#101 |
|
Is the solution as easy as overriding the setupConvertors method in the constructor of public JsonXStream() {
super(new JettisonMappedXmlDriver() {
private final NameCoder coder = initializeNameCoder();
protected NameCoder getNameCoder() {
return this.coder;
}
// add this as per x-stream/xstream#101?
@Override
protected void setupConverters() {
}
});Oh apparently it's not that easy: x-stream/xstream#101 (comment) |
@troshko111 Why was this issue closed? From what I see, the EurekaClient still relies on the xstream library, and it still has the illegal access issue (which is ignorable for now, but not futureproof). Will it not eventually have to be fixed? |
|
There was no activity for a while, I take it as nobody is interested in removing it? I glanced at the usage and it looks removable (does not leak into public contract much) but we'd need to preserve serialization compatibility, check the perf / GC pressure does not degrade, etc. We're open to discussing its removal if someone wants to take it on. I'll keep this open for some time then. |
|
@troshko111 I'm using JDK 11 right now. Isn't this going to be an issue if I want to upgrade to 13/14? |
|
Potentially, I did not investigate. Would appreciate if you reported back in case you do. |
|
Reviving this issue. There is a long thread on the xstream github page where it appears very evident that the maintainers do not want to make any effort to fix the illegal reflective access issue: x-stream/xstream#101 Therefore, I think it would be worthwile to migrate to another dependency that will work in future relases. |
|
We're open to making |
|
6 CVEs have been recently opened by OSS Fuzz guys against xstream and it doesn't seem likely they will be investigated soon.
@troshko111 would it be possible to proceed with making xstream an optional dependency? |
|
I have solved this for myself in my projects in a way such that I can exclude xstream from my dependencies without a classnotfound exception occurring by replacing /**
* customization of
* {@link com.netflix.discovery.shared.transport.jersey.EurekaJerseyClientImpl.EurekaJerseyClientBuilder.MyDefaultApacheHttpClient4Config}
* to work with our custom Jersey client
*/
class JsonOnlyApacheHttpClient4Config extends DefaultApacheHttpClient4Config {
public JsonOnlyApacheHttpClient4Config(EurekaClientConfig clientConfig) {
MonitoredConnectionManager cm = createDefaultSslCM();
getSingletons().add(new JsonOnlyJerseyProvider());
// Common properties to all clients
cm.setDefaultMaxPerRoute(clientConfig.getEurekaServerTotalConnectionsPerHost());
cm.setMaxTotal(clientConfig.getEurekaServerTotalConnections());
getProperties().put(ApacheHttpClient4Config.PROPERTY_CONNECTION_MANAGER, cm);
String fullUserAgentName = ("Java-EurekaClient") + "/v" + buildVersion();
getProperties().put(CoreProtocolPNames.USER_AGENT, fullUserAgentName);
// To pin a client to specific server in case redirect happens, we handle redirects directly
// (see DiscoveryClient.makeRemoteCall methods).
getProperties().put(ClientConfig.PROPERTY_FOLLOW_REDIRECTS, Boolean.FALSE);
getProperties().put(ClientPNames.HANDLE_REDIRECTS, Boolean.FALSE);
}
/**
* @see SchemeRegistryFactory#createDefault()
*/
private MonitoredConnectionManager createDefaultSslCM() {
final SchemeRegistry registry = new SchemeRegistry();
registry.register(
new Scheme("http", 80, PlainSocketFactory.getSocketFactory()));
registry.register(
new Scheme("https", 443, new SSLSocketFactoryAdapter(SSLConnectionSocketFactory.getSocketFactory())));
return new MonitoredConnectionManager("DiscoveryClient-HTTPClient", registry);
}
}which requires also an alternative to /**
* Customization of {@link com.netflix.discovery.provider.DiscoveryJerseyProvider}
* which removes the xml codecs and hardcodes the json codes to jackson in order to not reference xstream
*/
@Provider
@Produces({"application/json"})
@Consumes("*/*")
class JsonOnlyJerseyProvider implements MessageBodyWriter<Object>, MessageBodyReader<Object> {
private static final Logger LOGGER = LoggerFactory.getLogger(com.netflix.discovery.provider.DiscoveryJerseyProvider.class);
private final EncoderWrapper jsonEncoder;
private final DecoderWrapper jsonDecoder;
public JsonOnlyJerseyProvider() {
this.jsonEncoder = CodecWrappers.getEncoder(CodecWrappers.LegacyJacksonJson.class);
this.jsonDecoder = CodecWrappers.getDecoder(CodecWrappers.LegacyJacksonJson.class);
LOGGER.info("Using JSON encoding codec {}", this.jsonEncoder.codecName());
LOGGER.info("Using JSON decoding codec {}", this.jsonDecoder.codecName());
}
/**
* As content is cached, we expect both ends use UTF-8 always. If no content charset encoding is explicitly
* defined, UTF-8 is assumed as a default.
* As legacy clients may use ISO 8859-1 we accept it as well, although result may be unspecified if
* characters out of ASCII 0-127 range are used.
*/
private static boolean isSupportedCharset(MediaType mediaType) {
Map<String, String> parameters = mediaType.getParameters();
if (parameters == null || parameters.isEmpty()) {
return true;
}
String charset = parameters.get("charset");
return charset == null
|| "UTF-8".equalsIgnoreCase(charset)
|| "ISO-8859-1".equalsIgnoreCase(charset);
}
/**
* Checks for the {@link Serializer} annotation for the given class.
*
* @param entityType The class to be serialized/deserialized.
* @return true if the annotation is present, false otherwise.
*/
private static boolean isSupportedEntity(Class<?> entityType) {
try {
Annotation annotation = entityType.getAnnotation(Serializer.class);
if (annotation != null) {
return true;
}
} catch (Throwable th) {
LOGGER.warn("Exception in checking for annotations", th);
}
return false;
}
private static Response createErrorReply(int status, Throwable cause, MediaType mediaType) {
StringBuilder sb = new StringBuilder(cause.getClass().getName());
if (cause.getMessage() != null) {
sb.append(": ").append(cause.getMessage());
}
return createErrorReply(status, sb.toString(), mediaType);
}
private static Response createErrorReply(int status, String errorMessage, MediaType mediaType) {
String message;
if (MediaType.APPLICATION_JSON_TYPE.equals(mediaType)) {
message = "{\"error\": \"" + errorMessage + "\"}";
} else {
message = "<error><message>" + errorMessage + "</message></error>";
}
return Response.status(status).entity(message).type(mediaType).build();
}
private static void closeInputOnError(InputStream inputStream) {
if (inputStream != null) {
LOGGER.error("Unexpected error occurred during de-serialization of discovery data, done connection cleanup");
try {
inputStream.close();
} catch (IOException e) {
LOGGER.debug("Cannot close input", e);
}
}
}
@Override
public boolean isReadable(Class serializableClass, Type type, Annotation[] annotations, MediaType mediaType) {
return isSupportedMediaType(mediaType) && isSupportedCharset(mediaType) && isSupportedEntity(serializableClass);
}
@Override
public Object readFrom(
Class serializableClass, Type type,
Annotation[] annotations, MediaType mediaType,
MultivaluedMap headers, InputStream inputStream) throws IOException {
DecoderWrapper decoder;
if (MediaType.MEDIA_TYPE_WILDCARD.equals(mediaType.getSubtype())) {
throw new RuntimeException("xml not supported");
} else if ("json".equalsIgnoreCase(mediaType.getSubtype())) {
decoder = jsonDecoder;
} else {
decoder = jsonDecoder; // default
}
try {
return decoder.decode(inputStream, serializableClass);
} catch (Throwable e) {
if (e instanceof Error) { // See issue: https://github.com/Netflix/eureka/issues/72 on why we catch Error here.
closeInputOnError(inputStream);
throw new WebApplicationException(e, createErrorReply(500, e, mediaType));
}
LOGGER.debug("Cannot parse request body", e);
throw new WebApplicationException(e, createErrorReply(400, "cannot parse request body", mediaType));
}
}
@Override
public long getSize(
Object serializableObject,
Class serializableClass,
Type type,
Annotation[] annotations,
MediaType mediaType) {
return -1;
}
@Override
public boolean isWriteable(Class serializableClass, Type type, Annotation[] annotations, MediaType mediaType) {
return isSupportedMediaType(mediaType) && isSupportedEntity(serializableClass);
}
@Override
public void writeTo(
Object serializableObject, Class serializableClass,
Type type, Annotation[] annotations, MediaType mediaType,
MultivaluedMap headers, OutputStream outputStream) throws IOException, WebApplicationException {
EncoderWrapper encoder = "json".equalsIgnoreCase(mediaType.getSubtype()) ? jsonEncoder : null;
// XML codec may not be available
if (encoder == null) {
throw new WebApplicationException(createErrorReply(400, "No codec available to serialize content type " + mediaType, mediaType));
}
encoder.encode(serializableObject, outputStream);
}
private boolean isSupportedMediaType(MediaType mediaType) {
return MediaType.APPLICATION_JSON_TYPE.isCompatible(mediaType);
}
}and then building my client as such: public static DiscoveryClient create(
EurekaClientConfig clientConfig,
ApplicationInfoManager applicationInfoManager) {
val args = new DiscoveryClient.DiscoveryClientOptionalArgs();
ClientConfig httpClientConfig = new JsonOnlyApacheHttpClient4Config(clientConfig);
args.setEurekaJerseyClient(new EurekaJerseyClientImpl(
clientConfig.getEurekaServerConnectTimeoutSeconds() * 1000,
clientConfig.getEurekaServerReadTimeoutSeconds() * 1000,
clientConfig.getEurekaConnectionIdleTimeoutSeconds() * 1000, httpClientConfig));
return new DiscoveryClient(applicationInfoManager, clientConfig, args);
}if it is decided to make similar changes in upstream, Id be happy to open a PR |
I would like to remove the xstream dependency from the eureka-client, but it is used in DiscoveryJerseyProvider.
eureka/eureka-client/src/main/java/com/netflix/discovery/provider/DiscoveryJerseyProvider.java
Lines 77 to 81 in 8009355
I am not exactly clear why the client needs to have this provider?
eureka/eureka-client/src/main/java/com/netflix/discovery/shared/transport/jersey/EurekaJerseyClientImpl.java
Lines 202 to 203 in 6efea5d
If the client really needs the provider, could the eureka-client have a different provider for the client that only uses the Json codecs?
FYI: Eureka-client 1.8.7+ added the xstream security manager, which forces the use of Xstream 1.4.10. Unfortunately, our large code base requires 1.4.3 and it is not a simple effort to uplift to Xstream 1.4.10 because of non-passive changes.
The text was updated successfully, but these errors were encountered: