Skip to content

Commit

Permalink
Fix plugin list API when audit logging enabled (#18173)
Browse files Browse the repository at this point in the history
* Add test that fails due to audit log panic
* Rebuild VersionedPlugin as map of primitive types before adding to response
* Changelog
* Fix casting in external plugin tests
  • Loading branch information
tomhjp authored and AnPucel committed Jan 14, 2023
1 parent 5d29e6c commit 3c721dc
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 5 deletions.
3 changes: 3 additions & 0 deletions changelog/18173.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
plugins: Listing all plugins while audit logging is enabled will no longer result in an internal server error.
```
8 changes: 4 additions & 4 deletions vault/external_plugin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,8 @@ func TestCore_EnableExternalKv_MultipleVersions(t *testing.T) {
t.Fatalf("%#v", resp)
}
found := false
for _, plugin := range resp.Data["detailed"].([]pluginutil.VersionedPlugin) {
if plugin.Name == pluginName && plugin.Version == "v1.2.3" {
for _, plugin := range resp.Data["detailed"].([]map[string]any) {
if plugin["name"] == pluginName && plugin["version"] == "v1.2.3" {
found = true
break
}
Expand Down Expand Up @@ -437,8 +437,8 @@ func TestCore_EnableExternalNoop_MultipleVersions(t *testing.T) {
t.Fatalf("%#v", resp)
}
found := false
for _, plugin := range resp.Data["detailed"].([]pluginutil.VersionedPlugin) {
if plugin.Name == "noop" && plugin.Version == "v1.2.3" {
for _, plugin := range resp.Data["detailed"].([]map[string]any) {
if plugin["name"] == "noop" && plugin["version"] == "v1.2.3" {
found = true
break
}
Expand Down
23 changes: 22 additions & 1 deletion vault/logical_system.go
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,28 @@ func (b *SystemBackend) handlePluginCatalogUntypedList(ctx context.Context, _ *l
}

if len(versionedPlugins) != 0 {
data["detailed"] = versionedPlugins
// Audit logging uses reflection to HMAC the values of all fields in the
// response recursively, which panics if it comes across any unexported
// fields. Therefore, we have to rebuild the VersionedPlugin struct as
// a map of primitive types to avoid the panic that would happen when
// audit logging tries to HMAC the contents of the SemanticVersion field.
var detailed []map[string]any
for _, p := range versionedPlugins {
entry := map[string]any{
"type": p.Type,
"name": p.Name,
"version": p.Version,
"builtin": p.Builtin,
}
if p.SHA256 != "" {
entry["sha256"] = p.SHA256
}
if p.DeprecationStatus != "" {
entry["deprecation_status"] = p.DeprecationStatus
}
detailed = append(detailed, entry)
}
data["detailed"] = detailed
}

return &logical.Response{
Expand Down
32 changes: 32 additions & 0 deletions vault/logical_system_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3130,6 +3130,38 @@ func TestSystemBackend_PluginCatalog_CRUD(t *testing.T) {
}
}

func TestSystemBackend_PluginCatalog_ListPlugins_SucceedsWithAuditLogEnabled(t *testing.T) {
core, b, root := testCoreSystemBackend(t)

tempDir := t.TempDir()
f, err := os.CreateTemp(tempDir, "")
if err != nil {
t.Fatal(err)
}

// Enable audit logging.
req := logical.TestRequest(t, logical.UpdateOperation, "audit/file")
req.Data = map[string]any{
"type": "file",
"options": map[string]any{
"file_path": f.Name(),
},
}
ctx := namespace.RootContext(nil)
resp, err := b.HandleRequest(ctx, req)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("resp: %#v, err: %v", resp, err)
}

// List plugins
req = logical.TestRequest(t, logical.ReadOperation, "sys/plugins/catalog")
req.ClientToken = root
resp, err = core.HandleRequest(ctx, req)
if err != nil || resp == nil || resp.IsError() {
t.Fatalf("resp: %#v, err: %v", resp, err)
}
}

func TestSystemBackend_PluginCatalog_CannotRegisterBuiltinPlugins(t *testing.T) {
c, b, _ := testCoreSystemBackend(t)
// Bootstrap the pluginCatalog
Expand Down
4 changes: 4 additions & 0 deletions vault/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
raftlib "github.com/hashicorp/raft"
"github.com/hashicorp/vault/api"
"github.com/hashicorp/vault/audit"
auditFile "github.com/hashicorp/vault/builtin/audit/file"
"github.com/hashicorp/vault/builtin/credential/approle"
"github.com/hashicorp/vault/command/server"
"github.com/hashicorp/vault/helper/constants"
Expand Down Expand Up @@ -129,6 +130,9 @@ func TestCoreWithSeal(t testing.T, testSeal Seal, enableRaw bool) *Core {
EnableUI: false,
EnableRaw: enableRaw,
BuiltinRegistry: NewMockBuiltinRegistry(),
AuditBackends: map[string]audit.Factory{
"file": auditFile.Factory,
},
}
return TestCoreWithSealAndUI(t, conf)
}
Expand Down

0 comments on commit 3c721dc

Please sign in to comment.