GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — [connectors,types,manager] Keep connector credentials out of diagnostics by bestouff · Pull Request #7786 · feldera/feldera Conversation
Contributor
Linked IssuePart of feldera/cloud#2136 (connector credentials in errors, logs and support bundles). PR 1 of 3: stop the known emission sites. PR 2 adds a redaction net for resolved secret references, PR 3 scrubs stored diagnostics in the pipeline manager. A read-role user could read a connector password from One commit per site:
The helper commit adds Describe Manual Test Plan
Checklist
Breaking Changes?Mark if you think the answer is yes for any of these components:
Describe Incompatible ChangesError and log texts change: they no longer contain the connection URI, the rejected option value or the configuration dump. No configuration or API field changes.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page.
Contributor
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
bestouff
marked this pull request as draft
bestouff
force-pushed
the
cloud-2136-diagnostics
branch
4 times, most recently
from
d0c965f to
9cb42fc
Compare
bestouff
marked this pull request as ready for review
Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page.
Contributor
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
bestouff
force-pushed
the
cloud-2136-diagnostics
branch
from
9cb42fc to
3968c3f
Compare
Contributor
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
mihaibudiu
reviewed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean that this string is always exactly
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Always
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This I don't understand. Why carry the field if it's a constant?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what makes it "Safe"?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Safe" because it's constructed from its schema and name, not from the input. I'll expand on it.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
bestouff
force-pushed
the
cloud-2136-diagnostics
branch
2 times, most recently
from
1ad643a to
aaf46f9
Compare
mihaibudiu
approved these changes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these macros do the damndest things
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In tracing's
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't really tell where the TLS bit is coming from
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
| .unwrap_or_default(); | ||
| debug!("postgres {endpoint_name}: opening connection{target}"); | ||
|
|
||
| let connector = make_tls_connector(&config.tls, endpoint_name).map_err(|e| { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
this must be the bit. But why single it in the docs?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Because it is the only branch in the function: a TLS and a plain connect. The reworded comment says what decides it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| use super::*; | ||
| use feldera_types::transport::postgres::PostgresTlsConfig; | ||
|
|
||
| const PASSWORD: &str = "hunter2-do-not-print"; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
hm. Is this Hunter Biden?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Looks like you're one of the lucky 10000 today: https://knowyourmeme.com/memes/hunter2
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I learned 2 more things now.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| match error { | ||
| KafkaError::ClientConfig(_, description, key, value) => { | ||
| // librdkafka quotes the value in its description. The quoted | ||
| // token goes whatever its length; a bare copy of a longer value |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
goes whatever it's length?
can it go in any other way?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
so this replaces ALL values? Isn't this a little extreme?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Only one value is replaced: the value of the option that librdkafka rejected. No other option appears in this error. "Goes" means it is replaced. librdkafka quotes that value, so the quoted copy is always replaced. A simple copy is also replaced when it is 4 bytes or longer. A shorter one is skipped: with a value like on, replacing every bare on would mangle words like configuration. Will be rewritten.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| .map(|url| url_without_credentials(url)) | ||
| .collect(); | ||
| let header_names: Vec<&str> = registry_headers.keys().map(String::as_str).collect(); | ||
| f.debug_struct("AvroSchemaRegistryConfig") |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
this is maybe misleading, since it does not include all the information. should the output say that?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Credential fields already show [REDACTED], but the headers show as a bare list of names. I'll print them as {name: "[REDACTED]"}, so the output says that a value exists and is hidden.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| let properties = std::collections::BTreeMap::from([( | ||
| "connectors".to_string(), | ||
| PropertyValue { | ||
| value: value.to_string(), |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Do we have to do this in the compiler too?
Maybe one of your subsequent PRs does?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yes, probably.
I'll stack one more PR for the road.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
(kidding, it'll be 1 more commit: the parse error will no longer quote the rejected token)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| The following limits apply: | ||
|
|
||
| - A value that is part of SQL, UDF code, an endpoint name, or table data is |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
connector configuration is part of SQL, so that sounds wrong.
Moreover, compiler warnings may include connector configuration, which can be saved into bundles.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| oauth_provider, | ||
| } = self; | ||
| // Header values are user data and can be credentials too. | ||
| let header_keys: Vec<&str> = headers.iter().map(|h| h.key.as_str()).collect(); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
same question about this being incomplete
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| /// fields, the name of a missing or repeated field, and the position that | ||
| /// serde_json appends. Callers supply the endpoint or source position | ||
| /// separately. | ||
| pub fn deserialization_error(error: &impl std::fmt::Display) -> String { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
hard for me to figure out if this is missing important parterns...
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The patterns that won't be recognized will fail with "could not be parsed; check its syntax and documented options". So it can miss details but not leak a value.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
aaf46f9 to
ed84b9c
Compare
Connection URLs and deserializer messages can include credentials. Give callers helpers that remove URL userinfo, query strings and fragments, and describe configuration failures without copying rejected values or map keys. The latter keeps the error category and the schema-defined parts of serde's message, such as the expected values; callers supply endpoint identity and source positions separately. Test URL credentials, repeated URLs, rejected scalars and fields, and custom deserializer errors. The following commits use these helpers at the emission sites. Related: feldera/cloud#2136 Signed-off-by: Xavier Bestel <[email protected]>
The Postgres input connector put the resolved connection URI into its connect error and into two log lines. The URI holds the password, also when the user supplied it as a Kubernetes Secret reference, because the connector sees the resolved value. The error reached the pipeline log, deployment_error, the monitor events and the support bundle, where any user with read access to the pipeline could read the password. Show the hosts, ports and database instead, parsed with tokio_postgres::Config, and nothing when the URI does not parse. The endpoint name in the error already identifies the connector. Part of feldera/cloud#2136. Signed-off-by: Xavier Bestel <[email protected]>
The pipeline printed its whole configuration on startup, before it resolved secret references and before the logger was up. Inline connector options such as sasl.password, environment values and storage sync keys went to the pipeline log and from there to the support bundle. Print one line per endpoint with its stream, transport and format names, a reviewed list of runtime fields, the storage backend name and the environment variable names. A field that is not on the list stays out, so a new field that holds a secret is safe by default. Part of feldera/cloud#2136. Signed-off-by: Xavier Bestel <[email protected]>
A malformed connectors property was copied into the compile error. Deserializer reasons could also echo a rejected scalar containing an inline credential, even after the whole property was omitted. Preserve the relation, property, source position, and JSON line/column. Replace the property value with [REDACTED] and report a safe error category and corrective action. Test malformed JSON and rejected scalars in both displayed and serialized errors. Related: feldera/cloud#2136 Signed-off-by: Xavier Bestel <[email protected]>
The Delta input connector put the table URI into its errors. A URI can carry credentials in its userinfo or a signed-URL token in its query string. Strip both before formatting. A local path shows as is; a string that does not parse and holds either part shows as [REDACTED]. Part of feldera/cloud#2136. Signed-off-by: Xavier Bestel <[email protected]>
When librdkafka rejects a client option, rdkafka's error carries the
option's value and repeats it in the description. The connectors turned
that error into text with {e}, so a rejected sasl.password, inline or
resolved from a Secret reference, went to the pipeline log and to
deployment_error.
Map the error at every client construction: name the option, keep the
description with the value masked, drop the value.
Part of feldera/cloud#2136.
Signed-off-by: Xavier Bestel <[email protected]>
Format configurations can contain resolved secret values. Both the configuration dump and serde's rejected scalar or map key exposed those values through initialization errors. Short secrets also bypass the runtime registry, so the source must be safe independently. Keep the endpoint identity, error code, and serialized field types. Replace the configuration with [REDACTED] and describe the failure using a fixed category and corrective action. Test parser and encoder errors with rejected scalar values, fields, and enum variants. Related: feldera/cloud#2136 Signed-off-by: Xavier Bestel <[email protected]>
The Kafka input and output connectors, the Avro encoder and the
controller printed a connector's complete config with {:?} at debug
level. For Kafka and Avro this is the resolved config, so a password
supplied as a Kubernetes Secret reference was in the log as soon as a
pipeline author set runtime_config.logging to debug.
Print the endpoint, topic and transport names instead. Give the Kafka
configs and the Avro schema registry config a Debug implementation that
names the options and headers and hides their values, so a later {:?}
of these types stays safe.
Part of feldera/cloud#2136.
Signed-off-by: Xavier Bestel <[email protected]>
The pipeline reports a configuration file that does not deserialize with serde's message, which quotes the rejected token. A credential in a wrong-shaped field, such as a NATS `credentials` given as a plain string, is rejected here before any redaction can run, so the value reached the startup error and `deployment_error`. Keep the error category, the expected values and the line and column, and drop the token, with the helper the other configuration errors use. Related: feldera/cloud#2136 Signed-off-by: Xavier Bestel <[email protected]>
Jackson quotes an unrecognized token in full: a 'connectors' value like "password": hunter2 gave "Unrecognized token 'hunter2'". The token can be an inline credential, and the message is stored in the program error, so the value also reached the support bundle. A bundle collected without source code drops the SQL excerpt, but it kept this message. Drop the quoted token from the message. The source position and the excerpt still point at the bad text. Related: feldera/cloud#2136 Signed-off-by: Xavier Bestel <[email protected]>
ed84b9c to
469134e
Compare
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.