Skip to content
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.

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

Vault OpenAPI does not allow you to tell whether an endpoint supports only capability 'list', or both 'list' and 'read' #12283

Closed
maxb opened this issue Aug 8, 2021 · 15 comments · Fixed by #13643
Assignees
Labels

Comments

@maxb
Copy link
Contributor

maxb commented Aug 8, 2021

Is your feature request related to a problem? Please describe.

I am writing a tool to consume Vault's automatic OpenAPI (sys/internal/specs/openapi) for validating policies.

I have found that whilst the OpenAPI lets you know almost what capabilities each Vault path implements, there is an omission:

Support of the 'list' capability can be inferred when Vault renders this into a 'get' operation:

                "parameters": [
                    {
                        "name": "list",
                        "description": "Return a list if `true`",
                        "in": "query",
                        "schema": {
                            "type": "string"
                        }
                    }
                ],

However, there is no way to reliably tell whether a path supporting the 'list' capability also supports 'read'. Some do and some do not.

Describe the solution you'd like

Represent whether a path supports only 'list', or 'list' and 'read' in the OpenAPI document. This could be done by, for list-only endpoints, marking the above-shown list parameter as '"required": true' and '"enum": ["true"]' - i.e. you must pass this parameter and the only valid value to pass is "true".

It would also be appropriate to change the description of the list parameter from "Return a list if true" to "Must be set to true" for list-only operations.

Describe alternatives you've considered

A smaller, but less machine-parseable change would be to just change the description as mentioned above.

@aphorise
Copy link
Contributor

Hey @maxb good suggestions - can you try doing a PR with screen shots of the incorporated changes you've already drafted?

@maxb
Copy link
Contributor Author

maxb commented Sep 11, 2021

Thanks, I will raise with my employer whether I can sign the CLA as an individual contributor, or the company needs to sign it. This might take a little while - if anyone else wants to take the idea and run with it in the meantime, that's fine too!

@heatherezell
Copy link
Contributor

Hi there @maxb! Were you able to check in about the CLA? Your company can sign it, and then if your GitHub account is associated with that organization and it's publicly viewable, it should see you as having "signed" it. Let me know if we can be of any assistance. Thanks!

@maxb
Copy link
Contributor Author

maxb commented Oct 22, 2021

I have a request in with my employer's open-source team to get the legal team to sign off on the CLA. Not sure what the timeline for that is going to look like at present.

@maxb
Copy link
Contributor Author

maxb commented Nov 14, 2021

Hi @hsimon-hashicorp,

My company's open source and legal teams are finding it difficult to find the Corporate version of the CLA and instructions for signing it, rather than an individual CLA - and looking at https://www.hashicorp.com/cla I can see their point, it does make reference to a Corporate CLA but only in passing.

Could you let me know where a company looking to enter into a Corporate CLA should start?

@heatherezell
Copy link
Contributor

Hi @maxb - sorry for the delay in getting back to you. I'll check and see if there's a way to surface that information for you. Thanks for your patience!

@heatherezell
Copy link
Contributor

@maxb I received a document with regards to the Corporate CLA, and the designated users will need to be provided as well. I can be reached via email at the first part of my GitHub username at hashicorp dot com - please send me a note and I'll provide you the document. Thanks so much for your patience!

@heatherezell
Copy link
Contributor

@maxb alternately, you can reach out to the legal team: [email protected] - thanks!

@digivava
Copy link
Collaborator

Hi @maxb, I've been taking a look into this issue and am finding it difficult to find an endpoint to test against that allows both LIST and plain Read, without list=true. Can you remember an endpoint you ran into that supported both?

@ncabatoff
Copy link
Collaborator

Hi @maxb, I've been taking a look into this issue and am finding it difficult to find an endpoint to test against that allows both LIST and plain Read, without list=true. Can you remember an endpoint you ran into that supported both?

/sys/raw is a good endpoint to look at for this: it allows both a GET /sys/raw/xyx and a LIST /sys/raw/xyz, which can be also be implemented using GET sys/raw/xyz?list=true. See https://github.com/hashicorp/vault/blob/main/vault/logical_raw.go#L178-L178

Note that the API docs say:

You can list secrets as well. To do this, either issue a GET with the query parameter list=true, or you can use the LIST HTTP verb. ... The API documentation uses LIST as the HTTP verb, but you can still use GET with the ?list=true query string.

Looking at https://github.com/hashicorp/vault/blob/main/http/logical.go#L44-L44, it seems that if you specify LIST as the method, the ?list param is optional.

I guess we don't use the list method in our OpenAPI output because it's not a standard HTTP method.

@kalafut
Copy link
Contributor

kalafut commented Jan 12, 2022

I’d be interested to know which endpoints support list but not read, which is the case I think @maxb is highlighting wouldn't be discernible in our OpenAPI.

The mapping from Vault operations to HTTP verbs is certainly not perfect, but I believe when we built out the OpenAPI generation there was the assumption that read corresponded to GET, and additionally some of those GET endpoints might be used to do list. I don’t think we ever considered only being able to do list. Maybe we have APIs where that's case, but I think it would be uncommon. I suspect we probably have the same issue with determining that an endpoint only supported create but not update

The currently mapping works pretty well for the common case, but I agree that this ambiguity isn’t ideal. Since we're already talking about Vault-specific behavior more than HTTP/OpenAPI, I was wondering about adding a few more custom attributes. We already support x-vault-createSupported (definition), and maybe we should just add attributes for all Vault operation types. e.g. add x-vault-readSupported, ...listSupported, etc. This change would be purely additive, machine readable, unambiguous, and wouldn’t complicate the existing parameter definition which, as I mentioned, maps pretty well to the common cases.

@ncabatoff
Copy link
Collaborator

I thought @maxb's proposal would work fine to eliminate this ambiguity, don't you?

This could be done by, for list-only endpoints, marking the above-shown list parameter as '"required": true' and '"enum": ["true"]' - i.e. you must pass this parameter and the only valid value to pass is "true".

I’d be interested to know which endpoints support list but not read

Here's one: https://github.com/hashicorp/vault/blob/main/builtin/credential/approle/path_role.go#L189-L189

I think we have a number of cases where LIST applies at the top level, but the corresponding GET embeds the thing being got into the request path, resulting in what looks like a different endpoint.

@kalafut
Copy link
Contributor

kalafut commented Jan 12, 2022

I think the "required" approach would work and is reasonable. It is probably a bit subtle if you're just looking the definition that read isn't supported, so if we went this way I'd also be in favor of also changing the parameter description in those cases, for the humans.

We still have the issue with create vs update though. We don't have to tackle that necessarily (it is slightly different in that there are no HTTP-level changes... it is purely describing behavior), and the x-vault-... approach could always be added later without conflict.

@digivava
Copy link
Collaborator

@kalafut All of the endpoints I tested (just trying out random ones that had the "list" parameter in the OpenAPI output json) allowed only List, not Read. v1/sys/policies/acl, v1/auth/aws/roles, v1/auth/approle/role, etc. I couldn't find any that allowed both, that's why I asked the issue opener. (But thanks @ncabatoff, looks like you found one!)

And yes, I'm currently working on applying the fix that they suggested, I just needed an endpoint that allowed both operations so I could test properly. Hoping to have a PR soon.

@kalafut
Copy link
Contributor

kalafut commented Jan 12, 2022

@digivava Oh I see what you and Nick were getting at now re /ad/roles not being the same endpoint as /ad/roles/{name}, for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants