Vault 32676 add vault build date to system view plugin env #29082

helenfufu · 2024-12-03T20:52:35Z

Description

This PR adds the Vault information (build date and builtin public keys) required for license checking in external plugins to the SystemView interface via the PluginEnv function. We:

Extend PluginEnvironment to include VaultBuildDate.
Add a GetVaultBuildDate helper function in the version package to be shared across CE and ENT. In the subsequent ENT PR, we plan to remove the existing, ENT-only vault.GetVaultBuildDate function and replace usages of it with this new, shared version.GetVaultBuildDate.
Add dynamicSystemView unit tests.

Related ENT PR: https://github.com/hashicorp/vault-enterprise/pull/7088 (which needs to be rebased off main after these CE changes are brought into ENT main)
Ticket: VAULT-32676
RFC: https://go.hashi.co/rfc/vlt-337

Local testing:

Tested enabling external plugin with old SDK and new Vault (new SDK)

# Build vault and ensure it's copied to $GOPATH/bin.
$ cd vault
$ git checkout vault-32676-add-vault-license-check-info-to-system-view-plugin-env
$ make dev
$ which vault
$ ls $GOPATH/bin | grep vault
$ vault version

# Build vcm with plugin-add capabilities and ensure it's copied to $GOPATH/bin.
$ cd vcm
$ git checkout plugin-add
$ which vcm
$ ls -alh ~/go/bin | grep vcm

# Create a 3-node Vault cluster
$ ls ~/.vcm
$ vcm cluster three-node vault-32676
$ vcm cluster status vault-32676
$ vcm node env -active-node -cluster vault-32676
$ export VAULT_TOKEN=redacted;export VAULT_ADDR=http://127.0.0.1:8200;
$ vault status
Key                     Value
---                     -----
Seal Type               shamir
Initialized             true
Sealed                  false
Total Shares            1
Threshold               1
Version                 1.19.0-beta1
Build Date              2024-12-03T14:05:26Z
Storage Type            raft
Cluster Name            vault-cluster-411aa9d3
Cluster ID              478296cc-b790-3fd7-8049-4303d2877ce4
Removed From Cluster    false
HA Enabled              true
HA Cluster              https://127.0.0.1:8201
HA Mode                 active
Active Since            2024-12-03T14:34:51.78506-08:00
Raft Committed Index    56
Raft Applied Index      56

# Validate I can register and enable an external plugin with old SDK and new Vault (new SDK)
$ vcm plugin add -cluster vault-32676 -version=0.20.0 vault-plugin-secrets-kv
$ vault plugin list | grep kv
kv                                   secret      v0.20.0+builtin
vault-plugin-secrets-kv              secret      v0.20.0
$ vault secrets enable -path=test-kv vault-plugin-secrets-kv
Success! Enabled the vault-plugin-secrets-kv secrets engine at: test-kv/
$ vault kv put test-kv/hello target=world
Success! Data written to: test-kv/hello
$ vault kv get test-kv/hello
===== Data =====
Key       Value
---       -----
target    world

TODO only if you're a HashiCorp employee

Backport Labels: If this fix needs to be backported, use the appropriate backport/ label that matches the desired release branch. Note that in the CE repo, the latest release branch will look like backport/x.x.x, but older release branches will be backport/ent/x.x.x+ent.
- LTS: If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
See vault-enterprise#7086 for ENT tests run with these CE changes. All tests passed.
Jira: If this change has an associated Jira, it's referenced either
in the PR description, commit message, or branch name.
RFC: If this change has an associated RFC, please link it in the description.
ENT PR: If this change has an associated ENT PR, please link it in the
description. Also, make sure the changelog is in this PR, not in your ENT PR.

github-actions · 2024-12-03T21:05:05Z

CI Results:
All Go tests succeeded! ✅

…o replace existing ent-specific vault.GetVaultBuildDate)

…work)

…ccessful no longer needs to override version.BuildDate

… with ent

helenfufu

Leaving a few notes around decision-making that hopefully help with review

helenfufu · 2024-12-04T01:32:24Z

vault/dynamic_system_view_test.go

@@ -286,6 +288,51 @@ func TestDynamicSystemView_GeneratePasswordFromPolicy_failed(t *testing.T) {
 	}
 }

+// TestDynamicSystemView_PluginEnv_successful checks that the PluginEnv method returns the expected values in a successful case.
+func TestDynamicSystemView_PluginEnv_successful(t *testing.T) {


Two notes:

I followed the naming convention for separate success/failure tests from the above test (i.e.TestDynamicSystemView_GeneratePasswordFromPolicy_successful and TestDynamicSystemView_GeneratePasswordFromPolicy_failed). Open to renaming if consistency is less important.

I also omitted a failure case, because the only failure point in PluginEnv is failure to parse the result of GetVaultBuildDate(). I looked into overriding version.BuildDate as it's done in an existing test TestGetSealStatus_RedactionSettings, but found that this is incompatible with the ENT repo whose licensing tests all rely on this date being set. Happy to work/pair on a failure case if anyone has ideas!

I also omitted a failure case, because the only failure point in PluginEnv is failure to parse the result of GetVaultBuildDate()

Good point

helenfufu · 2024-12-04T01:33:08Z

vault/testing_util.go

+func init() {
+	// The BuildDate is set as part of the build process in CI so we need to
+	// initialize it for testing.
+	if version.BuildDate == "" {
+		version.BuildDate = time.Now().UTC().Format(time.RFC3339)
+	}
+}


This is needed so a bunch of existing unit tests will pass without failing with failed to parse build date based on RFC3339: parsing time "" as "2006-01-02T15:04:05Z07:00": cannot parse "" as "2006". The pattern is copied from testing_util_ent.go in the ENT repo where we initialize the version.BuildDate to 1 year ago from now (to account for license timing). In the CE repo, we can just initialize to now.

If CE's version.BuildDate initialized to 1 year ago from now also works, I wonder if it makes sense to

rename testing_util.go to testing_util_stubs_oss.go

put init() of testing_util_ent.go in a brand new testing_util.go, where all shared functions between CE and ENT repos will be stored.

If CE's version.BuildDate initialized to 1 year ago from now also works

It should work - just pushed a commit to test it out so we can confirm. ceb7506

I wonder if it makes sense to

rename testing_util.go to testing_util_stubs_oss.go

put init() of testing_util_ent.go in a brand new testing_util.go, where all shared functions between CE and ENT repos will be stored.

Makes sense to me! Will keep an eye on the tests, and once they wrap, add a commit with the refactoring changes and update here.

Setting CE's version.BuildDate to 1 year ago worked. Here's the commit fe7bf90 that renames testing_util.go to testing_util_stubs_oss.go and moves init BuildDate to a new shared testing_util.go.

After making the CE changes, checked the following for ENT pieces:

re-ran the ENT tests on the latest pure CE changes to check there's no ENT breakage (https://github.com/hashicorp/vault-enterprise/pull/7086)

updated the ENT PR with the latest from the CE PR plus the additional change to remove the ENT-only testing_util_ent.go init so we rely on the new shared one in testing_util.go(https://github.com/hashicorp/vault-enterprise/pull/7088)

The ENT PR with the latest pure CE changes is failing with a data race condition (I re-ran once, and it's still failing), but the ENT PR with all our planned changes passes all tests. The failing tests don't look super related to this PR, but I wonder if the data race checks for re-initializing global variables, because after merging this CE PR but before merging the ENT PR, there will temporarily be two init() functions setting version.BuildDate to time.Now().UTC().AddDate(-1, 0, 0).Format(time.RFC3339). Thoughts:

This race condition seems insignificant, since version.BuildDate is set at build time. These two init() functions overwriting each other would only impact unit tests for the brief period after merging the CE PR and before merging the ENT PR, which seems like very limited impact. I'm not sure if it counts as ENT breakage though.

If this counts as ENT breakage and too complicated to sort out in light of time, reverting the most recent testing_util commit should avoid this problem, though ofc that means we wouldn't be applying your suggestion.

I'm not sure if there's a merge order for this refactoring that can avoid introducing any race: it seems there's no way to avoid both init() functions being present after merging a CE PR to add a shared function, but before merging the ENT PR to remove the ENT-only one.

We synced and realized the data race condition is just a flaky test (failing on the same GetSecretEngineUsageMetrics race) mentioned in #feed-vault-ci-official. On top of that, I had actually forgotten to push the change that removes the ENT-only init() in the final ENT PR, so both branches had the two init() functions with one branch passing and one branch failing the data race, suggesting further that it's just a flaky test.

helenfufu · 2024-12-04T01:33:30Z

version/version.go

+func GetVaultBuildDate() (time.Time, error) {
+	buildDate, err := time.Parse(time.RFC3339, BuildDate)
+	if err != nil {
+		return time.Time{}, fmt.Errorf("failed to parse build date based on RFC3339: %w", err)
+	}
+	return buildDate, nil
+}


Per the PR description, this is a copy of the existing, ENT-only vault.GetVaultBuildDate helper but we're making it shared in this common version package now.

github-actions · 2024-12-04T01:39:15Z

Build Results:
All builds succeeded! ✅

thyton

Looks good from the initial pass

thyton · 2024-12-04T07:00:33Z

vault/testing_util.go

+func init() {
+	// The BuildDate is set as part of the build process in CI so we need to
+	// initialize it for testing.
+	if version.BuildDate == "" {
+		version.BuildDate = time.Now().UTC().Format(time.RFC3339)
+	}
+}


If CE's version.BuildDate initialized to 1 year ago from now also works, I wonder if it makes sense to

rename testing_util.go to testing_util_stubs_oss.go

put init() of testing_util_ent.go in a brand new testing_util.go, where all shared functions between CE and ENT repos will be stored.

thyton · 2024-12-04T07:09:48Z

vault/dynamic_system_view_test.go

@@ -286,6 +288,51 @@ func TestDynamicSystemView_GeneratePasswordFromPolicy_failed(t *testing.T) {
 	}
 }

+// TestDynamicSystemView_PluginEnv_successful checks that the PluginEnv method returns the expected values in a successful case.
+func TestDynamicSystemView_PluginEnv_successful(t *testing.T) {


I also omitted a failure case, because the only failure point in PluginEnv is failure to parse the result of GetVaultBuildDate()

Good point

…ldDate to new shared testing_util.go

…system-view-plugin-env

thyton · 2024-12-10T15:34:35Z

The PR originally added build date and Vault builtin public keys to system view's plugin env. The PR is updated to only add build date. Vault builtin public keys will be baked in Enterprise plugin binaries

--------- Co-authored-by: Thy Ton <[email protected]>

helenfufu requested a review from thyton December 3, 2024 20:52

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Dec 3, 2024

helenfufu added this to the 1.19.0-rc milestone Dec 3, 2024

helenfufu added 12 commits December 3, 2024 14:56

add VaultInfo rpc to SystemView service then make proto

022a2e2

implement VaultInfo in static, grpc, and dynamic system views

57c5178

add version.GetVaultBuildDate helper to be used in both ce and ent (t…

a918734

…o replace existing ent-specific vault.GetVaultBuildDate)

add dynamic system view unit tests

08edbaf

add VaultBuildDate and VaultBuiltinPublicKeys to PluginEnvironment proto

8df8a82

extend PluginEnv implementation in dynamic system view

027513f

remove VaultInfo protos and implementations

3d60701

update unit tests

f333edf

strb getter GetBuiltinPublicKeys instead of variable (which wouldn't …

806cdb4

…work)

add changelog

4447857

init version.BuildDate following testing_util_ent

3ef19e9

lint (add go doc to new tests) and TestDynamicSystemView_PluginEnv_su…

de115fb

…ccessful no longer needs to override version.BuildDate

helenfufu force-pushed the vault-32676-add-vault-license-check-info-to-system-view-plugin-env branch from dfd51f9 to de115fb Compare December 3, 2024 22:56

remove TestDynamicSystemView_PluginEnv_failed since it's incompatible…

f96ea93

… with ent

helenfufu added backport/ent/1.16.x+ent Changes are backported to 1.16.x+ent backport/1.18.x backport/ent/1.17.x+ent Changes are backported to 1.17.x+ent labels Dec 4, 2024

helenfufu marked this pull request as ready for review December 4, 2024 01:22

helenfufu requested a review from a team as a code owner December 4, 2024 01:22

helenfufu commented Dec 4, 2024

View reviewed changes

thyton reviewed Dec 4, 2024

View reviewed changes

try 1 year ago for testing_util init BuildDate

ceb7506

helenfufu force-pushed the vault-32676-add-vault-license-check-info-to-system-view-plugin-env branch from 0f140bf to 49f8ce4 Compare December 4, 2024 19:06

rename testing_util.go to testing_util_stubs_oss.go and move init Bui…

fe7bf90

…ldDate to new shared testing_util.go

helenfufu force-pushed the vault-32676-add-vault-license-check-info-to-system-view-plugin-env branch from 49f8ce4 to fe7bf90 Compare December 4, 2024 19:08

Merge branch 'main' into vault-32676-add-vault-license-check-info-to-…

637b813

…system-view-plugin-env

vercel bot deployed to Preview December 6, 2024 16:14 View deployment

thyton added 2 commits December 6, 2024 08:17

remove VaultBuiltinPublicKeys from PluginEnvironment

a6f3ce7

update changelog

6d0848f

thyton force-pushed the vault-32676-add-vault-license-check-info-to-system-view-plugin-env branch from febd9e4 to 6d0848f Compare December 6, 2024 16:22

thyton approved these changes Dec 6, 2024

View reviewed changes

thyton changed the title ~~Vault 32676 add vault license check info to system view plugin env~~ Vault 32676 add vault build date to system view plugin env Dec 6, 2024

Merge branch 'main' into vault-32676-add-vault-license-check-info-to-…

c9982b5

…system-view-plugin-env

vercel bot deployed to Preview December 6, 2024 18:17 View deployment

Merge branch 'main' into vault-32676-add-vault-license-check-info-to-…

7ba9cfc

…system-view-plugin-env

vinay-gopalan approved these changes Dec 10, 2024

View reviewed changes

thyton merged commit c27a54a into main Dec 10, 2024
92 checks passed

thyton deleted the vault-32676-add-vault-license-check-info-to-system-view-plugin-env branch December 10, 2024 22:30

hc-github-team-secure-vault-core mentioned this pull request Dec 10, 2024

Backport of Vault 32676 add vault build date to system view plugin env into release/1.18.x #29143

Merged

6 tasks

Monkeychip pushed a commit that referenced this pull request Dec 18, 2024

add vault build date to system view plugin env VAULT-32676 (#29082)

9e449d9

--------- Co-authored-by: Thy Ton <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vault 32676 add vault build date to system view plugin env #29082

Vault 32676 add vault build date to system view plugin env #29082

helenfufu commented Dec 3, 2024 •

edited by thyton

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

helenfufu left a comment

helenfufu Dec 4, 2024

thyton Dec 4, 2024

helenfufu Dec 4, 2024

thyton Dec 4, 2024

helenfufu Dec 4, 2024

helenfufu Dec 4, 2024 •

edited

Loading

helenfufu Dec 4, 2024

helenfufu Dec 5, 2024 •

edited

Loading

helenfufu Dec 4, 2024

github-actions bot commented Dec 4, 2024 •

edited

Loading

thyton left a comment

thyton Dec 4, 2024

thyton Dec 4, 2024

thyton commented Dec 10, 2024 •

edited

Loading

Vault 32676 add vault build date to system view plugin env #29082

Vault 32676 add vault build date to system view plugin env #29082

Conversation

helenfufu commented Dec 3, 2024 • edited by thyton Loading

Description

TODO only if you're a HashiCorp employee

github-actions bot commented Dec 3, 2024 • edited Loading

helenfufu 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

helenfufu Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenfufu Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2024 • edited Loading

thyton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thyton commented Dec 10, 2024 • edited Loading

helenfufu commented Dec 3, 2024 •

edited by thyton

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

helenfufu Dec 4, 2024 •

edited

Loading

helenfufu Dec 5, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

thyton commented Dec 10, 2024 •

edited

Loading