Provide valid port on HPKP reports for QUIC connections

Provide valid port on HPKP reports for QUIC connections

If a QUIC connection fails HPKP-pin validation, include the correct
port in the violation report instead of simply passing 0.

BUG=592047
Committed: https://crrev.com/954bb547e884323f9da52a32a54385288ebdb04c
Cr-Commit-Position: refs/heads/master@{#385023}

[elawrence, 2016-03-11 21:16:16]

elawrence@chromium.org changed reviewers:
+ estark@chromium.org

[elawrence, 2016-03-11 21:16:16]

Emily-- Here's my proposal for the QUIC HPKP port-reporting change. (I'm not
sure how to re-run only the failed trybots, but I'm pretty sure they were just
flaky?)

[estark, 2016-03-14 20:13:58]

Took a quick pass and overall looks good, but weren't we going to have to do
something more complicated because this will break the webrtc build? I kicked
off a couple webrtc try bots to check. If they're broken... I think we have to
come up with some annoyingly complicated multi-sided patch strategy to do this
in multiple stages without breaking webrtc.

About the failed trybots, they do indeed look like flakes, but you can rerun
with either `git cl try --bot=linux_android_rel_ng` on the command line, or here
on the code review site with the "Choose trybots" link next to the list of bots
that have run.

https://codereview.chromium.org/1780983002/diff/20001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/1780983002/diff/20001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier_chromium.cc:106: uint16_t port_;
Please add a comment on this similar to line 104.

[estark, 2016-03-14 20:18:27]

On 2016/03/14 20:13:58, estark wrote:
> Took a quick pass and overall looks good, but weren't we going to have to do
> something more complicated because this will break the webrtc build? I kicked
> off a couple webrtc try bots to check. If they're broken... I think we have to
> come up with some annoyingly complicated multi-sided patch strategy to do this
> in multiple stages without breaking webrtc.

Oh, darn, looks like the patch failed to apply, which means someone probably
committed something in the meantime that conflicts. To fix this, you'll have to
rebase on master:
$ git fetch
$ git rebase origin/master
<resolve conflicts>
<gclient sync and build to make sure everything's okay after the rebase
completes>
$ git cl upload
(enter "rebase" or something like that as the title so that reviewers know it's
a rebase)

Note: reviewers generally prefer if you do `git cl upload` immediately after
rebasing, and then do a separate `git cl upload` for changes you make after
rebasing (i.e. always do a rebase in its own separate patchset rather than
mixing it in with other changes). That way diffs from the rebase won't get mixed
in with your actual changes.

> 
> About the failed trybots, they do indeed look like flakes, but you can rerun
> with either `git cl try --bot=linux_android_rel_ng` on the command line, or
here
> on the code review site with the "Choose trybots" link next to the list of
bots
> that have run.
> 
>
https://codereview.chromium.org/1780983002/diff/20001/net/quic/crypto/proof_v...
> File net/quic/crypto/proof_verifier_chromium.cc (right):
> 
>
https://codereview.chromium.org/1780983002/diff/20001/net/quic/crypto/proof_v...
> net/quic/crypto/proof_verifier_chromium.cc:106: uint16_t port_;
> Please add a comment on this similar to line 104.

[chromium-reviews, 2016-03-14 20:19:43]

Yes, I expect the iOS webrtc stuff to fail. As we talked about, I expect I
will have to do some interesting things in order to get the other
repository fixed, but I thought I'd best not attempt that until I had
confirmation that the way that I'd approached this at all (e.g. simply
plumbing an additional uint16 through the argument lists) was the right way
to go.


On Mon, Mar 14, 2016 at 3:13 PM, <estark@chromium.org> wrote:

> Took a quick pass and overall looks good, but weren't we going to have to
> do
> something more complicated because this will break the webrtc build? I
> kicked
> off a couple webrtc try bots to check. If they're broken... I think we
> have to
> come up with some annoyingly complicated multi-sided patch strategy to do
> this
> in multiple stages without breaking webrtc.
>
> About the failed trybots, they do indeed look like flakes, but you can
> rerun
> with either `git cl try --bot=linux_android_rel_ng` on the command line,
> or here
> on the code review site with the "Choose trybots" link next to the list of
> bots
> that have run.
>
>
>
>
https://codereview.chromium.org/1780983002/diff/20001/net/quic/crypto/proof_v...
> File net/quic/crypto/proof_verifier_chromium.cc (right):
>
>
>
https://codereview.chromium.org/1780983002/diff/20001/net/quic/crypto/proof_v...
> net/quic/crypto/proof_verifier_chromium.cc:106: uint16_t port_;
> Please add a comment on this similar to line 104.
>
> https://codereview.chromium.org/1780983002/
>



-- 
Eric Lawrence
Chrome Security

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[estark, 2016-03-14 21:20:03]

On 2016/03/14 20:19:43, chromium-reviews wrote:
> Yes, I expect the iOS webrtc stuff to fail. As we talked about, I expect I
> will have to do some interesting things in order to get the other
> repository fixed, but I thought I'd best not attempt that until I had
> confirmation that the way that I'd approached this at all (e.g. simply
> plumbing an additional uint16 through the argument lists) was the right way
> to go.

Ah, ok, I see. In that case, yeah, this general approach seems reasonable to me.

> 
> 
> On Mon, Mar 14, 2016 at 3:13 PM, <mailto:estark@chromium.org> wrote:
> 
> > Took a quick pass and overall looks good, but weren't we going to have to
> > do
> > something more complicated because this will break the webrtc build? I
> > kicked
> > off a couple webrtc try bots to check. If they're broken... I think we
> > have to
> > come up with some annoyingly complicated multi-sided patch strategy to do
> > this
> > in multiple stages without breaking webrtc.
> >
> > About the failed trybots, they do indeed look like flakes, but you can
> > rerun
> > with either `git cl try --bot=linux_android_rel_ng` on the command line,
> > or here
> > on the code review site with the "Choose trybots" link next to the list of
> > bots
> > that have run.
> >
> >
> >
> >
>
https://codereview.chromium.org/1780983002/diff/20001/net/quic/crypto/proof_v...
> > File net/quic/crypto/proof_verifier_chromium.cc (right):
> >
> >
> >
>
https://codereview.chromium.org/1780983002/diff/20001/net/quic/crypto/proof_v...
> > net/quic/crypto/proof_verifier_chromium.cc:106: uint16_t port_;
> > Please add a comment on this similar to line 104.
> >
> > https://codereview.chromium.org/1780983002/
> >
> 
> 
> 
> -- 
> Eric Lawrence
> Chrome Security
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[elawrence, 2016-03-24 19:14:35]

elawrence@chromium.org changed reviewers:
+ rtenneti@chromium.org

[elawrence, 2016-03-24 19:14:35]

elawrence@chromium.org changed required reviewers:
+ estark@chromium.org, rtenneti@chromium.org

[elawrence, 2016-03-24 19:14:36]

Proposing rtenneti@chromium.org as an owner/reviewer as this change is hits the
same files as https://codereview.chromium.org/1765603002 which he just reviewed.

(The WebRTC\p2p\quic code isn't compiled and owner Peter Thatcher told me not to
worry about this change's impact on that code as their verifier is a trivial
stub. Besides, 1765603002 would've broken it first anyway.)

[ramant (doing other things), 2016-03-24 23:19:02]

Description was changed from

==========
Provide valid port on HPKP reports for QUIC connections

If a QUIC connection fails HPKP-pin validation, include the correct
port in the violation report instead of simply passing 0.

BUG=592047
==========

to

==========
Provide valid port on HPKP reports for QUIC connections

If a QUIC connection fails HPKP-pin validation, include the correct
port in the violation report instead of simply passing 0.

BUG=592047
==========

[ramant (doing other things), 2016-03-24 23:19:03]

rtenneti@chromium.org changed reviewers:
+ rch@chromium.org

[ramant (doing other things), 2016-03-24 23:22:38]

lgtm

LGTM, but please wait for rch's approval.

Some of the files are shared code withe internal server. We may need to back
port some of the changes to the internal source tree.

[Ryan Hamilton, 2016-03-29 15:56:35]

On 2016/03/24 23:22:38, ramant wrote:
> lgtm
> 
> LGTM, but please wait for rch's approval.
> 
> Some of the files are shared code withe internal server. We may need to back
> port some of the changes to the internal source tree.

I trust Raman's review and am OOO until next week. I'm fine with landing. Raman,
can you coordiate with elawrence to make sure this gets merged back internally?

[Ryan Hamilton, 2016-04-04 15:54:29]

On 2016/03/29 15:56:35, Ryan Hamilton wrote:
> On 2016/03/24 23:22:38, ramant wrote:
> > lgtm
> > 
> > LGTM, but please wait for rch's approval.
> > 
> > Some of the files are shared code withe internal server. We may need to back
> > port some of the changes to the internal source tree.
> 
> I trust Raman's review and am OOO until next week. I'm fine with landing.
Raman,
> can you coordiate with elawrence to make sure this gets merged back
internally?

Did this land?

[chromium-reviews, 2016-04-04 15:56:00]

Not yet; I'm planning to do so today and submit for review the changes to
the Google3 version.

On Mon, Apr 4, 2016 at 10:54 AM, <rch@chromium.org> wrote:

> On 2016/03/29 15:56:35, Ryan Hamilton wrote:
> > On 2016/03/24 23:22:38, ramant wrote:
> > > lgtm
> > >
> > > LGTM, but please wait for rch's approval.
> > >
> > > Some of the files are shared code withe internal server. We may need
> to back
> > > port some of the changes to the internal source tree.
> >
> > I trust Raman's review and am OOO until next week. I'm fine with landing.
> Raman,
> > can you coordiate with elawrence to make sure this gets merged back
> internally?
>
> Did this land?
>
> https://codereview.chromium.org/1780983002/
>



-- 
Eric Lawrence
Chrome Security

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[elawrence, 2016-04-04 20:45:18]

The CQ bit was checked by elawrence@chromium.org

[commit-bot: I haz the power, 2016-04-04 20:45:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1780983002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1780983002/80001

[commit-bot: I haz the power, 2016-04-04 20:45:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-04 20:45:56]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[estark, 2016-04-04 20:47:22]

lgtm

[elawrence, 2016-04-04 20:48:32]

The CQ bit was checked by elawrence@chromium.org

[commit-bot: I haz the power, 2016-04-04 20:49:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1780983002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1780983002/80001

[commit-bot: I haz the power, 2016-04-04 22:03:19]

Description was changed from

==========
Provide valid port on HPKP reports for QUIC connections

If a QUIC connection fails HPKP-pin validation, include the correct
port in the violation report instead of simply passing 0.

BUG=592047
==========

to

==========
Provide valid port on HPKP reports for QUIC connections

If a QUIC connection fails HPKP-pin validation, include the correct
port in the violation report instead of simply passing 0.

BUG=592047
==========

[commit-bot: I haz the power, 2016-04-04 22:03:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-04 22:04:48]

Description was changed from

==========
Provide valid port on HPKP reports for QUIC connections

If a QUIC connection fails HPKP-pin validation, include the correct
port in the violation report instead of simply passing 0.

BUG=592047
==========

to

==========
Provide valid port on HPKP reports for QUIC connections

If a QUIC connection fails HPKP-pin validation, include the correct
port in the violation report instead of simply passing 0.

BUG=592047

Committed: https://crrev.com/954bb547e884323f9da52a32a54385288ebdb04c
Cr-Commit-Position: refs/heads/master@{#385023}
==========

[commit-bot: I haz the power, 2016-04-04 22:04:49]

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