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

NAS-131396 / 25.04 / Add support for user-linked API keys #14578

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

anodos325
Copy link
Contributor

The primary motivation for converting API keys so that they are explicitly linked to user accounts is to provide some method for TrueNAS Connect to use tie in to local or directory services accounts to a credential that is persistently stored in the TrueNAS Connect keychain.

The legacy API key mechanism was insufficient for this purpose because the mismatch between user accounts and API keys could lead to administrators retaining NAS access after account deletion, expiration, or locking. For more in-depth context and reasoning please refer to the internal design document NEP-053.

For this purpose, this commit makes the following changes:

  1. Legacy API keys are migrated to a user_identifier LEGACY_API_KEY which is automatically linked to the root, admin, or truenas_admin account depending on server configuration.
  2. If the legacy API key granted less than FULL_CONTROL, then the API key is migrated with a revoked state so that the system administrator has an opportunity to review and generate a new key and/or service account that provides the correct level of access.
  3. API key authentication now passes through libpam.
  4. user.query results now include an api_keys key that contains a list of IDs for API keys that exist for the user.
  5. Authenticated users with READONLY_ADMIN privilege or greater are able to create and manage their own API keys.

During development of this new feature, it was determined that the original API keys were written with insufficient hashing rounds leading to the following changes:

  1. On successful authentication the stored hash of any LEGACY_API_KEY is automatically upgraded to a newer sha512-based standard with significantly increased hashing rounds.
  2. Dependency on the passlib library was removed.

Since libpam is now used for all non-token authentication methods, a new auth login endpoint (auth.login_ex) was added to middleware to facilitate future enhancements for challenge-response authentication mechanisms. Authentication via username + password + OTP token has been converted to this new standard. This new endpoint is more closely aligned with standards and requirements in NIST SP 800-63B. In summary the following changes were made:

  1. An AuthentationContext dataclass was created to house a middleware session's PAM context and associated information. An instance of this is stored in the session's App object.
  2. A new endpoint auth.login_ex was added that is expandable and is aware of the server's configured "authentication assurance level".
  3. Existing login endpoints (auth.login, auth.login_with_api_key, auth.login_with_token) were converted to wrappers around auth.login_ex.
  4. Initial implementation of session lifetime and inactivity guidelines for different authentication assurance levels was added.
  5. Challenge-response workflow for username + password + OTP token implemented with associated tests.

The primary motivation for converting API keys so that they are explicitly
linked to user accounts is to provide some method for TrueNAS Connect
to use tie in to local or directory services accounts to a credential
that is persistently stored in the TrueNAS Connect keychain.

The legacy API key mechanism was insufficient for this purpose because
the mismatch between user accounts and API keys could lead to
administrators retaining NAS access after account deletion, expiration,
or locking. For more in-depth context and reasoning please refer to the
internal design document NEP-053.

For this purpose, this commit makes the following changes:
1. Legacy API keys are migrated to a user_identifier LEGACY_API_KEY
   which is automatically linked to the root, admin, or truenas_admin
   account depending on server configuration.
2. If the legacy API key granted less than FULL_CONTROL, then the
   API key is migrated with a `revoked` state so that the system
   administrator has an opportunity to review and generate a new
   key and/or service account that provides the correct level of
   access.
3. API key authentication now passes through libpam.
4. user.query results now include an `api_keys` key that contains a
   list of IDs for API keys that exist for the user.
5. Authenticated users with READONLY_ADMIN privilege or greater are
   able to create and manage their own API keys.

During development of this new feature, it was determined that the
original API keys were written with insufficient hashing rounds leading
to the following changes:

6. On successful authentication the stored hash of any LEGACY_API_KEY
   is automatically upgraded to a newer sha512-based standard with
   significantly increased hashing rounds.
7. Dependency on the passlib library was removed.

Since libpam is now used for all non-token authentication methods, a
new auth login endpoint (auth.login_ex) was added to middleware to
facilitate future enhancements for challenge-response authentication
mechanisms. Authentication via username + password + OTP token has been
converted to this new standard. This new endpoint is more closely
aligned with standards and requirements in NIST SP 800-63B. In summary
the following changes were made:

8. An AuthentationContext dataclass was created to house a middleware
   session's PAM context and associated information. An instance of
   this is stored in the session's App object.
9. A new endpoint auth.login_ex was added that is expandable and is
   aware of the server's configured "authentication assurance level".
10. Existing login endpoints (auth.login, auth.login_with_api_key,
   auth.login_with_token) were converted to wrappers around
   auth.login_ex.
11. Initial implementation of session lifetime and inactivity guidelines
   for different authentication assurance levels was added.
12. Challenge-response workflow for username + password + OTP token
   implemented with associated tests.
@anodos325 anodos325 added the jira label Sep 25, 2024
@bugclerk bugclerk changed the title Add support for user-linked API keys NAS-131396 / 25.04 / Add support for user-linked API keys Sep 25, 2024
@bugclerk
Copy link
Contributor

Copy link
Contributor

@mgrimesix mgrimesix left a comment

Choose a reason for hiding this comment

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

This is a superficial flake8 issue review.
Second pass will be real review.

from middlewared.utils.auth import LEGACY_API_KEY_USERNAME
from middlewared.utils.crypto import generate_pbkdf2_512, generate_string
from middlewared.utils.privilege import credential_has_full_admin
from middlewared.utils.sid import sid_is_valid
from middlewared.utils.time_utils import utc_now
if TYPE_CHECKING:
from middlewared.main import Middleware
Copy link
Contributor

Choose a reason for hiding this comment

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

Middleware is unused, but this might be unavoidable. The flake8 error is F401.
Maybe add # noqa: F401

tests/api2/test_api_key.py Outdated Show resolved Hide resolved
@anodos325 anodos325 force-pushed the user-api-keys branch 2 times, most recently from f0744c4 to cb5d4ac Compare September 26, 2024 17:01
Copy link
Member

@sonicaj sonicaj left a comment

Choose a reason for hiding this comment

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

Had an initial pass, awesome work 🚀

try:
if json.loads(row['allowlist']) != DEFAULT_ALLOW_LIST:
to_revoke.append(str(row['id']))
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

Should we log it somewhere ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants