Skip to content

Vault 27421 update cap/ldap dep#29302

Merged
helenfufu merged 10 commits intomainfrom
vault-27421-update-cap-ldap-dep
Jan 8, 2025
Merged

Vault 27421 update cap/ldap dep#29302
helenfufu merged 10 commits intomainfrom
vault-27421-update-cap-ldap-dep

Conversation

@helenfufu
Copy link
Copy Markdown
Contributor

@helenfufu helenfufu commented Jan 6, 2025

Description

This PR updates the github.com/hashicorp/cap/ldap dependency to the latest commit, currently hashicorp/cap@9047b8b, to address a low-severity security gap related to LDAP user search with upndomain. For more details, see the related cap PR: hashicorp/cap#151.

Since this introduces a potential breaking change, it should not be backported to earlier Vault versions.

Ticket: VAULT-27421

TODO only if you're a HashiCorp employee

  • Backport Labels: If this fix needs to be backported, use the appropriate backport/ label that matches the desired release branch. Note that in the CE repo, the latest release branch will look like backport/x.x.x, but older release branches will be backport/ent/x.x.x+ent.
    • LTS: If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
  • ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
    of a public function, even if that change is in a CE file, double check that
    applying the patch for this PR to the ENT repo and running tests doesn't
    break any tests. Sometimes ENT only tests rely on public functions in CE
    files.
  • Jira: If this change has an associated Jira, it's referenced either
    in the PR description, commit message, or branch name.
  • RFC: If this change has an associated RFC, please link it in the description.
  • ENT PR: If this change has an associated ENT PR, please link it in the
    description. Also, make sure the changelog is in this PR, not in your ENT PR.

@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jan 6, 2025
@helenfufu helenfufu added this to the 1.19.0-rc milestone Jan 6, 2025
@helenfufu helenfufu requested a review from fairclothjm January 6, 2025 23:38
@helenfufu helenfufu marked this pull request as ready for review January 6, 2025 23:38
@helenfufu helenfufu requested review from a team as code owners January 6, 2025 23:38
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 6, 2025

CI Results:
All Go tests succeeded! ✅

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 6, 2025

Build Results:
All builds succeeded! ✅

Copy link
Copy Markdown
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

Thanks! Can we also update cap/ldap in the sdk's go.mod?

github.com/hashicorp/cap/ldap v0.0.0-20240328153749-fcfe271d0227

@helenfufu helenfufu changed the title Vault 27421 update cap ldap dep Vault 27421 update cap/ldap dep Jan 7, 2025
Comment on lines +1 to +3
```release-note:change
cap/ldap: Upgrade cap/ldap to https://github.com/hashicorp/cap/commit/9047b8b3240fd6503b08a05e5a5455793c64100f, which includes a security improvement to ensure that user DN searches with `upndomain` configured will check that exactly one user is returned and error otherwise.
``` No newline at end of file
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would appreciate guidance here:

  1. Is it alright to link the exact commit, or would it be preferred to mention something along the lines of "latest commit on main"?
  2. Does this level of detail about the security improvement need to be documented in the changelog in addition to the upgrade guide? Wasn't sure if it was redundant to have it here as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think for the changelog, we should not reference cap/ldap but instead auth/ldap and any anywhere else this may have effect. I think that is the only one though. We want changelogs to describe the effect on the Vault user. So in this case, something like (I believe the effect is on login?):

release-note:change
auth/ldap: An error will now be returned on login if the number of entries returned from the user DN LDAP search is more than one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks for explaining 🙏

fairclothjm
fairclothjm previously approved these changes Jan 7, 2025
Copy link
Copy Markdown
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

LGTM! Should we add some test coverage to assert this behavior?

@helenfufu helenfufu merged commit b74e2e7 into main Jan 8, 2025
@helenfufu helenfufu deleted the vault-27421-update-cap-ldap-dep branch January 8, 2025 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants