Skip to content

add guidance about avoiding cross namespace references from namespaced resource #5455

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

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Feb 1, 2021

This is response to a question on slack. Having cross-namespace references in an API is a risky thing to do and this clarifies why.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 1, 2021
@k8s-ci-robot k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Feb 1, 2021
@@ -876,6 +876,20 @@ kind `Foo` by just the name (within the current namespace, if a namespaced
resource), or should be called `fooRef`, and should contain a subset of the
fields of the `ObjectReference` type.

Object references on a namespaced type should usually refer only to objects in
the same namespace. Because namespaces are a security boundary, cross namespace
references can have unexpected impacts, including:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does 'usually' in this case imply that there may be some cases in which it's okay for a reference to span namespaces? Or is this intended to account for cases such as a reference from a namespaced resource to a cluster scoped resource (e.g. a Pod's nodeName)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does 'usually' in this case imply that there may be some cases in which it's okay for a reference to span namespaces? Or is this intended to account for cases such as a reference from a namespaced resource to a cluster scoped resource (e.g. a Pod's nodeName)?

It is to account for cases like a double-opt-in between namespaces. I'm not prepared to say that I know so much that this is a never-ever, but the bar is fairly high.

able to express "give me that one over there" is dangerous across namespaces without addition work for permission checks
or opt-in's from both involved namespaces.
3. referential integrity problems that one party cannot solve. Referencing namespace/B from namespace/A doesn't imply the
power to control the other namespace. This means that you can refer to a thing you cannot create or update.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Points two and three are interesting because on one hand they intuitively make sense to me, but on the other hand they seem like they could occur within a namespace too. If you consider the below reference...

apiVersion: example.org/v1
kind: CoolSource
metadata:
  namespace: a
  name: verycool
spec:
  targetRef:
    apiVersion: example.org/v1
    kind: CoolTarget
    name: evencoolertarget

There's no guarantee that the author of the CoolSource has RBAC access to view or otherwise interact with a CoolTarget within the same namespace. That said, it does seem less likely (but obviously not impossible) that the author would have any kind of RBAC access to another namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no guarantee that the author of the CoolSource has RBAC access to view or otherwise interact with a CoolTarget within the same namespace. That said, it does seem less likely (but obviously not impossible) that the author would have any kind of RBAC access to another namespace.

There is no guarantee, but it is highly likely that inside of a single namespace the resources are at least viewable and likely edit-able.

1. leaking information about one namespace into another namespace. It's natural to place status messages or even bits of
content about the referenced object in the original. This is a problem across namespaces.
2. potential invasions into other namespaces. Often references give access to a piece of referred information, so being
able to express "give me that one over there" is dangerous across namespaces without addition work for permission checks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
able to express "give me that one over there" is dangerous across namespaces without addition work for permission checks
able to express "give me that one over there" is dangerous across namespaces without additional work for permission checks

Comment on lines +879 to +880
Object references on a namespaced type should usually refer only to objects in
the same namespace. Because namespaces are a security boundary, cross namespace
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to be squishy with a "usually", should we be clearer in saying built-in types and ownerReferences don't support cross-namespace references, and if a non-built-in type chooses to, it should do double opt-in or permission checks to prevent these problems?

Is it worth explicitly saying here that object references on built-in kubernetes types and in ownerReferences do not support cross namespace references

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to be squishy with a "usually", should we be clearer in saying built-in types and ownerReferences don't support cross-namespace references, and if a non-built-in type chooses to, it should do double opt-in or permission checks to prevent these problems?

Is it worth explicitly saying here that object references on built-in kubernetes types and in ownerReferences do not support cross namespace references

updated

@liggitt
Copy link
Member

liggitt commented Feb 22, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1847825 into kubernetes:master Feb 22, 2021
matthchr added a commit to matthchr/azure-service-operator that referenced this pull request Nov 23, 2021
As per kubernetes/community#5455, it is not
a reccomended pattern.
matthchr added a commit to matthchr/azure-service-operator that referenced this pull request Nov 23, 2021
As per kubernetes/community#5455, it is not
a recommended pattern.
matthchr added a commit to matthchr/azure-service-operator that referenced this pull request Nov 23, 2021
As per kubernetes/community#5455, it is not
a recommended pattern.
matthchr added a commit to matthchr/azure-service-operator that referenced this pull request Nov 23, 2021
As per kubernetes/community#5455, it is not
a recommended pattern.
matthchr added a commit to matthchr/azure-service-operator that referenced this pull request Nov 24, 2021
As per kubernetes/community#5455, it is not
a recommended pattern.
matthchr added a commit to Azure/azure-service-operator that referenced this pull request Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/developer-guide Issues or PRs related to the developer guide cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants