Skip to content

Commit

Permalink
controllers/vmagent: moves plain text secrets into file
Browse files Browse the repository at this point in the history 
instead of passing basicAuth password, bearerToken or oauth2 secret as plain text arg
path it as mounted secret from mounted pod dir
it improves security for vmagent
#440
  • Loading branch information
@f41gh7
f41gh7 committed Dec 29, 2022
1 parent 3510048 commit 18df7aa
Show file tree
Hide file tree
Showing 6 changed files with 80 additions and 64 deletions.
10 changes: 10 additions & 0 deletions api/v1beta1/vmagent_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,16 @@ type VMAgentRemoteWriteSpec struct {
Headers []string `json:"headers,omitempty"`
}

// AsMapKey key for internal cache map
func (rw *VMAgentRemoteWriteSpec) AsMapKey() string {
return fmt.Sprintf("remoteWrite-%s", rw.URL)
}

// AsSecretKey key for kubernetes secret data
func (rw *VMAgentRemoteWriteSpec) AsSecretKey(idx int, suffix string) string {
return fmt.Sprintf("RWS_%d-SECRET-%s", idx, strings.ToUpper(suffix))
}

// VmAgentStatus defines the observed state of VmAgent
// +k8s:openapi-gen=true
type VMAgentStatus struct {
Expand Down
53 changes: 21 additions & 32 deletions controllers/factory/scrapes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,43 +27,47 @@ type scrapesSecretsCache struct {
authorizationSecrets map[string]string
}

func CreateOrUpdateConfigurationSecret(ctx context.Context, cr *victoriametricsv1beta1.VMAgent, rclient client.Client, c *config.BaseOperatorConf) error {
func CreateOrUpdateConfigurationSecret(ctx context.Context, cr *victoriametricsv1beta1.VMAgent, rclient client.Client, c *config.BaseOperatorConf) (*scrapesSecretsCache, error) {

smons, err := SelectServiceScrapes(ctx, cr, rclient)
if err != nil {
return fmt.Errorf("selecting ServiceScrapes failed: %w", err)
return nil, fmt.Errorf("selecting ServiceScrapes failed: %w", err)
}

pmons, err := SelectPodScrapes(ctx, cr, rclient)
if err != nil {
return fmt.Errorf("selecting PodScrapes failed: %w", err)
return nil, fmt.Errorf("selecting PodScrapes failed: %w", err)
}

probes, err := SelectVMProbes(ctx, cr, rclient)
if err != nil {
return fmt.Errorf("selecting VMProbes failed: %w", err)
return nil, fmt.Errorf("selecting VMProbes failed: %w", err)
}

nodes, err := SelectVMNodeScrapes(ctx, cr, rclient)
if err != nil {
return fmt.Errorf("selecting VMNodeScrapes failed: %w", err)
return nil, fmt.Errorf("selecting VMNodeScrapes failed: %w", err)
}

statics, err := SelectStaticScrapes(ctx, cr, rclient)
if err != nil {
return fmt.Errorf("selecting PodScrapes failed: %w", err)
return nil, fmt.Errorf("selecting PodScrapes failed: %w", err)
}

ssCache, err := loadScrapeSecrets(ctx, rclient, smons, nodes, pmons, probes, statics, cr.Spec.APIServerConfig, nil, cr.Namespace)
ssCache, err := loadScrapeSecrets(ctx, rclient, smons, nodes, pmons, probes, statics, cr.Spec.APIServerConfig, cr.Spec.RemoteWrite, cr.Namespace)
if err != nil {
return fmt.Errorf("cannot load basic secrets for ServiceMonitors: %w", err)
return nil, fmt.Errorf("cannot load basic secrets for ServiceMonitors: %w", err)
}

additionalScrapeConfigs, err := loadAdditionalScrapeConfigsSecret(ctx, rclient, cr.Spec.AdditionalScrapeConfigs, cr.Namespace)
if err != nil {
return fmt.Errorf("loading additional scrape configs from Secret failed: %w", err)
return nil, fmt.Errorf("loading additional scrape configs from Secret failed: %w", err)
}

// how to store remoteWrite secrets properly?
// it must be keyed by some value
// e.g. basicAuth/idx/passwordFile
// e.g. bearer/idx/tokenFile
// Update secret based on the most recent configuration.
generatedConfig, err := generateConfig(
cr,
Expand All @@ -76,46 +80,31 @@ func CreateOrUpdateConfigurationSecret(ctx context.Context, cr *victoriametricsv
additionalScrapeConfigs,
)
if err != nil {
return fmt.Errorf("generating config for vmagent failed: %w", err)
return nil, fmt.Errorf("generating config for vmagent failed: %w", err)
}

s := makeConfigSecret(cr, c)
s := makeConfigSecret(cr, c, ssCache)
s.ObjectMeta.Annotations = map[string]string{
"generated": "true",
}

// Compress config to avoid 1mb secret limit for a while
var buf bytes.Buffer
if err = gzipConfig(&buf, generatedConfig); err != nil {
return fmt.Errorf("cannot gzip config for vmagent: %w", err)
return nil, fmt.Errorf("cannot gzip config for vmagent: %w", err)
}
s.Data[configFilename] = buf.Bytes()

curSecret := &v1.Secret{}
err = rclient.Get(ctx, types.NamespacedName{Namespace: cr.Namespace, Name: s.Name}, curSecret)
if errors.IsNotFound(err) {
log.Info("creating new configuration secret for vmagent")
return rclient.Create(ctx, s)
}

var (
generatedConf = s.Data[configFilename]
curConfig, curConfigFound = curSecret.Data[configFilename]
)
if curConfigFound {
if bytes.Equal(curConfig, generatedConf) {
log.Info("updating VMAgent configuration secret skipped, no configuration change")
return nil
}
log.Info("current VMAgent configuration has changed")
} else {
log.Info("no current VMAgent configuration secret found", "currentConfigFound", curConfigFound)
return ssCache, rclient.Create(ctx, s)
}

s.Annotations = labels.Merge(curSecret.Annotations, s.Annotations)
s.Finalizers = victoriametricsv1beta1.MergeFinalizers(curSecret, victoriametricsv1beta1.FinalizerName)
log.Info("updating VMAgent configuration secret")
return rclient.Update(ctx, s)
return ssCache, rclient.Update(ctx, s)
}

func SelectServiceScrapes(ctx context.Context, cr *victoriametricsv1beta1.VMAgent, rclient client.Client) (map[string]*victoriametricsv1beta1.VMServiceScrape, error) {
Expand Down Expand Up @@ -575,22 +564,22 @@ func loadScrapeSecrets(
if err != nil {
return nil, fmt.Errorf("could not generate basicAuth for remote write spec %s config. %w", rws.URL, err)
}
baSecrets[fmt.Sprintf("remoteWriteSpec/%s", rws.URL)] = &credentials
baSecrets[rws.AsMapKey()] = &credentials
}
if rws.OAuth2 != nil {
oauth2, err := loadOAuthSecrets(ctx, rclient, rws.OAuth2, namespace, nsSecretCache, nsCMCache)

if err != nil {
return nil, fmt.Errorf("cannot load oauth2 creds for :%s, ns: %s, err: %w", "remoteWrite", namespace, err)
}
oauth2Secret[fmt.Sprintf("remoteWriteSpec/%s", rws.URL)] = oauth2
oauth2Secret[rws.AsMapKey()] = oauth2
}
if rws.BearerTokenSecret != nil && rws.BearerTokenSecret.Name != "" {
token, err := getCredFromSecret(ctx, rclient, namespace, rws.BearerTokenSecret, buildCacheKey(namespace, rws.BearerTokenSecret.Name), nsSecretCache)
if err != nil {
return nil, fmt.Errorf("cannot get bearer token for remoteWrite: %w", err)
}
bearerSecrets[fmt.Sprintf("remoteWriteSpec/%s", rws.URL)] = token
bearerSecrets[rws.AsMapKey()] = token
}
}

Expand Down
29 changes: 27 additions & 2 deletions controllers/factory/scrapes_build.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package factory

import (
"fmt"
"github.com/VictoriaMetrics/VictoriaMetrics/lib/logger"
"path"
"reflect"
"regexp"
Expand Down Expand Up @@ -203,8 +204,8 @@ func generateConfig(
return yaml.Marshal(cfg)
}

func makeConfigSecret(cr *victoriametricsv1beta1.VMAgent, config *config.BaseOperatorConf) *v1.Secret {
return &v1.Secret{
func makeConfigSecret(cr *victoriametricsv1beta1.VMAgent, config *config.BaseOperatorConf, ssCache *scrapesSecretsCache) *v1.Secret {
s := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: cr.PrefixedName(),
Annotations: cr.AnnotationsFiltered(),
Expand All @@ -217,6 +218,30 @@ func makeConfigSecret(cr *victoriametricsv1beta1.VMAgent, config *config.BaseOpe
configFilename: {},
},
}
for idx, rw := range cr.Spec.RemoteWrite {
if rw.BearerTokenSecret != nil {
token, ok := ssCache.bearerTokens[rw.AsMapKey()]
if !ok {
logger.Fatalf("bug, remoteWriteSpec bearerToken is missing: %s", rw.AsMapKey())
}
s.Data[rw.AsSecretKey(idx, "bearerToken")] = []byte(token)
}
if rw.BasicAuth != nil && len(rw.BasicAuth.Password.Name) > 0 {
ba, ok := ssCache.baSecrets[rw.AsMapKey()]
if !ok {
logger.Fatalf("bug, remoteWriteSpec basicAuth is missing: %s", rw.AsMapKey())
}
s.Data[rw.AsSecretKey(idx, "basicAuthPassword")] = []byte(ba.password)
}
if rw.OAuth2 != nil {
oauth2, ok := ssCache.oauth2Secrets[rw.AsMapKey()]
if !ok {
logger.Fatalf("bug, remoteWriteSpec oauth2 is missing: %s", rw.AsMapKey())
}
s.Data[rw.AsSecretKey(idx, "oauth2Secret")] = []byte(oauth2.clientSecret)
}
}
return s
}

func sanitizeLabelName(name string) string {
Expand Down
2 changes: 1 addition & 1 deletion controllers/factory/scrapes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ scrape_configs:
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
testClient := k8stools.GetTestClientWithObjects(tt.predefinedObjects)
if err := CreateOrUpdateConfigurationSecret(context.TODO(), tt.args.cr, testClient, tt.args.c); (err != nil) != tt.wantErr {
if _, err := CreateOrUpdateConfigurationSecret(context.TODO(), tt.args.cr, testClient, tt.args.c); (err != nil) != tt.wantErr {
t.Errorf("CreateOrUpdateConfigurationSecret() error = %v, wantErr %v", err, tt.wantErr)
}
var expectSecret v1.Secret
Expand Down
44 changes: 18 additions & 26 deletions controllers/factory/vmagent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func CreateOrUpdateVMAgent(ctx context.Context, cr *victoriametricsv1beta1.VMAge
}
}
// we have to create empty or full cm first
err := CreateOrUpdateConfigurationSecret(ctx, cr, rclient, c)
ssCache, err := CreateOrUpdateConfigurationSecret(ctx, cr, rclient, c)
if err != nil {
l.Error(err, "cannot create configmap")
return reconcile.Result{}, err
Expand All @@ -107,12 +107,6 @@ func CreateOrUpdateVMAgent(ctx context.Context, cr *victoriametricsv1beta1.VMAge
}
}

// getting secrets for remotewrite spec
ssCache, err := LoadRemoteWriteSecrets(ctx, cr, rclient)
if err != nil {
return reconcile.Result{}, fmt.Errorf("cannot get remote write secrets for vmagent: %w", err)
}

newDeploy, err := newDeployForVMAgent(cr, c, ssCache)
if err != nil {
return reconcile.Result{}, fmt.Errorf("cannot build new deploy for vmagent: %w", err)
Expand Down Expand Up @@ -386,6 +380,11 @@ func makeSpecForVMAgent(cr *victoriametricsv1beta1.VMAgent, c *config.BaseOperat
ReadOnly: true,
MountPath: RelabelingConfigDir,
},
corev1.VolumeMount{
Name: "config",
ReadOnly: true,
MountPath: vmAgentConfDir,
},
)

for _, s := range cr.Spec.Secrets {
Expand Down Expand Up @@ -930,9 +929,8 @@ func BuildRemoteWrites(cr *victoriametricsv1beta1.VMAgent, ssCache *scrapesSecre
url = remoteFlag{flagSetting: "-remoteWrite.multitenantURL=", isNotNull: true}
}
authUser := remoteFlag{flagSetting: "-remoteWrite.basicAuth.username="}
authPassword := remoteFlag{flagSetting: "-remoteWrite.basicAuth.password="}
authPasswordFile := remoteFlag{flagSetting: "-remoteWrite.basicAuth.passwordFile="}
bearerToken := remoteFlag{flagSetting: "-remoteWrite.bearerToken="}
bearerTokenFile := remoteFlag{flagSetting: "-remoteWrite.bearerTokenFile="}
urlRelabelConfig := remoteFlag{flagSetting: "-remoteWrite.urlRelabelConfig="}
sendTimeout := remoteFlag{flagSetting: "-remoteWrite.sendTimeout="}
tlsCAs := remoteFlag{flagSetting: "-remoteWrite.tlsCAFile="}
Expand All @@ -941,7 +939,6 @@ func BuildRemoteWrites(cr *victoriametricsv1beta1.VMAgent, ssCache *scrapesSecre
tlsInsecure := remoteFlag{flagSetting: "-remoteWrite.tlsInsecureSkipVerify="}
tlsServerName := remoteFlag{flagSetting: "-remoteWrite.tlsServerName="}
oauth2ClientID := remoteFlag{flagSetting: "-remoteWrite.oauth2.clientID="}
oauth2ClientSecret := remoteFlag{flagSetting: "-remoteWrite.oauth2.clientSecret="}
oauth2ClientSecretFile := remoteFlag{flagSetting: "-remoteWrite.oauth2.clientSecretFile="}
oauth2Scopes := remoteFlag{flagSetting: "-remoteWrite.oauth2.scopes="}
oauth2TokenUrl := remoteFlag{flagSetting: "-remoteWrite.oauth2.tokenUrl="}
Expand Down Expand Up @@ -998,16 +995,15 @@ func BuildRemoteWrites(cr *victoriametricsv1beta1.VMAgent, ssCache *scrapesSecre
tlsInsecure.flagSetting += fmt.Sprintf("%v,", insecure)

var user string
var pass string
var passFile string
if rws.BasicAuth != nil {
if s, ok := ssCache.baSecrets[fmt.Sprintf("remoteWriteSpec/%s", rws.URL)]; ok {
if s, ok := ssCache.baSecrets[rws.AsMapKey()]; ok {
authUser.isNotNull = true

user = s.username
if len(s.password) > 0 {
authPassword.isNotNull = true
pass = s.password
authPasswordFile.isNotNull = true
passFile = path.Join(vmAgentConfDir, rws.AsSecretKey(i, "basicAuthPassword"))
}
if len(rws.BasicAuth.PasswordFile) > 0 {
passFile = rws.BasicAuth.PasswordFile
Expand All @@ -1016,17 +1012,14 @@ func BuildRemoteWrites(cr *victoriametricsv1beta1.VMAgent, ssCache *scrapesSecre
}
}
authUser.flagSetting += fmt.Sprintf("\"%s\",", strings.ReplaceAll(user, `"`, `\"`))
authPassword.flagSetting += fmt.Sprintf("\"%s\",", strings.ReplaceAll(pass, `"`, `\"`))
authPasswordFile.flagSetting += fmt.Sprintf("%s,", passFile)

var value string
if rws.BearerTokenSecret != nil && rws.BearerTokenSecret.Name != "" {
if s, ok := ssCache.bearerTokens[fmt.Sprintf("remoteWriteSpec/%s", rws.URL)]; ok {
bearerToken.isNotNull = true
value = s
}
bearerTokenFile.isNotNull = true
value = path.Join(vmAgentConfDir, rws.AsSecretKey(i, "bearerToken"))
}
bearerToken.flagSetting += fmt.Sprintf("\"%s\",", strings.ReplaceAll(value, `"`, `\"`))
bearerTokenFile.flagSetting += fmt.Sprintf("\"%s\",", strings.ReplaceAll(value, `"`, `\"`))

value = ""

Expand Down Expand Up @@ -1056,7 +1049,7 @@ func BuildRemoteWrites(cr *victoriametricsv1beta1.VMAgent, ssCache *scrapesSecre
}
headers.flagSetting += fmt.Sprintf("'%s',", value)
value = ""
var oaturl, oascopes, oaclientID, oaSecretKey, oaSecretKeyFile string
var oaturl, oascopes, oaclientID, oaSecretKeyFile string
if rws.OAuth2 != nil {
if len(rws.OAuth2.TokenURL) > 0 {
oauth2TokenUrl.isNotNull = true
Expand All @@ -1075,8 +1068,8 @@ func BuildRemoteWrites(cr *victoriametricsv1beta1.VMAgent, ssCache *scrapesSecre

sv := ssCache.oauth2Secrets[fmt.Sprintf("remoteWriteSpec/%s", rws.URL)]
if rws.OAuth2.ClientSecret != nil && sv != nil {
oaSecretKey = sv.clientSecret
oauth2ClientSecret.isNotNull = true
oauth2ClientSecretFile.isNotNull = true
oaSecretKeyFile = path.Join(vmAgentConfDir, rws.AsSecretKey(i, "oauth2Secret"))
}

if len(rws.OAuth2.ClientID.Name()) > 0 && sv != nil {
Expand All @@ -1087,13 +1080,12 @@ func BuildRemoteWrites(cr *victoriametricsv1beta1.VMAgent, ssCache *scrapesSecre
}
oauth2TokenUrl.flagSetting += fmt.Sprintf("%s,", oaturl)
oauth2ClientSecretFile.flagSetting += fmt.Sprintf("%s,", oaSecretKeyFile)
oauth2ClientSecret.flagSetting += fmt.Sprintf("%s,", oaSecretKey)
oauth2ClientID.flagSetting += fmt.Sprintf("%s,", oaclientID)
oauth2Scopes.flagSetting += fmt.Sprintf("%s,", oascopes)
}
remoteArgs = append(remoteArgs, url, authUser, authPassword, bearerToken, urlRelabelConfig, tlsInsecure, sendTimeout)
remoteArgs = append(remoteArgs, url, authUser, bearerTokenFile, urlRelabelConfig, tlsInsecure, sendTimeout)
remoteArgs = append(remoteArgs, tlsServerName, tlsKeys, tlsCerts, tlsCAs)
remoteArgs = append(remoteArgs, oauth2ClientID, oauth2ClientSecret, oauth2ClientSecretFile, oauth2Scopes, oauth2TokenUrl)
remoteArgs = append(remoteArgs, oauth2ClientID, oauth2ClientSecretFile, oauth2Scopes, oauth2TokenUrl)
remoteArgs = append(remoteArgs, headers, authPasswordFile)

for _, remoteArgType := range remoteArgs {
Expand Down
6 changes: 3 additions & 3 deletions controllers/factory/vmagent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@ func TestBuildRemoteWrites(t *testing.T) {
}},
},
},
want: []string{"-remoteWrite.oauth2.clientID=,some-id", "-remoteWrite.oauth2.clientSecret=,some-secret", "-remoteWrite.oauth2.scopes=,scope-1", "-remoteWrite.oauth2.tokenUrl=,http://some-url", "-remoteWrite.url=localhost:8429,localhost:8431", "-remoteWrite.sendTimeout=10s,15s"},
want: []string{"-remoteWrite.oauth2.clientID=,some-id", "-remoteWrite.oauth2.clientSecretFile=,/etc/vmagent/config/RWS_1-SECRET-OAUTH2SECRET", "-remoteWrite.oauth2.scopes=,scope-1", "-remoteWrite.oauth2.tokenUrl=,http://some-url", "-remoteWrite.url=localhost:8429,localhost:8431", "-remoteWrite.sendTimeout=10s,15s"},
},
{
name: "test bearer token",
Expand All @@ -703,7 +703,7 @@ func TestBuildRemoteWrites(t *testing.T) {
}},
},
},
want: []string{"-remoteWrite.bearerToken=\"\",\"token\"", "-remoteWrite.url=localhost:8429,localhost:8431", "-remoteWrite.sendTimeout=10s,15s"},
want: []string{"-remoteWrite.bearerTokenFile=\"\",\"/etc/vmagent/config/RWS_1-SECRET-BEARERTOKEN\"", "-remoteWrite.url=localhost:8429,localhost:8431", "-remoteWrite.sendTimeout=10s,15s"},
},
{
name: "test with headers",
Expand Down Expand Up @@ -731,7 +731,7 @@ func TestBuildRemoteWrites(t *testing.T) {
}},
},
},
want: []string{"-remoteWrite.bearerToken=\"\",\"token\"", "-remoteWrite.headers='','key: value^^second-key: value2'", "-remoteWrite.url=localhost:8429,localhost:8431", "-remoteWrite.sendTimeout=10s,15s"},
want: []string{"-remoteWrite.bearerTokenFile=\"\",\"/etc/vmagent/config/RWS_1-SECRET-BEARERTOKEN\"", "-remoteWrite.headers='','key: value^^second-key: value2'", "-remoteWrite.url=localhost:8429,localhost:8431", "-remoteWrite.sendTimeout=10s,15s"},
},
}
for _, tt := range tests {
Expand Down

0 comments on commit 18df7aa

Please sign in to comment.