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

Hashicorp Vault wallet implementation #781

Merged

Conversation

TsvetanG
Copy link

Type of change

  • New feature
  • Improvement (improvement to code, performance, etc)
  • Test update
  • Documentation update

Description

Hashicorp Vault wallet implementation as initiated by #568

The change brings in a new wallet implementation that uses Hashicorp Vault as a secure store of identities and related cryptographic material.
A new API is introduced to support a backend wallet. The documentation (readme) is updated to describe how to configure and use the new wallet. Some of the existing library dependencies are updated due to npm audit failures.

lbandrov and others added 3 commits September 4, 2024 14:39
---------

Signed-off-by: Lyubomir Bandrov <[email protected]>
Signed-off-by: Yanko Zhelyazkov <[email protected]>
Co-authored-by: Yanko Zhelyazkov <[email protected]>
Signed-off-by: Yanko Zhelyazkov <[email protected]>

add readme for hashicorp vault wallet config

Signed-off-by: Yanko Zhelyazkov <[email protected]>
@TsvetanG TsvetanG requested a review from a team as a code owner September 11, 2024 12:55
@yzhivkov yzhivkov force-pushed the feature/vault-storage branch 2 times, most recently from 30e52c4 to bdc6235 Compare September 11, 2024 17:17
Signed-off-by: Yanko Zhelyazkov <[email protected]>
Copy link
Contributor

@dshuffma-ibm dshuffma-ibm left a comment

Choose a reason for hiding this comment

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

Alrighty, be sure to read this part of the review before the code comments!

So first, this is really well written and thought out. Thanks for putting in all this effort. 👏

I've found a few things that need resolving, but the real hold up to merging this is we need to address the elephant in the room. This project is on life-support and has no real maintainers at the moment. The IBMers on this repo are only committed to keep this codebase updated to address security/exploit issues. There is nobody who can help with integration issues that might crop up from this new feature.

What you've added is well designed, but there is nobody to maintain it. I don't like the idea of adding something as critical to as a new wallet system when we don't have a test team, or a dev team, or even 1 active maintainer on this project. The wallet system is not some side feature of this project, it is critical to nearly every user action you can perform on this web app, including just reading data.

SO! my recommendation is that you take your fork of this project and advertise the wallet integration you've done there, and we leave this version as is. Then you can gather a community and turn your fork into the leading version of this project.

However, since I have moved on from this project, I don't think its fair for me to have the final say. I'll let the current maintainers and anybody out there voice their opinion. If there was just 1 active person, then maybe i'd change my mind. But anyway, you don't need to convince me, there are others than can approve.

Scores:
Code-Readability - 8/10
Code-Structure - 9/10
Chance this PR causes an issue with the vanilla wallet in some weird use case - 3/10
Chance this PR causes some issue in the future - 6.5/10
Chance that I'll be around to fix it - 1/10

packages/apollo/src/identity_store/IdentityStoreFactory.js Outdated Show resolved Hide resolved
packages/athena/routes/vault_apis.js Show resolved Hide resolved
packages/athena/routes/vault_apis.js Show resolved Hide resolved
packages/athena/routes/vault_apis.js Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dshuffma-ibm
Copy link
Contributor

dshuffma-ibm commented Sep 30, 2024

So what we could do is create an experimental branch for new features, like this one, where they could be proved out over a longer period of time before merging to main. This should help reduce risk and allow our small community to vet things under real situations.

@TsvetanG thoughts?

-------- update ----------
i've created a experimental branch off main, and changed the target of this PR to experimental

@dshuffma-ibm dshuffma-ibm changed the base branch from main to experimental September 30, 2024 12:42
@TsvetanG
Copy link
Author

So we could create an experimental branch for new features, like this one, where they could be proved out over a longer period of time before merging to main. This should help reduce risk and allow our small community to vet things under real situations.

@TsvetanG thoughts?

-------- update ---------- i've created a experimental branch off main, and changed the target of this PR to experimental

Hi @dshuffma-ibm that looks ok. We will need a build out of the new branch (experimental) and an image. For that purpose, we have to update the git workflow actions in the default branch (that is main). We will open a new pull request with the git actions change so it can be merged.

Copy link
Contributor

@dshuffma-ibm dshuffma-ibm left a comment

Choose a reason for hiding this comment

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

lgtm

@dshuffma-ibm dshuffma-ibm merged commit c58c41d into hyperledger-labs:experimental Oct 8, 2024
3 checks passed
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.

4 participants