6
\$\begingroup\$

I am trying to implement a IDP initiated Single Sign On Solution (service similar to onelogin's) to Cloud based Service providers such as Google Apps, Salesforce etc.

The project is a Maven eclipse project (Web app) and the main servlet which consumes SAML Request sent via HTTP-GET / HTTP-POST and generates a valid SAML Response, digitally signs it and attempts to POST the same to the ACS url is the SamlHandler.java file.

The code of which I am posting below for a review:

(Please note that the project is for learning and not for any commercial purposes)

I am breaking down the code so that it becomes easier to review the same.

Also kindly excuse my haphazard coding style and messy comments. This is still very much unfinished and so I did not give much importance to the code formatting.

checkSession method to verify that the user session is still valid.

public boolean checkSession(HttpServletRequest request) {

    try {

        HttpSession session = request.getSession(false);

        if (session.getAttribute("loggedIn").equals("yes")) {
            System.out.println("Session is valid.");
            // If the session is valid retrieve the user credentials
            // Every user will be uniquely identified by the email and
            // domain.
            email = (String) session.getAttribute("email");
            domain = (String) session.getAttribute("domain");
            sessionId = (String) session.getId();
            return true;

        }

        else {
            System.out.println("User is not logged in.");
            return false;

        }

    } catch (Exception e) {
        // If the loggedIn attribute is not set.
        System.out.println("User session invalid / User is not logged in.");
        return false;
    }

}

doPost and doGet method which invokes the handleSamlResponse method which takes on further processing of the request to yield the response.

public void doPost(HttpServletRequest request, HttpServletResponse response)
        throws ServletException, IOException {

    handleSamlRequest(request, response);

}

public void doGet(HttpServletRequest request, HttpServletResponse response)
        throws ServletException, IOException {

    handleSamlRequest(request, response);

}

handleSamlRequest method which parses the SAML Request attribute and invokes other methods to build the SAMLResponse which will be encoded to Base64 and POST-ed to the ACS URL.

public void handleSamlRequest(HttpServletRequest request,
        HttpServletResponse response) {

    // Validates the current user session before parsing the SAMLRequest
    if (!checkSession(request)) {
        try {
            response.sendRedirect("/SignOut.action");
        } catch (Exception e) {
            System.out
                    .println("Exception while trying to redirect to SignOut action.");
            e.printStackTrace();
        }
        return;
    }

    // If the session is valid proceed further ...

    // Parsing the RelayState
    // ----------------------
    try {

        relayState = request.getParameter("RelayState");

        System.out.println("\nThe received Relay State is : " + relayState);

        // If RelayState is Base64 encoded, it will not contain the text
        // "http"
        // hence 'try' decoding it ...
        if (!relayState.contains("http") && relayState.length() > 1) {

            relayStateb64 = relayState;
            relayStateIsb64 = true;

            // Base64 decode it
            relayState = new String(Base64.decodeBase64(relayState
                    .getBytes("UTF-8")), "UTF-8");

            System.out
                    .println("\nThe BASE64 Decoded Relay State Parameter is : "
                            + relayStateb64);

        }

    } catch (Exception e) {
        System.out
                .println("\n\nException while trying to parse the RelayState value.");
        e.printStackTrace();

        if (DEBUG) {
            try {
                System.in.read(); // Wait for Enter key to process further
            } catch (Exception e2) {
                System.out.println(e2.toString());
            }
        }
    }

    // Parsing the SAMLRequest
    // -----------------------
    try {

        String SAMLRequest = request.getParameter("SAMLRequest");

        parseAuthnRequest(SAMLRequest, request);

    } catch (Exception e) {
        System.out
                .println("Exception while trying to parse the SAMLRequest");
    }

    try {

        buildResponseXMLString();
        canonicalizeSamlResponse();
        generateSamlResponseObject();
        signSamlResponseObject2();
        encodeSamlResponse();
        postSamlResponse(response);

    } catch (Exception e) {
        System.out
                .println("Exception while constructing / posting the SAML Response data ...");
        System.out.println("The error is : " + e.toString());
        e.printStackTrace();
    }
}

I am skipping the code of the parseAuthnRequest method (which parses the SAML Request and extracts ACS URL and other parameters from the SAML Request object) as I feel okay about the same.

buildResponseXMLString which builds the SAML Response in XML Format from a template file.

public void buildResponseXMLString() {

    try {

        // The certificate and template xml file for saml response is stored
        // in the saml-data folder

        String strAssertionXMLTemplateFile = "/WEB-INF/classes/saml-data/saml.xml";
        String samlTemplateFileUrl = getServletContext().getResource(
                strAssertionXMLTemplateFile).toString();
        InputStream assertionTemplateFile = new URL(samlTemplateFileUrl)
                .openStream();

        // templateXmlString contains the raw SAML Response templates with
        // field handle to be replaced with appropriate parameters

        strResponseXML = new Scanner(assertionTemplateFile, "UTF-8")
                .useDelimiter("\\A").next().trim();

        assertionTemplateFile.close();

        System.out.println("\n\nThe assertion template is : \n"
                + strResponseXML);

        // Id generation
        SecureRandomIdentifierGenerator generator = new

        SecureRandomIdentifierGenerator();

        res_Id = generator.generateIdentifier().trim();
        res_assertionId = generator.generateIdentifier().trim();

        // Other important identifying parameters
        res_issuer = "###########################"; // Will be filled with valid issuer URL
        res_nameId = getEmail().trim();
        String dateTimeFormat = "yyyy-MM-dd'T'HH:mm:ss'+05:30'";
        res_issueInstant = new DateTime().toString(dateTimeFormat).trim();
        res_notbefore = new DateTime().toString(dateTimeFormat).trim();
        res_notonorafter = new DateTime().plusMinutes(5)
                .toString(dateTimeFormat).trim();

        // Filling the parameters into the template...
        strResponseXML = strResponseXML.replaceAll("_ASSERTION_ID",
                res_assertionId);
        strResponseXML = strResponseXML.replaceAll("_REQUEST_ID", req_Id);
        strResponseXML = strResponseXML.replaceAll("_RESPONSE_ID", res_Id);
        strResponseXML = strResponseXML.replaceAll("_ISSUE_INSTANT",
                res_issueInstant);
        strResponseXML = strResponseXML.replaceAll("_ISSUER", res_issuer);
        strResponseXML = strResponseXML.replaceAll("_NAMEID", res_nameId);
        strResponseXML = strResponseXML.replaceAll("_NOTBEFORE",
                res_notbefore);
        strResponseXML = strResponseXML.replaceAll("_NOTONORAFTER",
                res_notonorafter);
        strResponseXML = strResponseXML.replaceAll("_ACS_URL", acs);
        strResponseXML = strResponseXML.replaceAll("_DOMAIN", domain);

        // Replace the handles in strAssertionXML with appropriate
        // parameters

        System.out.println("\n\nThe complete SAML Response is : \n"
                + strResponseXML);

    } catch (Exception e) {
        System.out.println("Exception while filling SAML Response ...");
        System.out.println("The error is : " + e.toString());
        e.printStackTrace();
    }

}

signSamlResponseObject2 method (excuse the object in the method name, previously it used to sign a SamlResponse object from the OpenSAML library).

This is the part whose implementation I am very doubtful of:

public void signSamlResponseObject2() {

    try {

        String keyStoreFileName = "/WEB-INF/classes/saml-data/keystore.jks";
        InputStream fis = getServletContext().getResource(keyStoreFileName)
                .openStream();
        KeyStore ks = KeyStore.getInstance(KeyStore.getDefaultType());

        ks.load(fis, "******".toCharArray());
        fis.close();

        // Get Private Key Entry From keystore

        KeyStore.PrivateKeyEntry pkEntry = (KeyStore.PrivateKeyEntry) ks
                .getEntry("alias", new KeyStore.PasswordProtection(
                        "*******".toCharArray()));

        PrivateKey privKey = pkEntry.getPrivateKey();

        PublicKey pubKey = ks.getCertificate("alias").getPublicKey();

        X509Certificate cert = (X509Certificate) ks
                .getCertificate("alias");

        /*
         * // Getting x509 Certificate from the keystore directly.
         * 
         * KeyStore.TrustedCertificateEntry certEntry =
         * (KeyStore.TrustedCertificateEntry) ks .getEntry("alias", new
         * KeyStore.PasswordProtection( "******".toCharArray()));
         * 
         * X509Certificate cert = (X509Certificate)
         * certEntry.getTrustedCertificate();
         */

        // Create a DOM XMLSignatureFactory that will be used to generate
        // the
        // enveloped signature.

        // String providerName =
        // System.getProperty("jsr105Provider",JSR_105_PROVIDER);
        XMLSignatureFactory sigFactory = XMLSignatureFactory
                .getInstance("DOM");

        // Create a Reference to the enveloped document (we are
        // signing the whole document, so a URI of "" signifies that) and
        // also specify the SHA1 digest algorithm and the ENVELOPED
        // Transform.

        List envelopedTransform = Collections.singletonList(sigFactory
                .newTransform(Transform.ENVELOPED,
                        (TransformParameterSpec) null));

        Reference ref = sigFactory.newReference("",
                sigFactory.newDigestMethod(DigestMethod.SHA1, null),
                envelopedTransform, null, null);

        SignatureMethod signatureMethod = sigFactory.newSignatureMethod(
                SignatureMethod.DSA_SHA1, null);

        CanonicalizationMethod canonicalizationMethod = sigFactory
                .newCanonicalizationMethod(
                        CanonicalizationMethod.INCLUSIVE_WITH_COMMENTS,
                        (C14NMethodParameterSpec) null);

        // Create the SignedInfo
        SignedInfo signedInfo = sigFactory.newSignedInfo(
                canonicalizationMethod, signatureMethod,
                Collections.singletonList(ref));

        // Create a KeyValue containing the DSA PublicKey
        KeyInfoFactory keyInfoFactory = sigFactory.getKeyInfoFactory();
        KeyValue keyValuePair = keyInfoFactory.newKeyValue(pubKey);


        // Creating the x509 certificate data from Certificate object ( cert )

        List x509 = new ArrayList();

        x509.add(cert);

        X509Data x509Data = keyInfoFactory.newX509Data(x509);

        // Create a KeyInfo and add the KeyValue to it
        // keyInfoItems.add(Collections.singletonList(keyValuePair));

        // Adding the certificate data and the key value pair to the keyInfo 


        List keyInfoItems = new ArrayList();

        keyInfoItems.add(x509Data);
        keyInfoItems.add(keyValuePair);

        KeyInfo keyInfo = keyInfoFactory.newKeyInfo(keyInfoItems);


        // Building the org.jdom.Document object from the samlResponse
        // string
        // ------------------------------------------------------------------
        SAXBuilder builder = new SAXBuilder();
        org.jdom.Document doc = builder.build(new ByteArrayInputStream(
                strResponseXML.getBytes()));
        // ------------------------------------------------------------------

        // Convert the rootElement extracted from the doc to w3cElement
        // ------------------------------------------------------------------

        org.jdom.Element docRootElement = doc.getRootElement();
        doc = docRootElement.getDocument();

        XMLOutputter xmlOutputter = new XMLOutputter();
        StringWriter elemStrWriter = new StringWriter();
        xmlOutputter.output(doc, elemStrWriter);
        byte[] xmlBytes = elemStrWriter.toString().getBytes();
        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
        dbf.setNamespaceAware(true);
        org.w3c.dom.Element w3cElement = dbf.newDocumentBuilder()
                .parse(new ByteArrayInputStream(xmlBytes))
                .getDocumentElement();

        // --------------------------------------------------------------------

        // Create a DOMSignContext and specify the DSA PrivateKey and
        // location of the resulting XMLSignature's parent element

        DOMSignContext dsc = new DOMSignContext(privKey, w3cElement);

        // compute the correct location to insert the signature xml
        // (location is important because the SAML xsd's enforce sequence on
        // signed
        // info.)

        org.w3c.dom.Node xmlSigInsertionPoint = null;

        String JSR_105_PROVIDER = "org.jcp.xml.dsig.internal.dom.XMLDSigRI";
        String SAML_PROTOCOL_NS_URI_V20 = "urn:oasis:names:tc:SAML:2.0:protocol";

        org.w3c.dom.NodeList nodeList = w3cElement.getElementsByTagNameNS(
                SAML_PROTOCOL_NS_URI_V20, "Extensions");
        if (nodeList.getLength() != 0) {
            xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);
        } else {
            nodeList = w3cElement.getElementsByTagNameNS(
                    SAML_PROTOCOL_NS_URI_V20, "Status");
            xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);
        }
        dsc.setNextSibling(xmlSigInsertionPoint);

        // Marshal, generate (and sign) the enveloped signature
        XMLSignature signature = sigFactory.newXMLSignature(signedInfo,
                keyInfo);
        signature.sign(dsc);

        // Create the root dom element from the w3cElement using DOMBuilder
        DOMBuilder domBuilder = new DOMBuilder();
        org.jdom.Element signedElement = domBuilder.build(w3cElement);

        doc.setRootElement((org.jdom.Element) signedElement.detach());
        xmlOutputter = new XMLOutputter();
        strFinalResponse = xmlOutputter.outputString(doc);

        System.out.println("The signed SAML Response is : "
                + strFinalResponse);

    } catch (Exception e) {
        System.out
                .println("Exception while attempting to sign the SAML Response.");
        e.printStackTrace();
    }

}

The above code all put together is at this ideone link.

Now with the above clumsily written servlet, I am able to sign in using SAML Assertion to Salesforce. I also verified the resultant XML signature using external online tools and found it to be valid.

I would like a pro's opinion of my above code. I am new to web-app development and Java as such. Hence it would be extremely educative for me if you could guide me on how / where I could improve my code to a professional standards.

(The above code has been influenced by a number of sources and docs.)

\$\endgroup\$
6
\$\begingroup\$

the part whose implementation I am very doubtful of

I'm assuming your code works. If it doesn't, it's off-topic, and your question should get closed.

Duplication

    String dateTimeFormat = "yyyy-MM-dd'T'HH:mm:ss'+05:30'";
    res_issueInstant = new DateTime().toString(dateTimeFormat).trim();
    res_notbefore = new DateTime().toString(dateTimeFormat).trim();
    res_notonorafter = new DateTime().plusMinutes(5)
            .toString(dateTimeFormat).trim();

You create multiple DateTime objects. It's possible that between these lines of code, the processor switches to a different process to do things. This doesn't last very long, as your processor switches a LOT of times each second, but what you will see is that there can be a difference between these times. Don't rely on this behavior. Create only 1 DateTime object, and either set res_issueInstant and res_notbefore to be the same value, or specifically add a certain amount to res_notbefore.


    org.w3c.dom.NodeList nodeList = w3cElement.getElementsByTagNameNS(
            SAML_PROTOCOL_NS_URI_V20, "Extensions");
    if (nodeList.getLength() != 0) {
        xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);
    } else {
        nodeList = w3cElement.getElementsByTagNameNS(
                SAML_PROTOCOL_NS_URI_V20, "Status");
        xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);
    }

The last line in each if statement branch is duplicated. Let's take it out of the if statement.

    org.w3c.dom.NodeList nodeList = w3cElement.getElementsByTagNameNS(
            SAML_PROTOCOL_NS_URI_V20, "Extensions");
    if (nodeList.getLength() != 0) {
    } else {
        nodeList = w3cElement.getElementsByTagNameNS(
                SAML_PROTOCOL_NS_URI_V20, "Status");
    }
    xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);

It's still not nice, because you have an empty if statement block. Invert the if conditional and swap the blocks.

    org.w3c.dom.NodeList nodeList = w3cElement.getElementsByTagNameNS(
            SAML_PROTOCOL_NS_URI_V20, "Extensions");
    if (nodeList.getLength() == 0) {
        nodeList = w3cElement.getElementsByTagNameNS(
                SAML_PROTOCOL_NS_URI_V20, "Status");
    } else {
    }
    xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);

Lastly, remove the unnecessary else block.

    org.w3c.dom.NodeList nodeList = w3cElement.getElementsByTagNameNS(
            SAML_PROTOCOL_NS_URI_V20, "Extensions");
    if (nodeList.getLength() == 0) {
        nodeList = w3cElement.getElementsByTagNameNS(
                SAML_PROTOCOL_NS_URI_V20, "Status");
    }
    xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);


Another bit of code I saw:

    strResponseXML = strResponseXML.replaceAll("_ASSERTION_ID",
            res_assertionId);
    strResponseXML = strResponseXML.replaceAll("_REQUEST_ID", req_Id);
    strResponseXML = strResponseXML.replaceAll("_RESPONSE_ID", res_Id);
    strResponseXML = strResponseXML.replaceAll("_ISSUE_INSTANT",
            res_issueInstant);
    strResponseXML = strResponseXML.replaceAll("_ISSUER", res_issuer);
    strResponseXML = strResponseXML.replaceAll("_NAMEID", res_nameId);
    strResponseXML = strResponseXML.replaceAll("_NOTBEFORE",
            res_notbefore);
    strResponseXML = strResponseXML.replaceAll("_NOTONORAFTER",
            res_notonorafter);
    strResponseXML = strResponseXML.replaceAll("_ACS_URL", acs);
    strResponseXML = strResponseXML.replaceAll("_DOMAIN", domain);

This seems like it could be a for loop. I'd personally use a 2D String array here. A HashMap is a nice idea but you might want to preserve the order of operations, and a HashMap doesn't allow you to do so. Additionally, we want to make the code readable.

final String[][] replacementMappings = new String[][]{
    {"_ASSERTION_ID", res_assertionId},
    {"_REQUEST_ID", req_Id},
    {"_RESPONSE_ID", res_Id},
    {"_ISSUE_INSTANT", res_issueInstant},
    {"_ISSUER", res_issuer},
    {"_NAMEID", res_nameId},
    {"_NOTBEFORE", res_notbefore},
    {"_NOTONORAFTER", res_notonorafter},
    {"_ACS_URL", acs},
    {"_DOMAIN", domain}
}

I have declared the array to be final since we're not gonna be changing the mappings. Then, the for loop:

for(int i = 0; i < replacementMappings.length; i++){
    strResponseXML = strResponseXML.replaceAll(replacementMappings[i][0], replacementMappings[i][1];
}

This cleans up that section, mostly.

Constants

    String JSR_105_PROVIDER = "org.jcp.xml.dsig.internal.dom.XMLDSigRI";
    String SAML_PROTOCOL_NS_URI_V20 = "urn:oasis:names:tc:SAML:2.0:protocol";

You don't assign further values to these variables. I suggest making them final. Perhaps even moving them outside the function. Typically, constants are placed near the top of the class, above the other variable declarations. Assuming this class isn't a singleton already, I'd make these variables private static final Strings. This because there's no reason to have multiple references towards the same string. The reason you put it at the top of the class is that in the event you want to change the value, you don't have to go searching for the definition.

Comments

This is still very much unfinished and so I did not give much importance to the code formatting.

Code formatting can usually be done quickly. Your IDE will probably have some sort of ability to auto-format. Alternatively, both online and offline code formatters exist. Find one you like (or configure your IDE's formatting to your liking), and use it.


    // Create a KeyInfo and add the KeyValue to it
    // keyInfoItems.add(Collections.singletonList(keyValuePair));

    // Adding the certificate data and the key value pair to the keyInfo
    List keyInfoItems = new ArrayList();

    keyInfoItems.add(x509Data);
    keyInfoItems.add(keyValuePair);

Don't put code in comments, unless it's to illustrate usage.

Code in comments is dead code. It doesn't get run. It doesn't get tested. It doesn't get maintained. It will get out of date when a last-second bugfix is applied. It will eventually confuse reviewers, maintainers and you yourself.

"But Pim, what if I need the code later?"

Use source control, such as Git, Mercurial and Subversion. At the very least, comment that the code is removed because insert reason here.


You seem to comment the function bodies well.
What I don't know (because it's not in the pasted snippets) is whether you comment your function headers. Commenting function headers is a good idea because if you do it properly (javadoc-style), most IDE's will add the commentary as a tooltip when either mousing over the function name, or when selecting the method in a auto-complete dropdownlist. This helps you in your development.
Additionally, the first thing I read when I'm trying to understand what a function does is the commentary on the function. In particular, try to comment your public methods - these are the ones that will get called in other files, and these are the ones that people will generally be interested in.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for a detailed and very educative review of my code. :) Lastly could you kindly review my signing methodology ( with SAML specifications in mind ) \$\endgroup\$ – Raghav RV Aug 6 '14 at 12:39
  • \$\begingroup\$ Unfortunately I don't know what SAML specifications are. That said, if you wish to verify correctness of the code, I suggest implementing unit-tests instead of a review. Humans make mistakes. Plus, proof that when given a certain input, the code will produce a certain output is far better than me saying "looks fine to me". \$\endgroup\$ – Pimgd Aug 6 '14 at 12:45
  • \$\begingroup\$ I tried it with Salesforce and it works, but it doesn't work with google apps. Says "Unable to parse input". Do you think I should raise a separate SO question for the same ? ( Since it would fit better there ) ? \$\endgroup\$ – Raghav RV Aug 6 '14 at 12:47
  • \$\begingroup\$ I would indeed make a question on Stack Overflow for that. Code Review is for working code that you wish to improve. I also edited my answer - I saw some constants that you declare in the middle of a function, and have added suggested improvements for that. \$\endgroup\$ – Pimgd Aug 6 '14 at 12:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.