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

auth: Fix simpleToken to respect disabled state for assign #8695

Merged
merged 1 commit into from
Oct 14, 2017

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Oct 13, 2017

TokenProviders can be enabled and disabled. The tokenSimple implementation is disabled when the simpleTokenKeeper field is set tonil.

func (t *tokenSimple) assignSimpleTokenToUser(...), however, is missing a t.simpleTokenKeeper != nil check like those found in (t *tokenSimple) invalidateUser(...) and (t *tokenSimple) info(...), and if it is called when tokenSimple is disabled, a nil pointer derference panic will occur. This appears to be the cause of #8608:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x90197c]

goroutine 133 [running]:
github.com/coreos/etcd/cmd/vendor/github.com/coreos/etcd/auth.(*simpleTokenTTLKeeper).addSimpleToken(0x0, 0xc4239045c0, 0x16)
	/root/etcd/gopath/src/github.com/coreos/etcd/cmd/vendor/github.com/coreos/etcd/auth/simple_token.go:59 +0x3c
github.com/coreos/etcd/cmd/vendor/github.com/coreos/etcd/auth.(*tokenSimple).assignSimpleTokenToUser(0xc4203f64e0, 0xc421688178, 0x4, 0xc4239045c0, 0x16)
	/root/etcd/gopath/src/github.com/coreos/etcd/cmd/vendor/github.com/coreos/etcd/auth/simple_token.go:128 +0x12d
github.com/coreos/etcd/cmd/vendor/github.com/coreos/etcd/auth.(*tokenSimple).assign(0xc4203f64e0, 0x14db740, 0xc4238feae0, 0xc421688178, 0x4, 0xbb, 0x7ffd9f077a28, 0x0, 0x0, 0xc42002e700)
	/root/etcd/gopath/src/github.com/coreos/etcd/cmd/vendor/github.com/coreos/etcd/auth/simple_token.go:191 +0x1e7
...

Adding the check appears safe and is consistent with the intended behavior of the disabled state.

@@ -0,0 +1,67 @@
// Copyright 2016 The etcd Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

s/2016/2017/?

@jpbetz jpbetz force-pushed the fix-disabled-simple-token-assign branch from cec8a27 to 5c46805 Compare October 13, 2017 21:58
@gyuho
Copy link
Contributor

gyuho commented Oct 13, 2017

Also staticcheck found

Checking staticcheck...
auth/simple_token_test.go:63:2: this value of authInfo is never used (SA4006)

https://travis-ci.org/coreos/etcd/jobs/287753236#L1216

Could you remove authInfo? Thanks.

@xiang90
Copy link
Contributor

xiang90 commented Oct 14, 2017

/cc @mitake

@jpbetz jpbetz force-pushed the fix-disabled-simple-token-assign branch from 5c46805 to d3c9643 Compare October 14, 2017 04:44
Copy link
Contributor

@mitake mitake left a comment

Choose a reason for hiding this comment

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

@jpbetz lgtm, thanks a lot for fixing this!

@mitake
Copy link
Contributor

mitake commented Oct 14, 2017

@gyuho @xiang90 I'm merging this PR because the CI failures wouldn't be related to the change. Please let me know if there are problems.

@mitake mitake merged commit b989e19 into etcd-io:master Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants