Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1631123002:
The key conversion algorithm for Token binding (Closed)
|
Created:
4 years, 11 months ago by sgurun-gerrit only Modified:
4 years, 11 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe key conversion algorithm for Token binding
BUG=576874
Convert the keys from native format to Java.
Committed: https://crrev.com/0905e04fc78ee84151ef48171d275ae61adc040f
Cr-Commit-Position: refs/heads/master@{#371430}
Patch Set 1 #Patch Set 2 : minor update #
Total comments: 15
Patch Set 3 : address code review #Patch Set 4 : named const #
Messages
Total messages: 19 (8 generated)
Description was changed from ========== The key conversion algorithm for Token binding BUG=576874 Convert the keys from native format to Java. ========== to ========== The key conversion algorithm for Token binding BUG=576874 Convert the keys from native format to Java. ==========
sgurun@chromium.org changed reviewers: + kpaulhamus@chromium.org
On 2016/01/26 01:23:58, sgurun wrote: > mailto:sgurun@chromium.org changed reviewers: > + mailto:kpaulhamus@chromium.org Kim, please take a look at the key conversion routines
sgurun@chromium.org changed reviewers: + boliu@chromium.org
On 2016/01/26 01:24:17, sgurun wrote: > On 2016/01/26 01:23:58, sgurun wrote: > > mailto:sgurun@chromium.org changed reviewers: > > + mailto:kpaulhamus@chromium.org > > Kim, please take a look at the key conversion routines Bo, please take a look at general stuff,
lgtm
mnaganov@chromium.org changed reviewers: + mnaganov@chromium.org
Some drive-bys. https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java (right): https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java:75: if (privateKeyBytes != null && publicKeyBytes != null) { I think it makes sense to make an early return here in order to reduce indentation. https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java:77: String algorithm = "PBEWITHSHAAND3-KEYTRIPLEDES-CBC"; Should this be a static constant in the class? https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java:84: KeyFactory factory = KeyFactory.getInstance("EC"); It's not clear what "EC" is. I recommend making this a named constant. https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java:92: Log.e(TAG, "Failed converting key " + ex); "ex" should be the third argument. https://codereview.chromium.org/1631123002/diff/20001/android_webview/native/... File android_webview/native/token_binding_manager_bridge.cc (right): https://codereview.chromium.org/1631123002/diff/20001/android_webview/native/... android_webview/native/token_binding_manager_bridge.cc:45: env, reinterpret_cast<const uint8_t*>(private_key.data()), Can we use const_cast here? I guess you are just enforcing const-ness?
https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java (right): https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java:45: private static final String PASSWORD = ""; not used? https://codereview.chromium.org/1631123002/diff/20001/android_webview/native/... File android_webview/native/token_binding_manager_bridge.cc (right): https://codereview.chromium.org/1631123002/diff/20001/android_webview/native/... android_webview/native/token_binding_manager_bridge.cc:45: env, reinterpret_cast<const uint8_t*>(private_key.data()), On 2016/01/26 01:37:02, mnaganov wrote: > Can we use const_cast here? I guess you are just enforcing const-ness? Or why cast at all? There is a const overload of data() that should be used by the compiler? (Plus using Foo* as const Foo* should be fine too) https://codereview.chromium.org/1631123002/diff/20001/android_webview/native/... android_webview/native/token_binding_manager_bridge.cc:51: env, reinterpret_cast<const uint8_t*>(public_key.data()), ditto
https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java (right): https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java:45: private static final String PASSWORD = ""; Err, never mind, line wrap at 80 :( shame
thanks for the valuable reviews, all done, ptal https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java (right): https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java:75: if (privateKeyBytes != null && publicKeyBytes != null) { On 2016/01/26 01:37:02, mnaganov wrote: > I think it makes sense to make an early return here in order to reduce > indentation. Done. https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java:77: String algorithm = "PBEWITHSHAAND3-KEYTRIPLEDES-CBC"; On 2016/01/26 01:37:02, mnaganov wrote: > Should this be a static constant in the class? Done. https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java:84: KeyFactory factory = KeyFactory.getInstance("EC"); On 2016/01/26 01:37:02, mnaganov wrote: > It's not clear what "EC" is. I recommend making this a named constant. Done. https://codereview.chromium.org/1631123002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwTokenBindingManager.java:92: Log.e(TAG, "Failed converting key " + ex); On 2016/01/26 01:37:02, mnaganov wrote: > "ex" should be the third argument. Done. https://codereview.chromium.org/1631123002/diff/20001/android_webview/native/... File android_webview/native/token_binding_manager_bridge.cc (right): https://codereview.chromium.org/1631123002/diff/20001/android_webview/native/... android_webview/native/token_binding_manager_bridge.cc:45: env, reinterpret_cast<const uint8_t*>(private_key.data()), On 2016/01/26 01:50:19, boliu wrote: > On 2016/01/26 01:37:02, mnaganov wrote: > > Can we use const_cast here? I guess you are just enforcing const-ness? > > Or why cast at all? > > There is a const overload of data() that should be used by the compiler? > > (Plus using Foo* as const Foo* should be fine too) yeah agreed. no need. https://codereview.chromium.org/1631123002/diff/20001/android_webview/native/... android_webview/native/token_binding_manager_bridge.cc:51: env, reinterpret_cast<const uint8_t*>(public_key.data()), On 2016/01/26 01:50:19, boliu wrote: > ditto Done.
lgtm
The CQ bit was checked by sgurun@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kpaulhamus@chromium.org Link to the patchset: https://codereview.chromium.org/1631123002/#ps60001 (title: "named const")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1631123002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1631123002/60001
Message was sent while issue was closed.
Description was changed from ========== The key conversion algorithm for Token binding BUG=576874 Convert the keys from native format to Java. ========== to ========== The key conversion algorithm for Token binding BUG=576874 Convert the keys from native format to Java. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== The key conversion algorithm for Token binding BUG=576874 Convert the keys from native format to Java. ========== to ========== The key conversion algorithm for Token binding BUG=576874 Convert the keys from native format to Java. Committed: https://crrev.com/0905e04fc78ee84151ef48171d275ae61adc040f Cr-Commit-Position: refs/heads/master@{#371430} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0905e04fc78ee84151ef48171d275ae61adc040f Cr-Commit-Position: refs/heads/master@{#371430} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
