Create pub/priv keypair for federation #17071

techknowlogick · 2021-09-16T21:39:27Z

techknowlogick commented

2021-09-16 21:39:27 +00:00

mostly Fixes #16717

skipping migration, and creating keypair for each user until other related PR is merged.

mostly Fixes #16717 skipping migration, and creating keypair for each user until other related PR is merged.

👍 1

delvh reviewed 2021-09-16 21:52:58 +00:00

modules/activitypub/keypair.go

delvh commented

2021-09-16 21:52:57 +00:00

I don't think the publicKey function is needed as it is called only here.
Additionally, the function declaration seems overly complicated to me.
If I understand go syntax correctly, the following should still be valid, right?

	pubPem, err := pemBlockForPub(&priv.PublicKey)

I don't think the `publicKey` function is needed as it is called only here. Additionally, the function declaration seems overly complicated to me. If I understand go syntax correctly, the following should still be valid, right? ```suggestion pubPem, err := pemBlockForPub(&priv.PublicKey) ```

delvh reviewed 2021-09-16 21:53:36 +00:00

modules/activitypub/keypair.go

delvh commented

2021-09-16 21:53:36 +00:00

```suggestion ```

delvh reviewed 2021-09-17 06:28:34 +00:00

modules/activitypub/keypair.go

delvh commented

2021-09-17 06:28:34 +00:00

Is there a reason why this parameter is a generic interface and not a rsa.PublicKey?
Because while it most likely can accept anything, the only thing I think it should accept is a rsa.PublicKey.
Everything else is most likely a runtime error.

So, the result should most likely be

func pemBlockForPub(pub rsa.PublicKey) (string, error) {

or

func pemBlockForPub(pub *rsa.PublicKey) (string, error) {

Is there a reason why this parameter is a generic interface and not a `rsa.PublicKey`? Because while it most likely can accept anything, the only thing I think it should accept is a `rsa.PublicKey`. Everything else is most likely a runtime error. So, the result should most likely be ```suggestion func pemBlockForPub(pub rsa.PublicKey) (string, error) { ``` or ```suggestion func pemBlockForPub(pub *rsa.PublicKey) (string, error) { ```

delvh approved these changes 2021-09-19 09:52:45 +00:00

6543 (Migrated from github.com) reviewed 2021-09-19 10:39:44 +00:00

modules/activitypub/keypair.go

						
				@ -0,0 +27,4 @@

					return privPem, pubPem, nil

				}

				func pemBlockForPriv(priv *rsa.PrivateKey) (string, error) {

6543 (Migrated from github.com) commented

2021-09-19 10:39:44 +00:00

a smal simple unit test would be nice :)

techknowlogick reviewed 2021-09-20 02:50:23 +00:00

modules/activitypub/keypair.go

						
				@ -0,0 +27,4 @@

					return privPem, pubPem, nil

				}

				func pemBlockForPriv(priv *rsa.PrivateKey) (string, error) {

techknowlogick commented

2021-09-20 02:50:22 +00:00

tests have been added.

delvh reviewed 2021-09-20 06:48:11 +00:00

modules/activitypub/keypair_test.go

						
				@ -0,0 +25,4 @@

					assert.NotEmpty(t, pub)

					assert.Regexp(t, regexp.MustCompile("^-----BEGIN RSA PRIVATE KEY-----.*"), priv)

					assert.Regexp(t, regexp.MustCompile("^-----BEGIN PUBLIC KEY-----.*"), pub)

delvh commented

2021-09-20 06:48:10 +00:00

I think an additional statement where you encode and then decode something should be added.
This is just to show the integrity of the keys, even though I believe the crypto package already should take care of that.

I think an additional statement where you encode and then decode something should be added. This is just to show the integrity of the keys, even though I believe the crypto package already should take care of that.

techknowlogick reviewed 2021-09-27 19:38:17 +00:00

modules/activitypub/keypair_test.go

						
				@ -0,0 +25,4 @@

					assert.NotEmpty(t, pub)

					assert.Regexp(t, regexp.MustCompile("^-----BEGIN RSA PRIVATE KEY-----.*"), priv)

					assert.Regexp(t, regexp.MustCompile("^-----BEGIN PUBLIC KEY-----.*"), pub)

techknowlogick commented

2021-09-27 19:38:16 +00:00

more tests have been added

delvh reviewed 2021-09-27 20:29:46 +00:00

modules/activitypub/keypair_test.go

						
				@ -0,0 +24,4 @@

					assert.NotEmpty(t, priv)

					assert.NotEmpty(t, pub)

					assert.Regexp(t, regexp.MustCompile("^-----BEGIN RSA PRIVATE KEY-----.*"), priv)

delvh commented

2021-09-27 20:18:42 +00:00

Can't the if be replaced by its content?
I hope assert.NoError checks whether an error is present itself, otherwise that function makes only limited sense.
Correct me if this function categorically fails the test.

	assert.NoError(t, err)

Can't the if be replaced by its content? I hope `assert.NoError` checks whether an error is present itself, otherwise that function makes only limited sense. Correct me if this function categorically fails the test. ```suggestion assert.NoError(t, err) ```

delvh commented

2021-09-27 20:18:59 +00:00

	assert.NoError(t, err)

```suggestion assert.NoError(t, err) ```

modules/activitypub/keypair_test.go

delvh commented

2021-09-27 20:29:31 +00:00

Interesting.
I expected that you would decode msg here to check whether decodedMsg == msg.
That was to show that these keys indeed work as intended.
This implementation surprises me.

Interesting. I expected that you would decode `msg` here to check whether `decodedMsg == msg`. That was to show that these keys indeed work as intended. This implementation surprises me.

techknowlogick reviewed 2021-09-27 20:34:56 +00:00

modules/activitypub/keypair_test.go

techknowlogick commented

2021-09-27 20:34:56 +00:00

I'm verifying a signature instead of encrypting data.

techknowlogick reviewed 2021-09-27 20:35:10 +00:00

modules/activitypub/keypair_test.go

						
				@ -0,0 +24,4 @@

					assert.NotEmpty(t, priv)

					assert.NotEmpty(t, pub)

					assert.Regexp(t, regexp.MustCompile("^-----BEGIN RSA PRIVATE KEY-----.*"), priv)