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

chore: replace hashicorp/vault with openbao #2707

Merged
merged 9 commits into from
Nov 7, 2024
Merged

chore: replace hashicorp/vault with openbao #2707

merged 9 commits into from
Nov 7, 2024

Conversation

mschfh
Copy link
Collaborator

@mschfh mschfh commented Nov 6, 2024

Summary

This PR removes github.com/hashicorp/vault to reduce transitive dependencies, such as github.com/Azure/azure-sdk-for-go.

before:

1.5 GiB [##########################] /github.com
386.7 MiB [######                    ] /google.golang.org
 78.7 MiB [#                         ] /golang.org
 20.3 MiB [                          ] /go.mongodb.org

after:

480.8 MiB [##########################] /github.com
385.6 MiB [####################      ] /google.golang.org
 78.8 MiB [####                      ] /golang.org
 15.6 MiB [                          ] /cloud.google.com

Fixes https://github.com/brave-intl/bat-go/security/dependabot/143.
Fixes https://github.com/brave-intl/bat-go/security/dependabot/144.
Fixes https://github.com/brave-intl/bat-go/security/dependabot/145.
Fixes https://github.com/brave-intl/bat-go/security/dependabot/146.
Fixes https://github.com/brave-intl/bat-go/security/dependabot/147.
Fixes https://github.com/brave-intl/bat-go/security/dependabot/148.

Type of Change

  • Product feature
  • Bug fix
  • Performance improvement
  • Refactor
  • Other

Tested Environments

  • Development
  • Staging
  • Production

Before Requesting Review

  • Does your code build cleanly without any errors or warnings?
  • Have you used auto closing keywords?
  • Have you added tests for new functionality?
  • Have validated query efficiency for new database queries?
  • Have documented new functionality in README or in comments?
  • Have you squashed all intermediate commits?
  • Is there a clear title that explains what the PR does?
  • Have you used intuitive function, variable and other naming?
  • Have you requested security and/or privacy review if needed
  • Have you performed a self review of this PR?

@mschfh mschfh self-assigned this Nov 6, 2024
@mschfh mschfh changed the title Mschfh deps chore: replace hashicorp/vault dependency Nov 6, 2024
@pavelbrm pavelbrm changed the title chore: replace hashicorp/vault dependency chore: replace hashicorp/vault with openbao Nov 6, 2024
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@pavelbrm pavelbrm marked this pull request as ready for review November 6, 2024 10:21
@mschfh
Copy link
Collaborator Author

mschfh commented Nov 6, 2024

Copy link

github-actions bot commented Nov 6, 2024

[puLL-Merge] - brave-intl/bat-go@2707

Description

This PR updates the Go version from 1.18 to 1.21/1.22 across the project and makes several dependency updates. It also includes some minor changes to vault-related imports and configuration.

Changes

Changes

  1. .github/workflows/ci.yml:

    • Updated Go version to 1.22
    • Simplified the module caching process
  2. Dockerfile:

    • Removed explicit user and ownership changes
  3. Makefile:

    • Updated golangci-lint version from v1.49.0 to v1.57.2
  4. main/go.mod:

    • Updated Go version to 1.21
    • Removed several unused dependencies
    • Updated remaining dependencies
  5. services/go.mod:

    • Updated Go version to 1.21
    • Updated dependencies
  6. tools/go.mod:

    • Updated Go version to 1.21
    • Replaced github.com/hashicorp/vault with github.com/hashicorp/vault/api
    • Updated dependencies
  7. tools/payments/cmd/create/go.mod:

    • Updated Go version to 1.22.1
    • Replaced github.com/hashicorp/vault with github.com/openbao/openbao/sdk/v2
  8. tools/payments/cmd/create/main.go:

    • Updated import from github.com/hashicorp/vault/shamir to github.com/openbao/openbao/sdk/v2/helper/shamir
  9. tools/vault/signer/vaultsigner.go:

    • Updated import from github.com/hashicorp/vault/command/config to github.com/hashicorp/vault/api/cliconfig

Possible Issues

  1. The change from github.com/hashicorp/vault to github.com/openbao/openbao/sdk/v2 in the payments create command might introduce compatibility issues if the APIs are not identical.

  2. Updating to a newer Go version might reveal previously hidden issues or cause subtle changes in behavior, especially if the code relies on any language features that have changed between versions.

  3. The removal of explicit user and ownership changes in the Dockerfile might have security implications, depending on how the container is used.

Security Hotspots

No significant security issues are immediately apparent in this change. However, any dependency update should be carefully reviewed for potential security implications, especially those related to cryptographic operations or authentication (like the vault-related changes).

@kdenhartog
Copy link
Member

I've looked through this and not concerned with switching over to OpenBAO nor with the modifications made to Shamir.

I've also opened an issue to remove the payments service from this repo since we've now migrated it.

@pavelbrm pavelbrm merged commit e5e94a1 into master Nov 7, 2024
13 checks passed
@pavelbrm pavelbrm deleted the mschfh-deps branch November 7, 2024 01:49
@cipherboy
Copy link

cipherboy commented Nov 14, 2024

\o hey @pavelbrm @kdenhartog -- quick question for y'all since I see you pulled the OpenBao SDK --- My understanding was that our replace directive would make it hard for users to consume, but it doesn't seem like that's the case?

I think leaving it as-is would be ideal, since this impacts testing of the SDK with the latest API on our end so I'd prefer to leave it if it isn't causing issues for downstream consumers.

@pavelbrm
Copy link
Contributor

Hey @cipherboy thanks for reaching out!

our replace directive would make it hard for users to consume

It depends on how diligently the code base will be maintained. Any features from the replaced modules that are available via a released version should also be available to consumers. This means that when the parent module (that which contains the replace directives) is ready for releasing (creating a new tag), the inner module (the replaced one the outer code depends on) should be tagged and released first.

As far as I can tell, the original repository, hashicorp/vault does not do a good job at that.

To illustrate what I am referring to, consider the following example from the original repo. It replaces many of its internal components, api among them, here. So everywhere in that module they rely on the most recent code.

But the most recent publicly available code for api is v1.15.0. At some point, yet unpublished features from the api module had been used throughout the parent module, and it resulted in problems.

I noticed an issue whilst upgrading vault from v1.13.12 to v1.18.1. The latter uses code from the unpublished version of api in some parts, while others still depend on api/v.1.15.0. To fix the problem, I had to find out the commit of v1.18.1, and then upgrade api to that commit so that dependencies could be met (the build was failing because api/v1.15.0 was lacking some fields that were added and used by vault/v1.18.1).

Ultimately, during the work on that PR, it was decided to migrate to OpenBao.


This goes back to the question when to define a module, and when use just a package. A module is needed when independent versioning is needed. That is, a piece of software that evolves somewhat independently from others.

The replace directive helps with development, but requires some maintenance overhead to avoid breaking things for the clients. The question then becomes whether the benefits are worth the effort.

Frankly, our own repository does not do a good job at that, and it's something that needs to be fixed.

Hope that helps.

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

Successfully merging this pull request may close these issues.

4 participants