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

Update built dependency to 0.7.1 #452

Merged
merged 8 commits into from
Feb 12, 2024
Merged

Update built dependency to 0.7.1 #452

merged 8 commits into from
Feb 12, 2024

Conversation

soenkeliebau
Copy link
Member

Description

Requires some small changes to the code, as the Option struct has been removed in favor of feature flags.

fixes #451

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

Reviewer

Acceptance

soenkeliebau and others added 2 commits February 12, 2024 10:27
Requires some small changes to the code, as the Option struct has been removed in favor of feature flags.
nightkr
nightkr previously approved these changes Feb 12, 2024
Cargo.toml Outdated
@@ -27,5 +27,8 @@ strum = { version = "0.25", features = ["derive"] }
tokio = { version = "1.29", features = ["full"] }
tracing = "0.1"

[patch.crates-io]
built = { git = "https://github.com/stackabletech/built.git", branch = "fix/dont-write-env" }
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to submit a PR for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have opened lukaslueg/built#62 to see if they'd be willing to consider one, but I would say in principle we would like to do that, yes.

@soenkeliebau
Copy link
Member Author

Integration test still running, but looks okay so far I think ..

https://ci.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/hbase-operator-it-custom/139/

@soenkeliebau
Copy link
Member Author

Test was successful

nightkr
nightkr previously approved these changes Feb 12, 2024
@soenkeliebau soenkeliebau added this pull request to the merge queue Feb 12, 2024
Merged via the queue into main with commit f6c1fbf Feb 12, 2024
30 checks passed
@soenkeliebau soenkeliebau deleted the fix/RUSTSEC-2024-0013 branch February 12, 2024 14:39
@lfrancke
Copy link
Member

I'm not sure I like this.
What does the SBOM for this look like? We now need to maintain this ourselves.

I want us to think twice before moving to a fork of something.

@lfrancke
Copy link
Member

In fact: I think I'm in favor of reverting this.
We have a few more weeks to go and even then we can live with this vulnerability as it's nothing an end-user can control.

@soenkeliebau
Copy link
Member Author

I am sure that I don't like it, but @nightkr and I discussed this and it seemed the "least bad" option that allows us to get rid of the RUSTSEC in our stack right now.

We can investigate moving of built onto another crate or wait if there is an appetite for our fix upstream. Both of which will take time which this workaround could buy us.

I am also happy to revert and wait for feedback on our issue upstream.

soenkeliebau added a commit that referenced this pull request Feb 12, 2024
@lfrancke
Copy link
Member

I would favor reverting just so everything is consistent but we can also keep it in and wait for upstream.
I don't want to maintain a fork. I think I'd rather figure out where the code is used and disable that part of built or instead basically say (in VEX terms) that the code cannot be controlled by users (which is the case as git should only be used during a github action run - which we control - to figure out the current commit/tag etc.)
So, I think we are not affected by this and I don't want to maintain a fork for something we don't need.

In other words: We can get rid of the RUSTSEC by just saying it's not valid here.

@soenkeliebau
Copy link
Member Author

#453

@soenkeliebau
Copy link
Member Author

soenkeliebau commented Feb 12, 2024

I think I'd rather figure out where the code is used and disable that part of built

That is exactly what the changed code in our fork does. The issue is, that upstream removed the ability to disable that code.

@soenkeliebau
Copy link
Member Author

Actually, we only use built as a build dependency, so the code is not even present in our shipped artifacts, so in VEX terms we can even say "vulnerable code not present".

github-merge-queue bot pushed a commit that referenced this pull request Feb 12, 2024
@NickLarsenNZ
Copy link
Member

Actually, we only use built as a build dependency, so the code is not even present in our shipped artifacts, so in VEX terms we can even say "vulnerable code not present".

Perhaps it is a non-issue in this case, but in general, even a build-time vulnerability can impact built artifacts. Just wanted to point it out so we don't get too comfortable ignoring build-time vulns.

@soenkeliebau
Copy link
Member Author

Very true, and hopefully at some point it'll go away when we can upgrade again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

RUSTSEC-2024-0013: Memory corruption, denial of service, and arbitrary code execution in libgit2
4 participants