Add UpstreamAuthority 'vault' plugin as built-in #1611

hiyosi · 2020-06-03T02:54:23Z

Pull Request check list

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality

UpstreamAuthority plugin

Description of change

Add a new UpstreamAuthority vault plugin.
This plugin treats the Vault PKI Secret Engine as an Upstream PKI and issues intermediate CA certificates for the SPIRE Server.

Which issue this PR fixes

azdagron

Thank you @hiyosi for the PR! This will be exciting to get merged. Please feel free to reach out if there is any question or concerns with any suggestions or comments.

azdagron · 2020-06-05T14:45:47Z

doc/plugin_server_upstreamauthority_vault.md

+The vault plugin requests to sign intermediate certificate (for signing X.509 SVIDs)to Vault PKI Engine.
+The plugin doesn't support `PublishJWTKeyRequest` and `PublishJWTKeyResponse`, 
+this means that you **SHOULD NOT** use `JWT SVID` in multiple SPIRE environment with this plugin.   


Suggested change

The vault plugin requests to sign intermediate certificate (for signing X.509 SVIDs)to Vault PKI Engine.

The plugin doesn't support `PublishJWTKeyRequest` and `PublishJWTKeyResponse`,

this means that you **SHOULD NOT** use `JWT SVID` in multiple SPIRE environment with this plugin.

The vault plugin signs intermediate CA certificates for SPIRE using the Vault PKI Engine.

The plugin does not support the `PublishJWTKey` RPC and is therefore not appropriate for use in nested SPIRE topologies where JWT-SVIDs are in use.

azdagron · 2020-06-05T15:00:14Z

doc/plugin_server_upstreamauthority_vault.md

+
+| key | type | required | description | default |
+|:----|:-----|:---------|:------------|:--------|
+| vault_addr  | string |   | A URL of Vault server. (e.g., https://vault.example.com:8443/) | `${VAULT_ADDR}` |


I know the vault client takes a URL, but I wonder if our configuration should be a little more strict and just take a host with optional port (defaulting to 443) so that we can built the URL ourselves and force https://? Is there ever a need for the URL to have anything other than the host and port, e.g., userinfo or a path component?

If we keep this as-is, I'd suggest the the following:

Suggested change

| vault_addr | string | | A URL of Vault server. (e.g., https://vault.example.com:8443/) | `${VAULT_ADDR}` |

| vault_addr | string | | The URL of the Vault server. (e.g., https://vault.example.com:8443/) | `${VAULT_ADDR}` |

Is there ever a need for the URL to have anything other than the host and port, e.g., userinfo or a path component?

I've read in the documentation(https://www.vaultproject.io/docs/auth), it doesn't seem to be necessary .
Therefore I think we can set a host and port instead of the URL.

I've tried to fix it, please check.
4394dd6

azdagron · 2020-06-05T15:00:28Z

doc/plugin_server_upstreamauthority_vault.md

+| key | type | required | description | default |
+|:----|:-----|:---------|:------------|:--------|
+| vault_addr  | string |   | A URL of Vault server. (e.g., https://vault.example.com:8443/) | `${VAULT_ADDR}` |
+| pki_mount_point  | string |  | Name of mount point where PKI secret engine is mounted | pki |


azdagron · 2020-06-05T15:04:50Z

doc/plugin_server_upstreamauthority_vault.md

+|:----|:-----|:---------|:------------|:--------|
+| vault_addr  | string |   | A URL of Vault server. (e.g., https://vault.example.com:8443/) | `${VAULT_ADDR}` |
+| pki_mount_point  | string |  | Name of mount point where PKI secret engine is mounted | pki |
+| ca_cert_path     | string |  | Path to a CA certificate file that the client verifies the server certificate. Only PEM format is supported. | `${VAULT_CACERT}` |


Suggested change

| ca_cert_path | string | | Path to a CA certificate file that the client verifies the server certificate. Only PEM format is supported. | `${VAULT_CACERT}` |

| ca_cert_path | string | | Path to a CA certificate file used to verify the Vault server certificate. Only PEM format is supported. | `${VAULT_CACERT}` |

azdagron · 2020-06-05T15:07:00Z

doc/plugin_server_upstreamauthority_vault.md

+| vault_addr  | string |   | A URL of Vault server. (e.g., https://vault.example.com:8443/) | `${VAULT_ADDR}` |
+| pki_mount_point  | string |  | Name of mount point where PKI secret engine is mounted | pki |
+| ca_cert_path     | string |  | Path to a CA certificate file that the client verifies the server certificate. Only PEM format is supported. | `${VAULT_CACERT}` |
+| tls_skip_verify  | string |  | If true, vault client accepts any server certificates | false |


azdagron · 2020-06-05T16:50:46Z

pkg/server/plugin/upstreamauthority/vault/vault_test.go

+	csr, err := ioutil.ReadFile(testReqCSR)
+	vps.Require().NoError(err)
+
+	req, err := getTestMintX509CARequest(csr)
+	vps.Require().NoError(err)


We do these two steps together a lot. Can we just have one helper that loads from a file and creates the request? If it is a member of the suite then it can even assert success directly instead of returning an error that each test has to assert individually:

Suggested change

csr, err := ioutil.ReadFile(testReqCSR)

vps.Require().NoError(err)

req, err := getTestMintX509CARequest(csr)

vps.Require().NoError(err)

req := vps.loadMintX509CARequestFromTestFile(testReqCSR)

I agree.
I added a helper func (vps *VaultPluginSuite) loadMintX509CARequestFromTestFile(csrFile string) *upstreamauthority.MintX509CARequest { ... }
3fc46ac

azdagron · 2020-06-05T20:47:56Z

pkg/server/plugin/upstreamauthority/vault/vault_test.go

+	s, addr, err := vps.fakeVaultServer.NewTLSServer()
+	vps.Require().NoError(err)
+
+	s.Start()
+	defer s.Close()
+
+	p := New()
+	p.SetLogger(hclog.Default())


This is a lot of boilerplate for each test case do repeat. Is there a way we can DRY this up a little bit?

I added a helper function func (vps *VaultPluginSuite) newPlugin() *Plugin { ... }
3fc46ac

azdagron · 2020-06-05T20:57:07Z

pkg/server/plugin/upstreamauthority/vault/vault_test.go

+	for i := range res.X509CaChain {
+		cert, err := x509.ParseCertificate(res.X509CaChain[i])


Suggested change

for i := range res.X509CaChain {

cert, err := x509.ParseCertificate(res.X509CaChain[i])

for _, certDER := range res.X509CaChain {

cert, err := x509.ParseCertificate(certDER)

azdagron · 2020-06-05T20:57:37Z

pkg/server/plugin/upstreamauthority/vault/vault_test.go

+	for i := range res.UpstreamX509Roots {
+		upstream, err := x509.ParseCertificate(res.UpstreamX509Roots[i])


Suggested change

for i := range res.UpstreamX509Roots {

upstream, err := x509.ParseCertificate(res.UpstreamX509Roots[i])

for _, upstreamDER := range res.UpstreamX509Roots {

upstream, err := x509.ParseCertificate(upstreamDER)

azdagron · 2020-06-05T20:59:09Z

pkg/server/plugin/upstreamauthority/vault/vault_test.go

+	spiretest.Run(t, new(VaultPluginSuite))
+}
+
+type VaultPluginSuite struct {


The structure of these tests might align better with table-driven tests, but that's something we can improve later.

I agree with some cases are better with table driven tests.
If we should improve the tests within this PR, I think I can do.

hiyosi · 2020-06-08T08:58:05Z

@azdagron
Awesome! Thank you for your review!!
I fixed and commit you pointed out. Please check it again.
(I committed with fixup! , so they should be rebased before merging.)

azdagron

Thank you for addressing the comments, @hiyosi! Just a few more comments and I think this is good.

azdagron · 2020-06-08T20:18:18Z

doc/plugin_server_upstreamauthority_vault.md

+| key | type | required | description | default |
+|:----|:-----|:---------|:------------|:--------|
+| vault_host  | string |   | A Hostname of the Vault server or combination of Hostname and Port. | `${VAULT_HOST}` |
+| vault_path_prefix | string |  | A URL Path Prefix for the Vault API. e.g., If the Vault places behind the reverse proxy, it is possible to customize the URL Path.. e.g., the Vault places behind the reverse proxy, it is possible to customize the URL Path base. ( <vault_path_prefix>/v1/api/...) | `${VAULT_PATH_PREFIX}` |  


My concern about vault_addr was that it required a URL but all that it seemed practical for people to change would be the host and port, since the scheme should be https and I didn't see a reason to specify a path.

If deploying behind behind a reverse proxy is a normal thing for people to do, where there might be a need to have a path prefix, and considering that the vault client APIs document the configurable as being a URL then maybe the original configurable was good? 😬

OK. I'll revert the commits.
In original case, Should we check the URI scheme ?
e.g., If it's not https, outputs some warn logs.

revert: 73782ba

azdagron · 2020-06-08T20:20:01Z

pkg/server/plugin/upstreamauthority/vault/renewer.go

+	Logger hclog.Logger
+}
+
+func NewRenew(logger hclog.Logger) *Renew {


I'm ok with the original code here, i.e., passing everything needed to construct the renewer in this function, I just thought maybe the logger could also be passed as a parameter instead of using a default logger.

I understand the intent of the your comment😀
I think there is no problem with passing all the argument including logger, so I'll fix it.

fix: 8e74413

azdagron

This is great. Thanks for this contribution, @hiyosi !

azdagron · 2020-06-09T15:51:11Z

@hiyosi, would you mind squashing and rebasing this? I'm happy to merge afterwards. Thanks!

azdagron · 2020-06-09T22:54:08Z

I apologize, I didn't see you had updated this and merged a different PR. Can you rebase again?

Signed-off-by: Tomoya Usami <tousami@zlab.co.jp>

amartinezfayo

Hi @hiyosi, thank you for this contribution. This is really great!
I have a couple of suggestions that I missed when I first looked at this. I'm commenting here but I'm happy to move this to an issue if you feel that is appropriate. Thanks again!

amartinezfayo · 2020-06-17T16:47:20Z

pkg/server/plugin/upstreamauthority/vault/vault.go

+	// Name of the mount point where PKI secret engine is mounted. (e.g., /<mount_point>/ca/pem)
+	PKIMountPoint string `hcl:"pki_mount_point"`
+	// Configuration for the Token authentication method
+	TokenAuth TokenAuthConfig `hcl:"token_auth"`


I'm wondering if having pointers to the config structs would be better. Looks like the configuration settings in all authentication methods have either default values or can be alternatively defined through environment variables, which makes empty auth configs valid. I.e.: I want to use TOKEN authentication method and I have defined the VAULT_TOKEN environment variable. In that case I could just have token_auth { } as a valid configuration, but currently there is no way to identify that I set an empty token_auth config so the configuration would be rejected even if I have set the environment variable. If we use a pointer we may check if config.TokenAuth != nil in parseAuthMethod.

@amartinezfayo
Thank you for your suggestion.
I understand the problem. I'll fix it.

amartinezfayo · 2020-06-17T17:45:00Z

pkg/server/plugin/upstreamauthority/vault/vault.go

+		return APPROLE, nil
+	}
+
+	return 0, errors.New("must be configured one of these authentication method 'Token or Cert or AppRole'")


We may validate that only one of the auth methods is set?

amartinezfayo · 2020-06-17T18:02:47Z

doc/plugin_server_upstreamauthority_vault.md

+| token_auth       | struct |  | Configuration for the Token authentication method | |
+| approle_auth     | struct |  | Configuration for the AppRole authentication method | |
+
+The plugin supports **Client Certificate**, **Token** and **AppRole** authentication methods.


Should we document any permissions required depending on the authentication method? E.g.: if a token is used, does the token need to be attached to a policy with certain capabilities?

I agree with you.
I'll try to improve the document.

hiyosi requested review from ajessup, amartinezfayo, APTy, azdagron, evan2645 and mcpherrinm as code owners June 3, 2020 02:54

hiyosi force-pushed the builtin-vault-upstream-authority branch 3 times, most recently from 130582b to e1759e3 Compare June 3, 2020 05:53

azdagron reviewed Jun 5, 2020

View reviewed changes

hiyosi force-pushed the builtin-vault-upstream-authority branch 3 times, most recently from f9e029e to 4394dd6 Compare June 8, 2020 08:02

azdagron reviewed Jun 8, 2020

View reviewed changes

hiyosi force-pushed the builtin-vault-upstream-authority branch from 2fc7301 to 73782ba Compare June 9, 2020 00:44

azdagron previously approved these changes Jun 9, 2020

View reviewed changes

hiyosi dismissed azdagron’s stale review via d19f9bf June 9, 2020 22:42

hiyosi force-pushed the builtin-vault-upstream-authority branch 2 times, most recently from d19f9bf to a0da240 Compare June 9, 2020 22:47

Add vault upstreamauthority plugin as built-in

cbb5c01

Signed-off-by: Tomoya Usami <tousami@zlab.co.jp>

hiyosi force-pushed the builtin-vault-upstream-authority branch from 7ab4b27 to cbb5c01 Compare June 9, 2020 23:32

azdagron approved these changes Jun 10, 2020

View reviewed changes

azdagron merged commit bfb201f into spiffe:master Jun 10, 2020

hiyosi deleted the builtin-vault-upstream-authority branch June 10, 2020 02:47

amartinezfayo reviewed Jun 17, 2020

View reviewed changes

This was referenced Jun 18, 2020

Make it possible to configure empty "vault" auth config #1664

Merged

Update the "vault" plugin document. #1665

Merged

anvega mentioned this pull request Jun 19, 2020

Update CHANGELOG.md with v0.10.1 release #1655

Merged

InverseIntegral mentioned this pull request Sep 18, 2024

Add HashiCorp Vault key manager plugin to SPIRE server #5500

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UpstreamAuthority 'vault' plugin as built-in #1611

Add UpstreamAuthority 'vault' plugin as built-in #1611

hiyosi commented Jun 3, 2020

azdagron left a comment

azdagron Jun 5, 2020

azdagron Jun 5, 2020

hiyosi Jun 8, 2020 •

edited

Loading

hiyosi Jun 8, 2020

azdagron Jun 5, 2020

azdagron Jun 5, 2020

azdagron Jun 5, 2020

azdagron Jun 5, 2020

hiyosi Jun 8, 2020 •

edited

Loading

azdagron Jun 5, 2020

hiyosi Jun 8, 2020 •

edited

Loading

azdagron Jun 5, 2020

azdagron Jun 5, 2020

azdagron Jun 5, 2020

hiyosi Jun 8, 2020

hiyosi commented Jun 8, 2020 •

edited

Loading

azdagron left a comment

azdagron Jun 8, 2020

hiyosi Jun 9, 2020

hiyosi Jun 9, 2020

azdagron Jun 8, 2020

hiyosi Jun 8, 2020 •

edited

Loading

hiyosi Jun 9, 2020

azdagron left a comment

azdagron commented Jun 9, 2020

azdagron commented Jun 9, 2020

amartinezfayo left a comment

amartinezfayo Jun 17, 2020

hiyosi Jun 17, 2020

amartinezfayo Jun 17, 2020

amartinezfayo Jun 17, 2020

hiyosi Jun 17, 2020

	\| vault_addr \| string \| \| A URL of Vault server. (e.g., https://vault.example.com:8443/) \| `${VAULT_ADDR}` \|
	\| vault_addr \| string \| \| The URL of the Vault server. (e.g., https://vault.example.com:8443/) \| `${VAULT_ADDR}` \|

	\| pki_mount_point \| string \| \| Name of mount point where PKI secret engine is mounted \| pki \|
	\| pki_mount_point \| string \| \| Name of the mount point where PKI secret engine is mounted \| pki \|

	\| ca_cert_path \| string \| \| Path to a CA certificate file that the client verifies the server certificate. Only PEM format is supported. \| `${VAULT_CACERT}` \|
	\| ca_cert_path \| string \| \| Path to a CA certificate file used to verify the Vault server certificate. Only PEM format is supported. \| `${VAULT_CACERT}` \|

	\| tls_skip_verify \| string \| \| If true, vault client accepts any server certificates \| false \|
	\| insecure_skip_verify \| string \| \| If true, the Vault server certificate is not verified (not for production use) \| false \|

		for i := range res.X509CaChain {
		cert, err := x509.ParseCertificate(res.X509CaChain[i])

		for i := range res.UpstreamX509Roots {
		upstream, err := x509.ParseCertificate(res.UpstreamX509Roots[i])

Add UpstreamAuthority 'vault' plugin as built-in #1611

Add UpstreamAuthority 'vault' plugin as built-in #1611

Conversation

hiyosi commented Jun 3, 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

hiyosi Jun 8, 2020 • 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

hiyosi Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiyosi Jun 8, 2020 • 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

hiyosi commented Jun 8, 2020 • edited Loading

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

hiyosi Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azdagron left a comment

Choose a reason for hiding this comment

azdagron commented Jun 9, 2020

azdagron commented Jun 9, 2020

amartinezfayo 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

hiyosi Jun 8, 2020 •

edited

Loading

hiyosi Jun 8, 2020 •

edited

Loading

hiyosi Jun 8, 2020 •

edited

Loading

hiyosi commented Jun 8, 2020 •

edited

Loading

hiyosi Jun 8, 2020 •

edited

Loading