tlsconfig trace implementation #150

joewilliams · 2020-07-28T23:04:05Z

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

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?

v2/spiffetls/tlsconfig/config.go

v2/spiffetls/tlsconfig/hooks.go

joewilliams · 2020-08-03T22:31:26Z

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?

v2/spiffetls/tlsconfig/hooks.go

joewilliams · 2020-08-04T19:32:33Z

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 · 2020-08-04T21:58:37Z

@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 · 2020-08-04T22:04:11Z

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

azdagron · 2020-08-07T15:15:58Z

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 · 2020-08-07T16:56:55Z

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

aybabtme · 2020-08-07T17:31:44Z

@azdagron PTAL

joewilliams · 2020-08-07T17:35:25Z

👍

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 · 2020-08-07T18:37:54Z

v2/spiffetls/tlsconfig/config.go

@@ -26,63 +27,86 @@ func HookTLSClientConfig(config *tls.Config, bundle x509bundle.Source, authorize
 	config.VerifyPeerCertificate = WrapVerifyPeerCertificate(config.VerifyPeerCertificate, bundle, authorizer)
 }

+// A Option changes the defaults used to by mTLS ClientConfig functions.
+type Option func(*options)


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` }) }

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 · 2020-08-07T18:46:31Z

v2/spiffetls/tlsconfig/trace.go

+	End   time.Time
+	Cert  *tls.Certificate
+	Err   error


These fields aren't used.

Oups! Bad copy-pasta!

azdagron · 2020-08-07T18:46:51Z

v2/go.sum

@@ -17,6 +17,7 @@ github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5a
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA=
+github.com/spiffe/go-spiffe v1.0.0 h1:zP+DNibBYpALBw597lwikzhriUhMqSUU3zCjBAD+BFw=


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

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

aybabtme · 2020-08-07T19:18:25Z

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 · 2020-08-07T19:34:40Z

@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).

v2/spiffetls/tlsconfig/config_test.go

v2/spiffetls/tlsconfig/trace.go

v2/spiffetls/tlsconfig/config_test.go

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.

v2/spiffetls/tlsconfig/config.go

v2/spiffetls/dial.go

v2/spiffetls/listen.go

v2/spiffetls/option.go

aybabtme · 2020-09-15T12:48:50Z

@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.

Signed-off-by: Joe Williams <[email protected]> add time to function call typo forgotten imports aybabtme's suggestions function name change clean up use two structs Signed-off-by: Antoine Grondin <[email protected]> introduce func. opts. pattern and convert GetCertificate tracing to it Signed-off-by: Antoine Grondin <[email protected]> adjust trickle down occurences of tlsconfig.Trace Signed-off-by: Antoine Grondin <[email protected]> collapse all option types into 1 for whole tlsconfig package Signed-off-by: Antoine Grondin <[email protected]> rework option type and trace mechanism Signed-off-by: Antoine Grondin <[email protected]> remove weird go-sum addition Signed-off-by: Antoine Grondin <[email protected]> test clean up Signed-off-by: Antoine Grondin <[email protected]> Update v2 to beta Signed-off-by: Andrew Harding <[email protected]> Signed-off-by: Antoine Grondin <[email protected]> Print out the expected peer and domains when encountering mismatches Signed-off-by: Kyle Anderson <[email protected]> Signed-off-by: Antoine Grondin <[email protected]> assert that GetCertificate gets called as expected Signed-off-by: Antoine Grondin <[email protected]> pass config.Option to every func Signed-off-by: Antoine Grondin <[email protected]> pluralize options funcs Signed-off-by: Antoine Grondin <[email protected]>

joewilliams · 2020-09-15T23:44:37Z

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

azdagron · 2020-09-17T10:58:27Z

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

joewilliams requested review from amartinezfayo and evan2645 as code owners July 28, 2020 23:04

joewilliams mentioned this pull request Jul 28, 2020

generic metric hooks #149

Open

azdagron reviewed Aug 3, 2020

View reviewed changes

v2/spiffetls/tlsconfig/config.go Outdated Show resolved Hide resolved

v2/spiffetls/tlsconfig/hooks.go Outdated Show resolved Hide resolved

aybabtme reviewed Aug 4, 2020

View reviewed changes

v2/spiffetls/tlsconfig/hooks.go Outdated Show resolved Hide resolved

aybabtme force-pushed the metrics branch from 7a309c7 to fdb921e Compare August 4, 2020 21:02

joewilliams force-pushed the metrics branch from fdb921e to 1ac9bf1 Compare August 4, 2020 21:26

joewilliams changed the title ~~naive tlsconfig hooks implementation~~ tlsconfig trace implementation Aug 4, 2020

azdagron reviewed Aug 7, 2020

View reviewed changes

amartinezfayo reviewed Aug 9, 2020

View reviewed changes

v2/spiffetls/tlsconfig/config_test.go Outdated Show resolved Hide resolved

v2/spiffetls/tlsconfig/trace.go Show resolved Hide resolved

v2/spiffetls/tlsconfig/config_test.go Outdated Show resolved Hide resolved

v2/spiffetls/tlsconfig/config_test.go Outdated Show resolved Hide resolved

aybabtme force-pushed the metrics branch from d802b56 to c6b9a8f Compare September 14, 2020 20:33

azdagron reviewed Sep 14, 2020

View reviewed changes

v2/spiffetls/tlsconfig/config.go Show resolved Hide resolved

v2/spiffetls/dial.go Show resolved Hide resolved

v2/spiffetls/listen.go Show resolved Hide resolved

v2/spiffetls/option.go Outdated Show resolved Hide resolved

v2/spiffetls/option.go Outdated Show resolved Hide resolved

aybabtme force-pushed the metrics branch from a808759 to 2ec467b Compare September 15, 2020 12:42

azdagron approved these changes Sep 15, 2020

View reviewed changes

joewilliams force-pushed the metrics branch from 2ec467b to 0ca6ea6 Compare September 15, 2020 17:53

azdagron merged commit 5bab594 into spiffe:master Sep 17, 2020

aybabtme deleted the metrics branch February 15, 2021 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tlsconfig trace implementation #150

tlsconfig trace implementation #150

joewilliams commented Jul 28, 2020 •

edited

Loading

azdagron left a comment

joewilliams commented Aug 3, 2020

joewilliams commented Aug 4, 2020 •

edited

Loading

joewilliams commented Aug 4, 2020

azdagron commented Aug 4, 2020

azdagron commented Aug 7, 2020

aybabtme commented Aug 7, 2020 via email •

edited

Loading

aybabtme commented Aug 7, 2020

joewilliams commented Aug 7, 2020

azdagron left a comment

azdagron Aug 7, 2020

aybabtme Aug 7, 2020

azdagron Aug 7, 2020

aybabtme Aug 7, 2020 •

edited

Loading

azdagron Aug 7, 2020

aybabtme Aug 7, 2020

aybabtme Aug 7, 2020

aybabtme commented Aug 7, 2020 •

edited

Loading

aybabtme commented Aug 7, 2020

amartinezfayo left a comment

azdagron left a comment

aybabtme commented Sep 15, 2020

azdagron left a comment

joewilliams commented Sep 15, 2020

azdagron commented Sep 17, 2020

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

tlsconfig trace implementation #150

tlsconfig trace implementation #150

Conversation

joewilliams commented Jul 28, 2020 • edited Loading

azdagron left a comment

Choose a reason for hiding this comment

joewilliams commented Aug 3, 2020

joewilliams commented Aug 4, 2020 • edited Loading

joewilliams commented Aug 4, 2020

azdagron commented Aug 4, 2020

azdagron commented Aug 7, 2020

aybabtme commented Aug 7, 2020 via email • edited Loading

aybabtme commented Aug 7, 2020

joewilliams commented Aug 7, 2020

azdagron left a comment

Choose a reason for hiding this comment

azdagron Aug 7, 2020

Choose a reason for hiding this comment

aybabtme Aug 7, 2020

Choose a reason for hiding this comment

azdagron Aug 7, 2020

Choose a reason for hiding this comment

aybabtme Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

azdagron Aug 7, 2020

Choose a reason for hiding this comment

aybabtme Aug 7, 2020

Choose a reason for hiding this comment

aybabtme Aug 7, 2020

Choose a reason for hiding this comment

aybabtme commented Aug 7, 2020 • edited Loading

aybabtme commented Aug 7, 2020

amartinezfayo left a comment

Choose a reason for hiding this comment

azdagron left a comment

Choose a reason for hiding this comment

aybabtme commented Sep 15, 2020

azdagron left a comment

Choose a reason for hiding this comment

joewilliams commented Sep 15, 2020

azdagron commented Sep 17, 2020

joewilliams commented Jul 28, 2020 •

edited

Loading

joewilliams commented Aug 4, 2020 •

edited

Loading

aybabtme commented Aug 7, 2020 via email •

edited

Loading

aybabtme Aug 7, 2020 •

edited

Loading

aybabtme commented Aug 7, 2020 •

edited

Loading