tlsconfig trace implementation

[joewilliams]

This is an implementation of TLSConfig hooks scoped just to getTLSCertificate.

cc #149

[azdagron]

Thanks for this PoC, @joewilliams. I wonder, if instead of requiring a parameter at the end of the tlsconfig.*Config() methods how this would look if we adopted the Options pattern used in other packages? Thoughts?

[joewilliams]

I wonder, if instead of requiring a parameter at the end of the tlsconfig.*Config() methods how this would look if we adopted the Options pattern used in other packages? Thoughts?

I assume that would work something like WithHooks() which would return a function containing the Hooks struct? I could see it being cumbersome needing to WithHooks any invocation of a package but maybe no more cumbersome than the struct in this PR. For something like workloadapi it might work well but for something like tlsconfig maybe less so?

That said, I don't have a strong preference here. @aybabtme what do you think?

[joewilliams]

I have integrated @aybabtme's suggestions, the important part is in

go-spiffe/v2/spiffetls/tlsconfig/config.go

Line 177 in 2415008

func getTLSCertificate(svid x509svid.Source, trace Trace) (*tls.Certificate, error) {

and

go-spiffe/v2/spiffetls/tlsconfig/trace.go

Line 8 in 2415008

type GetTLSCertificateStart struct {

[joewilliams]

@azdagron @aybabtme graciously implemented the WithOptions code for this PR. I think it's in a place for further review before we implement more calls and etc. For TLSConfig I think we'd want to add tracing for GetCertificate, VerifyPeerCertificate and AdaptMatcher to start. I don't want to start that work until there is agreement on how the API works. I think a future tlsconfig.HookMTLSServerConfig call with full tracing, when this PR is completed fully, would look something like:

// authorizerTraceConfig = stuff
// getCertTraceConfig = stuff
// verifyPeerTraceConfig = stuff

authorizerWithTracing := tlsconfig.AuthorizeMemberOf(
    tlsconfig.WithAuthorizerOption(
			tlsconfig.WithAuthorizerTrace(authorizerTraceConfig),
		)
)

tlsconfig.HookMTLSServerConfig(config, svid, bundle, authorizerWithTracing,
		tlsconfig.WithMTLSServerGetCertificateOption(
			tlsconfig.WithGetCertificateTrace(getCertTraceConfig),
		),
		tlsconfig.WithMTLSServerVerifyPeerCertificate(
			tlsconfig.WithMTLSServerVerifyPeerCertificate(verifyPeerTraceConfig),
		),
	)

[azdagron]

Awesome. I'll take a detailed look as soon as I can.

[azdagron]

My opinion is that the above approach is too granular? I don't think we need separate options for Authorizer,GetCertificate,VerifyPeerCertificate. I also don't anticipate that we'll need per-method options. I think an option type for the whole package is probably sufficient?

How does the following look?

trace := tlsconfig.Trace{
    // populate the functions for the trace points you are interested in, leave the rest zero initialized
}

tlsconfig.HookMTLSServerConfig(config, svid, bundle, tlsconfig.WithTrace(trace))

[aybabtme]

Sounds good, I'll collapse the options into 1 type.

[aybabtme]

@azdagron PTAL

[joewilliams]

👍

[azdagron]

Thanks, folks! I appreciate your patience with the back-and-forth on this.

As I reviewed the Start/End structs, I wondered if we could get away with not doing the measurement of elapsed time itself in the library by having the *Start callbacks return an interface{} that is given to the *End callback. Maybe something like:

type Trace struct {
    GetCertificate() interface{}
    GotCertificate(interface{}, GotCertificateInfo)

An implementation could be:

trace.GetCertificate = func() interface{} {
    // ... log or do whatever with your metrics to start recording elapsed time ...
    return time.Now()
}
trace.GotCertificate = func(value interface{}, info GotCertificateInfo) {
   elapsed := time.Since(value.(*time.Time))
   // ... do something with `elapsed` and `info` ...
}

I'm not even sure how I feel about the above to be honest, but it came to mind.
Pros:

• it keeps all assumptions about how the trace is going to be used out of the library, providing only the barebones, but allowing for trace implementors to provide context between start/stop events when needed.

Cons:

• type assertions, woof!
• increases the boilerplate on trace implementations

Thoughts??

[azdagron]

We should probably use the same pattern we have for other options:

Suggested change

	type Option func(*options)
	type Option interface {
	apply(*options)
	}
	type option func(*options)
	func (fn option) apply(o *option) {
	fn(o)
	}

Then the options can be implemented in terms of option we get the flexibility later if we do end up needing to have per-method options mixed in with common options.

func WithSomeOption() Option {
    return option(func(o *options) {
        // modify `o`
    })
}

[aybabtme]

Makes sense, I'd never seen what the purpose of these interface-based options were, so always used the simpler func-based pattern. I see now that this makes it possible to reuse options and still have scoped down option types for narrower use-cases, which obviates the need for the N option types I had included in this PR initially!

Nice! The more you know!

[azdagron]

These fields aren't used.

[aybabtme]

Oups! Bad copy-pasta!

[azdagron]

Hmm, where did this come from? What is pulling in go-spiffe v1?

[aybabtme]

it's a

[aybabtme]

not sure where this came from but it's cleaned up now

[aybabtme]

I like your idea, I was thinking of something similar initially using context.Context but there no instance of it anywhere in scope here. The advantage of context.Context is we'd be able to use a type-safe key argument, which would allow nested Trace implementations to be invoked without as much risk of mangling the type that the receiving GotCertificate sees, sort of how http.Handler middleware get stacked up. Seems unlikely to be needed here though, so interface{} lgtm.

[aybabtme]

@azdagron PTAL

[amartinezfayo]

This is a great improvement. Thank you for this contribution!
I have some minor suggestions around the testing. I think that it would also be good to update some of our example code to show how this capability can be leveraged (not necessarily as part of this PR).

[azdagron]

Just a few small changes so the interface is future proof (i.e. some functions in tlsconfig don't take options). I think we're good to go after that.

[aybabtme]

@azdagron thanks for the quick review! I addressed your comments, PTAL.

[azdagron]

\o/! Would you mind squashing this? (to preserve DCO). I'm happy to merge after that.

[joewilliams]

@azdagron things are green, should be good to go.

[azdagron]

\o/ thanks folks! I'll have a follow-up PR with some additional events.