New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP/2 enforce HTTP message flow #6962

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@Scottmitch
Member

Scottmitch commented Jul 11, 2017

Motivation:
codec-http2 currently does not strictly enforce the HTTP/1.x semantics with respect to the number of headers defined in RFC 7540 Section 8.1 [1]. We currently don't validate the number of headers nor do we validate that the trailing headers should indicate EOS.

[1] https://tools.ietf.org/html/rfc7540#section-8.1

Modifications:

  • DefaultHttp2ConnectionDecoder should only allow decoding of a single headers and a single trailers
  • DefaultHttp2ConnectionEncoder should only allow encoding of a single headers and optionally a single trailers

Result:
Constraints of RFC 7540 restricting the number of headers/trailers is enforced.

@Scottmitch Scottmitch added the defect label Jul 11, 2017

@Scottmitch Scottmitch added this to the 4.1.14.Final milestone Jul 11, 2017

@Scottmitch Scottmitch self-assigned this Jul 11, 2017

@Scottmitch Scottmitch requested a review from normanmaurer Jul 11, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch
Member

Scottmitch commented Jul 11, 2017

public void trailersDoNotEndStreamThrows() {
writeAllFlowControlledFrames();
final int streamId = 6;
ChannelPromise promise = newPromise();

This comment has been minimized.

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

This comment has been minimized.

@Scottmitch

Scottmitch Jul 18, 2017

Member

I don't think it is necessary because we are mocking out the underlying transport ... we mostly care when things have been failed (because it is done at the encoder level).

@Scottmitch

Scottmitch Jul 18, 2017

Member

I don't think it is necessary because we are mocking out the underlying transport ... we mostly care when things have been failed (because it is done at the encoder level).

This comment has been minimized.

@normanmaurer

normanmaurer Jul 18, 2017

Member

ok fair enough

@normanmaurer

normanmaurer Jul 18, 2017

Member

ok fair enough

public void trailersDoNotEndStreamWithDataThrows() {
writeAllFlowControlledFrames();
final int streamId = 6;
ChannelPromise promise = newPromise();

This comment has been minimized.

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

private void tooManyHeadersThrows(boolean eos) {
writeAllFlowControlledFrames();
final int streamId = 6;
ChannelPromise promise = newPromise();

This comment has been minimized.

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

final int streamId = 6;
ChannelPromise promise = newPromise();
encoder.writeHeaders(ctx, streamId, EmptyHttp2Headers.INSTANCE, 0, false, promise);
ChannelPromise promise2 = newPromise();

This comment has been minimized.

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

private void tooManyHeadersWithDataThrows(boolean eos) {
writeAllFlowControlledFrames();
final int streamId = 6;
ChannelPromise promise = newPromise();

This comment has been minimized.

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

Http2Stream stream = connection.stream(streamId);
when(remoteFlow.hasFlowControlled(eq(stream))).thenReturn(true);
ChannelPromise promise2 = newPromise();

This comment has been minimized.

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

@normanmaurer

normanmaurer Jul 17, 2017

Member

should we assert the state of the promise after the write call ?

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jul 18, 2017

Member

@ejona86 - The PR has been corrected to support informational headers flow. PTAL.

Member

Scottmitch commented Jul 18, 2017

@ejona86 - The PR has been corrected to support informational headers flow. PTAL.

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jul 18, 2017

Member

@ejona86 - Updated to support multiple info headers ... PTAL.

Member

Scottmitch commented Jul 18, 2017

@ejona86 - Updated to support multiple info headers ... PTAL.

@@ -282,6 +284,14 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
return;
}
boolean isInformational = !connection.isServer() &&
HttpStatusClass.valueOf(headers.status()) == INFORMATIONAL;
if ((isInformational || !endOfStream) && stream.isHeadersReceived() || stream.isTrailersReceived()) {

This comment has been minimized.

@netkins

netkins Jul 19, 2017

BLOCKER NullPointerException might be thrown as 'stream' is nullable here rule
BLOCKER NullPointerException might be thrown as 'stream' is nullable here rule

@netkins

netkins Jul 19, 2017

BLOCKER NullPointerException might be thrown as 'stream' is nullable here rule
BLOCKER NullPointerException might be thrown as 'stream' is nullable here rule

private static final byte META_STATE_SENT_TRAILERS = 1 << 2;
private static final byte META_STATE_SENT_PUSHPROMISE = 1 << 3;
private static final byte META_STATE_RECV_HEADERS = 1 << 4;
private static final byte META_STATE_RECV_TRAILERS = 1 << 5;

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Jul 19, 2017

Member

Optional: you might consider using EnumSet for this.

@carl-mastrangelo

carl-mastrangelo Jul 19, 2017

Member

Optional: you might consider using EnumSet for this.

This comment has been minimized.

@Scottmitch

Scottmitch Jul 19, 2017

Member

I will just stick with manual bit masking for now ... less object overhead and self contained.

@Scottmitch

Scottmitch Jul 19, 2017

Member

I will just stick with manual bit masking for now ... less object overhead and self contained.

boolean endOfStream) {
boolean isInformational = isServer && HttpStatusClass.valueOf(headers.status()) == INFORMATIONAL;
if ((isInformational || !endOfStream) && stream.isHeadersSent() || stream.isTrailersSent()) {
throw new IllegalStateException("Stream " + stream.id() + " sent too many headers EOS: " + endOfStream);

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Jul 19, 2017

Member

nit: IllegalStateException is for calling Java methods out of order, not for network related errors.

From an API standpoint, I would expect a method named "validate..." to also have a throws clause.

@carl-mastrangelo

carl-mastrangelo Jul 19, 2017

Member

nit: IllegalStateException is for calling Java methods out of order, not for network related errors.

From an API standpoint, I would expect a method named "validate..." to also have a throws clause.

This comment has been minimized.

@Scottmitch

Scottmitch Jul 19, 2017

Member

IllegalStateException ...

Lets consider this for a followup PR. This PR is just being consistent with the existing approach.

From an API standpoint

IIUC is generally not common practice to include throws clauses for runtime exceptions. Can you can submit a followup PR to make the changes you have in mind to clarify?

@Scottmitch

Scottmitch Jul 19, 2017

Member

IllegalStateException ...

Lets consider this for a followup PR. This PR is just being consistent with the existing approach.

From an API standpoint

IIUC is generally not common practice to include throws clauses for runtime exceptions. Can you can submit a followup PR to make the changes you have in mind to clarify?

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Jul 19, 2017

Member

IIUC is generally not common practice to include throws clauses for runtime exceptions. Can you can submit a followup PR to make the changes you have in mind to clarify?

I am coming from a Guava mindset, which has a special exception for "The remote side messed up" called VerificationException. I think you don't depend on Guava, but it's a useful idea. If there isn't any recovery path from such an exception, why not just make it RuntimeException, or IORuntimeException, or some such?

Followup PR for ISE is fine. ISE is really meant for thinks like calling read() after close(). It's a programmer error.

@carl-mastrangelo

carl-mastrangelo Jul 19, 2017

Member

IIUC is generally not common practice to include throws clauses for runtime exceptions. Can you can submit a followup PR to make the changes you have in mind to clarify?

I am coming from a Guava mindset, which has a special exception for "The remote side messed up" called VerificationException. I think you don't depend on Guava, but it's a useful idea. If there isn't any recovery path from such an exception, why not just make it RuntimeException, or IORuntimeException, or some such?

Followup PR for ISE is fine. ISE is really meant for thinks like calling read() after close(). It's a programmer error.

This comment has been minimized.

@Scottmitch

Scottmitch Jul 19, 2017

Member

@carl-mastrangelo - Yip I'm not against having a special exceptions type for these types of things. Lets do it in a followup PR so it can be applied consistently and the change is isolated.

@Scottmitch

Scottmitch Jul 19, 2017

Member

@carl-mastrangelo - Yip I'm not against having a special exceptions type for these types of things. Lets do it in a followup PR so it can be applied consistently and the change is isolated.

HTTP/2 enforce HTTP message flow
Motivation:
codec-http2 currently does not strictly enforce the HTTP/1.x semantics with respect to the number of headers defined in RFC 7540 Section 8.1 [1]. We currently don't validate the number of headers nor do we validate that the trailing headers should indicate EOS.

[1] https://tools.ietf.org/html/rfc7540#section-8.1

Modifications:
- DefaultHttp2ConnectionDecoder should only allow decoding of a single headers and a single trailers
- DefaultHttp2ConnectionEncoder should only allow encoding of a single headers and optionally a single trailers

Result:
Constraints of RFC 7540 restricting the number of headers/trailers is enforced.
@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch
Member

Scottmitch commented Jul 19, 2017

4.1 (a91df58)

@Scottmitch Scottmitch closed this Jul 19, 2017

@Scottmitch Scottmitch deleted the Scottmitch:http2_http_message_flow branch Jul 19, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jul 19, 2017

Member

@ejona86 - if there are any remaining issues I can submit a followup PR.

Member

Scottmitch commented Jul 19, 2017

@ejona86 - if there are any remaining issues I can submit a followup PR.

@ejona86

This comment has been minimized.

Show comment
Hide comment
@ejona86

ejona86 Jul 20, 2017

Contributor

LGTM

Contributor

ejona86 commented Jul 20, 2017

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment