PMM-9947: Encryption #897

ritbl · 2022-05-17T10:35:03Z

E2E encryption proposal.

FB:

PMM-9947: FB Percona-Lab/pmm-submodules#2552

github-actions · 2022-06-03T10:38:23Z

utils/rsa_encryptor/encryptor_test.go

+var privateKey []byte
+
+//go:embed test.pub
+var publicKey []byte


🚫 [golangci-lint] _{reported by reviewdog 🐶}
publicKey is unused (deadcode)

github-actions · 2022-06-03T10:38:23Z

utils/rsa_encryptor/encryptor_test.go

+		assert.Nil(t, err)
+		textWithEmbeddedBlock := fmt.Sprintf("--arg=%s", block)
+
+		fmt.Println(textWithEmbeddedBlock)


🚫 [golangci-lint] _{reported by reviewdog 🐶}
use of fmt.Println forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)

github-actions · 2022-06-03T10:38:23Z

utils/rsa_encryptor/encryptor_test.go

+var publicKey []byte
+
+func TestDecryptEmbeddedWithFormat(t *testing.T) {
+	key := "key1"


🚫 [golangci-lint] _{reported by reviewdog 🐶}
string key1 has 3 occurrences, make it a constant (goconst)

github-actions · 2022-06-03T10:38:23Z

utils/rsa_encryptor/encryptor.go

+
+	// extract key
+	bodyStart := strings.Index(text, EncryptedTextBlockCipherStart)
+	key := text[:bodyStart]


🚫 [golangci-lint] _{reported by reviewdog 🐶}
offBy1: Index() can return -1; maybe you wanted to do text[:bodyStart+1] (gocritic)

github-actions · 2022-06-03T10:38:23Z

utils/rsa_encryptor/encryptor.go

@@ -0,0 +1,293 @@
+package rsa_encryptor


🚫 [golangci-lint] _{reported by reviewdog 🐶}
don't use an underscore in package name (golint)

github-actions · 2022-06-03T10:38:24Z

utils/rsa_encryptor/encryptor.go

+		Type:  "PUBLIC KEY",
+		Bytes: publicKeyBytes,
+	}
+	publicPem, err := os.Create(publicKeyPath)


🚫 [golangci-lint] _{reported by reviewdog 🐶}
G304: Potential file inclusion via variable (gosec)

github-actions · 2022-06-03T10:38:24Z

utils/rsa_encryptor/encryptor.go

+	}, nil
+}
+
+func (s *Service) DecryptDsn(dsn string) (string, error) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
exported: exported method Service.DecryptDsn should have comment or be unexported (revive)

github-actions · 2022-06-03T10:38:24Z

utils/rsa_encryptor/encryptor.go

+	}
+	privateKeyPem, err := os.Create(privateKeyPath)
+	if err != nil {
+		return errors.Wrapf(err, "Error when create [%s]: %s \n", privateKeyPath, err)


🚫 [golangci-lint] _{reported by reviewdog 🐶}
error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)

github-actions · 2022-06-03T10:38:24Z

utils/rsa_encryptor/encryptor.go

+	}
+	err = privateKeyPem.Chmod(perm)
+	if err != nil {
+		return errors.Wrapf(err, "Error when changing file permissions [%s]: %s \n", privateKeyPath, err)


🚫 [golangci-lint] _{reported by reviewdog 🐶}
error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)

github-actions · 2022-06-03T10:38:24Z

utils/rsa_encryptor/encryptor.go

+	}
+	err = pem.Encode(privateKeyPem, privateKeyBlock)
+	if err != nil {
+		return errors.Wrapf(err, "Error when encode private pem: %s \n", err)


🚫 [golangci-lint] _{reported by reviewdog 🐶}
error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)

BupycHuk

Thanks, finally I found a time to review this PR. Sorry for that.
How are we going to share public and private keys between pmm-admin and pmm-agent? Are we going to inject them during the build of PMM or just store them in repository or generate them on PMM managed side and share accross clients?
If we will store them in repository than still it's not secure. pmm-admin allows to add agents to another host and if pmm-agent on another host doesn't support ecryption we will have a problem.
If we will inject keys during build we should disable encryption by default for builds made by community to not have problems with compatability.

Tests are failing. Please fix them.
Please fix linters as well.

BupycHuk · 2022-06-14T08:06:30Z

admin/commands/management/add_mongodb.go

@@ -150,6 +157,13 @@ func (cmd *addMongoDBCommand) Run() (commands.Result, error) {
 		}
 	}

+	encryptor := rsa_encryptor.GetEncryptor(ctx)


can we pass the encryptor instead of passing context and trying to extract the encryptor from context. As far as I see there is no check for the existence of an encryptor and that's why your tests are failing.

I'll fix the tests

BupycHuk · 2022-06-14T08:11:10Z

utils/rsa_encryptor/encryptor.go

+type Formatter func(string) string
+
+// Service provides RSA encryption interface for sensitive data.
+type Service struct {


can we call it Encryptor?

Too many public methods, let's make methods private if we don't use them outside of this package.

can we call it Encryptor?
ok

Too many public methods, let's make methods private if we don't use them outside of this package.
What method/methods you think should be made private?

I think I was clear here. All methods are not used outside of this package.

BupycHuk · 2022-06-14T08:12:36Z

utils/rsa_encryptor/encryptor.go

+}
+
+// GenerateKeys creates asymmetric keys.
+func (s *Service) GenerateKeys(privateKeyPath, publicKeyPath string, bits int, perm fs.FileMode) error {


I don't see any usage of this method.

should it be a method at all? looks like it can be just a function

utils/rsa_encryptor/encryptor.go

BupycHuk · 2022-06-14T08:50:16Z

utils/rsa_encryptor/encryptor.go

+	"github.com/pkg/errors"
+)
+
+const (


Do we really need all these constants be public?

Ok. Will make them private

ritbl · 2022-06-20T06:51:46Z

@BupycHuk

How are we going to share public and private keys between pmm-admin and pmm-agent?

We have multiple options:
1: Leave as it is in this PR - anyone with a bit of patience can find all the keys and decrypt data
Security: 1 out of 5 (very bad). Just grab the keys from repo....
Ease of change / backward compatibility: 5 out of 5 (great).

2: Inject keys in the build time. Someone needs to generate keys and put in secrets in the repo.
Security: 2 out of 5 (bad). Keys will be embedded. It will make harder to get the key but it is still very easy.
Ease of change / backward compatibility: 4 out of 5 (good)

3: Inject keys before pmm starts.
Security: 3-4 out of 5 (good). Proper solution will take time and client now need to manage the keys. It can be make semi-transparent, but still it will require a lot more work.
Ease of change / backward compatibility: 1 out of 5 (very bad)

We can start with any of these, the key can be rotated later. I think any solution would be better if we compare it with the current one.

by default for builds made by community to not have problems with compatability

Actually, it is not needed. It will all work. It is backward compatible. We can start with key being even stored in source code (just because it is easy). Later we can provide better key management solution and create simple rotation job. The key name is embedded into metadata, so that we can have multiple keys.

BupycHuk · 2022-06-20T08:26:28Z

pmm-admin allows to add agents to another host; if pmm-agent on another host doesn't support encryption, we will have a problem.
it's not a typical case, but it's still possible.

ritbl · 2022-06-20T09:44:44Z

@BupycHuk

pmm-admin allows to add agents to another host; if pmm-agent on another host doesn't support encryption, we will have a problem.
it's not a typical case, but it's still possible.

Admin perform encryption, for that public key is used. It doesn't need to be kept in secret.
We can embed public key and even put in the source code. Only private key should be taken care of (agent).

artemgavrilov · 2022-06-22T09:22:50Z

@BupycHuk @ritbl
To be honest I have feeling that this kind of protection should be done on different level. I think it's responsibility of a system administrator to setup TLS certificates for PMM server and we should just respect these settings. This is more obvious and typical solution than custom encryption of just passwords (other information may be sensitive as well). To prevent passwords leaking to logs we can just make codebase audit.

Options 1 and 2 have no sense, it's increasing complexity with almost zero profit. Option 3 may work, but again system administrator should take care for keys management. For now I thinks we can notify user in logs/UI that connection is insecure and ask him to setup TLS.

ademidoff

@ritbl Do you mind if I convert it to draft to suppress notifications?

ritbl requested a review from BupycHuk as a code owner May 17, 2022 10:35

This was referenced May 17, 2022

PMM-9947: Encryption PoC percona/pmm-admin#222

Open

PMM-9947: Encryption PoC percona/pmm-agent#385

Open

ritbl requested review from artemgavrilov, ademidoff and Hlmn-Rmr June 2, 2022 13:20

ritbl added 2 commits June 3, 2022 12:26

Encryption PoC

cb75f16

reformat

79ae423

ritbl force-pushed the PMM-9947-encryption branch from d949477 to 79ae423 Compare June 3, 2022 10:32

github-actions bot reviewed Jun 3, 2022

View reviewed changes

encryption for agent and admin

e6686d8

ritbl mentioned this pull request Jun 3, 2022

PMM-9947: FB Percona-Lab/pmm-submodules#2552

Closed

BupycHuk requested changes Jun 16, 2022

View reviewed changes

style

6296145

ritbl requested a review from BupycHuk June 20, 2022 06:35

ritbl and others added 2 commits June 20, 2022 08:52

Merge branch 'main' into PMM-9947-encryption

e57b801

refactor

306d925

ritbl added 2 commits June 20, 2022 10:53

style

b319a37

mod tidy

9f90ac9

ritbl changed the title ~~PMM-9947: Encryption PoC~~ PMM-9947: Encryption Jun 20, 2022

fix tests

b2b527b

ademidoff marked this pull request as draft September 16, 2022 08:45

ademidoff reviewed Sep 16, 2022

View reviewed changes

BupycHuk closed this Jun 7, 2023

BupycHuk deleted the PMM-9947-encryption branch June 7, 2023 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PMM-9947: Encryption #897

PMM-9947: Encryption #897

ritbl commented May 17, 2022 •

edited

Loading

github-actions bot Jun 3, 2022

github-actions bot Jun 3, 2022

github-actions bot Jun 3, 2022

github-actions bot Jun 3, 2022

github-actions bot Jun 3, 2022

github-actions bot Jun 3, 2022

github-actions bot Jun 3, 2022

github-actions bot Jun 3, 2022

github-actions bot Jun 3, 2022

github-actions bot Jun 3, 2022

BupycHuk left a comment

BupycHuk Jun 14, 2022

ritbl Jun 20, 2022

BupycHuk Jun 14, 2022

BupycHuk Jun 14, 2022

ritbl Jun 20, 2022

BupycHuk Jun 20, 2022

BupycHuk Jun 14, 2022

BupycHuk Jun 14, 2022

BupycHuk Jun 14, 2022

ritbl Jun 20, 2022

ritbl commented Jun 20, 2022 •

edited

Loading

BupycHuk commented Jun 20, 2022

ritbl commented Jun 20, 2022

artemgavrilov commented Jun 22, 2022

ademidoff left a comment

PMM-9947: Encryption #897

PMM-9947: Encryption #897

Conversation

ritbl commented May 17, 2022 • edited Loading

github-actions bot Jun 3, 2022

Choose a reason for hiding this comment

github-actions bot Jun 3, 2022

Choose a reason for hiding this comment

github-actions bot Jun 3, 2022

Choose a reason for hiding this comment

github-actions bot Jun 3, 2022

Choose a reason for hiding this comment

github-actions bot Jun 3, 2022

Choose a reason for hiding this comment

github-actions bot Jun 3, 2022

Choose a reason for hiding this comment

github-actions bot Jun 3, 2022

Choose a reason for hiding this comment

github-actions bot Jun 3, 2022

Choose a reason for hiding this comment

github-actions bot Jun 3, 2022

Choose a reason for hiding this comment

github-actions bot Jun 3, 2022

Choose a reason for hiding this comment

BupycHuk 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

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

ritbl commented Jun 20, 2022 • edited Loading

BupycHuk commented Jun 20, 2022

ritbl commented Jun 20, 2022

artemgavrilov commented Jun 22, 2022

ademidoff left a comment

Choose a reason for hiding this comment

ritbl commented May 17, 2022 •

edited

Loading

ritbl commented Jun 20, 2022 •

edited

Loading