Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2047763002:
Switch AwTokenBindingManager's internals to PKCS#8. (Closed)
DescriptionSwitch AwTokenBindingManager's internals to PKCS#8.
There's no need to do the silly encrypt with empty password and then decrypt in
Java dance. Just serialize to a PKCS#8 blob and parse it back out normally.
BUG=603319
Committed: https://crrev.com/526946fc5c0cb41ef269d5cbdcec5ca81a957c93
Cr-Commit-Position: refs/heads/master@{#401083}
Patch Set 1 #Patch Set 2 : fix build #
Depends on Patchset: Messages
Total messages: 20 (6 generated)
davidben@chromium.org changed reviewers: + sgurun@chromium.org
Note: I have not tested this. It appears https://codereview.chromium.org/1631123002 landed without any unit tests, so you should probably manually test this (or tell me how to) before stamping.
On 2016/06/07 18:24:30, davidben wrote: > Note: I have not tested this. It appears > https://codereview.chromium.org/1631123002 landed without any unit tests, so you > should probably manually test this (or tell me how to) before stamping. will test tomorrow,
Description was changed from ========== Switch AwTokenBindingManager's internals to PKCS#8. There's no need to do the silly encrypt with empty password and then decrypt in Java dance. Just serialize to a PKCS#8 blob and parse it back out normally. BUG=603319 ========== to ========== Switch AwTokenBindingManager's internals to PKCS#8. There's no need to do the silly encrypt with empty password and then decrypt in Java dance. Just serialize to a PKCS#8 blob and parse it back out normally. BUG=603319 ==========
sgurun@chromium.org changed reviewers: + kpaulhamus@chromium.org
On 2016/06/08 02:17:10, sgurun wrote: > On 2016/06/07 18:24:30, davidben wrote: > > Note: I have not tested this. It appears > > https://codereview.chromium.org/1631123002 landed without any unit tests, so > you > > should probably manually test this (or tell me how to) before stamping. > > will test tomorrow, kim, FYI
(It seems I'm dumb and this thing doesn't even compile. Will fix it tomorrow and ping the thread. Sorry about that! I think I just need to put TAG back.)
On 2016/06/08 02:30:23, davidben wrote: > (It seems I'm dumb and this thing doesn't even compile. Will fix it tomorrow and > ping the thread. Sorry about that! I think I just need to put TAG back.) Nice, glad you're addressing this. But please do test so we don't break any existing uses :)
On 2016/06/08 18:43:37, kpaulhamus wrote: > On 2016/06/08 02:30:23, davidben wrote: > > (It seems I'm dumb and this thing doesn't even compile. Will fix it tomorrow > and > > ping the thread. Sorry about that! I think I just need to put TAG back.) > > Nice, glad you're addressing this. But please do test so we don't break any > existing uses :) How would I go about testing this? I don't actually do anything with WebView. (Was hoping there would be either unit tests or one of you would know how to test it. :-P ) This doesn't change the API, just internals about how C++ and Java communicate the key, so it should be fairly straight-forward to test. If even one test case works, then I'm unlikely to have broken anything. :-) (The encrypted PKCS#8 code ends up calling into the PKCS#8 code after decrypting anyway.)
On 2016/06/08 18:47:46, davidben wrote: > On 2016/06/08 18:43:37, kpaulhamus wrote: > > On 2016/06/08 02:30:23, davidben wrote: > > > (It seems I'm dumb and this thing doesn't even compile. Will fix it tomorrow > > and > > > ping the thread. Sorry about that! I think I just need to put TAG back.) > > > > Nice, glad you're addressing this. But please do test so we don't break any > > existing uses :) > > How would I go about testing this? I don't actually do anything with WebView. > (Was hoping there would be either unit tests or one of you would know how to > test it. :-P ) > > This doesn't change the API, just internals about how C++ and Java communicate > the key, so it should be fairly straight-forward to test. If even one test case > works, then I'm unlikely to have broken anything. :-) (The encrypted PKCS#8 code > ends up calling into the PKCS#8 code after decrypting anyway.) Selim, do you recall how you tested this when you worked on it?
On 2016/06/08 18:53:37, kpaulhamus wrote: > On 2016/06/08 18:47:46, davidben wrote: > > On 2016/06/08 18:43:37, kpaulhamus wrote: > > > On 2016/06/08 02:30:23, davidben wrote: > > > > (It seems I'm dumb and this thing doesn't even compile. Will fix it > tomorrow > > > and > > > > ping the thread. Sorry about that! I think I just need to put TAG back.) > > > > > > Nice, glad you're addressing this. But please do test so we don't break any > > > existing uses :) > > > > How would I go about testing this? I don't actually do anything with WebView. > > (Was hoping there would be either unit tests or one of you would know how to > > test it. :-P ) > > > > This doesn't change the API, just internals about how C++ and Java communicate > > the key, so it should be fairly straight-forward to test. If even one test > case > > works, then I'm unlikely to have broken anything. :-) (The encrypted PKCS#8 > code > > ends up calling into the PKCS#8 code after decrypting anyway.) > > Selim, do you recall how you tested this when you worked on it? Yes, I believe I wrote an apk to use the apis and then compared them to a known value that you supplied. if you know how to add a test, feel free to do it :) I will test this manually (will ping the the thread when done, today/tomorrow- don't think it is urgent.)
On 2016/06/08 22:01:03, sgurun wrote: > On 2016/06/08 18:53:37, kpaulhamus wrote: > > On 2016/06/08 18:47:46, davidben wrote: > > > On 2016/06/08 18:43:37, kpaulhamus wrote: > > > > On 2016/06/08 02:30:23, davidben wrote: > > > > > (It seems I'm dumb and this thing doesn't even compile. Will fix it > > tomorrow > > > > and > > > > > ping the thread. Sorry about that! I think I just need to put TAG back.) > > > > > > > > Nice, glad you're addressing this. But please do test so we don't break > any > > > > existing uses :) > > > > > > How would I go about testing this? I don't actually do anything with > WebView. > > > (Was hoping there would be either unit tests or one of you would know how to > > > test it. :-P ) > > > > > > This doesn't change the API, just internals about how C++ and Java > communicate > > > the key, so it should be fairly straight-forward to test. If even one test > > case > > > works, then I'm unlikely to have broken anything. :-) (The encrypted PKCS#8 > > code > > > ends up calling into the PKCS#8 code after decrypting anyway.) > > > > Selim, do you recall how you tested this when you worked on it? > > Yes, I believe I wrote an apk to use the apis and then compared them to a known > value that you supplied. if you know how to add a test, feel free to do it :) I > will test this manually (will ping the the thread when done, today/tomorrow- > don't think it is urgent.) Sounds good. Yeah, it's no rush. I suspect I know too little about how WebView works to be able to usefully put together a test. :-)
On 2016/06/08 22:06:44, davidben wrote: > On 2016/06/08 22:01:03, sgurun wrote: > > On 2016/06/08 18:53:37, kpaulhamus wrote: > > > On 2016/06/08 18:47:46, davidben wrote: > > > > On 2016/06/08 18:43:37, kpaulhamus wrote: > > > > > On 2016/06/08 02:30:23, davidben wrote: > > > > > > (It seems I'm dumb and this thing doesn't even compile. Will fix it > > > tomorrow > > > > > and > > > > > > ping the thread. Sorry about that! I think I just need to put TAG > back.) > > > > > > > > > > Nice, glad you're addressing this. But please do test so we don't break > > any > > > > > existing uses :) > > > > > > > > How would I go about testing this? I don't actually do anything with > > WebView. > > > > (Was hoping there would be either unit tests or one of you would know how > to > > > > test it. :-P ) > > > > > > > > This doesn't change the API, just internals about how C++ and Java > > communicate > > > > the key, so it should be fairly straight-forward to test. If even one test > > > case > > > > works, then I'm unlikely to have broken anything. :-) (The encrypted > PKCS#8 > > > code > > > > ends up calling into the PKCS#8 code after decrypting anyway.) > > > > > > Selim, do you recall how you tested this when you worked on it? > > > > Yes, I believe I wrote an apk to use the apis and then compared them to a > known > > value that you supplied. if you know how to add a test, feel free to do it :) > I > > will test this manually (will ping the the thread when done, today/tomorrow- > > don't think it is urgent.) > > Sounds good. Yeah, it's no rush. I suspect I know too little about how WebView > works to be able to usefully put together a test. :-) LGTM
On 2016/06/21 18:26:23, sgurun wrote: > On 2016/06/08 22:06:44, davidben wrote: > > On 2016/06/08 22:01:03, sgurun wrote: > > > On 2016/06/08 18:53:37, kpaulhamus wrote: > > > > On 2016/06/08 18:47:46, davidben wrote: > > > > > On 2016/06/08 18:43:37, kpaulhamus wrote: > > > > > > On 2016/06/08 02:30:23, davidben wrote: > > > > > > > (It seems I'm dumb and this thing doesn't even compile. Will fix it > > > > tomorrow > > > > > > and > > > > > > > ping the thread. Sorry about that! I think I just need to put TAG > > back.) > > > > > > > > > > > > Nice, glad you're addressing this. But please do test so we don't > break > > > any > > > > > > existing uses :) > > > > > > > > > > How would I go about testing this? I don't actually do anything with > > > WebView. > > > > > (Was hoping there would be either unit tests or one of you would know > how > > to > > > > > test it. :-P ) > > > > > > > > > > This doesn't change the API, just internals about how C++ and Java > > > communicate > > > > > the key, so it should be fairly straight-forward to test. If even one > test > > > > case > > > > > works, then I'm unlikely to have broken anything. :-) (The encrypted > > PKCS#8 > > > > code > > > > > ends up calling into the PKCS#8 code after decrypting anyway.) > > > > > > > > Selim, do you recall how you tested this when you worked on it? > > > > > > Yes, I believe I wrote an apk to use the apis and then compared them to a > > known > > > value that you supplied. if you know how to add a test, feel free to do it > :) > > I > > > will test this manually (will ping the the thread when done, today/tomorrow- > > > > don't think it is urgent.) > > > > Sounds good. Yeah, it's no rush. I suspect I know too little about how WebView > > works to be able to usefully put together a test. :-) > > LGTM Thanks!
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047763002/20001
Message was sent while issue was closed.
Description was changed from ========== Switch AwTokenBindingManager's internals to PKCS#8. There's no need to do the silly encrypt with empty password and then decrypt in Java dance. Just serialize to a PKCS#8 blob and parse it back out normally. BUG=603319 ========== to ========== Switch AwTokenBindingManager's internals to PKCS#8. There's no need to do the silly encrypt with empty password and then decrypt in Java dance. Just serialize to a PKCS#8 blob and parse it back out normally. BUG=603319 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Switch AwTokenBindingManager's internals to PKCS#8. There's no need to do the silly encrypt with empty password and then decrypt in Java dance. Just serialize to a PKCS#8 blob and parse it back out normally. BUG=603319 ========== to ========== Switch AwTokenBindingManager's internals to PKCS#8. There's no need to do the silly encrypt with empty password and then decrypt in Java dance. Just serialize to a PKCS#8 blob and parse it back out normally. BUG=603319 Committed: https://crrev.com/526946fc5c0cb41ef269d5cbdcec5ca81a957c93 Cr-Commit-Position: refs/heads/master@{#401083} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/526946fc5c0cb41ef269d5cbdcec5ca81a957c93 Cr-Commit-Position: refs/heads/master@{#401083} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
