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