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

Ignore Bootstrap Token secrets that don't use predictable names. #41754

Merged
merged 1 commit into from
Feb 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/cmd/token.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ func RunDeleteToken(out io.Writer, cmd *cobra.Command, tokenId string) error {
return err
}

tokenSecretName := fmt.Sprintf("%s%s", kubeadmconstants.BootstrapTokenSecretPrefix, tokenId)
tokenSecretName := fmt.Sprintf("%s%s", bootstrapapi.BootstrapTokenSecretPrefix, tokenId)
if err := client.Secrets(metav1.NamespaceSystem).Delete(tokenSecretName, nil); err != nil {
return fmt.Errorf("failed to delete bootstrap token [%v]", err)
}
Expand Down
2 changes: 0 additions & 2 deletions cmd/kubeadm/app/constants/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,6 @@ const (

// DefaultTokenDuration specifies the default amount of time that a bootstrap token will be valid
DefaultTokenDuration = time.Duration(8) * time.Hour
// BootstrapTokenSecretPrefix the the prefix that will be used for the Secrets that are created as type bootstrap.kubernetes.io/token
BootstrapTokenSecretPrefix = "bootstrap-token-"

// CSVTokenBootstrapUser is currently the user the bootstrap token in the .csv file
// TODO: This should change to something more official and supported
Expand Down
3 changes: 1 addition & 2 deletions cmd/kubeadm/app/phases/token/bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
clientset "k8s.io/client-go/kubernetes"
"k8s.io/client-go/pkg/api/v1"
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
tokenutil "k8s.io/kubernetes/cmd/kubeadm/app/util/token"
bootstrapapi "k8s.io/kubernetes/pkg/bootstrap/api"
)
Expand All @@ -41,7 +40,7 @@ func UpdateOrCreateToken(client *clientset.Clientset, d *kubeadmapi.TokenDiscove
if valid, err := tokenutil.ValidateToken(d); !valid {
return err
}
secretName := fmt.Sprintf("%s%s", kubeadmconstants.BootstrapTokenSecretPrefix, d.ID)
secretName := fmt.Sprintf("%s%s", bootstrapapi.BootstrapTokenSecretPrefix, d.ID)
var lastErr error
for i := 0; i < tokenCreateRetries; i++ {
secret, err := client.Secrets(metav1.NamespaceSystem).Get(secretName, metav1.GetOptions{})
Expand Down
6 changes: 6 additions & 0 deletions pkg/bootstrap/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ import (
)

const (
// BootstrapTokenSecretPrefix is the prefix for bootstrap token names.
// Bootstrap tokens secrets must be named in the form
// `bootstrap-token-<token-id>`. This is the prefix to be used before the
// token ID.
BootstrapTokenSecretPrefix = "bootstrap-token-"

// SecretTypeBootstrapToken is used during the automated bootstrap process (first
// implemented by kubeadm). It stores tokens that are used to sign well known
// ConfigMaps. They may also eventually be used for authentication.
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/bootstrap/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ go_test(
deps = [
"//pkg/bootstrap/api:go_default_library",
"//vendor:github.com/davecgh/go-spew/spew",
"//vendor:github.com/stretchr/testify/assert",
"//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
"//vendor:k8s.io/apimachinery/pkg/runtime/schema",
"//vendor:k8s.io/client-go/kubernetes/fake",
Expand Down
4 changes: 3 additions & 1 deletion pkg/controller/bootstrap/bootstrapsigner.go
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,9 @@ func (e *BootstrapSigner) getTokens() map[string]string {

// Check and warn for duplicate secrets. Behavior here will be undefined.
if _, ok := ret[tokenID]; ok {
glog.V(3).Infof("Duplicate bootstrap tokens found for id %s, ignoring on in %s/%s", tokenID, secret.Namespace, secret.Name)
// This should never happen as we ensure a consistent secret name.
// But leave this in here just in case.
glog.V(1).Infof("Duplicate bootstrap tokens found for id %s, ignoring on in %s/%s", tokenID, secret.Namespace, secret.Name)
continue
}

Expand Down
18 changes: 10 additions & 8 deletions pkg/controller/bootstrap/bootstrapsigner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ func init() {
spew.Config.DisableMethods = true
}

const testTokenID = "abc123"

func newBootstrapSigner() (*BootstrapSigner, *fake.Clientset) {
options := DefaultBootstrapSignerOptions()
cl := fake.NewSimpleClientset()
Expand Down Expand Up @@ -69,7 +71,7 @@ func TestSimpleSign(t *testing.T) {
cm := newConfigMap("", "")
signer.configMaps.Add(cm)

secret := newTokenSecret("tokenID", "tokenSecret")
secret := newTokenSecret(testTokenID, "tokenSecret")
addSecretSigningUsage(secret, "true")
signer.secrets.Add(secret)

Expand All @@ -78,7 +80,7 @@ func TestSimpleSign(t *testing.T) {
expected := []core.Action{
core.NewUpdateAction(schema.GroupVersionResource{Version: "v1", Resource: "configmaps"},
api.NamespacePublic,
newConfigMap("tokenID", "eyJhbGciOiJIUzI1NiIsImtpZCI6InRva2VuSUQifQ..QAvK9DAjF0hSyASEkH1MOTB5rJMmbWEY9j-z1NSYILE")),
newConfigMap(testTokenID, "eyJhbGciOiJIUzI1NiIsImtpZCI6ImFiYzEyMyJ9..QSxpUG7Q542CirTI2ECPSZjvBOJURUW5a7XqFpNI958")),
}

verifyActions(t, expected, cl.Actions())
Expand All @@ -87,10 +89,10 @@ func TestSimpleSign(t *testing.T) {
func TestNoSignNeeded(t *testing.T) {
signer, cl := newBootstrapSigner()

cm := newConfigMap("tokenID", "eyJhbGciOiJIUzI1NiIsImtpZCI6InRva2VuSUQifQ..QAvK9DAjF0hSyASEkH1MOTB5rJMmbWEY9j-z1NSYILE")
cm := newConfigMap(testTokenID, "eyJhbGciOiJIUzI1NiIsImtpZCI6ImFiYzEyMyJ9..QSxpUG7Q542CirTI2ECPSZjvBOJURUW5a7XqFpNI958")
signer.configMaps.Add(cm)

secret := newTokenSecret("tokenID", "tokenSecret")
secret := newTokenSecret(testTokenID, "tokenSecret")
addSecretSigningUsage(secret, "true")
signer.secrets.Add(secret)

Expand All @@ -102,10 +104,10 @@ func TestNoSignNeeded(t *testing.T) {
func TestUpdateSignature(t *testing.T) {
signer, cl := newBootstrapSigner()

cm := newConfigMap("tokenID", "old signature")
cm := newConfigMap(testTokenID, "old signature")
signer.configMaps.Add(cm)

secret := newTokenSecret("tokenID", "tokenSecret")
secret := newTokenSecret(testTokenID, "tokenSecret")
addSecretSigningUsage(secret, "true")
signer.secrets.Add(secret)

Expand All @@ -114,7 +116,7 @@ func TestUpdateSignature(t *testing.T) {
expected := []core.Action{
core.NewUpdateAction(schema.GroupVersionResource{Version: "v1", Resource: "configmaps"},
api.NamespacePublic,
newConfigMap("tokenID", "eyJhbGciOiJIUzI1NiIsImtpZCI6InRva2VuSUQifQ..QAvK9DAjF0hSyASEkH1MOTB5rJMmbWEY9j-z1NSYILE")),
newConfigMap(testTokenID, "eyJhbGciOiJIUzI1NiIsImtpZCI6ImFiYzEyMyJ9..QSxpUG7Q542CirTI2ECPSZjvBOJURUW5a7XqFpNI958")),
}

verifyActions(t, expected, cl.Actions())
Expand All @@ -123,7 +125,7 @@ func TestUpdateSignature(t *testing.T) {
func TestRemoveSignature(t *testing.T) {
signer, cl := newBootstrapSigner()

cm := newConfigMap("tokenID", "old signature")
cm := newConfigMap(testTokenID, "old signature")
signer.configMaps.Add(cm)

signer.signConfigMap()
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/bootstrap/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func newTokenSecret(tokenID, tokenSecret string) *v1.Secret {
return &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: metav1.NamespaceSystem,
Name: "secretName",
Name: bootstrapapi.BootstrapTokenSecretPrefix + tokenID,
ResourceVersion: "1",
},
Type: bootstrapapi.SecretTypeBootstrapToken,
Expand Down
24 changes: 24 additions & 0 deletions pkg/controller/bootstrap/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package bootstrap

import (
"regexp"
"time"

"github.com/golang/glog"
Expand All @@ -25,6 +26,9 @@ import (
bootstrapapi "k8s.io/kubernetes/pkg/bootstrap/api"
)

var namePattern = `^` + regexp.QuoteMeta(bootstrapapi.BootstrapTokenSecretPrefix) + `([a-z0-9]{6})$`
var nameRegExp = regexp.MustCompile(namePattern)

// getSecretString gets a string value from a secret. If there is an error or
// if the key doesn't exist, an empty string is returned.
func getSecretString(secret *v1.Secret, key string) string {
Expand All @@ -36,13 +40,33 @@ func getSecretString(secret *v1.Secret, key string) string {
return string(data)
}

// parseSecretName parses the name of the secret to extract the secret ID.
func parseSecretName(name string) (secretID string, ok bool) {
r := nameRegExp.FindStringSubmatch(name)
if r == nil {
return "", false
}
return r[1], true
}

func validateSecretForSigning(secret *v1.Secret) (tokenID, tokenSecret string, ok bool) {
nameTokenID, ok := parseSecretName(secret.Name)
if !ok {
glog.V(3).Infof("Invalid secret name: %s. Must be of form %s<secret-id>.", secret.Name, bootstrapapi.BootstrapTokenSecretPrefix)
return "", "", false
}

tokenID = getSecretString(secret, bootstrapapi.BootstrapTokenIDKey)
if len(tokenID) == 0 {
glog.V(3).Infof("No %s key in %s/%s Secret", bootstrapapi.BootstrapTokenIDKey, secret.Namespace, secret.Name)
return "", "", false
}

if nameTokenID != tokenID {
glog.V(3).Infof("Token ID (%s) doesn't match secret name: %s", tokenID, nameTokenID)
Copy link
Member

Choose a reason for hiding this comment

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

It should stop here, right? This is an invalid Secret, so return "", "", false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arg -- right. I should have a test for that too.

return "", "", false
}

tokenSecret = getSecretString(secret, bootstrapapi.BootstrapTokenSecretKey)
if len(tokenSecret) == 0 {
glog.V(3).Infof("No %s key in %s/%s Secret", bootstrapapi.BootstrapTokenSecretKey, secret.Namespace, secret.Name)
Expand Down
75 changes: 72 additions & 3 deletions pkg/controller/bootstrap/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,16 @@ import (
"testing"
"time"

"github.com/stretchr/testify/assert"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/pkg/api/v1"
bootstrapapi "k8s.io/kubernetes/pkg/bootstrap/api"
)

const (
givenTokenID = "tokenID"
givenTokenID = "abc123"
givenTokenID2 = "def456"
givenTokenSecret = "tokenSecret"
)

Expand Down Expand Up @@ -81,7 +84,7 @@ func TestValidateSecretForSigning(t *testing.T) {
secret := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: metav1.NamespaceSystem,
Name: "secretName",
Name: bootstrapapi.BootstrapTokenSecretPrefix + givenTokenID,
ResourceVersion: "1",
},
Type: bootstrapapi.SecretTypeBootstrapToken,
Expand Down Expand Up @@ -113,7 +116,7 @@ func TestValidateSecret(t *testing.T) {
secret := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: metav1.NamespaceSystem,
Name: "secretName",
Name: bootstrapapi.BootstrapTokenSecretPrefix + givenTokenID,
ResourceVersion: "1",
},
Type: bootstrapapi.SecretTypeBootstrapToken,
Expand All @@ -135,3 +138,69 @@ func TestValidateSecret(t *testing.T) {
t.Errorf("Unexpected Token Secret. Expected %q, got %q", givenTokenSecret, tokenSecret)
}
}

func TestBadSecretName(t *testing.T) {
secret := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: metav1.NamespaceSystem,
Name: givenTokenID,
ResourceVersion: "1",
},
Type: bootstrapapi.SecretTypeBootstrapToken,
Data: map[string][]byte{
bootstrapapi.BootstrapTokenIDKey: []byte(givenTokenID),
bootstrapapi.BootstrapTokenSecretKey: []byte(givenTokenSecret),
bootstrapapi.BootstrapTokenUsageSigningKey: []byte("true"),
},
}

_, _, ok := validateSecretForSigning(secret)
if ok {
t.Errorf("Token validation should fail with bad name")
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Add case when metadata.name != data["token-id"]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

func TestMismatchSecretName(t *testing.T) {
secret := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: metav1.NamespaceSystem,
Name: bootstrapapi.BootstrapTokenSecretPrefix + givenTokenID2,
ResourceVersion: "1",
},
Type: bootstrapapi.SecretTypeBootstrapToken,
Data: map[string][]byte{
bootstrapapi.BootstrapTokenIDKey: []byte(givenTokenID),
bootstrapapi.BootstrapTokenSecretKey: []byte(givenTokenSecret),
bootstrapapi.BootstrapTokenUsageSigningKey: []byte("true"),
},
}

_, _, ok := validateSecretForSigning(secret)
if ok {
t.Errorf("Token validation should fail with mismatched name")
}
}

func TestParseSecretName(t *testing.T) {
tokenID, ok := parseSecretName("bootstrap-token-abc123")
assert.True(t, ok, "parseSecretName should accept valid name")
assert.Equal(t, "abc123", tokenID, "parseSecretName should return token ID")

_, ok = parseSecretName("")
assert.False(t, ok, "parseSecretName should reject blank name")

_, ok = parseSecretName("abc123")
assert.False(t, ok, "parseSecretName should reject with no prefix")

_, ok = parseSecretName("bootstrap-token-")
assert.False(t, ok, "parseSecretName should reject no token ID")

_, ok = parseSecretName("bootstrap-token-abc")
assert.False(t, ok, "parseSecretName should reject short token ID")

_, ok = parseSecretName("bootstrap-token-abc123ghi")
assert.False(t, ok, "parseSecretName should reject long token ID")

_, ok = parseSecretName("bootstrap-token-ABC123")
assert.False(t, ok, "parseSecretName should reject invalid token ID")
}