Initial #9

chris-giblin · 2022-04-12T07:21:34Z

Initial commit for review

tombentley

Very quick initial pass.

doc/getting-start.md

encmod/src/main/java/io/strimzi/kafka/topicenc/EncModControl.java

encmod/src/main/java/io/strimzi/kafka/topicenc/EncryptionModule.java

encmod/src/main/java/io/strimzi/kafka/topicenc/policy/JsonPolicyRepo.java

vertx-proxy/src/main/java/io/strimzi/kafka/proxy/vertx/MessageHandler.java

chris-giblin · 2022-04-13T14:48:42Z

Thanks @tombentley for the first pass and resulting suggestions, all of which I have committed.

tombentley · 2022-04-14T06:53:32Z

encmod/src/main/java/io/strimzi/kafka/topicenc/enc/CryptoUtils.java

+	public static final String AES_GCM_PADDING = AES + "/GCM/PKCS5Padding";
+	public static final String AES256_GCM_NOPADDING = "AES_256/GCM/NoPadding";


Do both of these have the property that size(plaintext) == size (ciphertext)?

size (ciphertext) = size(plaintext) + len(GCM authtag) where len(GCM authtag) = 16

AES GCM is essentially a streaming cipher without padding. The cipher "AES/GCM/PKCS5Padding" is from a different crypto provider I was testing and can be removed. Most Java crypto providers do not support this cipher. When it is supported by a crypto provider, my understanding is that "AES/GCM/PKCS5Padding" is effectively AES GCM NoPadding.

For our purposes, we can assume no padding for AES GCM encryption (although there is always a 16 byte authentication tag, as mentioned above).

tombentley · 2022-04-14T07:14:36Z

encmod/src/main/java/io/strimzi/kafka/topicenc/EncryptionModule.java

+					    // encrypt record value:
+					    byte[] plaintext = new byte[record.valueSize()];
+					    record.value().get(plaintext);
+						EncData ciphertext = encrypter.encrypt(plaintext);


I was thinking about topic encryption and the fact that it can increase the size of messages, both due to padding in the cipher (possibly, see https://github.com/strimzi/topic-encryption/pull/9/files#r850136676) and also for including things like the IV. If the proxy and the broker had the same max.message.size then it would mean the proxy might accept a message, but the broker reject it because of size inflation caused by the proxy. You could get away with just configuring the proxy with a smaller max.message.size than the broker if you could assume the inflation is a constant for a whole batch. But right now encryption is per-message. Which means for small messages the extra IV data could result in a large proportional increase in batch size, even in the absence of padding from the encryption algorithm.

Valid points.

In the existing scheme, the encryption metadata stored with each message, in bytes:

2 (version)

2 (IV length)

16 (IV)

2 (ciphertext length)

16 (GCM authentication tag)

Total: 38 bytes "overhead" per encrypted message.

I have created issue #15

tombentley

This is a much deeper pass on the encryption bits (not really done the proxy bits, but I'm happy to leave those for now)

tombentley · 2022-04-22T06:02:30Z

.gitignore

@@ -38,3 +38,7 @@ classes/
 # MacOS
 .DS_Store

+# config files
+vertx-proxy/src/main/resources/config.json


Should this really be excluded?

The config file could contain credentials as we integrate with key management systems. Excluding the config file, at least for the time being, is a defensive action to prevent credentials being accidentally pushed into github in the future.

encmod/src/main/java/io/strimzi/kafka/topicenc/EncryptionModule.java

tombentley · 2022-04-22T06:52:43Z

vertx-proxy/src/main/java/io/strimzi/kafka/proxy/vertx/AbstractKafkaMsg.java

+	Buffer rawMsg;
+	ByteBuffer payload;


these members are accessed directly by subclasses. Either protected or default visibility

tombentley · 2022-04-22T06:53:20Z

vertx-proxy/src/main/java/io/strimzi/kafka/proxy/vertx/Config.java

+        public static final String LISTENING_PORT = "listening_port";
+        public static final String KAFKA_BROKERS = "kafka_broker";
+        public static final String POLICY_REPO = "policy_repo";


If they're just constants why not declare them on the outer class?

I anticipate many config params and use static classes as a way to group related constants as opposed to maintaining a long list of constants. But I am perfectly willing to forego the inner class for now and address large number of constants when we cross that bridge.

vertx-proxy/src/main/java/io/strimzi/kafka/proxy/vertx/Config.java

vertx-proxy/src/main/java/io/strimzi/kafka/proxy/vertx/KafkaProxyVerticle.java

vertx-proxy/src/main/java/io/strimzi/kafka/proxy/vertx/KafkaReqMsg.java

Signed-off-by: Chris Giblin <[email protected]>

…comment corrections Signed-off-by: Chris Giblin <[email protected]>

Signed-off-by: Chris Giblin <[email protected]>

….java Co-authored-by: Tom Bentley <[email protected]> Signed-off-by: Chris Giblin <[email protected]>

Signed-off-by: Chris Giblin <[email protected]>

…comment corrections Signed-off-by: Chris Giblin <[email protected]>

Signed-off-by: Chris Giblin <[email protected]>

…itial

chris-giblin · 2022-04-25T09:47:41Z

Closing this PR because numerous ongoing commits and a rebase error have made this PR too unwieldy.
The changes are now consolidated in the new PR #16 which supersedes this one.

chris-giblin requested review from tombentley and SeanRooooney April 12, 2022 07:21

chris-giblin force-pushed the initial branch from f115598 to d9b1990 Compare April 13, 2022 09:48

tombentley reviewed Apr 13, 2022

View reviewed changes

chris-giblin force-pushed the initial branch from 2c30e47 to bfb532e Compare April 13, 2022 15:45

tombentley reviewed Apr 14, 2022

View reviewed changes

tombentley reviewed Apr 22, 2022

View reviewed changes

This was referenced Apr 22, 2022

Design, implement error handling stratgey when encrypt/decrypt throw exceptions #11

Open

Review implementation of cryptographic random numbers #12

Open

Review buffer usage in the encrypt/decrypt and serder #13

Open

chris-giblin added 18 commits April 24, 2022 14:02

add a gitignore file for Java dev

492d71d

Signed-off-by: Chris Giblin <[email protected]>

added reference to the new modules

1cdc81e

Signed-off-by: Chris Giblin <[email protected]>

initial version of proxy and encryption module

46e039c

Signed-off-by: Chris Giblin <[email protected]>

set log level to INFO

8ba3b15

Signed-off-by: Chris Giblin <[email protected]>

reorg imports

e953cf2

Signed-off-by: Chris Giblin <[email protected]>

Add configuration by JSON file

954d333

Signed-off-by: Chris Giblin <[email protected]>

add config by json file for proxy

53666ec

Signed-off-by: Chris Giblin <[email protected]>

filter config file

75da212

Signed-off-by: Chris Giblin <[email protected]>

include vertx-config

64afaa9

Signed-off-by: Chris Giblin <[email protected]>

add getting started document

ca28b96

Signed-off-by: Chris Giblin <[email protected]>

use kafka-dump-log.sh as per suggestion from Tom

b48c635

Signed-off-by: Chris Giblin <[email protected]>

add debug logging, use int to count enc/dec as per suggestion by Tom

9bc4ed9

Signed-off-by: Chris Giblin <[email protected]>

Add javadoc

e63c383

Signed-off-by: Chris Giblin <[email protected]>

debug logging, r->record

f7ad948

Signed-off-by: Chris Giblin <[email protected]>

Add javadoc comments

71e5f44

Signed-off-by: Chris Giblin <[email protected]>

remove empty implementation, will add back when ready

1dcd19f

Signed-off-by: Chris Giblin <[email protected]>

use int to count decryptions, Fetch instantiation only during debug, …

27de901

…comment corrections Signed-off-by: Chris Giblin <[email protected]>

revise README, link to getting started doc

c602497

Signed-off-by: Chris Giblin <[email protected]>

chris-giblin and others added 19 commits April 25, 2022 10:40

renamed start-started

14f5d54

Signed-off-by: Chris Giblin <[email protected]>

set members to 'private final'

094607d

Signed-off-by: Chris Giblin <[email protected]>

Update encmod/src/main/java/io/strimzi/kafka/topicenc/enc/CryptoUtils…

df76ba5

….java Co-authored-by: Tom Bentley <[email protected]> Signed-off-by: Chris Giblin <[email protected]>

set members to 'private'

444751a

Signed-off-by: Chris Giblin <[email protected]>

set members to 'private'

48fbd8b

Signed-off-by: Chris Giblin <[email protected]>

throw GeneralSecurityException, require non-empty IV

73f1fe7

Signed-off-by: Chris Giblin <[email protected]>

throw GeneralSecurityException

c153f2a

Signed-off-by: Chris Giblin <[email protected]>

set members to 'private final'

6492bb7

Signed-off-by: Chris Giblin <[email protected]>

set members to 'private'

a9c7fde

Signed-off-by: Chris Giblin <[email protected]>

set members to 'private'

e3b8156

Signed-off-by: Chris Giblin <[email protected]>

Remove encMod and config members - not used in this class

e23b88b

Signed-off-by: Chris Giblin <[email protected]>

set members to 'private final'

9a54db3

Signed-off-by: Chris Giblin <[email protected]>

remove unused props, dependencies

513af0c

Signed-off-by: Chris Giblin <[email protected]>

remove method with rx4java argument

edfa964

Signed-off-by: Chris Giblin <[email protected]>

make test methods public

66b46f8

Signed-off-by: Chris Giblin <[email protected]>

refactor to allow catching of ecnryption exceptions (prep for issue 11)

c63c3ce

Signed-off-by: Chris Giblin <[email protected]>

add a gitignore file for Java dev

76617a8

Signed-off-by: Chris Giblin <[email protected]>

declare members as private

17560b0

Signed-off-by: Chris Giblin <[email protected]>

merge

50497f2

Signed-off-by: Chris Giblin <[email protected]>

chris-giblin force-pushed the initial branch from f3b8487 to 50497f2 Compare April 25, 2022 08:56

chris-giblin added 6 commits April 25, 2022 11:15

add getting started document

eaade32

Signed-off-by: Chris Giblin <[email protected]>

use kafka-dump-log.sh as per suggestion from Tom

bc6d882

Signed-off-by: Chris Giblin <[email protected]>

use int to count decryptions, Fetch instantiation only during debug, …

b530d53

…comment corrections Signed-off-by: Chris Giblin <[email protected]>

correct Java version 11 -> 17

e65d18a

Signed-off-by: Chris Giblin <[email protected]>

renamed start-started

73d9256

Signed-off-by: Chris Giblin <[email protected]>

Merge branch 'initial' of github.com:strimzi/topic-encryption into in…

41737da

…itial

chris-giblin mentioned this pull request Apr 25, 2022

Initial2 #16

Merged

chris-giblin closed this Apr 25, 2022

chris-giblin deleted the initial branch May 6, 2022 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial #9

Initial #9

chris-giblin commented Apr 12, 2022

tombentley left a comment

chris-giblin commented Apr 13, 2022

tombentley Apr 14, 2022

chris-giblin Apr 14, 2022

chris-giblin Apr 14, 2022

tombentley Apr 14, 2022

chris-giblin Apr 14, 2022 •

edited

Loading

chris-giblin Apr 25, 2022

tombentley left a comment

tombentley Apr 22, 2022

chris-giblin Apr 22, 2022

tombentley Apr 22, 2022

chris-giblin Apr 24, 2022

tombentley Apr 22, 2022

chris-giblin Apr 22, 2022 •

edited

Loading

chris-giblin commented Apr 25, 2022

		public static final String AES_GCM_PADDING = AES + "/GCM/PKCS5Padding";
		public static final String AES256_GCM_NOPADDING = "AES_256/GCM/NoPadding";

Initial #9

Initial #9

Conversation

chris-giblin commented Apr 12, 2022

tombentley left a comment

Choose a reason for hiding this comment

chris-giblin commented Apr 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-giblin Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombentley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-giblin Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

chris-giblin commented Apr 25, 2022

chris-giblin Apr 14, 2022 •

edited

Loading

chris-giblin Apr 22, 2022 •

edited

Loading