Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using annotations for new API features creates the potential for security lapses #30819

Closed
smarterclayton opened this issue Aug 17, 2016 · 90 comments
Closed

Comments

@smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Aug 17, 2016

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:

  1. They are unstructured and can be preserved in etcd even after the feature is graduated from alpha.
  2. They can contain arbitrarily complex values (like init containers) or simple fields (like hostname for pods)

Annotations come with a set of issues:

  1. 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

    1. No way to communicate lifecycle via kubectl explain to end users
  2. 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.

    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
    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
  3. 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.

  4. 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).

  5. 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.

    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.

For these reasons, I believe we should not be using annotations for alpha level fields internally, and probably not externally.

Alternative

Instead, 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.

Alpha 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.

E.g.:

Kube 1.2:

pod:
  spec:
    alphaInitContainers:  # silently dropped by the storage layer
    - name: init-1

Kube 1.3:

pod:
  spec:
    alphaInitContainers: # visible in swagger
    - name: init-1
       ...

$ kubectl explain pod.spec.alphaInitContainers
This field allows you to provide specify "init containers" - run before other containers.
...
Feature is alpha and may change without notice.

Kube 1.4:

pod:
  spec:
    alphaInitContainers: # ignored, but visible in swagger.  kubectl validate could flag this as unsupported
    - name: init-1
    initContainers:
    - name: init-1

$ kubectl explain pod.spec.alphaInitContainers
This field has been removed - use pod.spec.initContainers

Kube 1.5

pod:
  spec:
    alphaInitContainers: # fails kubectl validate - alpha is no longer supported
                         # silently dropped from storage whenever a pod is updated
    initContainers:
    - name: init-1

$ kubectl explain pod.spec.alphaInitContainers
This field has been removed - use pod.spec.initContainers
@smarterclayton
Copy link
Contributor Author

@smarterclayton smarterclayton commented Aug 17, 2016

@kubernetes/sig-api-machinery @bgrant0607

@lavalamp
Copy link
Member

@lavalamp lavalamp commented Aug 17, 2016

@erictune

On Wed, Aug 17, 2016 at 2:26 PM, Clayton Coleman notifications@github.com
wrote:

@kubernetes/sig-api-machinery
https://github.com/orgs/kubernetes/teams/sig-api-machinery @bgrant0607
https://github.com/bgrant0607


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#30819 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglsjL_pR8c7dXwF6bTYrRc_UxfTp0ks5qg3yjgaJpZM4Jm55v
.

@timstclair
Copy link

@timstclair timstclair commented Aug 17, 2016

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.

Alternative solution: Reserve *.{alpha,beta}.kubernetes.io/* for internal use, and treat any annotations matching that pattern specially. We could either reject them in the case that the system does not know about them, or silently drop them (as we do with unknown fields). Of course this doesn't address your other points.

@smarterclayton
Copy link
Contributor Author

@smarterclayton smarterclayton commented Aug 17, 2016

We probably want something that explicitly disables certain API features at admission and on egress (Tim raised that on the service external name issue) based on the state of flags. Annotations are marginally easier to deal with in bulk via admission / generic code.

A weakness - a typo in a client library would bypass the whitelist filter (controller reads "alpha.kubernetes.ios/*" rather than ".io"). Smaller gap and would likely be caught in review, but isn't fail safe.

@smarterclayton
Copy link
Contributor Author

@smarterclayton smarterclayton commented Aug 17, 2016

Re: admission - that can also potentially address #4, but we don't support nested admission chains today and would also be vulnerable to new code coming in that the admission chain doesn't check.

@erictune
Copy link
Member

@erictune erictune commented Aug 17, 2016

Let's prefix alpha field names with the actual letter alpha/α/α and beta ones with beta/γ/β. That way you know you are doing something odd when you use it.

@thockin
Copy link
Member

@thockin thockin commented Aug 17, 2016

I would be OK with this approach. I feel like we considered it in the past and rejected it, but I have lost state, so...

I do feel like ad-hoc is just not working.

@smarterclayton
Copy link
Contributor Author

@smarterclayton smarterclayton commented Aug 17, 2016

I've lost state too - I know the "preserve data after the new field goes
live" was one, but Brian almost certainly remembers

On Wed, Aug 17, 2016 at 7:29 PM, Tim Hockin notifications@github.com
wrote:

I would be OK with this approach. I feel like we considered it in the past
and rejected it, but I have lost state, so...

I do feel like ad-hoc is just not working.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#30819 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_pzLsgP6tOMe8eke2XdCpJi1BOdmbks5qg5lOgaJpZM4Jm55v
.

@dchen1107
Copy link
Member

@dchen1107 dchen1107 commented Aug 18, 2016

I don't remember all the states neither since we quickly rejected the idea.

One of the reasons is that we worried too many duplicate fields in one API object. But we are doing this today if the feature was introduced as non-alpha initially, and need to change later; the only difference is introducing the field with a new name without prefix. I think that is even less manageable.

Another reason is back then, we thought we can skip both conversion and validation logic at master side and simply pass it through to other components, like Kubelet to take care the rest. For example, kubernetes.io/ingress-bandwidth, the validation is done at Kubelet. But in reality, we do both conversion and validation for those alpha feature today.

I believe there must be some other deep reasons, but above are all I can remember.

@thockin
Copy link
Member

@thockin thockin commented Aug 18, 2016

I propose we leave this until post code-freeze. We're not changing tracks
right now. Let's circle up next week or the week after and see if we can
get consensus on how to proceed. Fair?

On Wed, Aug 17, 2016 at 6:07 PM, Dawn Chen notifications@github.com wrote:

I don't remember all the states neither since we quickly rejected the
idea.

One of the reasons is that we worried too many duplicate fields in one API
object. But we are doing this today if the feature was introduced as
non-alpha initially, and need to change later; the only difference is
introducing the field with a new name without prefix. I think that is even
less manageable.

Another reason is back then, we thought we can skip both conversion and
validation logic at master side and simply pass it through to other
components, like Kubelet to take care the rest. For example,
kubernetes.io/ingress-bandwidth, the validation is done at Kubelet. But
in reality, we do both conversion and validation for those alpha feature
today.

I believe there must be some other deep reasons, but above are all I can
remember.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#30819 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVGqXclaCId5yQKcGmFi4ONvSe1tqks5qg7BFgaJpZM4Jm55v
.

@dchen1107
Copy link
Member

@dchen1107 dchen1107 commented Aug 18, 2016

Agreed with @thockin at #30819 (comment)

I will continue processing AppArmor changes as is.

@smarterclayton smarterclayton added this to the v1.5 milestone Aug 18, 2016
@smarterclayton
Copy link
Contributor Author

@smarterclayton smarterclayton commented Aug 18, 2016

Yes this is marked as a 1.5 thing now.

@erictune
Copy link
Member

@erictune erictune commented Aug 18, 2016

More specific example of OP's item 4:

If you wanted to give kubelet only the privileges it needs, you might want to give it permission to update Status, but only to read Spec. Init-container status is stored in annotations. So you cannot do that.

@lavalamp
Copy link
Member

@lavalamp lavalamp commented Aug 18, 2016

I thought you could write to annotations by posting to the status endpoint?

On Thu, Aug 18, 2016 at 9:12 AM, Eric Tune notifications@github.com wrote:

More specific example of OP's item 4:

If you wanted to give kubelet only the privileges it needs, you might want
to give it permission to update Status, but only to read Spec.
Init-container status is stored in annotations. So you cannot do that.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#30819 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglhp4RUoufXtJmH2ekNOcrg2vAz7xks5qhIR5gaJpZM4Jm55v
.

@bgrant0607 bgrant0607 added the team/api label Aug 22, 2016
@bgrant0607
Copy link
Member

@bgrant0607 bgrant0607 commented Aug 22, 2016

@lavalamp @erictune Yes, annotations can be written via the status subresource endpoint.

@bgrant0607
Copy link
Member

@bgrant0607 bgrant0607 commented Aug 22, 2016

@smarterclayton @thockin @lavalamp @erictune @dchen1107 @timstclair @davidopp

Several of the reasons why we started to use annotations in this way are mentioned in this issue, and a number of them have both advantages and disadvantages:

  • Don't appear in Swagger or documentation. We could develop another mechanism (see #4855) to achieve this for features we're not ready to advertise widely, such as incomplete features.
  • Consistent with how we planned to represent fields that we only wanted to appear in new API versions (e.g., v2alpha1) but which had to be persisted using older API versions.
  • Annotations can be conveyed to other components without support compiled into apiserver, whereas unknown fields are dropped during decoding. We'll continue to need something like this for extensions and pluggable components, such as the scheduler.
  • Can be preserved in etcd when support is removed, such as when the features graduate, but also in other scenarios, such as when rolling back to an earlier release. Enables post-upgrade/downgrade actions, but probably also requires them in order to avoid landmines.

Multiple fields representing the same information are very tricky to deal with regardless whether we use fields or annotations.

I'll throw out another idea, which is that we could adopt semantic versioning for API versions, and rev a minor API version (e.g., v1.3) when adding a field. That would enable us to avoid the multiple-field problem.

Anyway, I'm ok with using fields rather than annotations.

In practice, the level of forward compatibility we ensure is probably determined by the number of users of a feature more than mechanism (annotation vs field) or even policy (alpha vs GA).

@thockin
Copy link
Member

@thockin thockin commented Aug 22, 2016

perhaps we should elect a small (2-3) number of these to trial in the 1.5
cycle?

On Mon, Aug 22, 2016 at 10:02 AM, Brian Grant notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton @thockin
https://github.com/thockin @lavalamp https://github.com/lavalamp
@erictune https://github.com/erictune @dchen1107
https://github.com/dchen1107 @timstclair https://github.com/timstclair
@davidopp https://github.com/davidopp

Several of the reasons why we started to use annotations in this way are
mentioned in this issue, and a number of them have both advantages and
disadvantages:

  • Don't appear in Swagger or documentation. We could develop another
    mechanism (see #4855
    #4855) to achieve
    this for features we're not ready to advertise widely, such as incomplete
    features.
  • Consistent with how we planned to represent fields that we only
    wanted to appear in new API versions (e.g., v2alpha1) but which had to be
    persisted using older API versions.
  • Annotations can be conveyed to other components without support
    compiled into apiserver, whereas unknown fields are dropped during
    decoding. We'll continue to need something like this for extensions and
    pluggable components, such as the scheduler.
  • Can be preserved in etcd when support is removed, such as when the
    features graduate, but also in other scenarios, such as when rolling back
    to an earlier release. Enables post-upgrade/downgrade actions, but probably
    also requires them in order to avoid landmines.

Multiple fields representing the same information are very tricky to deal
with regardless whether we use fields or annotations.

I'll throw out another idea, which is that we could adopt semantic
versioning for API versions, and rev a minor API version (e.g., v1.3) when
adding a field. That would enable us to avoid the multiple-field problem.

Anyway, I'm ok with using fields rather than annotations.

In practice, the level of forward compatibility we ensure is probably
determined by the number of users of a feature more than mechanism
(annotation vs field) or even policy (alpha vs GA).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30819 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVIm8hmxi2Pd9HcrbXCkfRy60x5O_ks5qidYqgaJpZM4Jm55v
.

@smarterclayton
Copy link
Contributor Author

@smarterclayton smarterclayton commented Aug 22, 2016

@smarterclayton
Copy link
Contributor Author

@smarterclayton smarterclayton commented Aug 22, 2016

Per object schemas has been suggested in the past, but one advantage
of native fields is you can test swagger for the presence of the field
(are init containers supported) from a client library and from kubectl
validate. A field named "alpha" is clearly communicated - we could
try the fields without the implied per object schema and look at what
might be needed.

Hypothetical flow (no preservation)

  1. PR adds alpha field. Struct name is "Feature". JSON name is
    "alphaFeature". Swagger says "Alpha: may change without notice."
  2. Feature moves to beta. Rename field either "betaFeature" (API may
    change) or "feature" (we are settled on API). Update swagger
    description. Protobuf tag only changes if field is semantically
    different.
  3. Feature moves to GA - rename field to "feature", update swagger.

Hypothetical preserve old data:

2a: Beta: Adds new field to struct AlphaFeature with JSON name
"alphaFeature" (and internal version). Field Feature has JSON name
"betaFeature" or "feature". Swagger updated.
3a. GA: Drop field AlphaFeature, preserve "betaFeature" the same way as Alpha.

All internal data is ignored (no internal code ever reads from
AlphaFeature / BetaFeature field). This reduces internal code change
and makes review easier to reason about (alpha/beta is orthogonal to
the feature actually working).

Can generate a description of API changes by structural diffs to the
swagger doc. All swagger doc describes what level of support a
feature has.

@ae6rt
Copy link
Contributor

@ae6rt ae6rt commented Sep 2, 2016

As a user, I would not feel put upon if new features had a struct name with an alpha-like prefix. Or without the prefix but activated in the minor version of a SemVer version. Injecting the new field value in an annotation, such as for the current init-container case, feels a little strange. I was surprised when I first saw this approach - but I'm not complaining - and figured there must have been a reason for it.

@smarterclayton
Copy link
Contributor Author

@smarterclayton smarterclayton commented Sep 27, 2016

Ok, so proposal for 1.5. We switch to adding new fields for annotations on at least 1 of our alpha and one of our beta fields.

  1. Adding a new alpha field:
    1. Create the field with the "proper" name in the Go struct - i.e. InitContainers
    2. Give the field a JSON serialization name prefixed with alpha, i.e. alphaInitContainers
    3. Add a line to the description of the field (for swagger) This field is marked alpha and may change in future releases (todo: determine whether we want a structured annotation so we can generate a list of alpha fields)
    4. Add a new internal field with the proper name InitContainers (the fact that we still have JSON on internal fields needs to be fixed).
  2. Promoting a field to beta (field is unlikely to be changed)
    1. Add a new field to the Go struct with the alpha name - AlphaInitContainers
    2. Mark the alpha field as "no conversion"
    3. Give the field JSON serialization alphaInitContainers
    4. Change the beta field JSON serialization to initContainers
    5. Update the Godoc for the alpha field This field is no longer read and will be preserved until the field ... becomes part of the official API
    6. Update the Godoc for the beta field This field is marked beta and may change before release
  3. Promoting a field to GA
    1. Remove the alpha field
    2. Remove the comment on the beta field
@smarterclayton
Copy link
Contributor Author

@smarterclayton smarterclayton commented Oct 2, 2016

2.7. optionally add a conversion from annotations

@thockin
Copy link
Member

@thockin thockin commented Oct 3, 2016

Why not JSON-serialize beta fields as betaInitContainers ?

We need a story for simultaneously supporting alpha and beta, for
transition. I feel pretty strongly that we can and should o that most of
the time.

On Sun, Oct 2, 2016 at 10:27 AM, Clayton Coleman notifications@github.com
wrote:

2.7. optionally add a conversion from annotations


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30819 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVC5nVXa6e5N1ALVXDhSOgS1nwJy_ks5qv-llgaJpZM4Jm55v
.

@smarterclayton
Copy link
Contributor Author

@smarterclayton smarterclayton commented Oct 3, 2016

The point of beta would be that we expect the field to be "correct". The
least impactful option is to put the field in its expected form. Agree
it's less obvious to all consumers, but it mitigates transitions much more
effectively.

My concern is that the gyrations we're doing around annotations and forcing
changes is increasing the complexity of the alpha - beta - ready transition
for ourselves and for end users. I'd prefer to focus on the three stages
as reflecting actual stages of lifecycle rather than today where we are
forcing ourselves into painful changes. After all, if beta changes are
supposed to be unlikely, why wouldn't we target minimal disruption for
users with choice of external name? If during beta we decide something has
to be changed, we either do it breaking (we screwed up) or its minor. The
former should be unlikely.

Why are we simulataneously supporting alpha and beta? Isn't the point to
not support alpha once it goes to beta? Is the cost we are paying worth
it?

On Oct 2, 2016, at 11:57 PM, Tim Hockin notifications@github.com wrote:

Why not JSON-serialize beta fields as betaInitContainers ?

We need a story for simultaneously supporting alpha and beta, for
transition. I feel pretty strongly that we can and should o that most of
the time.

On Sun, Oct 2, 2016 at 10:27 AM, Clayton Coleman notifications@github.com
wrote:

2.7. optionally add a conversion from annotations


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<
#30819 (comment)
,
or mute the thread
<
https://github.com/notifications/unsubscribe-auth/AFVgVC5nVXa6e5N1ALVXDhSOgS1nwJy_ks5qv-llgaJpZM4Jm55v

.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30819 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p-coQSHFbguiyl2RAJNcDpxqmE51ks5qwH0-gaJpZM4Jm55v
.

@gmarek
Copy link
Member

@gmarek gmarek commented Oct 3, 2016

TL;DR; - please prioritize this discussion, as we don't have much development time left for this release.

I'll be moving multiple schedulers to beta, so I'd like to have some clear decision sooner rather than later, as it seems that it's going to be rather long process...

Also it'd be good to have some common decision on whether we want to have one minor release of having both alpha and beta (also beta and GA), or when graduating a feature we want to stop supporting "lower" version straight away. I guess supporting two versions for one minor release would be good for users, but it'll cause some development pain.

k8s-github-robot pushed a commit that referenced this issue May 15, 2017
Automatic merge from submit-queue (batch tested with PRs 45826, 45747, 45548, 45606, 41766)

CRI: using typed filed for sysctls

**What this PR does / why we need it**:

CRI supports sysctls via annotations [today](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/api/v1alpha1/runtime/api.proto#L237), we should move them to typed and structured fields instead. (refer [here](#30819 (comment)))

**Which issue this PR fixes** 

Part of #39130.

**Release note**:

```release-note
NONE
```
@lavalamp lavalamp modified the milestones: v1.8, v1.7 May 26, 2017
@liggitt
Copy link
Member

@liggitt liggitt commented Aug 1, 2017

I've tried to summarize the discussions with api-machinery to update guidance for alpha fields at kubernetes/community#869

berryjam pushed a commit to berryjam/kubernetes that referenced this issue Aug 18, 2017
Add language to make it explicit that annotations are not to be altered
by runtimes, and should only be used for features that are opaque to the
Kubernetes APIs. Unfortunately there are currently exceptions
introduced in [1][1], but this change makes it clear that they are to be
changed and that no more such semantic-affecting annotations should be
introduced.

In the spirit of the discussion and conclusion in [2][2].

Also captures the link between the annotations returned by various
status queries and those supplied in associated configs.

[1]: kubernetes#34819
[2]: kubernetes#30819 (comment)
k8s-github-robot pushed a commit to kubernetes/community that referenced this issue Aug 23, 2017
@liggitt liggitt added the kind/cleanup label Sep 1, 2017
@k8s-github-robot
Copy link
Contributor

@k8s-github-robot k8s-github-robot commented Sep 7, 2017

[MILESTONENOTIFIER] Milestone Labels Complete

@smarterclayton

Issue label settings:

  • sig/api-machinery: Issue will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the issue owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/cleanup: Adding tests, refactoring, fixing old bugs.
Additional instructions available here The commands available for adding these labels are documented here
@spiffxp
Copy link
Member

@spiffxp spiffxp commented Sep 18, 2017

Should this issue still be in the v1.8 milestone? Seems like this would benefit from a definition of done before putting into a release milestone

@mbohlool
Copy link
Member

@mbohlool mbohlool commented Sep 19, 2017

@erictune, @smarterclayton, @gmarek this is a daily and friendly ping from release team. Is this still relevant to 1.8? If yes, who is actively working on it?

@lavalamp
Copy link
Member

@lavalamp lavalamp commented Sep 19, 2017

This doesn't seem actionable, and it seems to have significant overlap with #34508.

@lavalamp lavalamp removed this from the v1.8 milestone Sep 19, 2017
@liggitt
Copy link
Member

@liggitt liggitt commented Sep 19, 2017

I think the updated guidance to stop using annotations for alpha features stops the bleeding, and the field gate proposal captures concrete improvements that need to happen to make working with alpha fields sustainable

@liggitt liggitt closed this Sep 19, 2017
calebamiles pushed a commit to kubernetes/cloud-provider-gcp that referenced this issue Mar 21, 2018
Automatic merge from submit-queue

Loadbalanced client src ip preservation enters beta

Sounds like we're going to try out the proposal (kubernetes/kubernetes#30819 (comment)) for annotations -> fields on just one feature in 1.5 (scheduler). Or do we want to just convert to fields right now?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.