Builds on https://github.com/gravitational/teleport/pull/39974 to
prefer writing a single string instead of a json object to logs. This
more closely follows the behavior of the text formatter which
will output strings. The primary motivation is to ensure that no
secrets or unwanted items of a struct make it into json logs because
it is converting an object differently than the text handler.
There are two other options to avoid this, but both rely on doing
the right thing when adding an attribute, which may be overlooked or
forgotten and ultimately not noticed because only text ouput was
tested with.
1) Call .String() directly on the attribute, but this goes against
the performance guidance in the slog docs.
2) Wrap the attribute in a logutils.StringerAttr which follows the
recomendations in the slog docs:
> If you want to avoid eagerly paying the cost of the String call
> without causing the handler to potentially inspect the structure
> of the value, wrap the value in a fmt.Stringer implementation
> that hides its Marshal methods.
This change is a change in how we are handling the untrusted input. Instead of adopting a strategy of quoting (which was incomplete), this instead escapes any special characters.
Although github.com/google/safetext is new, and without any current tags for us to reference, I believe it would be better to reference their implementation than to update the UnixShellQuote to handle the newly identified cases.
The default slog json handler attempts to format attributes with
json.Marshal instead of a string representation, which resulted in
errors being output as json objects instead of just the message.
```
"error":{"error":{"message":"Unable to communicate with the Teleport Auth Service"}}
```
In order to restore json output to contain only the error message
instead of the entire object special handling is done in our slog
handler wrapper that replaces the error object with the message.
The existing resource parser was converted to use typical under
the hood. Aside from the type safety improvements the making the
switch allows a single predicate expression to be parsed once and
evaluated per resource. The previous parser worked backwards, it
created a parser per resource with the same expression which lead
to elevated memory consumption and poor performance.
An additional bug was fixed that prevented predicate filtering from
working properly with the unified resource API due to applying
predicates prior to filtering by kind. This resulted in queriers
for server specific things (resource.spec.hostname) to fail even
when only the server resource was specified in the resource kinds.
Benchamrk results for the changes in this commit compared to master
show a 75% reduction in the test time and a 96% reduction in allocations
during the tests.
```bash
$ benchstat e77e5e080b1848da830a9030d43c38be55a5b8d5.txt 8eff752a93dfe96225a71431af829015e83400fa.txt
goos: darwin
goarch: arm64
pkg: github.com/gravitational/teleport/lib/auth
│ e77e5e080b1848da830a9030d43c38be55a5b8d5.txt │ 8eff752a93dfe96225a71431af829015e83400fa.txt │
│ sec/op │ sec/op vs base │
ListUnifiedResourcesFilter/labels-12 73.88m ± 1% 64.74m ± 6% -12.37% (p=0.000 n=10)
ListUnifiedResourcesFilter/predicate_path-12 294.06m ± 20% 74.83m ± 5% -74.55% (p=0.000 n=10)
ListUnifiedResourcesFilter/predicate_index-12 307.63m ± 4% 76.32m ± 4% -75.19% (p=0.000 n=10)
ListUnifiedResourcesFilter/search_lower-12 130.7m ± 5% 130.5m ± 4% ~ (p=0.971 n=10)
ListUnifiedResourcesFilter/search_upper-12 147.4m ± 5% 138.7m ± 2% -5.87% (p=0.003 n=10)
ListUnifiedResources/simple_labels-12 1.748 ± 2% 1.744 ± 2% ~ (p=0.631 n=10)
geomean 246.6m 150.6m -38.93%
│ e77e5e080b1848da830a9030d43c38be55a5b8d5.txt │ 8eff752a93dfe96225a71431af829015e83400fa.txt │
│ B/op │ B/op vs base │
ListUnifiedResourcesFilter/labels-12 735.2Ki ± 0% 724.6Ki ± 1% -1.44% (p=0.000 n=10)
ListUnifiedResourcesFilter/predicate_path-12 238.873Mi ± 0% 7.585Mi ± 0% -96.82% (p=0.000 n=10)
ListUnifiedResourcesFilter/predicate_index-12 238.869Mi ± 0% 7.587Mi ± 0% -96.82% (p=0.000 n=10)
ListUnifiedResourcesFilter/search_lower-12 16.77Mi ± 0% 16.77Mi ± 0% ~ (p=0.436 n=10)
ListUnifiedResourcesFilter/search_upper-12 16.78Mi ± 0% 16.76Mi ± 0% ~ (p=0.052 n=10)
ListUnifiedResources/simple_labels-12 1.272Gi ± 0% 1.273Gi ± 0% ~ (p=0.739 n=10)
geomean 49.67Mi 15.69Mi -68.41%
│ e77e5e080b1848da830a9030d43c38be55a5b8d5.txt │ 8eff752a93dfe96225a71431af829015e83400fa.txt │
│ allocs/op │ allocs/op vs base │
ListUnifiedResourcesFilter/labels-12 11.45k ± 0% 11.41k ± 0% -0.37% (p=0.000 n=10)
ListUnifiedResourcesFilter/predicate_path-12 4962.0k ± 0% 161.5k ± 0% -96.75% (p=0.000 n=10)
ListUnifiedResourcesFilter/predicate_index-12 4812.0k ± 0% 161.5k ± 0% -96.64% (p=0.000 n=10)
ListUnifiedResourcesFilter/search_lower-12 161.6k ± 0% 161.6k ± 0% ~ (p=0.419 n=10)
ListUnifiedResourcesFilter/search_upper-12 161.7k ± 0% 161.6k ± 0% ~ (p=0.116 n=10)
ListUnifiedResources/simple_labels-12 23.70M ± 0% 23.70M ± 0% +0.01% (p=0.019 n=10)
geomean 743.7k 238.5k -67.93%
```
Provides an opt-in mechanism to allow parsers to support dynamically
resolving variables at runtime. While this defeats some of the
static compile type benefits that typical guarantees, it is required
to allow existing parsers to be converted to use typical.
The existing resource parsers allow searching for variabls by their
fully qualified path. For example, the hostname of a server may
be quiered via `resource.spec.hostname`. In order to support this
typical could use some reflection to automatically add variables
to every nested field but that would be complicated and expensive.
Instead the ParserSpec can now be configured with
`GetUnknownIdentifier`, which allows creators of the parsers to
have full control over how a dynamic variable is resolved. The
parser prevents any dynamic resolution if the value is not configured
to limit the unsafe behvior to only parsers that explicitly opt in.
* AutoDiscover: add support for EICE nodes
* ignore openssh nodes when updating labels from serverinfo
* downcase imports
* improve comment on why we need account/instance and subkind
* add applyovertime method
* skip nodes that wont expire in the near future
* create helper functions
* Quote user supplied inputs provided to scripts to avoid RCE
This change introduces the func `utils.UnixShellQuote` which will quote any inputs which could potentially allow execution or script escape.
This is utilized to ensure that scripts produced from a potential Phishing link could not contain code execution which may expose a user.
* awsAccessGraphOIDCSync: Ensure role parameter is quoted correctly
* join_tokens: Move shell quote to `getJoinScript` rather than where parameters are extracted
This will increase safety moving forward, but it requires a more conservative quoting strategy.
* Make slog attributes output the same content regardless of format
There were a few instances of fmt.Stringers being passed to slog
as attributes to delay their evaluation as per the performance
considerations of slog: https://pkg.go.dev/log/slog#hdr-Performance_considerations.
However, this resulted in the json handler emitting different
content since they check to see if attributes are json.Marshalers
and not fmt.Stringers. To rectify the situation, a new slog.LogValuer
implementation was added that wraps a fmt.Stringer so that attributes
can be lazily constructed while producing the same output no matter
the log output format.
* fix: wrap identity and correct typo
* Bump github.com/gravitational/trace from 1.3.1 to 1.4.0
## Changelog
- Bump google.golang.org/grpc from 1.43.0 to 1.53.0 by @dependabot in #94
- Bump golang.org/x/net from 0.8.0 to 0.17.0 by @dependabot in #100
- Bump google.golang.org/grpc from 1.53.0 to 1.56.3 by @dependabot in #101
- Bump golang.org/x/crypto from 0.14.0 to 0.17.0 by @dependabot in #105
## Breaking Changes
- Remove logrus dependency by @rosstimothy in #107
- Remove references to UDP logrus hook from README by @rosstimothy in #108
Full Changelog: https://github.com/gravitational/trace/compare/v1.3.1...v1.4.0
* remove final references to removed trace functionality
* fix: formatter error handling
It was discovered that some customers' EKS clusters did not have their IMDSv2 hop limits
set correctly, causing requests for key functionality to attempt IMDSv1 fallback and failing.
For now, re-enable IMDSv1 fallback by way of removing the explicit disabling of
`EC2MetadataEnableFallback` until better documentation, error handling, and other work
can be done to inform customers that they need to correctly set their IMDSv2 hop limits.
* Remove logging from circuit breaker
The logging is never output, and if it was would be very verbose.
Instead of dropping the level down to trace the logger is removed
entirely.
* Add constants from trace library
The logging functionality in trace is being removed in an upcoming
release. To prepare the logging component keys are copied from trace
into teleport.
* Convert trace.ComponentFields to teleport.ComponentFields
* Replace trace.Component with teleport.ComponentKey
* Replace trace logging constants
* Extends SyncMap
Adds some useful utility methods to the the SyncMap class. The
motivating example for these additional methods is the Okta reconciler,
which is currently using several map+mutex pairs.
* Update sync_map.go
* kube: Return errors when users aren't allowed to list resources
Previously to this commit, if the user was denied to list resources from
a certain namespace, Teleport would return an empty list instead of
returning an error if it's impossible for the request to match any
resource in that namespace.
This commit returns an error for cases where the role's
`kubernetes_resources` denies any resource to be returned. This is a
small nit to incorporate some customer expectations.
Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
* Update lib/utils/replace.go
Co-authored-by: Hugo Shaka <hugo.hervieux@goteleport.com>
* handle code review suggestions
* Update lib/utils/replace.go
Co-authored-by: Anton Miniailo <anton@goteleport.com>
* fix test names
---------
Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
Co-authored-by: Hugo Shaka <hugo.hervieux@goteleport.com>
Co-authored-by: Anton Miniailo <anton@goteleport.com>
* Fix TestReadEnvironmentFile on macOS
This test writes sample data to a temporary file and then tries to
parse it. In #34177 we disallowed reading the environment file from
a symlink, but the Go utilities we use to create temp files end up
using symlinks on macOS.
Fix this by breaking out the core functionality such that it only
requires an io.Reader instead of an os.File.
* Use consistent language in log output
Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
---------
Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
* chore: Bump golangci-lint to v1.56.0
* Use only enable linters for testifylint
* Fix various testifylint offenders
* Fix testifylint offenders in api/
Adds the new linter and configures it to prevent direct error
comparisions and type assertions in favor of errors.Is and errors.As.
All current violations caught by the linter have been updated
accordingly.
* Move AssembleAppFQDN to lib/utils
* Update comment for endpoint_uri
* Fix comment for TotalCount, move AppServerOrSAMLIdPServiceProvider
* Remove unnecessary libclient import
* Add fqdn to apps returned from tsh daemon
* Drop "Server" from AppServerOrSAMLIdPServiceProvider, add Go doc
* Rerun make grpc
If you mistakenly point `cert_file` at a private key
instead of a cert, you would say a "no PEM data found"
error. This is misleading, as there is PEM data, it is
just not the correct type.
The FnCacheConfig now provides an optional OnExpires callback which
will be called when any item in the cache is removed due to expiry.
Eviction of expired entries used to only happen on retrieval
attempts, but this can now be triggered on demand using
(FnCache) RemoveExpired. A new Shutdown method was also added to
allow all items in the cache to be removed. If OnExpires is set
then it will be called for every item in the cache during Shutdown.
When creating a reverse proxy the ErrorLog was being created
with the writer returned from logrus.WriterLevel. This writer
uses an io.Pipe and a goroutine to get any log messages written
from a standard log.Logger to our custom formated logrus.Entry.
The ErrorLog only ever omits output in a few very rare (hopefully)
error cases, which means we were creating an extra goroutine per
reverse proxy which in most cases would never be used to produce
any added output. Additionally, now that we are starting to support
log/slog, any output from a log.Logger will now be formatted via
our registered default slog handler. Meaning that we can still
get the benefits of producing output from the ErrorLog in the
desired format without the penalty incurred by using logrus.WriterLevel.
Partially addresses #36541.
In addition to configuring output format users can configure the
fields which are included in log messages. Prior to this our slog
handlers didn't support this feature and always output the default
fields. Now, both the slog and logrus output is identical no matter
the extra_fields configuration as verified by TestExtraFields. It
was also found that due to the way the logurs json formatter was
configuring itself in CheckAndSetDefaults the timestamp was always
included in the final output even if it was not included in the
configured extra_fields.
* Update slog handlers to support configured extra_fields
In addition to configuring output format users can configure the
fields which are included in log messages. Prior to this our slog
handlers didn't support this feature and always output the default
fields. Now, both the slog and logrus output is identical no matter
the extra_fields configuration as verified by TestExtraFields. It
was also found that due to the way the logurs json formatter was
configuring itself in CheckAndSetDefaults the timestamp was always
included in the final output even if it was not included in the
configured extra_fields.
* fix: update component comment
* Better control on user injected environment values
This commit includes two changes:
* In `environment` we expanded the list of MacOS environment values which should be filtered. It was demonstrated that these can be used to get code execution in MacOS.
* In `reexec` we no longer provide the cmdmsg.Environment for the `teleport exec`. As part of the development of a9055bccd9 it was attempted to fully clear out the environment, but testing showed that to be potentially problematic. It was believed the safest option was to use the cmd environment, however this introduces a new source of environment variables. Because this exec happens under `root` this is particularly dangerous (even more so when combined with the missed OSX values mentioned above). As such we now are only providing the filtered exec environment, which is a closer (but safer) option to the functionality prior to a9055bccd9.
* Update filter to exclude all DYLD_ and LD_ prefixed variables
Converts services.Reconciler to be generic over any type instead
of being confined to operating on a types.ResourceWithLabels. This
allows decoupling of the two since there is nothing specific to
resources required for the reconciler to operate.
Prefer the stdlib version of the experimental package.
Includes a depguard update to prevent future regressions,
as well as an e-ref update to appease depguard.
Generated with:
$ gofmt -w -r '"golang.org/x/exp/slices" -> "slices"' . && \
make fix-imports
The new `tsh latency` command allows visualizing latency to
resources in real time. Currently, supported is limited to SSH
instances, but may be extended to anything that implements a
`latency.Pinger` to support other protocols in the future.
Tests were failing if too much time had passed between the output
of each logger being generated. The InDelta assertion was supposed
to prevent this, but because it used nanoseconds the delta of 10
was always exceeded and resulted in errors like below.
```
Max difference between 1701993842000000000 and 1701993843000000000 allowed is 10, but difference was -1e+09
```
To avoid this the assertion is now using Unix instead of UnixNano
so that we allow up to 10 seconds of drift in the timestamps.
Monitors both the UI<->Proxy and Proxy<->SSH host connections in
order to provide users with near real time latency data for ssh
connections established via the UI. The client portion of the
connection is measured by how long it takes to receive a web socket
pong in response to sending a web socket ping. The host portion of
the connection is measured by how long it takes to receive a reply
of a keepalive@openssh.com global SSH request. The statistics are
periodically sent via a new envelope type over the web socket where
they are consumed and displayed to users.
CI is slow enough that the messages output from logrus and slog do
not occur at the same exact time. To account for the difference
the timestamp is parsed and validated to be close enough in time.
* Remove necessity of ServerWithRoles implementing ClientI
* Delete clear unnecessary code from ServerWithRoles that was stubby
* Remove unused CertAuthority CRUD methods from ServerWithRoles
* Remove uncalled User CRUD methods from ServerWithRoles
* Remove unused Unimplemented gRPC util
* Document `presenceForAPIServer`
Co-authored-by: Alan Parra <alan.parra@goteleport.com>
---------
Co-authored-by: Alan Parra <alan.parra@goteleport.com>
In all places that a logrus logger is being initialized an equivalent
slog logger is now being intialized as well. The TeleportProcess
was also updated to have a slog.Logger which is created with the
same fields that its logrus equivalent has. This does not convert
any logging to actually use slog yet, but does pave the way for
conversions to start.
Forrest