search

kubeguard / guard

🔑 Kubernetes Authentication & Authorization WebHook Server
https://kubeguard.dev
Apache License 2.0
590 stars 81 forks source link

[authz/azure] fix: drop global shared username variable #385

Closed bcho closed 6 months ago

bcho commented 6 months ago

Fix #384

The username is stored in package level, which is not safe for concurrent access. This pull request fixed by removing this variable and propagate the username in the request helper methods.

A unit test has been added in this patch, before the fix, we can reproduce this issue with this:

$ cd authz/providers/azure
$ go test -v -race ./...
=== RUN   TestCheckAccess/concurrent_access_to_CheckAccess_method
==================
WARNING: DATA RACE
Write at 0x000104760020 by goroutine 123:
  go.kubeguard.dev/guard/authz/providers/azure/rbac.prepareCheckAccessRequestBody()
      <path>/guard/authz/providers/azure/rbac/checkaccessreqhelper.go:509 +0x27c
  go.kubeguard.dev/guard/authz/providers/azure/rbac.(*AccessInfo).CheckAccess()
      <path>/guard/authz/providers/azure/rbac/rbac.go:296 +0xa8
  go.kubeguard.dev/guard/authz/providers/azure/rbac.TestCheckAccess.func4.1()
      <path>/guard/authz/providers/azure/rbac/rbac_test.go:149 +0x78
  go.kubeguard.dev/guard/authz/providers/azure/rbac.TestCheckAccess.func4.3()
      <path>/guard/authz/providers/azure/rbac/rbac_test.go:154 +0x44

Previous write at 0x000104760020 by goroutine 120:
  go.kubeguard.dev/guard/authz/providers/azure/rbac.prepareCheckAccessRequestBody()
      <path>/guard/authz/providers/azure/rbac/checkaccessreqhelper.go:509 +0x27c
  go.kubeguard.dev/guard/authz/providers/azure/rbac.(*AccessInfo).CheckAccess()
      <path>/guard/authz/providers/azure/rbac/rbac.go:296 +0xa8
  go.kubeguard.dev/guard/authz/providers/azure/rbac.TestCheckAccess.func4.1()
      <path>/guard/authz/providers/azure/rbac/rbac_test.go:149 +0x78
  go.kubeguard.dev/guard/authz/providers/azure/rbac.TestCheckAccess.func4.3()
      <path>/guard/authz/providers/azure/rbac/rbac_test.go:154 +0x44

Goroutine 123 (running) created at:
  go.kubeguard.dev/guard/authz/providers/azure/rbac.TestCheckAccess.func4()
      <path>/guard/authz/providers/azure/rbac/rbac_test.go:147 +0x500
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1648 +0x40

Goroutine 120 (running) created at:
  go.kubeguard.dev/guard/authz/providers/azure/rbac.TestCheckAccess.func4()
      <path>/guard/authz/providers/azure/rbac/rbac_test.go:147 +0x500
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1648 +0x40
==================

Above test error indicates the variable was being write/read in different goroutines (HTTP handler) without protection.

After the fix, the test can pass with -race enabled.