Refactor the way foreman knows about smart-proxy errors

[ik5]

Hello,

I wish to refactor this: https://github.com/theforeman/foreman/blob/develop/lib/proxy_api/resource.rb#L54 to actually report the issue returned by smart-proxy (it’s part of the body), and raise an exception about it.

At the moment, if there is a non 200 OK status code, then a general error is reported, but it misses the actual issue that happened, and reported by smart-proxy.

In my case, it is to report that the subnet given by the user is not configured either on foreman or on the smart-proxy dhcp plugin, but I think it can help in all of the smart-proxy usage, when something happen over there.

What do you think?

[lzap]

Can you give us an example of current behavior and expected one? It actually works today, the body is part of the exception message and it contains the root cause. Indeed it’s clunky and needs improvement, I’d like to see how the message looks today and your proposal.

Keep in mind that our error handling code is broken - it returns JSON content type but the response is actually a plain text. See log_halt method in proxy, not sure if we want to break this behavior @Dmitri_Dolguikh? If you plan to change that on proxy side, that could be a problem. There are people using proxy API directly, it’s not just Foreman.

[Dmitri_Dolguikh]

it returns JSON content type but the response is actually a plain text

This can become a problem if response body contains more than an error message. I’m also curious to learn about changes Ido is proposing: I would suggest looking into splitting 400 and 500 errors into more specific ones to help with error differentiation (newer modules and api in Smart-Proxy already do this).

[ik5]

First of all to fix the format in proxy, it should be JSON as well, with constant key of “error” for example.
But the actual fix is for the way the response is handled.

At the moment there isn’t really any content validation. There is only an attempt to parse it as JSON if there is a response, and that’s it.

If there is an error message but as JSON format, it will be reported as a valid response, but it is not the actual case.
The content even if passed the JSON parser should be checked for, and be “understood” before the actual usage of it, because otherwise it raises a security issue for malicious content.

Further more, the error message is only written to the log, but not reported to caller of the method, and I wish to fix that as well using throw (rather then raise).

[Dmitri_Dolguikh]

First of all to fix the format in proxy, it should be JSON as well, with constant key of “error” for example.

I’m not convinced we need to encode error messages in JSON, as the response contains just that, no other information. Http status acts as an error code.

because otherwise it raises a security issue for malicious content.

The assumption is that in production environments ssl is used for foreman/client - smart-proxy communications, which eliminates concerns about man-in-the-middle attacks.

the error message is only written to the log, but not reported to caller of the method

Errors are returned to the caller.

I wish to fix that as well using throw (rather then raise).

Not sure what you mean by this.

[ik5]

First of all to fix the format in proxy, it should be JSON as well, with constant key of “error” for example.

I’m not convinced we need to encode error messages in JSON, as the response contains just that, no other information. Http status acts as an error code.

Using HTTP status code is good, but in practice, as I understand the code, it is not used to know if there is an error, it is used to return false. The exception handler is for JSON parsing.
Having said that, using two different types of content type without changing the header (that is you are reporting application/json) even on an error, means that it should be expected a JSON.

I strongly believe in “Least expected surprise” methodology, so if the content is usually sent using JSON, it should always be JSON.

BTW, body payload of "Opps something went wrong" without curly braces is JSON and will be parsed by the engine as JSON, but I would like to see an object: { "error": "Opps something went wrong" }, because first of all it is returning like every other content, and secondly, helps us to know that there is an error, without guessing or thinking that something else went wrong.

because otherwise it raises a security issue for malicious content.

The assumption is that in production environments ssl is used for foreman/client - smart-proxy communications, which eliminates concerns about man-in-the-middle attacks.

TLS is not a valid “defence” for avoiding malicious payload, because malicious does not always means an outside attacker, but rather an invalid content, and bad escaping that can create an issue for us.

Validating any input is a best practice, and can help us avoid any pitiful with the payload.

Please don’t get me wrong, TLS is a strong security factor, but it should not be the only security mechanism IMO, but rather one in a big chain of mechanisms that are used.

the error message is only written to the log, but not reported to caller of the method

Errors are returned to the caller.

Can you please show me where on the code I happens, because all I see, is that it returns false, what am I missing?

I wish to fix that as well using throw (rather then raise).

Not sure what you mean by this.

In ruby raise means that I created an exception, while throw means that I pass an exception along.
So in other words, raise create a new call stack, while throw sends the existed exception stack.

In this case I wish to create the effect of “I didn’t created the exception, but here is the exception for you”.
Does that makes any sense?

[Dmitri_Dolguikh]

Using HTTP status code is good, but in practice, as I understand the code, it is not used to know if there is an error, it is used to return false.

Smart-proxy does return different 4xx and 5xx codes depending on the error, which would help with reporting/resolving errors (including figuring out if an error is a transient or permanent one).

Having said that, using two different types of content type without changing the header (that is you are reporting application/json) even on an error, means that it should be expected a JSON.

The encoding can be dropped. Again, I don’t see any point in using JSON if all we pass back is a single string.

Can you please show me where on the code I happens, because all I see, is that it returns false, what am I missing?

https://github.com/theforeman/smart-proxy/blob/develop/modules/dhcp/dhcp_api.rb#L36 (and below. Please see other end-points in this and other modules for more examples),
https://github.com/theforeman/smart-proxy/blob/develop/lib/proxy/helpers.rb#L10

[Dmitri_Dolguikh]

In this case I wish to create the effect of “I didn’t created the exception, but here is the exception for you”.
Does that makes any sense?

I don’t understand how you plan to use this. You can’t re-raise an exception in a controller – sinatra/rack default error handler will catch it and return a 500 error with a stacktrace, which is probably not what you are looking for.

[ik5]

I just realized that we are talking about two different things.

I’m talking about https://github.com/theforeman/foreman/blob/develop/lib/proxy_api/resource.rb#L54 (def parse(response)) of foreman - It parses the response from smart-proxy and return the content.

My issue with it is the way it return a content. It just return false if something wrong, and I want to change that it will throw an error.

I didn’t meant by any means to touch smart-proxy in that manner, sorry if I indicated otherwise.

Does that makes more sense now regarding what I wish to do?