Heartbeat TLS metadata

[andrewvc]

WIP Work toward adding extra TLS metadata. Would resolve #6300 and #3741 once complete.

The PR is based off of #7784 , hence the large diff. We should wait for that to merge before reviewing this further.

I'm submitting this PR early to get feedback from @urso on this approach to injecting the TLS metadata into the dialchain. The code is very rough, but I wanted to get feedback early before I start adding tests and rounding this out.

Another goal of this PR would be to add tests to the namespaces and types this PR touches. Heartbeat needs more tests and the strategy here is to add a feature, and retroactively add tests alongside it.

One thing we might also want to consider is that there is shared code for the dialchain in libbeat/outputs/transport that probably should just move up a level to libbeat/transport.

[houndci-bot]

exported function TestTLSMetaDialer should have comment or be unexported

[houndci-bot]

exported function TLSMetaDialer should have comment or be unexported

[houndci-bot]

exported method DialerMeta.SetTls should have comment or be unexported
method SetTls should be SetTLS

[houndci-bot]

struct field Tls should be TLS

[houndci-bot]

exported type DialerMeta should have comment or be unexported

[andrewvc]

This is only a subset of the fields for now, but I would expand this to the full field list.

[andrewkroh]

This seems like it would be a good opportunity to reuse the code from Packetbeat for turning this data into an event. This would help ensure that we are consistent with field names.

https://github.com/elastic/beats/blob/35a7882f69d6daf9ab72f36c6509a00aaa2904d0/packetbeat/protos/tls/parse.go#L546:6

[ruflin]

Also take into account what @ph contributed to ECS: https://github.com/elastic/ecs#tls Perhaps we can extend this further.

[houndci-bot]

exported function CertToMap should have comment or be unexported

[houndci-bot]

exported function TLSChecks should have comment or be unexported

[houndci-bot]

func parameter testUrl should be testURL

[houndci-bot]

comment on exported function TestTLSMetaDialer should be of the form "TestTLSMetaDialer ..."

[houndci-bot]

comment on exported function TLSDialer should be of the form "TLSDialer ..."

[houndci-bot]

struct field Tls should be TLS

[andrewvc]

This is much improved, but now needs some modifications to work with ECS. We have much richer data than the few fields ECS asks for, but I'll examine this in more detail tomorrow.

[urso]

This panics if rv is no map. You can add a guard by checking rv.Kind() == reflect.Map.

[urso]

panics if o is no array or slice or map.

[urso]

panic if o is no array or slice.

[urso]

We really need all the wiring up? There is quite some indirection here, I'd rather keep out if possible. The conn connection object should be a tls connection (as is being produced by TLSDialer). We should read the connection state from the conn parameter if possible.

[andrewvc]

Hmmm, what exactly are you thinking here? tls.Conn in crypto/tls/conn.go is not a super-type (is that the go lingo?) of net.Conn, so cannot be cast. It simply nests a net.Conn as a property.

AFAICT this means we have to use this sort of indirection. Would love a simpler alternative though. Do I misunderstand?

[urso]

tls.Conn wraps another connection, because TLS sits on top of another network layer. But tls.Conn implements net.Conn itself. In go we do not have inheritance or subtype relationships. But tls.Conn structurally satisfies the net.Conn interface. When building layers of dialers, we have to fix the return values of dialers, due to lack of generics and sub-typing (no co-/contra-variance on methods). The common denominator for dialers is given by:

type Dialer interface {
	Dial(network, address string) (net.Conn, error)
}

As net.Conn is an interface, each layer will wrap it's input layer + use a concrete type which is finally promoted to the interface value being returned.

This is reflected in the DialerChain type:

type DialerChain struct {
	Net    NetDialer
	Layers []Layer
}

The NetDialer produces the most low-level connection object. Each layer will wrap the current connection object, returning a new connection object.

As we want to do some measurements before and after a network layer becomes active, I introduced the beforeDial and afterDial wrappers, so to reduce common boilerplate and amount of wrapper functions needed. Again, lack of generics and sub-type relationships we do enforce the use of net.Conn. But in general, once a dialer-chain is setup, we have this layout:

+----+-----------------------+----+ net.Conn
     |  After Layer II       |
     +-----------------------+ Layer II Conn type
     |  Dial Layer II        |
     +-----------------------+
     |  Before Layer II      |
+---------------------------------+  net.Conn
     |  After Layer I        |
     +-----------------------+ Layer I Conn type
     |  Dial Layer I         |
     +-----------------------+
     |  Before Layer I       |
+---------------------------------+ net.Conn
     |  Net Layer (e.g. TCP) |
     +-----------------------+

A Layer must make no assumptions about the underlying layer. A layer must assume the connection object to wrap to be of type net.Conn. But within a layer (dial + after steps), we know the concrete type returned by the dialer and to be consumed by the 'after'-handler. Due to limitations of the type-system, we are forced to cast the interface value to the concrete type, though.

In case of the TLS layer, we use transport.TLSDialer. The concrete type returned by (*TLSDialer).Dial is *tls.Conn. => we can cast the interface value to *tls.Conn in the 'after'-handler. As we know the concrete type in the 'after'-handler, we don't need another behind-the-scenes type capturing the concrete type. Admittedly, it's more type safe, but it's somewhat more involved and somewhat over-engineered for something that should be simple (as the after-handler is the only consumer that requires to access the tls state).

[andrewvc]

Thanks for the very detailed writeup. I'm realizing that my confusion was in typing conn.(tls.Conn) instead of conn.(*tls.Conn). That was the root of my assumption that the interface wasn't implemented.

Thanks for the detailed diagram of the dialer stack, that's very helpful!

[ruflin]

We should move that diagram to the code docs somewhere.

[andrewvc]

I'm pausing this a bit because we need better ECS schema definitions for TLS. I've opened up a GH issue to start the discussion here: elastic/ecs#64

[ruflin]

I'm a bit concerned that certificates here is going to be a nested object in Elasticsearch it seems.

What are the expected queries the user will run against these fields?

[andrewvc]

Added a bunch of specs for mapval.Path since code coverage checks were failing.

I added a little to this file, so it's fair to add more coverage in this PR IMHO.

[andrewvc]

OK, should be ready for final review.

[ruflin]

as usual goimports messes with you :-)

[ruflin]

I'm a bit concerned about this import. Beats should not have dependencies with each other. Share code like this be moved into libbeat.

[ruflin]

Do we really want to panic here or rather return / log a critical error?

[urso]

Lets keep the panic here. If this fails it's definitely a programming error. Given we have TLS tests, this panic should only occur during testing.
If we hide this behind an error message being logged, we might also have test failures (or a bug at worst), but then we will have to search for the root cause. Being a little more offensive can help.

[ruflin]

Would be nice to have a code comment here that states we check all certs and take the oldest / newest timestamp. for before and after and not sure from the last certificate in the chain.

[ruflin]

See comment below about packetbeat dependency.

[ruflin]

goimports messing with you.

[ruflin]

Let's use an error here instead of a panic. What if someone uses this for production code and is not aware this can apnice?

[andrewvc]

Well, this practically cannot panic. The only way that would happen is if someone was messing deep in the guts of mapval and added a new type without overriding this.

I would say then, that if this did panic, it'd be a bug in mapval itself, not in user-facing code. Checking invariants like that seems like the right place for panic no?

[andrewvc]

I'm realizing this is similar to the other panic described earlier. Any case where another type is encountered would definitely be a programming error. You would need to create a new PathComponentType with a different (manual) value.

@urso I see you +1'd this change, but opposed removing the other panic. Maybe you can help me understand the difference?

[urso]

Assuming mapval package could be used in other places in beats, the mapval package might be used to validate any kind of external input. We might even consider users to configure validation (based on mapval) in the heartbeat config file. When working with external input, mapval should not panic.

If the panic can only indicate programming errors, I'm fine with the panic. If the panic can be triggered by external (yet unchecked) input then we should go with an error. mapval has to check input, so we must assume any kind of invalid input.

It's common in String methods to not create a panic, but return an 'Unknown'-string.

Btw. consider using stringer like:

//go:generate stringer -type=PathComponentType -linecomment=true

const (
	PCMapKey PathComponentType = 1 + iota // map

	// PCSliceIdx is the Type for slice indices.
	PCSliceIdx                            // slice
)

Notice how I set PCMapKey to 1. That is, the zero-value is invalid. Oftentimes one defines an 'invalid'-const to be 0 like:

//go:generate stringer -type=PathComponentType -linecomment=true

const (
    PCMapInvalid PathComponentType = iota // invalid
	PCMapKey                              // map
	PCSliceIdx                            // slice
)

[andrewvc]

Thanks for the feedback, I'll take your advice and return an unknown string. I think that's a great compromise.

Also, thanks for the tip on the 1+, makes a lot of sense.

[ruflin]

Better move this libbeat.

[andrewvc]

Cool, will move that whole package.

[urso]

As you are already a heavy user of reflection, how about this:

var equalChecks = map[reflect.Type]reflect.Value

// accepts and register equal function for function signatures:
// func(a, b T) bool
func RegisterEqual(fn interface{}) {
  v := reflect.ValueOf(fn)
  t := v.Type()
  if t.Kind() != reflect.Function { ... }
  if t.NumIn() != 2 { ...}
  if t.NumOut() != 1 { ... }
  if t.In(0) != t.In(1) { ... }
  if t.Out(0) != <bool> { ... }

  equalChecks[t.In(0)] = v
}

func IsEqual(to interface{}) IsDef {
  vTo := reflect.ValueOf(to)
  return Is("equals", func(path Path, in interface{}) *Results {
    vIn := reflect.ValueOf(in)
    if vTo.Type() != vIn.Type() { ... }

    // we always query, in case someone did add a comparison function after
    checker := equalChecks(vTo.Type())
    if checker == nil {
      // use DeepEqual
    }

    equal := checker.Call([]reflect.Value{vTo, VIn}).Bool()
    if !equal {
      ...
    }
  }
}

[andrewvc]

This is pretty nifty, out of curiosity I wonder if using reflect is faster than a type switch? I assume they must be pretty similar perf-wise under the covers?

I am however worried about YAGNI. How many special equals cases are there? reflect.DeepEquals works pretty well, this is the first case it hasn't.

If there are only a few, I personally prefer the switch because its very concise, however, if we can foresee adding 10 of these cases a map sounds better.

WDYT?

[urso]

reflection is somewhat smaller.

In go an interface{} value has the internal representation:

type ifcType struct {
  typ *typeDescr
  value *unsafe.Pointer
}

The reflection package defines Value to be:

type Value struct {
  typ *typeDescr
  value *unsafe.Pointer
  flags flags
}

when using a type-switch, the generated code checks if the 'typ' matches and interprets the value pointer as your actual type. If your target type is also a pointer, its a comparison + copy pointer into new variable.

When using reflection, each call to any Value method checks flags, types and so on, checking if your call is actually valid. The Value.Call method is super involved. Reflection is definitely slower, but gives you some flexibility here.

reflect.DeepEquals has it's limits. It compares each field (including private fields) in a struct. Private fields might be different, while 2 objects might still compare equal. E.g. when comparing regular expressions. DeepEquals compares public and private fields. But the regex package introduced an object pool pointer in tip. Each regexp.Regex type holds a unique pointer => comparison fails, if one does not compare the same pointer, even if two regular expression are generated from the same string.

[andrewvc]

Cool, thanks for the detailed walkthrough. I think you're right, we'll probably add more of these, I'll add the registry.

[urso]

do not use underscores for variable names or function names. Event tests.

[andrewvc]

Ah, that was generated by gotests, so I assumed it was idiomatic. Will change.

[andrewvc]

After a discussion on slack, we decided this was fine since the linter is fine. https://go-review.googlesource.com/c/go/+/86001 seems to have added support for this.

[houndci-bot]

exported function IsDeepEqual should have comment or be unexported

[houndci-bot]

exported function IsDeepEqual should have comment or be unexported

[andrewvc]

@urso I've incorporated both of your suggestions,

for using reflection in equality in c274e0f and for path panic-ing in 5a0a42e

[ruflin]

@andrewvc Could you update the PR message to be the commit message or squash the commits into 1 so it can be merged?

[andrewvc]

Squashed