Issue 687063002: Refactor: Expose JwkReader/JwkWriter helper classes.

Issue 687063002: Refactor: Expose JwkReader/JwkWriter helper classes. (Closed)

Created:
6 years, 1 month ago by eroman

Modified:
6 years, 1 month ago

Reviewers:
Ryan Sleevi

CC:
chromium-reviews, darin-cc_chromium.org, jam

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Refactor: Expose JwkReader/JwkWriter helper classes. This tidies the code up a bit, and I use these interfaces in a subsequent changelist (for EC keys). The vision is that jwk.h/jwk.cc should just define these two helpers, and all the other algorithm-specific logic (EC keys, RSA keys, AES keys, HMAC keys etc..) is built using JwkReader/JwkWriter, and lives in the algorithm-specific files. BUG=399094 Committed: https://crrev.com/4ccb5e4ec3cf49ab49d47a2cd59fe5165cf42f79 Cr-Commit-Position: refs/heads/master@{#302697}

Patch Set 1 : #

Total comments: 7

Patch Set 2 : Expose JwkReader::GetAlg() #

Total comments: 19

Patch Set 3 : Address feedback from sleevi: property/key --> member #

Patch Set 4 : utf-8 --> UTF-8 #

Total comments: 4

Patch Set 5 : update moar comments #

Created: 6 years, 1 month ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+323 lines, -241 lines)			Patch
	M	content/child/webcrypto/jwk.h	View	1 2 3 4	5 chunks	+112 lines, -7 lines	0 comments	Download
	M	content/child/webcrypto/jwk.cc	View	1 2	11 chunks	+211 lines, -234 lines	0 comments	Download

Messages

Total messages: 12 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

eroman

https://codereview.chromium.org/687063002/diff/20001/content/child/webcrypto/jwk.cc File content/child/webcrypto/jwk.cc (left): https://codereview.chromium.org/687063002/diff/20001/content/child/webcrypto/jwk.cc#oldcode223 content/child/webcrypto/jwk.cc:223: class JwkWriter { This class declaration was moved into ...

6 years, 1 month ago (2014-10-29 02:35:11 UTC) #3

Ryan Sleevi

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/jwk.h File content/child/webcrypto/jwk.h (right): https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/jwk.h#newcode23 content/child/webcrypto/jwk.h:23: // Helper class for parsing a JWK from JSON. ...

6 years, 1 month ago (2014-10-30 02:30:40 UTC) #4

eroman

6 years, 1 month ago (2014-10-30 16:04:35 UTC) #5

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/jwk.h (right):

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/jwk.h:23: // Helper class for parsing a JWK from JSON.
On 2014/10/30 02:30:40, Ryan Sleevi wrote:
> Explain why this exists.
> 
> // Helper class for parsing a JWK from JSON.
> //
> // This primarily exists to ensure strict enforcement of the JWK schema,
> // as the type and presence of particular fields is security relevant.
> // For example, GetString() will ensure a given JSON field is present
> // and is a string type, and will fail if these conditions aren't met.

Thanks for all the comment reviews BTW!

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/jwk.h:84: Status GetAlg(std::string* alg, bool* has_alg)
const;
On 2014/10/30 02:30:40, Ryan Sleevi wrote:
> Why is this a custom method instead of GetOptionalString("alg", ...)

I thought it was advantageous to internalize the "alg" string. It isn't likely
to ever need to be changed since JWK is pretty much frozen, but it felt like a
better encapsulation since none of the other core properties (kty, use, key_ops,
ext) needed to be exposed.

If you disagree I can certainly delete this member without much consequence.

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/jwk.h:87: Status VerifyAlg(const std::string&
expected_algorithm) const;
On 2014/10/30 02:30:40, Ryan Sleevi wrote:
> Why isn't this part of Init?

The reason for this aberration is AES keys.

In the case of AES keys, the expected name of the algorithm (for instance
"A128GCM") is tied to the length of the key data, and is not known until parsing
the JWK. By not having the check be mandatory in Init() allows using the
JwkReader() to test all the other stuff first and determine the key length.

How about I make these changes:

  * I add |expected_alg| parameter to Init(), since in all other cases that is
cleaner.
  * Allow |expected_alg.empty()| to have a special meaning -- don't test "alg"

eroman

6 years, 1 month ago (2014-10-30 17:00:30 UTC) #6

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/jwk.h (right):

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/jwk.h:23: // Helper class for parsing a JWK from JSON.
On 2014/10/30 02:30:40, Ryan Sleevi wrote:
> Explain why this exists.
> 
> // Helper class for parsing a JWK from JSON.
> //
> // This primarily exists to ensure strict enforcement of the JWK schema,
> // as the type and presence of particular fields is security relevant.
> // For example, GetString() will ensure a given JSON field is present
> // and is a string type, and will fail if these conditions aren't met.

Done.

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/jwk.h:33: // must have kty of xpected_kty, have an ext
compatible with
On 2014/10/30 02:30:39, Ryan Sleevi wrote:
> typo: expected_kty

Done.

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/jwk.h:34: // |expected_extractable| and have usages
compatible with expected_usages.
On 2014/10/30 02:30:40, Ryan Sleevi wrote:
> "|expected_extractable|, and"
> 
> Oxford commas rule!

Done (changed to bullets)

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/jwk.h:41: bool HasKey(const std::string& key) const;
On 2014/10/30 02:30:39, Ryan Sleevi wrote:
> s/Key/Member/ throughout
> 
> That is, the name of properties of objects in JSON is "member"
> 
> (and std::string key -> member_name)

Done.

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/jwk.h:59: // |*property_exists| if it was found. Note
that |*result| is owned by |dict|.
On 2014/10/30 02:30:39, Ryan Sleevi wrote:
> |dict_|

Done.

https://codereview.chromium.org/687063002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/jwk.h:87: Status VerifyAlg(const std::string&
expected_algorithm) const;
On 2014/10/30 16:04:35, eroman wrote:
> On 2014/10/30 02:30:40, Ryan Sleevi wrote:
> > Why isn't this part of Init?
> 
> The reason for this aberration is AES keys.
> 
> In the case of AES keys, the expected name of the algorithm (for instance
> "A128GCM") is tied to the length of the key data, and is not known until
parsing
> the JWK. By not having the check be mandatory in Init() allows using the
> JwkReader() to test all the other stuff first and determine the key length.
> 
> How about I make these changes:
> 
>   * I add |expected_alg| parameter to Init(), since in all other cases that is
> cleaner.
>   * Allow |expected_alg.empty()| to have a special meaning -- don't test "alg"

Done, made an optional part of Init()

Ryan Sleevi

lgtm https://codereview.chromium.org/687063002/diff/80001/content/child/webcrypto/jwk.h File content/child/webcrypto/jwk.h (right): https://codereview.chromium.org/687063002/diff/80001/content/child/webcrypto/jwk.h#newcode26 content/child/webcrypto/jwk.h:26: // type and presence of particular fields is ...

6 years, 1 month ago (2014-11-04 20:59:50 UTC) #7

eroman

https://codereview.chromium.org/687063002/diff/80001/content/child/webcrypto/jwk.h File content/child/webcrypto/jwk.h (right): https://codereview.chromium.org/687063002/diff/80001/content/child/webcrypto/jwk.h#newcode26 content/child/webcrypto/jwk.h:26: // type and presence of particular fields is security ...

6 years, 1 month ago (2014-11-04 21:50:45 UTC) #8

commit-bot: I haz the power

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

6 years, 1 month ago (2014-11-04 21:52:31 UTC) #10

commit-bot: I haz the power

6 years, 1 month ago (2014-11-04 23:06:14 UTC) #12

Message was sent while issue was closed.

Patchset 5 (id:??) landed as
https://crrev.com/4ccb5e4ec3cf49ab49d47a2cd59fe5165cf42f79
Cr-Commit-Position: refs/heads/master@{#302697}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages