Encode JSON key IDs and keys as base64url

Encode JSON key IDs and keys as base64url

Key IDs and keys used in JSON data should be encoded base64url,
not just base64 with trailing = removed.

BUG=459834
TEST=new tests pass

Committed: https://crrev.com/ac02ac60a90b8dec389e45d87bb0b022b8cca051
Cr-Commit-Position: refs/heads/master@{#317097}

[jrummell, 2015-02-19 03:59:34]

jrummell@chromium.org changed reviewers:
+ ddorwin@chromium.org, sandersd@chromium.org

[jrummell, 2015-02-19 03:59:34]

PTAL.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc
File media/cdm/json_web_key.cc (right):

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:72: //}
Will enable this once layout tests are fixed.

[ddorwin, 2015-02-19 04:45:56]

LGTM % comments. Thanks!

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc
File media/cdm/json_web_key.cc (right):

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:27: const char kBase64Plus[] = "+";
Do these need to be strings instead of a char? I guess ReplaceChars requires
this.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:28: const char kBase64UrlMinus[] = "-";
s/Minus/PlusReplacement/

We can read the string to see "Minus", but the proposed name makes it more
obvious what it is.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:48: // base64url encoding also means the characters
'-' and '_' must be used
nit: drop "also "

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:65: //if (encoded_text.find(kBase64Plus) !=
std::string::npos) {
You can use find_first_of() to search for both at once.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:81: // base64url encoding also means the characters
'-' and '_' must be used
ditto

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key_unitt...
File media/cdm/json_web_key_unittest.cc (right):

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key_unitt...
media/cdm/json_web_key_unittest.cc:475: TEST_F(JSONWebKeyTest,
Base64UrlEncoding) {
Ideally, we'd have both special characters. We can do that later once I get some
examples for the spec.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key_unitt...
media/cdm/json_web_key_unittest.cc:485: EXPECT_NE(encoded_text.find('+'),
std::string::npos);
Does the encode give us padding? If so, we can check that too (to confirm we do
stripping too).

[jrummell, 2015-02-19 18:18:31]

New patchsets have been uploaded after l-g-t-m from ddorwin@chromium.org

[jrummell, 2015-02-19 18:20:07]

Thanks for the review.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc
File media/cdm/json_web_key.cc (right):

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:27: const char kBase64Plus[] = "+";
On 2015/02/19 04:45:56, ddorwin wrote:
> Do these need to be strings instead of a char? I guess ReplaceChars requires
> this.

Correct. ReplaceChars needs it.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:28: const char kBase64UrlMinus[] = "-";
On 2015/02/19 04:45:56, ddorwin wrote:
> s/Minus/PlusReplacement/
> 
> We can read the string to see "Minus", but the proposed name makes it more
> obvious what it is.

Done.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:48: // base64url encoding also means the characters
'-' and '_' must be used
On 2015/02/19 04:45:56, ddorwin wrote:
> nit: drop "also "

Done.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:65: //if (encoded_text.find(kBase64Plus) !=
std::string::npos) {
On 2015/02/19 04:45:56, ddorwin wrote:
> You can use find_first_of() to search for both at once.

Acknowledged.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key.cc#ne...
media/cdm/json_web_key.cc:81: // base64url encoding also means the characters
'-' and '_' must be used
On 2015/02/19 04:45:56, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key_unitt...
File media/cdm/json_web_key_unittest.cc (right):

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key_unitt...
media/cdm/json_web_key_unittest.cc:475: TEST_F(JSONWebKeyTest,
Base64UrlEncoding) {
On 2015/02/19 04:45:56, ddorwin wrote:
> Ideally, we'd have both special characters. We can do that later once I get
some
> examples for the spec.

Found one.

https://codereview.chromium.org/934423002/diff/1/media/cdm/json_web_key_unitt...
media/cdm/json_web_key_unittest.cc:485: EXPECT_NE(encoded_text.find('+'),
std::string::npos);
On 2015/02/19 04:45:56, ddorwin wrote:
> Does the encode give us padding? If so, we can check that too (to confirm we
do
> stripping too).

Added extra case.

[jrummell, 2015-02-19 18:20:13]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-02-19 18:21:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/934423002/20001

[ddorwin, 2015-02-19 18:25:08]

lgtm

[commit-bot: I haz the power, 2015-02-19 19:24:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-19 19:24:53]

Patchset 2 (id:??) landed as
https://crrev.com/ac02ac60a90b8dec389e45d87bb0b022b8cca051
Cr-Commit-Position: refs/heads/master@{#317097}