Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add function $redacted$() #24790

Closed
piotrrzysko opened this issue Jan 24, 2025 · 5 comments
Closed

Add function $redacted$() #24790

piotrrzysko opened this issue Jan 24, 2025 · 5 comments

Comments

@piotrrzysko
Copy link
Member

In #24563, we'd like to introduce the redaction of sensitive information in statements like CREATE CATALOG in query events and endpoints that expose query text (e.g., v1/query).

There is an idea to mask secrets by replacing them with a function call that always fails. For example, the redacted text of:

CREATE CATALOG catalogA USING postgresql WITH (
   "connection-url" = 'jdbc:postgresql://localhost:4000/trino',
   "connection-user" = 'admin',
   "connection-password" = '1234'
)

Would look like this:

CREATE CATALOG catalogA USING postgresql WITH (
   "connection-url" = 'jdbc:postgresql://localhost:4000/trino',
   "connection-user" = 'admin',
   "connection-password" = $redacted$()
)

This approach prevents users from copying the redacted query text and unintentionally creating a catalog with the password ***.

To summarize: we propose introducing the $redacted$() function, which always fails. I’m extracting this proposal into a separate GitHub issue because I believe it requires a syntax review.

cc: @dain @hashhar

@ksobolew
Copy link
Contributor

This approach prevents users from copying the redacted query text and unintentionally creating a catalog with the password ***.

Saving users from themselves - I like that :)

@wendigo
Copy link
Contributor

wendigo commented Jan 24, 2025

Can we instead use a function fail ? :)

trino> select fail('This is gonna be gud');
Query 20250124_135029_00000_simzd failed: This is gonna be gud

@wendigo
Copy link
Contributor

wendigo commented Jan 24, 2025

You can even do:

select fail(124, 'This is gonna be gud');

which will use 124 == INVALID_CATALOG_PROPERTY error code

@wendigo wendigo closed this as completed Jan 24, 2025
@piotrrzysko
Copy link
Member Author

I have a few concerns:

  • fail is a hidden function and does not seem to be intended for use in query text.
  • As a user, I would find it confusing to see something like the following in the logs:
    CREATE CATALOG catalogA USING postgresql WITH (  
       "connection-url" = 'jdbc:postgresql://localhost:4000/trino',  
       "connection-user" = 'admin',  
       "connection-password" = fail(124, '...')  
    )  
    This raises several questions:
    • What does this magic number mean?
    • What is the fail function, and why is it undocumented?

Even if we provide a clear message (e.g., "This is a redacted property"), why can't we use a function with a concise, self-explanatory name instead?

@wendigo
Copy link
Contributor

wendigo commented Jan 24, 2025

this is an error-code and it's optional, fail is hidden so it's not listed but it's used across a codebase so it won't be removed in the future.

wdyt @martint ?

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

No branches or pull requests

3 participants