Server CA Manager cleanup #859

azdagron · 2019-04-15T18:44:35Z

CA no longer returns full SVID chain back to signing authority and
manager code required to facilitate is removed.
X509CA and JWT keys rotate separately (currently at the same interval)
removed unnecessary details out of the CA struct so it can be trivially
used from tests.
replaced on-disk JSON store with a protobuf-based journal that
maintains a number of old entries for debugging.

- CA no longer returns full SVID chain back to signing authority and manager code required to facilitate is removed. - removed unnecessary details out of the CA struct so it can be trivally used from tests. - replaced on-disk JSON store with a protobuf-based journal that maintains a number of old entries for debugging. Signed-off-by: Andrew Harding <[email protected]>

Signed-off-by: Andrew Harding <[email protected]>

evan2645

Super clean, things are a lot easier to follow now... thank you for this!

A few comments here, some more important than others.

evan2645 · 2019-04-23T20:05:10Z

pkg/server/endpoints/node/handler.go

@@ -703,7 +708,7 @@ func (h *Handler) signCSRs(ctx context.Context,
 			}

 		} else if spiffeID == h.c.TrustDomain.String() {
-			h.c.Log.Debugf("Signing downstream SVID for %v on request by %v", spiffeID, callerID)
+			signLog.Debug("Signing downstream SVID for caller")


We should be explicit about this being a CA cert

evan2645 · 2019-04-23T20:06:26Z

pkg/server/endpoints/node/handler.go

@@ -691,7 +696,7 @@ func (h *Handler) signCSRs(ctx context.Context,
 				return nil, errors.New("SVID serial number does not match")
 			}

-			h.c.Log.Debugf("Signing SVID for %v on request by %v", spiffeID, callerID)
+			signLog.Debug("Signing SVID for caller")


Maybe this can be "Renewing agent SVID"?

evan2645 · 2019-04-23T20:07:14Z

pkg/server/endpoints/node/handler.go

+		signLog := h.c.Log.WithFields(logrus.Fields{
+			"caller_id": callerID,
+			"spiffe_id": spiffeID,
+		})


As long as we're in here, it would be good to add source ip and port to this entry e.g. source_address

evan2645 · 2019-04-23T21:50:26Z

pkg/server/ca/ca.go

-	token, err := ca.jwtSigner.SignToken(jsr.SpiffeId, jsr.Audience, expiresAt, signer, kp.jwtSigningKey.Kid)
-	if err != nil {
-		return "", fmt.Errorf("unable to sign JWT-SVID: %v", err)
+func (ca *CA) calculateLifetime(ttl time.Duration, expirationCap time.Time) (notBefore, notAfter time.Time) {


nit: perhaps capLifetime? It would make the purpose of the second argument very clear when seen from a callsite

evan2645 · 2019-04-24T20:44:58Z

pkg/server/ca/manager.go

+	}
+
+	upstreamCA, _ := m.c.Catalog.GetUpstreamCA()
+	x509CA, trustBundle, err := SignX509CA(ctx, signer, upstreamCA, m.c.UpstreamBundle, m.c.TrustDomain.Host, m.c.CASubject, notBefore, notAfter)


Any reason not to have UpstreamSignX509CA and check ok? This function signature is a mouthful and it seems that SignX509CA complexity can be reduced by a lighter if/else check here

Hmm, i did this to DRY up this code when called from the fakeserverca.... i could dup the if/then code if you think it would be clearer...

Not a blocker. I think this is probably the only real spot where GetUpstreamCA matters, ignoring the ok return parameter is mostly what caught my eye, in addition to the curiousity of why we are passing a signer and also an upstreamCA. I do think having UpstreamSignX509CA would be an improvement, but it's pretty minor.

I didn't carefully consider impact on fakeserverca, though I would probably lean towards optimization in the "real" code. As you wish!

I ended up fixing it and liked the results. good suggestion.

evan2645 · 2019-04-24T20:53:54Z

pkg/server/ca/manager.go

@@ -237,11 +304,21 @@ func (m *manager) pruneBundleEvery(ctx context.Context, interval time.Duration)
 	}
 }

-func (m *manager) pruneBundle(ctx context.Context) (err error) {
+func (m *Manager) activateJWTKey() {


This function should probably appear after prepareJWTKey and before pruneBundleEvery

😬 I done goofed resolving a merge conflict. Thanks for catching.

evan2645 · 2019-04-24T22:42:24Z

pkg/server/ca/ca.go

 	DNSList []string
 }

-type serverCAConfig struct {
+type X509CA struct {


I think this struct could be a little clearer if we separately define something like Certificate and UpstreamChain. I know that in node api and workload api we have a single slice where element 0 is the leaf and the rest is the chain (if any), but AFAICT the reason for that is that this is how TLS does it.

IsIntermediate is then defined by len(UpstreamChain) > 0. The IsIntermediate bool isn't often evaluated, and where it is, an empty UpstreamChain seems like it could simplify things. The name IsIntermediate is also a little confusing because when upstream_bundle is false and we are using UpstreamCA, the cert is still technically an intermediate.

I can get behind that. Will fix.

evan2645 · 2019-04-24T22:56:26Z

pkg/server/ca/ca.go


-	template, err := CreateServerCATemplate(csrDER, ca.c.TrustDomain.Host, notBefore, notAfter, serialNumber)
+	template, err := CreateX509SVIDTemplate(csrDER, ca.c.TrustDomain.Host, notBefore, notAfter, serialNumber)


Where does CA bit get set? Key usages there are different too.

I get worried about DRYing the CA minting process with the regular leaf SVID minting process, and I think that this change validates the paranoia (not that it is your fault, it can happen to anyone). Note that previously we called CreateServerCATemplate. Handling CA certificate signing for downstream servers is a super sensitive operation... aside from having nuanced differences in the minting process, it is also easier to introduce security bugs/holes that pass CR (I almost missed this one myself).

I'd feel a lot more comfortable having repeated code, one path which handles minting of downstream CA certificate, and one which handles minting of leaf certificates.

Relatedly, seems that we don't have adequate unit test coverage on SignX509SVID vs SignX509CASVID. I can see that we were anemic there from before, but now would be an awesome time to beef that up to assert that the former is not giving CA certs, and the latter is giving properly-formed CA certs (e.g. CA bit is set, key usage is appropriate, etc)

Whoops 😬 I'll un-DRY and beef up the testing.

Signed-off-by: Andrew Harding <[email protected]>

…leanup Signed-off-by: Andrew Harding <[email protected]>

evan2645

Great!! thanks again, and sorry it took so long to review. A couple small non-blocking nits, approved anyway 🍨 🦅

evan2645 · 2019-04-25T18:43:30Z

pkg/server/ca/ca.go

+	// UpstreamChain contains the CA certificate and intermediates necessary
+	// to chain back to the upstream trust bundle. It is only set if the CA
+	// is signed by an UpstreamCA and the upstream trust bundle is included in
+	// the SPIRE trust bundle (see the upstream_bundle configurable).


The word included is a little confusing since IIUC if upstream_bundle is set, then the SPIRE trust bundle is the upstream bundle? Perhaps It is only set if the CA is signed by an UpstreamCA and upstream_bundle is set to true?

evan2645 · 2019-04-25T18:58:46Z

pkg/server/ca/ca_test.go

+	s.NotEmpty(svid.SubjectKeyId, "SubjectKeyId is not set")
+	s.Equal(x509.KeyUsageKeyEncipherment|x509.KeyUsageKeyAgreement|x509.KeyUsageDigitalSignature, svid.KeyUsage, "key usage does not match")
+	s.Equal([]x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, svid.ExtKeyUsage, "ext key usage does not match")
+	s.False(svid.IsCA, "CA bit is set")


Awesome, thanks for this ❤️

evan2645 · 2019-04-25T19:00:10Z

pkg/server/ca/ca_test.go

+
+	// SPIFFE ID should be set to that of the trust domain
+	if s.Len(svid.URIs, 1, "has no URIs") {
+		s.Equal("spiffe://example.org", svid.URIs[0].String())


Should we be allowing this? Perhaps we should file an issue, I think it is against SPIFFE spec

evan2645 · 2019-04-25T19:06:05Z

pkg/server/ca/journal.proto

-    // when the upstream bundle has been included in the trust bundle (see
-    // upstream_bundle configurable).
-    bool is_intermediate = 4;
+    // DER encoded upstream CA chain. This will also contain the CA certificate.


nit: this may also contain the CA certificate?

- changed AllowAnyInTrustDomain to only allow trust domain members, and not the trust domain ID itself - cleaned up some comments - added a few more tests around ID validation for signing ops Signed-off-by: Andrew Harding <[email protected]>

Signed-off-by: Andrew Harding <[email protected]>

evan2645 · 2019-04-25T20:18:09Z

pkg/server/ca/ca_test.go

 	}

 	// Subject is hard coded by the CA and should not be pulled from the CSR.
 	s.Equal("O=SPIRE,C=US", svid.Subject.String())
 }

+func (s *CATestSuite) TestSignX509SVIDCannotSignTrustDomainID() {


evan2645 · 2019-04-25T20:18:51Z

pkg/server/ca/ca_test.go

 	s.Require().NoError(err)
 	s.Require().Len(svid, 1)
 	s.Require().Equal(s.clock.Now().Add(-backdate), svid[0].NotBefore)
 	s.Require().Equal(s.clock.Now().Add(time.Minute), svid[0].NotAfter)
 }

+func (s *CATestSuite) TestSignCAX509SVIDValidatesTrustDomain() {


azdagron requested review from amartinezfayo, evan2645, MarcosDY, martincapello and walmav as code owners April 15, 2019 18:44

fix k8s-test attestation check

799ae8e

Signed-off-by: Andrew Harding <[email protected]>

evan2645 reviewed Apr 24, 2019

View reviewed changes

azdagron added 2 commits April 25, 2019 10:27

address PR comments

6adfa34

Signed-off-by: Andrew Harding <[email protected]>

Merge remote-tracking branch 'origin/master' into server-ca-manager-c…

eed5ffd

…leanup Signed-off-by: Andrew Harding <[email protected]>

evan2645 previously approved these changes Apr 25, 2019

View reviewed changes

azdagron added 2 commits April 25, 2019 14:12

Address more PR comments

111b823

- changed AllowAnyInTrustDomain to only allow trust domain members, and not the trust domain ID itself - cleaned up some comments - added a few more tests around ID validation for signing ops Signed-off-by: Andrew Harding <[email protected]>

Merge remote-tracking branch 'origin' into server-ca-manager-cleanup

0f8decd

Signed-off-by: Andrew Harding <[email protected]>

azdagron dismissed evan2645’s stale review via 0f8decd April 25, 2019 20:14

evan2645 approved these changes Apr 25, 2019

View reviewed changes

azdagron merged commit 4f587d6 into spiffe:master Apr 25, 2019

azdagron deleted the server-ca-manager-cleanup branch April 25, 2019 20:30

evan2645 mentioned this pull request Jul 11, 2019

Agent crashes with transport: error while dialing: x509: signature check attempts limit reached while verifying certificate chain #1004

Closed

This was referenced Feb 8, 2022

Update CA manager filter journal JwtKeys entries #2751

Closed

Filter out local JwtKeys that do not exist in the database bundle #2770

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server CA Manager cleanup #859

Server CA Manager cleanup #859

azdagron commented Apr 15, 2019

evan2645 left a comment

evan2645 Apr 23, 2019

azdagron Apr 24, 2019

evan2645 Apr 23, 2019

azdagron Apr 24, 2019

evan2645 Apr 23, 2019

azdagron Apr 24, 2019

evan2645 Apr 23, 2019

azdagron Apr 24, 2019

evan2645 Apr 24, 2019

azdagron Apr 24, 2019

evan2645 Apr 24, 2019

azdagron Apr 25, 2019

evan2645 Apr 24, 2019

azdagron Apr 24, 2019

evan2645 Apr 24, 2019

azdagron Apr 24, 2019

evan2645 Apr 24, 2019

azdagron Apr 24, 2019

evan2645 left a comment

evan2645 Apr 25, 2019

evan2645 Apr 25, 2019

evan2645 Apr 25, 2019

evan2645 Apr 25, 2019

evan2645 Apr 25, 2019

evan2645 Apr 25, 2019


		template, err := CreateServerCATemplate(csrDER, ca.c.TrustDomain.Host, notBefore, notAfter, serialNumber)
		template, err := CreateX509SVIDTemplate(csrDER, ca.c.TrustDomain.Host, notBefore, notAfter, serialNumber)

Server CA Manager cleanup #859

Server CA Manager cleanup #859

Conversation

azdagron commented Apr 15, 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

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