Istio Citadel Agent integrates with Vault CA providers #10638

lei-tang · 2018-12-22T09:46:11Z

Istio CA integrates with Vault CA providers

This PR is for the following issues:

security/pkg/nodeagent/caclient/providers/vault1/client.go

security/pkg/nodeagent/caclient/providers/vault1/client_test.go

myidpt · 2018-12-27T02:24:06Z

security/pkg/nodeagent/caclient/client.go

 )

 const (
 	googleCAName = "GoogleCA"
 	citadelName  = "Citadel"
+	vaultCAName1 = "VaultCA1"


Why do you put "1" in the name?

Done: changed to VaultCA per discussion.

Please remove 1.

myidpt · 2018-12-27T02:30:58Z

security/pkg/nodeagent/caclient/providers/vault1/client.go

+}
+
+// NewVaultClient create a CA client for the Vault provider 1.
+func NewVaultClient1(tls bool, tlsRootCert []byte,


Same here. If you want to add another Vault client in the future, try to give it a more descriptive name?

Done: changed to VaultClient per discussion.

myidpt · 2018-12-27T02:31:54Z

security/pkg/nodeagent/caclient/providers/vault1/client.go

+	var client *api.Client
+	var err error
+	if tls {
+		client, err = createVaultClientTLS(vaultAddr, tlsRootCert)


nit: createVaultTLSClient

myidpt · 2018-12-27T02:36:58Z

security/pkg/nodeagent/caclient/providers/vault1/client.go

+// CSR Sign calls Vault to sign a CSR.
+func (c *vaultClient1) CSRSign(ctx context.Context, csrPEM []byte, saToken string,
+	certValidTTLInSec int64) ([]string /*PEM-encoded certificate chain*/, error) {
+	token, err := loginVaultK8sAuthMethod(c.client, c.vaultLoginPath, c.vaultLoginRole, saToken)


Can we cache the token so we don't need to log in every time when CSRSign is called?

No caching the tokens for the following reasons:

The tokens may expire or be revoked.

The cached tokens may be leaked.

The code is simpler when no caching.

The code is stateless when no caching, as opposed to stateful when caching.

The storage overhead of caching.

We can do this for simplicity.

myidpt · 2018-12-27T02:46:23Z

security/pkg/nodeagent/caclient/providers/vault1/client.go

+		return nil, fmt.Errorf("failed to login Vault: %v", err)
+	}
+	c.client.SetToken(token)
+	_, certChain, err := signCsrByVault(c.client, c.vaultSignCsrPath, certValidTTLInSec, csrPEM)


Why do you return the first item if it's not used? :)

Changed signCsrByVault() to not return the first item.

myidpt · 2018-12-27T02:47:22Z

security/pkg/nodeagent/caclient/providers/vault1/client.go

+
+	if len(certChain) <= 1 {
+		log.Errorf("certificate chain length is %d, expected more than 1", len(certChain))
+		return nil, fmt.Errorf("invalid cert. chain in the response")


nit: "cert. chain" --> "cert chain"

Done: changed to "certificate chain".

myidpt · 2018-12-27T03:04:08Z

security/pkg/nodeagent/caclient/providers/vault1/client_test.go

+
+// NewMockVaultServer creates a mock Vault server for testing purpose.
+// token: required access token
+func NewMockVaultServer(t *testing.T, tls bool, loginRole, token, loginResp, signResp string) *MockVaultServer {


Make this internal by starting with lowercase letter?

myidpt · 2018-12-27T03:10:24Z

security/pkg/nodeagent/caclient/providers/vault1/client_test.go

+
+	for id, tc := range testCases {
+		ch := make(chan *MockVaultServer)
+		go func() {


In stead of creating the server each time, can you create two servers in the beginning, one with TLS and one without TLS?

security/pkg/nodeagent/caclient/providers/vault/client_test.go

security/pkg/nodeagent/caclient/providers/vault/client.go

quanjielin · 2018-12-28T04:18:27Z

security/pkg/nodeagent/caclient/providers/vault/client.go

+		return nil, fmt.Errorf("failed to login Vault: %v", err)
+	}
+	c.client.SetToken(token)
+	certChain, err := signCsrByVault(c.client, c.vaultSignCsrPath, certValidTTLInSec, csrPEM)


csr doesn't contain identity info(which is only available in saToken), both google ca and citadel extract identity from saToken, will vault support the same logic ?

This is up to Vault provider. Will sync up offline on this comment.

quanjielin · 2018-12-28T04:20:19Z

security/pkg/nodeagent/caclient/client.go

@@ -48,6 +50,9 @@ func NewCAClient(endpoint, CAProviderName string, tlsFlag bool) (caClientInterfa
 	switch CAProviderName {
 	case googleCAName:
 		return gca.NewGoogleCAClient(endpoint, tlsFlag)
+	case vaultCAName1:
+		//TO-DO (lei-tang): fill vault parameters from environmental variables
+		return vault.NewVaultClient(tlsFlag, nil, "vaultAddr", "vaultLoginRole", "vaultLoginPath", "vaultSignCsrPath")


is ubuntu CA bundle is good enough to call vault, or do you plan to pass some other tlsRootCert here ?

The CA bundle should be good enough; details are TBD.

Please enable uses to set "vaultAddr" and "vaultLoginRole" through flags.
Does it make sense to make "vualtLoginPath" and "vaultSignCsrPath" static?

Flags have been added. According to the document, all these configs should be read from environmental variables.

quanjielin · 2018-12-28T04:21:26Z

security/pkg/nodeagent/caclient/providers/vault/client.go

+		return nil, fmt.Errorf("failed to post to %v: %v", csrSigningPath, err)
+	}
+	if res == nil {
+		log.Errorf("sign response is nil")


nit: Error, here and below

lei-tang · 2018-12-31T04:24:18Z

/test e2e-simpleTests

lei-tang · 2018-12-31T05:00:58Z

/test istio-pilot-multicluster-e2e

myidpt

Thanks for the work! Generally looks good to me. Some small comments. Good to go after you fix them.

istio-testing · 2019-01-01T04:25:19Z

@lei-tang: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-pilot-multicluster-e2e.sh	`0a47c84`	link	`/test istio-pilot-multicluster-e2e`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

myidpt · 2019-01-01T05:47:07Z

/lgtm
/approve
Thanks for the work!

istio-testing · 2019-01-01T05:47:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lei-tang, myidpt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~security/OWNERS~~ [myidpt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

quanjielin · 2019-01-02T18:42:11Z

security/pkg/nodeagent/caclient/client.go

@@ -44,10 +46,13 @@ type configMap interface {
 }

 // NewCAClient create an CA client.
-func NewCAClient(endpoint, CAProviderName string, tlsFlag bool) (caClientInterface.Client, error) {
+func NewCAClient(endpoint, CAProviderName string, tlsFlag bool, vaultAddr, vaultRole,
+	vaultAuthPath, vaultSignCsrPath string) (caClientInterface.Client, error) {


don't pass vault specific params in this func, better to get those env variable in vault client.

Putting the CA provider inside vault client has pros and cons.

Pros: less perimeters.

Cons: CA provider is not exposed at Node Agent. The code is difficult to understand because how the CA provider is configured is hidden somewhere.

those variables are CA provider specific, since we already passed CAProviderName as input param, setting those variables should be pushed to lower layer(each plugin). let's revisit this after e2e works.

quanjielin · 2019-01-02T18:43:20Z

security/pkg/nodeagent/sds/server.go

@@ -62,6 +62,18 @@ type Options struct {

 	// PluginNames is plugins' name for certain authentication provider.
 	PluginNames []string
+
+	// The Vault CA address.


again, move those vault specific param to vault client

lei-tang requested review from myidpt, incfly, wattli and quanjielin December 22, 2018 09:46

googlebot added the cla: yes label Dec 22, 2018

golangcibot reviewed Dec 22, 2018

View reviewed changes

golangcibot reviewed Dec 23, 2018

View reviewed changes

security/pkg/nodeagent/caclient/providers/vault1/client_test.go Outdated Show resolved Hide resolved

security/pkg/nodeagent/caclient/providers/vault1/client_test.go Outdated Show resolved Hide resolved

lei-tang force-pushed the vault_integ_release_1.1 branch 3 times, most recently from fa237ee to 2e017ad Compare December 24, 2018 09:55

golangcibot reviewed Dec 24, 2018

View reviewed changes

security/pkg/nodeagent/caclient/providers/vault1/client_test.go Outdated Show resolved Hide resolved

lei-tang force-pushed the vault_integ_release_1.1 branch 2 times, most recently from f17c8bc to c55e0f1 Compare December 24, 2018 14:26

golangcibot reviewed Dec 24, 2018

View reviewed changes

lei-tang force-pushed the vault_integ_release_1.1 branch from c55e0f1 to 221dc94 Compare December 24, 2018 14:31

golangcibot reviewed Dec 24, 2018

View reviewed changes

security/pkg/nodeagent/caclient/providers/vault1/client_test.go Outdated Show resolved Hide resolved

lei-tang force-pushed the vault_integ_release_1.1 branch from 221dc94 to 1c437ea Compare December 24, 2018 14:36

golangcibot reviewed Dec 25, 2018

View reviewed changes

golangcibot reviewed Dec 27, 2018

View reviewed changes

myidpt reviewed Dec 27, 2018

View reviewed changes

lei-tang force-pushed the vault_integ_release_1.1 branch 2 times, most recently from 58b2149 to fa07c19 Compare December 27, 2018 10:18

golangcibot reviewed Dec 27, 2018

View reviewed changes

security/pkg/nodeagent/caclient/providers/vault/client_test.go Outdated Show resolved Hide resolved

security/pkg/nodeagent/caclient/providers/vault/client_test.go Outdated Show resolved Hide resolved

security/pkg/nodeagent/caclient/providers/vault/client.go Show resolved Hide resolved

lei-tang force-pushed the vault_integ_release_1.1 branch from fa07c19 to b1fdcb0 Compare December 27, 2018 10:25

quanjielin reviewed Dec 28, 2018

View reviewed changes

lei-tang changed the title ~~[Work-in-progress] Istio CA integrates with Vault CA providers~~ Istio CA integrates with Vault CA providers Dec 31, 2018

myidpt reviewed Dec 31, 2018

View reviewed changes

lei-tang added 9 commits January 1, 2019 10:36

Istio integrates with Vault CA providers

7ac9076

Add tests to the Vault CA client

cfd9369

Add the support to TLS Vault connection

755e520

Add a test for a TLS Vault server

b30130e

Change to use a Vault TLS server in the endpoints-jenkins project

a081611

Change the vault client name

fc28b9d

Fix goimports error and logf statements

3f36d6c

Configure the test Vault server

9c41b03

Change naming in client.go

e0bf1fe

lei-tang force-pushed the vault_integ_release_1.1 branch from f212a88 to 0f0fa0c Compare January 1, 2019 03:22

Add flags of Vault CA config

0a47c84

lei-tang force-pushed the vault_integ_release_1.1 branch from 0f0fa0c to 0a47c84 Compare January 1, 2019 03:37

istio-testing assigned myidpt Jan 1, 2019

istio-testing added the lgtm label Jan 1, 2019

istio-testing added the approved label Jan 1, 2019

istio-testing merged commit f97a89f into istio:release-1.1 Jan 1, 2019

myidpt changed the title ~~Istio CA integrates with Vault CA providers~~ Istio Citadel Agent integrates with Vault CA providers Jan 1, 2019

quanjielin reviewed Jan 2, 2019

View reviewed changes

Istio Citadel Agent integrates with Vault CA providers #10638

Istio Citadel Agent integrates with Vault CA providers #10638

Conversation

lei-tang commented Dec 22, 2018 • edited

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

myidpt Dec 31, 2018 • edited

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

lei-tang commented Dec 31, 2018

lei-tang commented Dec 31, 2018

myidpt left a comment

Choose a reason for hiding this comment

istio-testing commented Jan 1, 2019

myidpt commented Jan 1, 2019

istio-testing commented Jan 1, 2019

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

lei-tang commented Dec 22, 2018 •

edited

myidpt Dec 31, 2018 •

edited