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

release-21.1: server: add ui-only status endpoint for node info #71719

Merged
merged 2 commits into from Oct 21, 2021

Conversation

dhartunian
Copy link
Collaborator

@dhartunian dhartunian commented Oct 19, 2021

Backport 1/1 commits from #71181.

This PR also includes the bugfix from #71749.

/cc @cockroachdb/release


Previously, a change was made to the _status/nodes endpoint to secure
it behind an Admin role in order to limit the spread of sensitive
information in node payloads to non-Admins. This included data such as
crdb args, env variables, and addresses. This change created a
regression in the DB Console because the cluster overview page relies
heavily on information gleaned from the nodes endpoint. The page became
useless for non-Admins since it couldn't receive any response from the
endpoint.

This change creates a new gRPC endpoint and payload types to mirror
those found in the roachpb package for the purpose of continuing to
expose node information to the DB Console. These endpoints are under the
_status/nodes_ui prefix and convert internal roachpb payloads to the
external facing ones (currently the payload types match exactly, but now
they can change freely), adding in sensitive data only when the caller
has the Admin role.

The DB Console has been modified to only use the new nodes_ui endpoint
which will dynamically return more data when the logged-in user is an
Admin.

non-UI callers on the _status/nodes gRPC endpoint such as the debug
zip commands and others have not been modified.

Resolves #70702

Release note (ui change): Non-Admin users of the DB Console have
regained the ability to view the cluster overview page. Users without
the Admin role will still see most data about their nodes but
information such as command line arguments, environment variables, and
IP addresses and DNS names of nodes will be hidden.


Release Justification: This fixes a security-related regression introduced
earlier which restricted access to node status endpoints to Admins only.

@dhartunian dhartunian requested review from a team October 19, 2021 17:37
@dhartunian dhartunian requested a review from a team as a code owner October 19, 2021 17:37
@blathers-crl
Copy link

blathers-crl bot commented Oct 19, 2021

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

return nil, err
}

internalResp, _, err := s.nodesHelper(ctx, 0 /* limit */, 0 /* offset */)
Copy link
Collaborator

Choose a reason for hiding this comment

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

friendly reminder to include #71749

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@rafiss
Copy link
Collaborator

rafiss commented Oct 20, 2021

also just checking if there are still plans to backport to v20.2?

@dhartunian
Copy link
Collaborator Author

also just checking if there are still plans to backport to v20.2?

Yep, just put that up here: #71793

dhartunian and others added 2 commits October 21, 2021 10:57
Previously, a change was made to the `_status/nodes` endpoint to secure
it behind an Admin role in order to limit the spread of sensitive
information in node payloads to non-Admins. This included data such as
crdb args, env variables, and addresses. That previous change created a
regression in the DB Console because the cluster overview page relies
heavily on information gleaned from the nodes endpoint. The page became
useless for non-Admins since it couldn't receive any response from the
endpoint.

This change creates a new gRPC endpoint and payload types to mirror
those found in the `roachpb` package for the purpose of continuing to
expose node information to the DB Console. These endpoints are under the
`_status/nodes_ui` prefix and convert internal `roachpb` payloads to the
external facing ones (currently the payload types match exactly, but now
they can change freely), adding in sensitive data only when the caller
has the Admin role.

The DB Console has been modified to only use the new `nodes_ui` endpoint
which will dynamically return more data when the logged-in user is an
Admin.

non-UI callers on the `_status/nodes` gRPC endpoint such as the debug
zip commands and others have not been modified.

Resolves cockroachdb#70702

Release note (ui change): Non-Admin users of the DB Console have
regained the ability to view the cluster overview page. Users without
the Admin role will still see most data about their nodes but
information such as command line arguments, environment variables, and
IP addresses and DNS names of nodes will be hidden.
NodesUI wasn't checking an error and could trigger an NPE in that way.
I found this in a most comical way, by standing up a three node
roachprod cluster and creating an asymmetric network partition where
n2 wasn't able to contact n1:

```
iptables -A INPUT --source 10.142.0.80 -j DROP
```

I don't think this was ever released; the problem was introduced in
PR cockroachdb#71181, on 2021-10-11.

Release note (bug fix): CockroachDB could crash if network connectivity
was impaired. The stack trace (in cockroach-stderr.log) would contain
`server.(*statusServer).NodesUI` in that case.
Copy link
Collaborator

@rimadeodhar rimadeodhar left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 9 files at r1, 1 of 2 files at r2, 2 of 2 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @rafiss)


pkg/server/status.go, line 1357 at r5 (raw file):

}

func (s *statusServer) NodesUI(

Nit: Nit can we add a comment to this exported function prior to merge?


pkg/server/status.go, line 1583 at r5 (raw file):

}

func (s *statusServer) NodeUI(

Nit: Comment.


pkg/server/status.go, line 1380 at r6 (raw file):

	}

	return resp, err

Nit: This can be return resp, nil to make it obvious that err at this point is nil.

@dhartunian
Copy link
Collaborator Author

Since nits aren't affecting the behavior, I'm going to skip them to keep backports as consistent as possible between the 3 versions that are running this code.

@dhartunian dhartunian merged commit baf67a7 into cockroachdb:release-21.1 Oct 21, 2021
@dhartunian dhartunian deleted the backport21.1-71181 branch October 21, 2021 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants