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

Document the fallout from crdb PR #42563 #6321

Merged
merged 1 commit into from Jan 22, 2020

Conversation

knz
Copy link
Contributor

@knz knz commented Jan 10, 2020

No description provided.

@knz knz requested a review from jseldess January 10, 2020 17:40
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor Author

knz commented Jan 13, 2020

Updated as requested by @mattcrdb to mention the privileged UI screens.

Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, @knz. This LGTM with a nit, but I'd like another writer on the team to review for concision. I'll find someone.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jseldess and @knz)


releases/v19.1.6.md, line 42 at r1 (raw file):

### Security updates

- In previous releases, CockroachDB was allowing non-authenticated

nit: CockroachDB was allowing > CockroachDB allowed


releases/v19.2.2.md, line 44 at r1 (raw file):

- In previous releases, CockroachDB was allowing non-authenticated

Same as above.

Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jseldess and @knz)


releases/v19.1.6.md, line 50 at r1 (raw file):

  endpoints.

  A possible fallout from this change is that users might have

The second paragraph of a bullet needs to be indented two more times. Otherwise, you get:

Screen Shot 2020-01-13 at 2.35.52 PM.png

Same applies below.

@taroface taroface self-requested a review January 13, 2020 20:05
Copy link
Contributor

@taroface taroface left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @taroface)


releases/v19.1.6.md, line 56 at r1 (raw file):

  workarounds, is discussed
  [here](https://github.com/cockroachdb/cockroach/issues/43870).

Suggest this wording for conciseness (turning the second paragraph into a callout):

  • CockroachDB previously allowed non-authenticated access to privileged HTTP endpoints like /_admin/v1/events, which operate using root user permissions and can thus access (and sometimes modify) any and all data in the cluster. This security vulnerability has been patched by disallowing non-authenticated access to these endpoints.

      {{site.data.alerts.callout_danger}}
      Users who have built monitoring automation using these HTTP endpoints must modify their automation to work using an HTTP session token. Possible workarounds are discussed [here](https://github.com/cockroachdb/cockroach/issues/43870).
      {{site.data.alerts.end}}
    

releases/v19.1.6.md, line 72 at r1 (raw file):

  further instructions and workarounds, is discussed
  [here](https://github.com/cockroachdb/cockroach/issues/43870). The
  main envisioned solution is [to enable the `root` to access the web

Does/should this refer to "the affected Admin UI screens"? This seems to imply that no user besides root can access the Admin UI.


releases/v19.1.6.md, line 75 at r1 (raw file):

  UI](https://github.com/cockroachdb/cockroach/issues/43847) in a
  future version of CockroachDB.

Similar to above:

�- Some Admin UI screens (e.g., Jobs) were previously displayed using root user permissions, regardless of the logged-in user's credentials. This enabled insufficiently privileged users to access privileged information. This security vulnerability has been patched by using the credentials of the logged-in user to display all Admin UI pages, with insufficient privileges resulting in empty UI screens.

	{{site.data.alerts.callout_danger}}
	Users without a valid enterprise license are now unable to access certain Admin UI screens unless they have an admin role, which must be granted using a valid license. A future CockroachDB version may enable the `root` to access the Admin UI. Possible workarounds are discussed [here](https://github.com/cockroachdb/cockroach/issues/43870).
	{{site.data.alerts.end}}

@knz
Copy link
Contributor Author

knz commented Jan 13, 2020

Do you mind taking this PR over and polish it as you see fit?

@knz
Copy link
Contributor Author

knz commented Jan 15, 2020

cc @aaron-crl for your information

@knz
Copy link
Contributor Author

knz commented Jan 15, 2020

Please also include a link to cockroachdb/cockroach#44033 for the blank pages in the UI.

Copy link
Contributor

@taroface taroface left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)

@taroface taroface dismissed their stale review January 16, 2020 21:45

Took over the PR

Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

:lgtm: with a few nits. But it'd also be good to have Artem review and make sure this is aligned with the external communication.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @knz)


releases/v19.1.6.md, line 42 at r2 (raw file):

### Security updates

- CockroachDB previously allowed non-authenticated access to privileged HTTP endpoints like `/_admin/v1/events`, which operate using root user permissions and can thus access (and sometimes modify) any and all data in the cluster. This security vulnerability has been patched by disallowing non-authenticated access to these endpoints.

root user > root user


releases/v19.1.6.md, line 48 at r2 (raw file):

    {{site.data.alerts.end}}

- Some Admin UI screens (e.g., Jobs) were previously displayed using root user permissions, regardless of the logged-in user's credentials. This enabled insufficiently privileged users to access privileged information. This security vulnerability has been patched by using the credentials of the logged-in user to display all Admin UI pages, with insufficient privileges resulting in empty UI screens.

root user > root user


releases/v19.1.6.md, line 51 at r2 (raw file):

    {{site.data.alerts.callout_info}}
    Users without a valid enterprise license are now unable to access certain Admin UI screens unless they have an admin role, which must be granted using a valid license. Possible workarounds are discussed [here](https://github.com/cockroachdb/cockroach/issues/43870). A future CockroachDB version may [enable the `root` to access the Admin UI](https://github.com/cockroachdb/cockroach/issues/43847).

admin role > admin role

the root to access > the root user to access


releases/v19.1.6.md, line 87 at r2 (raw file):

| `/_status/stores/{node_id}`                            | Store details                     | Operational details                                |                     |

Note: "special" endpoints are subject to the cluster setting

Maybe make this an info callout? Also make cluster setting a link:

[cluster setting](../v20.1/cluster-settings.html)

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

looking at it again, there's a link and a sentence missing. I'm also synchronizing my comments here with Artem's doc :)

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @knz)

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @knz)


releases/v19.1.6.md, line 42 at r2 (raw file):

### Security updates

- CockroachDB previously allowed non-authenticated access to privileged HTTP endpoints like `/_admin/v1/events`, which operate using root user permissions and can thus access (and sometimes modify) any and all data in the cluster. This security vulnerability has been patched by disallowing non-authenticated access to these endpoints.

to these endpoints and restricting access to admin users only.


releases/v19.1.6.md, line 45 at r2 (raw file):

    {{site.data.alerts.callout_info}}
    Users who have built monitoring automation using these HTTP endpoints must modify their automation to work using an HTTP session token. Possible workarounds are discussed [here](https://github.com/cockroachdb/cockroach/issues/43870).

using an HTTP session token for an admin user.


releases/v19.1.6.md, line 51 at r2 (raw file):

    {{site.data.alerts.callout_info}}
    Users without a valid enterprise license are now unable to access certain Admin UI screens unless they have an admin role, which must be granted using a valid license. Possible workarounds are discussed [here](https://github.com/cockroachdb/cockroach/issues/43870). A future CockroachDB version may [enable the `root` to access the Admin UI](https://github.com/cockroachdb/cockroach/issues/43847).

Please remove all references to "enabling root to have a password" in the release note. It's not yet sure that this will be the preferred solution, in fact it's more likely we're going to backport cockroachdb/cockroach#43872 instead (but we don't know for sure). The link to the issue is the most important.


releases/v19.1.6.md, line 51 at r2 (raw file):

    {{site.data.alerts.callout_info}}
    Users without a valid enterprise license are now unable to access certain Admin UI screens unless they have an admin role, which must be granted using a valid license. Possible workarounds are discussed [here](https://github.com/cockroachdb/cockroach/issues/43870). A future CockroachDB version may [enable the `root` to access the Admin UI](https://github.com/cockroachdb/cockroach/issues/43847).

The issue link must not be to issue 43870 for the second point; the relevant issue link is cockroachdb/cockroach#44033 (separate issue)


releases/v19.1.6.md, line 90 at r2 (raw file):

`server.remote_debugging.mode`. Unless the setting was customized,
clients are only able to connect from the same machine as the node.
The list of UI pages affected by the second issue above includes but is not limited to:

- Job details
- Database details
- Table details
- Zone configurations

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jseldess, @knz, and @taroface)


releases/v19.1.6.md, line 51 at r3 (raw file):
This sentence currently implies that we've changed the product to remove the functionality for non-admin users. This is not the case. The current limitation is a bug and will be fixed. I recommend the following:

Access to these Admin UI pages is currently restricted to admin users only. We acknowledge this is a regression and intend to resolve this limitation as soon as possible in a patch revision. This situation is discussed further in (link to issue 44033).

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

Another thing: the general sentiment that "a license is currently necessary to access these things" is applicable to both points, not just the second point. This is also a a known limitation which we intend to lift with another product upgrade.

To summarize there are two known limitations as a result of the change:

  1. root cannot use HTTP login and a license is needed to create an admin user other than root. This limitation affects both points above. The workaround is to use a temporary eval license to create the 2nd admin user. This workaround is discussed on both issues 43870 and 44033. We intend to provide a product feature to enable root access to the HTTP endpoints (UI and monitoring), for core users without a license.

  2. access to the Jobs page, database details etc is currently limited to admin users. This is not intended (these pages should be accessible to non-admin users as well). This is a bug that was there before and only became visible due to the sec fix. This situation is discussed in issue 44033 and we intend to patch it.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jseldess, @knz, and @taroface)

@knz knz force-pushed the 20200110-sec-update branch 2 times, most recently from 1b4ee9e to 3da617b Compare January 17, 2020 14:32
@knz
Copy link
Contributor Author

knz commented Jan 17, 2020

I have propagated the new understanding to the text. PTAL

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jseldess, @knz, and @taroface)


releases/v19.1.6.md, line 50 at r1 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

The second paragraph of a bullet needs to be indented two more times. Otherwise, you get:

Screen Shot 2020-01-13 at 2.35.52 PM.png

Same applies below.

Done


releases/v19.1.6.md, line 56 at r1 (raw file):

Previously, taroface (Ryan Kuo) wrote…

Suggest this wording for conciseness (turning the second paragraph into a callout):

  • CockroachDB previously allowed non-authenticated access to privileged HTTP endpoints like /_admin/v1/events, which operate using root user permissions and can thus access (and sometimes modify) any and all data in the cluster. This security vulnerability has been patched by disallowing non-authenticated access to these endpoints.

      {{site.data.alerts.callout_danger}}
      Users who have built monitoring automation using these HTTP endpoints must modify their automation to work using an HTTP session token. Possible workarounds are discussed [here](https://github.com/cockroachdb/cockroach/issues/43870).
      {{site.data.alerts.end}}
    

Done


releases/v19.1.6.md, line 72 at r1 (raw file):

Previously, taroface (Ryan Kuo) wrote…

Does/should this refer to "the affected Admin UI screens"? This seems to imply that no user besides root can access the Admin UI.

Addressed above.


releases/v19.1.6.md, line 75 at r1 (raw file):

Previously, taroface (Ryan Kuo) wrote…

Similar to above:

�- Some Admin UI screens (e.g., Jobs) were previously displayed using root user permissions, regardless of the logged-in user's credentials. This enabled insufficiently privileged users to access privileged information. This security vulnerability has been patched by using the credentials of the logged-in user to display all Admin UI pages, with insufficient privileges resulting in empty UI screens.

	{{site.data.alerts.callout_danger}}
	Users without a valid enterprise license are now unable to access certain Admin UI screens unless they have an admin role, which must be granted using a valid license. A future CockroachDB version may enable the `root` to access the Admin UI. Possible workarounds are discussed [here](https://github.com/cockroachdb/cockroach/issues/43870).
	{{site.data.alerts.end}}

Done


releases/v19.1.6.md, line 42 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

root user > root user

Done.


releases/v19.1.6.md, line 48 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

root user > root user

Done


releases/v19.1.6.md, line 51 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

admin role > admin role

the root to access > the root user to access

Done


releases/v19.1.6.md, line 87 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Maybe make this an info callout? Also make cluster setting a link:

[cluster setting](../v20.1/cluster-settings.html)

Done


releases/v19.2.2.md, line 44 at r1 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Same as above.

Done.

Copy link
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

:lgtm: after a few tweaks.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jseldess, @knz, and @taroface)


releases/v19.1.6.md, line 87 at r2 (raw file):

Previously, knz (kena) wrote…

Done

Thanks, @knz. But now you should remove Note: as that language is added automatically by the callout style.

Also, the link should point to v19.1, not v20.1.


releases/v19.2.2.md, line 101 at r6 (raw file):

{{site.data.alerts.callout_info}}
Note: "special" endpoints are subject to the [cluster setting](../v20.1/cluster-settings.html)

Remove Note: and the link should point to v19.2, not v20.1.


releases/v2.1.10.md, line 100 at r6 (raw file):

{{site.data.alerts.callout_info}}
Note: "special" endpoints are subject to the [cluster setting](../v20.1/cluster-settings.html)

Remove Note: and the link should point to v2.1, not v20.1.

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jseldess, @knz, and @taroface)


releases/v19.1.6.md, line 87 at r2 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Thanks, @knz. But now you should remove Note: as that language is added automatically by the callout style.

Also, the link should point to v19.1, not v20.1.

Done.


releases/v19.2.2.md, line 101 at r6 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Remove Note: and the link should point to v19.2, not v20.1.

Done.


releases/v2.1.10.md, line 100 at r6 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Remove Note: and the link should point to v2.1, not v20.1.

Done.

@knz
Copy link
Contributor Author

knz commented Jan 17, 2020

@tim-o requests:

Generally I think Monday for both would be better - we want to be coordinated and also avoid looking like we’re sneaking something in before the weekend. I don’t want anyone to start an upgrade now. It’s also 4pm in London and after business hours in the rest of the world.

@tim-o
Copy link
Contributor

tim-o commented Jan 22, 2020

Merging. Thanks @dbist, @jseldess , and @knz !

@tim-o tim-o merged commit b4fef95 into cockroachdb:master Jan 22, 2020
@taroface taroface moved this from In review to Done in Education sprint 16 (1/6 - 1/17) Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants