SecretDiscoveryService server implementation #667

azdagron · 2019-01-09T23:31:47Z

This PR adds an implementation of the Envoy Secret Discovery Service to the SPIRE agent. The SDS server is off by default and must be enabled with the enable_sds config value.

Specifically, the SDS server is hosted on the same UDS socket as the Workload API. It performs workload attestation to determine which secrets to make available to the caller. Resources currently map as follows:

SVIDs map to TLSCertificate resources with a resource name matching the SPIFFE ID of the SVID.
Bundles map to ValidationContext resources with a resource name matching the trust domain ID of the bundle.

At some point, we may consider symbolic names like "default_tls_certificate" or default_validation_context" that map to certain resources, but that is out of scope for the initial
implementation.

This commit adds an implementation of the Envoy Secret Discovery Service to the SPIRE agent. The SDS server is off by default and must be enabled with the `enable_sds` config value. Specifically, the SDS server is hosted on the same UDS socket as the Workload API. It performs workload attestation to determine which secrets to make available to the caller. Resources currently map as follows: - SVIDs map to TLSCertificate resources with a resource name matching the SPIFFE ID of the SVID. - Bundles map to ValidationContext resources with a resource name matching the trust domain ID of the bundle. At some point, we may consider symbolic names like "default_tls_certificate" or "default_validation_context" that map to certain resources, but that is out of scope for the initial implementation. Signed-off-by: Andrew Harding <azdagron@gmail.com>

evan2645

this looks great, thanks @azdagron

I reviewed this 1) while sick, and 2) without intimate knowledge of expected xDS API behavior (though I did reference the docs)... so, apologies in advance for any strange questions or comments :). Hopefully I'll be better by the time this is ready for another pass

evan2645 · 2019-01-16T21:29:11Z

pkg/agent/endpoints/sds/handler.go

+	}
+	defer done()
+
+	metrics = metrics


metrics is unused

evan2645 · 2019-01-16T23:48:22Z

pkg/agent/endpoints/sds/handler.go

+	for {
+		select {
+		case newReq := <-reqch:
+			h.c.Log.Debugf("stream-secrets: recv: res=%q ver=%q typ=%q nonce=%q", newReq.ResourceNames, newReq.VersionInfo, newReq.TypeUrl, newReq.ResponseNonce)


Seems like a good opportunity to take advantage of the structured logger fields

evan2645 · 2019-01-16T23:52:37Z

pkg/agent/endpoints/sds/handler.go

+			// match the last sent nonce, otherwise the request should be
+			// ignored.
+			if ((lastNonce == "") != (newReq.ResponseNonce == "")) || lastNonce != newReq.ResponseNonce {
+				h.c.Log.Warnf("unexpected nonce %q (expected %q); ignoring request", newReq.ResponseNonce, lastNonce)


Nit: log message should be capitalized. A little more clarity would be good too e.g. Received unexpected nonce %q (expected %q); ignoring request. The subsystem_name field should be enough to indicate that it's related to the SDS API

evan2645 · 2019-01-16T23:57:39Z

pkg/agent/endpoints/sds/handler.go

+			// The nonce should be empty (if we've never sent a response) or
+			// match the last sent nonce, otherwise the request should be
+			// ignored.
+			if ((lastNonce == "") != (newReq.ResponseNonce == "")) || lastNonce != newReq.ResponseNonce {


Is this different than if lastNonce != newReq.ResponseNonce {? Seems to me that if we have lastNonce set to none, we should also expect newReq to have no nonce set?

seems to me you are correct

evan2645 · 2019-01-17T00:02:09Z

pkg/agent/endpoints/sds/handler.go

+			if req.ErrorDetail != nil {
+				// The caller has failed to apply the secrets. Wait until the
+				// next update to send down new info.
+				h.c.Log.Errorf("caller failed to apply secrets: %q", req.ErrorDetail.Message)


nit: clearer message and capitalization would be good (we know its x509 etc) e.g. Envoy instance failed to install an SVID: %q

Is it possible to know a SPIFFE ID or something here to identify which instance?

the "secret" could be a validation context :) i will add some details though, like the resource names.

evan2645 · 2019-01-17T00:04:17Z

pkg/agent/endpoints/sds/handler.go

+			return err
+		}
+
+		h.c.Log.Debugf("stream-secrets: send: ver=%q typ=%q nonce=%q count=%d", resp.VersionInfo, resp.TypeUrl, resp.Nonce, len(resp.Resources))


Same comment here on logging

evan2645 · 2019-01-17T00:27:29Z

pkg/agent/endpoints/sds/handler.go

+			}
+			if req.ErrorDetail != nil {
+				// The caller has failed to apply the secrets. Wait until the
+				// next update to send down new info.


What sort of conditions can cause this to fail? Waiting for the next update might be too little too late...?

If the update is malformed, or empty for example. There may be transient error conditions, but my interpretation of https://github.com/envoyproxy/data-plane-api/blob/master/XDS_PROTOCOL.md#when-to-send-an-update is that I shouldn't re-send the update unless there is some change from the last DiscoveryResponse that was sent. I assume there is no corrective action I can take on the payload to get envoy to suddenly accept it.

Ahh... ok yes, I see that now. I misinterpreted one of the error diagrams in that doc.

I guess I figured that there are probably transient error conditions on envoy side, but maybe there are not...? My main concern here was a transient envoy error preventing an SVID update, then subsequently falling offline because presumably that SVID would expire before we try to create a new one (which would really be the third one).

I am definitely no expert here... I guess we can take it as is and see if we run into anything

evan2645 · 2019-01-18T19:50:55Z

pkg/agent/endpoints/sds/handler.go

+}
+
+func (h *Handler) FetchSecrets(ctx context.Context, req *api_v2.DiscoveryRequest) (*api_v2.DiscoveryResponse, error) {
+	h.c.Log.Debugf("fetch-secrets: res=%q ver=%q typ=%q", req.ResourceNames, req.VersionInfo, req.TypeUrl)


Same comment here on logging

evan2645 · 2019-01-18T19:51:12Z

pkg/agent/endpoints/sds/handler.go

+	}
+	defer done()
+
+	metrics = metrics


metrics is unused

evan2645 · 2019-01-18T19:53:43Z

pkg/agent/endpoints/sds/handler.go

+func (h *Handler) callerPID(ctx context.Context) (pid int32, err error) {
+	info, ok := auth.CallerFromContext(ctx)
+	if !ok {
+		return 0, errors.New("Unable to fetch credentials from context")


nit: these errors should be lowercased

Signed-off-by: Andrew Harding <azdagron@gmail.com>

evan2645

Wooooo 🔵 🐙

evan2645 · 2019-01-23T02:03:43Z

pkg/agent/endpoints/sds/handler.go

+				errch <- err
+				return
+			}
+			reqch <- req


The xDS docs say that the management server should respond to the latest request received by envoy, though it seems like there is room for a second request to race the first response. It's not super clear to me how we can avoid that gracefully, but thought I'd point it out... maybe I am missing something

I'm not sure it is possible to avoid the race no matter what you do, at least the way the protocol is constructed 🤔

Yea I thought the same... 🤷

azdagron requested review from amartinezfayo, evan2645, MarcosDY, martincapello and walmav as code owners January 9, 2019 23:31

evan2645 reviewed Jan 18, 2019

View reviewed changes

azdagron added 2 commits January 18, 2019 14:01

clean up logging messages

cb33bb2

Signed-off-by: Andrew Harding <azdagron@gmail.com>

log more error details

097eaef

Signed-off-by: Andrew Harding <azdagron@gmail.com>

evan2645 approved these changes Jan 23, 2019

View reviewed changes

Merge branch 'master' into sds

9936aed

azdagron merged commit aeec64a into spiffe:master Jan 23, 2019

azdagron deleted the sds branch January 23, 2019 02:21

evan2645 mentioned this pull request Feb 4, 2019

Prepare for 0.7.3 #703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SecretDiscoveryService server implementation #667

SecretDiscoveryService server implementation #667

azdagron commented Jan 9, 2019

evan2645 left a comment

evan2645 Jan 16, 2019

azdagron Jan 18, 2019

evan2645 Jan 16, 2019

azdagron Jan 18, 2019

evan2645 Jan 16, 2019

azdagron Jan 18, 2019

evan2645 Jan 16, 2019

azdagron Jan 18, 2019

evan2645 Jan 17, 2019

azdagron Jan 18, 2019

evan2645 Jan 17, 2019

azdagron Jan 18, 2019

evan2645 Jan 17, 2019

azdagron Jan 18, 2019

evan2645 Jan 23, 2019

evan2645 Jan 18, 2019

azdagron Jan 18, 2019

evan2645 Jan 18, 2019

azdagron Jan 18, 2019

evan2645 Jan 18, 2019

azdagron Jan 18, 2019

evan2645 left a comment

evan2645 Jan 23, 2019

azdagron Jan 23, 2019

evan2645 Jan 23, 2019

SecretDiscoveryService server implementation #667

SecretDiscoveryService server implementation #667

Conversation

azdagron commented Jan 9, 2019

evan2645 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan2645 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment