Skip to content
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

Deprecate disabling HTTP header validation #13609

Merged
merged 6 commits into from
Nov 28, 2023

Conversation

chrisvest
Copy link
Contributor

Motivation:
HTTP request/response splitting is a serious vulnerability, which is mitigated by enabling HTTP header validation. We already do header validation by default, but we have many constructors in our HTTP codec that permit turning it off. We should discourage all our down-stream users from turning header validation off.

Modification:

  • Deprecate all constructors in our HTTP codec, that allow header validation to be turned off.
  • Offer alternative constructors to all the newly deprecated ones, where header validation is enabled.
  • Trailer validation in DefaultLastHttpContent is now enabled by default in the DefaultLastHttpContent(ByteBuf, HttpHeaders) constructor (Behavioral change!)
  • Add better constructors, and more guidance in the javadocs, for DefaultHttpHeaders for those who want to use custom header validators.

Result:
We hopefully encourage integrators to validate headers. And we hopefully get fewer issues reported about disabled header validation when people run security scanners on their code.

@franz1981
Copy link
Contributor

@vietj too
Any concern?

@slandelle
Copy link
Contributor

My sole concern is not the deprecation but if there's a plan for removal at some point. Sometimes, one side, typically the client one, has zero control over the other side and if it's buggy, it has to live with.

@chrisvest
Copy link
Contributor Author

No plans to remove the ability to opt out. I think we'll always need a way to control that aspect. Would be nice to have fewer constructors, though. Maybe we can use s builder pattern. Either here or Netty 5 only.

@chrisvest
Copy link
Contributor Author

I'm exploring the builder idea to see if the number of constructors can be cut down a bit.

@chrisvest
Copy link
Contributor Author

@idelpivnitskiy @slandelle With a combination of factories, builders, and configs, I think we now have a much nicer API for both guiding people toward better defaults, and offering all the knobs advanced users might need. Please take a look.

@chrisvest
Copy link
Contributor Author

@slandelle @normanmaurer @idelpivnitskiy Rebased this. Are we happy with it?

Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely love refactoring of all request/response and encoder/decoder types. Great work, Chris!

It took me sometime to wrap my head around factory-builder-factory though 😄
See my comments below:

/**
* A configuration object for specifying the behaviour of {@link HttpObjectDecoder} and its subclasses.
* <p>
* The {@link HttpDecoderConfig} objects are mutable to reduce allocation,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutability also has problems. WDYT about introducing HttpDecoderConfig interface and HttpDecoderConfigBuilder as a final class? Builder will be a good compromise between memory allocations and providing an immutable config by default. For cases when users need runtime mutability, they can implement interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructors that take HttpDecoderConfig objects always extract all fields, so I think it's okay.

* The {@link HttpDecoderConfig} objects are mutable to reduce allocation,
* but also {@link Cloneable} in case a defensive copy is needed.
*/
public final class HttpDecoderConfig implements Cloneable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, there was a recommendation to use a copy-ctor pattern vs implementing a Clineable interface somewhere, maybe in Effective Java?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the argument for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this article from 2000 is the latest word on that, but since the field types are not expected to have interior mutability, I think it's fine here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the one.
It's like a question when to use Atomic* vs Atomic*Updater. Sometimes it's easier to stick to a recommended pattern instead of considering each case independently. Up to you what approach you would like to keep.

Motivation:
HTTP request/response splitting is a serious vulnerability, which is mitigated by enabling HTTP header validation.
We already do header validation by default, but we have many constructors in our HTTP codec that permit turning it off.
We should discourage all our down-stream users from turning header validation off.

Modification:
- Add an HttpHeadersFactory interface, implemented by an HttpHeadersBuilder, for specifying how headers should be constructed.
- Add an HttpDecoderConfig for encapsulating all the HttpObjectDecoder parameters.
- Deprecate all constructors in our HTTP codec, that allow header validation to be turned off.
- Offer alternative constructors to all the newly deprecated ones, that take HttpHeadersFactory.

Result:
We hopefully encourage integrators to validate headers.
And we hopefully get fewer issues reported about disabled header validation when people run security scanners on their code.
This also avoids a future compatibility headache if we were to ever add more fields to it, since that would imply adding more parameters to the private `with` method.
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning the factory/builder!

* The {@link HttpDecoderConfig} objects are mutable to reduce allocation,
* but also {@link Cloneable} in case a defensive copy is needed.
*/
public final class HttpDecoderConfig implements Cloneable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the one.
It's like a question when to use Atomic* vs Atomic*Updater. Sometimes it's easier to stick to a recommended pattern instead of considering each case independently. Up to you what approach you would like to keep.

@chrisvest
Copy link
Contributor Author

@idelpivnitskiy Addressed all your comments

Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@chrisvest chrisvest merged commit 84a13a9 into netty:4.1 Nov 28, 2023
14 checks passed
@chrisvest chrisvest deleted the 4.1-http-hdr-value-validation branch November 28, 2023 17:58
@chrisvest chrisvest added this to the 4.1.102.Final milestone Nov 28, 2023
@chrisvest
Copy link
Contributor Author

I'll make a separate PR for Netty 5 since this is too big to just forward-merge.

chrisvest added a commit to chrisvest/netty that referenced this pull request Dec 1, 2023
This is a forward port of netty#13609, aka 84a13a9.

Motivation:
HTTP request/response splitting is a serious vulnerability, which is mitigated by enabling HTTP header validation.
We already do header validation by default, but we have many constructors in our HTTP codec that permit turning it off.
We should discourage all our down-stream users from turning header validation off.

Modification:
- Remove many constructors in our HTTP codec, that allow header validation to be turned off.
- Offer alternative constructors to all the newly removed ones, where header validation is enabled.
- Offer alternative constructors, that take HttpDecoderConfig or HttpHeadersFactory instances.
- Trailer validation in `DefaultLastHttpContent` is now enabled by default in the `DefaultLastHttpContent(Buffer, HttpHeaders)`
- Add more guidance in the javadocs.

Result:
We hopefully encourage integrators to validate headers. And we hopefully get fewer issues reported about disabled header validation when people run security scanners on their code.
chrisvest added a commit that referenced this pull request Dec 4, 2023
This is a forward port of #13609, aka
84a13a9.

Motivation:
HTTP request/response splitting is a serious vulnerability, which is
mitigated by enabling HTTP header validation. We already do header
validation by default, but we have many constructors in our HTTP codec
that permit turning it off. We should discourage all our down-stream
users from turning header validation off.

Modification:
- Remove many constructors in our HTTP codec, that allow header
validation to be turned off.
- Offer alternative constructors to all the newly removed ones, where
header validation is enabled.
- Offer alternative constructors, that take HttpDecoderConfig or
HttpHeadersFactory instances.
- Trailer validation in `DefaultLastHttpContent` is now enabled by
default in the `DefaultLastHttpContent(Buffer, HttpHeaders)`
- Add more guidance in the javadocs.

Result:
We hopefully encourage integrators to validate headers. And we hopefully
get fewer issues reported about disabled header validation when people
run security scanners on their code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants