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

Support user provided service-account-signing-key and issuer #1012

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
7 changes: 7 additions & 0 deletions bindata/bootkube/config/bootstrap-config-overrides.yaml
Expand Up @@ -72,6 +72,13 @@ apiServerArguments:
- /etc/kubernetes/secrets/aggregator-signer.crt
service-account-key-file:
- /etc/kubernetes/secrets/service-account.pub
{{- if .UserProvidedBoundSASigningKey}}
- /etc/kubernetes/secrets/bound-service-account-signing-key.pub
{{- end}}
service-account-issuer: {{if .ServiceAccountIssuer}}
- {{.ServiceAccountIssuer}}{{end}}
service-account-signing-key-file: {{if .UserProvidedBoundSASigningKey}}
- /etc/kubernetes/secrets/bound-service-account-signing-key.key{{end}}
tls-cert-file:
- /etc/kubernetes/secrets/kube-apiserver-service-network-server.crt
tls-private-key-file:
Expand Down
12 changes: 12 additions & 0 deletions bindata/bootkube/manifests/secret-bound-sa-token-signing-key.yaml
@@ -0,0 +1,12 @@
apiVersion: v1
kind: Secret
metadata:
name: next-bound-service-account-signing-key
namespace: openshift-kube-apiserver-operator
annotations: {{- if .UserProvidedBoundSASigningKey}}
"auth.openshift.io/user-supplied-bound-token-key": "true" {{end}}
data:
{{- if .UserProvidedBoundSASigningKey}}
service-account.key: {{ .Assets | load "bound-service-account-signing-key.key" | base64 }}
service-account.pub: {{ .Assets | load "bound-service-account-signing-key.pub" | base64 }}
{{- end}}
44 changes: 44 additions & 0 deletions pkg/cmd/render/render.go
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"io/ioutil"
"net"
"os"
"path/filepath"

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand Down Expand Up @@ -38,6 +39,7 @@ type renderOpts struct {
etcdServerURLs []string
etcdServingCA string
clusterConfigFile string
clusterAuthFile string
}

// NewRenderCommand creates a render command.
Expand Down Expand Up @@ -79,6 +81,7 @@ func (r *renderOpts) AddFlags(fs *pflag.FlagSet) {
fs.StringArrayVar(&r.etcdServerURLs, "manifest-etcd-server-urls", r.etcdServerURLs, "The etcd server URL, comma separated.")
fs.StringVar(&r.etcdServingCA, "manifest-etcd-serving-ca", r.etcdServingCA, "The etcd serving CA.")
fs.StringVar(&r.clusterConfigFile, "cluster-config-file", r.clusterConfigFile, "Openshift Cluster API Config file.")
fs.StringVar(&r.clusterAuthFile, "cluster-auth-file", r.clusterAuthFile, "Openshift Cluster Authentication API Config file.")
}

// Validate verifies the inputs.
Expand Down Expand Up @@ -142,6 +145,10 @@ type TemplateData struct {

// BindNetwork is the network (tcp4 or tcp6) to bind to
BindNetwork string

ServiceAccountIssuer string

UserProvidedBoundSASigningKey bool
}

// Run contains the logic of the render command.
Expand All @@ -162,6 +169,22 @@ func (r *renderOpts) Run() error {
return fmt.Errorf("unable to parse restricted CIDRs from config %q: %v", r.clusterConfigFile, err)
}
}
if len(r.clusterAuthFile) > 0 {
clusterAuthFileData, err := ioutil.ReadFile(r.clusterAuthFile)
if err != nil && !os.IsNotExist(err) {
Copy link
Contributor

@tnozicka tnozicka Nov 25, 2020

Choose a reason for hiding this comment

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

ignoring a not found error for a flag that is explicitly set should at least log what happened

return fmt.Errorf("failed to load authentication config: %v", err)
}
if len(clusterAuthFileData) > 0 {
if err := discoverServiceAccountIssuer(clusterAuthFileData, &renderConfig); err != nil {
return fmt.Errorf("unable to parse service-account issuers from config %q: %v", r.clusterAuthFile, err)
}
}
}
if _, err := os.Stat(filepath.Join(r.generic.AssetInputDir, "bound-service-account-signing-key.key")); err == nil {
if _, err := os.Stat(filepath.Join(r.generic.AssetInputDir, "bound-service-account-signing-key.pub")); err == nil {
renderConfig.UserProvidedBoundSASigningKey = true
}
}
if len(renderConfig.ClusterCIDR) > 0 {
anyIPv4 := false
for _, cidr := range renderConfig.ClusterCIDR {
Expand Down Expand Up @@ -283,6 +306,27 @@ func mustReadTemplateFile(fname string) genericrenderoptions.Template {
return genericrenderoptions.Template{FileName: fname, Content: bs}
}

func discoverServiceAccountIssuer(clusterAuthFileData []byte, renderConfig *TemplateData) error {
configJson, err := yaml.YAMLToJSON(clusterAuthFileData)
if err != nil {
return err
}
clusterConfigObj, err := runtime.Decode(unstructured.UnstructuredJSONScheme, configJson)
if err != nil {
return err
}
clusterConfig, ok := clusterConfigObj.(*unstructured.Unstructured)
if !ok {
return fmt.Errorf("unexpected object in %t", clusterConfigObj)
}
issuer, found, err := unstructured.NestedString(
clusterConfig.Object, "spec", "serviceAccountIssuer")
if found && err == nil {
renderConfig.ServiceAccountIssuer = issuer
}
return err
}

func discoverCIDRs(clusterConfigFileData []byte, renderConfig *TemplateData) error {
if err := discoverCIDRsFromNetwork(clusterConfigFileData, renderConfig); err != nil {
if err = discoverCIDRsFromClusterAPI(clusterConfigFileData, renderConfig); err != nil {
Expand Down
221 changes: 221 additions & 0 deletions pkg/cmd/render/render_test.go
Expand Up @@ -129,6 +129,43 @@ func TestDiscoverCIDRsFromClusterAPI(t *testing.T) {
}
}

func TestDiscoverServiceAccountIssuer(t *testing.T) {
tests := []struct {
config string

issuer string
}{{
config: `apiVersion: config.openshift.io/v1
kind: Authentication
metadata:
name: cluster
spec: {}`,
}, {
config: `apiVersion: config.openshift.io/v1
kind: Authentication
metadata:
name: cluster
spec:
serviceAccountIssuer: https://test.dummy.url`,
issuer: "https://test.dummy.url",
}}
for _, test := range tests {
t.Run("", func(t *testing.T) {
renderConfig := TemplateData{
LockHostPath: "",
EtcdServerURLs: []string{""},
EtcdServingCA: "",
}
if err := discoverServiceAccountIssuer([]byte(test.config), &renderConfig); err != nil {
t.Fatalf("failed to discoverServiceAccountIssuer: %v", err)
}
if !reflect.DeepEqual(renderConfig.ServiceAccountIssuer, test.issuer) {
t.Fatalf("Got: %s, expected: %v", renderConfig.ServiceAccountIssuer, test.issuer)
}
})
}
}

func TestDiscoverCIDRs(t *testing.T) {
testCase := []struct {
config []byte
Expand Down Expand Up @@ -276,6 +313,190 @@ func TestRenderCommand(t *testing.T) {
return nil
},
},
{
name: "scenario 4 checks service account issuer when authentication no exists",
args: []string{
"--asset-input-dir=" + assetsInputDir,
"--templates-input-dir=" + templateDir,
"--cluster-auth-file=" + filepath.Join(assetsInputDir, "authentication.yaml"),
"--asset-output-dir=",
"--config-output-file=",
},
testFunction: func(cfg *kubecontrolplanev1.KubeAPIServerConfig) error {
if len(cfg.APIServerArguments["service-account-issuer"]) > 0 {
return fmt.Errorf("expected the service-account-issuer to be empty, but it was %s", cfg.APIServerArguments["service-account-issuer"])
}
return nil
},
},
{
name: "scenario 5 checks service account issuer when authentication exists but empty",
args: []string{
"--asset-input-dir=" + assetsInputDir,
"--templates-input-dir=" + templateDir,
"--cluster-auth-file=" + filepath.Join(assetsInputDir, "authentication.yaml"),
"--asset-output-dir=",
"--config-output-file=",
},
setupFunction: func() error {
data := ``
return ioutil.WriteFile(filepath.Join(assetsInputDir, "authentication.yaml"), []byte(data), 0644)
},
testFunction: func(cfg *kubecontrolplanev1.KubeAPIServerConfig) error {
if len(cfg.APIServerArguments["service-account-issuer"]) > 0 {
return fmt.Errorf("expected the service-account-issuer to be empty, but it was %s", cfg.APIServerArguments["service-account-issuer"])
}
return nil
},
},
{
name: "scenario 6 checks service account issuer when authentication exists but empty spec",
args: []string{
"--asset-input-dir=" + assetsInputDir,
"--templates-input-dir=" + templateDir,
"--cluster-auth-file=" + filepath.Join(assetsInputDir, "authentication.yaml"),
"--asset-output-dir=",
"--config-output-file=",
},
setupFunction: func() error {
data := `apiVersion: config.openshift.io/v1
kind: Authentication
metadata:
name: cluster
spec: {}`
return ioutil.WriteFile(filepath.Join(assetsInputDir, "authentication.yaml"), []byte(data), 0644)
},
testFunction: func(cfg *kubecontrolplanev1.KubeAPIServerConfig) error {
if len(cfg.APIServerArguments["service-account-issuer"]) > 0 {
return fmt.Errorf("expected the service-account-issuer to be empty, but it was %s", cfg.APIServerArguments["service-account-issuer"])
}
return nil
},
},
{
name: "scenario 7 checks service account issuer when authentication spec has issuer set",
args: []string{
"--asset-input-dir=" + assetsInputDir,
"--templates-input-dir=" + templateDir,
"--cluster-auth-file=" + filepath.Join(assetsInputDir, "authentication.yaml"),
"--asset-output-dir=",
"--config-output-file=",
},
setupFunction: func() error {
data := `apiVersion: config.openshift.io/v1
kind: Authentication
metadata:
name: cluster
spec:
serviceAccountIssuer: https://test.dummy.url`
return ioutil.WriteFile(filepath.Join(assetsInputDir, "authentication.yaml"), []byte(data), 0644)
},
testFunction: func(cfg *kubecontrolplanev1.KubeAPIServerConfig) error {
if len(cfg.APIServerArguments["service-account-issuer"]) == 0 {
return fmt.Errorf("expected the service-account-issuer to be set, but it was empty")
}
if !reflect.DeepEqual(cfg.APIServerArguments["service-account-issuer"], kubecontrolplanev1.Arguments([]string{"https://test.dummy.url"})) {
return fmt.Errorf("expected the service-account-issuer to be [ https://test.dummy.url ], but it was %s", cfg.APIServerArguments["service-account-issuer"])
}
return nil
},
},
{
name: "scenario 8 no user provided bound-sa-signing-keys",
args: []string{
"--asset-input-dir=" + assetsInputDir,
"--templates-input-dir=" + templateDir,
"--asset-output-dir=",
"--config-output-file=",
},
testFunction: func(cfg *kubecontrolplanev1.KubeAPIServerConfig) error {
if len(cfg.APIServerArguments["service-account-signing-key-file"]) > 0 {
return fmt.Errorf("expected the service-account-issuer to be empty, but it was %s", cfg.APIServerArguments["service-account-signing-key-file"])
}
return nil
},
},
{
name: "scenario 9 user provided bound-sa-signing-key only no public part",
args: []string{
"--asset-input-dir=" + filepath.Join(assetsInputDir, "0"),
"--templates-input-dir=" + templateDir,
"--asset-output-dir=",
"--config-output-file=",
},
setupFunction: func() error {
data := `DUMMY DATA`
if err := os.Mkdir(filepath.Join(assetsInputDir, "0"), 0700); err != nil {
return err
}
return ioutil.WriteFile(filepath.Join(assetsInputDir, "0", "bound-service-account-signing-key.key"), []byte(data), 0644)
},
testFunction: func(cfg *kubecontrolplanev1.KubeAPIServerConfig) error {
if len(cfg.APIServerArguments["service-account-signing-key-file"]) > 0 {
return fmt.Errorf("expected the service-account-issuer to be empty, but it was %s", cfg.APIServerArguments["service-account-signing-key-file"])
}
return nil
},
},
{
name: "scenario 10 user provided bound-sa-signing-key only public part",
args: []string{
"--asset-input-dir=" + filepath.Join(assetsInputDir, "1"),
"--templates-input-dir=" + templateDir,
"--asset-output-dir=",
"--config-output-file=",
},
setupFunction: func() error {
data := `DUMMY DATA`
if err := os.Mkdir(filepath.Join(assetsInputDir, "1"), 0700); err != nil {
return err
}
return ioutil.WriteFile(filepath.Join(assetsInputDir, "1", "bound-service-account-signing-key.pub"), []byte(data), 0644)
},
testFunction: func(cfg *kubecontrolplanev1.KubeAPIServerConfig) error {
if len(cfg.APIServerArguments["service-account-signing-key-file"]) > 0 {
return fmt.Errorf("expected the service-account-issuer to be empty, but it was %s", cfg.APIServerArguments["service-account-signing-key-file"])
}
return nil
},
},
{
name: "scenario 11 user provided bound-sa-signing-key and public part",
args: []string{
"--asset-input-dir=" + filepath.Join(assetsInputDir, "2"),
"--templates-input-dir=" + templateDir,
"--asset-output-dir=",
"--config-output-file=",
},
setupFunction: func() error {
data := `DUMMY DATA`
if err := os.Mkdir(filepath.Join(assetsInputDir, "2"), 0700); err != nil {
return err
}
if err := ioutil.WriteFile(filepath.Join(assetsInputDir, "2", "bound-service-account-signing-key.key"), []byte(data), 0644); err != nil {
return err
}
if err := ioutil.WriteFile(filepath.Join(assetsInputDir, "2", "bound-service-account-signing-key.pub"), []byte(data), 0644); err != nil {
return err
}
return nil
},
testFunction: func(cfg *kubecontrolplanev1.KubeAPIServerConfig) error {
if len(cfg.APIServerArguments["service-account-signing-key-file"]) == 0 {
return fmt.Errorf("expected the service-account-issuer to be set, but it was empty")
}
if !reflect.DeepEqual(cfg.APIServerArguments["service-account-signing-key-file"], kubecontrolplanev1.Arguments([]string{"/etc/kubernetes/secrets/bound-service-account-signing-key.key"})) {
return fmt.Errorf("expected the service-account-issuer to be [ /etc/kubernetes/secrets/bound-service-account-signing-key.key ], but it was %s", cfg.APIServerArguments["service-account-signing-key-file"])
}
if !reflect.DeepEqual(
cfg.APIServerArguments["service-account-key-file"],
kubecontrolplanev1.Arguments([]string{"/etc/kubernetes/secrets/service-account.pub", "/etc/kubernetes/secrets/bound-service-account-signing-key.pub"}),
) {
return fmt.Errorf("expected the service-account-issuer to be [ /etc/kubernetes/secrets/service-account.pub , /etc/kubernetes/secrets/bound-service-account-signing-key.pub ], but it was %s", cfg.APIServerArguments["service-account-key-file"])
}
return nil
},
},
}

for _, test := range tests {
Expand Down
3 changes: 3 additions & 0 deletions pkg/operator/boundsatokensignercontroller/controller.go
Expand Up @@ -153,6 +153,9 @@ func (c *BoundSATokenSignerController) ensurePublicKeyConfigMap(ctx context.Cont
}

currPublicKey := string(operatorSecret.Data[PublicKeyKey])
if currPublicKey == "" {
return fmt.Errorf("no current %s found, one must be set in %s/%s secret", PublicKeyKey, operatorNamespace, NextSigningKeySecretName)
}
hasKey := configMapHasValue(configMap, currPublicKey)
if !hasKey {
// Increment until a unique name is found to ensure that the new public key
Expand Down