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

Add possibility to decode generated encoded root token to api #20595

Merged

Conversation

l-with
Copy link
Contributor

@l-with l-with commented May 15, 2023

solves #5236 (closed)

this makes it possible to get a root token completely using the API by adding the possibility to decode the generated encoded root token at /v1/sys/decode-token

I think it's a good thing if the API supports the same things as the cli (here vault itself).

@l-with l-with changed the title Add possibility to decode token to api Add possibility to decode generated encoded root token to api May 15, 2023
@ncabatoff
Copy link
Collaborator

Hi @l-with, thanks for writing this improvement.

I can't remember why we didn't do this already. Maybe we just thought that it wasn't worth creating an endpoint to do an XOR? If so that was probably wrongheaded, this seems like a good addition to me today.

My main objection to the way you've implemented this is that you're following the example of other code related to root token generation and putting the handler in the http package, then adding an explicit mux.Handle call to the handler. I would like us to have as few such handlers as possible. The ones we have currently all have some reason why they can't be treated like regular logical requests, e.g. they don't want to return a JSON response, or they want to use unseal/recovery keys as their authentication mechanism, or they want to work even when Vault is sealed.

I can't see of any reason why your new endpoint couldn't be handled as a logical request, as in a new SystemBackend method invoked via a new path handler added in NewSystemBackend. I'm guessing you want to make it Unauthenticated, which is probably fine, and which you can also configure in NewSystemBackend.

@l-with
Copy link
Contributor Author

l-with commented May 17, 2023

Hi @ncabatoff, thank you for the positive feedback.

I spent a few hours to understand the implementation structure. The current implementation was the way I found out.
I will have a look at your suggestion.
Perhaps I will ask you for some hints.

Sorry, I accentually closed the request. It was not my intention.

@l-with l-with closed this May 17, 2023
@l-with l-with reopened this May 17, 2023
@l-with
Copy link
Contributor Author

l-with commented May 17, 2023

Hi @ncabatoff
I started the refactoring. I would be appreciated to get your feedback, if this is the way what you suggested.

@ncabatoff
Copy link
Collaborator

Hi @ncabatoff I started the refactoring. I would be appreciated to get your feedback, if this is the way what you suggested.

Yup, this is exactly what I had in mind. I think you're almost there, except for the tests.

@l-with
Copy link
Contributor Author

l-with commented May 17, 2023

Hi @ncabatoff I started the refactoring. I would be appreciated to get your feedback, if this is the way what you suggested.

Yup, this is exactly what I had in mind. I think you're almost there, except for the tests.

Thanks, I'll refactor the test also.
I have a question: Can I avoid returning the complete type Response and extract the data attribute?

@ncabatoff
Copy link
Collaborator

Hi @ncabatoff I started the refactoring. I would be appreciated to get your feedback, if this is the way what you suggested.

Yup, this is exactly what I had in mind. I think you're almost there, except for the tests.

Thanks, I'll refactor the test also. I have a question: Can I avoid returning the complete type Response and extract the data attribute?

Not sure I understand. If you mean where your handler returns

resp := &logical.Response{
		Data: map[string]interface{}{
			"token": token,
		},
	}

then I don't know of a more succinct way to do it. And we need to return that type (and put the returned value(s) in the Data map) in order to conform to expectations for how a regular Vault endpoint behaves.

@l-with
Copy link
Contributor Author

l-with commented May 17, 2023

Hi @ncabatoff I started the refactoring. I would be appreciated to get your feedback, if this is the way what you suggested.

Yup, this is exactly what I had in mind. I think you're almost there, except for the tests.

Thanks, I'll refactor the test also. I have a question: Can I avoid returning the complete type Response and extract the data attribute?

Not sure I understand. If you mean where your handler returns

resp := &logical.Response{
		Data: map[string]interface{}{
			"token": token,
		},
	}

then I don't know of a more succinct way to do it. And we need to return that type (and put the returned value(s) in the Data map) in order to conform to expectations for how a regular Vault endpoint behaves.

Thanks for the quick answer. I asked because of the difference in the structure of the response_body between the backend handlers, e.g. /sys/internal/ui/mounts and the http handlers, e.g. /sys/generate-root/attempt.

@l-with
Copy link
Contributor Author

l-with commented May 18, 2023

@ncabatoff
I implemented the test in logical_system_test.go.
Please tell me if this is the right place for testing the new logical backend.

Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

A couple of little cleanups are needed, but overall this looks nearly done

vault/logical_system_paths.go Outdated Show resolved Hide resolved
vault/logical_system_paths.go Show resolved Hide resolved
vault/logical_system_test.go Outdated Show resolved Hide resolved
vault/logical_system_test.go Outdated Show resolved Hide resolved
api/go.mod Outdated Show resolved Hide resolved
l-with added 5 commits May 18, 2023 15:27

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@l-with l-with requested a review from ncabatoff May 18, 2023 14:10
Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

Looks good! One last minor improvement suggestion for you to consider, it's optional though.

vault/logical_system_test.go Show resolved Hide resolved
Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

Looks good! One last minor improvement suggestion for you to consider, it's optional though.

@ncabatoff
Copy link
Collaborator

Oh, sorry, I forgot about docs. Could I trouble you to add the new endpoint to the api docs? I can give you some pointers if it's not clear what's involved... you seem to be pretty good at figuring this stuff out on your own though so I'm not sure you need the help.

@ncabatoff
Copy link
Collaborator

Oh, sorry, I forgot about docs. Could I trouble you to add the new endpoint to the api docs? I can give you some pointers if it's not clear what's involved... you seem to be pretty good at figuring this stuff out on your own though so I'm not sure you need the help.

Do you mean openAPI doc or is https://developer.hashicorp.com/vault/api-docs or parts from it generated by the repository?

The latter, e.g. https://github.com/hashicorp/vault/blob/main/website/content/api-docs/system/generate-root.mdx
I suspect this will mean a new mdx file in that directory (since I don't see an existing one that this would fit into), and a small addition to https://github.com/hashicorp/vault/blob/main/website/data/api-docs-nav-data.json to add the new path to the sidebar.

@l-with l-with requested a review from yhyakuna as a code owner May 18, 2023 16:38
@l-with
Copy link
Contributor Author

l-with commented May 18, 2023

Oh, sorry, I forgot about docs. Could I trouble you to add the new endpoint to the api docs? I can give you some pointers if it's not clear what's involved... you seem to be pretty good at figuring this stuff out on your own though so I'm not sure you need the help.

Do you mean openAPI doc or is https://developer.hashicorp.com/vault/api-docs or parts from it generated by the repository?

The latter, e.g. https://github.com/hashicorp/vault/blob/main/website/content/api-docs/system/generate-root.mdx I suspect this will mean a new mdx file in that directory (since I don't see an existing one that this would fit into), and a small addition to https://github.com/hashicorp/vault/blob/main/website/data/api-docs-nav-data.json to add the new path to the sidebar.

Sorry, I deleted my comment, since I already started exactly what you suggested.

@l-with l-with requested a review from ncabatoff May 18, 2023 16:40
Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

Docs look good, one last tweak to the changelog and I'll merge it

changelog/20595.txt Outdated Show resolved Hide resolved
@ncabatoff
Copy link
Collaborator

Sorry, one more request: one of our checks is failing:

Error: vault/logical_system_test.go:2423:1: Test TestSystemBackend_decodeToken is missing a go doc

Could you add a godoc to the test func explaining its purpose please?

@l-with
Copy link
Contributor Author

l-with commented May 18, 2023

Sorry, one more request: one of our checks is failing:

Error: vault/logical_system_test.go:2423:1: Test TestSystemBackend_decodeToken is missing a go doc

Could you add a godoc to the test func explaining its purpose please?

done

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

Successfully merging this pull request may close these issues.

None yet

2 participants