-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not get into a 500 state when a bad app registers #108
Conversation
eureka-pull-requests #92 SUCCESS |
Previously, the DataCenterInfo was not effectively "validated" until read time, which meant that a malicious (or simply badly coded) app could take the Eureka cluster offline by specifying a dataCenterName that wasn't one of Amazon, MyOwn, or Netflix. Move the validation code to write time by doing the conversion to DataCenterInfo.Name as part of the deserialization process.
@@ -488,11 +488,13 @@ public Object unmarshal(HierarchicalStreamReader reader, | |||
dataCenterName)) { | |||
info = new AmazonInfo(); | |||
} else { | |||
final DataCenterInfo.Name name = | |||
DataCenterInfo.Name.valueOf(dataCenterName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An illegal name would throw an IllegalArgumentException here which would break the de-serialization.
Can we catch IllegalArgumentException (illegal name) and default to Name.MyOwn just to be more tolerable to name issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like converting a bogus value to an arbitrary value; Name.Amazon
is just as likely to be correct as Name.MyOwn
(and probably more so, given that part of Eureka's sweet-spot is in AWS deployments).
As far as I can tell, there are only two places we deserialize: in the server, when an app registers; and in the client, when we get data from the server. In both cases, I think the actual best behavior would be to drop the bad registration on the floor: bad clients can't register, and if one somehow succeeds (talking to an old Eureka server, perhaps), we stop it replicating.
This implies to me that throwing a new exception, and catching it at the call sites to do something sane, is actually the best option, but it's also a much more invasive change. As-is, this code ends up preventing bad clients from registering in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I understand your point of view.
eureka-pull-requests #93 SUCCESS |
Do not get into a 500 state when a bad app registers
Previously, the DataCenterInfo was not effectively "validated" until read
time, which meant that a malicious (or simply badly coded) app could take the
Eureka cluster offline by specifying a dataCenterName that wasn't one of
Amazon, MyOwn, or Netflix. Move the validation code to write time by doing
the conversion to DataCenterInfo.Name as part of the deserialization process.