Skip to content

Commit

Permalink
OpenAPI: Separate ListOperation from ReadOperation (#21723)
Browse files Browse the repository at this point in the history 
* OpenAPI: Separate ListOperation from ReadOperation

Historically, since Vault's ReadOperation and ListOperation both map to
the HTTP GET method, their representation in the generated OpenAPI has
been a bit confusing.

This was partially mitigated some time ago, by making the `list` query
parameter express whether it was required or optional - but only in
a way useful to human readers - the human had to know, for example, that
the schema of the response body would change depending on whether `list`
was selected.

Now that there is an effort underway to automatically generate API
clients from the OpenAPI spec, we have a need to fix this more
comprehensively. Fortunately, we do have a means to do so - since Vault
has opinionated treatment of trailing slashes, linked to operations
being list or not, we can use an added trailing slash on the URL path to
separate list operations in the OpenAPI spec.

This PR implements that, and then fixes an operation ID which becomes
duplicated, with this change applied.

See also hashicorp/vault-client-go#174, a bug which will be fixed by
this work.

* Set further DisplayAttrs in auth/github plugin

To mask out more duplicate read/list functionality, now being separately
generated to OpenAPI client libraries as a result of this change.

* Apply requested changes to operation IDs

I'm not totally convinced its worth the extra lines of code, but
equally, I don't have strong feelings about it, so I'll just make the
change.

* Adjust logic to prevent any possibility of generating OpenAPI paths with doubled final slashes

Even in the edge case of improper use of regex patterns and operations.

* changelog

* Fix TestSudoPaths to pass again... which snowballed a bit...

Once I looked hard at it, I found it was missing several sudo paths,
which led to additional bug fixing elsewhere.

I might need to pull some parts of this change out into a separate PR
for ease of review...

* Fix other tests

* More test fixing

* Undo scope creep - back away from fixing sudo paths not shown as such in OpenAPI, at least within this PR

Just add TODO comments for now.
  • Loading branch information
@maxb
maxb authored Jul 13, 2023
1 parent c2cbd5a commit e47dd9d
Show file tree
Hide file tree
Showing 11 changed files with 228 additions and 90 deletions.
40 changes: 24 additions & 16 deletions api/plugin_helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ const (
// path matches that path or not (useful specifically for the paths that
// contain templated fields.)
var sudoPaths = map[string]*regexp.Regexp{
"/auth/token/accessors": regexp.MustCompile(`^/auth/token/accessors/?$`),
"/auth/token/accessors": regexp.MustCompile(`^/auth/token/accessors/?$`),
// TODO /auth/token/revoke-orphan requires sudo but isn't represented as such in the OpenAPI spec
"/pki/root": regexp.MustCompile(`^/pki/root$`),
"/pki/root/sign-self-issued": regexp.MustCompile(`^/pki/root/sign-self-issued$`),
"/sys/audit": regexp.MustCompile(`^/sys/audit$`),
Expand All @@ -52,28 +53,33 @@ var sudoPaths = map[string]*regexp.Regexp{
"/sys/config/cors": regexp.MustCompile(`^/sys/config/cors$`),
"/sys/config/ui/headers": regexp.MustCompile(`^/sys/config/ui/headers/?$`),
"/sys/config/ui/headers/{header}": regexp.MustCompile(`^/sys/config/ui/headers/.+$`),
"/sys/leases": regexp.MustCompile(`^/sys/leases$`),
"/sys/leases/lookup/": regexp.MustCompile(`^/sys/leases/lookup/?$`),
"/sys/leases/lookup/{prefix}": regexp.MustCompile(`^/sys/leases/lookup/.+$`),
"/sys/leases/revoke-force/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-force/.+$`),
"/sys/leases/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-prefix/.+$`),
"/sys/plugins/catalog/{name}": regexp.MustCompile(`^/sys/plugins/catalog/[^/]+$`),
"/sys/plugins/catalog/{type}": regexp.MustCompile(`^/sys/plugins/catalog/[\w-]+$`),
"/sys/plugins/catalog/{type}/{name}": regexp.MustCompile(`^/sys/plugins/catalog/[\w-]+/[^/]+$`),
"/sys/raw": regexp.MustCompile(`^/sys/raw$`),
"/sys/raw/{path}": regexp.MustCompile(`^/sys/raw/.+$`),
"/sys/remount": regexp.MustCompile(`^/sys/remount$`),
"/sys/revoke-force/{prefix}": regexp.MustCompile(`^/sys/revoke-force/.+$`),
"/sys/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/revoke-prefix/.+$`),
"/sys/rotate": regexp.MustCompile(`^/sys/rotate$`),
"/sys/internal/inspect/router/{tag}": regexp.MustCompile(`^/sys/internal/inspect/router/.+$`),
"/sys/leases": regexp.MustCompile(`^/sys/leases$`),
// This entry is a bit wrong... sys/leases/lookup does NOT require sudo. But sys/leases/lookup/ with a trailing
// slash DOES require sudo. But the part of the Vault CLI that uses this logic doesn't pass operation-appropriate
// trailing slashes, it always strips them off, so we end up giving the wrong answer for one of these.
"/sys/leases/lookup": regexp.MustCompile(`^/sys/leases/lookup/?$`),
"/sys/leases/lookup/{prefix}": regexp.MustCompile(`^/sys/leases/lookup/.+$`),
"/sys/leases/revoke-force/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-force/.+$`),
"/sys/leases/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-prefix/.+$`),
"/sys/plugins/catalog/{name}": regexp.MustCompile(`^/sys/plugins/catalog/[^/]+$`),
"/sys/plugins/catalog/{type}": regexp.MustCompile(`^/sys/plugins/catalog/[\w-]+$`),
"/sys/plugins/catalog/{type}/{name}": regexp.MustCompile(`^/sys/plugins/catalog/[\w-]+/[^/]+$`),
"/sys/raw": regexp.MustCompile(`^/sys/raw$`),
"/sys/raw/{path}": regexp.MustCompile(`^/sys/raw/.+$`),
"/sys/remount": regexp.MustCompile(`^/sys/remount$`),
"/sys/revoke-force/{prefix}": regexp.MustCompile(`^/sys/revoke-force/.+$`),
"/sys/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/revoke-prefix/.+$`),
"/sys/rotate": regexp.MustCompile(`^/sys/rotate$`),
// TODO /sys/seal requires sudo but isn't represented as such in the OpenAPI spec
// TODO /sys/step-down requires sudo but isn't represented as such in the OpenAPI spec

// enterprise-only paths
"/sys/replication/dr/primary/secondary-token": regexp.MustCompile(`^/sys/replication/dr/primary/secondary-token$`),
"/sys/replication/performance/primary/secondary-token": regexp.MustCompile(`^/sys/replication/performance/primary/secondary-token$`),
"/sys/replication/primary/secondary-token": regexp.MustCompile(`^/sys/replication/primary/secondary-token$`),
"/sys/replication/reindex": regexp.MustCompile(`^/sys/replication/reindex$`),
"/sys/storage/raft/snapshot-auto/config/": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/?$`),
"/sys/storage/raft/snapshot-auto/config": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/?$`),
"/sys/storage/raft/snapshot-auto/config/{name}": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/[^/]+$`),
}

Expand Down Expand Up @@ -252,6 +258,8 @@ func SudoPaths() map[string]*regexp.Regexp {
// Determine whether the given path requires the sudo capability.
// Note that this uses hardcoded static path information, so will return incorrect results for paths in namespaces,
// or for secret engines mounted at non-default paths.
// Expects to receive a path with an initial slash, but no trailing slashes, as the Vault CLI (the only known and
// expected user of this function) sanitizes its paths that way.
func IsSudoPath(path string) bool {
// Return early if the path is any of the non-templated sudo paths.
if _, ok := sudoPaths[path]; ok {
Expand Down
32 changes: 31 additions & 1 deletion builtin/credential/github/backend.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"net/url"

"github.com/google/go-github/github"
cleanhttp "github.com/hashicorp/go-cleanhttp"
"github.com/hashicorp/go-cleanhttp"
"github.com/hashicorp/vault/sdk/framework"
"github.com/hashicorp/vault/sdk/logical"
"golang.org/x/oauth2"
Expand Down Expand Up @@ -43,6 +43,21 @@ func Backend() *backend {
OperationPrefix: operationPrefixGithub,
OperationSuffix: "team-mapping",
}
teamMapPaths[0].Operations = map[logical.Operation]framework.OperationHandler{
logical.ListOperation: &framework.PathOperation{
Callback: teamMapPaths[0].Callbacks[logical.ListOperation],
Summary: teamMapPaths[0].HelpSynopsis,
},
logical.ReadOperation: &framework.PathOperation{
Callback: teamMapPaths[0].Callbacks[logical.ReadOperation],
Summary: teamMapPaths[0].HelpSynopsis,
DisplayAttrs: &framework.DisplayAttributes{
OperationVerb: "list",
OperationSuffix: "teams2", // The ReadOperation is redundant with the ListOperation
},
},
}
teamMapPaths[0].Callbacks = nil

b.UserMap = &framework.PolicyMap{
PathMap: framework.PathMap{
Expand All @@ -61,6 +76,21 @@ func Backend() *backend {
OperationPrefix: operationPrefixGithub,
OperationSuffix: "user-mapping",
}
userMapPaths[0].Operations = map[logical.Operation]framework.OperationHandler{
logical.ListOperation: &framework.PathOperation{
Callback: userMapPaths[0].Callbacks[logical.ListOperation],
Summary: userMapPaths[0].HelpSynopsis,
},
logical.ReadOperation: &framework.PathOperation{
Callback: userMapPaths[0].Callbacks[logical.ReadOperation],
Summary: userMapPaths[0].HelpSynopsis,
DisplayAttrs: &framework.DisplayAttributes{
OperationVerb: "list",
OperationSuffix: "users2", // The ReadOperation is redundant with the ListOperation
},
},
}
userMapPaths[0].Callbacks = nil

allPaths := append(teamMapPaths, userMapPaths...)
b.Backend = &framework.Backend{
Expand Down
15 changes: 8 additions & 7 deletions builtin/logical/pki/backend_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6767,8 +6767,8 @@ func TestProperAuthing(t *testing.T) {
"cert/unified-delta-crl": shouldBeUnauthedReadList,
"cert/unified-delta-crl/raw": shouldBeUnauthedReadList,
"cert/unified-delta-crl/raw/pem": shouldBeUnauthedReadList,
"certs": shouldBeAuthed,
"certs/revoked": shouldBeAuthed,
"certs/": shouldBeAuthed,
"certs/revoked/": shouldBeAuthed,
"certs/revocation-queue": shouldBeAuthed,
"certs/unified-revoked": shouldBeAuthed,
"config/acme": shouldBeAuthed,
Expand Down Expand Up @@ -6817,7 +6817,7 @@ func TestProperAuthing(t *testing.T) {
"issuer/default/sign-verbatim": shouldBeAuthed,
"issuer/default/sign-verbatim/test": shouldBeAuthed,
"issuer/default/sign/test": shouldBeAuthed,
"issuers": shouldBeUnauthedReadList,
"issuers/": shouldBeUnauthedReadList,
"issuers/generate/intermediate/exported": shouldBeAuthed,
"issuers/generate/intermediate/internal": shouldBeAuthed,
"issuers/generate/intermediate/existing": shouldBeAuthed,
Expand All @@ -6829,7 +6829,7 @@ func TestProperAuthing(t *testing.T) {
"issuers/import/cert": shouldBeAuthed,
"issuers/import/bundle": shouldBeAuthed,
"key/default": shouldBeAuthed,
"keys": shouldBeAuthed,
"keys/": shouldBeAuthed,
"keys/generate/internal": shouldBeAuthed,
"keys/generate/exported": shouldBeAuthed,
"keys/generate/kms": shouldBeAuthed,
Expand All @@ -6839,7 +6839,7 @@ func TestProperAuthing(t *testing.T) {
"revoke": shouldBeAuthed,
"revoke-with-key": shouldBeAuthed,
"roles/test": shouldBeAuthed,
"roles": shouldBeAuthed,
"roles/": shouldBeAuthed,
"root": shouldBeAuthed,
"root/generate/exported": shouldBeAuthed,
"root/generate/internal": shouldBeAuthed,
Expand All @@ -6864,7 +6864,7 @@ func TestProperAuthing(t *testing.T) {
"unified-crl/delta/pem": shouldBeUnauthedReadList,
"unified-ocsp": shouldBeUnauthedWriteOnly,
"unified-ocsp/dGVzdAo=": shouldBeUnauthedReadList,
"eab": shouldBeAuthed,
"eab/": shouldBeAuthed,
"eab/" + eabKid: shouldBeAuthed,
}

Expand Down Expand Up @@ -6953,7 +6953,8 @@ func TestProperAuthing(t *testing.T) {

handler, present := paths[raw_path]
if !present {
t.Fatalf("OpenAPI reports PKI mount contains %v->%v but was not tested to be authed or authed.", openapi_path, raw_path)
t.Fatalf("OpenAPI reports PKI mount contains %v -> %v but was not tested to be authed or not authed.",
openapi_path, raw_path)
}

openapi_data := raw_data.(map[string]interface{})
Expand Down
5 changes: 3 additions & 2 deletions builtin/logical/ssh/backend_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2765,7 +2765,7 @@ func TestProperAuthing(t *testing.T) {
"public_key": shouldBeUnauthedReadList,
"roles/test-ca": shouldBeAuthed,
"roles/test-otp": shouldBeAuthed,
"roles": shouldBeAuthed,
"roles/": shouldBeAuthed,
"sign/test-ca": shouldBeAuthed,
"tidy/dynamic-keys": shouldBeAuthed,
"verify": shouldBeUnauthedWriteOnly,
Expand Down Expand Up @@ -2809,7 +2809,8 @@ func TestProperAuthing(t *testing.T) {

handler, present := paths[raw_path]
if !present {
t.Fatalf("OpenAPI reports SSH mount contains %v->%v but was not tested to be authed or authed.", openapi_path, raw_path)
t.Fatalf("OpenAPI reports SSH mount contains %v -> %v but was not tested to be authed or not authed.",
openapi_path, raw_path)
}

openapi_data := raw_data.(map[string]interface{})
Expand Down
3 changes: 3 additions & 0 deletions changelog/21723.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
openapi: List operations are now given first-class representation in the OpenAPI document, rather than sometimes being overlaid with a read operation at the same path
```
103 changes: 76 additions & 27 deletions sdk/framework/openapi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,12 @@ type OASLicense struct {
}

type OASPathItem struct {
Description string `json:"description,omitempty"`
Parameters []OASParameter `json:"parameters,omitempty"`
Sudo bool `json:"x-vault-sudo,omitempty" mapstructure:"x-vault-sudo"`
Unauthenticated bool `json:"x-vault-unauthenticated,omitempty" mapstructure:"x-vault-unauthenticated"`
CreateSupported bool `json:"x-vault-createSupported,omitempty" mapstructure:"x-vault-createSupported"`
DisplayNavigation bool `json:"x-vault-displayNavigation,omitempty" mapstructure:"x-vault-displayNavigation"`
DisplayAttrs *DisplayAttributes `json:"x-vault-displayAttrs,omitempty" mapstructure:"x-vault-displayAttrs"`
Description string `json:"description,omitempty"`
Parameters []OASParameter `json:"parameters,omitempty"`
Sudo bool `json:"x-vault-sudo,omitempty" mapstructure:"x-vault-sudo"`
Unauthenticated bool `json:"x-vault-unauthenticated,omitempty" mapstructure:"x-vault-unauthenticated"`
CreateSupported bool `json:"x-vault-createSupported,omitempty" mapstructure:"x-vault-createSupported"`
DisplayAttrs *DisplayAttributes `json:"x-vault-displayAttrs,omitempty" mapstructure:"x-vault-displayAttrs"`

Get *OASOperation `json:"get,omitempty"`
Post *OASOperation `json:"post,omitempty"`
Expand Down Expand Up @@ -309,6 +308,7 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *

// Process each supported operation by building up an Operation object
// with descriptions, properties and examples from the framework.Path data.
var listOperation *OASOperation
for opType, opHandler := range operations {
props := opHandler.Properties()
if props.Unpublished || forceUnpublished {
Expand All @@ -324,11 +324,6 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
}
}

// If both List and Read are defined, only process Read.
if opType == logical.ListOperation && operations[logical.ReadOperation] != nil {
continue
}

op := NewOASOperation()

operationID := constructOperationID(
Expand Down Expand Up @@ -408,24 +403,17 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
}
}

// LIST is represented as GET with a `list` query parameter.
// LIST is represented as GET with a `list` query parameter. Code later on in this function will assign
// list operations to a path with an extra trailing slash, ensuring they do not collide with read
// operations.
if opType == logical.ListOperation {
// Only accepts List (due to the above skipping of ListOperations that also have ReadOperations)
op.Parameters = append(op.Parameters, OASParameter{
Name: "list",
Description: "Must be set to `true`",
Required: true,
In: "query",
Schema: &OASSchema{Type: "string", Enum: []interface{}{"true"}},
})
} else if opType == logical.ReadOperation && operations[logical.ListOperation] != nil {
// Accepts both Read and List
op.Parameters = append(op.Parameters, OASParameter{
Name: "list",
Description: "Return a list if `true`",
In: "query",
Schema: &OASSchema{Type: "string"},
})
}

// Add tags based on backend type
Expand Down Expand Up @@ -521,18 +509,79 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
switch opType {
case logical.CreateOperation, logical.UpdateOperation:
pi.Post = op
case logical.ReadOperation, logical.ListOperation:
case logical.ReadOperation:
pi.Get = op
case logical.DeleteOperation:
pi.Delete = op
case logical.ListOperation:
listOperation = op
}
}

openAPIPath := "/" + path
if doc.Paths[openAPIPath] != nil {
backend.Logger().Warn("OpenAPI spec generation: multiple framework.Path instances generated the same path; last processed wins", "path", openAPIPath)
// The conventions enforced by the Vault HTTP routing code make it impossible to match a path with a trailing
// slash to anything other than a ListOperation. Catch mistakes in path definition, to enforce that if both of
// the two following blocks of code (non-list, and list) write an OpenAPI path to the output document, then the
// first one will definitely not have a trailing slash.
originalPathHasTrailingSlash := strings.HasSuffix(path, "/")
if originalPathHasTrailingSlash && (pi.Get != nil || pi.Post != nil || pi.Delete != nil) {
backend.Logger().Warn(
"OpenAPI spec generation: discarding impossible-to-invoke non-list operations from path with "+
"required trailing slash; this is a bug in the backend code", "path", path)
pi.Get = nil
pi.Post = nil
pi.Delete = nil
}

// Write the regular, non-list, OpenAPI path to the OpenAPI document, UNLESS we generated a ListOperation, and
// NO OTHER operation types. In that fairly common case (there are lots of list-only endpoints), we avoid
// writing a redundant OpenAPI path for (e.g.) "auth/token/accessors" with no operations, only to then write
// one for "auth/token/accessors/" immediately below.
//
// On the other hand, we do still write the OpenAPI path here if we generated ZERO operation types - this serves
// to provide documentation to a human that an endpoint exists, even if it has no invokable OpenAPI operations.
// Examples of this include kv-v2's ".*" endpoint (regex cannot be translated to OpenAPI parameters), and the
// auth/oci/login endpoint (implements ResolveRoleOperation only, only callable from inside Vault).
if listOperation == nil || pi.Get != nil || pi.Post != nil || pi.Delete != nil {
openAPIPath := "/" + path
if doc.Paths[openAPIPath] != nil {
backend.Logger().Warn(
"OpenAPI spec generation: multiple framework.Path instances generated the same path; "+
"last processed wins", "path", openAPIPath)
}
doc.Paths[openAPIPath] = &pi
}

// If there is a ListOperation, write it to a separate OpenAPI path in the document.
if listOperation != nil {
// Append a slash here to disambiguate from the path written immediately above.
// However, if the path already contains a trailing slash, we want to avoid doubling it, and it is
// guaranteed (through the interaction of logic in the last two blocks) that the block immediately above
// will NOT have written a path to the OpenAPI document.
if !originalPathHasTrailingSlash {
path += "/"
}

listPathItem := OASPathItem{
Description: pi.Description,
Parameters: pi.Parameters,
DisplayAttrs: pi.DisplayAttrs,

// Since the path may now have an extra slash on the end, we need to recalculate the special path
// matches, as the sudo or unauthenticated status may be changed as a result!
Sudo: specialPathMatch(path, sudoPaths),
Unauthenticated: specialPathMatch(path, unauthPaths),

Get: listOperation,
}

openAPIPath := "/" + path
if doc.Paths[openAPIPath] != nil {
backend.Logger().Warn(
"OpenAPI spec generation: multiple framework.Path instances generated the same path; "+
"last processed wins", "path", openAPIPath)
}
doc.Paths[openAPIPath] = &listPathItem
}
doc.Paths[openAPIPath] = &pi
}

return nil
Expand Down
Loading

0 comments on commit e47dd9d

Please sign in to comment.