-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(api-client): add cert auth method #29546
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
feat(api-client): add cert auth method #29546
Conversation
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Wito Chandra seems not to be a GitHub user. Have you signed the CLA already but the status is still pending? Recheck it. |
3a7fea6
to
135bd95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! Thanks for creating this. Left some comments for improvement.
@witochandra Can you please address these concerns as well? We need to this to proceed with your PR. |
@witochandra please feel free to tag me when reviewing the feedback. Thanks again! |
@witochandra We also need a changelog entree for this PR: https://github.com/hashicorp/vault/blob/main/changelog/README.md |
I have addressed that, but I'm not sure what is still pending. 🤔 There were two comments from that bot. One mentioned that I have signed the CLA, while the other stated that I haven't. Maybe there is a bug in the bot? 😅 Additionally, the email associated with the commits is from my GitHub account. |
Hmm interesting, maybe try to sign again? There is also the link to |
Have tried both. When I clicked the read & sign button, it showed a form that I have filled & signed. When I clicked the recheck link, it redirected to this PR page. In the github action, it showed that the CLA has been signed too. Maybe bug in the bot? 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your contribution, @witochandra! Your PR looks good to me and we can go ahead with merging!
Hmmm this is strange, maybe a not surprise from GitHub pipelines sometimes 😄 |
@aslamovamir | @rebwill , Please LMK if there are any specific adjustments required to close this. |
@witochandra sorry for the delay, your PR has been merged! Thank you very much for your contribution to Vault! |
Hello @witochandra, First off thanks for the contribution! We had another internal review which has led us to reworking this cert auth client API quite a bit. The API you provided worked well but it behaved a little differently than the other implementations by not using the passed in api.Client. Not doing so meant it didn't honor many of the options that an end-user might have had set, such as namespaces, retry behavior and forced them to setup a client with the TLS configuration (as once they logged in they would probably want to communicate with Vault for something else) as well as passing it in to this new API. I've filed #29931 that reworks the client, let me know if you have any further questions and or comments. |
* feat(api-client): add cert auth method * chore: apply feedbacks * doc: add copyright & update changelog --------- Co-authored-by: Amir Aslamov <amir.aslamov@hashicorp.com>
Description
This PR adds cert auth method in the vault/api/auth. #29544
https://developer.hashicorp.com/vault/api-docs/auth/cert#login-with-tls-certificate-method