Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1737693002:
Allow command-line arguments to override EF public key (Closed)
DescriptionAllow command-line arguments to override EF public key
This adds a command-line flag, --origin-trial-public-key, which can be used by
developers to override the public key used to verify the signed tokens for
origin trials.
BUG=603588
Committed: https://crrev.com/ee4f4a742db6b3e5c8bb8462f9da270c26be491c
Cr-Commit-Position: refs/heads/master@{#389832}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressing comments from PS#1 #
Total comments: 19
Patch Set 3 : Addressing comments from PS#2 #Patch Set 4 : Fix use-after-free due to StringPiece shared state #
Total comments: 15
Patch Set 5 : Addressing feedback from PS#4 #
Total comments: 2
Patch Set 6 : Remove finch integration from this CL #Patch Set 7 : Rebase #
Total comments: 8
Patch Set 8 : Remove new method from content API #Messages
Total messages: 62 (19 generated)
iclelland@chromium.org changed reviewers: + asvitkine@google.com, chasej@chromium.org
+r chasej -- PTAL at all of it, thanks :) +r asvitkine -- can you PTAL at the Finch usage? I'd like to know if what I'm proposing here is appropriate use of that framework, before I extend it to cover the other cases in the referenced bug.
Looking good, with minor comments. https://codereview.chromium.org/1737693002/diff/1/chrome/browser/origin_trial... File chrome/browser/origin_trials/origin_trial_controller.h (right): https://codereview.chromium.org/1737693002/diff/1/chrome/browser/origin_trial... chrome/browser/origin_trials/origin_trial_controller.h:8: #include "base/command_line.h" Can this be a forward declaration instead? https://codereview.chromium.org/1737693002/diff/1/chrome/renderer/origin_tria... File chrome/renderer/origin_trials/origin_trial_key_manager_unittest.cc (right): https://codereview.chromium.org/1737693002/diff/1/chrome/renderer/origin_tria... chrome/renderer/origin_trials/origin_trial_key_manager_unittest.cc:64: // EXPECT_EQ(...) Was this comment supposed to be another check? Should be cleaned up.
https://codereview.chromium.org/1737693002/diff/1/chrome/browser/origin_trial... File chrome/browser/origin_trials/origin_trial_controller.h (right): https://codereview.chromium.org/1737693002/diff/1/chrome/browser/origin_trial... chrome/browser/origin_trials/origin_trial_controller.h:8: #include "base/command_line.h" On 2016/02/26 06:10:34, chasej wrote: > Can this be a forward declaration instead? Done. https://codereview.chromium.org/1737693002/diff/1/chrome/renderer/origin_tria... File chrome/renderer/origin_trials/origin_trial_key_manager_unittest.cc (right): https://codereview.chromium.org/1737693002/diff/1/chrome/renderer/origin_tria... chrome/renderer/origin_trials/origin_trial_key_manager_unittest.cc:64: // EXPECT_EQ(...) On 2016/02/26 06:10:34, chasej wrote: > Was this comment supposed to be another check? Should be cleaned up. Was supposed to be the line above it. Fixed, thanks.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
General override approach seems fine to me - some comments about the implementation. https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... File chrome/browser/origin_trials/origin_trial_controller.cc (right): https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.cc:19: DCHECK(command_line); Nit: Not needed. https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.cc:22: if (!variations::GetVariationParams(kFieldTrialName, &field_params)) { Nit: No {}'s https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.cc:27: if (override_public_key.size()) { Nit: !.empty() https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.cc:29: override_public_key); I'm not a big fan of plumbing this through the command-line. Also, seems a bit heavy to have its own class & file for this logic. Why not just add this as a function to OriginTrialKeyManager() and have all the logic there? https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:320: if (command_line->HasSwitch(switches::kOriginTrialPublicKey)) Nit: {}'s https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.cc (right): https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.cc:30: if (!base::Base64Decode(ascii_public_key, &new_public_key)) { Nit: No {}'s https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.cc:33: if (new_public_key.size() != 32) { Nit: No {}'s https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:15: ~OriginTrialKeyManager(); Nit: Add empty line below. https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:21: }; Nit: DISALLOW_COPY_AND_ASSIGN()
https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... File chrome/browser/origin_trials/origin_trial_controller.cc (right): https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.cc:29: override_public_key); On 2016/02/26 16:29:08, Alexei Svitkine (slow) wrote: > I'm not a big fan of plumbing this through the command-line. I wrote the command-line handling code first, as a way to initialize the renderer with a new key. (The command line itself seems like the way to pass that information between processes on init) From there, it seemed natural to have the remote configuration update that as well. Is there a better mechanism for passing finch params to a renderer on startup? The docs claim that GetVariationParamValue is only accessible from the browser, or else I would just access the parameters on-demand from the renderer. > > Also, seems a bit heavy to have its own class & file for this logic. Why not > just add this as a function to OriginTrialKeyManager() and have all the logic > there? The biggest reason was that OriginTrialKeyManager is in chrome/renderer, and this logic exists in chrome/browser. If it's not going to be seen as a layering violation to move this code there, then I'm happy to have it all in one place.
https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... File chrome/browser/origin_trials/origin_trial_controller.cc (right): https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.cc:29: override_public_key); On 2016/02/26 16:49:14, iclelland wrote: > On 2016/02/26 16:29:08, Alexei Svitkine (slow) wrote: > > I'm not a big fan of plumbing this through the command-line. > > I wrote the command-line handling code first, as a way to initialize the > renderer with a new key. (The command line itself seems like the way to pass > that information between processes on init) From there, it seemed natural to > have the remote configuration update that as well. Is there a better mechanism > for passing finch params to a renderer on startup? The docs claim that > GetVariationParamValue is only accessible from the browser, or else I would just > access the parameters on-demand from the renderer. > > > > > Also, seems a bit heavy to have its own class & file for this logic. Why not > > just add this as a function to OriginTrialKeyManager() and have all the logic > > there? > > The biggest reason was that OriginTrialKeyManager is in chrome/renderer, and > this logic exists in chrome/browser. If it's not going to be seen as a layering > violation to move this code there, then I'm happy to have it all in one place. Ah, you're right - I missed that you're checking it from the renderer. That makes sense then.
https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... File chrome/browser/origin_trials/origin_trial_controller.cc (right): https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.cc:19: DCHECK(command_line); On 2016/02/26 16:29:08, Alexei Svitkine (slow) wrote: > Nit: Not needed. Removed. https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.cc:22: if (!variations::GetVariationParams(kFieldTrialName, &field_params)) { On 2016/02/26 16:29:08, Alexei Svitkine (slow) wrote: > Nit: No {}'s Thanks; these were all leftovers from my in-development logging :) All removed. https://codereview.chromium.org/1737693002/diff/20001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.cc:27: if (override_public_key.size()) { On 2016/02/26 16:29:08, Alexei Svitkine (slow) wrote: > Nit: !.empty() Done. https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:320: if (command_line->HasSwitch(switches::kOriginTrialPublicKey)) On 2016/02/26 16:29:08, Alexei Svitkine (slow) wrote: > Nit: {}'s Done. https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.cc (right): https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.cc:30: if (!base::Base64Decode(ascii_public_key, &new_public_key)) { On 2016/02/26 16:29:08, Alexei Svitkine (slow) wrote: > Nit: No {}'s Done. https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.cc:33: if (new_public_key.size() != 32) { On 2016/02/26 16:29:08, Alexei Svitkine (slow) wrote: > Nit: No {}'s Done. https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:15: ~OriginTrialKeyManager(); On 2016/02/26 16:29:08, Alexei Svitkine (slow) wrote: > Nit: Add empty line below. Done. https://codereview.chromium.org/1737693002/diff/20001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:21: }; On 2016/02/26 16:29:08, Alexei Svitkine (slow) wrote: > Nit: DISALLOW_COPY_AND_ASSIGN() Done. Thanks.
lgtm
lgtm
Description was changed from ========== Allow field trials to override EF public key This adds a command-line flag, --origin-trial-public-key, which can be used to override the public key used to verify the signed tokens for origin trials. It also allows for Finch field trials to override the key, so that it can be updated outside of the browser release schedule if necessary. BUG=589830 ========== to ========== Allow field trials to override EF public key This adds a command-line flag, --origin-trial-public-key, which can be used to override the public key used to verify the signed tokens for origin trials. It also allows for Finch field trials to override the key, so that it can be updated outside of the browser release schedule if necessary. BUG=589830 ==========
chasej@chromium.org changed reviewers: - asvitkine@google.com
+r jam -- Can you PTAL? Thanks
iclelland@chromium.org changed reviewers: + jam@chromium.org
(Somehow the +r didn't actually take effect last time. Trying again)
On 2016/02/29 15:44:35, iclelland wrote: > +r jam -- Can you PTAL? Thanks please pick an owner from chrome/OWNERS (i.e. should pick someone from there before someone from src/owners)
iclelland@chromium.org changed reviewers: + sky@chromium.org - jam@chromium.org
On 2016/03/03 16:40:36, jam wrote: > On 2016/02/29 15:44:35, iclelland wrote: > > +r jam -- Can you PTAL? Thanks > > please pick an owner from chrome/OWNERS (i.e. should pick someone from there > before someone from src/owners) (Sorry about that; I didn't see the distinction in git cl owners, and I knew you had previously reviewed some of these files. Removed.) -r jam +r sky -- Do you mind taking a look?
This seems like there could be security implications here. Has this gone through a security review?
https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... File chrome/browser/origin_trials/origin_trial_controller.h (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. style guide says no (c) anymore. https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.h:12: class OriginTrialController { There is no point in using a class for this. If you really want the class, then use DISALLOW_IMPLICIT_CONSTRUCTORS. https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... File chrome/browser/origin_trials/origin_trial_controller_unittest.cc (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller_unittest.cc:47: scoped_ptr<base::CommandLine> command_line_; Why bother with scoped_ptr for either of these? https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller_unittest.cc:49: }; DISALLOW... https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:19: base::StringPiece GetPublicKey() const; Is there a reason you don't return a const std::string& here?
On 2016/03/03 20:48:37, sky wrote: > This seems like there could be security implications here. Has this gone through > a security review? Not specifically, but I will get someone to do that before landing. Thanks.
https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... File chrome/browser/origin_trials/origin_trial_controller.h (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/03/03 20:55:30, sky wrote: > style guide says no (c) anymore. Thanks, I missed that. https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller.h:12: class OriginTrialController { On 2016/03/03 20:55:30, sky wrote: > There is no point in using a class for this. If you really want the class, then > use DISALLOW_IMPLICIT_CONSTRUCTORS. The class is really just there for namespacing, not to hold any state. Done. https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... File chrome/browser/origin_trials/origin_trial_controller_unittest.cc (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller_unittest.cc:47: scoped_ptr<base::CommandLine> command_line_; On 2016/03/03 20:55:30, sky wrote: > Why bother with scoped_ptr for either of these? There's no compelling reason; I can simplify it by just maintaining a base::CommandLine/FieldTrialList value here. Changed. https://codereview.chromium.org/1737693002/diff/60001/chrome/browser/origin_t... chrome/browser/origin_trials/origin_trial_controller_unittest.cc:49: }; On 2016/03/03 20:55:30, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:19: base::StringPiece GetPublicKey() const; On 2016/03/03 20:55:30, sky wrote: > Is there a reason you don't return a const std::string& here? I was originally using uint8_t[] for key material everywhere, but it was suggested (https://codereview.chromium.org/1522813002/) to use StringPiece instead to pass them around, for safety. I suspect that std::string would be just as suitable as a container type, and possibly easier to work with. I'm not partial to StringPiece in any way.
palmer@chromium.org changed reviewers: + palmer@chromium.org
https://codereview.chromium.org/1737693002/diff/80001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.cc (right): https://codereview.chromium.org/1737693002/diff/80001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.cc:28: // Base64-decode the incoming string. Set the key if it is correctly formatted It's taking all my willpower to not say "Nit: Punctuate the 2nd sentence." ;D https://codereview.chromium.org/1737693002/diff/80001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.cc:32: if (new_public_key.size() != 32) This doesn't allow for larger keys, if we ever wanted them. I'm not saying this is a good idea, necessarily, but perhaps something like if (new_public_key.size() < sizeof(kDefaultPublicKey)) return false; ? A variety of other sanity checks could apply. For examples: * the high-order bit should be set in an RSA public key modulus (so that it's *really* a 2048-bit value, instead of just a bitstring with some meaningless 0s in the high-order positions) * it shouldn't have any known/public/small/non-prime factors I wonder if there is a is_rsa_pubkey_minimally_sane function somewhere that you can just call. (It's definitely a task for a crypto expert; not me, for example.) I think all we could really hope to achieve would be to catch Obviously Wrong keys; I don't think we can really know, after the fact, if a key was generated Properly.
palmer@chromium.org changed reviewers: + estark@chromium.org
+estark; I think she's a better crypto friend than I am, and EF may already be in her review queue. One question I have: Can't we use Component Updater as a better way to get new keys to Chrome? And, I'm not sure why we need key rotation on a faster schedule than 6-week Chrome updates.
estark tells me your using Ed25519, not RSA; same principle likely applies. There does exist a RSA_private_key_from_bytes in Boring/OpenSSL, which any RSA should probably at least pass before using it; I don't know if B/OSSL has a similar function for your key type. Anyway, just something to keep in mind.
I think the crypto parts of this look okay to me. AFAIK there's not much sanity checking to be done on an Ed25519 public key except that it's a point on the curve, which ED25519_verify() must do. However, I'm not clear on what this mechanism accomplishes (setting a public key via field trial). I skimmed the design doc but didn't see anything explaining it. Is it in case the private key is compromised? Is Finch reliable enough in that scenario? I also have some questions about other parts of the trial token code that I stumbled upon while reading this CL. (For example, why does the token parsing code live in content/common but appear to only be used from the renderer? Can it move into content/renderer? And did you consider using JSON instead of the custom pipe-separated notation that requires its own parsing code?) iclelland@, I don't want to derail this CL, so what would be the best way to discuss those questions?
On 2016/03/17 00:23:28, estark wrote: > I think the crypto parts of this look okay to me. AFAIK there's not much sanity > checking to be done on an Ed25519 public key except that it's a point on the > curve, which ED25519_verify() must do. Yes, I thing that's the first thing that it does (see x25519_ge_frombytes_vartime) > > However, I'm not clear on what this mechanism accomplishes (setting a public key > via field trial). I skimmed the design doc but didn't see anything explaining > it. Is it in case the private key is compromised? Is Finch reliable enough in > that scenario? > > I also have some questions about other parts of the trial token code that I > stumbled upon while reading this CL. (For example, why does the token parsing > code live in content/common but appear to only be used from the renderer? Can it > move into content/renderer? It did live there, until mek@ landed crrev.com/1742053002 -- we are intending on using this in the browser as well. > And did you consider using JSON instead of the > custom pipe-separated notation that requires its own parsing code?) The physical encoding of the token was adapted from palmer@'s comments on the original design doc (content now moved to https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M , but the comments are still in the comment thread of the design document) We wanted to have something reasonably compact (hence Ed25519 over RSA), that would survive developer-cut-and-paste, and easily fit within both a <meta> tag and an HTTP header, without becoming an escaping nightmare. I don't think we seriously considered using plaintext JSON for that reason. > iclelland@, > I don't want to derail this CL, so what would be the best way to discuss those > questions? I'm OOO until Tuesday now, but I'd be happy to set up a meeting to talk about all of it as soon as I'm back.
On 2016/03/17 03:06:37, iclelland wrote: > On 2016/03/17 00:23:28, estark wrote: > > I think the crypto parts of this look okay to me. AFAIK there's not much > sanity > > checking to be done on an Ed25519 public key except that it's a point on the > > curve, which ED25519_verify() must do. > > Yes, I thing that's the first thing that it does (see > x25519_ge_frombytes_vartime) > > > > > However, I'm not clear on what this mechanism accomplishes (setting a public > key > > via field trial). I skimmed the design doc but didn't see anything explaining > > it. Is it in case the private key is compromised? Is Finch reliable enough in > > that scenario? > > > > I also have some questions about other parts of the trial token code that I > > stumbled upon while reading this CL. (For example, why does the token parsing > > code live in content/common but appear to only be used from the renderer? Can > it > > move into content/renderer? > > It did live there, until mek@ landed crrev.com/1742053002 -- we are intending on > using this in the browser as well. Gotcha. But, I see the end result of that review was thakis@ asking if it's okay to have this in the browser process and mek@ agreeing to get a security review before actually using it, so let's talk about it when you get back. :) > > > And did you consider using JSON instead of the > > custom pipe-separated notation that requires its own parsing code?) > > The physical encoding of the token was adapted from palmer@'s comments on the > original design doc (content now moved to > https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M > , but the comments are still in the comment thread of the design document) > > We wanted to have something reasonably compact (hence Ed25519 over RSA), that > would survive developer-cut-and-paste, and easily fit within both a <meta> tag > and an HTTP header, without becoming an escaping nightmare. I don't think we > seriously considered using plaintext JSON for that reason. It sounds like base64-encoded stringified JSON would accomplish all those things without having to maintain any of your own parsing code. > > > iclelland@, > > I don't want to derail this CL, so what would be the best way to discuss those > > questions? > > I'm OOO until Tuesday now, but I'd be happy to set up a meeting to talk about > all of it as soon as I'm back. Ok, sounds good, drop something on my calendar whenever you get back. (And probably palmer too if he wants.) Thanks for the explanations!
On 2016/03/18 02:59:01, estark wrote: > On 2016/03/17 03:06:37, iclelland wrote: > > On 2016/03/17 00:23:28, estark wrote: > > > I think the crypto parts of this look okay to me. AFAIK there's not much > > sanity > > > checking to be done on an Ed25519 public key except that it's a point on the > > > curve, which ED25519_verify() must do. > > > > Yes, I thing that's the first thing that it does (see > > x25519_ge_frombytes_vartime) > > > > > > > > However, I'm not clear on what this mechanism accomplishes (setting a public > > key > > > via field trial). I skimmed the design doc but didn't see anything > explaining > > > it. Is it in case the private key is compromised? Is Finch reliable enough > in > > > that scenario? > > > > > > I also have some questions about other parts of the trial token code that I > > > stumbled upon while reading this CL. (For example, why does the token > parsing > > > code live in content/common but appear to only be used from the renderer? > Can > > it > > > move into content/renderer? > > > > It did live there, until mek@ landed crrev.com/1742053002 -- we are intending > on > > using this in the browser as well. > > Gotcha. But, I see the end result of that review was thakis@ asking if it's okay > to have this in the browser process and mek@ agreeing to get a security review > before actually using it, so let's talk about it when you get back. :) > > > > > > And did you consider using JSON instead of the > > > custom pipe-separated notation that requires its own parsing code?) > > > > The physical encoding of the token was adapted from palmer@'s comments on the > > original design doc (content now moved to > > > https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M > > , but the comments are still in the comment thread of the design document) > > > > We wanted to have something reasonably compact (hence Ed25519 over RSA), that > > would survive developer-cut-and-paste, and easily fit within both a <meta> tag > > and an HTTP header, without becoming an escaping nightmare. I don't think we > > seriously considered using plaintext JSON for that reason. > > It sounds like base64-encoded stringified JSON would accomplish all those things > without having to maintain any of your own parsing code. > > > > > > iclelland@, > > > I don't want to derail this CL, so what would be the best way to discuss > those > > > questions? > > > > I'm OOO until Tuesday now, but I'd be happy to set up a meeting to talk about > > all of it as soon as I'm back. > > Ok, sounds good, drop something on my calendar whenever you get back. (And > probably palmer too if he wants.) Thanks for the explanations! estark -- Do you have any other issues with this code (besides the whole "should we change the token format completely" question, which is being dealt with outside of this issue)?
On 2016/04/06 19:38:52, iclelland wrote: > On 2016/03/18 02:59:01, estark wrote: > > On 2016/03/17 03:06:37, iclelland wrote: > > > On 2016/03/17 00:23:28, estark wrote: > > > > I think the crypto parts of this look okay to me. AFAIK there's not much > > > sanity > > > > checking to be done on an Ed25519 public key except that it's a point on > the > > > > curve, which ED25519_verify() must do. > > > > > > Yes, I thing that's the first thing that it does (see > > > x25519_ge_frombytes_vartime) > > > > > > > > > > > However, I'm not clear on what this mechanism accomplishes (setting a > public > > > key > > > > via field trial). I skimmed the design doc but didn't see anything > > explaining > > > > it. Is it in case the private key is compromised? Is Finch reliable enough > > in > > > > that scenario? > > > > > > > > I also have some questions about other parts of the trial token code that > I > > > > stumbled upon while reading this CL. (For example, why does the token > > parsing > > > > code live in content/common but appear to only be used from the renderer? > > Can > > > it > > > > move into content/renderer? > > > > > > It did live there, until mek@ landed crrev.com/1742053002 -- we are > intending > > on > > > using this in the browser as well. > > > > Gotcha. But, I see the end result of that review was thakis@ asking if it's > okay > > to have this in the browser process and mek@ agreeing to get a security review > > before actually using it, so let's talk about it when you get back. :) > > > > > > > > > And did you consider using JSON instead of the > > > > custom pipe-separated notation that requires its own parsing code?) > > > > > > The physical encoding of the token was adapted from palmer@'s comments on > the > > > original design doc (content now moved to > > > > > > https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M > > > , but the comments are still in the comment thread of the design document) > > > > > > We wanted to have something reasonably compact (hence Ed25519 over RSA), > that > > > would survive developer-cut-and-paste, and easily fit within both a <meta> > tag > > > and an HTTP header, without becoming an escaping nightmare. I don't think we > > > seriously considered using plaintext JSON for that reason. > > > > It sounds like base64-encoded stringified JSON would accomplish all those > things > > without having to maintain any of your own parsing code. > > > > > > > > > iclelland@, > > > > I don't want to derail this CL, so what would be the best way to discuss > > those > > > > questions? > > > > > > I'm OOO until Tuesday now, but I'd be happy to set up a meeting to talk > about > > > all of it as soon as I'm back. > > > > Ok, sounds good, drop something on my calendar whenever you get back. (And > > probably palmer too if he wants.) Thanks for the explanations! > > estark -- Do you have any other issues with this code (besides the whole "should > we change the token format completely" question, which is being dealt with > outside of this issue)? Talking to the component update folks, it may be that this is a good candidate for that mechanism instead. I'm going to split this CL up into two parts, and leave just the command-line addition here, so that developers can still manually test with their own keys, and move the finch bits into a new CL (which may not land, depending on what goes on with component updater).
Description was changed from ========== Allow field trials to override EF public key This adds a command-line flag, --origin-trial-public-key, which can be used to override the public key used to verify the signed tokens for origin trials. It also allows for Finch field trials to override the key, so that it can be updated outside of the browser release schedule if necessary. BUG=589830 ========== to ========== Allow command-line arguments to override EF public key This adds a command-line flag, --origin-trial-public-key, which can be used by developers to override the public key used to verify the signed tokens for origin trials. BUG=589830 ==========
Description was changed from ========== Allow command-line arguments to override EF public key This adds a command-line flag, --origin-trial-public-key, which can be used by developers to override the public key used to verify the signed tokens for origin trials. BUG=589830 ========== to ========== Allow command-line arguments to override EF public key This adds a command-line flag, --origin-trial-public-key, which can be used by developers to override the public key used to verify the signed tokens for origin trials. BUG=603588 ==========
> > estark -- Do you have any other issues with this code (besides the whole > "should > > we change the token format completely" question, which is being dealt with > > outside of this issue)? > estark - ping :) > Talking to the component update folks, it may be that this is a good candidate > for that mechanism instead. I'm going to split this CL up into two parts, and > leave just the command-line addition here, so that developers can still manually > test with their own keys, and move the finch bits into a new CL (which may not > land, depending on what goes on with component updater). That is now being worked on in https://codereview.chromium.org/1887743003/ I've created crbug.com/603588 to cover both (without mentioning Finch, for instance, in the title)
lgtm
On 2016/04/14 15:57:28, estark wrote: > lgtm Thanks! sky -- can you take a look?
https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:19: base::StringPiece GetPublicKey() const; On 2016/03/07 14:48:19, iclelland wrote: > On 2016/03/03 20:55:30, sky wrote: > > Is there a reason you don't return a const std::string& here? > > I was originally using uint8_t[] for key material everywhere, but it was > suggested (https://codereview.chromium.org/1522813002/) to use StringPiece > instead to pass them around, for safety. > > I suspect that std::string would be just as suitable as a container type, and > possibly easier to work with. I'm not partial to StringPiece in any way. You still have StringPiece. Is there a reason you didn't change it to return const std::string& ? https://codereview.chromium.org/1737693002/diff/120001/chrome/common/chrome_c... File chrome/common/chrome_content_client.h (right): https://codereview.chromium.org/1737693002/diff/120001/chrome/common/chrome_c... chrome/common/chrome_content_client.h:92: OriginTrialKeyManager* GetOriginTrialKeyManager() { nit: origin_trial_key_manager() . Also, provide a newline from the previous function as it looks like its part of the previous overrides when it isn't. https://codereview.chromium.org/1737693002/diff/120001/chrome/common/origin_t... File chrome/common/origin_trials/origin_trial_key_manager.cc (right): https://codereview.chromium.org/1737693002/diff/120001/chrome/common/origin_t... chrome/common/origin_trials/origin_trial_key_manager.cc:21: : public_key_(std::string(reinterpret_cast<const char*>(kDefaultPublicKey), Rather than the cast why not declare kDefaultPublicKey as a const char[] ? https://codereview.chromium.org/1737693002/diff/120001/chrome/common/origin_t... File chrome/common/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/120001/chrome/common/origin_t... chrome/common/origin_trials/origin_trial_key_manager.h:20: bool SetPublicKeyFromASCIIString(const std::string& ascii_public_key); Is there a reason to go with the setter instead of making this class immutable and creating a new one when you want to change the value? Less threading issues that way.
https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:19: base::StringPiece GetPublicKey() const; On 2016/04/14 16:19:51, sky wrote: > On 2016/03/07 14:48:19, iclelland wrote: > > On 2016/03/03 20:55:30, sky wrote: > > > Is there a reason you don't return a const std::string& here? > > > > I was originally using uint8_t[] for key material everywhere, but it was > > suggested (https://codereview.chromium.org/1522813002/) to use StringPiece > > instead to pass them around, for safety. > > > > I suspect that std::string would be just as suitable as a container type, and > > possibly easier to work with. I'm not partial to StringPiece in any way. > > You still have StringPiece. Is there a reason you didn't change it to return > const std::string& ? I mistook your original request to change it to be a design question. In an unrelated CL, palmer@ and I discussed the possibility of changing the public keys to std::vector<uint8_t> instead. I mentioned there that it would be a more substantial CL, since the keys are handled in a number of different places in the code. (The same comment applies here as well, if I were to change the type in this one) (https://codereview.chromium.org/1858763003/#msg18) All that really matters is that we have an 8-bit-safe container for ~32 bytes which we can pass around, and which can eventually be converted to a const uint8_t* for use by boringssl. std::string fits that bill, so does a vector of uint8_t. I'd like to make that change everywhere (probably in a separate CL, since the scope is bigger than this one) once I know which way to change it. If you feel that std::string is the better way to go, I'll do that; otherwise, I'll convert it all to std::vector. https://codereview.chromium.org/1737693002/diff/120001/chrome/common/chrome_c... File chrome/common/chrome_content_client.h (right): https://codereview.chromium.org/1737693002/diff/120001/chrome/common/chrome_c... chrome/common/chrome_content_client.h:92: OriginTrialKeyManager* GetOriginTrialKeyManager() { On 2016/04/14 16:19:51, sky wrote: > nit: origin_trial_key_manager() . Also, provide a newline from the previous > function as it looks like its part of the previous overrides when it isn't. Done, thanks. (Should show up in the next patchset) https://codereview.chromium.org/1737693002/diff/120001/chrome/common/origin_t... File chrome/common/origin_trials/origin_trial_key_manager.cc (right): https://codereview.chromium.org/1737693002/diff/120001/chrome/common/origin_t... chrome/common/origin_trials/origin_trial_key_manager.cc:21: : public_key_(std::string(reinterpret_cast<const char*>(kDefaultPublicKey), On 2016/04/14 16:19:51, sky wrote: > Rather than the cast why not declare kDefaultPublicKey as a const char[] ? The signedness of plain char isn't defined, and so I wasn't certain whether or not it is technically valid to assign character literals outside of the [0,0x7f] range to them. (Also see the previous comment about std::string -- is it better to make these char's, and build a std::string out of them, or leave them as uint8_t, and return a std::vector?) https://codereview.chromium.org/1737693002/diff/120001/chrome/common/origin_t... File chrome/common/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/120001/chrome/common/origin_t... chrome/common/origin_trials/origin_trial_key_manager.h:20: bool SetPublicKeyFromASCIIString(const std::string& ascii_public_key); On 2016/04/14 16:19:51, sky wrote: > Is there a reason to go with the setter instead of making this class immutable > and creating a new one when you want to change the value? Less threading issues > that way. That's a good point; I'll change that, and swap out the manager when the key changes instead. There are more values that could be set in the future (disabled features, revoked tokens, etc), but I should be able to gather them all at once, and make them constructor arguments instead.
https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:19: base::StringPiece GetPublicKey() const; On 2016/04/14 19:29:07, iclelland wrote: > On 2016/04/14 16:19:51, sky wrote: > > On 2016/03/07 14:48:19, iclelland wrote: > > > On 2016/03/03 20:55:30, sky wrote: > > > > Is there a reason you don't return a const std::string& here? > > > > > > I was originally using uint8_t[] for key material everywhere, but it was > > > suggested (https://codereview.chromium.org/1522813002/) to use StringPiece > > > instead to pass them around, for safety. > > > > > > I suspect that std::string would be just as suitable as a container type, > and > > > possibly easier to work with. I'm not partial to StringPiece in any way. > > > > You still have StringPiece. Is there a reason you didn't change it to return > > const std::string& ? > > I mistook your original request to change it to be a design question. > > In an unrelated CL, palmer@ and I discussed the possibility of changing the > public keys to std::vector<uint8_t> instead. I mentioned there that it would be > a more substantial CL, since the keys are handled in a number of different > places in the code. (The same comment applies here as well, if I were to change > the type in this one) > > (https://codereview.chromium.org/1858763003/#msg18) > > All that really matters is that we have an 8-bit-safe container for ~32 bytes > which we can pass around, and which can eventually be converted to a const > uint8_t* for use by boringssl. std::string fits that bill, so does a vector of > uint8_t. I'd like to make that change everywhere (probably in a separate CL, > since the scope is bigger than this one) once I know which way to change it. > > If you feel that std::string is the better way to go, I'll do that; otherwise, > I'll convert it all to std::vector. As you have a setter that takes a std::string I would inclined to make the getter return a string. too. I'm ok with a vector too, as long as they are consistent.
https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:19: base::StringPiece GetPublicKey() const; On 2016/04/14 20:58:04, sky wrote: > On 2016/04/14 19:29:07, iclelland wrote: > > On 2016/04/14 16:19:51, sky wrote: > > > On 2016/03/07 14:48:19, iclelland wrote: > > > > On 2016/03/03 20:55:30, sky wrote: > > > > > Is there a reason you don't return a const std::string& here? > > > > > > > > I was originally using uint8_t[] for key material everywhere, but it was > > > > suggested (https://codereview.chromium.org/1522813002/) to use StringPiece > > > > instead to pass them around, for safety. > > > > > > > > I suspect that std::string would be just as suitable as a container type, > > and > > > > possibly easier to work with. I'm not partial to StringPiece in any way. > > > > > > You still have StringPiece. Is there a reason you didn't change it to return > > > const std::string& ? > > > > I mistook your original request to change it to be a design question. > > > > In an unrelated CL, palmer@ and I discussed the possibility of changing the > > public keys to std::vector<uint8_t> instead. I mentioned there that it would > be > > a more substantial CL, since the keys are handled in a number of different > > places in the code. (The same comment applies here as well, if I were to > change > > the type in this one) > > > > (https://codereview.chromium.org/1858763003/#msg18) > > > > All that really matters is that we have an 8-bit-safe container for ~32 bytes > > which we can pass around, and which can eventually be converted to a const > > uint8_t* for use by boringssl. std::string fits that bill, so does a vector of > > uint8_t. I'd like to make that change everywhere (probably in a separate CL, > > since the scope is bigger than this one) once I know which way to change it. > > > > If you feel that std::string is the better way to go, I'll do that; otherwise, > > I'll convert it all to std::vector. > > As you have a setter that takes a std::string I would inclined to make the > getter return a string. too. I'm ok with a vector too, as long as they are > consistent. I'm not sure I understand you here, sorry -- the setter is explicitly a conversion, from the external representation of the key (a base64-encoded string) to the internal representation that gets held and passed around and used by boringssl (the raw 32 bytes). I don't think that tying the internal representation to the same type as the external is the goal here. Or am I misreading completely, and I should do the decoding from ASCII somewhere else in the code?
LGTM https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... File chrome/renderer/origin_trials/origin_trial_key_manager.h (right): https://codereview.chromium.org/1737693002/diff/60001/chrome/renderer/origin_... chrome/renderer/origin_trials/origin_trial_key_manager.h:19: base::StringPiece GetPublicKey() const; On 2016/04/15 03:09:46, iclelland wrote: > On 2016/04/14 20:58:04, sky wrote: > > On 2016/04/14 19:29:07, iclelland wrote: > > > On 2016/04/14 16:19:51, sky wrote: > > > > On 2016/03/07 14:48:19, iclelland wrote: > > > > > On 2016/03/03 20:55:30, sky wrote: > > > > > > Is there a reason you don't return a const std::string& here? > > > > > > > > > > I was originally using uint8_t[] for key material everywhere, but it was > > > > > suggested (https://codereview.chromium.org/1522813002/) to use > StringPiece > > > > > instead to pass them around, for safety. > > > > > > > > > > I suspect that std::string would be just as suitable as a container > type, > > > and > > > > > possibly easier to work with. I'm not partial to StringPiece in any way. > > > > > > > > You still have StringPiece. Is there a reason you didn't change it to > return > > > > const std::string& ? > > > > > > I mistook your original request to change it to be a design question. > > > > > > In an unrelated CL, palmer@ and I discussed the possibility of changing the > > > public keys to std::vector<uint8_t> instead. I mentioned there that it would > > be > > > a more substantial CL, since the keys are handled in a number of different > > > places in the code. (The same comment applies here as well, if I were to > > change > > > the type in this one) > > > > > > (https://codereview.chromium.org/1858763003/#msg18) > > > > > > All that really matters is that we have an 8-bit-safe container for ~32 > bytes > > > which we can pass around, and which can eventually be converted to a const > > > uint8_t* for use by boringssl. std::string fits that bill, so does a vector > of > > > uint8_t. I'd like to make that change everywhere (probably in a separate CL, > > > since the scope is bigger than this one) once I know which way to change it. > > > > > > If you feel that std::string is the better way to go, I'll do that; > otherwise, > > > I'll convert it all to std::vector. > > > > As you have a setter that takes a std::string I would inclined to make the > > getter return a string. too. I'm ok with a vector too, as long as they are > > consistent. > > I'm not sure I understand you here, sorry -- the setter is explicitly a > conversion, from the external representation of the key (a base64-encoded > string) to the internal representation that gets held and passed around and used > by boringssl (the raw 32 bytes). I don't think that tying the internal > representation to the same type as the external is the goal here. Fair enough. > Or am I misreading completely, and I should do the decoding from ASCII somewhere > else in the code? Nope. I still don't see you gaining anything by returning a StringPiece, but you said you were going to revisit that. So I'll approve this.
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1737693002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737693002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737693002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
iclelland@chromium.org changed reviewers: + aj.slater@gmail.com
(Just when I thought I had all the reviews covered :( ) +r jam (again, sorry) Can you PTAL at the two files in content/ ? Thanks, Ian
iclelland@chromium.org changed reviewers: + jam@chromium.org - aj.slater@gmail.com
-aj +jam (typo, sorry for the noise)
sorry for the delay, I missed this cl. in the future please IM me if I take more than a few hours to respond. https://codereview.chromium.org/1737693002/diff/120001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1737693002/diff/120001/content/app/content_ma... content/app/content_main_runner.cc:256: content_client->InitializeOriginTrials(); instead of adding a new ContentClient method, which we only do if necessary per https://www.chromium.org/developers/content-module/content-api, can you use existing methods on ContentMainDelegate? i.e. BasicStartupComplete?
> sorry for the delay, I missed this cl. in the future please IM me if I take more than a few hours to respond. I'll do that next time -- this time I ended up on vacation instead, so I didn't even notice the delay :) I see that this change technically removes you from the required reviewers, but can you take a look at the way I've moved this into chrome_main_delegate.cc? https://codereview.chromium.org/1737693002/diff/120001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/1737693002/diff/120001/content/app/content_ma... content/app/content_main_runner.cc:256: content_client->InitializeOriginTrials(); On 2016/04/21 17:56:19, jam wrote: > instead of adding a new ContentClient method, which we only do if necessary per > https://www.chromium.org/developers/content-module/content-api, can you use > existing methods on ContentMainDelegate? i.e. BasicStartupComplete? Yes I can. Other content embedders will need to follow the same logic if they want this feature, in that case, but it will be easy enough for them to do that. (And it's not a required feature; unless they want omaha/finch-style updating of origin trials configuration, it's not needed at all).
On 2016/04/25 15:26:18, iclelland wrote: > > sorry for the delay, I missed this cl. in the future please IM me if I take > more > than a few hours to respond. > > I'll do that next time -- this time I ended up on vacation instead, so I didn't > even notice the delay :) > > I see that this change technically removes you from the required reviewers, but > can you take a look at the way I've moved this into chrome_main_delegate.cc? > > https://codereview.chromium.org/1737693002/diff/120001/content/app/content_ma... > File content/app/content_main_runner.cc (right): > > https://codereview.chromium.org/1737693002/diff/120001/content/app/content_ma... > content/app/content_main_runner.cc:256: > content_client->InitializeOriginTrials(); > On 2016/04/21 17:56:19, jam wrote: > > instead of adding a new ContentClient method, which we only do if necessary > per > > https://www.chromium.org/developers/content-module/content-api, can you use > > existing methods on ContentMainDelegate? i.e. BasicStartupComplete? > > Yes I can. Other content embedders will need to follow the same logic if they > want this feature, in that case, but it will be easy enough for them to do that. > (And it's not a required feature; unless they want omaha/finch-style updating of > origin trials configuration, it's not needed at all). lgtm
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, chasej@chromium.org, asvitkine@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/1737693002/#ps140001 (title: "Remove new method from content API")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737693002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737693002/140001
Message was sent while issue was closed.
Description was changed from ========== Allow command-line arguments to override EF public key This adds a command-line flag, --origin-trial-public-key, which can be used by developers to override the public key used to verify the signed tokens for origin trials. BUG=603588 ========== to ========== Allow command-line arguments to override EF public key This adds a command-line flag, --origin-trial-public-key, which can be used by developers to override the public key used to verify the signed tokens for origin trials. BUG=603588 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Allow command-line arguments to override EF public key This adds a command-line flag, --origin-trial-public-key, which can be used by developers to override the public key used to verify the signed tokens for origin trials. BUG=603588 ========== to ========== Allow command-line arguments to override EF public key This adds a command-line flag, --origin-trial-public-key, which can be used by developers to override the public key used to verify the signed tokens for origin trials. BUG=603588 Committed: https://crrev.com/ee4f4a742db6b3e5c8bb8462f9da270c26be491c Cr-Commit-Position: refs/heads/master@{#389832} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ee4f4a742db6b3e5c8bb8462f9da270c26be491c Cr-Commit-Position: refs/heads/master@{#389832} |
