pgcrypto: add helper functions for PKCS padding

[andyyang890]

This patch adds helper functions for PKCS padding/unpadding, which is
needed for pgcrypto's raw encryption functions.

Informs #21001

Release note: None

[cockroach-teamcity]

This change is 

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890 and @ecwall)

---

pkg/sql/sem/builtins/pgcrypto/padding.go line 33 at r1 (raw file):

// PKCSUnpad removes the padding added by PKCSPad.
func PKCSUnpad(data []byte, blockSize int) ([]byte, error) {

i want to make sure i understand: why do we need the blockSize here? it wasn't mentioned in that RFC, and it seems like this other library doesn't need it https://github.com/zenazn/pkcs7pad/blob/master/pkcs7pad.go

---

pkg/sql/sem/builtins/pgcrypto/padding.go line 45 at r1 (raw file):

	paddingLen := data[len(data)-1]
	if paddingLen == 0 || int(paddingLen) > blockSize {

i think we should also add a check to make sure paddingLen <= len(data). otherwise i think the loop could hit an index-out-of-bounds error. and we should be able to add a test for that

---

pkg/sql/sem/builtins/pgcrypto/padding_test.go line 122 at r1 (raw file):

		},
		"non-empty data with partial-block padding": {
			data:      []byte{'a', 'b', 2, 2},

continuing my last comment: how about a test case like []byte{'a', 'b', 20, 20},

[andyyang890]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall and @rafiss)

---

pkg/sql/sem/builtins/pgcrypto/padding.go line 33 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i want to make sure i understand: why do we need the blockSize here? it wasn't mentioned in that RFC, and it seems like this other library doesn't need it https://github.com/zenazn/pkcs7pad/blob/master/pkcs7pad.go

I guess it's not strictly necessary but I thought it might be nice to have an extra check to make sure that the data is a multiple of the block size. (Otherwise, it was either padded incorrectly or the function is being called with the wrong block size.)

It also solves the problem you mentioned below of checking paddingLen <= len(data). Iflen(data)%blockSize==0 andpaddingLen <= blockSize, then we necessarily have that paddingLen <= len(data).

In any case, I've removed it and will move this check into the function that calls PKCSUnpad.

---

pkg/sql/sem/builtins/pgcrypto/padding.go line 45 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think we should also add a check to make sure paddingLen <= len(data). otherwise i think the loop could hit an index-out-of-bounds error. and we should be able to add a test for that

Done.

---

pkg/sql/sem/builtins/pgcrypto/padding_test.go line 122 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

continuing my last comment: how about a test case like []byte{'a', 'b', 20, 20},

Done.

[rafiss]

lgtm!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall)

[andyyang890]

TFTR!

bors r=rafiss

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)