Skip to content
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.

Sign up for GitHub

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

Handle SCEP PKI message in POST request body #542

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Handle SCEP PKI message in POST request body #542

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60b7e9e
Handle SCEP messages sent in POST body
borama Aug 29, 2020
c7dd461
Use single-line Base64 encoding
borama Aug 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 37 additions & 2 deletions base/ca/src/com/netscape/cms/servlet/cert/scep/CRSEnrollment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
package com.netscape.cms.servlet.cert.scep;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.PublicKey;
Expand All @@ -32,6 +34,7 @@

import javax.servlet.ServletConfig;
import javax.servlet.ServletException;
import javax.servlet.ServletInputStream;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
Expand Down Expand Up @@ -190,7 +193,7 @@ public class CRSEnrollment extends HttpServlet {
private static final String PROP_FLATTENDN = "flattenDN";
private static final String PROP_ENTRYOC = "entryObjectclass";

// URL parameters
// URL parameters (message may be optionally present in body for POST requests)
private static final String URL_OPERATION = "operation";
private static final String URL_MESSAGE = "message";

Expand Down Expand Up @@ -355,6 +358,11 @@ public void service(HttpServletRequest httpReq,
String message = null;
mEncryptionAlgorithm = mConfiguredEncryptionAlgorithm;

logger.debug("http method=" + httpReq.getMethod());

// Try reading binary message in POST body and Base64-encode it
message = readMessageFromPostBody(httpReq);

// Parse the URL from the HTTP Request. Split it up into
// a structure which enables us to read the form elements
ArgBlock input = new ArgBlock(toHashtable(httpReq));
Expand All @@ -363,7 +371,9 @@ public void service(HttpServletRequest httpReq,
// Read in two form parameters - the router sets these
operation = (String) input.get(URL_OPERATION);
logger.debug("operation=" + operation);
message = (String) input.get(URL_MESSAGE);
if (message == null) {
message = (String) input.get(URL_MESSAGE);
}
logger.debug("message=" + message);

if (!mEnabled) {
Expand Down Expand Up @@ -404,6 +414,31 @@ public void service(HttpServletRequest httpReq,

}

/*
* If this is a POST request, try reading the binary message from the request body.
*/
private String readMessageFromPostBody(HttpServletRequest httpReq) {
String message = null;

try {
if (httpReq.getMethod().equalsIgnoreCase("POST")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ServletInputStream is = httpReq.getInputStream();
ByteArrayOutputStream bstream = new ByteArrayOutputStream(10000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we pull Commons IO, do we want to use IOUtil.Copylarge?


int r;
while ((r = is.read()) > -1) {
bstream.write(r);
}

message = Utils.base64encode(bstream.toByteArray(), false);
}
} catch (IOException e) {
logger.warn("CSREnrollment: exception while reading POST body: " + e.getMessage(), e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

}

return message;
}

private boolean isAlgorithmAllowed(String[] allowedAlgorithm, String algorithm) {
boolean allowed = false;

Expand Down