Encryption support producer #560

GPrabhudas · 2021-07-08T09:45:05Z

Breakdown of PR #552

This PR includes the encryption changes at producer side.

- use base crypto package for encryption

cckellogg · 2021-07-12T02:22:55Z

pulsar/internal/commands.go

@@ -269,6 +270,117 @@ func serializeBatch(wb Buffer,
 	wb.PutUint32(checksum, checksumIdx)
 }

+// copy of the method serializeBatch(....) with an extension to encrypt payload
+func serializeBatchWithEncryption(wb Buffer,


What are the differences between serializeBatch and serializeBatchWithEncryption? It looks like a lot of duplicate code?

I preferred to put this in separate function instead of modifying the exiting one. Let me check if I can remove duplicate code

reused the serializeBatch

cckellogg · 2021-07-12T02:26:57Z

pulsar/producer.go

@@ -163,6 +165,19 @@ type ProducerOptions struct {
 	// PartitionsAutoDiscoveryInterval is the time interval for the background process to discover new partitions
 	// Default is 1 minute
 	PartitionsAutoDiscoveryInterval time.Duration
+
+	// EncryptionKeys list of encryption key names to encrypt session key
+	EncryptionKeys []string


Should we wrap these into a struct? Maybe something like this?

type Encryption { Keys []string KeyReader crypto.KeyReader etc ]

Makes sense

cckellogg · 2021-07-12T02:27:44Z

go.mod

@@ -14,10 +14,7 @@ require (
 	github.com/google/uuid v1.1.2
 	github.com/inconshreveable/mousetrap v1.0.0 // indirect
 	github.com/klauspost/compress v1.10.8


Curious why is the mod and sum file changing? Can these changes be done in a separate PR?

Let me recheck again :)

synced with master branch

cckellogg · 2021-07-12T02:28:17Z

pulsar/internal/commands.go

+	wb.PutUint32(checksum, checksumIdx)
+}
+
+func encryptPayload(msgMetadata *pb.MessageMetadata,


Should this return an error?

cckellogg · 2021-07-12T02:30:01Z

pulsar/internal/commands.go

+
+	// there was a error in encrypting the payload and
+	// crypto failure action is set to crypto.ProducerCryptoFailureActionFail
+	if encryptedPayload == nil {


I'm not sure panic is the correct behavior here.

Its better to throw an exception because client is setting ProducerCryptoFailureActionFail to fail.

cckellogg · 2021-07-12T02:30:33Z

pulsar/internal/commands.go

+		crypto.NewMessageMetadataSupplier(msgMetadata),
+		compressedPayload)
+
+	if err != nil {


Should this error be log somewhere?

cckellogg · 2021-07-12T02:33:18Z

pulsar/internal/commands.go

+	// [TOTAL_SIZE] [CMD_SIZE][CMD] [MAGIC_NUMBER][CHECKSUM] [METADATA_SIZE][METADATA] [PAYLOAD]
+
+	// compress the payload
+	compressedPayload := compressionProvider.Compress(nil, uncompressedPayload.ReadableSlice())


We want to compress before encrypting?

As per java implementation => Yes compress and then encrypt.

cckellogg · 2021-07-12T02:53:37Z

pulsar/producer_partition.go

+	}
+}
+
+func (p *partitionProducer) updateDataKey() error {


Does this need to be thread safe? Also, can we move the key refreshing to another MR.

I can move it to next MR.
Just ask: why this to be moved to another MR?

cckellogg · 2021-07-12T02:55:31Z

pulsar/producer_partition.go

 	} else if p.batchBuilder == nil {
-		provider, err := GetBatcherBuilderProvider(p.options.BatcherBuilderType)


Why change this? The p masks the partitionProducer and make the code harder to follow.

Just tried to remove duplicate code :) If it is not ok, I'll revert it.

cckellogg · 2021-07-12T02:59:05Z

pulsar/internal/batch_builder.go

 	)

 	return &bc, nil
 }

+// UseEncryptionKeys encryption key names to use


What about removing these config options and passing an Encryptor interface/struct. I think the config functions work better when things are scoped in their own package - it make it name spacing clearer. Also, it doesn't really match the rest of the code.

type Encrypter interface { Encrypt(msgMetadata, payload []byte) ([]byte, error) }

If encryption is not provided there can be a noop encryter

wolfstudy · 2021-07-19T08:37:05Z

@cckellogg @GPrabhudas Any update for this? We are ready to initiate a new version of the release, hoping to include this new change in the new version (because some of the code has been merged in), can we consider advancing the progress here.

GPrabhudas · 2021-07-19T08:55:52Z

@cckellogg @GPrabhudas Any update for this? We are ready to initiate a new version of the release, hoping to include this new change in the new version (because some of the code has been merged in), can we consider advancing the progress here.

I'll work on it and address the review suggestions.

cckellogg · 2021-07-20T02:58:19Z

@wolfstudy what is your timeframe for a new release?

wolfstudy · 2021-07-20T03:22:45Z

@wolfstudy what is your timeframe for a new release?

What I want is to start the release process after merging the change related to this function. If the development of this feature takes some time, we can consider including it in the next version.

cckellogg · 2021-07-20T13:09:59Z

@wolfstudy what is your timeframe for a new release?

What I want is to start the release process after merging the change related to this function. If the development of this feature takes some time, we can consider including it in the next version.

There are additional MRs (besides this one) to get this feature working. Currently, just the crypto package has been merged. It's probably better to cut a release now and then add the crypto implementation (producer and consumer) into the next release. To me that is better than rushing to get MRs in.

- move it to Consumer MR

…Fanatics/pulsar-client-go into encryption-support-ext-producer

cckellogg · 2021-08-02T04:15:36Z

pulsar/internal/crypto/producer_encryptor.go

+// Encrypt producer encryptor
+func (e *producerEncryptor) Encrypt(payload []byte, msgMetadata crypto.MessageMetadataSupplier) ([]byte, error) {
+	// encryption is enabled but KeyReader interface is not implemented
+	if e.keyReader == nil {


Should this be detected and an error raised while setting up the producer?

cckellogg · 2021-08-02T04:16:47Z

pulsar/producer_partition.go

+		logCtx := fmt.Sprintf("[%v] [%v] [%v]", p.topic, p.producerName, p.producerID)
+		messageCrypto, err := crypto.NewDefaultMessageCrypto(logCtx, true, logger)
+		if err != nil {
+			logger.WithError(err).Error("Unable to get MessageCrypto instance. Producer creation is abandoned")


Will there be more context in the err of why this failed?

Yes. Error is returned if generation of data key is failed. Here we are logging the same error with other information.

cckellogg · 2021-08-02T04:24:54Z

pulsar/encryption.go

+// ProducerEncryptionInfo encryption related fields required by the producer
+type ProducerEncryptionInfo struct {
+	// KeyReader read RSA public/private key pairs
+	Keyreader crypto.KeyReader


KeyReader

cckellogg · 2021-08-02T04:31:24Z

pulsar/internal/crypto/encryptor.go

+
+// Encryptor support encryption
+type Encryptor interface {
+	Encrypt([]byte, crypto.MessageMetadataSupplier) ([]byte, error)


Since this is internal we can pass the *pb.MessageMetadata and avoid having to create a supplier for each message when encryption is not being used.

pulsar/internal/commands.go

cckellogg · 2021-08-09T17:51:36Z

pulsar/internal/crypto/producer_encryptor.go

+		// send unencrypted message
+		if e.producerCryptoFailureAction == crypto.ProducerCryptoFailureActionSend {
+			e.logger.Errorf("Encryption of payload failed : %v", err)
+			e.logger.Warn("ProducerCryptoFailureAction is set to send, sending unecrypted message")


I would log only one warning message here.

logger.WithError(err).Warnf("Encryption failed for payload sending unencrypted message ProducerCryptoFailureAction is set to send")

cckellogg · 2021-08-09T17:52:54Z

pulsar/producer_partition.go

+func (p *partitionProducer) generateDataKey() error {
+	if p.options.Encryption != nil {
+		if p.options.Encryption.KeyReader != nil {
+			return p.options.Encryption.MessageCrypto.AddPublicKeyCipher(p.options.Encryption.Keys,


Could the MessageCrypto be nil here?

No. It won't be nil.
If not provided, it is getting initialized with default one.
https://github.com/Fanatics/pulsar-client-go/blob/encryption-support-ext-producer/pulsar/producer_partition.go#L141

cckellogg · 2021-08-09T17:58:41Z

pulsar/producer_partition.go

+		}
+
+		if encryption.MessageCrypto == nil {
+			logCtx := fmt.Sprintf("[%v] [%v] [%v]", p.topic, p.producerName, p.producerID)


Maybe this can be update in another MR. It might be better to create a context logger instead of sending a string

cl := logger.WithFields(log.Fields{ "topic": topic, "producer": producerName, "producerID", producerID })

cckellogg · 2021-08-27T05:37:03Z

pulsar/internal/key_based_batch_builder.go

 		if b != nil {
 			batchesData[idx] = b
 			sequenceIDs[idx] = s
 			callbacks[idx] = c
+		} else if err != nil {


Should the error be set with the other arrays so they are always the same length? the only way b can be nil is if there are no messages right.

@cckellogg can this PR me merged if all looks good :)

PGarule added 3 commits July 7, 2021 23:02

add ability to encrypt messages

d8bb8fe

- use base crypto package for encryption

fix typo

aa40a8d

lint fixes

3b8d912

GPrabhudas mentioned this pull request Jul 8, 2021

Encryption support in Go client #552

Closed

cckellogg reviewed Jul 12, 2021

View reviewed changes

wolfstudy assigned GPrabhudas Jul 12, 2021

wolfstudy added this to the 0.6.0 milestone Jul 12, 2021

wolfstudy added the type/feature label Jul 12, 2021

PGarule and others added 5 commits July 20, 2021 19:33

address review suggestions

e1a8108

revert go mod

d200b7c

remove encryption context

feaf120

- move it to Consumer MR

try to fix check issues

a2bd72b

Merge branch 'master' into encryption-support-ext-producer

f204347

wolfstudy modified the milestones: 0.6.0, 0.7.0 Jul 21, 2021

PGarule added 3 commits July 21, 2021 13:06

remove unused code

a561923

Merge branch 'encryption-support-ext-producer' of https://github.com/…

559dbaf

…Fanatics/pulsar-client-go into encryption-support-ext-producer

remove embedded crypto struct

d7246bc

GPrabhudas requested a review from cckellogg July 24, 2021 16:30

cckellogg reviewed Aug 2, 2021

View reviewed changes

review suggestions

62937c5

cckellogg reviewed Aug 9, 2021

View reviewed changes

PGarule added 3 commits August 16, 2021 09:58

remove duplicate log

39d43b2

lint code style issue fix

d88b04e

return error from flush methods on serialization error

41b10bc

update test case and do lazy data key generation

c362133

cckellogg reviewed Aug 27, 2021

View reviewed changes

PGarule added 2 commits August 27, 2021 13:05

address review changes

06c7612

add comments on test case

4dc8ddb

cckellogg approved these changes Aug 28, 2021

View reviewed changes

cckellogg merged commit 1d3a9cc into apache:master Sep 3, 2021

		} else if p.batchBuilder == nil {
		provider, err := GetBatcherBuilderProvider(p.options.BatcherBuilderType)

Encryption support producer #560

Encryption support producer #560

Conversation

GPrabhudas commented Jul 8, 2021

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

GPrabhudas Jul 12, 2021 • edited

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

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

GPrabhudas Jul 12, 2021 • edited

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

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

wolfstudy commented Jul 19, 2021

GPrabhudas commented Jul 19, 2021

cckellogg commented Jul 20, 2021

wolfstudy commented Jul 20, 2021 • edited

cckellogg commented Jul 20, 2021

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

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

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

GPrabhudas Jul 12, 2021 •

edited

GPrabhudas Jul 12, 2021 •

edited

wolfstudy commented Jul 20, 2021 •

edited