[jira] [Updated] (HADOOP-10632) Minor improvements to Crypto input and output streams
[ https://issues.apache.org/jira/browse/HADOOP-10632?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen Wittenauer updated HADOOP-10632: -- Fix Version/s: (was: 3.0.0) 2.6.0 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: 2.6.0 Attachments: HADOOP-10632.1.patch, HADOOP-10632.2.patch, HADOOP-10632.3.patch, HADOOP-10632.4.patch, HADOOP-10632.patch Minor follow up feedback on the crypto streams -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Updated] (HADOOP-10632) Minor improvements to Crypto input and output streams
[ https://issues.apache.org/jira/browse/HADOOP-10632?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Yi Liu updated HADOOP-10632: Attachment: HADOOP-10632.4.patch Thanks Alejandro, update the patch. 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 Attachments: HADOOP-10632.1.patch, HADOOP-10632.2.patch, HADOOP-10632.3.patch, HADOOP-10632.4.patch, HADOOP-10632.patch Minor follow up feedback on the crypto streams -- This message was sent by Atlassian JIRA (v6.2#6252)
[jira] [Updated] (HADOOP-10632) Minor improvements to Crypto input and output streams
[
https://issues.apache.org/jira/browse/HADOOP-10632?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Yi Liu updated HADOOP-10632:
Attachment: HADOOP-10632.2.patch
Thanks Alejandro for nice review, the new patch includes update for your
comments.
{quote}
CryptoInputStream#freeBuffers(), not sure we should tap into SUN internal APIs
here. do we gain something by cleaning up the buffers? And if we do, we should
first check that the DB is a sun.nio.ch.DB instance. (Sorry, I’ve missed this
one in my first passed)
{quote}
Let's add ByteBuffer instance check.
{quote}
CryptoInputStream#decrypt(), you changed the signature to take the outBuffer as
param, wouldn’t make sense to take both input and output buffers as parameters
then, then the usage form the read-pos would be simpler. What I’m thinking is
that the read-pos/readFully should leave alone all instance vars related to
normal stream reading and use a complete different set of vars that are
instantiated on their first use and reset before every use.
CryptoInputStream, read-pos() and readFully(), wouldn’t we have to consider the
padding based on the requested pos there? If so, the decrypt(), following
previous comment, would have to receive the padding as param as well, no?
{quote}
Now we use a complete different set of vars for read-pos() and readFully(), and
they are thread-safe.
{quote}
On failing the Crypto streams constructors if the buffer size is not a multiple
of the block size, I’ve thought about that initially, but that seemed a too
strong requirement to me, that is why I suggested flooring the request buffer
to the closest previous block size multiple.
{quote}
OK, let's floor the buffer size.
{quote}
Also, the CryptoCodec has some javadoc links that are incorrect, when referring
to classes, don't prefix the class name with #.
{quote}
Right, let's update it.
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
Attachments: HADOOP-10632.1.patch, HADOOP-10632.2.patch,
HADOOP-10632.patch
Minor follow up feedback on the crypto streams
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Updated] (HADOOP-10632) Minor improvements to Crypto input and output streams
[
https://issues.apache.org/jira/browse/HADOOP-10632?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Yi Liu updated HADOOP-10632:
Attachment: HADOOP-10632.3.patch
Hi Alejandro, thanks for your comments. The new patch includes update for your
comments.
{quote}
how the read-pos/readFully is being handled to be thread-safe.
{quote}
We use method local vars for read-pos/readFully.
{quote}
the {{getBufferSize/freeDB/freeBuffers/checkBufferSize}} methods are duplicated
in the both crypto input/output stream, wouldn't make sense to move them to a
CryptoStreamUtils class not to dup code?
{quote}
Right, let's reuse them in {{CryptoStreamUtils}}
{quote}
For read-pos/readFully logic, similarly to the bufferPool for BBs, shouldn't we
have a decryptorPool? As the decryptor creates a cipher and this triggers a JCE
selection and all that stuff in, that may be expensive/synchronized.
{quote}
Right, let's add a {{decryptorPool}}
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
Attachments: HADOOP-10632.1.patch, HADOOP-10632.2.patch,
HADOOP-10632.3.patch, HADOOP-10632.patch
Minor follow up feedback on the crypto streams
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Updated] (HADOOP-10632) Minor improvements to Crypto input and output streams
[
https://issues.apache.org/jira/browse/HADOOP-10632?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Yi Liu updated HADOOP-10632:
Attachment: HADOOP-10632.patch
[~tucu00], thanks for your nice comments. The new patch includes update for
all your comments except the last item which I want to discuss with you.
{quote}
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).
{quote}
I think they each have their advantages. The approach here, doesn’t touch the
stream ones, needs to create a new decryptor, and different set of input/output
buffers, key, iv and some other instance vars.
The code logic of original one is not complicated too, it needs to restore the
{{outBuffer}} and {{decryptor}}, but much less code.
Alejandro, I’d personally like to keep the original one. Do you find some
potential issue?
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
Attachments: HADOOP-10632.patch
Minor follow up feedback on the crypto streams
--
This message was sent by Atlassian JIRA
(v6.2#6252)
[jira] [Updated] (HADOOP-10632) Minor improvements to Crypto input and output streams
[
https://issues.apache.org/jira/browse/HADOOP-10632?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Yi Liu updated HADOOP-10632:
Attachment: HADOOP-10632.1.patch
Hi Alejandro, in the new patch, for {{CryptoInputStream#decrypt(long position,
...)}} method, I use a different output buffer to avoid restoring {{outBuffer}}
which may cause few performance issue of bytes copy. For decryptor, still uses
the existing one.
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
Attachments: HADOOP-10632.1.patch, HADOOP-10632.patch
Minor follow up feedback on the crypto streams
--
This message was sent by Atlassian JIRA
(v6.2#6252)
