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

Race seen in client sync #17

Closed
liggitt opened this issue Oct 9, 2015 · 7 comments
Closed

Race seen in client sync #17

liggitt opened this issue Oct 9, 2015 · 7 comments
Assignees
@ericchiang
Labels
bug

Comments

@liggitt
Copy link

liggitt commented Oct 9, 2015

reported in openshift/origin#5025 (comment)

==================

WARNING: DATA RACE

Read by goroutine 53:

  github.com/coreos/go-oidc/oidc.(*Client).VerifyJWT()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/client.go:285 +0x261

  github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc.(*OIDCAuthenticator).AuthenticateToken()

      github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc/_test/_obj_test/oidc.go:140 +0x1bb

  github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc.TestOIDCAuthentication()

      /home/travis/gopath/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc/oidc_test.go:378 +0x1654

  testing.tRunner()

      /tmp/workdir/go/src/testing/testing.go:456 +0xdc

Previous write by goroutine 90:

  github.com/coreos/go-oidc/oidc.(*providerConfigRepo).Set()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/client.go:194 +0x86

  github.com/coreos/go-oidc/oidc.(*ProviderConfigSyncer).sync()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/provider.go:111 +0x15a

  github.com/coreos/go-oidc/oidc.(*ProviderConfigSyncer).(github.com/coreos/go-oidc/oidc.sync)-fm()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/provider.go:95 +0x3b

  github.com/coreos/go-oidc/oidc.(*pcsStepNext).step()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/provider.go:136 +0x7c

  github.com/coreos/go-oidc/oidc.(*ProviderConfigSyncer).Run.func1()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/provider.go:95 +0x1c9

Goroutine 53 (running) created at:

  testing.RunTests()

      /tmp/workdir/go/src/testing/testing.go:561 +0xaa3

  testing.(*M).Run()

      /tmp/workdir/go/src/testing/testing.go:494 +0xe4

  main.main()

      github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc/_test/_testmain.go:106 +0x384

Goroutine 90 (running) created at:

  github.com/coreos/go-oidc/oidc.(*ProviderConfigSyncer).Run()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/provider.go:100 +0x122

  github.com/coreos/go-oidc/oidc.(*Client).SyncProviderConfig()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/client.go:158 +0x391

  github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc.New()

      github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc/_test/_obj_test/oidc.go:124 +0xfbc

  github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc.TestOIDCAuthentication()

      /home/travis/gopath/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc/oidc_test.go:373 +0x14f6

  testing.tRunner()

      /tmp/workdir/go/src/testing/testing.go:456 +0xdc

==================

==================

WARNING: DATA RACE

Read by goroutine 53:

  github.com/coreos/go-oidc/oidc.(*Client).maybeSyncKeys()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/client.go:181 +0x18c

  github.com/coreos/go-oidc/oidc.(*Client).(github.com/coreos/go-oidc/oidc.maybeSyncKeys)-fm()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/client.go:285 +0x3b

  github.com/coreos/go-oidc/oidc.(*JWTVerifier).Verify()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/verification.go:150 +0x439

  github.com/coreos/go-oidc/oidc.(*Client).VerifyJWT()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/client.go:287 +0x4c4

  github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc.(*OIDCAuthenticator).AuthenticateToken()

      github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc/_test/_obj_test/oidc.go:140 +0x1bb

  github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc.TestOIDCAuthentication()

      /home/travis/gopath/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc/oidc_test.go:378 +0x1654

  testing.tRunner()

      /tmp/workdir/go/src/testing/testing.go:456 +0xdc

Previous write by goroutine 90:

  github.com/coreos/go-oidc/oidc.(*providerConfigRepo).Set()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/client.go:194 +0x86

  github.com/coreos/go-oidc/oidc.(*ProviderConfigSyncer).sync()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/provider.go:111 +0x15a

  github.com/coreos/go-oidc/oidc.(*ProviderConfigSyncer).(github.com/coreos/go-oidc/oidc.sync)-fm()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/provider.go:95 +0x3b

  github.com/coreos/go-oidc/oidc.(*pcsStepNext).step()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/provider.go:136 +0x7c

  github.com/coreos/go-oidc/oidc.(*ProviderConfigSyncer).Run.func1()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/provider.go:95 +0x1c9

Goroutine 53 (running) created at:

  testing.RunTests()

      /tmp/workdir/go/src/testing/testing.go:561 +0xaa3

  testing.(*M).Run()

      /tmp/workdir/go/src/testing/testing.go:494 +0xe4

  main.main()

      github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc/_test/_testmain.go:106 +0x384

Goroutine 90 (running) created at:

  github.com/coreos/go-oidc/oidc.(*ProviderConfigSyncer).Run()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/provider.go:100 +0x122

  github.com/coreos/go-oidc/oidc.(*Client).SyncProviderConfig()

      /home/travis/gopath/src/github.com/openshift/origin/Godeps/_workspace/src/github.com/coreos/go-oidc/oidc/client.go:158 +0x391

  github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc.New()

      github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc/_test/_obj_test/oidc.go:124 +0xfbc

  github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc.TestOIDCAuthentication()

      /home/travis/gopath/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/oidc/oidc_test.go:373 +0x14f6

  testing.tRunner()

      /tmp/workdir/go/src/testing/testing.go:456 +0xdc

==================
@yifan-gu
Copy link
Contributor

subscribe

@yifan-gu
Copy link
Contributor

cc @bobbyrullo

@liggitt
Copy link
Author

liggitt commented Oct 16, 2015

seen in go 1.5.1

@yifan-gu
Copy link
Contributor

@liggitt Thanks for reporting. The race can be fixed by adding a mutex, but I think we need to check the if the provider config is expired before using the key endpoints here https://github.com/coreos/go-oidc/blob/master/oidc/client.go#L181. @bobbyrullo

@yifan-gu
Copy link
Contributor

Or let the provider syncer hold the lock before syncing. Also let it sync a few second before the the config expires

@yifan-gu
Copy link
Contributor

yifan-gu commented Dec 8, 2015

cc @ericchiang

@timothysc
Copy link

details on k8s failure https://paste.fedoraproject.org/298806/03063144/

go version go1.5.1 linux/amd64

@ericchiang ericchiang added the bug label Dec 8, 2015
@ericchiang ericchiang self-assigned this Dec 8, 2015
ericchiang added a commit to ericchiang/go-oidc that referenced this issue Dec 8, 2015
@ericchiang
oidc: guard client provider config against race conditions
ba7fc61 
Add a RWMutex around a client's provider config to prevent race
conditions when updating after syncing.

Fixes coreos#17
ericchiang added a commit to ericchiang/go-oidc that referenced this issue Dec 9, 2015
@ericchiang
oidc: fix race condition in provider config syncing
58dee99 
Add a RWMutex around Client's providerConfig field. Syncing the
provider config writes to the field, while numerous other actions
read from it.

Fixes coreos#17
@timothysc timothysc mentioned this issue Dec 9, 2015
Merged
@ericchiang ericchiang mentioned this issue Dec 9, 2015
Closed
@yifan-gu yifan-gu mentioned this issue Dec 9, 2015
Merged
ericchiang added a commit to ericchiang/dex that referenced this issue Dec 10, 2015
@ericchiang
Godeps: update coreos/go-oidc to include OIDC race condition fixes
faee39f 
Update github.com/coreos/go-oidc/... to include coreos/go-oidc#17
which fixes a race condition in the OIDC connector.
@ericchiang ericchiang mentioned this issue Dec 10, 2015
Merged
lukaszraczylo pushed a commit to lukaszraczylo/go-oidc that referenced this issue Apr 13, 2024
@ericchiang @lukaszraczylo
oidc: fix race condition in provider config syncing
ce5d50c 
Add a RWMutex around Client's providerConfig field. Syncing the
provider config writes to the field, while numerous other actions
read from it.

Fixes coreos#17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ericchiang ericchiang

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@timothysc @liggitt @ericchiang @yifan-gu