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.

Sign up for GitHub

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

Jump to bottom

Add support for Vault TOTP secrets #7873

Merged
merged 1 commit into from
Mar 19, 2020
Merged

Add support for Vault TOTP secrets #7873

merged 1 commit into from
Mar 19, 2020

Conversation

lordofthejars
Copy link
Contributor

@lordofthejars lordofthejars commented Mar 16, 2020 •
edited
Loading

PR for adding support for Vault TOTP: https://www.vaultproject.io/api/secret/totp/index.html

When the PR is merged I'll send another PR with a new document explaining how to use it.

Fixes #7865

@geoand geoand requested a review from vsevel March 16, 2020 08:04
@sberyozkin
Copy link
Member

@lordofthejars IMHO it is ready to go, all looks quite perfect to me, waiting for @vsevel 's feedback, cheers

@lordofthejars
Copy link
Contributor Author

@vsevel Added the comments you mentioned.

vsevel
vsevel reviewed Mar 17, 2020
View reviewed changes
integration-tests/vault/src/test/java/io/quarkus/vault/VaultTOTPITCase.java Outdated
@Test
public void readKey() {
CreateKeyParameters createKeyParameters = new CreateKeyParameters(
"otpauth://totp/Vault:[email protected]?secret=Y64VEVMBTSXCYIWRSHRNDZW62MPGVU2G&issuer=Vault");
Copy link
Contributor

Choose a reason for hiding this comment

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

create a constant?

@vsevel
Copy link
Contributor

vsevel commented Mar 17, 2020

hello I have added only minor comments. mapper.setSerializationInclusion(Include.NON_NULL) is the only one that could break the other use cases, but should not. let's wait for the CI.

@lordofthejars
Copy link
Contributor Author

@vsevel Last commit with latest changes on test side, now let's wait until the build result. Locally, it worked.

@lordofthejars
Copy link
Contributor Author

@vsevel Second round done :)

@sberyozkin
Copy link
Member

Hi Vincent @vsevel, please approve when you are totally happy with the PR and I'll be glad to merge

@lordofthejars
Copy link
Contributor Author

@sberyozkin @vsevel Same here please :) #7891

@vsevel
Copy link
Contributor

vsevel commented Mar 18, 2020

@lordofthejars I just approved. very nice addition to the vault extension. thanks.

@lordofthejars
Copy link
Contributor Author

Thank you very much to you for starting the vault extension and reviewing the PR.

gsmet
gsmet requested changes Mar 18, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I will have a quick look today, given Vincent and Sergey already are OK with it.

Can you squash the commits though?

@lordofthejars
Copy link
Contributor Author

@gsmet Done.

@sberyozkin sberyozkin self-requested a review March 18, 2020 18:26
@sberyozkin
Copy link
Member

@gsmet Thanks

@gsmet gsmet changed the title fix(#7865): Adds support for Vault TOTP secrets Add support for Vault TOTP secrets Mar 19, 2020
@gsmet gsmet added this to the 1.4.0 milestone Mar 19, 2020
@gsmet gsmet merged commit 49b873e into quarkusio:master Mar 19, 2020
@gsmet
Copy link
Member

gsmet commented Mar 19, 2020

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sberyozkin sberyozkin sberyozkin approved these changes

@gsmet gsmet gsmet approved these changes

@vsevel vsevel vsevel approved these changes

Assignees
No one assigned
Labels
area/testing area/vault release/noteworthy-feature
Projects
None yet
Milestone
1.4.0.CR1
Development

Successfully merging this pull request may close these issues.

Vault TOTP Support
4 participants
@lordofthejars @sberyozkin @vsevel @gsmet