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

Update Hashing Algorithm #5

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

davidcorrigan714
Copy link
Collaborator

@davidcorrigan714 davidcorrigan714 commented Jan 23, 2023

All Submissions:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My code follows the code style of this project.
  • I ran lint checks locally prior to submission.
  • Have you checked to ensure there aren't other open PRs for the same update/change?

What about the current behavior has changed?

This provider stores secret values in the state file by hashing them with bcrypt. bcrypt only uses the first 72 characters for the hash which results in issues with Artifactory JWTs because the first 72 characters of those tokens contain data about the algorithms and subjects which don't change when they're regenerated. The recommendation from OWASP is to use Argon2 as the current standard for password hashing. I've adopted that for our fork of the provider in this PR. I have no inclination of what Microsoft might ultimately do to resolve the issue upstream but worst case our secrets will get re-pushed to Azure DevOps when upstream resolves this issue and we either merge it in or use the upstream provider again.

Note that the real change is just the change to azuredevops/internal/utils/secretmemo/secretmemo.go and the replacement library really is just about a drop in replacement for bcrypt. Then I tweaked the tests, and updated the modules & vendoring.

Issue Number: microsoft#692

Does this introduce a change to go.mod, go.sum or vendor/?

  • Yes
  • No

Adding the Argo2 library to the bundle

Does this introduce a breaking change?

  • Yes
  • No

Not really "breaking" but maybe worth a release note should this be upstreamed since passwords might be updated on target systems when this goes live.

@davidcorrigan714 davidcorrigan714 changed the base branch from main to ni-main January 23, 2023 19:34
@irwand
Copy link
Collaborator

irwand commented Jan 23, 2023

Hmm.. I didn't notice it before.. is it standard practice that we'd commit dependencies' code into git?

Copy link
Collaborator

@irwand irwand left a comment

Choose a reason for hiding this comment

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

Can you comment on dependencies getting committed into the repo?

@davidcorrigan714
Copy link
Collaborator Author

davidcorrigan714 commented Jan 23, 2023

From what I gather there's some history to why Golang has used vendoring, in this case that's how Microsoft has their package structured so that's that and I don't have much say or opinion either way on it.

@davidcorrigan714 davidcorrigan714 merged commit 4656598 into ni-main Jan 23, 2023
davidcorrigan714 pushed a commit that referenced this pull request Dec 9, 2023
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.

2 participants