JWT workload API extensions #616

azdagron · 2018-11-05T22:51:13Z

adds CLI to fetch and validate JWT SVIDs
adds an "issued at" claim to server minted JWTs
extends node API to convey "issued at" so the
agent can cache appropriately w/o having to parse the JWT.
JWT lifetime is currently hard-coded to five minutes w/ a renewal time
of 2.5 minutes.

- adds CLI to fetch and validate JWT SVIDs - adds an "issued at" claim to server minted JWTs - extends node API to convey "issued at" so the agent can cache appropriately w/o having to parse the JWT. - JWT lifetime is currently hard-coded to five minutes w/ a renewal time of 2.5 minutes. Signed-off-by: Andrew Harding <azdagron@gmail.com>

evan2645 · 2018-11-05T23:22:23Z

cmd/spire-agent/cli/api/common.go

+		return net.DialTimeout("unix", addr, timeout)
+	}
+	// Workload API is unauthenticated
+	conn, err := grpc.Dial(socketPath, grpc.WithInsecure(), grpc.WithDialer(dialer))


We should probably put this in /api directory so folks consuming the workload api client library can benefit from these updates. In general, it's good practice to build the CLI tooling on top of our own client libraries

you're talking about the dialing details? sure, i can put them in some helper in /api/workload.

hmm, turns out there is already an /api/workload/dial package with Dial functionality. I'll switch to it.

or are you talking about making a high level JWTClient or something?

Yes and yes :)

The code in CLI package should be relatively minimal, a thin interaction layer that sits on top of the client library. IMO, having a lot of logic that lives in the CLI code indicates that the client library is lacking in some way

Are you ok with this work being in a follow up PR?

Sure, let's make an issue though so we don't forget to do it

evan2645 · 2018-11-05T23:28:13Z

cmd/spire-agent/cli/api/validate_jwt.go

+
+	"github.com/mitchellh/cli"
+	"github.com/spiffe/spire/proto/api/workload"
+


evan2645 · 2018-11-05T23:29:51Z

cmd/spire-agent/cli/api/validate_jwt.go

+
+func (c *validateJWTCommand) appendFlags(fs *flag.FlagSet) {
+	fs.Var(&c.audience, "audience", "comma separated list of audience values")
+	fs.StringVar(&c.svid, "svid", "", "JWT SVID")


should the token be modeled as a flag like this, or as the last argument?

for consistency sake, i'd say a flag. we have a lot of required flags across our various CLI's that are modeled in this way...

evan2645 · 2018-11-05T23:31:34Z

cmd/spire-agent/cli/api/validate_jwt.go

+
+	resp, err := c.validateJWTSVID(ctx, client)
+	if err != nil {
+		return fmt.Errorf("unable to validate JWT SVID: %v", err)


we should distinguish between "we ran into an error" and "this jwt is not valid"

so i can see that happening in two ways: 1) check for some explicit gRPC error which makes the coupling tight between client and server, or 2) ValidateJWTSVIDResponse gains a bool valid field (or we could treat an empty spiffe_id field as "not valid", but i prefer the boolean).

I'm leaning towards number #2, I don't like relying on the gRPC transport to deliver "failure".

SIG-Spec discussed this at one point, and the consensus was that number one was the way to go for safety reasons (it is easy to get a false positive if a client forgets to check the value of the valid bool).

We are missing text in the current Workload API extensions document that describes this, as well as what error code should be used. Looking back through SIG-Spec notes, this was discussed on the August 23rd call, and the result was "use InvalidArgument with an appropriate description attached"

ok, i'll go with InvalidArgument

evan2645 · 2018-11-05T23:32:54Z

doc/SPIRE101.md

@@ -187,9 +187,9 @@ plugins {

 14. Simulate the workload API interaction and retrieve the workload SVID bundle by running the `api` subcommand in the agent. Run the command as user **_workload_** created in step #4 with uid 1000

-        su -c "./cmd/spire-agent/spire-agent api fetch" workload
+        su -c "./cmd/spire-agent/spire-agent api fetch x509 " workload


I wonder if there are other places that we need to update this... Should we make x509 the default subcommand if one isn't given?

that seems like a safe default, i'll do that.

evan2645 · 2018-11-05T23:49:10Z

pkg/agent/endpoints/workload/handler_test.go

@@ -131,13 +131,13 @@ func (s *HandlerTestSuite) TestFetchX509SVID() {
 func (s *HandlerTestSuite) TestSendResponse() {
 	emptyUpdate := new(cache.WorkloadUpdate)
 	s.stream.EXPECT().Send(gomock.Any()).Times(0)
-	err := s.h.sendResponse(emptyUpdate, s.stream)
+	err := s.h.sendX509SVIDResponse(emptyUpdate, s.stream)


Tests for JWT handler functionality?

oops. good catch.

evan2645 · 2018-11-05T23:49:56Z

pkg/agent/manager/cache/cache.go

+	// GetJWTSVID retrieves a cached JWT SVID based on the subject and
+	// intended audience.
+	GetJWTSVID(spiffeID string, audience []string) *client.JWTSVID
+	//	SetJWTSVID caches a JWT SVID based on the subject and intended audience.


nit: errant tab

evan2645 · 2018-11-05T23:53:02Z

pkg/agent/manager/cache/cache.go

+
+	// GetJWTSVID retrieves a cached JWT SVID based on the subject and
+	// intended audience.
+	GetJWTSVID(spiffeID string, audience []string) *client.JWTSVID


nit: personal preference for ok pattern here since it gives explicit error checking over implicit

evan2645 · 2018-11-05T23:58:28Z

proto/api/workload/workload.proto

@@ -42,7 +44,51 @@ message X509SVID {
    repeated string federates_with = 5;
 }

+
+message JWTASVID {


JWTSVID... sorry for the last minute switch

evan2645 · 2018-11-05T23:59:20Z

proto/api/workload/workload.proto

+}
+
+message ValidateJWTASVIDRequest {
+    repeated string audience = 1;


I wonder if this should be repeated or not? Feels like the validator should expect only a single audience, but maybe I'm overlooking something

One scenario I can think of is migration of audience value. You'd want a window where the old audience value is still acceptable until all of the JWTs minted against the old audience expired.

Hmm yea... It can be done on the other side too, i.e. the subject sends a token with both old and new audiences, and the migrating validator identifies with one or the other.

I'm having a hard time forming an opinion here, but it kind of feels like it's more natural to have the validator identify with a single subject... I'm curious to hear other thoughts on the matter though

I guess my point is that the subject would have to be updated to know about the new audience value. I can imagine a partial rollout where old subjects are still requesting the old value, updated subjects are requesting the new value, and the target would be receiving JWT's from both.

After some offline discussion, we've decided to move to a non-repeated audience field for now. It is much easier to transition to a repeated field later if we determine we need it than moving in the opposite direction.

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

evan2645

🤴 ⏯

evan2645 · 2018-11-07T18:50:43Z

pkg/agent/endpoints/workload/handler.go

+		return nil, errs.New("audience must be specified")
+	}
+	if req.Svid == "" {
+		return nil, errs.New("svid must be specified")


seems like these should also be InvalidArgument gRPC code?

evan2645 · 2018-11-07T18:53:29Z

pkg/agent/manager/cache/cache.go

 	key := keyFromJWTSpiffeIDAndAudience(spiffeID, audience)
 	c.m.Lock()
 	defer c.m.Unlock()
-	return c.jwtSVIDS[key]
+	svid, ok := c.jwtSVIDS[key]
+	return svid, ok


interesting...

yep. gotta coerce a comma-ok here.

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

azdagron requested review from ajessup, amartinezfayo, drrt, evan2645, MarcosDY, martincapello and walmav as code owners November 5, 2018 22:51

evan2645 reviewed Nov 6, 2018

View reviewed changes

azdagron added 2 commits November 6, 2018 16:10

Merge branch 'master' into jwt-workload-extensions

d68ee6f

address PR comments

5a85be9

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

evan2645 previously approved these changes Nov 7, 2018

View reviewed changes

fixed up ValidateJWTSVID grpc status codes

4f7fbcb

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

azdagron dismissed evan2645’s stale review via 4f7fbcb November 7, 2018 20:17

evan2645 approved these changes Nov 7, 2018

View reviewed changes

azdagron merged commit bdb77b8 into spiffe:master Nov 7, 2018

azdagron deleted the jwt-workload-extensions branch November 7, 2018 21:06

skdwriting mentioned this pull request Dec 3, 2018

Support and verification for message based communication(like MQTT) among workloads (including hierarchical spire) #646

Closed


		"github.com/mitchellh/cli"
		"github.com/spiffe/spire/proto/api/workload"

JWT workload API extensions #616

JWT workload API extensions #616

Conversation

azdagron commented Nov 5, 2018 • edited Loading

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

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

azdagron Nov 6, 2018 • edited Loading

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

Choose a reason for hiding this comment

azdagron commented Nov 5, 2018 •

edited

Loading

azdagron Nov 6, 2018 •

edited

Loading