2
\$\begingroup\$

I had been working on several projects that required Header Message Authentication Codes to be generated when brokering files with 3rd parties like Netflix.

Unfortunately on older systems like Windows Server 2003, Solaris and early versions of RHEL there was no easy way to generate HMACs which could be Base encoded.

So I created this Java application to do what I needed.

I have come from a C++ background so would appreciate some review to my Java code.

Particularly interested in people's opinions of how I've balanced the logic between Main and other classes.

HMAC_BASE on github

Main.java

/**
 * 
 */
package com;



import helper.Base;
import helper.HMAC;
import helper.IOHandler;
import helper.Posix;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.security.InvalidAlgorithmParameterException;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import org.apache.commons.cli.CommandLine;
import org.apache.commons.cli.ParseException;
import org.apache.log4j.Level;
import org.apache.log4j.Logger;



/**
 * @author Bruce Taylor
 * 
 */
public class Main
{

    // one instantiation of log4j per class
    private static Logger log = Logger.getLogger(Main.class.getName());
    private static CommandLine cl;
    private static IOHandler io = new IOHandler();;

    /*
     * Current methods
     */
    private static enum methods
    {
        base, hmac_base
    }

    /*
     * Sets the loglevel
     */
    public static void setLogLevel()
    {
        if (cl.hasOption("debug"))
        {
            log.info("Debugging has been enabled.");
            log.setLevel(Level.DEBUG);
        }
        else
            log.setLevel(Level.OFF);

    }

    /*
     * Perform HMAC operation
     */
    public static byte[] doHMAC(String str) throws InvalidKeyException, NoSuchAlgorithmException, InvalidAlgorithmParameterException, UnsupportedEncodingException
    {
        log.info("Input string for HMAC : " + str);
        if (cl.hasOption("hash") && cl.hasOption("key"))
        {
            String hashKey = cl.getOptionValue("key");
            String hashMethod = cl.getOptionValue("hash");
            log.info("Hashing algorithm being used is : " + hashMethod);
            log.info("Secret key is : " + hashKey);

            HMAC hmac = new HMAC(hashKey.getBytes(), hashMethod);
            hmac.generateHash(str);
            return hmac.getHash();
        }
        else
        {
            if (cl.hasOption("hash"))
            {
                log.error("No key given for HMAC.");
                System.exit(-1);
            }
            if (cl.hasOption("key"))
            {
                log.error("No hashing algorithm given for HMAC.");
                System.exit(-1);
            }

        }
        return null;

    }

    /*
     * @param str Perform Base operation
     */
    public static byte[] doBase(byte[] bs) throws UnsupportedEncodingException
    {
        log.info("Input string for Base : " + bs);

        if (cl.hasOption("encode"))
        {
            String baseMethod = cl.getOptionValue("encode");
            log.info("Doing encode in " + baseMethod);
            if (Base.isBaseMethodValid(baseMethod) == true)
                return Base.encode(bs, baseMethod);
            else
                throw new UnsupportedEncodingException("Unsupported encoder : " + baseMethod);

        }
        else if (cl.hasOption("decode"))
        {
            String baseMethod = cl.getOptionValue("encode");
            log.info("Doing decode");
            if (Base.isBaseMethodValid(baseMethod) == true)
                return Base.decode(bs, baseMethod);
            else
                throw new UnsupportedEncodingException("Unsupported encoder : " + baseMethod);
        }
        else if (cl.hasOption("decode") && cl.hasOption("encode"))
        {
            log.error("Both encode and decode methods have been called, remove one method.");
            System.exit(-1);
        }

        else
        {
            log.error("No base operation specified.");
            System.exit(-1);
        }

        return null;
    }

    /*
     * Retrieves input arg and passes it to IOHandler
     */
    public static String handleInput() throws IOException
    {
        log.info("Checking input.");

        if (cl.hasOption("input"))
        {
            String input = cl.getOptionValue("input");
            log.info("An input of : " + input + " was specified.");

            io.interpretInput(input);

            // user wants to remove whitespace from input
            if (cl.hasOption("cleanseWs"))
                io.cleanseWhiteSpace();
            // user wants to remove new lines from input
            if (cl.hasOption("cleanseNl"))
                io.cleanseNewLine();
            // user wants to remove characters from input using custom regex
            if (cl.hasOption("cleanse"))
                io.cleansePattern(cl.getOptionValue("cleanse"));

        }
        else
        {
            log.error("No input specified, exiting.");
            System.exit(-1);
        }
        log.info("Final input is : " + io.getString());
        return io.getString();
    }

    /*
     * Produces output either to the command line or to a specified file
     * @param output
     */
    public static void handleOutput(byte[] output) throws IOException
    {
        log.info("Preparing output");

        if (cl.hasOption("file"))
        {
            String filename = cl.getOptionValue("file");
            log.info("Value to be written to file : " + filename);
            io.outputToFile(filename, output);
        }
        else
        {
            log.info("Outputting using standard output.");
            System.out.println(new String(output, "UTF-8"));
        }
    }

    /**
     * @param args
     */
    public static void main(String[] args)
    {
        try
        {
            // initialise Posix object and parse arguments from command line
            cl = new Posix(args).parseArgs();

            // set loglevel depending on presence of -debug argument
            setLogLevel();

            log.info("Parsing command line arguments.");
            // check mimimum arguments given
            if (cl.getOptions().length > 1)
            {
                // can't use string switch in Java 1.6

                // do base only
                if (cl.hasOption("base"))
                {
                    try
                    {
                        log.info("A BASE operation will only be performed in this execution.");
                        byte[] output = doBase(handleInput().getBytes());
                        log.info("Final output is : " + new String(output, "UTF-8"));
                        handleOutput(output);
                    }
                    catch (UnsupportedEncodingException e)
                    {
                        System.out.println(e.getLocalizedMessage() + ", only the following BASE encoding methods " + java.util.Arrays.asList(Base.validBaseMethods.values()) + " are currently accepted.");
                        System.exit(-1);
                    }
                    catch (IOException e)
                    {
                        System.out.println(e.getLocalizedMessage() + ", please ensure file and current working directory is writable.");
                        System.exit(-1);
                    }

                }
                // do hmac and base
                else if (cl.hasOption("hmac_base"))
                {
                    log.info("Both HMAC and BASE operations will be performed in this execution.");

                    try
                    {
                        byte[] output = doBase(doHMAC(handleInput()));
                        log.info("Final output is : " + new String(output, "UTF-8"));
                        handleOutput(output);
                    }
                    catch (InvalidKeyException e)
                    {
                        System.out.println(e.getLocalizedMessage() + ", the HMAC key provided is invalid.");
                        System.exit(-1);
                    }
                    catch (NoSuchAlgorithmException e)
                    {
                        System.out.println(e.getLocalizedMessage() + ", this really shouldn't come up as an error message.");
                        System.exit(-1);
                    }
                    catch (InvalidAlgorithmParameterException e)
                    {
                        System.out.println(e.getLocalizedMessage() + ", only the following HMAC methods " + java.util.Arrays.asList(HMAC.validHmacMethods.values()) + " are accepted.");
                        System.exit(-1);
                    }
                    catch (UnsupportedEncodingException e)
                    {
                        System.out.println(e.getLocalizedMessage() + ", only the following BASE encoding methods " + java.util.Arrays.asList(Base.validBaseMethods.values()) + " are currently accepted.");
                        System.exit(-1);
                    }
                    catch (IOException e)
                    {
                        System.out.println(e.getLocalizedMessage() + ", please ensure file and current working directory is writable.");
                        System.exit(-1);
                    }
                }
                // default
                else
                {
                    log.error("No operation method specific. Expected either \"hmac_base\" or \"base\" .");
                    System.exit(-1);
                    // TODO
                }

            }
            else
            {
                log.error("The minimum amount of arguments required were not given. Expected " + methods.values());
                System.exit(-1);
            }

        }

        // end of posix try block
        catch (ParseException e)
        {
            // occurs with invalid posix options given
            System.out.println(e.getLocalizedMessage() + ", please check the user manual for all accepted command arguments.");
            System.exit(-1);
        }

    }
}

// the spice must flow

P.s. my jUnit tests are foobar currently, so would appreciate some guidance on how to properly pass POSIX arguments iteratively to Main from a test case.

\$\endgroup\$
3
\$\begingroup\$

Code

To summarise the issues I see below, I'd suggest using less static methods/classes because it makes testing way harder and in general looks unnecessary in most places except maybe the helper stuff. I'd also take a look at common libraries, so you don't have to rewrite e.g. file reading and input cleaning. Obvious candidates would be Guava, Apache Commons and so on.

I'm not too happy with the different classes. CommandOptions is good, because you set up the command line options once there. But then, you actually handle them in Main. I'd move the checks into CommandOptions, then have regular methods, getHash, getKey, which do the logic you currently have in doHMAC and doBase.

The helper classes look okay I guess, but it's still confusing that your helpers depend on the main classes Main and CommandOptions and not the other way round.

  • You strive for compatibility with Java 1.6, so I'm to skip recommendations for newer language features; just going to say that the exception handling can be a bit shorter if you do the dispatching yourself, i.e. catch Exception, print .getLocalizedMessage and then check the type yourself. IMO having less code for that is preferable.
  • The indentation isn't optimal; you can get rid of some levels of indentation by exiting early, moving blocks into methods, etc. E.g. in the main method, you can move the check for any arguments before the rest and then exit if there aren't any arguments. Same in Base.java:148.
  • Lots of (empty) comments which don't add any information. E.g. Sets the loglevel is something I can already see from the method name setLoglevel.
  • The actual functionality is very tightly coupled with option parsing and error handling by System.exit. It would be better to move options in a separate part, so that methods like doHMAC are "obviously correct" instead of having to wade through the logging/option handling parts.
  • Also, System.exit directly in such a method is very harsh. I'd rather propagate an exception so that the method is usable from other places as well, like you already do in the main method. That way, parts of the application could also be reused as a library.
  • if (foo == true) or if (foo == false) is not necessary, just use if (foo) (and similar cases as well).
  • Enumerations usually use upper case constants (methods in Main.java) and the enumeration itself should follow regular class naming.
  • doHMAC, doBase have a return null that looks like it's either not executed or wrong anyway.
  • The staticness of Main.java is detrimental to the ease of testing that class. First steps to change that would probably be using parameters (either for the methods, or the class/constructor) instead of static members.
  • Base.java:74 is a bug I think; after lower casing you'll never match it with Base32. Also, what happens if the method is suddenly foo? Also, again way too much static.
  • HMAC.java:75, why new Hex();? That's not used anywhere.

As an example for "too much code",Posix.java:49, the whole method can just be return new PosixParser().parse(comOptions, args); with no loss of functionality.

Some of these things can also be discovered by automatic checkers like CheckStyle, PMD and so on. Some will also warn about package dependencies, so it might be useful to run those on the project as well.

Tests

As you already saw passing arguments via a private static member like CommandLine is not the best way to do that. Although I'm pretty sure there are mocking libraries to circumvent the restrictions.

So what you can do immediately is to make Main a regular, non-static class, with cl and io either passed in via constructor arguments, or available via getCommandLine, etc. In you test case you'd instantiate the application each time, set command line arguments and then call whatever function you want. For that it would also be advisable to move logic out of the main method, or you're okay with not testing that part.

Packaging

  • Usually you'd use reverse domain names for packages. There's also a quasi standard project layout used by Maven. In any case com and helper as top-level package names aren't great.
  • It would be good to have a Maven or ant file to build it without an IDE. I haven't yet compiled it for that exact reason.
  • The README.md has a section Installation, but if you really want people to use it, they shouldn't have to know how to build a runnable JAR file from scratch. That ties in with the point above: provide either/and a Makefile, Maven build file and ant build file and then also put some paragraphs into Installation on how to exactly invoke commands to build the application (and run tests). E.g. some along the lines of "Invoke 'mvn compile' to create 'build/hmac.jar'." Maybe even have an installation routine to install a small wrapper script to call that jar, "Invoke 'mvn install PREFIX=/usr/local' to install the 'hmac' script on your machine." Obviously there's a lot more work there, so you can just start of with some build instructions first.

Hope that helps! Also take it with a grain of salt, I'm biased towards Spring, so that shows I guess.

\$\endgroup\$
  • \$\begingroup\$ Thanks for taking the time for reviewing my code. I will do some refactoring on it this week. Perhaps I should do some Java training or certification? Although I do feel that I can take what I know from C into Java, it might not be to conventions but I can learn those on the way. Many thanks! \$\endgroup\$ – tomaytotomato Jan 7 '15 at 15:27
  • \$\begingroup\$ Eh, it looks good enough I think; since you already enough to produce working code I doubt the value of certification, but YMMV. If you maybe take a look at a Java style guide, or so maybe that's helpful; if you care about testing then using dependency injection and non-static methods just makes your life easier, since your code is already doing what it should. \$\endgroup\$ – ferada Jan 7 '15 at 16:07

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.