rusackas commented on code in PR #40671:
URL: https://github.com/apache/superset/pull/40671#discussion_r3342857270
##########
superset/security/guest_token.py:
##########
@@ -14,13 +14,72 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import logging
from typing import Optional, TypedDict, Union
from flask_appbuilder.security.sqla.models import Group, Role
from flask_login import AnonymousUserMixin
from superset.utils.backports import StrEnum
+logger = logging.getLogger(__name__)
+
+# JWT claim that carries the revocation version a token was minted with.
+GUEST_TOKEN_REVOCATION_CLAIM = "rev" # noqa: S105
+
+# Tokens minted before the revocation feature existed carry no version claim.
+# They are treated as version 0, which is only rejected once an admin has
+# explicitly bumped the expected version above 0.
+DEFAULT_GUEST_TOKEN_REVOCATION_VERSION = 0
+
+
+def get_current_guest_token_revocation_version() -> int:
+ """
+ Return the minimum guest-token revocation version accepted at validation
time.
+
+ The value is stored in the metadata database via the ``key_value`` store
so it
+ can be bumped at runtime (e.g. via the ``revoke-guest-tokens`` CLI command)
+ without a code deploy. A missing entry means revocation has never been
+ triggered, so the default version is returned.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.key_value.shared_entries import get_shared_value
+ from superset.key_value.types import SharedKey
+
+ try:
+ value = get_shared_value(SharedKey.GUEST_TOKEN_REVOCATION_VERSION)
+ except Exception: # pylint: disable=broad-except
+ # Never let a metadata-store hiccup hard-fail token validation; fall
back
+ # to the default version (fail-open to the pre-feature behaviour).
+ logger.warning(
+ "Could not read guest token revocation version", exc_info=True
+ )
+ return DEFAULT_GUEST_TOKEN_REVOCATION_VERSION
Review Comment:
Thanks for flagging. This is an intentional, documented tradeoff rather than
a vulnerability under our security model. The revocation version lives in the
operator-controlled metadata database, which sits inside the operator trust
boundary (see SECURITY.md). A read failure here is an infrastructure outage,
not a condition an embedded guest-token holder can induce, so this does not let
any untrusted principal exceed their capabilities. Failing closed on a
transient metadata-store hiccup would instead take down all embedded access
during routine DB blips, which is the worse outcome for an opt-in feature whose
pre-feature behavior is "no revocation." The fallback to the default version is
deliberate and called out in the inline comment. Closing as out of scope.
##########
superset/security/guest_token.py:
##########
@@ -14,13 +14,72 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import logging
from typing import Optional, TypedDict, Union
from flask_appbuilder.security.sqla.models import Group, Role
from flask_login import AnonymousUserMixin
from superset.utils.backports import StrEnum
+logger = logging.getLogger(__name__)
+
+# JWT claim that carries the revocation version a token was minted with.
+GUEST_TOKEN_REVOCATION_CLAIM = "rev" # noqa: S105
+
+# Tokens minted before the revocation feature existed carry no version claim.
+# They are treated as version 0, which is only rejected once an admin has
+# explicitly bumped the expected version above 0.
+DEFAULT_GUEST_TOKEN_REVOCATION_VERSION = 0
+
+
+def get_current_guest_token_revocation_version() -> int:
+ """
+ Return the minimum guest-token revocation version accepted at validation
time.
+
+ The value is stored in the metadata database via the ``key_value`` store
so it
+ can be bumped at runtime (e.g. via the ``revoke-guest-tokens`` CLI command)
+ without a code deploy. A missing entry means revocation has never been
+ triggered, so the default version is returned.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.key_value.shared_entries import get_shared_value
+ from superset.key_value.types import SharedKey
+
+ try:
+ value = get_shared_value(SharedKey.GUEST_TOKEN_REVOCATION_VERSION)
+ except Exception: # pylint: disable=broad-except
+ # Never let a metadata-store hiccup hard-fail token validation; fall
back
+ # to the default version (fail-open to the pre-feature behaviour).
+ logger.warning(
+ "Could not read guest token revocation version", exc_info=True
+ )
+ return DEFAULT_GUEST_TOKEN_REVOCATION_VERSION
+ if value is None:
+ return DEFAULT_GUEST_TOKEN_REVOCATION_VERSION
+ try:
+ return int(value)
+ except (TypeError, ValueError):
+ logger.warning("Invalid guest token revocation version stored: %r",
value)
+ return DEFAULT_GUEST_TOKEN_REVOCATION_VERSION
+
+
+def bump_guest_token_revocation_version() -> int:
+ """
+ Increment and persist the guest-token revocation version, revoking every
+ outstanding guest token minted with a lower version.
+
+ :return: the new revocation version
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.key_value.shared_entries import upsert_shared_value
+ from superset.key_value.types import SharedKey
+
+ new_version = get_current_guest_token_revocation_version() + 1
+ upsert_shared_value(SharedKey.GUEST_TOKEN_REVOCATION_VERSION, new_version)
Review Comment:
Thanks, but this is not in scope. The bump only happens via the
revoke-guest-tokens CLI command, which is an Admin/operator action, and Admin
is a fully trusted principal in our model (SECURITY.md). A "lost update"
requires two operators running the revocation command simultaneously, which is
not an attacker-reachable path and not a privilege a guest-token holder can
exercise. The practical consequence of a rare concurrent double-bump is simply
that one of two near-simultaneous revocations needs to be re-run, monotonic
version increases are still preserved. Adding DB-side atomic CAS for a
manually-invoked admin CLI is unwarranted complexity here. Closing as out of
scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
