add athm build and test issuer [chromium/src : main]
Aykut Bulut (Gerrit)
Aykut Bulut added 1 comment![]( "Open in Gerrit")
# ATHM (Anonymous Tokens with Hidden Metadata): a cxx bridge overI think moving this block to common/BUILD.gn and the source files `athm_ffi.rs`, `athm_test_issuer*` to common would be better.
Related details
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aykut Bulut (Gerrit)
Aykut Bulut added 10 comments![]( "Open in Gerrit")
base::span<const uint8_t> deployment_id);Is there any limit on the size of `deployment_id` in spec? If yes I assume we check it in the downstream code. Should we concern ourselves with that here?
// wire encodings, which is the shape they would take over IPC or the network.Last statement might be a bit misleading. The IPC will happen in a shape similar to the following, once we integrate with the ATHM client side crypto. I think we can just remove that. It is a bit pre-mature to talk chromium IPC here. We can refer to the ATHM spec for serialization etc. if appropriate. Code here should not assume much on how the upper levels in stack are implemented.
AthmKeyMaterial key_material =`AthmKeyMaterial` is in `private_verification_tokens` namespace, ideally we keep this internal to the ATHM implementation and un-accessible outside of it. We can fix this in the Client class that will be used in prod. No AI, writing this as a team note.
Similarly for other types introduced in `athm_ffi.rs`.
AthmBytesResult result = athm_issue(I assume `athm_issue` returns error when `hidden_metadata` is invalid with respect to num_buckets`. Practically we can do that check here but I guess having it fail in `athm_request` and procss the error code (that might be spec'ed to reach here).
No action required. Wanted to confirm whether how I think about this is correct.
AthmClientRequest bridge_result = athm_client_request(I think this can be better if it returns a `std::unique_ptr<>` or `std::optional<>` `ClientRequest.ok` is removed. Similar to `AthmTestIssuer::Create`. `unique_ptr` is good if it is not easy to support move/copy constructor/operator semantics for the class. `ClientRequest` is this case is a struct where default move/copy constructor/operators are supported. `std::optional<>` is more suitable here.
Currently `ClientRequest` might be in an invalid state when `ok` is false. With the change described above, we know once you have a `ClientRequest` it is valid.
base::span<const uint8_t> context,Would it be better if this takes `ClientRequest` as the argument instead of getting `context` and `request` separately?
for (uint8_t metadata = 0; metadata < kBuckets; ++metadata) {A better way to do this might be using parameterized tests. See [`PrivateVerificationTokensIssuerConfigInternalMissingFieldTest`](https://source.chromium.org/chromium/chromium/src/+/main:components/private_verification_tokens/common/private_verification_tokens_issuer_config_unittest.cc;drc=659b68fc2ae543db0f0048d64521eccc94c6f51c;l=245)
EXPECT_FALSE(first.context.empty());A better way to do this is using matchers.
```
EXPECT_THAT(first.context, testing::SizeIs(kExpectedSize));
```
ASSERT_EQ(tampered.size(), 98u);Defining global constants in anonymous namespaces would be good for this.
ASSERT_EQ(tampered.size(), 98u);We should consider writing fuzzers for this and other athm parts in general.
Related details
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Schlesinger (Gerrit)
Sam Schlesinger added 10 comments![]( "Open in Gerrit")
# ATHM (Anonymous Tokens with Hidden Metadata): a cxx bridge overI think moving this block to common/BUILD.gn and the source files `athm_ffi.rs`, `athm_test_issuer*` to common would be better.
Done
Is there any limit on the size of `deployment_id` in spec? If yes I assume we check it in the downstream code. Should we concern ourselves with that here?
Done
// wire encodings, which is the shape they would take over IPC or the network.Last statement might be a bit misleading. The IPC will happen in a shape similar to the following, once we integrate with the ATHM client side crypto. I think we can just remove that. It is a bit pre-mature to talk chromium IPC here. We can refer to the ATHM spec for serialization etc. if appropriate. Code here should not assume much on how the upper levels in stack are implemented.
Done
I assume `athm_issue` returns error when `hidden_metadata` is invalid with respect to num_buckets`. Practically we can do that check here but I guess having it fail in `athm_request` and procss the error code (that might be spec'ed to reach here).
No action required. Wanted to confirm whether how I think about this is correct.
Done
I think this can be better if it returns a `std::unique_ptr<>` or `std::optional<>` `ClientRequest.ok` is removed. Similar to `AthmTestIssuer::Create`. `unique_ptr` is good if it is not easy to support move/copy constructor/operator semantics for the class. `ClientRequest` is this case is a struct where default move/copy constructor/operators are supported. `std::optional<>` is more suitable here.
Currently `ClientRequest` might be in an invalid state when `ok` is false. With the change described above, we know once you have a `ClientRequest` it is valid.
Done
Would it be better if this takes `ClientRequest` as the argument instead of getting `context` and `request` separately?
Done
for (uint8_t metadata = 0; metadata < kBuckets; ++metadata) {A better way to do this might be using parameterized tests. See [`PrivateVerificationTokensIssuerConfigInternalMissingFieldTest`](https://source.chromium.org/chromium/chromium/src/+/main:components/private_verification_tokens/common/private_verification_tokens_issuer_config_unittest.cc;drc=659b68fc2ae543db0f0048d64521eccc94c6f51c;l=245)
Done
A better way to do this is using matchers.
```
EXPECT_THAT(first.context, testing::SizeIs(kExpectedSize));
```
Done
Defining global constants in anonymous namespaces would be good for this.
Done
We should consider writing fuzzers for this and other athm parts in general.
- Aykut Bulut
- David Benjamin
- chrome-third-pa...@google.com
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/third_party/rust/config.gwsq:
third-party-rust: flowe...@google.com
It looks like you’re modifying `//third_party/rust`. An appropriate reviewer has been automatically added to this CL.
`//third_party/rust` reviewer(s): flowe...@google.com
Reviewer source(s):
flowe...@google.com is from context(googleclient/chrome/chromium_gwsq/third_party/rust/config.gwsq)
Related details
- Aykut Bulut
- David Benjamin
- Julia Hansbrough
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Schlesinger (Gerrit)
Sam Schlesinger added 2 comments![]( "Open in Gerrit")
base::span<const uint8_t> deployment_id);Sam SchlesingerIs there any limit on the size of `deployment_id` in spec? If yes I assume we check it in the downstream code. Should we concern ourselves with that here?
Done
The code enforces 255 bytes, but the spec doesn't say a specific number.
AthmBytesResult result = athm_issue(Sam SchlesingerI assume `athm_issue` returns error when `hidden_metadata` is invalid with respect to num_buckets`. Practically we can do that check here but I guess having it fail in `athm_request` and procss the error code (that might be spec'ed to reach here).
No action required. Wanted to confirm whether how I think about this is correct.
Done
Yes, confirmed.
Julia Hansbrough (Gerrit)
Julia Hansbrough added 1 comment![]( "Open in Gerrit")
It looks like this is adding a new third-party Rust crate. Can the addition of the crate please be done in a separate CL?
Related details
- Aykut Bulut
- David Benjamin
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Schlesinger (Gerrit)
Sam Schlesinger added 1 comment![]( "Open in Gerrit")
It looks like this is adding a new third-party Rust crate. Can the addition of the crate please be done in a separate CL?
Sure, will do. I'm adding two crates, for context: athm and zeroize, zeroize is a dependency of athm. Sounds like you want a separate CL for zeroize then the athm stuff here, or should we split it up further?
Related details
- Aykut Bulut
- David Benjamin
- Julia Hansbrough
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Julia Hansbrough (Gerrit)
Julia Hansbrough added 1 comment![]( "Open in Gerrit")
Sam SchlesingerIt looks like this is adding a new third-party Rust crate. Can the addition of the crate please be done in a separate CL?
Sure, will do. I'm adding two crates, for context: athm and zeroize, zeroize is a dependency of athm. Sounds like you want a separate CL for zeroize then the athm stuff here, or should we split it up further?
- Aykut Bulut
- David Benjamin
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aykut Bulut (Gerrit)
Aykut Bulut added 1 comment![]( "Open in Gerrit")
Thanks Sam, PVT changes LGTM. Some of my comments fell through the cracks, but we can fix them as we go (returning unique_ptr, parameterized testing, fuzzing etc).
I saw comments on splitting the CL which is great. I will re-review the PVT changes once they are separated to a new CL.
Related details
- David Benjamin
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
# directory. NEEDS BORINGSSL OWNERS SIGN-OFF.Hmm. @schop...@google.com asserted to me that you all only needed scalar add, and thus you had solved your constant-time problems. But I see you all are doing scalar multiply, and not in a constant-time way. (Multiplication requires Montgomery multiplication to be efficient and constant-time, which is a leaky abstraction. I.e. the strategy to use Rust's mul trait is not going to set you up for success here.)
Can you clarify what's going on and the list of operations you *actually* need?
Related details
- Phillipp Schoppmann
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
# directory. NEEDS BORINGSSL OWNERS SIGN-OFF.Hmm. @schop...@google.com asserted to me that you all only needed scalar add, and thus you had solved your constant-time problems. But I see you all are doing scalar multiply, and not in a constant-time way. (Multiplication requires Montgomery multiplication to be efficient and constant-time, which is a leaky abstraction. I.e. the strategy to use Rust's mul trait is not going to set you up for success here.)
Can you clarify what's going on and the list of operations you *actually* need?
Sam Schlesinger (Gerrit)
Sam Schlesinger added 2 comments![]( "Open in Gerrit")
Sam SchlesingerIt looks like this is adding a new third-party Rust crate. Can the addition of the crate please be done in a separate CL?
Julia HansbroughSure, will do. I'm adding two crates, for context: athm and zeroize, zeroize is a dependency of athm. Sounds like you want a separate CL for zeroize then the athm stuff here, or should we split it up further?
That split sounds fine to me!
Thank you, will do that.
# directory. NEEDS BORINGSSL OWNERS SIGN-OFF.David BenjaminHmm. @schop...@google.com asserted to me that you all only needed scalar add, and thus you had solved your constant-time problems. But I see you all are doing scalar multiply, and not in a constant-time way. (Multiplication requires Montgomery multiplication to be efficient and constant-time, which is a leaky abstraction. I.e. the strategy to use Rust's mul trait is not going to set you up for success here.)
Can you clarify what's going on and the list of operations you *actually* need?
Specifically this code here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/anonymous_tokens/src/athm/src/backend/boringssl.rs;drc=5297514b9c3e05684182103f434ee218c97afed9;l=372
Good catch! I'll look into this today, thanks for checking in detail.
Related details
- David Benjamin
- Julia Hansbrough
- Phillipp Schoppmann
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
# directory. NEEDS BORINGSSL OWNERS SIGN-OFF.David BenjaminHmm. @schop...@google.com asserted to me that you all only needed scalar add, and thus you had solved your constant-time problems. But I see you all are doing scalar multiply, and not in a constant-time way. (Multiplication requires Montgomery multiplication to be efficient and constant-time, which is a leaky abstraction. I.e. the strategy to use Rust's mul trait is not going to set you up for success here.)
Can you clarify what's going on and the list of operations you *actually* need?
Sam SchlesingerSpecifically this code here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/anonymous_tokens/src/athm/src/backend/boringssl.rs;drc=5297514b9c3e05684182103f434ee218c97afed9;l=372
Good catch! I'll look into this today, thanks for checking in detail.
BTW, between this and another feature, I think we'll just expose `EC_SCALAR` in BoringSSL.
But you'll still have to substantially rework that code because the interface you picked want needs to be aware of Montgomery reduction.
Related details
- Julia Hansbrough
- Phillipp Schoppmann
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Schlesinger (Gerrit)
Sam Schlesinger added 1 comment![]( "Open in Gerrit")
# directory. NEEDS BORINGSSL OWNERS SIGN-OFF.David BenjaminHmm. @schop...@google.com asserted to me that you all only needed scalar add, and thus you had solved your constant-time problems. But I see you all are doing scalar multiply, and not in a constant-time way. (Multiplication requires Montgomery multiplication to be efficient and constant-time, which is a leaky abstraction. I.e. the strategy to use Rust's mul trait is not going to set you up for success here.)
Can you clarify what's going on and the list of operations you *actually* need?
Sam SchlesingerSpecifically this code here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/anonymous_tokens/src/athm/src/backend/boringssl.rs;drc=5297514b9c3e05684182103f434ee218c97afed9;l=372
David BenjaminGood catch! I'll look into this today, thanks for checking in detail.
BTW, between this and another feature, I think we'll just expose `EC_SCALAR` in BoringSSL.
But you'll still have to substantially rework that code because the interface you picked want needs to be aware of Montgomery reduction.
After reviewing and remembering my discussions with Phillipp, his argument is that it is fine as scalar multiplication isn't used by the client, so this is fine. I double-checked this today, but I could have missed something so I'll wait to merge this until Phillipp takes a look as well before merging this. I believe we discussed and reviewed this when we merged the boringssl backend though. The boringssl backend should definitely not be used by an issuer for this reason though, so maybe we need to make that even clearer in the chrome codebase in case issuers just pull this code to use.
Related details
- David Benjamin
- Julia Hansbrough
- Phillipp Schoppmann
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
# directory. NEEDS BORINGSSL OWNERS SIGN-OFF.David BenjaminHmm. @schop...@google.com asserted to me that you all only needed scalar add, and thus you had solved your constant-time problems. But I see you all are doing scalar multiply, and not in a constant-time way. (Multiplication requires Montgomery multiplication to be efficient and constant-time, which is a leaky abstraction. I.e. the strategy to use Rust's mul trait is not going to set you up for success here.)
Can you clarify what's going on and the list of operations you *actually* need?
Sam SchlesingerSpecifically this code here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/anonymous_tokens/src/athm/src/backend/boringssl.rs;drc=5297514b9c3e05684182103f434ee218c97afed9;l=372
David BenjaminGood catch! I'll look into this today, thanks for checking in detail.
Sam SchlesingerBTW, between this and another feature, I think we'll just expose `EC_SCALAR` in BoringSSL.
But you'll still have to substantially rework that code because the interface you picked want needs to be aware of Montgomery reduction.
After reviewing and remembering my discussions with Phillipp, his argument is that it is fine as scalar multiplication isn't used by the client, so this is fine. I double-checked this today, but I could have missed something so I'll wait to merge this until Phillipp takes a look as well before merging this. I believe we discussed and reviewed this when we merged the boringssl backend though. The boringssl backend should definitely not be used by an issuer for this reason though, so maybe we need to make that even clearer in the chrome codebase in case issuers just pull this code to use.
If you're not using it, the code shouldn't be in there. That will (rightfully) cause every reviewer to raise their eyebrows at using a non-constant-time modular multiplication function on a secret.
(What are you using for the server? The `p256` crate? At a glance, it looks like you were able to fit it into that API because they're using Barrett reduction instead of Montgomery reduction. That's a little surprising to me since I think Montgomery reduction is more common. I suppose you could still implement this API using Montgomery reduction, you'd just a too restrictive abstraction that leaves performance on the table whenever you have multiple multiplications to perform.)
BTW, between this and another feature, I think we'll just expose EC_SCALAR in BoringSSL.
- Julia Hansbrough
- Phillipp Schoppmann
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillipp Schoppmann (Gerrit)
Phillipp Schoppmann added 1 comment![]( "Open in Gerrit")
# directory. NEEDS BORINGSSL OWNERS SIGN-OFF.David BenjaminHmm. @schop...@google.com asserted to me that you all only needed scalar add, and thus you had solved your constant-time problems. But I see you all are doing scalar multiply, and not in a constant-time way. (Multiplication requires Montgomery multiplication to be efficient and constant-time, which is a leaky abstraction. I.e. the strategy to use Rust's mul trait is not going to set you up for success here.)
Can you clarify what's going on and the list of operations you *actually* need?
Sam SchlesingerSpecifically this code here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/anonymous_tokens/src/athm/src/backend/boringssl.rs;drc=5297514b9c3e05684182103f434ee218c97afed9;l=372
David BenjaminGood catch! I'll look into this today, thanks for checking in detail.
Sam SchlesingerBTW, between this and another feature, I think we'll just expose `EC_SCALAR` in BoringSSL.
But you'll still have to substantially rework that code because the interface you picked want needs to be aware of Montgomery reduction.
David BenjaminAfter reviewing and remembering my discussions with Phillipp, his argument is that it is fine as scalar multiplication isn't used by the client, so this is fine. I double-checked this today, but I could have missed something so I'll wait to merge this until Phillipp takes a look as well before merging this. I believe we discussed and reviewed this when we merged the boringssl backend though. The boringssl backend should definitely not be used by an issuer for this reason though, so maybe we need to make that even clearer in the chrome codebase in case issuers just pull this code to use.
If you're not using it, the code shouldn't be in there. That will (rightfully) cause every reviewer to raise their eyebrows at using a non-constant-time modular multiplication function on a secret.
(What are you using for the server? The `p256` crate? At a glance, it looks like you were able to fit it into that API because they're using Barrett reduction instead of Montgomery reduction. That's a little surprising to me since I think Montgomery reduction is more common. I suppose you could still implement this API using Montgomery reduction, you'd just a too restrictive abstraction that leaves performance on the table whenever you have multiple multiplications to perform.)
BTW, between this and another feature, I think we'll just expose EC_SCALAR in BoringSSL.
crbug/529737443
The library currently implements both client and server side of the protocol. However only the client side is used by Chrome. I don't know if there's a way to remove the server side implementation when importing the code, replacing all implementations we don't need by `unimplemented!`? Alternatively we could try to split up the backend traits into server and client side, and only implement the client side using boringssl. WDYT @samschl...@google.com @davi...@chromium.org ?
And yes, on the server side we use the p256 crate / the rustcrypto backend for all operations.
Related details
- Julia Hansbrough
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Julia Hansbrough (Gerrit)
Julia Hansbrough added 1 comment![]( "Open in Gerrit")
removing myself from the attention set since i don't think i have the context for some of these discussions; feel free to add me again if e.g. the CLs have been split and you need someone to review a new third-party crate, etc
Related details
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Schlesinger (Gerrit)
Sam Schlesinger added 1 comment![]( "Open in Gerrit")
removing myself from the attention set since i don't think i have the context for some of these discussions; feel free to add me again if e.g. the CLs have been split and you need someone to review a new third-party crate, etc
I have indeed split the CL and need someone to review athm and the other CL for zeroize: https://chromium-review.googlesource.com/c/chromium/src/+/8075619.
Related details
- Julia Hansbrough
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Julia Hansbrough (Gerrit)
Julia Hansbrough added 1 comment![]( "Open in Gerrit")
(removing myself fomr the review line because as per an offline convo w/ sam i don't think my input is needed; lmk if that's not correct)
Related details
- Sam Schlesinger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Schlesinger (Gerrit)
Sam Schlesinger added 2 comments![]( "Open in Gerrit")
It looks like this is adding a new third-party Rust crate. Can the addition of the crate please be done in a separate CL?
Julia HansbroughSure, will do. I'm adding two crates, for context: athm and zeroize, zeroize is a dependency of athm. Sounds like you want a separate CL for zeroize then the athm stuff here, or should we split it up further?
Sam SchlesingerThat split sounds fine to me!
Thank you, will do that.
Done
# directory. NEEDS BORINGSSL OWNERS SIGN-OFF.David BenjaminHmm. @schop...@google.com asserted to me that you all only needed scalar add, and thus you had solved your constant-time problems. But I see you all are doing scalar multiply, and not in a constant-time way. (Multiplication requires Montgomery multiplication to be efficient and constant-time, which is a leaky abstraction. I.e. the strategy to use Rust's mul trait is not going to set you up for success here.)
Can you clarify what's going on and the list of operations you *actually* need?
Sam SchlesingerSpecifically this code here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/anonymous_tokens/src/athm/src/backend/boringssl.rs;drc=5297514b9c3e05684182103f434ee218c97afed9;l=372
David BenjaminGood catch! I'll look into this today, thanks for checking in detail.
Sam SchlesingerBTW, between this and another feature, I think we'll just expose `EC_SCALAR` in BoringSSL.
But you'll still have to substantially rework that code because the interface you picked want needs to be aware of Montgomery reduction.
David BenjaminAfter reviewing and remembering my discussions with Phillipp, his argument is that it is fine as scalar multiplication isn't used by the client, so this is fine. I double-checked this today, but I could have missed something so I'll wait to merge this until Phillipp takes a look as well before merging this. I believe we discussed and reviewed this when we merged the boringssl backend though. The boringssl backend should definitely not be used by an issuer for this reason though, so maybe we need to make that even clearer in the chrome codebase in case issuers just pull this code to use.
Phillipp SchoppmannIf you're not using it, the code shouldn't be in there. That will (rightfully) cause every reviewer to raise their eyebrows at using a non-constant-time modular multiplication function on a secret.
(What are you using for the server? The `p256` crate? At a glance, it looks like you were able to fit it into that API because they're using Barrett reduction instead of Montgomery reduction. That's a little surprising to me since I think Montgomery reduction is more common. I suppose you could still implement this API using Montgomery reduction, you'd just a too restrictive abstraction that leaves performance on the table whenever you have multiple multiplications to perform.)
BTW, between this and another feature, I think we'll just expose EC_SCALAR in BoringSSL.
crbug/529737443
The library currently implements both client and server side of the protocol. However only the client side is used by Chrome. I don't know if there's a way to remove the server side implementation when importing the code, replacing all implementations we don't need by `unimplemented!`? Alternatively we could try to split up the backend traits into server and client side, and only implement the client side using boringssl. WDYT @samschl...@google.com @davi...@chromium.org ?
And yes, on the server side we use the p256 crate / the rustcrypto backend for all operations.
In discussions, we decided we actually had a path to full constant time, so we implemented that and this should be resolved.
Related details
- David Benjamin
- Phillipp Schoppmann
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |