Skip to content

connect: Enable renewing the intermediate cert in the primary DC #8784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 9, 2020
Merged

connect: Enable renewing the intermediate cert in the primary DC #8784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2ec94b0
connect: Enable renewing the intermediate cert in the primary DC
kyhavlov Sep 30, 2020
e13f4af
connect: Check for expired root cert when cross-signing
kyhavlov Oct 9, 2020
4fc0f6d
Merge branch 'master' into renew-intermediate-primary
kyhavlov Oct 9, 2020
01ce9f5
Update CI for leader renew CA test using Vault
kyhavlov Oct 9, 2020
cc901df
Add changelog note
kyhavlov Oct 9, 2020
876500e
Fix intermediate refresh test comments
kyhavlov Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/8784.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
connect: Fixed an issue where the Vault intermediate was not renewed in the primary datacenter.
```
2 changes: 2 additions & 0 deletions GNUmakefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,8 @@ test-connect-ca-providers:
ifeq ("$(CIRCLECI)","true")
# Run in CI
gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report.xml" -- -cover -coverprofile=coverage.txt ./agent/connect/ca
# Run leader tests that require Vault
gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-leader.xml" -- -cover -coverprofile=coverage-leader.txt -run TestLeader_Vault_ ./agent/consul
Comment on lines +360 to +361
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try and move these tests that require vault into the agent/connect/ca or test/integration packages at some point. But I don't see any reason to block merging this PR. We can revisit once we have a better idea of how we would like to organize our integration tests.

I'm not sure if this extra coverage file will get sent to codecov, I suspect only the coverage.txt will get sent, but that's something we can fix in a follow up as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely feels strange in agent/consul, although it does test internal leader logic. It couldn't go directly in agent/connect/ca but maybe creating a new subpackage in one of these would be the way to go (like agent/consul/integration or something similar, that has tests like this which involve leader functionality and something external like a CA).

else
# Run locally
@echo "Running /agent/connect/ca tests in verbose mode"
Expand Down
7 changes: 7 additions & 0 deletions agent/connect/ca/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ import (
// on servers and CA provider.
var ErrRateLimited = errors.New("operation rate limited by CA provider")

// PrimaryIntermediateProviders is a list of CA providers that make use use of an
// intermediate cert in the primary datacenter as well as the secondary. This is used
// when determining whether to run the intermediate renewal routine in the primary.
var PrimaryIntermediateProviders = map[string]struct{}{
"vault": {},
}
Comment on lines +22 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like implementing this kind of thing as a method on the type. Then calling code can use an interface to check if the type has the method:

if _, ok := provider.(interface{
    IntermediateCertInPrimaryDC()
}); ok {
   ...
}

But I think we could easily change that later, so I don't want to hold up this bug fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I overlooked that pattern when thinking about this 👍


// ProviderConfig encapsulates all the data Consul passes to `Configure` on a
// new provider instance. The provider must treat this as read-only and make
// copies of any map or slice if it might modify them internally.
Expand Down
7 changes: 3 additions & 4 deletions agent/connect/ca/provider_consul.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,13 @@ import (
"github.com/hashicorp/go-hclog"
)

const (

var (
// NotBefore will be CertificateTimeDriftBuffer in the past to account for
// time drift between different servers.
CertificateTimeDriftBuffer = time.Minute
)

var ErrNotInitialized = errors.New("provider not initialized")
ErrNotInitialized = errors.New("provider not initialized")
)

type ConsulProvider struct {
Delegate ConsulProviderStateDelegate
Expand Down
17 changes: 16 additions & 1 deletion agent/connect/ca/provider_vault.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"io/ioutil"
"net/http"
"strings"
"time"

"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/structs"
Expand Down Expand Up @@ -384,6 +385,7 @@ func (v *VaultProvider) GenerateIntermediate() (string, error) {
"csr": csr,
"use_csr_values": true,
"format": "pem_bundle",
"ttl": v.config.IntermediateCertTTL.String(),
})
if err != nil {
return "", err
Expand Down Expand Up @@ -456,6 +458,7 @@ func (v *VaultProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
"use_csr_values": true,
"format": "pem_bundle",
"max_path_length": 0,
"ttl": v.config.IntermediateCertTTL.String(),
})
if err != nil {
return "", err
Expand All @@ -475,8 +478,20 @@ func (v *VaultProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
// CrossSignCA takes a CA certificate and cross-signs it to form a trust chain
// back to our active root.
func (v *VaultProvider) CrossSignCA(cert *x509.Certificate) (string, error) {
rootPEM, err := v.ActiveRoot()
if err != nil {
return "", err
}
rootCert, err := connect.ParseCert(rootPEM)
if err != nil {
return "", fmt.Errorf("error parsing root cert: %v", err)
}
if rootCert.NotAfter.Before(time.Now()) {
return "", fmt.Errorf("root certificate is expired")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that means that Vault would happily sign a cert with an expired root CA?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it's because the sign-self-issued endpoint does minimal validation (only checks for CA cert and self-issued) so it was possible for the cert to expire and the provider would still cross-sign with it.


var pemBuf bytes.Buffer
err := pem.Encode(&pemBuf, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
err = pem.Encode(&pemBuf, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
if err != nil {
return "", err
}
Expand Down
163 changes: 11 additions & 152 deletions agent/connect/ca/provider_vault_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,11 @@ import (
"encoding/json"
"fmt"
"io/ioutil"
"os"
"os/exec"
"sync"
"testing"
"time"

"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/sdk/freeport"
"github.com/hashicorp/consul/sdk/testutil/retry"
"github.com/hashicorp/go-hclog"
vaultapi "github.com/hashicorp/vault/api"
Expand Down Expand Up @@ -42,7 +38,7 @@ func TestVaultCAProvider_VaultTLSConfig(t *testing.T) {
func TestVaultCAProvider_SecondaryActiveIntermediate(t *testing.T) {
t.Parallel()

skipIfVaultNotPresent(t)
SkipIfVaultNotPresent(t)

provider, testVault := testVaultProviderWithConfig(t, false, nil)
defer testVault.Stop()
Expand All @@ -55,7 +51,7 @@ func TestVaultCAProvider_SecondaryActiveIntermediate(t *testing.T) {

func TestVaultCAProvider_RenewToken(t *testing.T) {
t.Parallel()
skipIfVaultNotPresent(t)
SkipIfVaultNotPresent(t)

testVault, err := runTestVault(t)
require.NoError(t, err)
Expand All @@ -70,7 +66,7 @@ func TestVaultCAProvider_RenewToken(t *testing.T) {
require.NoError(t, err)
providerToken := secret.Auth.ClientToken

_, err = createVaultProvider(t, true, testVault.addr, providerToken, nil)
_, err = createVaultProvider(t, true, testVault.Addr, providerToken, nil)
require.NoError(t, err)

// Check the last renewal time.
Expand All @@ -92,7 +88,7 @@ func TestVaultCAProvider_RenewToken(t *testing.T) {
func TestVaultCAProvider_Bootstrap(t *testing.T) {
t.Parallel()

skipIfVaultNotPresent(t)
SkipIfVaultNotPresent(t)

provider, testVault := testVaultProvider(t)
defer testVault.Stop()
Expand Down Expand Up @@ -153,7 +149,7 @@ func assertCorrectKeyType(t *testing.T, want, certPEM string) {
func TestVaultCAProvider_SignLeaf(t *testing.T) {
t.Parallel()

skipIfVaultNotPresent(t)
SkipIfVaultNotPresent(t)

for _, tc := range KeyTestCases {
tc := tc
Expand Down Expand Up @@ -237,7 +233,7 @@ func TestVaultCAProvider_SignLeaf(t *testing.T) {
func TestVaultCAProvider_CrossSignCA(t *testing.T) {
t.Parallel()

skipIfVaultNotPresent(t)
SkipIfVaultNotPresent(t)

tests := CASigningKeyTypeCases()

Expand Down Expand Up @@ -292,7 +288,7 @@ func TestVaultCAProvider_CrossSignCA(t *testing.T) {
func TestVaultProvider_SignIntermediate(t *testing.T) {
t.Parallel()

skipIfVaultNotPresent(t)
SkipIfVaultNotPresent(t)

tests := CASigningKeyTypeCases()

Expand Down Expand Up @@ -321,7 +317,7 @@ func TestVaultProvider_SignIntermediate(t *testing.T) {
func TestVaultProvider_SignIntermediateConsul(t *testing.T) {
t.Parallel()

skipIfVaultNotPresent(t)
SkipIfVaultNotPresent(t)

// primary = Vault, secondary = Consul
t.Run("pri=vault,sec=consul", func(t *testing.T) {
Expand Down Expand Up @@ -382,19 +378,19 @@ func getIntermediateCertTTL(t *testing.T, caConf *structs.CAConfiguration) time.
return dur
}

func testVaultProvider(t *testing.T) (*VaultProvider, *testVaultServer) {
func testVaultProvider(t *testing.T) (*VaultProvider, *TestVaultServer) {
return testVaultProviderWithConfig(t, true, nil)
}

func testVaultProviderWithConfig(t *testing.T, isPrimary bool, rawConf map[string]interface{}) (*VaultProvider, *testVaultServer) {
func testVaultProviderWithConfig(t *testing.T, isPrimary bool, rawConf map[string]interface{}) (*VaultProvider, *TestVaultServer) {
testVault, err := runTestVault(t)
if err != nil {
t.Fatalf("err: %v", err)
}

testVault.WaitUntilReady(t)

provider, err := createVaultProvider(t, isPrimary, testVault.addr, testVault.rootToken, rawConf)
provider, err := createVaultProvider(t, isPrimary, testVault.Addr, testVault.RootToken, rawConf)
if err != nil {
testVault.Stop()
t.Fatalf("err: %v", err)
Expand Down Expand Up @@ -443,140 +439,3 @@ func createVaultProvider(t *testing.T, isPrimary bool, addr, token string, rawCo

return provider, nil
}

// skipIfVaultNotPresent skips the test if the vault binary is not in PATH.
//
// These tests may be skipped in CI. They are run as part of a separate
// integration test suite.
func skipIfVaultNotPresent(t *testing.T) {
vaultBinaryName := os.Getenv("VAULT_BINARY_NAME")
if vaultBinaryName == "" {
vaultBinaryName = "vault"
}

path, err := exec.LookPath(vaultBinaryName)
if err != nil || path == "" {
t.Skipf("%q not found on $PATH - download and install to run this test", vaultBinaryName)
}
}

func runTestVault(t *testing.T) (*testVaultServer, error) {
vaultBinaryName := os.Getenv("VAULT_BINARY_NAME")
if vaultBinaryName == "" {
vaultBinaryName = "vault"
}

path, err := exec.LookPath(vaultBinaryName)
if err != nil || path == "" {
return nil, fmt.Errorf("%q not found on $PATH", vaultBinaryName)
}

ports := freeport.MustTake(2)
returnPortsFn := func() {
freeport.Return(ports)
}

var (
clientAddr = fmt.Sprintf("127.0.0.1:%d", ports[0])
clusterAddr = fmt.Sprintf("127.0.0.1:%d", ports[1])
)

const token = "root"

client, err := vaultapi.NewClient(&vaultapi.Config{
Address: "http://" + clientAddr,
})
if err != nil {
returnPortsFn()
return nil, err
}
client.SetToken(token)

args := []string{
"server",
"-dev",
"-dev-root-token-id",
token,
"-dev-listen-address",
clientAddr,
"-address",
clusterAddr,
}

cmd := exec.Command(vaultBinaryName, args...)
cmd.Stdout = ioutil.Discard
cmd.Stderr = ioutil.Discard
if err := cmd.Start(); err != nil {
returnPortsFn()
return nil, err
}

testVault := &testVaultServer{
rootToken: token,
addr: "http://" + clientAddr,
cmd: cmd,
client: client,
returnPortsFn: returnPortsFn,
}
t.Cleanup(func() {
testVault.Stop()
})
return testVault, nil
}

type testVaultServer struct {
rootToken string
addr string
cmd *exec.Cmd
client *vaultapi.Client

// returnPortsFn will put the ports claimed for the test back into the
returnPortsFn func()
}

var printedVaultVersion sync.Once

func (v *testVaultServer) WaitUntilReady(t *testing.T) {
var version string
retry.Run(t, func(r *retry.R) {
resp, err := v.client.Sys().Health()
if err != nil {
r.Fatalf("err: %v", err)
}
if !resp.Initialized {
r.Fatalf("vault server is not initialized")
}
if resp.Sealed {
r.Fatalf("vault server is sealed")
}
version = resp.Version
})
printedVaultVersion.Do(func() {
fmt.Fprintf(os.Stderr, "[INFO] agent/connect/ca: testing with vault server version: %s\n", version)
})
}

func (v *testVaultServer) Stop() error {
// There was no process
if v.cmd == nil {
return nil
}

if v.cmd.Process != nil {
if err := v.cmd.Process.Signal(os.Interrupt); err != nil {
return fmt.Errorf("failed to kill vault server: %v", err)
}
}

// wait for the process to exit to be sure that the data dir can be
// deleted on all platforms.
if err := v.cmd.Wait(); err != nil {
return err
}

if v.returnPortsFn != nil {
v.returnPortsFn()
}

return nil
}
Loading