TLV capture and parsing for proxy protocol v2

[stu-elastic]

• Adds rawTLVs to Header struct
• API Change
  • non-io.EOF errors encountered when reading from payload reader are returned
    from parseVersion2 and Read
  • Padding is saved in rawTLV
• TLV splitting is implemented by Header.TLVs(), there is no TLV length validation without
  this explicit call
• Includes helpers for parsing AWS VPCEs and SSL information
• Parsing of other types possible by consumers via the PP2Type constants using
  the TLV type
• Adds PP2SSL exported type with exported fields and helpers for parsing the SSL TLV type.

[Freeaqingme]

@stu-elastic I'm curious how you are expecting this functionality to be used. Perhaps you could add a little documentation? The only way I'm able to access the tlv stuff is by doing:

func handleConn(conn net.Conn) {
	defer conn.Close()

	hdr, err := proxyproto.Read(bufio.NewReader(conn))

That seems a little error prone, given that if someone already read a single byte from conn the header can no longer be accessed. I think it would be more neat if we could do:

	hdr := conn.(*proxyproto.Conn).Header()

The header itself is already stored in Conn when the first byte is read so that doesn't require any changes.

[Freeaqingme]

I was looking in the proxy protocol spec for (a reference to) a list of registered extension types, expecting to find the AWS extension type there. It appears it's not a registered type (I couldn't find any), but it simply uses the 'application-specific' range.

That means that, potentially, someone else can also use the 0xEA extension for completely different purposes.

This makes me wonder if we should intertwine the AWS stuff in tlv.go, or if it'd be better if we fleshed out some kind of plugin system where people could also register their own extension types. Thoughts?

[stu-elastic]

I'm curious how you are expecting this functionality to be used.

Once you have the header, grab the TLVs, then parse what you want out of the TLVs, perhaps using a helper.

tlvs, err := header.TLVs()
if err != nil {
  return "", err
}
vpceid, ok := proxyproto.AWSVPECID(tlvs)
if !ok {
  return "", errors.New("No VPCE ID")
}
return vpceid, nil

I'll add some examples to the godoc as we progress in the review.

I think it would be more neat if we could do:

	hdr := conn.(*proxyproto.Conn).Header()

That's nice and clean, however it's out of scope for this change which is focused on parsing TLVs once the header is acquired.

That means that, potentially, someone else can also use the 0xEA extension for completely different purposes.

Agreed.

This makes me wonder if we should intertwine the AWS stuff in tlv.go

Good point. What do you think about moving the TLV receiver in AWSVPCEType and AWSVPCEID to a parameter? We could restructure SSL parsing simillarly since both rely on exported fields.

if it'd be better if we fleshed out some kind of plugin system where people could also register their own extension types. Thoughts?

Interesting thought. I'd assumed folks would take the TLV struct and perform their own parsing.
We could expose splitTLVs to give them an additional primitive to do the parsing.

The form I followed for parsing TLVs, is

1. Check TLV.Type and TLV.Length. See AWSVPCEType and SSLType.
2. Then parse the internal structure.

Off the top of my head, I can't think of a good way to structure a plugin system that is idiomatic while avoiding a bunch of casting, since we don't know the result type for an arbitrary plugin.

I like the idea of exporting splitTLVs and taking a TLV as a parameter, rather than as a receiver for the parsers. We could also move the parsers into their own file. What do you think?

[pires]

I will take the weekend to look into this.

[pires]

I'll add some examples to the godoc as we progress in the review.

It's been a long-standing request to provide examples of the usage of this library and I've been thinking the best option would be to add some example tests. WDYT?

That means that, potentially, someone else can also use the 0xEA extension for completely different purposes.

This is my understanding as well.

if it'd be better if we fleshed out some kind of plugin system where people could also register their own extension types.

I can't think of a good way to structure a plugin system that is idiomatic while avoiding a bunch of casting, since we don't know the result type for an arbitrary plugin.

I agree. However, I don't think this library should embrace non-standardized extensions, such as the AWS implementation proposed in this PR, mostly because no guarantees can be made that multiple extensions won't make the decision to use the same identifier.

I like the idea of exporting splitTLVs and taking a TLV as a parameter, rather than as a receiver for the parsers. We could also move the parsers into their own file. What do you think?

I like this idea. What if the parsers could even live in a separate repo? We could have something like go-roxyproto-aws? Do you think this is too much?

[stu-elastic]

I've merged from master and changed SplitTLVs to an exported function as well as separated out the TLV parsing into a subpackage, tlvparse.

The AWS VPC Endpoint ID extension is implemented as an application extension within the spec.

The functionality is 43 lines. Having it as an external repo would be a lot of overhead, especially if we want to keep the functionality around after doing any refactoring.

The refactoring takes parsed TLVs as parameters. eg.

tlvs, err := hdr.TLVs()
if err != nil {
  return "", err
}
if tlvparse.IsAWSVPCEndpointID(tlvs[0]) {
  return tlvparse.AWSVPCEndpointID(tlvs[0])
}
return tlvparse.FindAWSVPCEndpointID(tlvs), nil

This separates the parsing of AWS VPC Endpoints from the core package.

Even though SSL parsing is within spec, I've refactored that similiarly. See func SSL(t proxyproto.TLV) (PP2SSL, error), func IsSSL(t proxyproto.TLV) bool and func FindSSL(tlvs []proxyproto.TLV) (PP2SSL, bool).

[pires]

I'm assuming you tested this in some scenario on AWS, right? I have no way to validate it atm. Other than that, LGTM.
@Freeaqingme will you have time to take a look some time soon, please?

[stu-elastic]

I'm assuming you tested this in some scenario on AWS, right?

Yes, we are using the initial version, not my refactor from yesterday, in production.

[pires]

Thank you very much @stu-elastic for this contribution. I'm merging 🎉