[jira] [Commented] (HADOOP-10632) Minor improvements to Crypto input and output streams

["Alejandro Abdelnur (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HADOOP-10632?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14010753#comment-14010753 ] 

Alejandro Abdelnur commented on HADOOP-10632:
---------------------------------------------

Yi, nice work. following some minor comments:

All crypto classes should be annotated as Private has Hadoop is not in the business of exposing crypto APIs as an available crypto library.

JCEAESCTREncryptor/JCEAESCTRDecryptor can be merged into a single class taking the cipher-mode as constructor param. and the encrypt/decrypt would delegate to a single process() method.

AESCTRCryptoCodec#calculateIV() the IV calculation can be done much more efficiently doing some byte shifting:

{code}
  private static final int CTR_OFFSET = 8;
...
    System.arraycopy(initIV, 0, IV, 0, 8);
    long l = (initIV[CTR_OFFSET + 0] << 56)
        + ((initIV[CTR_OFFSET + 1] & 0xFF) << 48)
        + ((initIV[CTR_OFFSET + 2] & 0xFF) << 40)
        + ((initIV[CTR_OFFSET + 3] & 0xFF) << 32)
        + ((initIV[CTR_OFFSET + 4] & 0xFF) << 24)
        + ((initIV[CTR_OFFSET + 5] & 0xFF) << 16)
        + ((initIV[CTR_OFFSET + 6] & 0xFF) << 8)
        + (initIV[CTR_OFFSET + 7] & 0xFF);
    l += counter;
    IV[CTR_OFFSET + 0] = (byte) (l >>> 56);
    IV[CTR_OFFSET + 1] = (byte) (l >>> 48);
    IV[CTR_OFFSET + 2] = (byte) (l >>> 40);
    IV[CTR_OFFSET + 3] = (byte) (l >>> 32);
    IV[CTR_OFFSET + 4] = (byte) (l >>> 24);
    IV[CTR_OFFSET + 5] = (byte) (l >>> 16);
    IV[CTR_OFFSET + 6] = (byte) (l >>> 8);
    IV[CTR_OFFSET + 7] = (byte) (l);
{code}

CryptoInputStream/CryptoOutputStream, besides the MIN_BUFFER_SIZE check we could floor the specified buffer size to a multiple of the CryptoCodec#getAlgorithmBlockSize()

CryptoInputStream/CryptoOutputStream, we should clone the key & initIV as well.

CryptoInputStream#read(), no need for doing {{if (usingByteBufferRead.booleanValue())}}, just do {{if (usingByteBufferRead)}}, 2 places.

CryptoInputStream#readFromUnderlyingStream(), it would be more intuitive to read if the inBuffer is passed as  parameter.

CryptoInputStream, comment "\{@link #org.apache.hadoop.fs.ByteBufferReadable\}" should not have the '#'

CryptoInputStream#decrypt(long position, ...) method, given that this method does not change the current position of the stream, wouldn’t be simpler to create a new decryptor and use a different set of input/output buffers without touching the stream ones? We could also use instance vars for them and init them the first time this method is called (if it is).


> Minor improvements to Crypto input and output streams
> -----------------------------------------------------
>
>                 Key: HADOOP-10632
>                 URL: https://issues.apache.org/jira/browse/HADOOP-10632
>             Project: Hadoop Common
>          Issue Type: Sub-task
>          Components: security
>    Affects Versions: fs-encryption (HADOOP-10150 and HDFS-6134)
>            Reporter: Alejandro Abdelnur
>            Assignee: Yi Liu
>             Fix For: 3.0.0
>
>
> Minor follow up feedback on the crypto streams



--
This message was sent by Atlassian JIRA
(v6.2#6252)