Closed Bug 1322373 Opened 3 years ago Closed 3 years ago

TLS 1.3 early-data for http/2

Categories

(Core :: Networking: HTTP, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: u408661, Assigned: mcmanus)

References

(Depends on 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

We need to support early-data when talking http/2 to servers, not just http/1.
Can we send the early data for more than one transaction? It should be possible.
Can we also rewind h2 transaction similar as we do with h1, in that case we do not need to keep the buffer and we can call RealignOutputQueue. (this is not that important, it will work without it as well).
(In reply to Dragana Damjanovic [:dragana] from comment #2)
> Can we send the early data for more than one transaction? It should be
> possible.

The chances of this even being possible on a new connection with h2 are pretty slim - most of the time, this will be the root pageload, so we won't have other resources to fetch yet.

> Can we also rewind h2 transaction similar as we do with h1, in that case we
> do not need to keep the buffer and we can call RealignOutputQueue. (this is
> not that important, it will work without it as well).

Sure, we could, but why bother? Not that the optmization of keeping the buffer matters all that much, but there's no particular reason to *not* keep the data around (and it makes the "early data rejected but still h2" case much easier).
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #3)
> (In reply to Dragana Damjanovic [:dragana] from comment #2)
> > Can we send the early data for more than one transaction? It should be
> > possible.
> 
> The chances of this even being possible on a new connection with h2 are
> pretty slim - most of the time, this will be the root pageload, so we won't
> have other resources to fetch yet.

not necessarily true - esp where conn is to a cdn carrying the static resources in a different origin. this is an impt feature. maybe it can be added on.
(In reply to Patrick McManus [:mcmanus] from comment #4)
> not necessarily true - esp where conn is to a cdn carrying the static
> resources in a different origin. this is an impt feature. maybe it can be
> added on.

Good point. Thinking about it, it should be pretty easy to add - just need to keep an array of pointers instead of the single pointer. Then just return the first one in the array to the connection, since that should be the first one that got dispatched to us (or near enough) and NET_RESET the rest.
Comment on attachment 8836178 [details]
Bug 1322373 - TLS 1.3 early-data for HTTP/2.

https://reviewboard.mozilla.org/r/111614/#review113862

I think we need some changes, but they aren't big changes. I also would like dragana to take a look at nsHttpConnection around the existing early data bits. but I'm comfortable with doing the h2 parts.

::: netwerk/protocol/http/ASpdySession.h:32
(Diff revision 1)
>    virtual bool RoomForMoreStreams() = 0;
>    virtual PRIntervalTime IdleTime() = 0;
>    virtual uint32_t ReadTimeoutTick(PRIntervalTime now) = 0;
>    virtual void DontReuse() = 0;
> +  virtual uint32_t SpdyVersion() = 0;
> +  virtual nsAHttpTransaction *Take0RTTTransaction() = 0;

TakeFOO normally means that the callee is giving up its reference so the caller can have it. It looks like this is just an accessor so it should either be called GetFOO or just FOO.. (or maybe it should be returning some already_addreffed var and dropping its own ref?)

::: netwerk/protocol/http/Http2Session.h:503
(Diff revision 1)
>    bool mGoAwayOnPush;
>  
>    bool mUseH2Deps;
>  
> +  bool mAttemptingEarlyData;
> +  // The stream that we are getting 0RTT data from.

is there a reason to be sure this won't be a uaf pointer?

its presence in the transactionHash is what keeps it alive.

I think you need to store it by ID and look it up in mStreamIDHash at use time (perhaps with a verifystream() thrown in).

I wouldn't try and keep it alive with a reference.

perhaps a cancel from gecko, or a rst from the peer could make this happen.

::: netwerk/protocol/http/Http2Session.cpp:546
(Diff revision 1)
>  
>    // Dont worry about errors on write, we will pick this up as a read error too
>    if (NS_FAILED(rv))
>      return;
>  
> -  if (countRead == avail) {
> +  if (countRead == avail && !mAttemptingEarlyData) {

we can remove a footgun here by moving up the sent += countRead line to be right after the NS_FAILED check and then just do if (attemptingEarlyData) {return;}

::: netwerk/protocol/http/Http2Session.cpp:2378
(Diff revision 1)
>            this));
>      FlushOutputQueue();
>      SetWriteCallbacks();
> +    if (mAttemptingEarlyData) {
> +      // We can still try to send our preamble as early-data
> +      *countRead = mOutputQueueUsed - mOutputQueueSent;

If you're going to return a countRead, I think you should return OK.

::: netwerk/protocol/http/Http2Session.cpp:2395
(Diff revision 1)
> +      // We can still send our preamble
> +      *countRead = mOutputQueueUsed - mOutputQueueSent;
> +      return NS_OK;
> +    }
> +
> +    m0RTTStream = stream;

what if m0RTTStream is not null? this function is called in a loop looking for things on the writeQ.. that's how you would get multiple transactons when we're ready to deal with that..

::: netwerk/protocol/http/Http2Session.cpp:2971
(Diff revision 1)
>  
>  nsresult
> +Http2Session::Finish0RTT(bool aRestart, bool aAlpnChanged)
> +{
> +  MOZ_ASSERT(mAttemptingEarlyData);
> +

LOG Finish0Rtt(arestart, aalpnchanged)

::: netwerk/protocol/http/Http2Session.cpp:2972
(Diff revision 1)
>  nsresult
> +Http2Session::Finish0RTT(bool aRestart, bool aAlpnChanged)
> +{
> +  MOZ_ASSERT(mAttemptingEarlyData);
> +
> +  if (m0RTTStream) {

see comment in .h file about referencing this by id instead of raw pointer

::: netwerk/protocol/http/Http2Session.cpp:2978
(Diff revision 1)
> +    // We use aAlpnChanged here, because if the ALPN selection hasn't changed,
> +    // then we have data we can just use and all we need to do is rewind the
> +    // session's internal buffer. If the ALPN selection changed, though, then
> +    // we need to do a bit more work to get all our transactions back into a
> +    // place where they can do http/1
> +    m0RTTStream->Finish0RTT(aAlpnChanged, aAlpnChanged);

I think you mean (aRestart, aAlpnChanged) ?

::: netwerk/protocol/http/Http2Session.cpp:2987
(Diff revision 1)
> +    // 0RTT failed
> +    if (aAlpnChanged) {
> +      // This is a slightly more involved case - we need to get all our streams/
> +      // transactions back in the queue so they can restart as http/1
> +
> +      // In order for the connection to be reused, we need to reset the

this path is pretty scary and I think we can simplify it if we don't care about keeping the connection open - and I don't as the change from h2 to h1 should not be a normal thing, as long as we recover with a new conn its ok.

If we are ok with that - won't close()->shutdown() just do the right thing?

this is pretty much going to be required when there is more than 1 transaction in early data anyhow.

otoh, you might be able to save the optimization by just monkeying with the reuse flag (setting it back to true) on the conn after Close() has returned below.. and I think you could set the connection in the trans to null (but I'm not 100% sure) and the cmgr will just find one according to its normal precedence rules.

its the cleanup up of streams here that makes me really nervous. that's been a source of bugs before and its best to leave it in the existing subroutines where possible.

::: netwerk/protocol/http/Http2Session.cpp:2995
(Diff revision 1)
> +      // the TCP handshake all over again. Later on, the connection will reclaim
> +      // this transaction from us and start it appropriately.
> +      if (m0RTTStream) {
> +        m0RTTTransaction = m0RTTStream->Transaction();
> +        m0RTTTransaction->SetConnection(mConnection);
> +        mStreamTransactionHash.Remove(m0RTTTransaction);

so this leaves a bunch of dangling uaf raw pointers around.. in mStreamIDHash, and the various nsDeque's (like readyForWrite)

::: netwerk/protocol/http/Http2Session.cpp:3004
(Diff revision 1)
> +      mGoAwayID = 0;
> +      mCleanShutdown = true;
> +
> +      // Close takes care of the rest of our work for us. The reason code here
> +      // doesn't matter, as we aren't actually going to send a GOAWAY frame.
> +      Close(NS_OK);

make the reason NET_ERROR_RESET just for the log. its closer to the truth.

::: netwerk/protocol/http/Http2Session.cpp:3012
(Diff revision 1)
> +      // we just need to rewind to the beginning of the preamble and try again.
> +      mOutputQueueSent = 0;
> +    }
> +  } else {
> +    // 0RTT succeeded
> +    if (mOutputQueueSent == mOutputQueueUsed) {

maybe just RealignOutputQueue() after attemptingEarlyData is set to false?

::: netwerk/protocol/http/nsHttpConnection.h:380
(Diff revision 1)
>                                                               // are waiting
>                                                               // for the end of
>                                                               // the handsake.
>      int64_t                        mContentBytesWritten0RTT;
>      bool                           mEarlyDataNegotiated; //Only used for telemetry
> +    nsAutoCString                  mEarlyNegotiatedALPN;

auto generally only goes on the stack.. so nsCString

::: netwerk/protocol/http/nsHttpConnection.cpp:508
(Diff revision 1)
>            LOG(("nsHttpConnection::EnsureNPNComplete [this=%p] - %d bytes "
>                 "has been sent during 0RTT.", this, mContentBytesWritten0RTT));
>            mContentBytesWritten = mContentBytesWritten0RTT;
> +          if (mSpdySession) {
> +              // We had already started 0RTT-spdy, now we need to fully set up
> +              // spdy, since we know we're sticking with it.

LOG(())
Attachment #8836178 - Flags: review?(mcmanus) → review-
Comment on attachment 8836178 [details]
Bug 1322373 - TLS 1.3 early-data for HTTP/2.

dragana, would you mind reviewing nsHttpConnection?

thanks nick!
Attachment #8836178 - Flags: review?(dd.mozilla)
Most of these are easy/reasonable enough (especially if we come from the perspective of not caring about reusing the connection - maybe I shouldn't have spent as much time on it as I did!). I'll rework this and should have a new iteration sometime Wednesday. But first, a few responses inline...

(In reply to Patrick McManus [:mcmanus] from comment #6)
> Comment on attachment 8836178 [details]
> Bug 1322373 - TLS 1.3 early-data for HTTP/2.
> 
> https://reviewboard.mozilla.org/r/111614/#review113862
> 
> I think we need some changes, but they aren't big changes. I also would like
> dragana to take a look at nsHttpConnection around the existing early data
> bits. but I'm comfortable with doing the h2 parts.
> 
> ::: netwerk/protocol/http/ASpdySession.h:32
> (Diff revision 1)
> >    virtual bool RoomForMoreStreams() = 0;
> >    virtual PRIntervalTime IdleTime() = 0;
> >    virtual uint32_t ReadTimeoutTick(PRIntervalTime now) = 0;
> >    virtual void DontReuse() = 0;
> > +  virtual uint32_t SpdyVersion() = 0;
> > +  virtual nsAHttpTransaction *Take0RTTTransaction() = 0;
> 
> TakeFOO normally means that the callee is giving up its reference so the
> caller can have it. It looks like this is just an accessor so it should
> either be called GetFOO or just FOO.. (or maybe it should be returning some
> already_addreffed var and dropping its own ref?)

Indeed - I know I had it returning already_addrefed at one point, not sure when that disappeared. But, if we get rid of the connection reuse logic, this goes away anyway (along with all the other dangling pointers, with a few other minor modifications).

> ::: netwerk/protocol/http/Http2Session.cpp:2978
> (Diff revision 1)
> > +    // We use aAlpnChanged here, because if the ALPN selection hasn't changed,
> > +    // then we have data we can just use and all we need to do is rewind the
> > +    // session's internal buffer. If the ALPN selection changed, though, then
> > +    // we need to do a bit more work to get all our transactions back into a
> > +    // place where they can do http/1
> > +    m0RTTStream->Finish0RTT(aAlpnChanged, aAlpnChanged);
> 
> I think you mean (aRestart, aAlpnChanged) ?

That all depends on if we want to re-use what we have in the session buffer or not. If we do, then using aAlpnChanged is the right thing to do - we don't want the transaction to rewind itself when we're going to be using a copy of that anyway (so far as the transaction knows, it's already sent its data, it's just buffered somewhere along the network). This is what the comment above was meant to explain, obviously I did a poor job writing it. I'll re-write for clarity. Or, we can just rewind everything and empty out the session buffer no matter what (except for the connection preamble).

> ::: netwerk/protocol/http/Http2Session.cpp:2987
> (Diff revision 1)
> > +    // 0RTT failed
> > +    if (aAlpnChanged) {
> > +      // This is a slightly more involved case - we need to get all our streams/
> > +      // transactions back in the queue so they can restart as http/1
> > +
> > +      // In order for the connection to be reused, we need to reset the
> 
> this path is pretty scary and I think we can simplify it if we don't care
> about keeping the connection open - and I don't as the change from h2 to h1
> should not be a normal thing, as long as we recover with a new conn its ok.
> 
> If we are ok with that - won't close()->shutdown() just do the right thing?

Absolutely. That's what happened before I got the "reuse the connection" case working.

> this is pretty much going to be required when there is more than 1
> transaction in early data anyhow.

I think this would be doable by letting all but the first 0rtt transaction rewind and go through the usual close/shutdown path. But that would likely make this code path even scarier than it already is :)

> otoh, you might be able to save the optimization by just monkeying with the
> reuse flag (setting it back to true) on the conn after Close() has returned
> below.. and I think you could set the connection in the trans to null (but
> I'm not 100% sure) and the cmgr will just find one according to its normal
> precedence rules.

I may have missed the proper ordering of things or something, but try as I wanted to, I couldn't get this to work with the (mostly) regular close/shutdown case. So I'm not confident in that.

> its the cleanup up of streams here that makes me really nervous. that's been
> a source of bugs before and its best to leave it in the existing subroutines
> where possible.

Sounds fine. Let's just dial back, it's a mostly-silly optimization, anyway.

> ::: netwerk/protocol/http/Http2Session.cpp:3012
> (Diff revision 1)
> > +      // we just need to rewind to the beginning of the preamble and try again.
> > +      mOutputQueueSent = 0;
> > +    }
> > +  } else {
> > +    // 0RTT succeeded
> > +    if (mOutputQueueSent == mOutputQueueUsed) {
> 
> maybe just RealignOutputQueue() after attemptingEarlyData is set to false?

That could work, but I wanted to avoid a memmove if possible. Maybe realignoutputqueue is smart enough for that, though, and I misread.
Comment on attachment 8836178 [details]
Bug 1322373 - TLS 1.3 early-data for HTTP/2.

https://reviewboard.mozilla.org/r/111614/#review114568

Have you tested chane from early_alpn=h1 to alpn=h2? It should just work because it is an easy case.
Attachment #8836178 - Flags: review?(dd.mozilla) → review+
(In reply to Dragana Damjanovic [:dragana] from comment #10)
> Comment on attachment 8836178 [details]
> Bug 1322373 - TLS 1.3 early-data for HTTP/2.
> 
> https://reviewboard.mozilla.org/r/111614/#review114568
> 
> Have you tested chane from early_alpn=h1 to alpn=h2? It should just work
> because it is an easy case.

I kind of assumed it worked, since that should've already been handled by the h1 early-data case :) I'll double-check, just to make sure I didn't break anything in the process.
(In reply to Dragana Damjanovic [:dragana] from comment #10)
> Comment on attachment 8836178 [details]
> Bug 1322373 - TLS 1.3 early-data for HTTP/2.
> 
> https://reviewboard.mozilla.org/r/111614/#review114568
> 
> Have you tested chane from early_alpn=h1 to alpn=h2? It should just work
> because it is an easy case.

I have only reviewed nsHttpConnection part.

Thanks for checking h1.
Comment on attachment 8836178 [details]
Bug 1322373 - TLS 1.3 early-data for HTTP/2.

https://reviewboard.mozilla.org/r/111614/#review114938

looks really good to me - thanks. I'm especially pleased it was that easy to accommodate N transactions.

can you make sure to test the N 0rtt transactions? It would probably be pretty easy to do so by using the 0rtt origin just for subresources of a sample page (say 3 js files).
Attachment #8836178 - Flags: review?(mcmanus) → review+
(In reply to Patrick McManus [:mcmanus] from comment #13)
> Comment on attachment 8836178 [details]
> Bug 1322373 - TLS 1.3 early-data for HTTP/2.
> 
> https://reviewboard.mozilla.org/r/111614/#review114938
> 
> looks really good to me - thanks. I'm especially pleased it was that easy to
> accommodate N transactions.
> 
> can you make sure to test the N 0rtt transactions? It would probably be
> pretty easy to do so by using the 0rtt origin just for subresources of a
> sample page (say 3 js files).

I'd love to... but mint appears to only be able to serve one file. I can't imagine we would send 3 requests for the same js (or image, or ...) file, even if it's referenced 3 times on the page... right? And I think the CF test server doesn't speak h2? So I'm at a loss for how to test this in a way that's useful.
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #14)
> (In reply to Patrick McManus [:mcmanus] from comment #13)
> > Comment on attachment 8836178 [details]
> > Bug 1322373 - TLS 1.3 early-data for HTTP/2.
> > 
> > https://reviewboard.mozilla.org/r/111614/#review114938
> > 
> > looks really good to me - thanks. I'm especially pleased it was that easy to
> > accommodate N transactions.
> > 
> > can you make sure to test the N 0rtt transactions? It would probably be
> > pretty easy to do so by using the 0rtt origin just for subresources of a
> > sample page (say 3 js files).
> 
> I'd love to... but mint appears to only be able to serve one file. I can't
> imagine we would send 3 requests for the same js (or image, or ...) file,
> even if it's referenced 3 times on the page... right? And I think the CF
> test server doesn't speak h2? So I'm at a loss for how to test this in a way
> that's useful.

(Though I suppose I could attempt to learn enough to to make mint serve more than one file - perhaps an entire directory!)
that's awkward. I believe you could do 3 XHR gets in parallel for the same URI... we changed things a while back so those weren't serialized on the cache. (put no-cache in the request headers just for emphasis)..

I would try it out in a non-early data scenario just to see if it was doing the necessary thing first :)

maybe barnes can help with a better strategy. I'll email
Actually, looking at mint again, I think I misinterpreted. There is only one set of bytes it will respond with, but it will respond to *any* URL with that set of bytes. So, I can send as many requests as I want, so long as I don't care that the responses are all the same (I don't).
(In reply to Patrick McManus [:mcmanus] from comment #13)
> can you make sure to test the N 0rtt transactions? It would probably be
> pretty easy to do so by using the 0rtt origin just for subresources of a
> sample page (say 3 js files).

And this works! 3 images - lots more early data sent and accepted. Calling this one ready to land.
Pushed by hurley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40d227b74dd1
TLS 1.3 early-data for HTTP/2. r=dragana,mcmanus
nick is on pto this week and this just looks a like a complaint about a compiler and logging string formatter. I'll try and get it relanded.
I have smoketested this against https://h2o.examp1e.net/ as well
Assignee: hurley → mcmanus
Status: NEW → ASSIGNED
Attachment #8836178 - Attachment is obsolete: true
Attachment #8839700 - Flags: review+
nick, I forgot to reset the author of the patch back to you before relanding. sorry!
https://hg.mozilla.org/mozilla-central/rev/001bc0207dbc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(In reply to Patrick McManus [:mcmanus] from comment #27)
> nick, I forgot to reset the author of the patch back to you before
> relanding. sorry!

No worries. If android's compiler weren't The Worst(tm) you wouldn't have had to fix my mess up :) Thanks for doing that!
Depends on: 1345240
Depends on: 1344890
Depends on: 1352156
You need to log in before you can comment on or make changes to this bug.