security(logging): fix aes implementation in audit logging

[joshua-goldstein]

Background

Audit logging is an enterprise feature with an option for encryption using AES. An AES cipher has three ingredients: a private key, an iv (or nonce), and plaintext. The nonce is public but it must be unique.

Problem

Currently we use a [16]byte nonce. The first 12 bytes come from a baseIv which is initialized when an audit log is created. The last 4 bytes come from the length of the log line being encrypted. This is problematic because two log lines will often have the same length, so due to these collisions we are reusing the same nonce many times. AES requires a unique nonce and without a unique nonce it is trivial to break. Many thanks to @HakuPiku for pointing this out to us.

Solution

Use crypto/rand to generate a unique nonce every time we encrypt a log line. Previously we used what was essentially a 16 byte header (12 byte baseIv and 4 byte length of line). We now use a [20] byte header, consisting of a unique nonce (16 bytes) and length of line (4 bytes).

Remarks

• For audit logging, x/log_writer.go controls all the audit logging (i.e., writing) while ee/audit/run_ee.go does all the decrypting. There are other encryption/decryption tools (in testutil, e.g.) that are used for other purposes (e.g., exports).
• The docker-compose file in contrib/local-test has been temporarily modified for the convenience of anyone wishing to test this PR. Simply spin up a cluster using make up and the docker-compose file is set to encrypt audit logs. This will not be merged.
• Customers may have old audit logs that will have to be decrypted using the deprecated method. How to handle this is currently a todo.
• There is a test x/log_writer.go that will have to be refactored to account for these changes. This is a todo.

References

• https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation
• https://pkg.go.dev/crypto/cipher#example-NewCTR

[coveralls]

Coverage decreased (-0.05%) to 37.046% when pulling 8c42c192679ed9a7cea29eff49c3b86139c6ffb6 on joshua/fix-logger into b69b8e51882fcfead02be63b0304e64ea75aa6ac on main.

[HakuPiku]

Any updates on when this will be merged and a security advisory created?

[joshua-goldstein]

I think we should consider writing more robust functions and test. This needs more work.

I have already added 4 integration tests, and we have a unit test to check basic parsing. What additional tests are necessary?

[joshua-goldstein]

I didn't realize that byte array/slice can't be declared as a constant. This article has a few ideas https://blog.boot.dev/golang/golang-constant-maps-slices/. I don't mind if we declare a function that returns the constant value. I think it is useful to avoid a allocation that converts byte slice to string.

What is the purpose of this? We are doing exactly one allocation per log file (default ~200 mb), so this is not a memory issue?

[joshua-goldstein]

The scope of this PR is now much larger than originally intended. This PR now introduces:

• Four new integration tests for encrypted audit logging in systest/audit_encrypted/audit_test.go, two of which verify that encrypted audit logs can be encrypted, and two that verify that deprecated audit logs can be decrypted
• One new unit test in ee/audit/run_ee_test.go to verify that truncating an encrypted audit log does not cause the decrypt process to panic
• A modified unit test to decrypt using the new audit log format

This PR also does some minor refactoring to the code.