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

pgwire,sql: user account expiration #41396

Closed
knz opened this issue Oct 7, 2019 · 16 comments · Fixed by #45541
Closed

pgwire,sql: user account expiration #41396

knz opened this issue Oct 7, 2019 · 16 comments · Fixed by #45541
Assignees
Labels
A-security A-sql-pgwire pgwire protocol issues. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@knz
Copy link
Contributor

knz commented Oct 7, 2019

Requested by large customer account:

  • disable a user (but not delete) temporarily
  • disable a user at a specific date
  • disable a user "some time after" the last log-in

The first two points are equivalent, the last is separate.

@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-pgwire pgwire protocol issues. A-security labels Oct 7, 2019
@knz
Copy link
Contributor Author

knz commented Oct 7, 2019

cc @BramGruneir

@knz
Copy link
Contributor Author

knz commented Oct 7, 2019

cc @rolandcrosby @piyush-singh

@knz
Copy link
Contributor Author

knz commented Oct 7, 2019

Discussed this with @bdarnell today: we can add a column on system.users to track the timestamp of last login. To avoid/reduce contention, we can round the timestamp to the closest second, and only write the new value if it hasn't changed (CPut).

@bdarnell
Copy link
Contributor

bdarnell commented Oct 7, 2019

#29633 also mentions disabling users.

disable a user "some time after" the last log-in

After the last login event, or after the last logged-in session ends? Should this imply that any existing sessions are terminated at that point?

only write the new value if it hasn't changed (CPut).

CPut still works like a write for contention purposes. To get any reduction in contention, we need to do our own reads before writing.

@knz
Copy link
Contributor Author

knz commented Oct 7, 2019

After the last login event,

After the last login (regardless of how long the session was)

Should this imply that any existing sessions are terminated at that point?

The user understand they can use CANCEL SESSIONS (with a suitable filter on users) to preempt existing sessions

To get any reduction in contention, we need to do our own reads before writing.

ok thanks for clarifying

@mberhault
Copy link
Contributor

The standard postgres way is a combination of:

  • VALID UNTIL 'timestamp' for CREATE|ALTER ROLE
  • NOLOGIN (as opposed to LOGIN), also on CREATE|ALTER ROLE

The former disables the password (not sure if that applies to other authentication methods) at the specified time. The latter prevents the user from logging in at all.

The only odd case in the ones mentioned in the original message is "disable some time after the last login". That's a very specific request.

@RoachietheSupportRoach
Copy link
Collaborator

Zendesk ticket #3849 has been linked to this issue.

@knz
Copy link
Contributor Author

knz commented Jan 14, 2020

@ajwerner made me realize we already have a form of user expiration: the expiration field in the client TLS certificate. Customers can use that already.

@knz
Copy link
Contributor Author

knz commented Jan 24, 2020

@RichardJCai when you have some idea of what's to be done here, can you explain it to me? I'm tasked with adding some audit logging on a separate area and the two things need to combine gracefully.

@RichardJCai
Copy link
Contributor

@knz Sure thing, I'll message you on Slack once I have a plan.

@RichardJCai
Copy link
Contributor

@solongordon
Some thoughts about VALID UNTIL

Here is postgres' documentation for valid until

VALID UNTIL 'timestamp'
The VALID UNTIL clause sets a date and time after which the role's password is no longer valid. If this clause is omitted the password will be valid for all time.

(From what I understand, if Postgres has password required - then all users must have a password to login. If no password is required, the password is ignored - this means even if the password is "expired" then the user can still login see here for more)

To copy postgres' behavior, users must have a "password" in order to be disabled.
Do we want this behavior or should valid until simply not allow a user to login after the timestamp?

@knz
Copy link
Contributor Author

knz commented Jan 28, 2020 via email

@aaron-crl
Copy link

  • make passwords work like pg;

Following the pg design pattern has the advantage of behaving as other users may be accustomed and it potentially backed by perspectives we have not captured here.

As a security admin, if I seek to functionally disabled user's access to a system, I'd like to have user level security controls and not have to know all the ways that a user might have been granted access to the system. I'm pretty sure setting "NOLOGIN" does this, though doesn't trigger based on last login.

So I can see reasons to do both standard pg behavior for expired passwords, but also have a user activity trigger to lock login.

  • make a new something for cert authn. The new something is something we're missing anyway and eventually needed to support : revocation lists for client certs.

I think this feature should exist as a base access control regardless. This is a use case where many of the traditional security challenges related to revocation lists don't apply.

TL;DR: My gut feeling is that we:

  1. Do password expiration the pg way for consistency.
  2. Create a user account lock feature (which can terminate existing sessions) with a trigger that can be time since last login. This will lock the account regardless of whether the user has valid credentials.
  3. Implement revocation lists for client certs so that we have a means of revoking credentials on demand.

@RichardJCai
Copy link
Contributor

  • make passwords work like pg;

Following the pg design pattern has the advantage of behaving as other users may be accustomed and it potentially backed by perspectives we have not captured here.

As a security admin, if I seek to functionally disabled user's access to a system, I'd like to have user level security controls and not have to know all the ways that a user might have been granted access to the system. I'm pretty sure setting "NOLOGIN" does this, though doesn't trigger based on last login.

So I can see reasons to do both standard pg behavior for expired passwords, but also have a user activity trigger to lock login.

  • make a new something for cert authn. The new something is something we're missing anyway and eventually needed to support : revocation lists for client certs.

I think this feature should exist as a base access control regardless. This is a use case where many of the traditional security challenges related to revocation lists don't apply.

TL;DR: My gut feeling is that we:

  1. Do password expiration the pg way for consistency.
  2. Create a user account lock feature (which can terminate existing sessions) with a trigger that can be time since last login. This will lock the account regardless of whether the user has valid credentials.
  3. Implement revocation lists for client certs so that we have a means of revoking credentials on demand.

For point 2, terminating existing sessions can be tricky (and expensive) since we would have to check before any action. I think for the first pass for this ticket, we should look to copy pg's password expiry unless the customer specifies otherwise.

@knz
Copy link
Contributor Author

knz commented Jan 30, 2020

For point 2 we have a mechanism for this already. The operator who wants to kick user X out would:

  1. set NOLOGIN for X
  2. use CANCEL SESSIONS with a filter on username X

and then they get what they want.

@nstewart
Copy link
Contributor

Hi team, one other point re: VALID UNTIL. This seems to be a pattern that comes up when creating short-lived, instance-specific credentials in microservices. Take a look at how valid until is used for Vault's postgres SQL engine: https://www.vaultproject.io/docs/secrets/databases/postgresql/

If we don't have the bandwidth to support VALID UNTIL for this release, I'll put it on the backlog for consideration in the 20.2 timeframe.

RichardJCai added a commit to RichardJCai/cockroach that referenced this issue Mar 3, 2020
Release note (sql feature): Added LOGIN option to enable login for roles/users.
When using CREATE USER syntax, LOGIN is enabled by default whereas when using CREATE ROLE,
LOGIN is not enabled by default.
Added VALID UNTIL option to indicate passwords are only valid until a given timestamp.
VALID UNTIL only affects PASSWORDS - if passwords are disabled, VALID UNTIL has no effect.
(Can login even with expired password if passwords are not required to login - same as PG)
SHOW USERS now shows all roles/users that have LOGIN enabled.
SHOW ROLES shows every role/user (now that roles=users).

Fixes cockroachdb#41396
RichardJCai added a commit to RichardJCai/cockroach that referenced this issue Mar 5, 2020
Release note (sql change): Added LOGIN option to enable login for roles/users.
When using CREATE USER syntax, LOGIN is enabled by default whereas when using CREATE ROLE,
LOGIN is not enabled by default.
Added VALID UNTIL option to indicate passwords are only valid until a given timestamp.
VALID UNTIL only affects PASSWORDS - if passwords are disabled, VALID UNTIL has no effect.
(Can login even with expired password if passwords are not required to login - same as PG)
SHOW USERS now shows all roles/users that have LOGIN enabled.
SHOW ROLES shows every role/user (now that roles=users).

Fixes cockroachdb#41396
RichardJCai added a commit to RichardJCai/cockroach that referenced this issue Mar 5, 2020
Release note (sql change): Added LOGIN option to enable login for roles/users.
When using CREATE USER syntax, LOGIN is enabled by default whereas when using CREATE ROLE,
LOGIN is not enabled by default.
Added VALID UNTIL option to indicate passwords are only valid until a given timestamp.
VALID UNTIL only affects PASSWORDS - if passwords are disabled, VALID UNTIL has no effect.
(Can login even with expired password if passwords are not required to login - same as PG)
SHOW USERS now shows all roles/users that have LOGIN enabled.
SHOW ROLES shows every role/user (now that roles=users).

Fixes cockroachdb#41396
RichardJCai added a commit to RichardJCai/cockroach that referenced this issue Mar 6, 2020
Release note (sql change): Added LOGIN option to enable login for roles/users.
When using CREATE USER syntax, LOGIN is enabled by default whereas when using CREATE ROLE,
LOGIN is not enabled by default.

Release note (sql change): Added VALID UNTIL option to indicate passwords are only valid until a given timestamp.
VALID UNTIL only affects PASSWORDS - if passwords are disabled, VALID UNTIL has no effect.
(Can login even with expired password if passwords are not required to login - same as PG)

Release note (sql change): SHOW USERS now shows all roles/users that have LOGIN enabled.

Release note (security update): User and role principals can now be prevented from logging in
using the NOLOGIN attribute which can be set using ALTER USER/ROLE.

Release note (security update): The password field (used exclusively for password-based authentication)
can now be configured to have an expiry date using the VALID UNTIL attribute, which can be set with ALTER USER/ROLE.
Note that the attribute sets an expiry date for the password, not the user account.
This is consistent with PostgreSQL.

Fixes cockroachdb#41396
RichardJCai added a commit to RichardJCai/cockroach that referenced this issue Mar 6, 2020
Release note (sql change): Added LOGIN option to enable login for roles/users.
When using CREATE USER syntax, LOGIN is enabled by default whereas when using CREATE ROLE,
LOGIN is not enabled by default.

Release note (sql change): Added VALID UNTIL option to indicate passwords are only valid until a given timestamp.
VALID UNTIL only affects PASSWORDS - if passwords are disabled, VALID UNTIL has no effect.
(Can login even with expired password if passwords are not required to login - same as PG)

Release note (sql change): SHOW USERS now shows all roles/users that have LOGIN enabled.

Release note (security update): User and role principals can now be prevented from logging in
using the NOLOGIN attribute which can be set using ALTER USER/ROLE.

Release note (security update): The password field (used exclusively for password-based authentication)
can now be configured to have an expiry date using the VALID UNTIL attribute, which can be set with ALTER USER/ROLE.
Note that the attribute sets an expiry date for the password, not the user account.
This is consistent with PostgreSQL.

Release note (security update): Any role created prior to 20.1 will be able to log in into
clusters started with --insecure, unless/until they are granted the NOLOGIN privilege.

Fixes cockroachdb#41396
RichardJCai added a commit to RichardJCai/cockroach that referenced this issue Mar 6, 2020
Release note (sql change): Added LOGIN option to enable login for roles/users.
When using CREATE USER syntax, LOGIN is enabled by default whereas when using CREATE ROLE,
LOGIN is not enabled by default.

Release note (sql change): Added VALID UNTIL option to indicate passwords are only valid until a given timestamp.
VALID UNTIL only affects PASSWORDS - if passwords are disabled, VALID UNTIL has no effect.
(Can login even with expired password if passwords are not required to login - same as PG)

Release note (sql change): SHOW USERS now shows all roles/users that have LOGIN enabled.

Release note (security update): User and role principals can now be prevented from logging in
using the NOLOGIN attribute which can be set using ALTER USER/ROLE.

Release note (security update): The password field (used exclusively for password-based authentication)
can now be configured to have an expiry date using the VALID UNTIL attribute, which can be set with ALTER USER/ROLE.
Note that the attribute sets an expiry date for the password, not the user account.
This is consistent with PostgreSQL.

Release note (security update): Any role created prior to 20.1 will be able to log in into
clusters started with --insecure, unless/until they are granted the NOLOGIN privilege.
For secure clusters, roles cannot log in by virtue of not having a password nor a client certificate.

Fixes cockroachdb#41396
RichardJCai added a commit to RichardJCai/cockroach that referenced this issue Mar 7, 2020
Fixes cockroachdb#41396

Release note (sql change): Added LOGIN option to enable login for roles/users.
When using CREATE USER syntax, LOGIN is enabled by default whereas when using CREATE ROLE,
LOGIN is not enabled by default.

Release note (sql change): Added VALID UNTIL option to indicate passwords are only valid until a given timestamp.
VALID UNTIL only affects PASSWORDS - if passwords are disabled, VALID UNTIL has no effect.
(Can login even with expired password if passwords are not required to login - same as PG)

Release note (sql change): SHOW USERS now shows all roles/users that have LOGIN enabled.

Release note (security update): User and role principals can now be prevented from logging in
using the NOLOGIN attribute which can be set using ALTER USER/ROLE.

Release note (security update): The password field (used exclusively for password-based authentication)
can now be configured to have an expiry date using the VALID UNTIL attribute, which can be set with ALTER USER/ROLE.
Note that the attribute sets an expiry date for the password, not the user account.
This is consistent with PostgreSQL.

Release note (security update): Any role created prior to 20.1 will be able to log in into
clusters started with --insecure, unless/until they are granted the NOLOGIN privilege.
For secure clusters, roles cannot log in by virtue of not having a password nor a client certificate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security A-sql-pgwire pgwire protocol issues. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
7 participants