SPIFFE Workload API support #238
Conversation
|
An open question is whether |
|
Hi @azdagron, thank you for the pull request! This looks good at first glance, I'll have to give it a more in-depth reading when I get around to it. In the meantime, could you provide some examples on how to use this? I'd like to write an integration test to make sure we don't accidentally break with new changes. |
|
Sure! Would you like those examples checked into the repository (and if so, where)? |
|
And how much detail do you need? SPIRE server/agent configuration, etc? I have a whole demo folder I can dump somewhere if you'd like. |
|
Ideally I'd like to have a page for this in the |
|
Demo folder sounds good, that's something we could add in the |
|
Awesome, I'll get right on that. |
|
Sorry for all the commit churn. There is now a demo to use as a reference. |
|
|
||
| func (c *spiffeTLSConfig) GetClientConfig() *tls.Config { | ||
| config := c.base.Clone() | ||
| config.InsecureSkipVerify = true |
There was a problem hiding this comment.
Add a comment explaining why this is safe?
| func (c *spiffeTLSConfig) GetServerConfig() *tls.Config { | ||
| config := c.base.Clone() | ||
| config.ClientAuth = tls.RequireAnyClientCert | ||
| config.InsecureSkipVerify = true |
There was a problem hiding this comment.
Same as above, add comment here explaining why this is safe.
certloader/spiffe_tls_config_test.go
Outdated
| type testLogger bytes.Buffer | ||
|
|
||
| func (l *testLogger) Printf(format string, args ...interface{}) { | ||
| fmt.Fprintf((*bytes.Buffer)(l), format, args...) |
There was a problem hiding this comment.
Nit: Could map this to t.Logf from the testing package maybe, would produce slightly cleaner output I think.
There was a problem hiding this comment.
oops, i idea here was to assert that certain things were logged but I forgot to implement the checks.
docs/SPIFFE-WORKLOAD-API.md
Outdated
| ------------------- | ||
|
|
||
| The identity of the peer, i.e. the [SPIFFE ID](https://github.com/spiffe/spiffe/blob/master/standards/SPIFFE-ID.md), is embedded as a URI SAN on the | ||
| X509-SVID. Accordingly, the existing `--verify-uri-san` and `--allow-uri-san` |
There was a problem hiding this comment.
Nit: While the old flags still exist (and will continue to work as aliases), we actually renamed these to drop the -san suffix and expose them as "allow-uri" and "verify-uri" now.
| @@ -0,0 +1,57 @@ | |||
| SPIFFE Workload API Support | |||
There was a problem hiding this comment.
These docs are great, thank you!
main.go
Outdated
| logger.Printf("error: unable to load certificates: %s\n", err) | ||
| return err | ||
| } | ||
| tlsConfigSource = certloader.TLSConfigSourceFromCertificate(cert) |
There was a problem hiding this comment.
Might be nice to factor this out into a function. I guess in general I should probably refactor main() a bit to make it more readable.
|
Left some extra comments, but LGTM aside from some minor things. |
|
Also re: "An open question is whether --workload-api-addr should imply --use-workload-api.... thoughts anybody?" I think probably yes, it would be good if it exited with an error if an addr is set without using the workload API. Not a big deal if it doesn't but kinda nice for people who are new and playing around with the flags. |
|
I think I was more interested in knowing if it was a good idea to have |
|
Hmm. The more I think about it, the more I'm personally in favor of specifying either |
|
Awesome, thanks @csstaub. And congrats on the PR 200 milestone! |
This PR provides a TLS config source that obtains up-to-date certificates and trusted roots from the SPIFFE Workload API.
It leverages the go-spiffe Workload API library.
The PR adds the
--use-workload-apiCLI flag to enable the behavior. By default, the location of the SPIFFE Workload API socket is picked up from theSPIFFE_ENDPOINT_SOCKETenvironment variable. The--workload-api-addrflag can be used to explicitly set the address.The functionality has been tested against SPIRE.