Implement spiffebundle.Bundle type #89

amartinezfayo · 2020-03-29T04:29:06Z

Implement the spiffebundle.Bundle type of the v2 API.
Add FromJWTKeys and FromX509Roots functions to x509bundle, jwtbundle and spiffebundle packages.

Fixes: #59

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron

My biggest concern here is false sharing as X.509 root slices and JWT key maps are passed around. We should come up with consistent behavior that 1) minimizes the number of copies, but 2) prevents false sharing where the method feels like it should produce an independent copy.

I don't think we have to go so far as duplicating the x.509 certificate and JWT keys, just the containers they are in.

azdagron · 2020-03-30T15:13:48Z

v2/bundle/spiffebundle/bundle.go

+			if err := bundle.AddJWTKey(key.KeyID, key.Key); err != nil {
+				return nil, spiffebundleErr.New("error adding key %d of JWKS: %v", i, errs.Unwrap(err))
+			}
+		case "":


I think this case can be removed. From the spec:

Clients encountering unknown use values MUST ignore the entire JWK element.

It was not clear to me what was the right thing to do here, since the spec also says "The use parameter MUST be set". I'm ok removing this.

Hmm. That's a good point. @evan2645, what do you think? Spec seems to say "fail if not set, and ignore if set to an unknown value"... ?

I'm sorry that the spec has caused this confusion. As part of this exchange, suggestions on how to clarify the text (and/or a PR/issue in https://github.com/spiffe/spiffe) would be useful.

Yes, use MUST be set... but requiring it to be set should not by itself suggest a particular behavior if a validator finds that this is not the case. All SPIFFE bundle producers MUST set the use claim. SPIFFE bundle validators MUST ignore JWK entries with unknown use claims (including an empty or missing use claim).

If we have logging available, this would be a good place to do it.

Is that something allowed by the spec? JWKS RFC only says that the keys array must exist, not that it must have at least one key...

Fair point. It does seem like a pretty strange condition, though...

The jwtbundle.Bundle type assumes that the use is jwt-svid, it does not require any SPIFFE specific parameters when parsing a JWKS document and does not set the use` when marshalling. That's the current behavior in this branch.

Yes I think that is the correct behavior. We should do whatever the JWKS RFC says when parsing jwt-svid bundle. IIRC the RFC says to ignore unknown values. One open question is, in this case, jwt-svid use value is non unknown, so should be be technically able to load a jwt-svid bundle given a spiffe bundle? Seems like a nicety. But I don't think we should not allow a JWKS bundle to be loaded as a SPIFFE bundle

@evan2645 @azdagron From the feedback received, I would keep this check and require the use field as a condition of conformance to the SPIFFE Trust Domain and Bundle spec, while we ignore the entire JWK element in case of an unknown use value. Are we ok with that?

@amartinezfayo. I think what we've settled with is to ignore keys with missing or unrecognized use values, i.e. don't enforce conformance as a validator, just do the safe thing (i.e. ignore keys that aren't safe to use).

We don't have a good story for library-level logging for this kind of thing.

Thanks for the clarification @azdagron.

@evan2645 I filed spiffe/spiffe#121 to consider a clarification from the spec perspective.

azdagron · 2020-03-30T15:20:29Z

v2/bundle/spiffebundle/bundle.go

+			x509Roots:   x509Bundle.X509Roots(),
+		}
+	}
+	return &Bundle{}


Unless there is a valid, ergonomic case for receiving a nil bundle, I'd advocate that we let it panic. Either way returning a valid Bundle pointer here seems like the wrong thing to do since it will not have a trust domain and will be unusable. If we decide not to panic, we should return nil.

Makes sense, I'll change this to panic.

azdagron · 2020-03-30T15:20:44Z

v2/bundle/spiffebundle/bundle.go

+			jwtKeys:     jwtBundle.JWTKeys(),
+		}
+	}
+	return &Bundle{}


Same comment here regarding panicking on a nil bundle...

Makes sense, I'll change this to panic.

azdagron · 2020-03-30T15:22:34Z

v2/bundle/spiffebundle/bundle.go

+	b.mtx.RLock()
+	defer b.mtx.RUnlock()
+
+	return b.x509Roots


We should probably copy the slice so we don't accidentally introduce false sharing.

azdagron · 2020-03-30T15:22:45Z

v2/bundle/spiffebundle/bundle.go

+	b.mtx.RLock()
+	defer b.mtx.RUnlock()
+
+	return b.jwtKeys


We should probably copy the map so we don't accidentally introduce false sharing.

azdagron · 2020-03-30T15:35:27Z

v2/bundle/spiffebundle/bundle_test.go

+		Raw: []byte("CERT 2"),
+	}
+	testCases = []testCase{
+		testCase{


nit: testCase type here can be deduced by the compiler and should be omitted

azdagron · 2020-03-30T15:38:15Z

v2/bundle/spiffebundle/bundle_test.go

+	bundleBytesMarshal, err := bundle.Marshal()
+	require.NoError(t, err)
+
+	// Prase the marshaled bundle


Suggested change

// Prase the marshaled bundle

// Parse the marshaled bundle

azdagron · 2020-03-30T15:41:24Z

v2/internal/x509util/util.go

+}
+
+// CertsEqual checks if two X.509 certificates are equal by comparing its raw bytes
+func CertsEqual(a, b *x509.Certificate) bool {


I'm embarrassed I didn't remember this during the PR for the x509 bundle. This function is provided by the stdlib (x509.Certificate.Equal) and has the same behavior.

azdagron · 2020-03-30T15:43:29Z

v2/bundle/spiffebundle/bundle.go

+	trustDomain spiffeid.TrustDomain
+
+	mtx            sync.RWMutex
+	refreshHint    *int64


I think things are a little nicer if this is a time.Duration and the conversion happens during marshalling/unmarshalling instead of inside SetRefreshHint/RefreshHint.

azdagron · 2020-03-30T15:46:30Z

v2/bundle/spiffebundle/bundle.go

+	SequenceNumber uint64 `json:"spiffe_sequence,omitempty"`
+	RefreshHint    int64  `json:"spiffe_refresh_hint,omitempty"`


These should be pointers so we have explicit behavior on set v.s. unset values. For example, as written, somebody couldn't set a sequence number of zero and have it show up in the marshaled bundle.

Suggested change

SequenceNumber uint64 `json:"spiffe_sequence,omitempty"`

RefreshHint int64 `json:"spiffe_refresh_hint,omitempty"`

SequenceNumber *uint64 `json:"spiffe_sequence,omitempty"`

RefreshHint *int64 `json:"spiffe_refresh_hint,omitempty"`

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron · 2020-04-01T22:50:13Z

v2/bundle/spiffebundle/bundle.go

 func FromX509Bundle(x509Bundle *x509bundle.Bundle) *Bundle {
 	if x509Bundle != nil {
 		return &Bundle{
 			trustDomain: x509Bundle.TrustDomain(),
 			x509Roots:   x509Bundle.X509Roots(),
 		}
 	}
-	return &Bundle{}
+	panic(spiffebundleErr.New("no X.509 bundle"))


I was thinking more along the lines just trying to use the bundle pointer and letting the runtime panic, like:

func FromX509Bundle(x509Bundle *x509bundle.Bundle) *Bundle { return &Bundle{ trustDomain: x509bundle.TrustDomain(), x509Bundle.X509Roots(), } }

Doing the explicit nil check with the panic seems like overkill.

azdagron · 2020-04-01T22:50:48Z

v2/bundle/spiffebundle/bundle.go

 func FromJWTBundle(jwtBundle *jwtbundle.Bundle) *Bundle {
 	if jwtBundle != nil {
 		return &Bundle{
 			trustDomain: jwtBundle.TrustDomain(),
 			jwtKeys:     jwtBundle.JWTKeys(),
 		}
 	}
-	return &Bundle{}
+	panic(spiffebundleErr.New("no JWT bundle"))


azdagron · 2020-04-01T22:55:32Z

v2/bundle/spiffebundle/set.go

@@ -1,49 +1,4 @@
 package spiffebundle


Hmm, is the removal of the set scaffolding accidental?

azdagron · 2020-04-01T22:59:37Z

v2/bundle/spiffebundle/bundle.go

+	b.mtx.RLock()
+	defer b.mtx.RUnlock()
+
+	return copyX509Roots(b.x509Roots)


copyX509Roots and copyJWTKey probably belong in a common package under internal/... we're going to want to update jwtbundle and x509bundle to use them in the respective JWTKeys and X509Roots functions.

azdagron · 2020-04-01T23:06:36Z

v2/bundle/spiffebundle/bundle.go

+			if err := bundle.AddJWTKey(key.KeyID, key.Key); err != nil {
+				return nil, spiffebundleErr.New("error adding key %d of JWKS: %v", i, errs.Unwrap(err))
+			}
+		case "":


@amartinezfayo. I think what we've settled with is to ignore keys with missing or unrecognized use values, i.e. don't enforce conformance as a validator, just do the safe thing (i.e. ignore keys that aren't safe to use).

We don't have a good story for library-level logging for this kind of thing.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

* Implement spiffebundle.Bundle type

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

amartinezfayo added 2 commits March 28, 2020 13:32

Implement spiffebundle.Bundle type

baa344d

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Implement spiffebundle.Bundle type

f3999cd

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

amartinezfayo requested a review from azdagron March 29, 2020 04:29

amartinezfayo requested a review from evan2645 as a code owner March 29, 2020 04:29

azdagron reviewed Mar 30, 2020

View reviewed changes

Address PR comments

c8b6d20

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron reviewed Apr 1, 2020

View reviewed changes

amartinezfayo added 2 commits April 1, 2020 20:29

Address PR comments

d129581

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Address PR comments

c0940cf

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron approved these changes Apr 2, 2020

View reviewed changes

amartinezfayo merged commit 94c7349 into spiffe:v2-api Apr 2, 2020

azdagron pushed a commit that referenced this pull request Apr 22, 2020

Implement spiffebundle.Bundle type (#89)

29ddf4a

* Implement spiffebundle.Bundle type

marcosy pushed a commit that referenced this pull request Apr 22, 2020

Implement spiffebundle.Bundle type (#89)

de85887

* Implement spiffebundle.Bundle type

martincapello pushed a commit that referenced this pull request Apr 22, 2020

Implement spiffebundle.Bundle type (#89)

c960285

* Implement spiffebundle.Bundle type

amartinezfayo added a commit that referenced this pull request Apr 22, 2020

Implement spiffebundle.Bundle type (#89)

f50aa45

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron mentioned this pull request May 28, 2020

Implement spiffebundle.Bundle type #59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement spiffebundle.Bundle type #89

Implement spiffebundle.Bundle type #89

amartinezfayo commented Mar 29, 2020

azdagron left a comment

azdagron Mar 30, 2020

amartinezfayo Mar 30, 2020

azdagron Mar 30, 2020

evan2645 Mar 30, 2020

evan2645 Mar 30, 2020

evan2645 Apr 1, 2020

amartinezfayo Apr 1, 2020

azdagron Apr 1, 2020

amartinezfayo Apr 1, 2020

amartinezfayo Apr 3, 2020

azdagron Mar 30, 2020

amartinezfayo Mar 30, 2020

azdagron Mar 30, 2020

amartinezfayo Mar 30, 2020

azdagron Mar 30, 2020

azdagron Mar 30, 2020

azdagron Mar 30, 2020

azdagron Mar 30, 2020

azdagron Mar 30, 2020

azdagron Mar 30, 2020

azdagron Mar 30, 2020

azdagron Apr 1, 2020

azdagron Apr 1, 2020

azdagron Apr 1, 2020

amartinezfayo Apr 1, 2020

azdagron Apr 1, 2020

azdagron Apr 1, 2020

		SequenceNumber uint64 `json:"spiffe_sequence,omitempty"`
		RefreshHint int64 `json:"spiffe_refresh_hint,omitempty"`

Implement spiffebundle.Bundle type #89

Implement spiffebundle.Bundle type #89

Conversation

amartinezfayo commented Mar 29, 2020

azdagron 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

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