Find better format for the names of secret keys

[pmorie]

Currently the key of a piece of secret data has to be a DNS subdomain. We went with DNS subdomains instead of defining a new format during the implementation because we assumed that a dns subdomain was a strict subset of the eventual format to use. It has been suggested in #7974 that we create a format to model file names use that format to validate keys in secrets.

[thockin]

Is there any reason to be restrictive at all? The assumption is that the
key in Secret.Data is the eventual filename in the volume, so why not just
allow it to be anything, just enforce it is a valid relative pathname.
e.g. "foo/.my.key." is valid, as is "my-key" or "MY_KEY". "/etc/passwd"
is not valid. "../../../../../../etc/passwd" is not valid.

Or go simpler and disallow '/' (no dirs, just files) and allow anything but
'\0' and '/'.

Or decouple and make the secret volume have a map of secret.data key ->
filename, then choose one of the above semantics. Given that Secrets are
not necessarily just files, this seems most flexible.

On Wed, May 13, 2015 at 9:35 PM, Paul Morie notifications@github.com
wrote:

Currently the key of a piece of secret data has to be a DNS subdomain. We
went with DNS subdomains instead of defining a new format during the
implementation because we assumed that a dns subdomain was a strict subset
of the eventual format to use. It has been suggested in #7974
#7974 that we
create a format to model file names use that format to validate keys in
secrets.

—
Reply to this email directly or view it on GitHub
#8236.

[deads2k]

Is this going to supersede #8072? I'm interested because allowing leading dots allows for a cleaner secret implementation for .dockercfg files.

Also, I'm a fan allowing anything except:

1. leading /
2. just . or .. as the only value in any step.

[pmorie]

Or decouple and make the secret volume have a map of secret.data key ->
filename, then choose one of the above semantics. Given that Secrets are
not necessarily just files, this seems most flexible.

This is what #4789 was about. There's a pull open for it. Thinking about it fresh, I would rather decouple than change the format of the keys. @deads2k, opinion?

[deads2k]

I could see using a list of structured objects

type SecretData struct{
    name string
    permissions int `omitempty`
    data []byte
}

type Secret struct{
    TypeMeta
    ObjectMeta

    // validation rule to prevent data.name collisions
    Data []SecretData `patchStrategy=merge patchKey=name`
}

The conversions and backwards compatibility would be a little ugly, but it ought to be possible.

[pmorie]

Wrong issue? I don't see how this is related to key names and I know patch
was being discussed in another issue.
On Thu, May 14, 2015 at 7:23 PM David Eads notifications@github.com wrote:

I could see using a list of structured objects

type SecretData struct{
name string
permissions int omitempty
data []byte
}

type Secret struct{
TypeMeta
ObjectMeta

// validation rule to prevent data.name collisions
Data []SecretData `patchStrategy=merge patchKey=name`

}

The conversions and backwards compatibility would be a little ugly, but it
ought to be possible.

—
Reply to this email directly or view it on GitHub
#8236 (comment)
.

[lavalamp]

Why is this hard? We use these to make filenames. We should require that they be valid filenames. We'll have @thockin review the validation function to detect sneaky exploits. :)

[thockin]

Well, Secrets CAN be stored in files but I don't think they necessarily
are. For example, permissions are a concept that ONLY applies to files -
it properly belongs in the volume object, not the secret itself. Likewise
filenames != secret names.

That may be a bit over normalized for real life, but let's not just smash
concepts together without exploring the proper alignment.

On Thu, May 14, 2015 at 5:48 PM, Daniel Smith notifications@github.com
wrote:

Why is this hard? We use these to make filenames. We should require that
they be valid filenames. We'll have @thockin https://github.com/thockin
review the validation function to detect sneaky exploits. :)

—
Reply to this email directly or view it on GitHub
#8236 (comment)
.

[pmorie]

That may be a bit over normalized for real life, but let's not just smash concepts
together without exploring the proper alignment.

+1 to that.

On Fri, May 15, 2015 at 12:28 AM, Tim Hockin notifications@github.com
wrote:

Well, Secrets CAN be stored in files but I don't think they necessarily
are. For example, permissions are a concept that ONLY applies to files -
it properly belongs in the volume object, not the secret itself. Likewise
filenames != secret names.

That may be a bit over normalized for real life, but let's not just smash
concepts together without exploring the proper alignment.

On Thu, May 14, 2015 at 5:48 PM, Daniel Smith notifications@github.com
wrote:

Why is this hard? We use these to make filenames. We should require that
they be valid filenames. We'll have @thockin <https://github.com/thockin

review the validation function to detect sneaky exploits. :)

—
Reply to this email directly or view it on GitHub
<
#8236 (comment)

.

—
Reply to this email directly or view it on GitHub
#8236 (comment)
.

[deads2k]

As a user trying to use the system, having a single object that contains a single mapping for my secrets is very easy to understand an use. key==filename is very natural and makes it easy for me to create and update my secrets without creating invalid conditions.

The create case becomes much more challenging. Instead of creating a single object, you have a secrets object with one dataKey->data mapping with restrictive keys and then a secrets-mapping object with another dataKey->filename. As user, this means that both objects have to match in order to get my secret data mapped to the correct filenames in the container.

The update case is even more difficult. With a single object, update one object and you're done. With a secret and a separate mapping, you have to coordinate updates between two different objects. If you update in the wrong order, you will end up with mismatched secrets and mappings.

Having a struct like this: #8236 (comment) or even the structure we currently have with more lenient keys allows most of the flexibility without introducing the additional difficulty of coordinated objects.

[pmorie]

@deads2k's point above swayed me on the subject of allowing filename-like names of secret keys. I don't think we need to fully solve that problem at this moment, in the sense of allowing relative paths, but I'm okay relaxing the key validation to allow leading dots as @deads2k wants to do in #8072.

Any thoughts @lavalamp @thockin ?

[derekwaynecarr]

If secrets had supported:

1. keys with a leading .
2. keys that allow subdirectories

This issue #4997 would not have taken me 2 mos to solve. Luckily, OpenShift 3 was in active development, and we could change the product to flatten our secret configuration data into a single directory whose filenames could align with the current limited secret key.

I fear other products will have similar issues, but will not be so willing to adapt to run on Kube.

I think as an end-user of secrets, I expect to:

1. at minimum have a .
2. ideally, co-locate related secret keys into a single secret, but allow sub-directory mounting

I also think its a matter of luck that I did not yet hit file permission issues, but that is a separate issue.

Strong +1 to changing this.

[pmorie]

I am 100% in favor of allowing subdirectories. Since we have an immediate need for leading dots, I think we should get #8072 in for now and make a follow-up issue for subdirectories.

[derekwaynecarr]

SSH keys is use case of something that will not work if put in secret without file permissions. per key, recording it while its on my mind.

[thockin]

If we think that the primary mode for secrets is going to be through the
volume, I am OK with this. Loosen validation, allow subdirs, add mode.
Make sure validation ensures that the filename can not escape - no ".." and
no leading /

On Fri, May 15, 2015 at 12:45 PM, Derek Carr notifications@github.com
wrote:

SSH keys is use case of something that will not work if put in secret
without file permissions. per key, recording it while its on my mind.

—
Reply to this email directly or view it on GitHub
#8236 (comment)
.

[pmorie]

@deads2k ^^ does your current PR fit the bill here?

[deads2k]

#8072 is not as loose as we've described here and it doesn't rework the content to allow for modes. It could be considered a step along the road.

[pmorie]

Let's get #8072 in as an incremental step towards this. Now, the scope of this issue should be:

1. Add ability to specify subdirs in secrets keys
2. Add validations attendant to the above like checks for .., ^/, etc
3. Add ability to control file mode

Do we really want the file mode on the secret itself? There's already an existing PR for file mode: #7908; if we really want this on the secret we should close it out.

[soltysh]

Do we really want the file mode on the secret itself? There's already an existing PR for file mode: #7908; if we really want this on the secret we should close it out.

I'm positive it's needed, an example top of my head would be ssh keys, which specifically needs to be 0400. Even if we do 0400 to be the default like it was stated in #4789, I'm sure there'll be requests to loosen that requirement, so having that mechanism in place is reasonable imho.

[bgrant0607]

Ref #8190

[pmorie]

I think we should close this -- it's a dupe of #4789