{"action":"created","issue":{"url":"https://api.github.com/repos/kubernetes/kubernetes/issues/30819","repository_url":"https://api.github.com/repos/kubernetes/kubernetes","labels_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/30819/labels{/name}","comments_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/30819/comments","events_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/30819/events","html_url":"https://github.com/kubernetes/kubernetes/issues/30819","id":171764482,"number":30819,"title":"Using annotations for new API features creates the potential for security lapses","user":{"login":"smarterclayton","id":1163175,"avatar_url":"https://avatars.githubusercontent.com/u/1163175?v=3","gravatar_id":"","url":"https://api.github.com/users/smarterclayton","html_url":"https://github.com/smarterclayton","followers_url":"https://api.github.com/users/smarterclayton/followers","following_url":"https://api.github.com/users/smarterclayton/following{/other_user}","gists_url":"https://api.github.com/users/smarterclayton/gists{/gist_id}","starred_url":"https://api.github.com/users/smarterclayton/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/smarterclayton/subscriptions","organizations_url":"https://api.github.com/users/smarterclayton/orgs","repos_url":"https://api.github.com/users/smarterclayton/repos","events_url":"https://api.github.com/users/smarterclayton/events{/privacy}","received_events_url":"https://api.github.com/users/smarterclayton/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/kubernetes/kubernetes/labels/priority/P1","name":"priority/P1","color":"eb6420"},{"url":"https://api.github.com/repos/kubernetes/kubernetes/labels/team/api","name":"team/api","color":"d2b48c"},{"url":"https://api.github.com/repos/kubernetes/kubernetes/labels/team/CSI-API%20Machinery%20SIG","name":"team/CSI-API Machinery SIG","color":"d2b48c"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":{"url":"https://api.github.com/repos/kubernetes/kubernetes/milestones/23","html_url":"https://github.com/kubernetes/kubernetes/milestones/v1.5","labels_url":"https://api.github.com/repos/kubernetes/kubernetes/milestones/23/labels","id":1945978,"number":23,"title":"v1.5","description":"","creator":{"login":"lavalamp","id":647318,"avatar_url":"https://avatars.githubusercontent.com/u/647318?v=3","gravatar_id":"","url":"https://api.github.com/users/lavalamp","html_url":"https://github.com/lavalamp","followers_url":"https://api.github.com/users/lavalamp/followers","following_url":"https://api.github.com/users/lavalamp/following{/other_user}","gists_url":"https://api.github.com/users/lavalamp/gists{/gist_id}","starred_url":"https://api.github.com/users/lavalamp/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lavalamp/subscriptions","organizations_url":"https://api.github.com/users/lavalamp/orgs","repos_url":"https://api.github.com/users/lavalamp/repos","events_url":"https://api.github.com/users/lavalamp/events{/privacy}","received_events_url":"https://api.github.com/users/lavalamp/received_events","type":"User","site_admin":false},"open_issues":213,"closed_issues":145,"state":"open","created_at":"2016-08-16T21:47:28Z","updated_at":"2016-10-12T13:54:25Z","due_on":"2016-12-09T00:00:00Z","closed_at":null},"comments":76,"created_at":"2016-08-17T21:26:29Z","updated_at":"2016-10-12T18:06:27Z","closed_at":null,"body":"Our current API strategy is to add annotations for new features.  These annotations are expected to overlay on top of the existing API fields, and alter system behavior in other components.  Our primary reason for annotations today:\r\n\r\n1. They are unstructured and can be preserved in etcd even after the feature is graduated from alpha.\r\n2. They can contain arbitrarily complex values (like init containers) or simple fields (like hostname for pods)\r\n\r\nAnnotations come with a set of issues:\r\n\r\n1. Alpha annotations are not discoverable and do not show up in our docs - users don't know how to use the features, and we get no automatic benefits from swagger or other tools\r\n  1. No way to communicate lifecycle via `kubectl explain` to end users\r\n2. Developers who implement those annotations must merge the value in to the internal or external types at various points in the lifecycle - conversion, validation, in client libraries, and in nested utilities.\r\n  1. Client utilities that receive a partial struct (like `api.PodSpec`) must also receive a list of annotations that alter that pod spec (for things like init containers) and potentially merge those\r\n  2. When an alpha feature is graduated, we typically change the implementation of that feature significantly as well (moving from annotation to field) which causes extra churn in the implementation, and the potential for new bugs\r\n3. Some alpha features may be missed by a developer unfamiliar with the annotation - without any of the normal tools to introspect code (code completion, reflective tests on structs) it is more likely that the impact of an annotation is missed when adding new code.\r\n4. The security implications of an annotation are harder to convey than a field, and code that allows annotation mutation but *not* field annotation can be exposed to malicious authors accidentally (bind, status, and a few other sub resources that protect spec can be abused to mutate init containers, for example).\r\n5. A malicious user can introduce an annotation for an upcoming alpha feature on a cluster before the feature is introduced, which means that prior to enabling an alpha feature a cluster admin has to sweep all possible objects to ensure no dangerous annotations are set.\r\n\r\n    This occurred for init containers - podsecuritypolicy is applied at creation time, but because init containers are represented as annotations a malicious user can set an init container annotation before Kube 1.3 is rolled out, and as soon as the cluster is upgraded that container would be executed.  This means that admins have to run a pre-upgrade migration to purge any dangerous / out of policy annotations.\r\n\r\nFor these reasons, I believe we should not be using annotations for alpha level fields internally, and probably not externally.\r\n\r\n## Alternative\r\n\r\nInstead, I believe we should represent alpha level fields in our Go structs and clearly denote them as alpha (perhaps with an named prefix).  Once the feature is out of alpha, we would leave the alpha level field in the external struct for at least 1 major version to allow upgraders to use that field.  We would then drop the alpha field, which would clean the database of that value.  Internal code would simply rename the field, and ignore the external field.\r\n\r\nAlpha level fields would be denoted by struct tags that allowed them to show up in swagger.  Once we dropped support for the field, swagger would no longer show them.  We would have to have a \"drop by default\" rule in place for swagger validations on fields that are no longer recognized for old alpha fields that aren't supported, but that's what we want for alpha.\r\n\r\nE.g.:\r\n\r\nKube 1.2:\r\n\r\n```\r\npod:\r\n  spec:\r\n    alphaInitContainers:  # silently dropped by the storage layer\r\n    - name: init-1\r\n```\r\nKube 1.3:\r\n```\r\npod:\r\n  spec:\r\n    alphaInitContainers: # visible in swagger\r\n    - name: init-1\r\n       ...\r\n\r\n$ kubectl explain pod.spec.alphaInitContainers\r\nThis field allows you to provide specify \"init containers\" - run before other containers.\r\n...\r\nFeature is alpha and may change without notice.\r\n```\r\nKube 1.4:\r\n```\r\npod:\r\n  spec:\r\n    alphaInitContainers: # ignored, but visible in swagger.  kubectl validate could flag this as unsupported\r\n    - name: init-1\r\n    initContainers:\r\n    - name: init-1\r\n\r\n$ kubectl explain pod.spec.alphaInitContainers\r\nThis field has been removed - use pod.spec.initContainers\r\n```\r\nKube 1.5\r\n```\r\npod:\r\n  spec:\r\n    alphaInitContainers: # fails kubectl validate - alpha is no longer supported\r\n                         # silently dropped from storage whenever a pod is updated\r\n    initContainers:\r\n    - name: init-1\r\n\r\n$ kubectl explain pod.spec.alphaInitContainers\r\nThis field has been removed - use pod.spec.initContainers\r\n```\r\n"},"comment":{"url":"https://api.github.com/repos/kubernetes/kubernetes/issues/comments/253291210","html_url":"https://github.com/kubernetes/kubernetes/issues/30819#issuecomment-253291210","issue_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/30819","id":253291210,"user":{"login":"bgrant0607","id":7725777,"avatar_url":"https://avatars.githubusercontent.com/u/7725777?v=3","gravatar_id":"","url":"https://api.github.com/users/bgrant0607","html_url":"https://github.com/bgrant0607","followers_url":"https://api.github.com/users/bgrant0607/followers","following_url":"https://api.github.com/users/bgrant0607/following{/other_user}","gists_url":"https://api.github.com/users/bgrant0607/gists{/gist_id}","starred_url":"https://api.github.com/users/bgrant0607/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bgrant0607/subscriptions","organizations_url":"https://api.github.com/users/bgrant0607/orgs","repos_url":"https://api.github.com/users/bgrant0607/repos","events_url":"https://api.github.com/users/bgrant0607/events{/privacy}","received_events_url":"https://api.github.com/users/bgrant0607/received_events","type":"User","site_admin":false},"created_at":"2016-10-12T18:06:27Z","updated_at":"2016-10-12T18:06:27Z","body":"@erictune I'm fine with that."}}
{"actor":{"display_login":"bgrant0607"}}