IPsec key rotation command.

[viktor-kurchenko]

encryption rotate-key command implementation to address: cilium/cilium#28174

[jschwinger233]

Thanks Viktor, this PR really helps! Just one small issue on new SPI calculation.

[jschwinger233]

Thanks! Regarding the ipsecKeyFromString function, this PR takes care of keys like 3 rfc4106(gcm(aes)) 3c7ae6f6c0e9a09255fce39440df90619514709e 128. However, cilium can also accept a key with other formats.
I think these have been in your plan, just a reminder. 👍

[viktor-kurchenko]

Thanks! Regarding the ipsecKeyFromString function, this PR takes care of keys like 3 rfc4106(gcm(aes)) 3c7ae6f6c0e9a09255fce39440df90619514709e 128. However, cilium can also accept a key with other formats. I think these have been in your plan, just a reminder. 👍

I've added other key format support.
Thank you very much @jschwinger233 !

[jschwinger233]

Very nice! Thanks Victor! A follow-up could be to support encryption algorithm change, such as from gcm(aes) to cbc(aes).

[asauber]

In the secret format with five fields,

3 hmac(sha256) e6b4bab427cd37bb64b39cd66a8476a62963174b78bc544fb525f4c2f548342b cbc(aes) 0f12337d9ee75095ff21402dc98476f5f9107261073b70bb37747237d2691d3e

the first field is the SPI, the second field is the authentication algorithm, the third field is the authentication key, the fourth field is the symmetric encryption mode, and the fifth field is the symmetric encryption key.

For symmetric encryption mode cbc(aes) means AES using the Cipher Block Chaining mode. There are many other valid algorithm choices and modes, such as ctr(aes), Counter Mode, or xctr(aes), an optimized implementation of Counter Mode. Other ciphers besides AES are also possible, although not available with typical kernel configs.

The reason that GCM has fewer parameters is that it's a mode of encryption that performs a hash and encipherment simultaneously.

All this is to say that your struct should not call the latter parameters cbcAlgo, and cbcRandom, these are the cipherMode and encryptionKey

[viktor-kurchenko]

In the secret format with five fields,

3 hmac(sha256) e6b4bab427cd37bb64b39cd66a8476a62963174b78bc544fb525f4c2f548342b cbc(aes) 0f12337d9ee75095ff21402dc98476f5f9107261073b70bb37747237d2691d3e

the first field is the SPI, the second field is the authentication algorithm, the third field is the authentication key, the fourth field is the symmetric encryption mode, and the fifth field is the symmetric encryption key.

For symmetric encryption mode cbc(aes) means AES using the Cipher Block Chaining mode. There are many other valid algorithm choices and modes, such as ctr(aes), Counter Mode, or xctr(aes), an optimized implementation of Counter Mode. Other ciphers besides AES are also possible, although not available with typical kernel configs.

The reason that GCM has fewer parameters is that it's a mode of encryption that performs a hash and encipherment simultaneously.

All this is to say that your struct should not call the latter parameters cbcAlgo, and cbcRandom, these are the cipherMode and encryptionKey

Thank you for the explanation @asauber.
So, the structure should look like:

type ipsecKey struct {
	spi       int
	spiSuffix bool
	algo      string
	random    string
	size      int
	cipherMode   string
	encryptionKey string
}

correct?

[asauber]

My recommendation would be:

type ipsecKey struct {
	spi          int
	spiSuffix    bool
	algo         string  // Purposefully ambiguous here because of modes like GCM
	key          string
	size         int
	cipherMode   string
	cipherKey    string
}

[viktor-kurchenko]

My recommendation would be:

type ipsecKey struct {
	spi          int
	spiSuffix    bool
	algo         string  // Purposefully ambiguous here because of modes like GCM
	key          string
	size         int
	cipherMode   string
	cipherKey    string
}

Fixed, thank you!

[asauber]

This function name (and other instances of "cbc") should be changed to cipherKeyFromSlice or similar.

[viktor-kurchenko]

Fixed.

[asauber]

Many security issues (CVEs) are the result of imprecise parsing, with Regex often being implicated.

The related threat model here is that an attacker can somehow control the ipsec key Kubernetes secret, which may make such hardening out of scope, although it is something that should be considered.

[viktor-kurchenko]

So, do you think we should avoid using regexp and parse it like a CSV string instead?

[asauber]

My gut feeling is that this is more precise than a simple field split, and is fine for now given the threat model.

[asauber]

Because this size is the length of a hex string, and not a number of bytes, this function is better named generateRandomHex. Otherwise, it's unclear that the size is a number of nybbles.

[viktor-kurchenko]

Makes sense, renamed. Thank you!