Usability of tlsconfig.MTLSClientConfig and tlsconfig.MTLSServerConfig

[edwarnicke]

I just spent the better part of half a day stuck with a mysterious inscrutable error from using MTLSServerConfig when I should have used MTLSClientConfig due to copypasta.

That's 100% on me.

I know you guys care a lot about usability, so I thought I'd open a discussion about this being a possible usability problem.

It would be good if either:

1. There was a tls.Config that was usable for both Server and Client side
   or
2. The methods were provided for both sides, with the unimplemented side returning a very clear error that is known to bubble up with used with grpc.

[azdagron]

Thanks for opening this, @edwarnicke! I'm sorry you got bit here!

One thing to note is that creating a config that works for both server and client is only practical for MTLS since both client and server need the same things (i.e., svid and bundle source with an authorizer). The same cannot be said for the TLS or WebMTLS family of functions.

We chose consistency in this case, but I can see your point. Ultimately I'm open to unifying the MTLS family of functions in lieu of consistency if there are no wide objections but am a little hesitant since it wouldn't solve the usability problem for the other family of functions.

That being said, your second suggestion might be promising. However, from what I can tell, it might only work definitively when using a "client" config in the server role (by setting the GetCertificate callback to something that fails) but not when using a "server" config in the client role, as the seemingly only available callback to inject the failure is GetClientCertificate which will not be invoked if the server side does not request a client certificate. Another option is to implement the ClientSessionCache interface with a bogus implementation that panics but that feels very gross and unideal.

Curious to hear others thoughts....

[edwarnicke]

Yeah... in fairness, I blame myself, not the go-spiffe lib for getting bit... I raised this because I expect others will get bitten too. It's a deeply cryptic problem to debug if you don't happen to notice by inspection the issue though... so minimally something to make the error more obvious at runtime would be extremely helpful.