Use separate annotation to specify the secret filename #89

sbeaulie · 2020-02-28T21:59:02Z

as per issue #88 before this fix the secret filename was limited to 21 chars and could not contain a directory separator "/"

This fix introduces two new annotations to use together

vault.hashicorp.com/agent-inject-location to specify the vault secret location
vault.hashicorp.com/agent-inject-filename to specify the filename to realize

It has the advantage of allowing long filenames, include directory separators
and if the location is absolute, it will create the file there, effectively giving
the option to save outside of /vault/secrets/

Note: backwards compatibility is respected for vault.hashicorp.com/agent-inject-secret knowing that it has limitations:

filename length of 21 (or 23 if not using templates)
cannot use "/" directory separator
filename is forced to lowercase version, I do not know the original reason for this, but kept it here

hashicorp-cla · 2020-02-28T21:59:06Z

All committers have signed the CLA.

as per issue hashicorp#88 before this fix the secret filename was limited to 21 chars and could not contain a directory separator /. This fix introduces two new annotations to use together vault.hashicorp.com/agent-inject-location to specify the vault secret location vault.hashicorp.com/agent-inject-filename to specify the filename to realize It has the advantage of allowing long filenames, include directory separators and if the location is absolute, it will create the file there, effectively giving the option to save outside of /vault/secrets/

sbeaulie · 2020-02-28T22:05:24Z

Of course I push this then realize there is a similar PR #71 made by @smitthakkar96
My PR includes a way to define the "mountpath" by setting an absolute location (read starting with "/") and also allows for a long filenames with "/" in them. It preserves case sensitivity because it reads the value of the annotation as-is and does not apply any forced lowercase to it, so hopefully it also meets this requirement.

smitthakkar96 · 2020-02-29T11:00:38Z

agent-inject/agent/annotations.go

+	// Anything after the final "-" is used as a unique id to match annotation rules together
+	// use in combination with AnnotationAgentInjectLocation
+	// If you specify a filename with an absolute location like "/tmp/oreo/my_secret.pem"
+	// it will get created there, if not it defaults to using a root directory of "/vault/secrets/""


/vault/secrets is a volume mount, if we are gonna do this we need to create additional volume mounts otherwise the secret won't be available in the main container

Yes, of course

smitthakkar96 · 2020-02-29T11:05:18Z

agent-inject/agent/annotations.go

@@ -20,18 +20,41 @@ const (
 	// be set to a true or false value, as parseable by strconv.ParseBool
 	AnnotationAgentInject = "vault.hashicorp.com/agent-inject"

-	// AnnotationAgentInjectSecret is the key annotation that configures Vault
+	// (Deprecated) AnnotationAgentInjectSecret is the key annotation that configures Vault


Since we are deprecating this, why not store json as the value of annotation, that way we can have more structured data when we process and also it would require less number of annotations.

Example

vault.hashicorp.com/agent-inject-secret-config-FOOBAR: '{"secretPath": "secrets/service/database", "fileName": "database", "mountPath": "/etc/container_environment", "template": "<template>", "postInjectionCommand": "<optional command>"}'

PS: we can come up with better names

Thoughts?

I can easily make that change but would like some guidance from the code owners here. If we are to move to one annotation with metadata, should we still also support the old annotations? (I think not)
If we want to support the legacy annotations, and both are present, which one should take precedence?

IMO new one should take precedence and we should support old one for backwards comparability and print warning in logs everytime it’s used

Throwing error incase both are present can also be an option. There is something definitely wrong if both are present.

smitthakkar96 · 2020-02-29T11:06:28Z

agent-inject/agent/annotations.go

+		raw := strings.ReplaceAll(name, secretNamePrefix, "")
+		name = strings.ToLower(raw) // why would we force-lowercase the filename here?
+		if len(name) >= 21 || strings.Contains(name, "/") {
+			//houston we have a problem, skip secret, can we log this somewhere?


Why are we doing this?

We should not skip but return a failure response IMHO. Errors should never pass silently. Container coming up with missing env var can fail or might run on a default that might cause a security issue.

Yes we can use the error class here.

sbeaulie · 2020-03-04T15:26:30Z

agent-inject/agent/config.go

 		tmpl := &Template{
 			Contents:    template,
-			Destination: fmt.Sprintf("/vault/secrets/%s", secret.Name),
+			Destination: destination,


based on further testing, it seems that there is a bug when the destination name contains a "/". Can someone help me trace this issue? Would it be in the vault agent code or the consul template code?

The issue is that the template does not seem to be applied if I chose a destination with that forward slash, we get the default

{{ with secret \"%s\" }}{{ range $k, $v := .Data }}{{ $k }}: {{ $v }}\n{{ end }}{{ end }}

ie the file is created with the right filename containing the "/", but the template is not applied to it.

When I select a destination without "/" the template is applied properly.

It's as if it will apply the template only to files within 1 level of "/vault/secrets/"

Also any filename longer than about 20 char gets created, but the same issue where the template does not get applied happens.

@tvoran or @jasonodonnell if you could help me trace that issue I could fix it too! I took a look at the vault agent code and cannot find where that length limitation could be. The unit tests include filename lengths that are quite long too so I'm wondering if the issue is somewhere in the integration of vault-k8s to vault-agent?

{ "destination": "/Users/smit.thakkar/projects/container_environment/CHAT_DJANGO_SECRET_KEY", "contents": "{{- with secret \"secret/beta/chat-service/chat-service-beta-tech/CHAT_DJANGO_SECRET_KEY\" -}} {{ .Data.value }} TEST_SMIT {{- end }}", "left_delimiter": "{{", "right_delimiter": "}}" },

I tried above and it worked for me.

I don't think filename longer than 20 char has any issue, what version are you using

when the destination name contains a "/" I get below error
2020-03-06T16:49:22.480+0400 [ERROR] template.server: template server error: error="error rendering "(dynamic)" => "/Users/smit.thakkar/projects/container_environment/CHAT_DJANGO_SECRET_KEY/": failed reading file: open /Users/smit.thakkar/projects/container_environment/CHAT_DJANGO_SECRET_KEY/: not a directory"

OR

2020-03-06T16:49:09.889+0400 [ERROR] template.server: template server error: error="error rendering "(dynamic)" => "/Users/smit.thakkar/projects/container_environment/": failed reading file: read /Users/smit.thakkar/projects/container_environment/: is a directory"

tvoran · 2020-07-17T01:03:55Z

Hi @sbeaulie, thanks for the work on this!

I would suggest removing the vault.hashicorp.com/agent-inject-location- annotation, since the secret location in vault is already specified using the vault.hashicorp.com/agent-inject-secret-<secret-name> annotation. Then if the vault.hashicorp.com/agent-inject-filename- annotation is present, it would be used for the file name (and possibly path) when rendering the secret; otherwise the <secret-name> would be used for the filename under /vault/secrets/. I think this would probably involve adding a FileName field to the Secret struct. Does that make sense?

Let me know if you'd like to continue working on this, otherwise I can continue it in another branch.

tvoran · 2020-07-23T01:47:05Z

Continued in #158

smitthakkar96 reviewed Feb 29, 2020

View reviewed changes

tvoran added enhancement New feature or request injector Area: mutating webhook service labels Mar 3, 2020

sbeaulie commented Mar 4, 2020

View reviewed changes

tvoran self-assigned this Jul 15, 2020

tvoran self-requested a review July 16, 2020 16:14

tvoran mentioned this pull request Jul 23, 2020

Add file annotation #158

Merged

tvoran closed this Jul 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use separate annotation to specify the secret filename #89

Use separate annotation to specify the secret filename #89

sbeaulie commented Feb 28, 2020 •

edited

Loading

hashicorp-cla commented Feb 28, 2020 •

edited

Loading

sbeaulie commented Feb 28, 2020

smitthakkar96 Feb 29, 2020

sbeaulie Mar 3, 2020

smitthakkar96 Feb 29, 2020 •

edited

Loading

sbeaulie Mar 3, 2020 •

edited

Loading

smitthakkar96 Mar 3, 2020

smitthakkar96 Mar 3, 2020

smitthakkar96 Feb 29, 2020 •

edited

Loading

smitthakkar96 Feb 29, 2020

sbeaulie Mar 3, 2020

sbeaulie Mar 4, 2020 •

edited

Loading

sbeaulie Mar 4, 2020

sbeaulie Mar 5, 2020

smitthakkar96 Mar 6, 2020

smitthakkar96 Mar 6, 2020

smitthakkar96 Mar 6, 2020

tvoran commented Jul 17, 2020

tvoran commented Jul 23, 2020

Use separate annotation to specify the secret filename #89

Use separate annotation to specify the secret filename #89

Conversation

sbeaulie commented Feb 28, 2020 • edited Loading

hashicorp-cla commented Feb 28, 2020 • edited Loading

sbeaulie commented Feb 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitthakkar96 Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

sbeaulie Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitthakkar96 Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbeaulie Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvoran commented Jul 17, 2020

tvoran commented Jul 23, 2020

sbeaulie commented Feb 28, 2020 •

edited

Loading

hashicorp-cla commented Feb 28, 2020 •

edited

Loading

smitthakkar96 Feb 29, 2020 •

edited

Loading

sbeaulie Mar 3, 2020 •

edited

Loading

smitthakkar96 Feb 29, 2020 •

edited

Loading

sbeaulie Mar 4, 2020 •

edited

Loading