1
\$\begingroup\$

I'm creating a rest api using Spring which utilize javax.crypto as library to do some encryption stuff. While everything works well, I'm curious whether the error handling like this is acceptable or not?

@Slf4j
@Component
public class RSAEncryption {

private static final String ENCRYPTION_ALGORITHM = "RSA";
    private static final String CHARSET = "UTF-8";

    public KeyPair generateKeyPair() {
        final int keySize = 2048;
        KeyPairGenerator keyPairGenerator = null;
        try {
            keyPairGenerator = KeyPairGenerator.getInstance(ENCRYPTION_ALGORITHM);
            keyPairGenerator.initialize(keySize);
            return keyPairGenerator.generateKeyPair();
        } catch (NoSuchAlgorithmException e) {
            LOGGER.error("Invalid algorithm supplied: [{}]", e.getMessage());
            return null;
        }
    }

    public PublicKey readPublicKey(byte[] keyBytes)
    {
        X509EncodedKeySpec publicSpec = new X509EncodedKeySpec(keyBytes);
        try {
            KeyFactory keyFactory = KeyFactory.getInstance(ENCRYPTION_ALGORITHM);
            PublicKey publicKey = keyFactory.generatePublic(publicSpec);
            return publicKey;
        } catch (InvalidKeySpecException e) {
            LOGGER.error("Failed to generate public key from spec: [{}]", e.getMessage());
            return null;
        } catch (NoSuchAlgorithmException e) {
            LOGGER.error("Invalid algorithm supplied: [{}]", e.getMessage());
            return null;
        }

    }

    public PrivateKey readPrivateKey(byte[] keyBytes)
    {
        PKCS8EncodedKeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);

        try {
            KeyFactory keyFactory = KeyFactory.getInstance(ENCRYPTION_ALGORITHM);
            PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
            return privateKey;
        } catch (InvalidKeySpecException e) {
            LOGGER.error("Failed to generate private key from spec: [{}]", e.getMessage());
            return null;
        } catch (NoSuchAlgorithmException e) {
            LOGGER.error("Invalid algorithm supplied: [{}]", e.getMessage());
            return null;
        }

    }

    public String encrypt(PrivateKey privateKey, String message) {
        try {
            Cipher cipher = Cipher.getInstance(ENCRYPTION_ALGORITHM);
            cipher.init(Cipher.ENCRYPT_MODE, privateKey);
            byte[] bytes = cipher.doFinal(message.getBytes(CHARSET));
            return new String(Base64.getEncoder().encode(bytes));
        } catch (NoSuchAlgorithmException e) {
            LOGGER.error("Invalid algorithm supplied: [{}]", e.getMessage());
            return null;
        } catch (NoSuchPaddingException e) {
            LOGGER.error("Invalid byte padding: [{}]", e.getMessage());
            return null;
        } catch (InvalidKeyException e) {
            LOGGER.error("Invalid key: [{}]", e.getMessage());
            return null;
        } catch (UnsupportedEncodingException e) {
            LOGGER.error("Invalid encoding: [{}]", e.getMessage());
            return null;
        } catch (IllegalBlockSizeException e) {
            LOGGER.error("Invalid byte's block size: [{}]", e.getMessage());
            return null;
        } catch (BadPaddingException e) {
            LOGGER.error("Bad byte's padding: [{}]", e.getMessage());
            return null;
        }

    }

    public String decrypt(PublicKey publicKey, byte[] encrypted) {

        try {
            Cipher cipher = Cipher.getInstance(ENCRYPTION_ALGORITHM);
            cipher.init(Cipher.DECRYPT_MODE, publicKey);
            byte[] bytes = cipher.doFinal(encrypted);
            return new String(bytes);
        } catch (NoSuchAlgorithmException e) {
            LOGGER.error("Invalid algorithm supplied: [{}]", e.getMessage());
            return null;
        } catch (NoSuchPaddingException e) {
            LOGGER.error("Invalid byte padding: [{}]", e.getMessage());
            return null;
        } catch (InvalidKeyException e) {
            LOGGER.error("Invalid key: [{}]", e.getMessage());
            return null;
        } catch (UnsupportedEncodingException e) {
            LOGGER.error("Invalid encoding: [{}]", e.getMessage());
            return null;
        } catch (IllegalBlockSizeException e) {
            LOGGER.error("Invalid byte's block size: [{}]", e.getMessage());
            return null;
        } catch (BadPaddingException e) {
            LOGGER.error("Bad byte's padding: [{}]", e.getMessage());
            return null;
        } catch (IllegalBlockSizeException e) {
            LOGGER.error("Illegal block size: [{}]", e.getMessage());
            return null;
        }

    }

}


and then the calling Service :

@Service
@Slf4j
public class CommonService {

    @Autowired
    private RSAEncryption rsaEncryption;

    private RSAKey checkKeysExistence(String key) throws IOException {
        boolean exist;
        S3Object publicKeyS3 = null;
        S3Object privateKeyS3 = null;
        try {
            publicKeyS3 = s3Client.getObject("bucket", key);
            privateKeyS3 = s3Client.getObject("bucket", key);
            exist = true;
        } catch (AmazonS3Exception e) {
            if(e.getStatusCode() == 404) {
                exist = false;
            } else {
                throw e;
            }
        }

        if(exist) {
            byte[] publicKeyBytes = publicKeyS3.getObjectContent().readAllBytes();  // should I catch IOException here or add to method signature?
            byte[] privateKeyBytes = privateKeyS3.getObjectContent().readAllBytes();
            PublicKey publicKey = rsaEncryption.readPublicKey(publicKeyBytes);
            PrivateKey privateKey = rsaEncryption.readPrivateKey(privateKeyBytes);
            return new RSAKey(publicKey, privateKey);

        } else {
            KeyPair keyPair = rsaEncryption.generateKeyPair();
            if(keyPair == null) {
                throw new MyOwnException2();
            } else {
                addFileToS3(.....)
                addFileToS3(.....);
                return new RSAKey(keyPair.getPublic(), keyPair.getPrivate());
            }
        }
    }

    public String encrypt(String namespace, String appId, String msg) {  
        try {
            RSAKey rsaKey = checkKeysExistence("private.der");
            String encrypted = rsaEncryption.encrypt(rsaKey.getPrivateKey(), msg);
            return encrypted;
        } catch (IOException e) {
            throw new MyOwnException();
        }

    }

    public String decrypt(String namespace, String encrypted) {
        try {
            RSAKey rsaKey = checkKeysExistence("public.der");
            String solve = rsaEncryption.decrypt(rsaKey.getPublicKey(), encrypted);
            return solve;
        } catch (IOException e) {
            throw new MyOwnException();
        }

    }
}

any advice will be much appreciated.

\$\endgroup\$

closed as off-topic by 200_success, Stephen Rauch, yuri, Toby Speight, Graipher Sep 10 '18 at 10:11

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – 200_success, Stephen Rauch, yuri, Toby Speight, Graipher
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ How do you handle MyOwnException? \$\endgroup\$ – Aris Sep 9 '18 at 13:56
  • 2
    \$\begingroup\$ LOGGER.error(...) is a bit sketchy, especially considering that you want us to review your error handling. \$\endgroup\$ – 200_success Sep 9 '18 at 19:03
  • \$\begingroup\$ @Aris this is spring app & I have mapper for the MyOwnException. \$\endgroup\$ – imeluntuk Sep 14 '18 at 5:43
1
\$\begingroup\$

The general principle seems a bit odd and more work than necessary.

Basically, in case of an error, you perform the following steps:

  • catch and log exception
  • return null
  • check for null in the caller
  • throw MyOwnException in the caller
  • catch and handle MyOwnException in some super-caller

The intermediate null makes it somewhat long-winded and brittle (what if a caller forgets to handle null?) and goes against the idea of exceptions, which can propagate up the call-stack and be handled at any suitable place.

Thus, I'd recommend to scratch the null part. Instead:

  • catch and log exception
  • throw MyOwnException (possibly with the original exception set as cause) instead of returning null
  • catch and handle MyOwnException in some super-caller

For all non-exception cases, make sure that you always return a correct and usable value. That way you can remove all the special case handling in the caller methods and have a sweet and simple sunny-days scenario as the implementation.

Apart from that, I'd recommend to use multi-catch blocks to reduce the redundant handling code in the various RSAEncryption class methods.

\$\endgroup\$

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