Proposal: Add 'sensitive' designation to a bundle parameter to the spec

[vdice]

As bundle parameters consist of values intended for the deployed application, some of which may be sensitive in nature (secrets, passwords, etc.), I'm wondering if it would be prudent to add an additional sensitive attribute that can optionally be added to a given parameter definition, of type boolean? (Default would be sensitive: false)

[simonferquel]

Should'nt sensitive information be passed as credentials instead of parameters ? (e.g. credentials are already identified as sensitive, and tools like Duffle do not store them in claims)

[jeremyrickard]

@simonferquel we had that thought too, but that's not what the specification indicates.

If we lookat the Credentials section, the following text appears:

What about parameters such as database passwords used by the application? Properly speaking, these are parameters if the application uses them.

Note that the CNAB specification does not mandate specific security implementations on storage of either parameters or credentials. CNAB Runtimes ought to consider that both parameters and credentials may contain secret data, and OUGHT to secure both parameter and credential data in ways appropriate to the underlying platform. It is recommended that credentials are not persisted between actions, and that parameters are persisted between actions.

Credentials, it seems, are intended to represent the identity of the person executing the bundle, while Parameters are intended for anything configuration related. I think that it's likely to be confusing to end users as is, so at a minimum maybe we need to add more explicit guidance/best practices. I think that if we intend for that conceptual separation, however, we really need a way for bundle authors to indicate that a parameter should be treated as a sensitive value. We're doing that in Porter at the moment, but it's not spec unfortunately....

[carolynvs]

I had the same confusion about credentials being the best place to put sensitive data and I bet pretty much everyone will do the same when they first use a bundle. It's really confusing.

I'd like us to either revisit that if we can? Or if that isn't possible anymore and we have to live with that decision, then we really do need a sensitive flag on parameters since they are going to be a mixed bag of boring things like "verbose=10" and values that really need to be treated differently like "mydbpassword=carolynlovesponies".

[ijc]

Given:

CNAB Runtimes ought to consider that ... parameters ... may contain secret data, and OUGHT to secure ... parameter ... data in ways appropriate to the underlying platform.

Is the spec not saying that all parameters should be treated by the runtime as if they had sensitive: true set on them? If so what additional information does sensitive: true give us?

Are there platforms where the runtime really wants to treat sensitive: false parameters differently to sensitive: true ones? In general I would say if you have an implementation secure enough to satisfy sensitive: true it seems less complex to use that same mechanism (even if strictly needlessly) for insensitive params too, rather than inventing a second parallel mechanism. It also avoids people accidentally tagging things the wrong way by just treating everything as potentially sensitive.

[technosophos]

To add to what @ijc is suggesting, take Kubernetes for an example. I always wondered why they added ConfigMaps after having Secrets. Why not just treat ALL params as secret, period? If there is a usecase, then I'd be in favor of adding sensitive with default true.

[jeremyrickard]

I think sensitive: true being the default makes sense.

This issue popped up because we added this in Porter. We bake in the manifest and then use it at runtime. We just added the capability to mask sensitive values from outputs. We're masking credentials by default and we're figuring out how to handle parameters and and outputs (porter step outputs, no bundle outputs yet). I had made an error in my bundle and included an invalid (whitespace) character in one of parameter values. I was able to pretty quickly figure out the error because I had flagged that parameter as not sensitive:

porter install --insecure --param-file  params.ini -c azure-kube-cred
installing porter-azure-wordpress...
executing porter install configuration from /cnab/app/porter.yaml
Create Azure MySQL
Starting deployment operations...
Error: error deploying "mysql-azure-porter-demo-wordpress" in resource group "porter-test": error while waiting for deployment to complete: Code="DeploymentFailed" Message="At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-debug for usage details." Details=[{"code":"Conflict","message":"{\r\n  \"status\": \"Failed\",\r\n  \"error\": {\r\n    \"code\": \"ResourceDeploymentFailure\",\r\n    \"message\": \"The resource operation completed with terminal provisioning state 'Failed'.\",\r\n    \"details\": [\r\n      {\r\n        \"code\": \"InvalidUserName\",\r\n        \"message\": \"'azureuser ' is not a valid name because it contains invalid characters.\"\r\n      }\r\n    ]\r\n  }\r\n}"}]
err: error deploying "mysql-azure-porter-demo-wordpress" in resource group "porter-test": error while waiting for deployment to complete: Code="DeploymentFailed" Message="At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-debug for usage details." Details=[{"code":"Conflict","message":"{\r\n  \"status\": \"Failed\",\r\n  \"error\": {\r\n    \"code\": \"ResourceDeploymentFailure\",\r\n    \"message\": \"The resource operation completed with terminal provisioning state 'Failed'.\",\r\n    \"details\": [\r\n      {\r\n        \"code\": \"InvalidUserName\",\r\n        \"message\": \"'azureuser ' is not a valid name because it contains invalid characters.\"\r\n      }\r\n    ]\r\n  }\r\n}"}]
Error: mixin execution failed: exit status 1
Error: failed to install the bundle: container exit code: 1

In this case, it's because I had a space in the azureuser string and that was invalid. So the use case here was the ability to mask things that really are secrets and not other params that contain otherwise not secret values.

I'm fine leaving this out of the spec, we can handle it within the runtime ourselves.

[technosophos]

Oh... actually, that makes sense to me. I could get behind that if what we're talking about is masking output, not necessarily suggesting storage techniques

[jeremyrickard]

I think we should be more explicit about storage maybe.

 may contain secret data, and OUGHT to secure ... parameter ... data in ways appropriate to the underlying platform.

OUGHT to secure ... parameter should probably be more stern. MUST secure or something, so people have more assurances that stuff will be treated with the proper care.

[jeremyrickard]

Here is another run of the output with masking, just as an FYI for anyone:

$ porter install --insecure --param-file params.ini -c azure-kube-cred
installing porter-azure-wordpress...
executing porter install configuration from /cnab/app/porter.yaml
Create Azure MySQL
Starting deployment operations...
Finished deployment operations...
Helm Install Wordpress
/usr/local/bin/helm helm install --name porter-ci-wordpress stable/wordpress --replace --set externalDatabase.database=wordpress --set externalDatabase.host=******* --set externalDatabase.password=******* --set externalDatabase.port=3306 --set externalDatabase.user=azureuser --set mariadb.enabled=false --set readinessProbe.initialDelaySeconds=120
NAME:   porter-ci-wordpress
LAST DEPLOYED: Thu Apr 25 15:11:44 2019
NAMESPACE: default
STATUS: DEPLOYED

Note host and password are marked as secure things, so we wiped them from the logs.

[technosophos]

My reasoning for avoiding MUST is that for a local-only system, it may make sense to not do encryption on at-rest... plus, some systems like Kubernetes can't do encryption at-rest by default, and Secrets really aren't all that secure.

[ijc]

FWIW having sensitive be a display only thing (i.e. specified that it MUST have no impact on the actual storage/sharing mechanism used) makes sense to me as a usecase.

Perhaps that OUGHT ought (pun intended 😄) to be a more formal SHOULD (per rfc2119 meaning)?

[ryanmoran]

Perhaps I am misreading the JSONSchema spec or the requirements of the proposed sensitive field outlined above, but I think this would be covered by writeOnly. My understanding of what is outlined there is an annotation that indicates that a field may be set or modified, but not viewed. I think this is what we would be trying to achieve with the sensitive flag.

FWIW, we are also planning to implement this type of secret masking functionality in our own runtime. In a manner like what @ijc described, we are storing all of our parameter configuration in a secure store, sensitive or not. So, the only place we see an issue with plaintext parameters is if we want to display them to a user. We plan to enable some users with limited privileges to see some set of non-sensitive parameters, while allowing super-users access to the full set. I had thought that we would be able to do this with writeOnly once we landed JSONSchema support.

cc @youreddy

[jeremyrickard]

@ryanmoran I think that makes sense!

[jeremyrickard]

Although, it doesn't look like we added writeOnly to the top level attributes in the Markdown, only readOnly. Should that get PR'ed?

[ryanmoran]

Yes. This looks like it was something we missed when we first landed the JSONSchema support. I opened #174 which adds the writeOnly field.

[jeremyrickard]

Closing this per #174