S3 Client Side Encryption by kstich · Pull Request #1395 · aws/aws-sdk-php

kstich · 2017-10-13T18:39:27Z

Adds the S3EncryptionClient and supporting implementations. This implements envelope encryption and utilizes OpenSSL to provide CBC and GCM (on PHP >= 7.1) support . Uses pluggable strategies (via MetadataStrategyInterface) for handling a MetadataEnvelope in conjunction with a MaterialsProvider. A KmsMaterialsProvider is provided for key management . Supports ->putObject[Async] and ->getObject[Async] operations. This is cross compatible with Java, Ruby, Go, and C++ for the provided feature set. A service guide is also included.

/cc @jeskew @mtdowling @cjyclaire

Resolves #831

… KMS to provide abstract pieces of MaterialsProvider.

…nts.

…provided AesStreamInterface based classes.

…tadata Headers.

…on file on S3.

…strategies for handling a MetadataEnvelope in conjunction with a MaterialsProvider. Supports putObject[Async] and getObject[Async] operations.

…related tests.

codecov-io · 2017-10-13T18:54:20Z

Codecov Report

Merging #1395 into master will decrease coverage by 0.33%.
The diff coverage is 95.91%.

@@             Coverage Diff              @@
##             master    #1395      +/-   ##
============================================
- Coverage      91.9%   91.56%   -0.34%     
- Complexity     2472     2521      +49     
============================================
  Files           142      156      +14     
  Lines          7545     7045     -500     
============================================
- Hits           6934     6451     -483     
+ Misses          611      594      -17

Impacted Files	Coverage Δ	Complexity Δ
src/TraceMiddleware.php	`96.77% <ø> (+0.62%)`	`47 <0> (ø)`	⬇️
src/S3/Crypto/HeadersMetadataStrategy.php	`100% <100%> (ø)`	`5 <5> (?)`
src/S3/Crypto/InstructionFileMetadataStrategy.php	`100% <100%> (ø)`	`6 <6> (?)`
src/Crypto/KmsMaterialsProvider.php	`100% <100%> (ø)`	`8 <8> (?)`
src/Crypto/Cipher/Cbc.php	`100% <100%> (ø)`	`10 <10> (?)`
src/Crypto/MetadataEnvelope.php	`100% <100%> (ø)`	`7 <7> (?)`
src/Crypto/MaterialsProvider.php	`100% <100%> (ø)`	`3 <3> (?)`
src/Crypto/DecryptionTrait.php	`92.18% <92.18%> (ø)`	`11 <11> (?)`
src/Crypto/AesGcmDecryptingStream.php	`93.1% <93.1%> (ø)`	`7 <7> (?)`
src/Crypto/AesGcmEncryptingStream.php	`93.54% <93.54%> (ø)`	`8 <8> (?)`
... and 137 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ba6da5...29bfb98. Read the comment docs.

kstich · 2017-10-13T19:08:54Z

Codecov report of decreased coverage is incorrect. Details show that 95.99% of the diff is covered which is higher than the current 91.90% coverage.

jeskew · 2017-10-28T16:57:16Z

.travis.yml


 after_success:
-  - bash <(curl -s https://codecov.io/bash)
+  - 'if [ $(phpenv version-name) == "7.1" ]; then bash <(curl -s https://codecov.io/bash); fi'


I think this may be to blame for the purported coverage drop. There may be code paths not taken in PHP 7.1

Yeah, it looks like CodeCov is taking the first report and using that, which explains the wild shifts as well. I'm going to set this up to use 7.1 with latest dependencies only, putting us on stable grounds that we can update with further PHP releases.

jeskew · 2017-10-30T16:24:53Z

features/s3Encryption/encryption.feature

+Feature: S3 Client Side Encryption
+
+  Scenario: Upload PHP's GCM encrypted fixtures
+    When I get all fixtures for "aes_gcm" from "aws-s3-shared-tests"


Is this the name of a particular bucket? Including a resource identifier here seems fragile.

It is a bucket name and it is fragile. This is how the AWS SDK for Go handles it and we do something similar for the other full integration tests as well. These tests need to talk to other SDKs and don't get a getResourcePrefix added. I'm open to suggestions on this.

The S3 test should be creating and deleting buckets via BeforeSuite and AfterSuite hooks. I understand that this test suite has to work this way to integrate with other SDK tests, but maybe in the future there could a test that works on multiple accounts.

jeskew · 2017-10-30T16:34:18Z

src/Crypto/EncryptionTrait.php

+            case 'gcm':
+                $cipherOptions['TagLength'] = 16;
+
+                $taggedStream = new AesTaggedGcmEncryptingStream(


Couldn't AesTaggedGcmEncryptingStream be removed if you just used an AppendStream here?

jeskew · 2017-10-30T16:35:14Z

src/Crypto/KmsMaterialsProvider.php

@@ -0,0 +1,109 @@
+<?php
+


Very nit but passim: I believe SDK style requires no newline between the file opener and the namespace declaration.

jeskew · 2017-10-30T16:37:24Z

src/Crypto/KmsMaterialsProvider.php

+
+        return new KmsMaterialsProvider(
+            $this->kmsClient,
+            $materialsDescription['kms_cmk_id']


What's a cmk? Could abbreviations be avoided in this parameter name, or is the expanded version too long to reasonably type?

'kms_cmk_id' is how the key is indexed in the materials description, this is consistent with the Java (and other) SDK implementations. The 'KeyId' naming on Encrypt is used for the $kmsKeyId variable.

jeskew · 2017-10-30T16:40:22Z

src/S3/Crypto/InstructionFileMetadataStrategy.php

+     */
+    public function save(MetadataEnvelope $envelope, array $args)
+    {
+        $this->client->putObject([


What if saving this instruction file fails? Is the ciphertext blob deleted from S3?

The $strategy->save() call is made before the PutObject call for the ciphertext. The instruction file will remain on S3 if there is a failure after it has been uploaded, not automatically deleted. I'll add a note to the guide and update the doc block for this file.

jeskew · 2017-10-30T16:46:06Z

src/S3/Crypto/S3EncryptionClient.php

+
+        $envelope = new MetadataEnvelope();
+
+        return Promise\promise_for($this->encrypt(


encrypt isn't returning a promise, is it?

It is not returning a promise, but an AesStreamInterface. This is why it's the only piece wrapped in the Promise\promise_for call.

I guess it's more of a style issue, but why does this need to be in an async context at all? With the exception of $this->client->putObjectAsync, this chain of functions seems to be operating synchronously and then returning non-promise values. Wouldn't it be easier to read if it was just iterative statements that returned the promise from putObjectAsync at the end?

The thought on async basing the manipulation was if someone invokes $s3EncryptionClient->putObjectAsync(), let's defer the heavy lifting that makes up the encryption work and other calls until they are executed. If someone invokes $s3EncryptionClient->putObject(), the work is immediately waited for anyways.

gotcha that makes sense

jeskew · 2017-10-30T16:52:29Z

tests/Crypto/AesEncryptionStreamTestTrait.php

+    public function plainTextProvider() {
+        return [
+            [Psr7\stream_for('The rain in Spain falls mainly on the plain.')],
+            [Psr7\stream_for('Ø¯Ø³Øªâ€ŒÙ†ÙˆØ´ØªÙ‡â€ŒÙ‡Ø§ Ù†Ù…ÛŒâ€ŒØ³ÙˆØ²Ù†Ø¯')],


I believe this is a copy-paste error. IIRC, this case (and the following one) were meant to exercise round-tripping of multibyte characters, e.g.:

[Psr7\stream_for('دست‌نوشته‌ها نمی‌سوزند')], [Psr7\stream_for('Рукописи не горят')],

jeskew · 2017-10-30T16:54:37Z

tests/Crypto/KmsMaterialsProviderTest.php

+        $provider = new KmsMaterialsProvider($client, $keyId);
+
+        $this->assertEquals(
+            ['kms_cmk_id' => $keyId],


From context, I'm guessing that CEK stands for "content encryption key" and CMK stands for "content master key"; why not just use EncryptionKey and MasterKey as the parameter names?

jeskew · 2017-10-30T16:56:27Z

tests/S3/Crypto/HeadersMetadataStrategyTest.php

@@ -0,0 +1,42 @@
+<?php
+


The test classes should probably be in a namespace corresponding to the directory structure.

cjyclaire

Haven't gone through all yet, post my stashed comments before I forget them : )

cjyclaire · 2017-10-18T22:43:22Z

src/Crypto/AbstractCryptoClient.php

+ */
+abstract class AbstractCryptoClient
+{
+    private static $supportedCiphers = [


why a hash? why not a array of supported ciphers?

cjyclaire · 2017-10-18T22:49:31Z

src/Crypto/AbstractCryptoClient.php

+     * Dependency to provide an interface for building a decryption stream for
+     * cipher text given metadata and materials to do so.
+     *
+     * @param string $cipherText Plain-text data to be encrypted using the


Typo, decrypted text data?

Good catch!

cjyclaire · 2017-10-30T00:47:36Z

src/S3/Crypto/S3EncryptionClient.php

+                switch ($args['@MetadataStrategy']) {
+                    case HeadersMetadataStrategy::class:
+                        return new HeadersMetadataStrategy();
+                        break;


Hmmm "return" is sufficient without "break"?

cjyclaire · 2017-10-30T00:48:49Z

src/S3/Crypto/S3EncryptionClient.php

+            );
+        }
+
+        return null;


Hmm PHP will return null if there is no return

Yeah, I wanted to be explicit here though since there are other return statements in this function.

cjyclaire · 2017-10-30T20:22:11Z

src/S3/Crypto/S3EncryptionClient.php

+     * @throws \InvalidArgumentException Thrown when arguments above are not
+     *                                   passed or are passed incorrectly.
+     */
+    public function putObjectAsync(array $args) {


What's the failed scenario look like? Especially for "getObject"? It will be great if some error scenario (besides invalid arguments) can be tested as well

…ream. Remove object reference from encrypt signature.

jeskew · 2017-11-02T22:33:00Z

features/s3Encryption/encryption.feature

+Feature: S3 Client Side Encryption
+
+  Scenario: Upload PHP's GCM encrypted fixtures
+    When I get all fixtures for "aes_gcm" from "aws-s3-shared-tests"


The S3 test should be creating and deleting buckets via BeforeSuite and AfterSuite hooks. I understand that this test suite has to work this way to integrate with other SDK tests, but maybe in the future there could a test that works on multiple accounts.

kstich and others added 25 commits October 12, 2017 15:28

Define a base AesStreamInterface.

8a7edd1

Define a CipherMethod interface and a CBC implementation based on it.

8f19eb2

Streams for Encrypting and Decrypting that take CipherMethods.

90277a9

Streams for Encrypting and Decrypting that use PHP 7.1 GCM support.

4a903f5

Tag appended GCM Encrypting stream.

4b73079

MetadataEnvelope to store envelope encryption metadata.

4df9a64

MaterialsProvider abstract definition. KmsMaterialsProvider that uses…

a716715

… KMS to provide abstract pieces of MaterialsProvider.

MetadataStrategyInterface to save and load data from a MetadataEnvelope.

5e331c8

Define, and provide some, base functionality for crypto handling clie…

8142f7f

…nts.

Traits that provide encrypt() and decrypt() functionality, using the …

fabde32

…provided AesStreamInterface based classes.

Strategy that saves/loads MetadataEnvelope contents from S3 Object Me…

13d6967

…tadata Headers.

Strategy that saves/loads MetadataEnvelope contents from an instructi…

dc82156

…on file on S3.

Adds the S3EncryptionClient with CBC and GCM support. Uses pluggable …

96a8152

…strategies for handling a MetadataEnvelope in conjunction with a MaterialsProvider. Supports putObject[Async] and getObject[Async] operations.

Update apigen version to handle trait overlap.

3d387a4

Remove Crypto *Stream Keys from traces.

9e1bd9f

Tests for CBC cipher and Aes streams.

443293c

Tests for KmsMaterialsProvider.

01eac8d

Tests for MetadataEnvelope. Adds a trait for future MetadataEnvelope …

ac92dfa

…related tests.

Tests for both MetadataStrategyInterface implementers.

aa56231

Tests for S3EncryptionClient functionality.

54b8d1f

Integration tests.

8f63f88

Service Guide for S3EncryptionClient.

1d0f5ab

Changelog document.

87e1197

Only run codecov on 7.1 for GCM availability.

0ee782a

Minor service guide updates for consistency with code updates.

a2adb70

kstich requested review from cjyclaire, jeskew and mtdowling October 13, 2017 18:39

Fallback to setExpectedException for older versions of PHPUnit.

3628c58

kstich mentioned this pull request Oct 13, 2017

client side encryption #831

Closed

alshamiri3 approved these changes Oct 18, 2017

View reviewed changes

jeskew reviewed Oct 30, 2017

View reviewed changes

kstich added 3 commits October 30, 2017 13:15

Send coverage to CodeCov for only 7.1 with latest dependencies.

48787f9

Proper multibyte character testing.

3c31451

Update docs to clarify instruction file handling after a failure.

be2c149

cjyclaire reviewed Oct 30, 2017

View reviewed changes

kstich added 5 commits October 30, 2017 13:56

Psr2 + SDK conventions fixes. Namespaces for S3 specific test classes.

18b4fea

Update documentation around CEKs.

19a2eaa

Update supportedCiphers setup. Fix doc typo.

b58f348

Remove extraneous break statements.

d7f3948

Remove need for AesTaggedGcmEncryptingStream and its use via AppendSt…

29bfb98

…ream. Remove object reference from encrypt signature.

cjyclaire approved these changes Nov 8, 2017

View reviewed changes

jeskew approved these changes Nov 8, 2017

View reviewed changes

kstich merged commit a2f90a9 into aws:master Nov 8, 2017

kstich deleted the s3_client_side_encryption branch January 17, 2018 05:12


		$envelope = new MetadataEnvelope();

		return Promise\promise_for($this->encrypt(

Comments

Conversation

kstich commented Oct 13, 2017

Uh oh!

codecov-io commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kstich commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjyclaire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Oct 13, 2017 •

edited

Loading