search

TritonDataCenter / node-manta

Node.js SDK for Manta
75 stars 54 forks source link

joyent/node-manta#296 add client encryption support #368

Open joyent-automation opened 5 years ago

joyent-automation commented 5 years ago

joyent/node-manta#296 add client encryption support

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/1110/. The raw archive of this CR is here. See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@geek commented at 2016-12-12T23:03:24

Patch Set 1: Code-Review+1

@geek commented at 2016-12-12T23:31:01

Uploaded patch set 2: Patch Set 1 was rebased.

@trentm commented at 2017-01-04T21:09:12

Patch Set 3:

(5 comments)

Wyatt,

Thanks. That's just a start. I haven't read through the actual encryption/decryption code yet, nor run the test suite.

Patch Set 3 code comments
/COMMIT_MSG#8 @trentm

Can an issue be added for this work, please?

/COMMIT_MSG#8 @geek

Fixed: https://cr.joyent.us/#/c/1110/6//COMMIT_MSG

lib/client.js#11 @trentm

Is there a need for adding this dep?

  • 'b64.encode' is just buff.toString('base64'), which could be used directly

  • 'b64.decode' does look like an implementation. Is there a reason to use this implementation vs. node's own? E.g.,

    s = 'the quick ☃ Лорем ипсум' 'the quick ☃ Лорем ипсум' encoded = new Buffer(s, 'utf8').toString('base64') 'dGhlIHF1aWNrIOKYgyAg0JvQvtGA0LXQvCDQuNC/0YHRg9C8' decoded = new Buffer(encoded, 'base64').toString('utf8') 'the quick ☃ Лорем ипсум'

lib/client.js#11 @geek

I am using this implementation because I maintain it, have full test coverage on it, and it will work well with streams if we get to that point. I am happy to remove though if you think it's overkill.

lib/client.js#836 @trentm

Is it agreed that we want to ignore a m-encrypt-support header that is of a type that we don't support?

lib/client.js#908 @trentm

I would love to see all the encryption-specific stuff move to a separate file, e.g. "cse.js". Then that file could have a nice top-block-comment that explained the basics of CSE quickly and pointed to RFD 71.

lib/client.js#993 @trentm

indentation

Patch Set 4 code comments
lib/cse.js#76 @joyent-automation

warning: missing semicolon

lib/cse.js#184 @joyent-automation

warning: missing semicolon

@geek commented at 2017-01-09T19:24:17

Uploaded patch set 6: Commit message was updated.

@jclulow commented at 2017-01-10T09:58:51

Patch Set 8:

(21 comments)

I took an initial look at this. I haven't really any experience with the crypto stuff, so it'd be good to make sure somebody like Alex Wilson takes a look at that.

@trentm commented at 2017-01-10T17:39:02

Patch Set 8:

(1 comment)

Patch Set 8 code comments
lib/client.js#179 @jclulow

I think these option names might be a bit confusing. It seems like "key" and "keyId" might be confused with the HTTP signature auth (SSH key) options in other places. The "cipher" option might conceivably be confused with a TLS option.

Could we prefix these options with something like "encrypt" or perhaps "cse"?

lib/client.js#179 @trentm

I'd be totally cool blessing the name/prefix "cse" in docs and exposed options.

lib/client.js#1865 @jclulow

It doesn't seem like this block changed, except for the added braces. It's best (from a "git blame" perspective) to minimise diff noise like this.

lib/client.js#1877 @jclulow

First up, the indentation here seems a bit off -- the file looks to be four spaces, but this seems to be two spaces?

Also, I don't think this function actually returns a value, so please put the return statements after the callback; e.g.

if (...) {
    cse.encrypt(... {
        if (err) {
            cb(err);
            return;
        }

        doPut(...);
    });
    return;
}
lib/client.js#1879 @jclulow

The "return" statement here is superfluous.

lib/cse.js#0 @jclulow

This file really, really needs some pretty extensive block comments that describe the format of the encrypted object stored in Manta, as well as documentation about all of the headers. The encrypted metadata format should be described as well.

lib/cse.js#50 @jclulow

With each of these, I think the message string can just be the name of the option; e.g.

assert.func(options.getKey, 'options.getKey');

The "assert-plus" module formats the message appropriately; e.g. the above will trip with:

"options.getKey (function) is required"

See also: https://github.com/mcavage/node-assert-plus/blob/30dd5d0/assert.js#L23

lib/cse.js#50 @geek

Done

lib/cse.js#54 @jclulow

This function doesn't appear to be returning a value, so please move the "return" statement down after the call to cb(). Ditto for L58, L60, L65, etc. i.e.,

if (invalidHeaders) {
    cb(...);
    return;
}
lib/cse.js#54 @geek

Done

lib/cse.js#80 @jclulow

See comments below about avoiding "removeAllListeners()".

lib/cse.js#80 @geek

Done

lib/cse.js#81 @jclulow

See comments below about VError wrapping here.

lib/cse.js#81 @geek

Done

lib/cse.js#89 @jclulow

See comments below about avoiding "removeAllListeners()".

lib/cse.js#89 @geek

Done

lib/cse.js#90 @jclulow

See comments below about VError wrapping here.

lib/cse.js#90 @geek

Done

lib/cse.js#93 @jclulow

It seems like we're waiting for the entire file to finish being decrypted before we return the output stream to the caller. Is that right?

If so, where does all of the decrypted data sit? Is it just building up in the stream buffers? Why doesn't backpressure kick in and stop the stream from flowing? What happens if I download a 2GB encrypted file using this mechanism?

lib/cse.js#93 @geek

That is the current implementation. We will need to make sure that this works with the range header as well, which is to say that the implementation isn't 100% done.

Do we have any tests for large files that exist? If not, we should also add large file tests for the normal/unencrypted stream methods.

lib/cse.js#110 @jclulow

It looks like "decryptMetadata()" can potentially call this callback with an error object, but we're not doing anything with it here. Should we be?

lib/cse.js#110 @geek

Done

lib/cse.js#119 @jclulow

As a global constant, it'd be better to name this: "REQUIRED_HEADERS".

lib/cse.js#119 @geek

Done

lib/cse.js#184 @jclulow

See comment above on "assert-plus" messages.

lib/cse.js#184 @geek

Done

lib/cse.js#188 @jclulow

I would not use "assert.ok()" for this, but rather explicitly throw an error. If the user has set "NODE_NDEBUG" in the environment, many (if not all) versions of "assert-plus" will just not check the assertion.

lib/cse.js#188 @geek

Done

lib/cse.js#202 @jclulow

As a general rule, it's best to avoid the use of "removeAllListeners()", especially without any arguments. Some Node classes attach internal housekeeping event handlers that are responsible for cleaning up resources like file descriptors, and "removeAllListeners()" will remove those too.

Instead, I would remove only the specific listeners you added, or explicitly unpipe/destroy/etc the streams here.

lib/cse.js#202 @geek

Done

lib/cse.js#203 @jclulow

It would be good to wrap this error in a VError object that includes some context about the operation, e.g.

cb(new VError(err, 'encrypted upload of "%s": cipher error', path));

(You'd need to pass the object path for that to work, but I think it would be worth it.)

lib/cse.js#203 @geek

Done

lib/cse.js#222 @jclulow

Consider a VError here with some context as well.

lib/cse.js#222 @geek

Done

lib/cse.js#229 @jclulow

It seems like in both cases here, we're encrypting the entire input stream before returning the encrypted output stream to the caller. Is that true? If so, I have the same questions about this as for "decrypt()".

lib/cse.js#229 @geek

Replied above.

lib/cse.js#272 @jclulow

This should perhaps use "hasOwnProperty()" on "CIPHERS" to avoid matching things on the Object prototype, such as "valueOf".

lib/cse.js#272 @geek

Done

@trentm commented at 2017-01-11T00:39:28

Patch Set 11:

(11 comments)

Patch Set 11 code comments
lib/client.js#179 @trentm

Apologies for waffling: I'd like to change my opinion w.r.t using the "cse" prefix. Given that the headers are "m-encrypt-*", I think the exposed options should be:

encryptKeyId encryptKey encryptGetKey

etc.

lib/client.js#843 @trentm

I think it would be nice to be able to specify the "cse_getKey" on the MantaClient constructor. Then one could:

client = manta.createClient({
    user: this.user,
    // ...
    cse_getKey: getMyEncryptionKey
});

// All `client.get's` would now decrypt without me having to pass around the
// ref to `getMyEncryptionKey`.

Here is a (untested) starter patch to the current patchset 11 code to enable that:

https://gist.github.com/trentm/ec5a27360ac4b4e913f5e3736fd1090e

The "!== undefined" check would allow one to pass options.cse_getKey: null to client.get to disable decryption for just that call. There may be better suggestions on how to have that expressed.

If this sounds reasonable, then I think we could do that same for encryption: allow specifying the encryption options (keyId, key, etc.) in the manta.createClient.

lib/client.js#843 @geek

Updated in patch 12: https://cr.joyent.us/#/c/1110/12/lib/client.js

I will update the other options to support being set in the constructor of the client

lib/cse.js#4 @trentm

Per earlier comments, this file needs a top block-comment explaining the file's scope and the basics of CSE, likely referring to the spec. It might make sense to remove the "Node.js SDK implementation" section from RFD 71 and put the implementation spec in this file's top-comment, and client user docs in the README.md

lib/cse.js#9 @trentm

This is in node 0.10 and that is our min-engine. So we can drop this if-block.

lib/cse.js#9 @geek

mind if I fix this in other modules as well? I saw another comment about not adding more churn than we need... but I agree with you that we need to fix issues like this.

lib/cse.js#13 @trentm

Drop 'b64' module usage per earlier comments.

lib/cse.js#13 @geek

This will be done in patch 15.

lib/cse.js#16 @trentm

This whole section of requires could be the following (Just get VError directly, don't need verror. Alphabetical order.):

var assert = require('assert-plus');
var crypto = require('crypto');
var PassThrough = require('stream').PassThrough;
var VError = require('verror').VError;
lib/cse.js#16 @geek

What is our standard for require ordering and shortcuts? I was following the style in client.js

lib/cse.js#45 @trentm

FWIW, this accepts "1blahblahblah"

> parseInt('1asdfa', 10)
1

I think it would be nice to have a method in this module that validates the 'm-encrypt-type' header matches a regex, e.g.:

/^(\w_-)+\/(\d+)\.(\d+)$/
lib/cse.js#45 @geek

Done

lib/cse.js#51 @trentm

This function still has the streaming issue that jclulow commented on.

lib/cse.js#51 @geek

This is fixed in the most recent patch.

lib/cse.js#53 @trentm

nit: Should there be an assert for 'res'?

lib/cse.js#53 @geek

Done

lib/cse.js#101 @trentm

Parens in wrong place here.

lib/cse.js#101 @geek

Done

lib/cse.js#194 @trentm

I think we shouldn't set a value on 'headers' until after the MAC check is done.

lib/cse.js#194 @geek

Done

Patch Set 12 code comments
lib/client.js#822 @trentm

Thoughts on the comment I had that doing an 'undefined' check would allow the caller to disable decryption for a particular call? (Actually, if we do allow this kind of mechanism via client.get(..., {cse_getKey: null}, ...) then I think a check using opts.hasOwnProperty('cse_getKey') would be better. That same comment I had before about perhaps choosing a more explicit additional boolean option to "do encryption work or not" might still be preferred).

Also, doing a typeof===function check here will mean that incorrectly passing in some other type of thing (i.e. a programming error) will silently be ignored.

@trentm commented at 2017-01-16T20:19:14

Patch Set 17:

(20 comments)

Wyatt,

I'm running out of steam on this review round, so I want to get these comments out now. I'll have another email with some Qs about this code and the RFD as well.

Patch Set 17 code comments
lib/cipher_stream.js#1 @trentm

nit: Apparently from some lawyer-y interaction, jclulow learned there are issues with the "(c)" and the "All rights reserved." such that we want the form to be something like:

Copyright 2017 Joyent, Inc.
lib/cipher_stream.js#4 @trentm

nit: alpha order for imports please.

lib/cipher_stream.js#7 @trentm

Perhaps a better name for this class would be something like ParseEncryptThenMacStream or ParseEtMStream (https://en.wikipedia.org/wiki/Authenticated_encryption#Encrypt-then-MAC_.28EtM.29).

lib/cipher_stream.js#8 @trentm

assert. calls for the args would be great.

lib/client.js#174 @trentm

We shouldn't be morphing the passed in opts and userOpts in here. It is just about building the merged options and returning that.

lib/client.js#174 @geek

Done

lib/client.js#186 @trentm

typo

Also, what is the 'hmac' option used for? I don't recognize that from earlier patchsets.

lib/client.js#186 @geek

Done

lib/client.js#515 @trentm

Should have a comment about options.encrypt here. Also, we want it to effectively default to "false" right?

lib/client.js#515 @geek

The default will be to not set encrypt, which won't disable encryption.

lib/client.js#528 @trentm

This'll break if options.encrypt isn't specified, right?

lib/client.js#528 @geek

fixed

lib/client.js#548 @trentm

This doesn't default to false if options.encrypt isn't specified. It should though, right?

lib/client.js#548 @geek

false indicates that you want to disable encryption, so it will default to an empty object, meaning use whatever is passed to get/put

lib/client.js#553 @trentm

I think this could be simplified to:

if (options.encrypt) { assert.optionalFunc(options.encrypt.getKey, 'options.encrypt.getKey'); }

And then in that if-block we could add asserts for the other options.encrypt object fields.

lib/client.js#553 @geek

Done

lib/client.js#860 @trentm

Would be nice to move parsing details of the m-encrypt-type header into the cse.js file.

lib/client.js#860 @geek

Done

lib/client.js#1896 @trentm

Replace with just options.encrypt?

lib/client.js#1896 @geek

Done

lib/client.js#1897 @trentm

I think it would be preferable to do assert checking on options.encrypt.keyId at the top of client.put and then here we'd just need to check:

if (options.encrypt && options.encrypt.keyId) {
lib/client.js#1897 @geek

Done

lib/create_client.js#122 @trentm

The RFD has a m-encrypt-hmac-type header now (it used to be m-encrypt-hmac I believe). Is that what this option is about? If so, I'd suggest it be called 'hmacType' to follow the header naming.

lib/create_client.js#122 @geek

Done

lib/cse.js#95 @trentm

I think it would be better if these errors were before the async call to options.getKey(). I.e. it is a shame to have some possibly pausing/slow/expensive/prompting action when things are just going to fail for some basic data type/format error afterwards.

lib/cse.js#95 @geek

Done

lib/cse.js#166 @trentm

Need a block comment doc'ing the options.

lib/cse.js#166 @geek

Done

lib/cse.js#167 @trentm

Perhaps the only options to this one should be the 'encrypt' object. Instead of it being 'options.encrypt.FOO' it would be 'options.FOO'.

That would correlate better with cse.decrypt which takes options.getKey. Ah, I see that would be a bit of a pain given other options like 'options.contentLength'. Perhaps cse.decrypt could change to take options.encrypt.FOO to match.

lib/cse.js#167 @geek

I simplified this on the encrypt side, see what you think.

lib/cse.js#174 @trentm

Missing. assert for options.contentLength used below.

lib/cse.js#174 @geek

Done

lib/cse.js#206 @trentm

Could this just be an assert? If this test fails, then it is a programming error in this constants in this module.

lib/cse.js#206 @geek

Done

Patch Set 18 code comments
lib/client.js#555 @joyent-automation

warning: missing semicolon

lib/cse.js#301 @joyent-automation

warning: empty statement or extra semicolon

Patch Set 19 code comments
lib/client.js#555 @joyent-automation

warning: missing semicolon

lib/cse.js#301 @joyent-automation

warning: empty statement or extra semicolon

Patch Set 20 code comments
lib/client.js#555 @joyent-automation

warning: missing semicolon

lib/cse.js#331 @joyent-automation

warning: empty statement or extra semicolon

Patch Set 21 code comments
lib/client.js#170 @trentm

nit: this allows null and empty string. Validation of falsey values is such a pain with JS. :)

It would be nice to assert the types of any userOpts.encrypt fields in this function, assuming that won't be done in the methods calling createOptions.

lib/client.js#170 @geek

That is considered not set in this case, which will default the value to an object on line 173.

lib/client.js#183 @trentm

I don't believe this handles options.encrypt properly yet. Using a test enc.js (https://gist.github.com/trentm/c453ec712d0a8cdb42de93d646681706) which is an extract of createOptions here, the results are:

$ node enc.js
--
opts: {}
userOpts: {}
encrypt: { cipher: undefined,
  keyId: undefined,
  key: undefined,
  getKey: undefined,
  hmacType: undefined }
--
opts: { encrypt: false }
userOpts: { encrypt: false }
encrypt: false
--
opts: { encrypt: false }
userOpts: { encrypt: { cipher: 'foo' } }
encrypt: false

The third one is definitely wrong: the userOpts.encrypt should win here.

The first case is contrived, but it would be nice to not result in an object with undefined properties.

lib/client.js#183 @geek

The third case is someone explicitly saying that they don't want encryption enabled for the entire client instance that they create. Therefore, any attempts to set it with user options will be ignored.

In the first case what values would you like to see?

lib/client.js#528 @trentm

Still need to remove this assert. I'd expect this would break some of the test suite, no?

lib/client.js#548 @trentm

This defaults to {}, not false. I'd be inclined to have it default to this.encrypt = false.

lib/client.js#548 @geek

false indicates that you want to disable encryption entirely, in this case not-setting the options mean that you don't care if encryption is enabled or not.

lib/cse.js#220 @trentm

There are other supported ciphers, right? If so, I think we should be pointing at whereever the authoritative source of supported ciphers will be. Dunno if that'll be in node-manta or the RFD or somewhere else.

lib/cse.js#237 @trentm

Should be the string 'options.headers'.

lib/cse.js#237 @geek

good catch! seems like assert needs to assert their arguments

@trentm commented at 2017-01-19T23:20:41

Patch Set 21:

(6 comments)

Looking good. Still waiting for AEAD support, right? Perhaps that is patchset 22 which came in while I was reviewing patchset 21.

@trentm commented at 2017-01-19T23:57:00

Patch Set 22:

(3 comments)

It woudl be good to see some sample code (doesn't need to be in the regular test suite) that demostrated doing enc/dec of a large file (multi-MB up to 1GB) to/from manta.

Patch Set 22 code comments
lib/cse.js#169 @trentm

"Only called for AEAD ciphers."

lib/cse.js#169 @geek

Done

lib/cse.js#172 @trentm

Delaying the piping to output until we've parsed the full ciphertext breaks streaming large files, no?

I haven't played with this (or with node's crypto.Decipher), but from the docs is sounds like we want to stream through to the output stream, and when we get the auth tag call setAuthTag before decipher.final() is called. I don't know if we need to call decipher.final() manually (and in a try/catch to trap an auth tag error).

Have you got this to work for you?

lib/cse.js#172 @geek

Working on this update now. Yesterday was getting it to work in general.

lib/cse.js#229 @trentm

AEAD

Patch Set 29 code comments
bin/mget#176 @joyent-automation

warning: identifer err hides an identifier in a parent scope

lib/cse.js#214 @joyent-automation

warning: undeclared identifier: handlePassthroughData

lib/cse.js#214 @joyent-automation

warning: undeclared identifier: passthrough

lib/cse.js#215 @joyent-automation

warning: undeclared identifier: handlePassthroughEnd

lib/cse.js#215 @joyent-automation

warning: undeclared identifier: passthrough

@trentm commented at 2017-01-27T00:29:10

Patch Set 35:

(4 comments)

play notes

export MANTA_ENCRYPTION_KEY_BYTES=$(echo -n aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | base64)
export MANTA_CLIENT_ENCRYPTION_KEY_ID=a
export MANTA_ENCRYPTION_ALGORITHM=AES256/GCM/NoPadding
echo 'hi there' > hello.txt
mpath=~~/stor/tmp/enc/hello.txt
mmkdir -p ~~/stor/tmp/enc

mput -f hello.txt $mpath --encrypt
minfo $mpath
mget $mpath

review notes

  • It would be nice to have more consistent envvar prefixes. Perhaps use MANTA_ENCRYPT_ is the prefix for all of them: MANTA_ENCRYPT_KEY (I dropped the "_BYTES") MANTA_ENCRYPT_KEY_ID MANTA_ENCRYPT_ALGORITHM

  • Likewise for the CLI options. Perhaps: -E, --encrypt --encrypt-key --encrypt-key-id --encrypt-algorithm

  • I'm unable to mget an encrypted file without all the params to decrypt it:

    $ unset MANTA_ENCRYPTION_ALGORITHM
$ unset MANTA_ENCRYPTION_KEY_BYTES
$ unset MANTA_CLIENT_ENCRYPTION_KEY_ID
$ mget $mpath
mget: VError: failed executing options.getKey: --enc_key or MANTA_ENCRYPTION_KEY_BYTES not found

    I wonder if there should be a '-E,--encrypt' option for 'mget', just as there is for 'mput'. There could be a matching MANTA_ENCRYPT=1 envvar to allow turning that on all the time.

  • Should have a test case for authMode.

  • Should there be a guard on size of metadata to encrypt/decrypt?

  • Should enc alg be case-insensitive? $ mput -f hello.txt /trent.mick/stor/tmp/enc/hello.txt --encrypt --enc_alg aes128/gcm/nopadding --enc_key secret --enc_keyid a mput: Error: Unsupported cipher algorithm: aes128/gcm/nopadding

  • Error for key length: $ mput -f hello.txt /trent.mick/stor/tmp/enc/hello.txt --encrypt --enc_alg AES128/GCM/NoPadding --enc_key secret --enc_keyid a mput: Error: Invalid key length What is a valid key length? It would be helpful if the error message caught this and gave details for the given alg.

  • Should we have 'm-encrypt-metadata' headers when no extra 'e-' metadata is given? Using my play notes below:

    $ minfo $mpath
...
m-encrypt-metadata-iv: +p9UN620NurC/h06D4fuVg==
m-encrypt-metadata-aead-tag-length: 16
m-encrypt-metadata: EXDfHe6JKFennviGmq3F7nBxvx0lV2f3chEl3nPsEdAzFTbA/nDuq7Lt
...

    My guess is this has something to do with the e-content-type handling. However, if no content-type is specified with the mput, I think we should have no value there. This shows that mput doesn't send a content-type header:

    $ mput -v -f hello.txt $mpath --encrypt 2> >(bunyan)
[2017-01-27T00:16:41.902Z] DEBUG: mput/MantaClient/27470 on danger0.local (/Users/trentm/joy/node-manta/lib/client.js:1149 in info): info: entered (req_id=740cd83f-3143-4d55-98be-d41fa33ea2bc, path=/trent.mick/stor/tmp/enc/hello.txt, id=740cd83f-3143-4d55-98be-d41fa33ea2bc, query={}, encrypt={})
    headers: {
      "accept": "application/json, */*",
      "x-request-id": "740cd83f-3143-4d55-98be-d41fa33ea2bc"
    }
[2017-01-27T00:16:41.958Z] TRACE: mput/MantaClient/27470 on danger0.local (/Users/trentm/joy/node-manta/node_modules/restify-clients/lib/HttpClient.js:314 in rawRequest): request sent
    HEAD /trent.mick/stor/tmp/enc/hello.txt HTTP/1.1
    Host: us-east.manta.joyent.com
    accept: application/json, */*
    x-request-id: 740cd83f-3143-4d55-98be-d41fa33ea2bc
    date: Fri, 27 Jan 2017 00:16:41 GMT
    authorization: Signature keyId="/trent.mick/keys/...
    user-agent: restify/1.4.1 (x64-darwin; v8/4.5.103.43; OpenSSL/1.0.2j) node/4.7.0
    accept-version: ~1.0
...

  • e-content-type isn't being handled properly. I added the following patch to mget to dump the response headers:

    diff --git a/bin/mget b/bin/mget
index 33e3338..c5561a8 100755
--- a/bin/mget
+++ b/bin/mget
@@ -160,6 +160,8 @@ function printEntry(obj) {
         client.get(p, function (err, stream, res) {
             ifError(err);

+           console.log('XXX res.headers', res.headers);
+
             var bar;
             var src = stream;
             if (opts.progress || drawProgressBar) {

    and:

    $ mput -f hello.txt $mpath  -H content-type:foo/bar --encrypt
$ mget $mpath
XXX res.headers { etag: '3fbfc5ba-8a5b-65fb-ec3e-b429dd3322aa',
  'last-modified': 'Fri, 27 Jan 2017 00:21:11 GMT',
  'm-encrypt-aead-tag-length': '16',
  'm-encrypt-plaintext-content-length': '9',
  'm-encrypt-type': 'client/1',
  'm-encrypt-key-id': 'a',
  'm-encrypt-iv': 'wQqitWFDHSw8JX1NvMDcrA==',
  'm-encrypt-cipher': 'AES256/GCM/NoPadding',
  'accept-ranges': 'bytes',
  'content-type': 'application/octet-stream',
  'content-md5': 'F9XUK9Nd14ys4b/Tk/S2ag==',
  'content-length': '25',
  'durability-level': '2',
  date: 'Fri, 27 Jan 2017 00:21:27 GMT',
  server: 'Manta',
  'x-request-id': 'f0b758c1-a820-4bcb-a191-c9bc20d451ac',
  'x-response-time': '23',
  'x-server-name': '07188c71-5cb5-497c-b5ed-3e82654aa9d0',
  connection: 'keep-alive',
  'x-request-received': 1485476486721,
  'x-request-processing-time': 411,
  'e-content-type': 'undefined',
  '': undefined }
hi there

    a few things:

    • The "content-type: foo/bar" isn't restored.
    • There is the bogus '': undefined entry.
    • 'e-content-type': 'undefined' shouldn't be there either.
@trentm commented at 2017-01-31T00:47:36

Patch Set 35:

Woudl also be good if the new test files used a self-identifying subdir for test files:

[trent.mick@172.26.1.149 /trent.mick/stor]$ ls
encrypted
metadata
node-manta-test-mfind-072df349
todecrypt-aead
todecrypt-metadata
todecrypt-range
todecrypt_getKey

Note that the mfind test uses a 'node-manta-test-'-prefixed dir for its created files.

Patch Set 35 code comments
lib/client.js#1898 @trentm

I don't think 'createOptions' adds a 'options.contentType'.

lib/cse.js#126 @trentm

is

lib/cse.js#494 @trentm

indentation

lib/parse_etm_stream.js#8 @trentm

"authTag" or "tag"

@trentm commented at 2017-02-22T01:02:46

Patch Set 36:

Wyatt,

Let me know if/when you'd like me to review again. Given that we are on patchset 36 :) I'll wait until there is a little bit of a steady state.

@trentm commented at 2017-03-06T23:32:57

Patch Set 37:

(3 comments)

[RFD 71]

m-encrypt-type doesn't exist or doesn't have the value 'client/VERSION', in which case the object is assumed not to be encrypted, and the usual processing of the file occurs, without decryption. In the event that the version isn't supported by the client, it should not try to decrypt the file and should return the encrypted form of the file.

If there is a file with m-encrypt-type: client/42, i.e. that matches the "client/VER" scheme, but isn't supported by the client... and the user has explicitly request encryption be done (via whatever option or env)... perhaps we want that to fail? I agree that if it is some other scheme m-encrypt-type: foo/42, then we should ignore it.

The RFD states that we error out if m-encrypt-cipher isn't a supported cipher. I'd think we want to error for an unsupported client/VER as well.

Either way, the node-manta docs (README and/or man page, and docs) should state the current support "client/VER" version supported.

  • Doc additions are necessary to explain CSE usage and options. E.g., hmacType and authMode aren't documented anywhere.

  • It would be nice to have a better error message for encryption key length error:

    $ MANTA_ENCRYPT_KEY=asdf mput -f hello ~~/stor/tmp/enc/hello --encrypt mput: Error: Invalid key length

  • "hmacType" is not exposed to the CLI.

  • "authMode" is not exposed to mget.

  • Should there be a guard on size of metadata to encrypt/decrypt?

  • mget/mput suggested changes: I've made a patch, mainly to mget and mput, for some suggested changes: https://gist.github.com/trentm/5c778fe4eead4a58232e2c2dbc6096ed

    • use the 'env' option to option processing
    • A start at changing from "--encrypt-algorithm" to "--encrypt-cipher", to match the internal name. Only a "start" because I haven't changed the man page files.
    • Add "mget --include" option (a la curl's -i/--include) to dump the headers. This allows seeing the decrypted e-* headers.
    • Put CSE options into an option group.
    • "-e" short option for mput's "--encrypt"
    • Add a "--decrypt, -e" option to "mget" and make decryption explicit and off by default. That then matches "mput" behaviour: explicit, off by default.
    • Rephrase the "NODE_MAJOR" stuff in lib/cse.js

  • mget: content-length is not restored after decryption, nor is the plaintext content-md5 (re)stored. Do we want to? RFD says: "SDKs may overwrite the value of Content-Length returned from the client API with the value of m-encrypt-plaintext-content-length." It doesn't mention content-md5.

Patch Set 37 code comments
bin/mput#274 @trentm

The changes here are wrong. The onPut doesn't get called until the upload has completed.

lib/cse.js#398 @trentm

This code will accidentally support 'client/1/blah'. You could add the strsplit dep (https://github.com/davepacheco/node-strsplit) and use:

var parts = strsplit(headers['m-encrypt-type'], '/', 2);
lib/parse_etm_stream.js#1 @trentm

A nit on the name: I wonder if that is no longer about EtM, so my bad on that name suggestion. After that, appending authTag for AEAD was added, so really this is now more generically about parsing off a suffix number of bytes from a stream.

@trentm commented at 2017-03-20T22:39:08

Patch Set 38:

(9 comments)

from patchset 37

  • behaviour for m-encrypt-type:client/42

    [RFD 71]

    m-encrypt-type doesn't exist or doesn't have the value 'client/VERSION', in which case the object is assumed not to be encrypted, and the usual processing of the file occurs, without decryption. In the event that the version isn't supported by the client, it should not try to decrypt the file and should return the encrypted form of the file.

    If there is a file with m-encrypt-type: client/42, i.e. that matches the "client/VER" scheme, but isn't supported by the client... and the user has explicitly request encryption be done (via whatever option or env)... perhaps we want that to fail? I agree that if it is some other scheme m-encrypt-type: foo/42, then we should ignore it.

    The RFD states that we error out if m-encrypt-cipher isn't a supported cipher. I'd think we want to error for an unsupported client/VER as well.

    Either way, the node-manta docs (README and/or man page, and docs) should state the current support "client/VER" version supported.

  • Doc additions are necessary to explain CSE usage and options. E.g., hmacType and authMode aren't documented anywhere.

  • It would be nice to have a better error message for encryption key length error:

    $ MANTA_ENCRYPT_KEY=asdf mput -f hello ~~/stor/tmp/enc/hello --encrypt
mput: Error: Invalid key length

  • Should there be a guard on size of metadata to encrypt/decrypt?

  • mget: content-length is not restored after decryption, nor is the plaintext content-md5 (re)stored. Do we want to? RFD says: "SDKs may overwrite the value of Content-Length returned from the client API with the value of m-encrypt-plaintext-content-length." It doesn't mention content-md5.

patchset 38

  • RFD and java-manta use "AES128/GCM/NoPadding" as opposed to "AES128/GCM/NOPADDING". It would be nice (for grepping, readability, interop) to stick to that capitalization in the node-manta code instead of all caps.

  • java-manta (per its README.md) has a different take on envvars. It would be nice to coordinate those.

    java-manta node-manta
    MANTA_CLIENT_ENCRYPTION (no equiv)
    MANTA_CLIENT_ENCRYPTION_KEY_ID MANTA_ENCRYPT_KEY_ID
    MANTA_ENCRYPTION_ALGORITHM MANTA_ENCRYPT_CIPHER
    MANTA_UNENCRYPTED_DOWNLOADS (no equiv)
    MANTA_ENCRYPTION_AUTH_MODE MANTA_ENCRYPT_AUTH_MODE
    MANTA_ENCRYPTION_KEY_PATH (no equiv)
    MANTA_ENCRYPTION_KEY_BYTES MANTA_ENCRYPT_KEY
    (no equiv) MANTA_ENCRYPT_HMAC

    A few questions here for java-manta:

    1. Does using java-manta as a library pick up MANTA_ environment variables by default? If so, is that avoidable when using java-manta? IMHO, an app that uses a library (like java-manta) as an implementation detail shouldn't respond to envvars with MANTA_.
    2. Would java-manta consider and would it be possible to change the names of some of these envvars? To (a) get java-manta and node-manta to use the same name for the same thing, (b) get a common prefix for all encryption-related envvars, and (c) possibly switch to the shorter MANTA_ENCRYPT_ prefix that node-manta is currently using. (a) and (b) are more important than (c).
    3. _CIPHER instead of _ALGORITHM? The RFD calls these things "ciphers" https://github.com/joyent/rfd/blob/master/rfd/0071/README.md#supported-ciphers
    4. MANTA_ENCRYPTION_ALGORITHM has a default. In my experience that leads to maintenance problems down the road. Admittedly that may be a long road. Specifically, what happens if/when the default (currently "AES128/CTR/NoPadding") is considered insecure. We can't change the default without breaking code, and then we are forced (without a major ver bump) to ship code that is "insecure" by default. Is there harm is not having a default? I.e. force the user to learn about the ciphers and choose one.

    Are you able to follow up with whoever is handling the java-manta work (is that ElijahZ)?

  • java-manta has manta.permit_unencrypted_downloads=false by default. The somewhat equivalent for node-manta's CLI is that if mget -e is used to explicitly use encryption, then should it error by default if the downloaded file is not encrypted?

  • I wonder if mput --encrypt-hmac HMAC ... should be --encrypt-hmac-type to be clearer.

  • node-manta authMode input handling could be more strict. Currently:

    $ MANTA_ENCRYPT_AUTH_MODE=asdf mget -e --include $mpath
XXX isStrictMode false
...

    Some suggestions in a patch: https://gist.github.com/trentm/dbd0e09a43d6da2bdd2964583bce094d

  • There should be tests around authMode=OptionalAuthentication and range GETs. Currently I suspect that isn't working: it'll fail on checking the HMAC.

Patch Set 38 code comments
bin/mget#49 @trentm

"MODE" would be fine to keep it shorter, I think.

See patch.

bin/mget#50 @trentm

This is wrong, IIUC. Authenticating is about the authTag (for AEAD) and the separate HMAC (i.e. the "M" in "EtM") for non-AEAD ciphers.

See patch.

bin/mget#52 @trentm

This help string should list the other option, "OptionalAuthentication", as well.

See patch.

bin/mput#53 @trentm

We should show valid values for this here.

docs/man/mput.md#59 @trentm

" CIPHER"

docs/man/mput.md#65 @trentm

Capitalize "KEY"

docs/man/mput.md#65 @geek

this is inconsistent with the other options (login, file)

lib/cse.js#244 @trentm

I don't think we want to skip this error on authMode=OptionalAuthentication. IIUC, the intent is to allow things like Range requests, where we don't have the full text with which to do authentication. However, if we can, then we should.

lib/cse.js#254 @trentm

Ditto for isStrictMode node a few lines earlier.

lib/cse.js#322 @trentm

Would be nice if this returned valid hmacType values. Perhaps change getHmacType to throw an error with these details (it is the function that knows how to list them).

Patch Set 39 code comments
bin/mget#69 @joyent-automation

warning: trailing_comma

lib/cse.js#598 @joyent-automation

warning: identifer hmac hides an identifier in a parent scope

@trentm commented at 2017-03-22T20:45:06

Patch Set 40:

(2 comments)

Looks like patchset 39-40 only partially worked through comments on patchset 38. Please let me know when you've finished working through them all.

Patch Set 40 code comments
bin/mget#37 @trentm

Oh, obviously this was incomplete.

docs/man/mput.md#59 @trentm

not answered from previous review.

docs/man/mput.md#59 @geek

the other options are lowercase

@geek commented at 2017-03-23T19:58:48

Patch Set 41: Code-Review+1

(50 comments)

@geek commented at 2017-03-23T20:26:24

Patch Set 42: Code-Review+1

@geek commented at 2017-03-23T20:26:37

Patch Set 42: -Code-Review

Question mget: content-length is not restored after decryption, nor is the plaintext content-md5 (re)stored. Do we want to? RFD says: "SDKs may overwrite the value of Content-Length returned from the client API with the value of m-encrypt-plaintext-content-length." It doesn't mention content-md5.

Answer This will require the headers to be displayed after the file contents. The content-type is changed, but after the response body is decrypted. I vote to not break the current behavior.

Question java-manta has manta.permit_unencrypted_downloads=false by default. The somewhat equivalent for node-manta's CLI is that if mget -e is used to explicitly use encryption, then should it error by default if the downloaded file is not encrypted?

Answer I vote to keep the current implementation and not error when a file isn't encrypted. Otherwise, each client/person must make an extra request for anything they aren't sure is encrypted... currently they can just pass along their key and get back the original file, even if the file isn't encrypted.

Question I wonder if mput --encrypt-hmac HMAC ... should be --encrypt-hmac-type to be clearer.

Answer --encrypt-hmac is inline with --encrypt-cipher, as opposed to --encrypt-cipher-algorithm to distinguish it from --encrypt-cipher-text