Handle SCEP PKI message in POST request body #542
Conversation
If the SCEP request is a HTTP POST request, we try to read the message as a binary string from the request body as outlined in https://tools.ietf.org/html/draft-gutmann-scep-16#section-4.3. Note that the content type is ignored here - all POST request are processed this way which may cause problems when parameters are sent as form values in the body.
|
\o hey @borama -- sorry, we've been a bit busy. Hope to take a look at it by late next week. :) |
|
Hi @cipherboy, sure, no worries, thanks! |
There was a problem hiding this comment.
Thank you for your contribution. I only have two comments, and I am soliciting @edewata 's and @cipherboy 's input and let them have the final call on whether changes are needed.
Otherwise, I have tested this patch with curl post and seems to work as expected.
| String message = null; | ||
|
|
||
| try { | ||
| if (httpReq.getMethod().equalsIgnoreCase("POST")) { |
There was a problem hiding this comment.
just a style thing. Personally, I'd put the the check on the method (POST or GET) outside of this method and branch on how each method retrieves its message. But I'll let our our refactor expert @edewata chip in on this.
There was a problem hiding this comment.
I'm not too familiar with SCEP. Does it support both POST and GET? In general if POST and GET are handled differently I'd suggest putting them into separate doPost() and doGet() methods.
| try { | ||
| if (httpReq.getMethod().equalsIgnoreCase("POST")) { | ||
| ServletInputStream is = httpReq.getInputStream(); | ||
| ByteArrayOutputStream bstream = new ByteArrayOutputStream(10000); |
There was a problem hiding this comment.
Is this an attempt to limit the size of the input data? If so, I don't think it'd work as the buffer will grow as needed for ByteArrayOutputStream. Now the question is on whether we want to set a limit on the input data. Any suggestion, @cipherboy or @edewata ?
There was a problem hiding this comment.
Yeah, I think this will limit the input data, but since the try-catch block surrounding this code will discard the exception, I'm not quite sure what would happen if the limit is exceeded, whether it will fall back to the old behavior or completely fail. IIUC the getParameter() cannot be called after getInputStream(), so it might completely fail.
Is there a defined limit on the size of a valid SCEP input?
There was a problem hiding this comment.
Since we pull Commons IO, do we want to use IOUtil.Copylarge?
| message = Utils.base64encode(bstream.toByteArray(), false); | ||
| } | ||
| } catch (IOException e) { | ||
| logger.warn("CSREnrollment: exception while reading POST body: " + e.getMessage(), e); |
There was a problem hiding this comment.
In general we don't want to catch and discard exceptions since it would make it harder to troubleshoot issues. I'd suggest to wrap and rethrow the exception like this:
throw new ServletException(e);
| try { | ||
| if (httpReq.getMethod().equalsIgnoreCase("POST")) { | ||
| ServletInputStream is = httpReq.getInputStream(); | ||
| ByteArrayOutputStream bstream = new ByteArrayOutputStream(10000); |
There was a problem hiding this comment.
Yeah, I think this will limit the input data, but since the try-catch block surrounding this code will discard the exception, I'm not quite sure what would happen if the limit is exceeded, whether it will fall back to the old behavior or completely fail. IIUC the getParameter() cannot be called after getInputStream(), so it might completely fail.
Is there a defined limit on the size of a valid SCEP input?
| String message = null; | ||
|
|
||
| try { | ||
| if (httpReq.getMethod().equalsIgnoreCase("POST")) { |
There was a problem hiding this comment.
I'm not too familiar with SCEP. Does it support both POST and GET? In general if POST and GET are handled differently I'd suggest putting them into separate doPost() and doGet() methods.
This is a minimal implementation of parsing the SCEP PKI message from the body of the request. When a POST request is received in the servlet, the code expects binary data in the POST body and tries to parse it into the
message, which is then handled the same way as the message from a GET parameter.I am not sure whether Dogtag uses POST requests in the servlet internally but if it does, it might bring problems as reading the body clashes with current code for determining the parameters (AFAIK, the body of a servlet request may be read only once and both the new code as well as the current code in the
toHashTablemethod try to read the request body). Ideally, we should only read POST body when theapplication/x-pki-messagecontent_type is present in the request but in reality it seems that many SCEP clients don't do that.