| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
LGTM w/ nits
bssl::UniquePtr<EVP_PKEY_CTX> ctx(EVP_PKEY_CTX_new(key_pair_.key(), nullptr));`std::unique_ptr`? `bssl::UniquePtr` is just alias of `std::unique_ptr`, and it seems you are only passing the raw pointer around.
```suggestion
std::unique_ptr<EVP_PKEY_CTX> ctx(EVP_PKEY_CTX_new(key_pair_.key(), nullptr));
```
shared_secret.resize(shared_secret_len);Do you need this call, since the constructor already default-inserts `shared_secret` with `shared_secret_len` 0s?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thanks Yuwei! Elly, please let me know if this looks reasonable when you get a chance.
bssl::UniquePtr<EVP_PKEY_CTX> ctx(EVP_PKEY_CTX_new(key_pair_.key(), nullptr));`std::unique_ptr`? `bssl::UniquePtr` is just alias of `std::unique_ptr`, and it seems you are only passing the raw pointer around.
```suggestion
std::unique_ptr<EVP_PKEY_CTX> ctx(EVP_PKEY_CTX_new(key_pair_.key(), nullptr));
```
bssl::UniquePtr uses a custom deleter so it is a little different.
In an earlier iteration I was using unique_ptr with that deleter and then found that bssl::UniquePtr existed which simplifed our code and s commonly used in Chromium when interacting with crypto APIs.
shared_secret.resize(shared_secret_len);Do you need this call, since the constructor already default-inserts `shared_secret` with `shared_secret_len` 0s?
Yeah you still need to resize. The initial shared secret length represents the largest value that can be generated but the actual value may be smaller so we need to resize to the actual length so we don't include garbage data at the end of the vector.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Add ECDH message crypter class for P2P control channel encryptionis the cryptographic design used here matching some existing spec? if so, can you link to it? if not, and you are designing something new here, we should talk more :)
// The AES-GCM encryption/decryption key.
std::unique_ptr<crypto::Aead> encryption_key_;normally this kind of thing is called a "context"
// A Base64 encoded representation of the public key, useful for logging.
std::string GetPublicKey() const;usually this would be (eg) PublicKeyAsBase64()
// The IV is prepended to the ciphertext. Returns std::nullopt on failure.is the IV random or fixed? is it generated by this object?
class EcdhAesCrypter {I might split this into two smaller classes like this:
1. An EcdhExchange class, which handles generating a private/public key pair, and has a DeriveCrypter() method
2. A nested Crypter class, which provides Encrypt() and Decrypt() methods
where EcdhExchange::DeriveCrypter() is like:
```
std::optional<Crypter> EcdhExchange::DeriveCrypter(base::span<const uint8_t> peer_public_key);
```
and the constructor for Crypter itself is only accessible to EcdhExchange. That way it's impossible to even _try_ to call Encrypt() / Decrypt() without first successfully deriving shared keys. Does that make sense?
constexpr int kAesGcmIvSizeBits = 96;
constexpr int kAesGcmKeySizeBits = 256;given that these are never used without being divided into bytes first I'd just represent them as byte counts directly
bool EcdhAesCrypter::DeriveEncryptionKey(I think the body of this method should instead be added to crypto/kex - we have crypto::kex::EcdhP256(), we could easily have a crypto::kex::EcdhP384(). The HKDF part can use crypto::kdf::Hkdf() from crypto/kdf as well, which doesn't require you to construct a string view.
base::span<const uint8_t> nonce = ciphertext_iv.first(kAesGcmIvSizeBytes);
base::span<const uint8_t> ciphertext =
ciphertext_iv.subspan(kAesGcmIvSizeBytes);you can use base::span::split_at()
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thanks for taking a look Elly! I'll look at refactoring the classes and moving code around and send you a follow-up. I'm also happy to chat offline about use case specific info.
Add ECDH message crypter class for P2P control channel encryptionis the cryptographic design used here matching some existing spec? if so, can you link to it? if not, and you are designing something new here, we should talk more :)
The TL;DR is that I'm implementing a signaling service for Google Corp CRD connections: go/crd-corp-signaling-design
I am going to be vague on the specifics here but certain use cases will require that the protobufs sent over the P2P channel are encrypted with a secret unknown to the session authorization service. I'm working on that aspect of the system now and plan to document that approach when I have something viable. I'm happy to have a quick GVC to dive into details if that is useful for context.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Add ECDH message crypter class for P2P control channel encryptionJoe Downingis the cryptographic design used here matching some existing spec? if so, can you link to it? if not, and you are designing something new here, we should talk more :)
The TL;DR is that I'm implementing a signaling service for Google Corp CRD connections: go/crd-corp-signaling-design
I am going to be vague on the specifics here but certain use cases will require that the protobufs sent over the P2P channel are encrypted with a secret unknown to the session authorization service. I'm working on that aspect of the system now and plan to document that approach when I have something viable. I'm happy to have a quick GVC to dive into details if that is useful for context.
Gotcha, okay. If you are doing a fresh design, you should use HPKE (RFC 9180). It's similar to what you are doing here.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shared_secret.resize(shared_secret_len);Joe DowningDo you need this call, since the constructor already default-inserts `shared_secret` with `shared_secret_len` 0s?
Yeah you still need to resize. The initial shared secret length represents the largest value that can be generated but the actual value may be smaller so we need to resize to the actual length so we don't include garbage data at the end of the vector.
Ah, I missed that `EVP_PKEY_derive` is writing back to `shared_secret_len`..
PTAL! Yuwei, can you take another look since your +1 was reset and I've made a fair number of changes?
Thanks!
Add ECDH message crypter class for P2P control channel encryptionJoe Downingis the cryptographic design used here matching some existing spec? if so, can you link to it? if not, and you are designing something new here, we should talk more :)
Elly FJThe TL;DR is that I'm implementing a signaling service for Google Corp CRD connections: go/crd-corp-signaling-design
I am going to be vague on the specifics here but certain use cases will require that the protobufs sent over the P2P channel are encrypted with a secret unknown to the session authorization service. I'm working on that aspect of the system now and plan to document that approach when I have something viable. I'm happy to have a quick GVC to dive into details if that is useful for context.
Gotcha, okay. If you are doing a fresh design, you should use HPKE (RFC 9180). It's similar to what you are doing here.
Thanks! I am working with a website client and HPKE APIs appear to be missing from the current crypto web API but it looks like there are some interesting WebRTC APIs I can use in a similar fashion so I'll consider this when I begin that part of the implementation.
// The AES-GCM encryption/decryption key.
std::unique_ptr<crypto::Aead> encryption_key_;normally this kind of thing is called a "context"
Done
// A Base64 encoded representation of the public key, useful for logging.
std::string GetPublicKey() const;usually this would be (eg) PublicKeyAsBase64()
Done
// The IV is prepended to the ciphertext. Returns std::nullopt on failure.is the IV random or fixed? is it generated by this object?
For messages received by the website peer, the the IV is random and generated via a common crypto base library. This function is currently used for testing and generates a random IV via a chromium base library.
I can reword the comment to be more clear.
I might split this into two smaller classes like this:
1. An EcdhExchange class, which handles generating a private/public key pair, and has a DeriveCrypter() method
2. A nested Crypter class, which provides Encrypt() and Decrypt() methodswhere EcdhExchange::DeriveCrypter() is like:
```
std::optional<Crypter> EcdhExchange::DeriveCrypter(base::span<const uint8_t> peer_public_key);
```and the constructor for Crypter itself is only accessible to EcdhExchange. That way it's impossible to even _try_ to call Encrypt() / Decrypt() without first successfully deriving shared keys. Does that make sense?
Done
constexpr int kAesGcmIvSizeBits = 96;
constexpr int kAesGcmKeySizeBits = 256;given that these are never used without being divided into bytes first I'd just represent them as byte counts directly
Done
I think the body of this method should instead be added to crypto/kex - we have crypto::kex::EcdhP256(), we could easily have a crypto::kex::EcdhP384(). The HKDF part can use crypto::kdf::Hkdf() from crypto/kdf as well, which doesn't require you to construct a string view.
Done
base::span<const uint8_t> nonce = ciphertext_iv.first(kAesGcmIvSizeBytes);
base::span<const uint8_t> ciphertext =
ciphertext_iv.subspan(kAesGcmIvSizeBytes);you can use base::span::split_at()
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Add ECDH message crypter class for P2P control channel encryptionJoe Downingis the cryptographic design used here matching some existing spec? if so, can you link to it? if not, and you are designing something new here, we should talk more :)
Elly FJThe TL;DR is that I'm implementing a signaling service for Google Corp CRD connections: go/crd-corp-signaling-design
I am going to be vague on the specifics here but certain use cases will require that the protobufs sent over the P2P channel are encrypted with a secret unknown to the session authorization service. I'm working on that aspect of the system now and plan to document that approach when I have something viable. I'm happy to have a quick GVC to dive into details if that is useful for context.
Joe DowningGotcha, okay. If you are doing a fresh design, you should use HPKE (RFC 9180). It's similar to what you are doing here.
Thanks! I am working with a website client and HPKE APIs appear to be missing from the current crypto web API but it looks like there are some interesting WebRTC APIs I can use in a similar fashion so I'll consider this when I begin that part of the implementation.
Wait, sorry - are you implementing an existing cryptographic protocol such as HPKE, or are you designing your own cryptographic protocol?
std::vector<uint8_t> nonce_bytes(kAesGcmIvSizeBytes);
base::RandBytes(nonce_bytes);```suggestion
const auto nonce = crypto::RandBytesAsArray<kAesGcmIvSizeBytes>();
```
EcdhKeyExchange host_key_exchange;
EcdhKeyExchange client_key_exchange;
auto host_crypter = host_key_exchange.CreateAesGcmCrypter(
client_key_exchange.public_key_bytes());
ASSERT_TRUE(host_crypter);
auto client_crypter = client_key_exchange.CreateAesGcmCrypter(
host_key_exchange.public_key_bytes());
ASSERT_TRUE(client_crypter);it seems like a lot of tests start this way... I wonder about a helper function:
```c++
std::pair<std::unique_ptr<EcdhKeyExchange::AesGcmCrypter>,
std::unique_ptr<EcdhKeyExchange::AesGcmCrypter>> MakeCrypterPair();
```
and then the test bodies can be like:
```c++
auto [server, client] = MakeCrypterPair();
// ...
```
because it's safe for the AesGcmCrypters to outlive the EcdhKeyExchanges.
what do you think?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
PTAL!
Add ECDH message crypter class for P2P control channel encryptionJoe Downingis the cryptographic design used here matching some existing spec? if so, can you link to it? if not, and you are designing something new here, we should talk more :)
Elly FJThe TL;DR is that I'm implementing a signaling service for Google Corp CRD connections: go/crd-corp-signaling-design
I am going to be vague on the specifics here but certain use cases will require that the protobufs sent over the P2P channel are encrypted with a secret unknown to the session authorization service. I'm working on that aspect of the system now and plan to document that approach when I have something viable. I'm happy to have a quick GVC to dive into details if that is useful for context.
Joe DowningGotcha, okay. If you are doing a fresh design, you should use HPKE (RFC 9180). It's similar to what you are doing here.
Elly FJThanks! I am working with a website client and HPKE APIs appear to be missing from the current crypto web API but it looks like there are some interesting WebRTC APIs I can use in a similar fashion so I'll consider this when I begin that part of the implementation.
Wait, sorry - are you implementing an existing cryptographic protocol such as HPKE, or are you designing your own cryptographic protocol?
I'm not designing my own protocol AFAIK. I'm using standard mechanisms like ECDH in our existing architecture to effectively double-encrypt some protobufs such that a MitM-style of WebRTC service cannot intercept or inject them. The P2P channels are already encrypted between the website client and native host agent built in Chromium. I'd be happy to go into specifics for this scenario offline (i.e. via GVC) and FWIW I am already engaged with several Corp security folks on this but I'm happy to talk with you about it as well since more scrutiny is good when considering security. I would prefer to leave the specifics out of this thread however.
std::vector<uint8_t> nonce_bytes(kAesGcmIvSizeBytes);
base::RandBytes(nonce_bytes);```suggestion
const auto nonce = crypto::RandBytesAsArray<kAesGcmIvSizeBytes>();
```
Done
EcdhKeyExchange host_key_exchange;
EcdhKeyExchange client_key_exchange;
auto host_crypter = host_key_exchange.CreateAesGcmCrypter(
client_key_exchange.public_key_bytes());
ASSERT_TRUE(host_crypter);
auto client_crypter = client_key_exchange.CreateAesGcmCrypter(
host_key_exchange.public_key_bytes());
ASSERT_TRUE(client_crypter);it seems like a lot of tests start this way... I wonder about a helper function:
```c++
std::pair<std::unique_ptr<EcdhKeyExchange::AesGcmCrypter>,
std::unique_ptr<EcdhKeyExchange::AesGcmCrypter>> MakeCrypterPair();
```and then the test bodies can be like:
```c++
auto [server, client] = MakeCrypterPair();
// ...
```because it's safe for the AesGcmCrypters to outlive the EcdhKeyExchanges.
what do you think?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Add ECDH message crypter class for P2P control channel encryptionJoe Downingis the cryptographic design used here matching some existing spec? if so, can you link to it? if not, and you are designing something new here, we should talk more :)
Elly FJThe TL;DR is that I'm implementing a signaling service for Google Corp CRD connections: go/crd-corp-signaling-design
I am going to be vague on the specifics here but certain use cases will require that the protobufs sent over the P2P channel are encrypted with a secret unknown to the session authorization service. I'm working on that aspect of the system now and plan to document that approach when I have something viable. I'm happy to have a quick GVC to dive into details if that is useful for context.
Joe DowningGotcha, okay. If you are doing a fresh design, you should use HPKE (RFC 9180). It's similar to what you are doing here.
Elly FJThanks! I am working with a website client and HPKE APIs appear to be missing from the current crypto web API but it looks like there are some interesting WebRTC APIs I can use in a similar fashion so I'll consider this when I begin that part of the implementation.
Joe DowningWait, sorry - are you implementing an existing cryptographic protocol such as HPKE, or are you designing your own cryptographic protocol?
I'm not designing my own protocol AFAIK. I'm using standard mechanisms like ECDH in our existing architecture to effectively double-encrypt some protobufs such that a MitM-style of WebRTC service cannot intercept or inject them. The P2P channels are already encrypted between the website client and native host agent built in Chromium. I'd be happy to go into specifics for this scenario offline (i.e. via GVC) and FWIW I am already engaged with several Corp security folks on this but I'm happy to talk with you about it as well since more scrutiny is good when considering security. I would prefer to leave the specifics out of this thread however.
I realized I didn't address the HPKE question. My <limited> understanding is we don't have an HPKE library in //crypto or //third_party I can use in Chromium. Were you suggesting that I add something like Tink to third_party?
If so, I would be amenable to that but it's a lot of work and presumably it would be something your team should be the OWNER for long-term.
Please let me know if this is what you were thinking/suggesting or if I'm off base : )
| Code-Review | +1 |
lgtm!
Add ECDH message crypter class for P2P control channel encryptionJoe Downingis the cryptographic design used here matching some existing spec? if so, can you link to it? if not, and you are designing something new here, we should talk more :)
Elly FJThe TL;DR is that I'm implementing a signaling service for Google Corp CRD connections: go/crd-corp-signaling-design
I am going to be vague on the specifics here but certain use cases will require that the protobufs sent over the P2P channel are encrypted with a secret unknown to the session authorization service. I'm working on that aspect of the system now and plan to document that approach when I have something viable. I'm happy to have a quick GVC to dive into details if that is useful for context.
Joe DowningGotcha, okay. If you are doing a fresh design, you should use HPKE (RFC 9180). It's similar to what you are doing here.
Elly FJThanks! I am working with a website client and HPKE APIs appear to be missing from the current crypto web API but it looks like there are some interesting WebRTC APIs I can use in a similar fashion so I'll consider this when I begin that part of the implementation.
Joe DowningWait, sorry - are you implementing an existing cryptographic protocol such as HPKE, or are you designing your own cryptographic protocol?
Joe DowningI'm not designing my own protocol AFAIK. I'm using standard mechanisms like ECDH in our existing architecture to effectively double-encrypt some protobufs such that a MitM-style of WebRTC service cannot intercept or inject them. The P2P channels are already encrypted between the website client and native host agent built in Chromium. I'd be happy to go into specifics for this scenario offline (i.e. via GVC) and FWIW I am already engaged with several Corp security folks on this but I'm happy to talk with you about it as well since more scrutiny is good when considering security. I would prefer to leave the specifics out of this thread however.
I realized I didn't address the HPKE question. My <limited> understanding is we don't have an HPKE library in //crypto or //third_party I can use in Chromium. Were you suggesting that I add something like Tink to third_party?
If so, I would be amenable to that but it's a lot of work and presumably it would be something your team should be the OWNER for long-term.
Please let me know if this is what you were thinking/suggesting or if I'm off base : )
BoringSSL already has HPKE support: https://source.chromium.org/chromium/chromium/src/+/main:third_party/boringssl/src/include/openssl/hpke.h - there isn't an existing //crypto wrapper for it, but you could use it directly if you wanted to pending me adding a wrapper.
The design you have here is essentially HPKE-P256-HKDF-SHA256 in base mode already, so I wonder if you can just use that instead of doing ECDH + AEAD yourself. The reason you may wish to do this is that the HPKE implementation will take care of stuff like choosing nonces for you, and if you ever decide to change algorithms (eg to something post-quantum) you just need to switch which HPKE algorithm you are using.
However, if you don't want to use the BoringSSL APIs for HPKE, and prefer to do what you're doing here, that is okay with me too.
+cc davidben@
// Storage for the derived encryption key. Must outlive |context_|.
DerivedKey derived_key_;this will become unnecessary once I land https://chromium-review.googlesource.com/c/chromium/src/+/7477298 :)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +2 |
Add ECDH message crypter class for P2P control channel encryptionJoe Downingis the cryptographic design used here matching some existing spec? if so, can you link to it? if not, and you are designing something new here, we should talk more :)
Elly FJThe TL;DR is that I'm implementing a signaling service for Google Corp CRD connections: go/crd-corp-signaling-design
I am going to be vague on the specifics here but certain use cases will require that the protobufs sent over the P2P channel are encrypted with a secret unknown to the session authorization service. I'm working on that aspect of the system now and plan to document that approach when I have something viable. I'm happy to have a quick GVC to dive into details if that is useful for context.
Joe DowningGotcha, okay. If you are doing a fresh design, you should use HPKE (RFC 9180). It's similar to what you are doing here.
Elly FJThanks! I am working with a website client and HPKE APIs appear to be missing from the current crypto web API but it looks like there are some interesting WebRTC APIs I can use in a similar fashion so I'll consider this when I begin that part of the implementation.
Joe DowningWait, sorry - are you implementing an existing cryptographic protocol such as HPKE, or are you designing your own cryptographic protocol?
Joe DowningI'm not designing my own protocol AFAIK. I'm using standard mechanisms like ECDH in our existing architecture to effectively double-encrypt some protobufs such that a MitM-style of WebRTC service cannot intercept or inject them. The P2P channels are already encrypted between the website client and native host agent built in Chromium. I'd be happy to go into specifics for this scenario offline (i.e. via GVC) and FWIW I am already engaged with several Corp security folks on this but I'm happy to talk with you about it as well since more scrutiny is good when considering security. I would prefer to leave the specifics out of this thread however.
Elly FJI realized I didn't address the HPKE question. My <limited> understanding is we don't have an HPKE library in //crypto or //third_party I can use in Chromium. Were you suggesting that I add something like Tink to third_party?
If so, I would be amenable to that but it's a lot of work and presumably it would be something your team should be the OWNER for long-term.
Please let me know if this is what you were thinking/suggesting or if I'm off base : )
BoringSSL already has HPKE support: https://source.chromium.org/chromium/chromium/src/+/main:third_party/boringssl/src/include/openssl/hpke.h - there isn't an existing //crypto wrapper for it, but you could use it directly if you wanted to pending me adding a wrapper.
The design you have here is essentially HPKE-P256-HKDF-SHA256 in base mode already, so I wonder if you can just use that instead of doing ECDH + AEAD yourself. The reason you may wish to do this is that the HPKE implementation will take care of stuff like choosing nonces for you, and if you ever decide to change algorithms (eg to something post-quantum) you just need to switch which HPKE algorithm you are using.
However, if you don't want to use the BoringSSL APIs for HPKE, and prefer to do what you're doing here, that is okay with me too.
+cc davidben@
OK thanks for the extra info. I poked around a bit and see some other folks with Tink integrations (e.g., components/signin/public/base/hybrid_encryption_key.cc) so that looks like the right way to go.
I'm going to land what I have for now as I need to have the P2P proto encryption working in ~2 weeks for internal dogfooders but can replace the ECDH mechanism with HPKE afterwards.
// Storage for the derived encryption key. Must outlive |context_|.
DerivedKey derived_key_;this will become unnecessary once I land https://chromium-review.googlesource.com/c/chromium/src/+/7477298 :)
Thanks for the heads-up! I've updated my CL to use the new Aead c'tor.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: remoting/base/ecdh_key_exchange.cc
Insertions: 2, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: remoting/base/ecdh_key_exchange.h
Insertions: 1, Deletions: 4.
The diff is too large to show. Please review the diff.
```
Add ECDH message crypter class for P2P control channel encryption
In order to support new features like session recording, which will
require an intermediate service in the P2P connection, we will need
to be able to encrypt sensitive content (e.g., input, auth tokens)
so that information is not available or injectable by the
intermediate service.
This CL introduces the class we will use for establishing the
symmetric keys and decrypting the payloads received from the
client.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |