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

Use snakeyaml to write configured features #16857

Closed

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Apr 3, 2023

Description

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Apr 3, 2023
@wendigo wendigo requested review from kokosing and nineinchnick April 3, 2023 15:19
@martint
Copy link
Member

martint commented Apr 3, 2023

Given the recent CVE in snakeyaml, I'm not sure this is something we want to do. Also, why introduce yet another dependency when Jackson can already do this?

@nineinchnick
Copy link
Member

@wendigo can you fill in the description and/or the commit message to say what problem this is solving and how?

@wendigo
Copy link
Contributor Author

wendigo commented Apr 3, 2023

@martint Jackson is delegating to snakeyaml afaik hence the change to remove indirection

@wendigo
Copy link
Contributor Author

wendigo commented Apr 3, 2023

Version 2.0 is the one that fixes this CVE. jackson still relies on the old version

@wendigo
Copy link
Contributor Author

wendigo commented Apr 3, 2023

@nineinchnick
Copy link
Member

See this FasterXML/jackson-dataformats-text#382
They say Jackson is not vulnerable, they'll update to SnakeYAML 2.0 in 2.15.0 which has a rc2 now.

@wendigo
Copy link
Contributor Author

wendigo commented Apr 3, 2023

If only security scanners could read that @nineinchnick ;)

@martint
Copy link
Member

martint commented Apr 3, 2023

Just to be clear, it's a goal to Trino to be secure, but it's not a goal to make security scanners happy.

@ct-starburst
Copy link

@wendigo the name of this PR had me thinking you were adding in more snakeyaml, which I think added some confusion.

Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is ok, but I still wonder why this shows up in security scanners. We should not be shipping the PTL in the final artifacts. Can you give more details which scanner picked it up and what exactly is getting flagged?

"configured_connectors",
configuredFeatures.asMap().getOrDefault(CONNECTOR, ImmutableList.of()),
"configured_password_authenticators",
configuredFeatures.asMap().getOrDefault(PASSWORD_AUTHENTICATOR, ImmutableList.of())))), writer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit: can you move the writer to the next line? It's hard to spot that yaml.dump() actually takes two arguments.

@kokosing
Copy link
Member

kokosing commented Apr 3, 2023

The change is ok, but I still wonder why this shows up in security scanners.

This library was in elastic and pinot (see #16842 and #16838). This change is only changing test tools to remove it snakeyaml so we can ban this dependency.

@wendigo
Copy link
Contributor Author

wendigo commented Apr 4, 2023

Closing. Snakeyaml always is generating anchors which is something that tempto does not like.

@wendigo wendigo closed this Apr 4, 2023
@wendigo wendigo deleted the serafin/use-snakeyaml-directly branch April 4, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants