mirror of
https://github.com/gravitational/teleport.git
synced 2026-10-11 22:49:54 +00:00
master
44
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
0d76b1dee6 |
auth: Add v9 downgrade property tests (#68916)
Exercise `maybeDowngradeRoleVersionToV8` with property tests so the guarantee holds for arbitrary roles, not just the handful of hand-written roles the example test checks. The downgrade is a security control. A pre-v19 agent cannot enforce a v9 role's `app_resources` restriction, so the downgraded v8 copy must move the role's allow app selector to the deny side and never leave one of its own apps reachable. The allow-to-deny move branches on whether a deny selector already exists, which is where a fail-open bug would go unnoticed. Generate random allow and deny label selectors and candidate apps, then assert three properties. Every app the role allowed before the downgrade is denied after. Every app it already denied stays denied. The input role comes back unmodified, because the caller passes the cache's shared role. Add apps matching each selector, and the deny expression, to every generated set, because random apps match a keyed selector in under a tenth of draws and the assertion body is otherwise skipped. Draw globs as selector values, since `services.MatchLabels` matches them as regular expressions and a downgrade that dropped them would serve a copy denying nothing. Cover the AND-to-OR downgrade in a second property, which adds a label expression on each side. An allow rule needs both `app_labels` and `app_labels_expression` to match while a deny rule needs either, so moving them across separately denies more apps than the role allowed. Add an example test for that over-deny, where a wildcard label set moved to the deny side blocks apps outside the allow rule. Use `pgregory.net/rapid`, already a direct dependency of this branch. |
||
|
|
7a177b60a3 |
buf: Drop the reserved-deletion except (#69554)
* types: Declare all app_resources fields in the proto
Declare the eight reserved AppResource fields (paths, methods, where,
allow_encoded, allow_code, allow_reason, deny_code_hint,
deny_reason_hint) and the AppResourcesExpressions role condition, and
regenerate the proto stubs, the role CRDs, the Terraform schema and
docs, and the derived equality functions. Declaring the fields before
the fine-grained matcher merges lets every client of this version name
them, so a client that round-trips a role written by a newer auth
preserves the fields instead of silently dropping them on write-back,
which would widen the rule.
Tighten IsAllowAllOnly to require every declared field besides
allow_all to be unset. Without this, a rule such as {allow_all: true,
methods: [GET]} written by a newer auth would pass the check once the
fields are named, and this version would treat a method-restricted
rule as unrestricted.
Add GetAppResourcesExpressions to the Role interface, and read it in
decideMinimalV9 so the app agent denies a request governed by a
predicate it cannot evaluate. A predicate restricts the rules it
accompanies, so a role that pairs allow_all with one is not
unrestricted, and a deny-side predicate blocks another role's
allow_all the same way a deny-side rule does. Auth already treats both
as restricting when it downgrades a role for a pre-v9 client, so
without this the agent and auth disagree about the same role.
Strip AppResourcesExpressions in the v9-to-v8 downgrade alongside
AppResources, and reject it at write, so a role can neither reach a
pre-v9 client with the restriction silently removed nor be stored
while this version cannot enforce it.
* docs: List the declared app_resources fields
Honour the docs review on the app-resources reference page. A run of
inline code-style field names inside a sentence is hard to scan, so the
eight rejected `app_resources` fields become a list, and
`app_resources_expressions` gets its own sentence rather than trailing
the same run.
* buf: Trim the reserved-deletion except comment
Drop the two sentences explaining why buf's ignore lists cannot scope the
except. The rule name and the removal condition are what a reader needs
here, and the scoping attempts are recorded on the pull request.
* types: Reword the app_resources_expressions and Methods docs
Honour the review on the proto field comments. Drop "desugared" from
`AppResourcesExpressions`, which needs the RFD to gloss, and state the
relation between the two forms directly instead. Open `Methods` with the
plural, matching `Paths` on the row above.
Regenerate the proto stubs, the role CRDs, the Terraform schema and docs.
* buf: Drop the reserved-deletion except
The `RESERVED_MESSAGE_NO_DELETE` except covered one deletion, the
`app_resources` reservations on `AppResource` and `RoleConditions`.
`make protos/breaking` compares against `origin/master`, so the except
is needed only while master still holds those reservations. Once master
declares the fields, nothing is deleted relative to the baseline and the
rule passes on its own.
Restore the rule for every proto in the repo, since `except` is
repo-wide and cannot be scoped to one file or one module.
|
||
|
|
677b0d6e1c |
types: Declare all app_resources fields (#69500)
* types: Declare all app_resources fields in the proto
Declare the eight reserved AppResource fields (paths, methods, where,
allow_encoded, allow_code, allow_reason, deny_code_hint,
deny_reason_hint) and the AppResourcesExpressions role condition, and
regenerate the proto stubs, the role CRDs, the Terraform schema and
docs, and the derived equality functions. Declaring the fields before
the fine-grained matcher merges lets every client of this version name
them, so a client that round-trips a role written by a newer auth
preserves the fields instead of silently dropping them on write-back,
which would widen the rule.
Tighten IsAllowAllOnly to require every declared field besides
allow_all to be unset. Without this, a rule such as {allow_all: true,
methods: [GET]} written by a newer auth would pass the check once the
fields are named, and this version would treat a method-restricted
rule as unrestricted.
Add GetAppResourcesExpressions to the Role interface, and read it in
decideMinimalV9 so the app agent denies a request governed by a
predicate it cannot evaluate. A predicate restricts the rules it
accompanies, so a role that pairs allow_all with one is not
unrestricted, and a deny-side predicate blocks another role's
allow_all the same way a deny-side rule does. Auth already treats both
as restricting when it downgrades a role for a pre-v9 client, so
without this the agent and auth disagree about the same role.
Strip AppResourcesExpressions in the v9-to-v8 downgrade alongside
AppResources, and reject it at write, so a role can neither reach a
pre-v9 client with the restriction silently removed nor be stored
while this version cannot enforce it.
* docs: List the declared app_resources fields
Honour the docs review on the app-resources reference page. A run of
inline code-style field names inside a sentence is hard to scan, so the
eight rejected `app_resources` fields become a list, and
`app_resources_expressions` gets its own sentence rather than trailing
the same run.
* buf: Trim the reserved-deletion except comment
Drop the two sentences explaining why buf's ignore lists cannot scope the
except. The rule name and the removal condition are what a reader needs
here, and the scoping attempts are recorded on the pull request.
* types: Reword the app_resources_expressions and Methods docs
Honour the review on the proto field comments. Drop "desugared" from
`AppResourcesExpressions`, which needs the RFD to gloss, and state the
relation between the two forms directly instead. Open `Methods` with the
plural, matching `Paths` on the row above.
Regenerate the proto stubs, the role CRDs, the Terraform schema and docs.
|
||
|
|
466fccde36 |
operator: Add role v9 CRD kind (#68737)
* operator: Add role v9 CRD kind
Register a TeleportRoleV9 kind with the Kubernetes operator so v9
roles can be managed declaratively. The reconciler reuses the shared
RoleV6 storage type with a version override, the same way the v7 and
v8 kinds do.
Generated changes:
make -C integrations/operator generate
* operator: Drop preserve-unknown on app_resources
Setting x-kubernetes-preserve-unknown-fields on app_resources rule items
relaxed the CRD schema to store fields it does not declare, so the operator
could reject them on decode. That keeps invalid data in cluster state and is
hard to walk back once shipped, even after Kubernetes field validation
improves. Rely on the structural schema and server-side field validation to
reject unknown fields at the API server instead, and drop the tests that
asserted the operator-side rejection.
* operator: Match the reconciler factory signature
The scoped operator work added an OperatorMetadata parameter to
ReconcilerFactory and updated every existing role reconciler. The v9
reconciler was written before that change, so master and this branch each
compile alone but not once merged, and the merge queue rejects the result.
Take the parameter and ignore it, as the v6 to v8 reconcilers do.
|
||
|
|
b887107e7f |
appresource: Add path tokenizer and validator (#68795)
* appresource: Add path tokenizer and validator Create the `lib/appresource` package with the request-path tokenizer and wire-form validator, promoted from the sketch branch. `Tokenize` validates a raw request path and splits it into segments without decoding the bytes it forwards. A separate decode is used only as a validation view. Tokenize rejects any byte outside the RFC 3986 path set less ";", admits the encoded separator `%2F`/`%2f` and percent-encoded non-ASCII content as the only escapes, rejects "." and ".." segments and "//" on a view that also decodes the encoded separator, and requires each segment, decoded once, to be valid UTF-8, NFKC-stable, and made of graphic runes. Together these prevent the canonicalization and fold bypasses where an upstream decodes or normalizes a path onto a segment the matcher never saw. Paths over 8 KiB are rejected. The package is a leaf so the app agent, role validation in `lib/services`, and tctl can all import it. Node matching, captures, and the encoded-slash grammar follow in later changes. * appresource: Validate path content per segment The NFKC and graphic-rune checks ran on the whole path body with the encoded separator `%2F` kept literal. A combining mark placed right after an encoded slash then composed with that literal `F` (`F` plus U+0307 forms `Ḟ`), so a legitimate path was rejected as not NFKC-normalized even though its real decoded content is normal. Move the content checks into `validateDecoded` so they run per segment on the decode-for-validation view, where `%2F` is a separator. An encoded slash and a raw `/` now validate the same way, and the merge drops a second `decodeSeparators` pass and split. Pin the non-breaking-space reject case to the NFKC rule, since the graphic check would otherwise let it pass for the wrong reason. * appresource: Address tokenizer review feedback Reject a segment that starts with a combining mark. The mark pairs with the previous rune, and net/http.ServeMux still splits on the literal slash, so upstreams may disagree on the segment boundary. RFC 5891 imposes the same rule on the start of a domain label. Re-add the rapid property tests. Unlike the fuzz target, which only replays its seed corpus under go test, they exercise the tokenizer invariants on every run. Register the tokenizer fuzz target in fuzz/oss-fuzz-build.sh and pin the encoded hash %23 (#) as rejected. * appresource: Allow the encoded space %20 in paths Allow %20 as content, alongside the non-ASCII escapes. A space has no raw form in a URL path, so the escape is its only representation, and rejecting it makes resources unreachable with no workaround. Jenkins serves job names with spaces at /job/My%20Job/lastBuild, and SharePoint serves its default library at Shared%20Documents. A space cannot decode into a separator, so it needs neither a rule-global grant nor a per-segment pattern, unlike the encoded separator %2F. Reject a segment that starts or ends with a space. Windows and IIS trim a trailing space from a segment, so ".. " would resolve as ".." and "secret " would resolve as "secret", escaping a carve-out. That is the same fold-to-a-different-segment problem the other content checks prevent, reached by trimming rather than normalization. Reject a segment of only dots and spaces, which folds the former "." and ".." equality check into one rule. A normalizer that strips spaces or a single trailing dot resolves ". ." and "..." the same way. Permit U+0020 alone in isGraphicRune, never the Zs category. U+1680 ogham space mark, U+2028, U+200B and U+0085 are NFKC-stable, so the graphic-rune check is the only place that rejects them. * appresource: Reject a segment's trailing dots Reject a segment whose trailing run of dots and spaces contains a dot. IIS and Windows trim trailing dots and spaces from a path component, so an upstream would resolve `secret.` and `secret%20.` as `secret`, a segment the matcher never saw. The edge-space rule already closes this trim vector for the space; close it for the dot the same way. A leading dot stays allowed, so `/.well-known` remains reachable. Fold the escape predicate shared by `validatePercentEscapes` and `decode` into `isAllowedEscape`, so the two functions visibly accept the same escape set. Clip paths and segments to 256 bytes in error messages, because a rejected path can be kilobytes long and the message survives into logs. Pin the acceptance of a combining mark riding a trailing interior space, which ends the segment with a mark rather than a space byte, so no upstream trim fires on it. * appresource: Reject a leading conjoining jamo Extend the leading-mark check with the NFKC boundary property. A conjoining Hangul jamo is a letter, not a mark, so unicode.IsMark misses it, yet it composes onto the character before it the same way. A segment of "ᆨ" after a segment ending in "가" joins to "각" once an upstream drops the separator, which is the fold the check prevents. Render an illegal byte as a one-byte slice instead of converting it to a rune, so a raw 0xC3 prints as "\xc3" rather than "Ã". * appresource: Simplify godocs and merge byte checks Drop the numbered rule list from the Tokenize godoc, along with the terms NFKC-stable, graphic rune, and conjoining Hangul jamo that a caller does not need in order to call the function correctly. Say what to pass, what comes back, and which paths are rejected, and keep the precise rules on the reject helpers next to the code that enforces them. Add an inline comment where the leading mark check needs both unicode.IsMark and the NFKC boundary property, since neither covers the other. Rename lengthCap to maxPathLength, which reads better at the call site, and merge validatePathBytes and validatePercentEscapes into validateRawBytes so the raw path is walked once rather than twice. Both loops were byte-level and neither depended on the other having finished, and the hex digits the escape branch skips are alphanumeric, so they were always legal path bytes anyway. Swap the formatted testify variants in the property tests for the plain ones, which take the same message and arguments. |
||
|
|
2a05d3edfe |
role: Add minimal v9 with app default-deny (#68736)
* types: Add minimal role v9 with app default-deny
Add role resource version v9. A v9 role denies all access to HTTP apps
by default: when it grants an app via `app_labels` or
`app_labels_expression`, every HTTP request is denied unless an
allow-side `app_resources` rule permits it (RFD 0303).
Add `app_resources` to a role's `spec.allow` (not `spec.deny`), rules
controlling access to an app's resources on each HTTP request. For now
a rule holds one field, `allow_all`, which grants unrestricted access,
equivalent to v8, and must be the only rule. Reserve field numbers for
the fine-grained fields (`paths`, `methods`, `where`, audit codes) so
later releases add them without renumbering. Bump the role version in
an N.0.0 release: the breaking default-deny aims to ship in Teleport
v19.0.0.
Reject at write time any `app_resources` use on pre-v9 roles, under
deny, in a rule that does not set `allow_all`, or in a rule next to
another rule.
Serve v9 roles to clients older than 19.0.0 as v8 copies: the auth
server strips `app_resources` and marks the copy with the
`teleport.internal/downgraded` label. The stored role stays v9; only
what goes over the wire changes. The downgrade fails closed: unless a
rule sets `allow_all` (exactly v8 access, lossless), the copy also
loses its allow `app_labels` and `app_labels_expression`, so an older
agent denies the app entirely instead of serving it unrestricted.
Enforce the default-deny in the app agent's plain-HTTP path; TCP, MCP,
cloud, and LLM apps are excluded. When v8 and v9 roles grant the same
app, drop the v8 roles from the decision so they cannot re-open
unrestricted access, and log a warning once.
Bundle the Terraform provider update so v9 roles can be managed as
code starting with v19.0.0. The operator v9 CRD kind follows
separately.
Generated changes:
make grpc/host
make derive
make -C integrations/terraform docs
make -C integrations/operator generate
* auth: Downgrade roles from GetCurrentUserRoles
Run every role streamed by GetCurrentUserRoles through
maybeDowngradeRole before sending, matching GetRole, GetRoles,
ListRoles, and the watch stream. Without it, a pre-19 client asking a
remote cluster for its mapped roles receives a v9 role with allow
app_labels intact and no app_resources enforcement it understands, so
client-side access checks treat a default-deny role as granting full
app access. The change also applies the pre-existing v8-to-v7
downgrade on this surface, which previously served v8 roles to v17
clients unchanged.
* terraform: Exclude deny-side app_resources from schema
Drop `app_resources` from the Terraform role schema's deny block, as
the server rejects any deny-side `app_resources` at write time and
RFD 0303 keeps the rules allow-only so that they fail closed. With the
field gone from the schema, Terraform rejects a deny-side rule at plan
time instead of leaving it to the server's write-time check. Follow the
existing `RoleV6.Spec.Deny.Namespaces` exclusion.
Generated changes:
make -C integrations/terraform docs
* docs: Split role versions into per-version headings
Break the Role versions section into one H4 per role version so
readers can jump straight to the version they care about. Move the
SAML IdP note under the v8 heading, since it describes a v8 change,
and keep the kubernetes_resources defaults table as its own H4 at
the end. Fix a mismatched backtick around `v5` in the moved
session-joining paragraph.
* app: Warn when version skew denies a v9 request
When the minimal v9 check denies a request because the user's roles
carry `app_resources` rules or role versions this Teleport version
does not implement, the app agent previously answered 403 without any
log line. In a mixed-version cluster an operator saw denied requests
with no explanation that the deny came from a version mismatch rather
than from an intended default-deny role.
Track the condition as `versionSkew` on `minimalV9Decision`:
deny-side rules, allow rules beyond a single pure `allow_all`, or a
role version above v9. When such a deny fires, log one warning per
user and app through the existing `v9WarnOnce` mechanism, telling the
operator to upgrade the app agent. An intended default-deny, a v9 role
without app rules, stays quiet. A follow-up PR adds a minimal audit
event for the same denials.
* app: Deny apps a downgraded v9 role restricted
A v9 role that restricts app access downgrades to v8 for a pre-v19 agent
that cannot enforce the restriction. Clearing the role's allow app labels
alone would only stop that role granting the apps; another v8 role the user
holds could still grant them, so the restriction would not hold on the old
agent. Move the role's allow app selector to the deny side instead, so no
other role grants the apps this role restricted. The deny is at least as
strict as the v9 restriction and never grants more.
If the role already denies apps by label, there is no room to merge a
second label map, so fall back to a wildcard deny and log it rather than
drop the existing deny.
* docs: Correct v9 downgrade mixed-version guidance
Describe the deny-side downgrade the code actually performs. Without
`allow_all`, the v8 copy moves the allow app selector to the deny side
rather than dropping it, so deny rules override allow rules across a user's
roles and a separate v8-or-older role can no longer reopen the app on
pre-v19 agents. Note that the moved selector denies conservatively and can
block more apps than the v9 role allowed in a mixed-version cluster.
* role: Deny v9 app access for unknown JSON fields
Empty any v9 allow app_resources rule whose stored JSON carried a field
this version does not recognize, so a rule written by a newer version cannot
read as more permissive than intended. An unrecognized field already denies
on the other surfaces: proto reads keep it in XXX_unrecognized so
IsAllowAllOnly rejects it, and writes reject it at validation. JSON unmarshal
drops unknown fields silently, so this re-scans the raw bytes on read. The
known field set is derived from the AppResource message, so a new proto field
extends it automatically. The result is a single rule across every surface,
an unrecognized field grants nothing, worst case over-deny.
* app: Deny app access from roles newer than v9
A role from a version above v9 may carry restrictions this agent cannot
evaluate, so treat it as deny even when its known fields form a plain
allow_all. Previously such a role set both versionSkew and allowed, and
enforceMinimalV9 returns early on allowed, so the request was forwarded and
the skew warning never fired.
* auth: Explain the greedy deny in the v9 downgrade
The downgrade moves a v9 role's allow app selector to the deny side, which
looks like it could clobber an existing deny expression. Record why it
cannot, since deny matches on either selector, and why label maps fall back
to a wildcard where expressions compose with an or.
* api: Guard the AppResource field set with a test
IsAllowAllOnly decides whether a rule is unrestricted by checking that
allow_all is set and no unknown bytes arrived over the wire. A field added
in tree, once one of the reserved numbers is unreserved, would restrict the
rule without making that check return false. Pin the field set so such an
addition fails the test, and name the checks to re-evaluate next to the
reserved numbers in the proto.
* app: Bound the v9 warning deduplication cache
The v9 enforcement warnings fire once per warning kind, user, and app, and
the sync.Map holding those keys never dropped an entry, so a long-running
app agent serving many users and apps grew it for the life of the process.
Hold the keys in a fixed-size LRU instead. Evicting the least recently
warned entry repeats its warning, which is the better failure mode.
* role: Address review feedback
Reword the checkAppResources godoc so the api module does not reference
lib/services. Move the unknown-field check for allow app_resources rules
from integrations/lib/tctl into lib/services next to the other role
unmarshal handling, and derive its known field set from the AppResource
message instead of hardcoding allow_all, so unreserving a proto field
extends it without a second edit. Replace the package-level
preV9RoleVersions slice with roleVersionPredatesV9, a function with a
switch, so nothing mutable sits at package scope.
* role: Address agent review feedback
Reword the `decideMinimalV9` godoc so only a v9 role can allow; a
version above v9 always denies. Drop the `AppResource` message summary
sentence that misstated the reserved field numbers as path, method, and
where rules; fields 6 to 9 hold the audit and deny-hint fields. Reword
the `TestAppResourceFields` comment so the api module does not point
at `lib/services`. Give `v10Grants` an `allow_all` rule so the
above-v9 test case fails if a newer-version role with a plain
`allow_all` rule ever allows again. Add a downgrade subtest for the
deny-expression OR-merge, which no test executed, and a `lib/services`
table test for `CheckAppResourcesKnownFields`, which was only
exercised from the integrations module after its move.
* role: Compact review-feedback additions
Tighten the previous commit's additions, partly to keep the PR under
5,000 added lines. Compact the `CheckAppResourcesKnownFields` table
test into direct assertions, flatten the allowed branch in
`decideMinimalV9`, iterate the `AppResource` fields with
`reflect.TypeFor`, drop the slice alias in
`denyAppAccessForUnknownFields`, and shorten two test comments and a
wrapped constructor call. No behavior change.
* role: Reuse access checking for v9 app decisions
The v9 default-deny check needs to know which individual role grants an
app. `roleGrantsApp` answered that by repeating the namespace and label
stages of `RoleSet` access checking inside `lib/srv/app`, where a second
implementation can drift from the original. Replace it with
`services.RoleGrantsResource`, which delegates to `RoleSet.checkAccess`
and follows the existing single-role idiom in
`lib/services/access_checker.go`.
Delegating changes one case. `resourceRequiresLabelMatching` exempts the
Identity Center app subkind from app label matching, so `checkAccess`
treats every role as granting such an app and v9 denies requests that
`roleGrantsApp` allowed. Add `isPlainHTTPApp` to skip enforcement for
that subkind alongside the cloud and LLM types, which is the scope v9 is
documented to cover, and log once per user and app when an app reaches
the check that should have been routed elsewhere. Drop the unused error
return from `decideMinimalV9`.
Remove `CheckAppResourcesKnownFields`. The property it enforced, that a
role never stores an `app_resources` rule with a field this version does
not understand, already holds on the write path, where `UnmarshalRole`
empties such a rule and `validateAppResources` then rejects it for no
longer setting `allow_all` alone. Its only caller was a test helper for
the access plugins, so assert the write path directly instead.
Describe `validateAppResources` by the property it guarantees rather than
by each check it runs, so the godoc does not drift as the checks change.
Explain why the role stream downgrades per client version, and say
explicitly that both versions in the `maybeDowngradeRole` ordering
comment are the client's, which read ambiguously as the server's. Cut the
role v9 reference section to the field it introduces and a link, and move
the rules, the mixed-version role behavior, and the agent upgrade it
requires to a new Application Resources reference page.
* docs: Use a defined tag on the app resources page
The application-access tag is not in the docs tags vocabulary, so the
docs build rejected the new page. Use privileged-access, which the
sibling roles reference page already carries.
* terraform: Regenerate the schema with protoc-gen-terraform v4
Master upgraded the generator to v4.0.0, which marks list-nested
attributes computed and preserves unknown elements by index. Regenerate
so the app_resources attributes match what the pinned generator emits.
|
||
|
|
2c92419557 |
appresource: Compile and evaluate where clauses (#68796)
* appresource: Compile and evaluate where clauses Create the leaf package `lib/appresource` with the predicate language for HTTP app access. Carry over the where environment from the sketch branch `julia/app/policy-matcher-sketch`. That environment covers the `user.name`, `user.roles`, `user.traits`, and `request.method` bindings, the `true` and `false` constants, and the list and string helpers (`set`, `contains`, `lower`, `upper`, `has_prefix`, `has_suffix`, `has_substring`). Export `CompileWhere` and `Where.Evaluate` as the entry point, so a caller can compile a where clause and match it against a request and a caller identity. Keep the parser unexported. Name the function for the clause it compiles, because a follow-up PR adds a second entry point for the desugared expression form. Pass the evaluation environment as one `Env` struct, so the `vars.` capture namespace can be added as a new field without changing the `Evaluate` signature. Validate the environment before evaluation and reject any method outside the canonical upper-case HTTP method list, so a case-varied method can neither slip past a negated comparison nor match a clause while the backend receives a different spelling. Aggregate `trace.BadParameter` with the parse error in `CompileWhere`, matching the sibling path tokenizer, so a caller can tell a malformed clause in a role from an internal fault without losing the parse error chain. Report no match when evaluation fails, so a caller that drops the error denies rather than allows. Bind `true` and `false` explicitly, because `typical` has no boolean literal, so a bare literal would otherwise fail as an unknown identifier. Leave the `allow_code` and `deny_hint` audit wrappers to the PR that adds the rule types. Both wrappers record against the rule that matched, and checking that a wrapper sits where its record can be attributed correctly needs the rule types to define what one rule's expression is. Compile clauses over identity and method only. Leave path matching and the `vars.` capture namespace to follow-up PRs as well. * appresource: Use net/http method constants Replace the hand-written method strings in `validMethods` with the `net/http` constants, so the list pins the canonical spellings instead of repeating them by hand. * appresource: Add property-based where clause tests Add rapid property tests over generated clauses and environments. The properties are that `CompileWhere` never panics on arbitrary input, that a clause and its negation never agree, that any method outside the canonical list is denied for every clause, that `contains` over `set` matches `slices.Contains`, and that evaluation is deterministic. * appresource: Address review comments Add `NewEnv` so the canonical HTTP method check runs once at the request boundary. A `where` clause is optional on a rule, so a rule with only `methods` and `paths` never builds a `Where` and never reached the check, which meant adding a no-op `where: "true"` to an otherwise identical rule changed the answer for a request spelled `get`. `Evaluate` repeats the check, so an environment a caller assembled itself still cannot authorize such a request. Rename `validateEnv` to `validateMethod`, because the request method is the only thing it inspects. Record why `CompileWhere` builds an aggregate rather than wrapping. The aggregate classifies the result as `BadParameter` for the caller and keeps `typical`'s typed error for `errors.As`, and `Parse` does not return a `BadParameter` for every failure. Drop the claim that `contains` matches the role where-clause function of the same name. `predicate.Contains` type-asserts its first argument to `[]string` and returns false for a string, while `typical` coerces a string to a one-element list, so the same clause text disagrees across the two languages. State that a trait key the identity does not have reads as an empty list, since a mistyped key under a negation then matches every caller. State that the `Evaluate` result is only meaningful when the error is nil, and that sharing an `Env` across evaluations requires the caller not to mutate its slices or map. Cover the clauses that type-check to something other than a bool, operator precedence, and an identity with no traits. Widen the `strconv.Quote` round-trip generator to arbitrary strings, since the restricted alphabet never exercised the escaping the property is about. Rename the tests that did not name the behaviour they check, and check that `CompileWhere` never returns a `Where` with a nil expression. * appresource: Address review feedback on tests Draw a well-formed clause for half the cases in `TestCompileReturnsValueOrError`. Arbitrary strings essentially never parse, so the success branch never ran and the test asserted nothing. Measured 0 successful compiles in 20000 draws of `rapid.String()`, against 2496 in 5000 draws of the mixed generator. Assert that a compile failure returns no `Where`, which no other test checks, since `TestInvalidWhereClause` discards the value. Keep the generator choice in a variable so a well-formed clause that fails to compile is a test failure rather than an early return. Drop `TestEvaluateDeterministic`. Evaluation holds no state and no map iteration reaches the result, so the property cannot fail. The cached parser path it exercised is already reached by the other property tests, which compile the same clause more than once. Use `identifierText` directly rather than aliasing it per call, now that the generator is built once for the package. Say why the two assertions after the `TestEnvBindings` table sit outside it, since they cover a non-GET method and a nil traits map that the fixed request and identity cannot express. Check the error class in `TestUnsupportedMethodRejected` as `TestNewEnv` already does. |
||
|
|
81c8b8250f |
app: Log expired app sessions at debug level (#68918)
* app: Log expired app sessions at debug level Downgrade the log emitted when an app session cannot be fetched from warning to debug for the common case where the session is missing, expired, or invalid. A client that keeps replaying a stale session cookie, for example a browser tab that polls in the background after the session TTL elapses, previously produced a warning with a full stack trace on every request and flooded the proxy logs. Reserve the warning for unexpected failures such as backend connection problems. Also remove the duplicate log that `authenticate` and `renewSession` emitted on top of the one in `getAppSession`, so a failed lookup is logged at most once. * fixup! app: Log expired app sessions at debug level x * app: Cover session log levels in tests The prior change lowered expected app-session lookup failures to debug level but left the classification thinly tested. Assert that a missing cookie stays at debug, that an unexpected backend error still logs at warning, and route the buffer swap through a helper that restores the handler's logger so a later subtest does not silently inherit the warning-level logger. Trim the `getAppSession` comment to drop the sentence that only restated the else branch. * app: Address review on session log change Simplify `extractCookie` per review by ignoring the error from `(*http.Request).Cookie`, whose only returned error is `http.ErrNoCookie`, and checking `rawCookie == nil` for the not-found case. Print the cookie name with `%+q` so any non-ASCII bytes are escaped. Drop the added session-log tests. This only lowers log verbosity and preserves behavior, and the existing app authentication tests already cover the paths involved. |
||
|
|
f798c19ac4 |
lib/utils/typical: Add function combinators (#68570)
Add three parser-combinator definitions to the `typical` expression language: - `NullaryFunction` defines functions that take no arguments (zero-argument counterpart of the existing `UnaryFunction`). - `TernaryFunctionWithEnv` defines three-argument functions that receive the evaluation env (env variant of the existing `TernaryFunction`). - `BinaryVariadicFunctionWithEnv` defines variadic functions with one fixed argument that also receive the env (env variant of the existing `BinaryVariadicFunction`). The upcoming fine-grained app access predicate language (RFD 0303) needs these forms to expose helpers that read from the evaluation env and to define constants that take no arguments. Make the argument-parse error messages uniform across all combinators. The non-variadic combinators wrapped parse errors with "parsing ... argument to (name)", while the variadic combinators and every evaluation-phase message already used "parsing ... to function (name)". Align them all on the "to function" form, and correct "1 or more argument" to the plural. This rewords a few pre-existing messages, so a label-expression test assertion is updated to match. |
||
|
|
da6575c877 |
app: Reject duplicate effective public_addr in static config (#66824)
* app: Reject duplicate effective public_addr in static config Extend the existing duplicate-name check in applyAppsConfig to also reject static apps that resolve to the same effective routing FQDN. Fail fast at startup rather than dispatch non-deterministically via servers[rand.N(len(servers))] in lib/web/app/match.go. Compute the effective FQDN per app as: - public_addr, normalized through idna.ToASCII + trim trailing dot + lowercase to match services.ValidateApp's proxy-collision compare, - name + proxy_public_addr (one entry per proxy public addr) when public_addr is empty, or when use_any_proxy_public_addr is true (matching utils.AssembleAppFQDN), - name + cluster_name as a fallback when proxy_service is enabled but proxy_public_addr is unset (matching proxyDNSName in lib/web/apps.go). Limit the check to static app_service.apps. Dynamic apps are out of scope; defer best-effort detection to a follow-up RFD. * app: Address review feedback on static FQDN dedupe Mirror runtime registration when computing default app FQDNs. `FindPublicAddr` in `lib/srv/app/watcher.go` uses an IP-valued proxy `public_addr` as-is and falls back to `cluster_name` only when the proxy `public_addr` is unset; the validator previously skipped IPs and fell back to `cluster_name` whenever a proxy `public_addr` looked like an IP, which let a YAML config slip through that would still collide at registration. Extract `utils.DefaultAppFQDN(appName, proxyHost, clusterName)` as the shared source of truth for default FQDN derivation. Both the static config validator (`lib/config`) and `FindPublicAddr` call it so the two paths stay in sync. Ignore the local `proxy_service` config when it is disabled. `FindPublicAddr` reads registered proxies from the backend, so a disabled local proxy contributes nothing at runtime; the validator was previously treating leftover `cfg.Proxy.PublicAddrs` entries as effective routing suffixes and could fail startup on a false duplicate FQDN when the apps would route through a different proxy. Make a malformed proxy `public_addr` or `cluster_name` non-fatal during validation. Enabling `app_service` should not block startup on an address that no app depends on. Skip the malformed entry, log a warning, and let the dedupe pass continue; a real collision still errors. Sort per-app FQDN iteration before checking the seen-set so the error message is stable when one app has several effective FQDNs that each collide with a previously seen app. Go map iteration is random and the prior code picked whichever order Go produced. Clarify the comment about `CheckAndSetDefaults`: it validates `app.Name` and `app.PublicAddr` as DNS-1123 forms, it does not transform them. The dedupe step relies on the input already being lowercase ASCII without a trailing dot. * app: Trim duplicate-FQDN docs and simplify sort Cut the FQDN-derivation and multi-proxy implementation detail from the duplicate-routing docs so users only see that colliding FQDNs are ambiguous and Teleport rejects them when `proxy_service` is colocated. Replace the collect-then-sort pair with `slices.Sorted`. |
||
|
|
18c407ca6d |
discovery: Fix watcher deadlock on group reassign (#67098)
Fix a self-deadlock in `(*Server).startDynamicWatcherUpdater`. When the watcher receives an `OpPut` event for an already-tracked `DiscoveryConfig` whose `DiscoveryGroup` no longer matches the local server, the `OpPut` handler acquires `dynamicDiscoveryConfigMu.RLock()` for the existence check, never releases it on the fall-through path, then calls `dynamicDiscoveryConfigMu.Lock()`. Go's `sync.RWMutex` does not allow a same-goroutine upgrade from read to write, so `Lock()` blocks forever. The dynamic-config watcher goroutine wedges and no further `DiscoveryConfig` events are processed until the process is restarted. Mirror the sibling `OpDelete` handler: take `RLock`, read, `RUnlock`, then branch on the membership flag, then acquire the write lock. Also drop a duplicate "part" in the adjacent comment. Add `TestDiscoveryServer_dynamicMatcherGroupReassign`, which drives an `OpPut` event for an already-tracked config with a non-matching group, then sends an unrelated event and asserts forward progress. Without the fix the test times out at 30s with the watcher goroutine blocked on `Lock()`. |
||
|
|
005a2aa755 |
app: Consolidate app name and public_addr validation (#65728)
* app: Consolidate app name and public_addr validation Consolidate app `name` and `public_addr` validation to fix six issues where Teleport rejects valid app names, accepts invalid ones, or crashes on startup. Preserve backwards compatibility for dynamic and heartbeat paths: clusters with mixed-case records already in the backend keep working. Strict write validation (admin writes + static config): - App name, static: `IsDNS1123Label` - no dots, max 63 chars. - App name, dynamic: `IsDNS1123Subdomain` - dots OK for AWS-OIDC, max 253 chars. - `public_addr`: `IsDNS1123Subdomain` - dots OK (it is a hostname), max 253 chars. All three k8s validators also enforce lowercase. RFC 1123 itself does not -- lowercase is a Teleport convention for interop with Kubernetes object names. `ValidateApp` is the single write-time validator, called from `CreateApp`, `UpdateApp`, `UpsertApplicationServer`, the inventory control stream, and app service startup. Heartbeats run `NormalizeAppServerForHeartbeat` first, which lowercases both `name` and `public_addr` and strips URL schemes and ports, so heartbeats from older agents end up stored lowercase. Link: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/ * app: Apply review feedback Address review nits from the PR: - Use t.Context() in new tests; add t.Cleanup for client Close. - Drop double Close() in inventory controller tests. - Use proto getters in identitycenter to avoid panic on nil Spec. - Replace multi-hyphen regex with strings.ReplaceAll loop. - Collapse mockRolesAnywhereClient.profiles into pages. - Retitle and flesh out the CHANGELOG entry. - Revert accidental enterprise submodule bump. * app: Apply review feedback Use proto getters for the remaining `acct.Spec.*` accesses in `IdentityCenterAccountToAppServer` so the function stays nil-safe end to end. Fix grammar in the app name doc bullet ("as is a duplicate name" -> "as are duplicate names"). * app: Preserve Identity Center launch URL Identity Center accounts surface in the unified-resource cache as synthetic AppServers and never traverse `ValidateApp` / `ValidateAppServer`, so the DNS-1123 normalization the previous commits introduced in `IdentityCenterAccountToAppServer` had no write path to satisfy. Keep the raw StartUrl in both `URI` and `PublicAddr`: the web Launch button builds the SSO launch href as `${publicAddr}&role_name=...`, so stripping scheme, path, port, or case breaks every Identity Center app launch. Drop the matching lowercase on `metadata.Name` for the same reason, and restore the delete handler in `processEventsAndUpdateCurrent` to its pre-PR shape (rebuild a `KindAppServer` header from the IC account event, no lowercase). Rewrite `TestIdentityCenterAccountToAppServer` to assert StartUrl passes through verbatim; remove the `ValidateApp` round-trip assertion, which was checking a path the synthetic AppServer never takes in real callers. * app: Apply review feedback * app: Apply review feedback Tighten the "Stricter application validation" CHANGELOG entry to focus on the three backwards-incompatible cases: static-config DNS label, unrecoverable public_addr, and duplicate names within one agent's teleport.yaml. Expand the write-path list to include the Terraform provider, Kubernetes operator, and direct API calls. Note that multi-agent same-name heartbeats remain supported for load balancing. * app: Allow underscores in dynamic app names Swap IsDNS1123Subdomain for IsDNS1123SubdomainWithUnderscore in ValidateApp, the required_apps loop, and ValidateAppServer. The strict variant rejected snake_case names that Teleport Cloud and self-hosted clusters both accept today, breaking the Terraform provider tests as the canary. Static yaml (lib/service/servicecfg/app.go) remains strict, since the rule there is a deliberate breaking change. public_addr also stays strict, since it is the actual hostname used for routing. Document the new rule and the strict-TLS-client caveat in the "Application name" section of connecting-apps.mdx. |
||
|
|
38bb2a94d9 |
app: Raise upstream response-header cap to 1h (#66647)
The app service caps how long it waits for an upstream HTTP response to start arriving via http.Transport.ResponseHeaderTimeout. The cap was set to 5 minutes, which kills legitimate long-running requests (slow POSTs, long-poll handshakes, delayed first bytes) even though the upstream is healthy and actively processing. Relax the cap to 1 hour rather than removing it: the app service deliberately leaves ReadTimeout and WriteTimeout unset on its inbound http.Server, has no MaxConnsPerHost on outbound, and no listener-side limiter, so this transport-level cap is the only Teleport-side bound on per-request lifetime. Raising it to 1 hour gives long-running workloads room to finish while still reaping wedged upstreams. Pick up a separate latent fix while in the area: extend utils.CanExplainNetworkError so HTTP/2 response-header timeouts also surface the friendly diagnostic page. HTTP/1 timeouts already match the context.DeadlineExceeded case via timeoutError.Is. HTTP/2's http2httpError does not wrap context.DeadlineExceeded; it implements net.Error with Timeout() returning true. Combine both shapes into one case, also catching syscall.ETIMEDOUT and other net.Error timeouts. Use testing/synctest for the transport-level regression tests so the multi-minute simulated waits are instant in wall-clock terms. Inject a net.Pipe via tr.DialContext to keep real socket I/O out of the synctest bubble. |
||
|
|
5218e32338 |
app: Fix stale comment in initApps (#66610)
Rewrite the early-return comment in `initApps`. Drop the incorrect claim that omitting `app_service` enables it. The call site is already gated by `cfg.Apps.Enabled`, which only flips to `true` inside `applyAppsConfig` when the file config explicitly opts in. `Apps` has no `defaultEnabled` override, unlike `Auth`, `Proxy`, `SSH`, and `Debug`. Describe instead what the early return actually guards: return when `app_service` is enabled but has no apps, debug app, MCP demo server, or resource matchers configured. |
||
|
|
e5814ddbc3 |
vnet: Stop re-applying utun alias every 10s on macOS (#66416)
* vnet: Skip ifconfig and route on darwin when state matches Before this fix, on macOS the VNet OS-configuration loop re-issued `ifconfig` and `route add` for the TUN interface on every 10-second tick, even when the desired state was unchanged. macOS implements the underlying `SIOCSIFADDR` as a delete-then-add, so each tick emitted an `RTM_DELADDR`/`RTM_NEWADDR` pair on the kernel route socket. Other linkmon subscribers, notably Tailscale's magicsock embedded in the Coder client, read that pair as a major link change, tore down their DERP relay and WireGuard handshake, and reconnected. When Coder traffic was routed through `utun4`, the reconnect cost showed up as multi-second latency spikes on inflight pings, recurring on the 10-second cadence. Track the configured IPv4, IPv6, and CIDR ranges in `platformOSConfigState` on darwin so the next tick skips re-applying unchanged values. linux and windows already do this. Verified locally on the same AU Mac client, same Coder workspace (GCP us-central1), same Config D (Coder traffic over `utun4`). Pre-fix is run 4 from the upstream investigation; post-fix is the same configuration with this PR applied: | metric `coder ping`| base (HTTP/no-VNet) | pre-fix (TCP/VNet) | post-fix (TCP/VNet) | |---|---|---|---| | min | 355 ms | 356 ms | 362 ms | | avg | 361 ms | 473 ms (+112) | 384 ms (+23) | | max | 394 ms | **2,692 ms** | **577 ms** | | stddev | 8 ms | 141 ms | **1.46 ms** | | spikes >= 1s in 180 pings | 0 | **7** | **0** | | `RTM_DELADDR`/`NEWADDR` pairs per 270s | 0 | **28** | **0** | * vnet: Tighten godoc and inline comment in osconfig_darwin Drop redundant words from the platformOSConfigState godoc and the inline comment above the deconfigure branch. The struct context and field names already imply "applied" and "OS-mutating", and the position marker "here" is unnecessary on a comment whose location already indicates the spot. * vnet: Align configuredIPv6 field name with linux Rename `configuredIPv6Alias` to `configuredIPv6` so the IPv4 / IPv6 / CIDR-list field names match `osconfig_linux.go`. The darwin-only `configuredIPv6Route` flag stays because the alias and route are gated separately on darwin: a transient `route add -inet6` failure must retry the route on the next tick without re-running ifconfig (which would flap the alias). |
||
|
|
4c1d77ba22 |
app: Preserve URL fragment through auth redirect (#65738)
* app: Preserve URL fragment through auth redirect Capture the hash from useLocation() in the AppLauncher component and route it through the URL fragment of every hop in the app access auth redirect flow. On the first leg, set the auth-exchange URL's hash so the browser carries the fragment back through the proxy 302 to the second leg. On the second leg, pack the original fragment alongside the existing session-cookie value in the URL hash so the inline JS in lib/web/app/redirect.go can parse both and reattach the original fragment to the final navigation. The fragment never reaches the proxy as a query parameter, keeping sensitive values such as OAuth implicit-flow access tokens or password-reset tokens out of proxy access logs. Drop the fragment when a required-apps chain is in play. The proxy 302 to the next app's launcher would inherit the fragment across origins, exposing the originally requested app's fragment to intermediate apps. Gate fragment forwarding on requiredApps.length <= 1 in the launcher on both legs, and have the inline JS in redirect.go drop the fragment on the chain branch as well. Chain-redirected apps lose the original fragment as a result. Issue: https://github.com/gravitational/teleport/issues/38653 * app: Address review feedback on fragment-redirect Refactor the inline JS in redirect.go so the success path builds a single redirectUrl from path || '/' and calls window.location.replace once. Resolve relative to currentOrigin and fall back to the origin root when the path crosses origins (e.g. a protocol-relative "//attacker.com/foo"). Attach the original fragment last so encoding goes through URL.hash. Same end-state for browsers (https://app vs https://app/), one place to read the fragment-handling contract. Trim the comments around the fragment-handling code. Drop RFC 9110 references that did not point at sections explicitly addressing fragment behaviour, the explanations of mechanics that any half- decent frontend reader already knows (URL fragment is client-side, window.location.hash carries a leading '#'), and the OAuth / password-reset specifics of what kind of value the original fragment may carry. Lead the wire-format comment with "encoded as URLSearchParams" so the custom mechanism is named up front. * app: Preserve URL fragment when launching while logged out Stash the URL fragment in sessionStorage before the launcher redirects to the web login page so the launcher can reattach it after login. Without this, the fragment is lost before the login page renders: `goToLogin` builds `redirect_uri` from `pathname + search` only, dropping the hash, and the subsequent JS-driven navigation to `/web/login` does not inherit fragments the way an HTTP redirect does (RFC 9110 § 15.4 applies to server- driven redirects, not client-side navigations). Use sessionStorage rather than passing the fragment through `redirect_uri`. A query parameter would carry the fragment value into proxy access logs (`/web/login?redirect_uri=...`, `/enterprise/saml-idp/sso?redirect=...`), defeating the security property that motivated this PR's logged-in path: fragment values such as OAuth implicit-flow `#access_token=...` and password-reset tokens never reach the server. sessionStorage keeps the value client-side and per-tab, and survives same-tab navigations through the login flow including SSO IdP bounces. Skip the stash on a required-apps chain. Forwarding a fragment across origins would expose values meant for the originally requested app to every intermediate app's domain, which is the same rule the launcher enforces on the logged-in path. The launcher reads the stashed value via a `useState` initializer so the read-and-clear runs exactly once per launcher mount, and the URL fragment from `useLocation()` continues to take precedence when both are present. |
||
|
|
199cd79bec |
app: Clean up orphaned app servers on startup (#64029)
* app: Clean up orphaned app servers on startup Delete stale app server heartbeat records on startup so that removed apps disappear immediately instead of lingering until TTL expiry. After a SIGHUP reload, the parent process exits with deleteResources=false (it has a forked child), and the child only heartbeats its own apps. Add a cleanup step that lists all app server records for this host and deletes any that do not correspond to a currently registered app. For agents with dynamic apps (ResourceMatchers), wait for the first successful reconciliation cycle before running cleanup so dynamic app records are not mistakenly deleted. Skip integration-managed app servers (e.g. AWS OIDC) that share the host UUID in all-in-one deployments. * app: Improve cleanupOrphanedAppServers comments Move the high-level explanation of why orphan cleanup exists into the method godoc, and trim the inline comment to cover only the timeout rationale. * app: Address review feedback Replace "15 minutes" with `[apidefaults.ServerAnnounceTTL]` in the cleanupOrphanedAppServers doc comment for accuracy (the TTL is 10 minutes, not 15). Drop the unused HostUUID field from suiteConfig. |
||
|
|
6c8ea32bc2 |
app: Add active sessions Prometheus gauge (#65008)
* fncache: Pass non-cancellable context to `OnExpiry` callbacks Call `OnExpiry` when `get()` replaces an expired entry on reload. Previously, entries that expired between cleanup intervals were silently dropped when a new request triggered a reload, skipping the `OnExpiry` callback entirely. Use `context.WithoutCancel(c.cfg.Context)` at both the `removeExpiredLocked` and `get` call sites so that `OnExpiry` work completes even after shutdown begins. `Shutdown` cancels `c.cfg.Context` via `c.cancel()` before processing entries, but these two call sites passed `c.cfg.Context` directly, meaning goroutines spawned by `OnExpiry` could run with an already-cancelled context. Update the `OnExpiry` doc comment to warn that the cache mutex may be held when the callback is invoked. * app: Add active sessions Prometheus gauge Add a `teleport_app_active_sessions` gauge labeled by app name that tracks HTTP app sessions on each agent. The gauge increments when a session chunk is created and decrements after the session chunk finishes closing, so it reflects sessions still holding resources (audit streams, disk I/O) rather than just sessions accepting new requests. TCP and MCP sessions are excluded because they bypass the session chunk cache. * fncache: Fix `OnExpires` typo in `Shutdown` doc comment Correct the stale field name `OnExpires` to `OnExpiry` in the `Shutdown` method's doc comment to match the actual field name in `FnCacheConfig`. |
||
|
|
cf76490d2a |
app: Preserve trailing slash in URI redirect (#65739)
* app: Preserve trailing slash in app URI redirect path.Clean strips trailing slashes, so a configured URI like http://backend:9000/dashboard/ produces a redirect to /dashboard instead of /dashboard/. The upstream returns 404 because it expects the trailing slash. Preserve the trailing slash from the original URI after cleaning by re-appending it when the administrator's URI ended with one. Add TestNeedsPathRedirect to cover the redirect path logic. * app: Assert empty location on no-redirect paths Add an else branch to TestNeedsPathRedirect that verifies the returned location string is empty when no redirect is expected. This guards against future regressions where needsPathRedirect might return a non-empty string alongside ok=false. |
||
|
|
40dfc718e4 |
app: Fix context leak in handleConnection (#65631)
* app: Fix context leak in handleConnection Move context creation from handleConnection to its caller HandleConnection, which unconditionally defers cancel(nil). Pass ctx and cancel as parameters to handleConnection and remove the cancel calls that were previously scattered across the TCP, MCP, and HTTP branches. Six error return paths in handleConnection created a child context via context.WithCancelCause but returned without calling cancel, leaving the child registered in the parent's children map until server shutdown. Under connection churn this caused monotonic memory growth. * app: Add comments clarifying context ownership and handler paths Clarify that HandleConnection owns the context and that TrackingReadConn may call cancel first with io.EOF. Also clarify that TCP and MCP handlers block until the session is done, while the HTTP handler returns immediately after handing the conn to http.Server. |
||
|
|
aa58a7296b |
app: Broadcast ready events for unconfigured services (#63628)
* app: Broadcast ready events for unconfigured services Broadcast `AppsReady` when `app_service` is enabled but has no static apps, debug app, MCP demo server, or resource matchers configured. Without the broadcast, `TeleportReadyEvent` is never emitted, blocking internal operations that wait on it (rotation sync, AWS OIDC deploy service updater). Apply the same fix to `initKubernetes`: broadcast `KubernetesReady` and transition the component to `stateOK` via `OnHeartbeat` when the kube service is enabled but the cluster license does not include Kubernetes access. Use `Info` level instead of `Warn` for the apps log message because `app_service` is implicitly enabled when the config section is omitted, which is the common case for deployments that do not use app access. * app: Fix test cleanup order and instanceRoles init Swap t.Cleanup registration order so supervisor.signalExit runs before stor.Close (Go LIFO), preventing a theoretical window where the register.kube goroutine accesses closed storage. Initialize instanceRoles in the test struct literal to suppress a spurious ERROR log from the "unexpected instance role" guard in RegisterWithAuthServer. * app: Cancel supervisor closeContext in test cleanup Call `supervisor.Wait()` in test cleanup to cancel `closeContext` and stop the `fanOut()` goroutine started by `NewSupervisor`. `signalExit()` alone only cancels `exitContext`, which is a sibling context, leaving `fanOut()` blocked on `closeContext.Done()`. |
||
|
|
e0908117e8 |
app: Fix ARN encoding in web launcher URL (#64827)
* app: Fix ARN encoding in web launcher redirect URL When an AWS console app role ARN contains a `/` in the path component (e.g. `arn:aws:iam::123:role/my-role`), the web launcher redirect truncates the ARN at the slash. This is a regression from the react-router v7 upgrade (PR #63496). Both v5 and v7 encode `/` as `%2F` in the URL path via `generatePath`. The difference is in `useParams()`: v5 returned the raw percent-encoded value, v7 auto-decodes it. In v5, `useParams().arn` was still percent-encoded (e.g. `role%2Fmy-role`). `getNewAuthExchangeUrl` put that encoded value into the query string, so Go's `q.Get("arn")` decoded one layer and got `role%2Fmy-role` (literal `%2F`, not a slash). `path.Join` treated `%2F` as ordinary characters and the full ARN survived. In v7, `useParams().arn` is auto-decoded to `role/my-role` (literal `/`). `getNewAuthExchangeUrl` puts that decoded value into the query string, Go's `q.Get("arn")` gets a literal `/`, and `path.Join` in `makeAppRedirectURL` splits the ARN at that slash. The redirect URL carries a truncated ARN, the session cert is issued with the truncated value, and the access check rejects the mismatch. Percent-encode the ARN with `url.PathEscape` before appending it to the URL path, and replace `path.Join` with `strings.Join` plus explicit `RawPath`/`Path` setting so that `%2F` survives URL serialization. * app: Percent-encode all path segments in launcher redirect Escape every segment in the redirect URL path, not just the ARN. This prevents any unexpected percent sequences in cluster names or public addresses from being emitted raw in the Location header. |
||
|
|
8f7cb166ba |
limiter: Short-circuit when no rates or connections are configured (#63751)
* limiter: Add benchmarks for noop vs active rate limiting
Add micro-benchmarks comparing the cost of each limiter path when no
limits are configured (noop) against a high default rate (equivalent
to the old 100M req/s fake rate). Cover all call sites:
RegisterRequest, HTTP middleware, RegisterRequestAndConnection, and
both gRPC interceptors (unary and stream).
Also add a macro-benchmark (BenchmarkHTTPServer) that measures
end-to-end HTTP throughput through a real TCP listener with concurrent
clients to show how the limiter's overhead compares to network I/O.
Before the short-circuit fix, NoRates and HighDefaultRate produce
identical results because `NewRateLimiter` injects the fake default:
BenchmarkRegisterRequest/NoRates 298 ns/op 64 B/op 3 allocs/op
BenchmarkRegisterRequest/HighDefaultRate 299 ns/op 64 B/op 3 allocs/op
BenchmarkHTTPMiddleware/NoLimits 405 ns/op 264 B/op 7 allocs/op
BenchmarkHTTPMiddleware/HighDefaultRate 406 ns/op 264 B/op 7 allocs/op
BenchmarkRegisterRequestAndConnection/NoLimits 320 ns/op 96 B/op 4 allocs/op
BenchmarkRegisterRequestAndConnection/HighDefaultRate 321 ns/op 96 B/op 4 allocs/op
BenchmarkUnaryInterceptor/NoLimits 320 ns/op 64 B/op 3 allocs/op
BenchmarkUnaryInterceptor/HighDefaultRate 321 ns/op 64 B/op 3 allocs/op
BenchmarkStreamInterceptor/NoLimits 338 ns/op 96 B/op 4 allocs/op
BenchmarkStreamInterceptor/HighDefaultRate 339 ns/op 96 B/op 4 allocs/op
BenchmarkHTTPServer/NoLimits 9770 ns/op 4752 B/op 57 allocs/op
BenchmarkHTTPServer/HighDefaultRate 9700 ns/op 4752 B/op 57 allocs/op
Apple M4 Pro, go1.26.1 darwin/arm64.
* limiter: Short-circuit when no rates or connections are configured
Every Teleport service creates a Limiter unconditionally, even when
no `connection_limits` are set. Previously, `NewRateLimiter` injected
a fake default rate of 100M req/s to satisfy a non-empty-rates
invariant in `TokenLimiter`. Every request still paid the full cost:
extract client IP, acquire mutex, FnCache lookup, and token bucket
consume - all for a rate that never rejects.
Remove the fake default rate and add early returns throughout the
limiter stack: `RateLimiter.RegisterRequest` returns immediately when
the effective rate set is empty, `TokenLimiter.ServeHTTP` skips IP
extraction and rate consumption, and `ConnectionsLimiter` skips
tracking when `maxConnections` is zero.
Micro-benchmarks on Apple M4 Pro show the noop path is 5-160x faster
in isolation (NoLimits = noop after fix, HighDefaultRate = old
behavior with fake 100M rate):
BenchmarkRegisterRequest/NoRates 1.85 ns/op 0 B/op 0 allocs/op
BenchmarkRegisterRequest/HighDefaultRate 306 ns/op 64 B/op 3 allocs/op
BenchmarkHTTPMiddleware/NoLimits 82.0 ns/op 208 B/op 4 allocs/op
BenchmarkHTTPMiddleware/HighDefaultRate 405 ns/op 264 B/op 7 allocs/op
BenchmarkRegisterRequestAndConnection/NoLimits 18.8 ns/op 32 B/op 1 allocs/op
BenchmarkRegisterRequestAndConnection/HighDefaultRate 323 ns/op 96 B/op 4 allocs/op
BenchmarkUnaryInterceptor/NoLimits 18.5 ns/op 0 B/op 0 allocs/op
BenchmarkUnaryInterceptor/HighDefaultRate 322 ns/op 64 B/op 3 allocs/op
BenchmarkStreamInterceptor/NoLimits 33.2 ns/op 32 B/op 1 allocs/op
BenchmarkStreamInterceptor/HighDefaultRate 337 ns/op 96 B/op 4 allocs/op
However, a macro-benchmark through a real TCP HTTP server shows no
measurable difference in end-to-end throughput. The limiter's ~300ns
is noise against the ~9500ns of TCP/HTTP overhead per request:
BenchmarkHTTPServer/NoLimits 9510 ns/op 4681 B/op 54 allocs/op
BenchmarkHTTPServer/HighDefaultRate 9600 ns/op 4752 B/op 57 allocs/op
This change is primarily about code clarity - remove the fake default
rate workaround and make the limiter honestly do nothing when no
limits are configured - rather than a meaningful throughput
improvement.
* limiter: Clarify FnCache default TTL comment
Reword the comment to explain that the default TTL only controls
the cleanup sweep interval, since all entries are inserted via
FnCacheGetWithTTL with their own TTL.
* limiter: Remove stale custom-rate code from rebase
Remove the `customRate` parameter and `RegisterRequestWithCustomRate`
that were incorrectly carried forward during the rebase. PR #64828
already removed custom rates from `RegisterRequest` on master by
giving each concern its own `RateLimiter` instance. Revert all
callers to the single-arg `RegisterRequest` signature.
* limiter: Guard against nil next handler in TokenLimiter.ServeHTTP
Add a nil check for `tl.next` before the noop short-circuit in
`ServeHTTP`. Previously `New()` returned an error when no rates
were configured, so a nil-next `TokenLimiter` could never reach
`ServeHTTP`. Now that `New()` succeeds with an empty `RateSet`,
the nil-next path is reachable. Mirror the guard used by
`ConnectionsLimiter.ServeHTTP`.
|
||
|
|
5503cd391c |
app: Use constant-time comparison for bearer token validation (#65204)
Replace direct string comparison with `checkSubjectToken`, which uses `subtle.ConstantTimeCompare`. The other code path in `auth.go` already used constant-time comparison via the same function. Preserve the audit event on mismatch. |
||
|
|
4df912cce9 |
limiter: Fix custom rate bucket clobbering (#64828)
* limiter: Add failing test for custom rate bucket clobbering Add an assertion to TestCustomRate that proves the bug: when a default-rate RegisterRequest call interleaves with custom-rate calls for the same client IP, the custom-rate bucket is clobbered because both share a single TokenBucketSet keyed by IP alone. The test fails on master at limiter_test.go:145. The next commit fixes the bug. * limiter: Use dedicated RateLimiter for account recovery Replace the shared-bucket custom rate approach with a separate RateLimiter instance for account recovery RPCs. This follows the same pattern as gravitational/teleport#64559 and avoids the shared-bucket clobber problem entirely: each concern gets its own RateLimiter with its own FnCache, so default-rate requests cannot reset stricter per-endpoint buckets. The `getCustomRate` middleware passed per-endpoint rate sets through a shared FnCache keyed only by client IP. When a default-rate request hit the same IP, `TokenBucketSet.Update` overwrote the cached bucket with the default rate parameters, resetting the stricter custom-rate state. Add a private `accountRecoveryLimiter` to the auth Middleware and wire it into `rateLimitUnaryInterceptor`, which applies the default limiter to all endpoints and the recovery limiter on top. Remove `getCustomRate`, `CustomRateFunc`, `UnaryServerInterceptorWithCustomRate`, and the `customRate` parameter from `RateLimiter.RegisterRequest` and `RegisterRequestFromAddr`. Export `ClientIPFromContext` from the limiter package for use by the middleware. Keep a deprecated `Limiter.RegisterRequestWithCustomRate` shim for backwards compatibility with enterprise callers (they all pass nil). * limiter: Remove stale accountRecoveryEndpoints comment Remove orphaned comment from the deleted `accountRecoveryEndpoints` variable. The comment was left behind when the variable was replaced by inline switch cases in `rateLimitUnaryInterceptor`. * limiter: Guard nil accountRecoveryLimiter in interceptor The accountRecoveryLimiter is nil when Middleware is constructed without calling newAccountRecoveryLimiter, which happens in initSecureGRPCServer for Kube-only traffic. The nil dereference is not reachable on current code paths because recovery RPCs are never routed to that server, but add a nil guard to make the interceptor safe independently of how the server is wired. * limiter: Remove RegisterRequestWithCustomRate Remove the backwards-compatibility shim now that all enterprise callers have been removed via gravitational/teleport.e#8359. * limiter: Extract ClientIPFromAddr to lib/utils Move the addr-to-IP parsing into `utils.ClientIPFromAddr` (next to `ClientIPFromConn`) so `lib/auth` does not depend on `lib/limiter` for IP extraction. Add an unexported `clientIPFromContext` in `lib/auth` for the gRPC peer lookup. Reduce the public API surface of `lib/limiter` that deals with gRPC concepts, keeping it closer to its intended role as a general-purpose rate limiter keyed by string tokens. |
||
|
|
9acfd17e9b |
e-ref: Bump enterprise submodule ref (#65114)
Unblock PR gravitational/teleport#64828 (limiter: Fix custom rate bucket clobbering), which depends on PR gravitational/teleport.e#8359 (devicetrust: Remove RegisterRequestWithCustomRate, merge commit `9a74e57`). |
||
|
|
132781d654 |
testplan: Fix moderated session instructions (#64704)
Change `--role` to `--mode` in the `tsh join` command, which is the correct flag name. Change `Ctrl+C` to `Ctrl+D` for the user terminal checkbox, since `Ctrl+D` (EOF) is what ends a shell session cleanly, while `Ctrl+C` sends SIGINT and does not end the session. |
||
|
|
6481d1912e |
docs: Clarify proxy public_addr for app access (#63865)
* docs: Clarify proxy public_addr role in app access redirects Update the `proxy-service.yaml` config reference comment for `public_addr` to note it is used for app access auth redirects and falls back to the cluster name if not set. Companion to https://github.com/gravitational/teleport/pull/63868 which adds the cluster-name fallback. * docs: Expand public_addr comment per review Explain the redirect flow instead of using the shorthand "app access auth redirects", which is not a standard phrase in the docs. * docs: Specify Teleport Proxy Service in config comment Clarify that "proxy" refers to the Teleport Proxy Service, not the Application Service, in the `public_addr` config reference comment. |
||
|
|
043a0a9356 |
app: Fall back to cluster name for auth redirect (#63868)
* app: Fall back to cluster name for auth redirect `redirectToLauncher()` hard-errors when `proxy_service.public_addr` is not set, even though the proxy knows its own identity via the cluster name. In practice this means browser app access fails unless the admin also sets the proxy-level address, which is not obvious from the docs. Fall back to the cluster name when `ProxyPublicAddrs` is empty in three places: `redirectToLauncher`, `APIHandler.ServeHTTP` (so `HasName` can route app requests), and the required-app redirect in `completeAppAuthExchange`. Derive the port from the incoming request (`r.Host`) rather than the internal listen address (`ProxyWebAddr`) so the redirect is correct even when a load balancer or reverse proxy translates ports (e.g. external :443 to internal :3080). Run the app/proxy address conflict check against the cluster-name fallback too. Remove the now-dead `WebPublicAddr` field and the `GetProxySettings` call that only existed to populate it. Downgrade the startup log from WARN to INFO since app access now works without explicit `proxy_service.public_addr`. * app: Guard ServeHTTP fallback with subdomain check |
||
|
|
344d500df8 |
app: Fix flaky TestAuthorizeWithLocks (#64030)
* app: Fix flaky TestAuthorizeWithLocks Subscribe to the lock watcher before calling `UpsertLock` and wait for the matching `OpPut` event before making the HTTP request. This eliminates the race where the request arrives before the watcher has processed the lock, causing an unexpected 200 instead of 403. The fix follows the subscribe-then-wait pattern from `lib/authz/permissions_test.go` (`upsertLockWithPutEvent`) rather than wrapping the HTTP assertion in `require.Eventually`. The `Eventually` approach would also fix the flake, but `checkHTTPResponse` takes `*testing.T` (not the `require.TestingT` interface) and creates a new pipe and goroutine per attempt - wasteful for what is really a synchronisation gap, not an assertion timing issue. Subscribing to the watcher addresses the root cause deterministically. Store the `lockWatcher` on the `Suite` struct so the test can subscribe to it. Also add error checking on `UpsertLock` and `DeleteLock` calls that were previously silent. * Address review feedback * app: Address review feedback on lock test Replace `goto ready` with a labeled `break loop` per codebase convention (only one `goto` exists today, in build assets). Drop the artificial 2s timeout and let the Go test harness handle timeouts instead. |
||
|
|
9021c526c2 |
app: Add connection limiter support to app service (#63720)
* app: Add connection limiter support to app service The app service is the only Teleport service without rate limiter support. Every other service (SSH, Auth, Proxy, Database, Kube, Windows Desktop) already has a `limiter.Config` field populated from `connection_limits` in the file config. Wire the limiter through the same path: add `Limiter` to `AppsConfig`, include it in the `limiters` slice in `configuration.go`, and pass it into `ConnectionsHandlerConfig`. Apply the limiter in `handleConnection` - the shared entry point for all app protocols - rather than in the HTTP middleware chain. This ensures HTTP, TCP, and MCP connections all honour `connection_limits`. Follow the database service pattern: call `RegisterRequestAndConnection` with the client IP before protocol dispatch, and release the slot when the connection closes. Skip limiting when the client IP cannot be extracted (e.g. `net.Pipe` connections used by integration app proxying) rather than rejecting the connection. For HTTP, transfer the release into the cleanup function that runs after the connection is served. For TCP and MCP, release on handler return via defer. Create the limiter unconditionally even when no `connection_limits` are configured (the default), to stay consistent with how every other Teleport service handles it. Move `go c.expireSessions()` to after all fallible initialization to avoid leaking the goroutine on error. Add a v19 breaking-change entry to the changelog and update the config reference to list Application alongside the other services. * Address review feedback * app: Address review feedback on limiter PR Clarify that the 15,000 connection cap is per `app_service` instance in the changelog entry. Replace the two-variable `release`/`releaseOnReturn` pattern in `handleConnection` with a single `var release func()`. Nil the variable when transferring ownership to the cleanup function so the deferred release becomes a no-op. Remove the unused error return from `newHTTPServer` - no code path returns an error. Replace the two unit tests that only exercised `lib/limiter` in isolation with four tests that verify `handleConnection` correctly wires the limiter: max connections, release-then-reconnect, rate limiting, and net.Pipe skip behaviour. Fix YAML indentation in the instance-wide config reference from 4-space to the standard 2-space. * app: Clarify connection limit scope in changelog Spell out all three dimensions of the 15K default: per client IP, per `app_service` instance, aggregated across all apps on that instance. * app: Use real limit in PipeSkipsLimiter test Configure `MaxConnections: 1` and pre-fill the limiter so a normal connection would be rejected. This verifies that net.Pipe connections actually bypass the limiter rather than passing vacuously because `MaxConnections: 0` disables limiting entirely. * Fix go lint error * app: Wire limiter config to proxy-side integration app handler The proxy-side `IntegrationAppHandler` creates a `ConnectionsHandler` for integration apps (cloud apps like AWS console) that run directly on the proxy without a separate app service. Wire `cfg.Apps.Limiter` through to this callsite so it enforces the same per-IP connection limits as the regular app service handler. * app: Rename `held` to `release` in limiter tests Match the variable name used in the production code and in the `RegisterRequestAndConnection` godoc example. * Address review feedback |
||
|
|
fc8b556d9f |
app: Warn when proxy public_addr is missing (#63683)
* app: Warn when proxy public_addr is missing When app_service is enabled and proxy_service has no public_addr, app FQDNs silently fall back to the cluster name, producing URLs that don't resolve. Log a warning at startup so admins know to set proxy_service.public_addr. * app: Skip warning when all apps set their own public_addr Address review feedback: apps can set public_addr individually, bypassing proxy_service.public_addr entirely. Only warn when at least one app lacks its own public_addr, and list affected app names in the log message. * app: Warn when proxy public_addr is missing app_service does not work without proxy_service.public_addr. Log a warning at startup in applyAppsConfig so operators see the problem before users hit the runtime error. Simplify the check to a single condition on proxy_service.public_addr rather than inspecting individual app public_addr fields - the proxy address is required regardless |
||
|
|
22ca85d293 |
discovery: Log KMS access denied errors as warning (#59167)
Log KMS access denied errors as warning, rather than returning an error, as this features too prominently in the web UI and it is quite common that KMS keys have strict policy for accessing tags or key policies even if they can be listed. |
||
|
|
3a7897bb4c |
docs: Document KMS key discovery in Identity Security AWS Sync (#58640)
Add KMS Keys to the list of discovered AWS resources and document the required IAM permissions (ListKeys, DescribeKey, ListResourceTags, ListAliases, GetKeyPolicy) for the Access Graph integration. Follows up on #56874 which added the KMS discovery implementation. |
||
|
|
565947d136 |
access-graph: Add AWS KMS key discovery to access graph (#56874)
* access-graph: Add KMS resource protos Add the `AWSKMSKeyV1` protobuf messages to support importing KMS key into the access graph. This new type is added to the `AWSResource` oneof. The Go bindings were regenerated using `make grpc`. * access-graph: Add AWS KMS key discovery to access graph Implement AWS KMS keys discovery and ingest into access graph. The fetcher discovers keys across all specified regions. For each KMS key, the fetcher retrieves the following details: * Key metadata, including ARN, creation date, and HSM cluster ID * Resource tags * Key aliases * The key policy document Add a mock KMS client to support comprehensive unit testing of the new fetcher, covering various key configurations and error cases. The new KMS key resource is integrated into the existing discovery framework, including the merge, deduplication, and reconciliation steps. The required IAM policy for access graph discovery is updated to include the necessary `kms:` permissions. Correct typo in the `GlueTableName` field name. * Address review feedback * Address second set of review feedback * Regenerate go-bindings for proto * Fix golden files with new AWS KMS actions |
||
|
|
e1fd4913e9 | Fix ypo in okta integration docs (#55795) | ||
|
|
780e36beb1 |
[docs] Add hcl language marker in the code blocks (#55656)
Add `hcl` language marker in the code blocks so that the terraform code in the newly added
identity activity center docs is properly rendered.
FWIW, if code blocks like the following were rendered correctly with file
contents inlines in preview mode this would have probably been picked up
earlier, no idea what it would take to make this happen though:
```hcl
(!examples/identity-activity-center/policy.tf!)
```
Related-pull-request: https://github.com/gravitational/teleport/pull/55519
|
||
|
|
42e369b8ef |
docs: Add missing quotes to variables.auto.tfvars (#55579)
Add missing quotes to variables.auto.tfvars in Identity Activity Center
setup instructions.
This is to fix resulting terroform error:
│ on variables.auto.tfvars line 1:
│ 1: aws_region = us-west-2
│
│ Variables may not be used here.
|
||
|
|
c7c0a0dbf2 |
exporter: Remove clockwork Clock (#55414)
Remove clockwork Clock which was intended to be used in teleport-enterprise tests, but it turnsout Go's new testing/synctest package is sufficient for our needs here. |
||
|
|
6e562902ef |
Test batch exporting in BulkExporter (#55160)
Use clockwork to make BulkExporter more testable from implementations in teleport-ent. Use existing test infrastructure to also test BatchExport in DateExporter. |
||
|
|
60892cfc84 |
[iac] Implement AWS CloudTrail log ingestion for Access Graph (#54989)
* [iac] Implement AWS CloudTrail log ingestion for Access Graph
Ingest AWS CloudTrail logs into Access Graph. Poll SQS queues for
notifications of new CloudTrail log files in S3, download these files,
and stream their raw contents to the Access Graph service via a new
gRPC stream.
Add configuration to enable SQS-based AWS CloudTrail log polling for
Access Graph AWS sync matchers. Specify SQS queue URL and region
for notifications.
In `lib/srv/discovery/access_graph_aws.go`:
- Orchestrate CloudTrail log ingestion using a new `startCloudtrailPoller` function.
- Ensure singleton poller execution with a semaphore lock (`access_graph_aws_cloudtrail_sync`).
- Stream log data to Access Graph via a new `AWSCloudTrailStream` gRPC stream.
- Exchange initial configuration and resume state with the server.
- Dynamically manage polling routines per AWS matcher using a `GenericReconciler`.
- Poll SQS queues for CloudTrail S3 object notifications in `pollEventsFromSQSFiles`.
- Download raw CloudTrail log files from S3.
- Stream S3 file payloads and AWS Account ID (from STS) to Access Graph.
- Delete SQS messages post-transmission.
- Filter AWS sync configurations for CloudTrail polling.
- Increase accessGraphClient gRPC message size limits to 50MB.
Update Content Security Policy in `lib/httplib/httpheaders.go`:
- Add `blob:` to `script-src`.
- Add `https://api.anthropic.com` and `https://*.amazonaws.com` to `connect-src`.
Improve concurrency safety in `lib/srv/discovery/discovery.go` by adding an RWMutex
to protect the `dynamicDiscoveryConfig` map.
I did not write the code of this PR. It has been rebased and pulled over
from the iac PoC (see #54626).
We should still add tests.
Related-pull-request: https://github.com/gravitational/teleport/pull/54626
Co-authored-by: Tiago Silva <[email protected]>
Co-authored-by: Matt Brock <[email protected]>
* add tests
* Fix golangci-lint errors
* fix naming
---------
Co-authored-by: Tiago Silva <[email protected]>
Co-authored-by: Matt Brock <[email protected]>
|
||
|
|
cd9cfb3756 |
Implement batched audit log export utilities (#54684)
* Fix typo * Implement audit log exporting utilities * Improve batch export |
||
|
|
f9e5f7ed50 |
Add protos for Identity Activity Center (#54653)
* protos: Add protos for Identity Activity Center Add new and extend protos for the new Identity Activity Center feature in Access Graph. Identity centers around activities of user, allows to easily correlate log items by identity target or type and proactively provides alerts such as impossible travel. * Fix according to review Fix according to review comments. |
||
|
|
a5ee4a1489 |
github: Fix Proto compatibility check for non-release base branches (#53653)
* Fix broken breaking check * Fix spellcheck linting Fix spellcheck linting as we have new spelling mistakes that only surfaced once lint.yaml was updated. * Update .github/workflows/lint.yaml Co-authored-by: Edoardo Spadolini <[email protected]> --------- Co-authored-by: Edoardo Spadolini <[email protected]> |