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.

Sign up for GitHub

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

Jump to bottom

Fix race accessing b.crls within cert auth #18945

Merged
merged 3 commits into from
Feb 1, 2023
Merged

Fix race accessing b.crls within cert auth #18945

merged 3 commits into from
Feb 1, 2023

Conversation

stevendpclark
Copy link
Contributor

  • Discovered by CircleCI the pathLogin, pathLoginRenew paths access and reload the b.crls member variable without a lock.
  • Also discovered that pathLoginResolveRole never populated an empty b.crls before usage within b.verifyCredentials
=== RUN   TestCRLFetch
==================
WARNING: DATA RACE
Write at 0x00c0031af8c0 by goroutine 12215:
  github.com/hashicorp/vault/builtin/credential/cert.(*backend).populateCRLs()
      /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/path_crls.go:85 +0xa5
  github.com/hashicorp/vault/builtin/credential/cert.(*backend).pathLogin()
      /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/path_login.go:95 +0x145
  github.com/hashicorp/vault/builtin/credential/cert.TestCRLFetch()
      /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/path_crls_test.go:123 +0x16f2
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x47

Previous read at 0x00c0031af8c0 by goroutine 12216:
  github.com/hashicorp/vault/builtin/credential/cert.(*backend).updateCRLs()
      /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/backend.go:136 +0xf7
  github.com/hashicorp/vault/builtin/credential/cert.(*backend).updateCRLs-fm()
      <autogenerated>:1 +0x64
  github.com/hashicorp/vault/builtin/credential/cert.TestCRLFetch.func1()
      /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/path_crls_test.go:42 +0x1e6

@stevendpclark
Fix race accessing b.crls within cert auth
5e034a1 
 - Discovered by CircleCI the pathLogin, pathLoginRenew paths access
   and reloads the b.crls member variable without a lock.
 - Also discovered that pathLoginResolveRole never populated an empty
   b.crls before usage within b.verifyCredentials
@stevendpclark stevendpclark added bug Used to indicate a potential bug auth/cert Authentication - certificates backport/1.12.x labels Feb 1, 2023
@stevendpclark stevendpclark added this to the 1.12.3 milestone Feb 1, 2023
@stevendpclark stevendpclark requested a review from a team February 1, 2023 18:56
sgmiller
sgmiller reviewed Feb 1, 2023
View reviewed changes
Copy link
Collaborator

@sgmiller sgmiller left a comment

Choose a reason for hiding this comment

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

One cleaner thing you could do here is make an request handler wrapper that calls populateCrlIfNil, and set it up when we setup paths.

@stevendpclark
Misc cleanup
b1b4fba 
 - Introduce a login path wrapper instead of repeating in all the
   various login methods the crl reloading
 - Cleanup updatedConfig, never returned an error and nothing looked at
   the error returned
 - Make the test within TestCRLFetch a little less timing sensitive as
   I was able to trigger a failure due to my machine taking more than
   150ms to load the new CRL
@stevendpclark stevendpclark merged commit f030cbc into main Feb 1, 2023
@stevendpclark
Copy link
Contributor Author

Thanks all!

@stevendpclark stevendpclark deleted the stevendpclark/vault-13148-race-test-crl-fetch branch February 1, 2023 21:23
Merged
stevendpclark added a commit that referenced this pull request Feb 1, 2023
@stevendpclark
Fix race accessing b.crls within cert auth (#18945)
bd62d16 
* Fix race accessing b.crls within cert auth

 - Discovered by CircleCI the pathLogin, pathLoginRenew paths access
   and reloads the b.crls member variable without a lock.
 - Also discovered that pathLoginResolveRole never populated an empty
   b.crls before usage within b.verifyCredentials

* Add cl

* Misc cleanup

 - Introduce a login path wrapper instead of repeating in all the
   various login methods the crl reloading
 - Cleanup updatedConfig, never returned an error and nothing looked at
   the error returned
 - Make the test within TestCRLFetch a little less timing sensitive as
   I was able to trigger a failure due to my machine taking more than
   150ms to load the new CRL
stevendpclark added a commit that referenced this pull request Feb 2, 2023
@hc-github-team-secure-vault-core @stevendpclark
Fix race accessing b.crls within cert auth (#18945) (#18949)
209b3dd 
* Fix race accessing b.crls within cert auth

 - Discovered by CircleCI the pathLogin, pathLoginRenew paths access
   and reloads the b.crls member variable without a lock.
 - Also discovered that pathLoginResolveRole never populated an empty
   b.crls before usage within b.verifyCredentials

* Add cl

* Misc cleanup

 - Introduce a login path wrapper instead of repeating in all the
   various login methods the crl reloading
 - Cleanup updatedConfig, never returned an error and nothing looked at
   the error returned
 - Make the test within TestCRLFetch a little less timing sensitive as
   I was able to trigger a failure due to my machine taking more than
   150ms to load the new CRL

Co-authored-by: Steven Clark <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncabatoff ncabatoff ncabatoff approved these changes

@sgmiller sgmiller sgmiller approved these changes

Assignees
No one assigned
Labels
auth/cert Authentication - certificates bug Used to indicate a potential bug
Projects
None yet
Milestone
1.12.3
Development

Successfully merging this pull request may close these issues.
3 participants
@stevendpclark @sgmiller @ncabatoff