hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alejandro Abdelnur (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-10632) Minor improvements to Crypto input and output streams
Date Wed, 28 May 2014 04:17:02 GMT

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

Mime
View raw message