Is this loop repeated several times in this file? Could we extract it to a function maybe?

I think the error scenario tests are different than the standard Encrypt/Decrypt tests. I merged the Standard vs Base64 wrapped tests into one, so that removed the duplication those two had.

Is this now duplicated with the cookies package?

This is definitely related to this PR cleaning that organization up: #538

Right I see, this content has just been moved from cipher.go to this file, makes sense now

this nesting is a bit much ... you could cut out a few levels by avoiding a t.Run() for every outer loop, and only use it for the inner-most portion, like:

	for name, initCipher := range cipherInits {
		// Test all 3 valid AES sizes
		for _, secretSize := range []int{16, 24, 32} {
			// ... prep/test secret key formats
			ciphers := map[string]Cipher{
				"Standard": cstd,
				"Base64":   cb64,
			}
			for cName, c := range ciphers {
				for _, dataSize := range []int{10, 100, 1000, 5000, 10000} {
					casename := fmt.Sprintf("%s_%d_%s_%d", name, secretSize, cName, dataSize)
					t.Run(casename, func(t *testing.T) {
						// ... actual test case here

or use some helper functions

@JoelSpeed So I think this (and all previous implementations) are problematic -- but I'm thinking its not worth fixing because the error is rare and the need to support this will go away once we stop allowing SHA1 signed session cookies:

Since AES-CFB isn't an authenticated encryption, there's likely a subset of values that will successfully Base64 decode & then AES decrypt into a valid string (where it just assumed the first bytes are the IV vs part of the data) -> and this won't raise an error (which would then leave the User/Email as-is assuming they were legacy unencrypted). And instead they will be mangled into garbage.

I'd prefer if you could write these as _ := c.DecryptInto to make it more obvious that this can error, but we are deliberately ignoring it (in addition to the comment preferably)

@NickMeves I'm not sure this problem is going to be so rare, have you seen the local test environment we added recently? I spun that up (make local-env-up), logged in (v5.1.1), built from master, updated the container, then I'm getting errors because the user is not a valid header value which I've tracked to being down to this function https://github.com/golang/net/blob/627f9648deb96c27737b83199d44bb5c1010cbcf/http/httpguts/httplex.go#L302

This should be private, and given it's for testing purposes, how about newTestCipher so that we know it's meant only for testing?

I'd prefer if you could write these as _ := c.DecryptInto to make it more obvious that this can error, but we are deliberately ignoring it (in addition to the comment preferably)

Just wondering if this and the other Ciphers should be private? Since we have constructors and an interface I don't think they need to be public, WDYT?

In the upcoming session refactor that builds off of this, different use cases use different encryption ciphers that are best for various use cases:

CFB, GCM that operate on pure []byte and never have a string are used with the MessagePack encoding (this is the secret sauce that allows us not to have a base64 encoding adding 33% bloat to fit an encrypted value into JSON that requires a string).

For Cookie sessions, pure CFB (not base64 wrapped) is used on encrypting sessions. CFB is missing authentication (which GCM has) but isn't weak to IV reuse issues (which prevents us from using GCM comfortably in cookies in case some users never cycle the cookie secret). We get authentication of the cookie via the SHA256 HMAC on the cookie.

For the encryption layer in DB based session stores (currently just Redis), GCM is great because it is better in all aspects than CFB (except IV reuse), and it doesn't have the IV reuse issue since we make a different secret that is unique to each session.

Base64 wrapped versions are left available for backwards compatibility with legacy JSON based sessions that still exist.

Hence, I left all public and accessible.

Oh, lol, I think I follow your suggestion now 😄

Treat the above as me over explaining a question you didn't ask.

So what's a good naming convention for the CFBCipher and GCMCipher to make the structs themselves private?

I would just go for cfbCipher and gcmCipher if that's ok with you?

lol -- so obvious 🤦

Not sure I understand why this is the case? Does it not survive a round trip? Are you envisioning the EncryptInto/DecryptInto will go away as part of the session state refactoring?

Only the Base64 wrapped version are ensured to get a string & return a string that can use the EncryptInto functionality.

The output of Encrypt from CFB/GCM pure versions will result in []byte that doesn't cast to a string to put back into the s *string

And for the second question -- yes, they won't be needed for encrypting the MessagePack encoding (compressed or not, []byte in both cases). So the pure CFB/GCM []byte -> []byte functions can be used (and we can potentially alter them to encrypt/decrypt in place if we deem that safe -- currently they are coded to treat the input as immutable).

Would only be needed to decode legacy sessions.

Right I see, this content has just been moved from cipher.go to this file, makes sense now

I think what we'd actually like to do is drop the backward compatibility, we discussed this in #601, basically, if we have an issue decrypting, or if the decrypted result is not valid UTF 8, then we believe that the session was previously unencrypted and decided that we should return an error, to force users to re-authenticate in this case.

So what we could do here, is just move Email and User into the range statement below I think, would have the same sort of effect right?

CC @steakunderscore can you also confirm my logic and this is what we had agreed upon

Now that #601 has merged I'll rebase and incorporate those changes here 👍

aaaaaand rebased. I think I captured the spirit of #601 with how we check the errors via the into methods