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

Resolve Docker CVEs #815

Merged
merged 10 commits into from
Oct 19, 2022
Merged

Conversation

tspearconquest
Copy link
Contributor

Fixes #802

Signed-off-by: Thomas Spear tspear@conquestcyber.com

…ed on alpine 3.15

Signed-off-by: Thomas Spear <tspear@conquestcyber.com>
@coveralls
Copy link

coveralls commented Sep 28, 2022

Coverage Status

Coverage remained the same at 57.91% when pulling bef6ac6 on tspearconquest:fix_docker_cves into 03e74b9 on golang-migrate:master.

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Could you also rebase from master to fix the lint issues?

Dockerfile Show resolved Hide resolved
Thomas Spear added 3 commits October 6, 2022 20:16
Signed-off-by: Thomas Spear <tspear@conquestcyber.com>
Signed-off-by: Thomas Spear <tspear@conquestcyber.com>
@JanBez
Copy link

JanBez commented Oct 10, 2022

Hi, thank you for fixing the issue, it is blocking me as well. Do you plan to merge it soon? I would appreciate it :-)

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Sorry for the slow turn around time!

go.mod Show resolved Hide resolved
@tspearconquest
Copy link
Contributor Author

tspearconquest commented Oct 15, 2022 via email

@tspearconquest
Copy link
Contributor Author

Hello, I've reverted my last commit as per your request and also synced master back in.

Please note that this reverts the alpine image in one of the Dockerfiles back to Alpine 3.15 because there is no golang:1.16-alpine3.16in Dockerhub as you can see below:

image

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback!

Dockerfile Outdated Show resolved Hide resolved
@tspearconquest
Copy link
Contributor Author

tspearconquest commented Oct 18, 2022 via email

… stages in docker image

Signed-off-by: Thomas Spear <tspear@conquestcyber.com>
@tspearconquest
Copy link
Contributor Author

The update is done.

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Sure, I will make this change during business hours tomorrow, however I'm going to take an opportunity now to express my frustration at the way this has been handled.

This is my first contribution to this project, and I'm disheartened to have to say that it also will most likely be my last.

I'm sorry to hear that you're frustrated. Thanks for your contributions thus far and I hope that you re-consider contributing again in the future!

It's incredibly unprofessional to ask me 3 separate times to adjust my pull request to patch a high severity CVE when you could've simply patched this yourself while you were upgrading the project to golang 1.19 and closed my PR without merging it. A simple "I'm sorry, your PR was closed because I've upgraded the project and patched this myself in the process," would have sufficed because the users would be able to trust that the software was again safe. Instead, they have been left vulnerable for over a week, and I'm forced to consider creating an internal private fork of this repo to patch future CVEs because my lead developer can't trust that migrate will accept a simple module update in a timely manner. I'm certain that doing so will only add complexity to our internal projects and place extra maintenance burden on myself and the rest of my small DevOps team, however I don't see much other choice given how long it's taken to get this in to upstream.

I can appreciate that you may have been trying to consider my feelings about wanting to contribute by keeping this open and allowing my contribution to continue to be included in the project; however, I'm a security-minded professional and the top priority for me is ensuring that users have access to safe, patched software. That comes over and above any feelings I may have about my contribution being included or receiving credit for it. To be clear, my feelings about my contribution being included are of practically no concern; I just simply wanted this patched ASAP and had hoped to avoid making an internal private fork to do so.

The proposed changes are simple enough for me to do myself but I wanted to give you credit in the commit history and release notes. Since you've given me permission, I'll go ahead and fix any issues and push to your repo/branch to speed things up. Note, not every contributor may feel this way e.g. trade off recognition for release speed.
I could have easily been yelled at or shamed for closing your PR to patch the security vulnerabilities as I cannot know the preferences of a new contributor unless they're stated. As far as I'm aware, the impact of these vulnerabilities are limited since running the migrate docker image is generally used for the CLI use case and not integrated into an application/server so I didn't have a sense of urgency for fixing these issues. I also didn't get a sense of urgency in our correspondence. In the future, please indicate the potential impact and urgency for security issues. I've created a security policy that mentions this so hopefully this will be less of an issue in the future.

If you're ever blocked or have a disagreement we can't resolve, feel free to fork this repo and make any changes you'd like.

Also, keep in mind, I'm volunteering my time for this project and have other obligations, commitments, and deadlines as well. Please understand that I try my best to respond in a timely fashion but may not always be able to do so.

I would be elated at this point, and I think many of your users would agree with me here, if you had patched this, as mentioned above, when you were upgrading the golang version, because I wouldn't be feeling the need to make a fork, and all users would have the ability to be running a patched version of migrate already.

Additionally, as I have mentioned in a previous comment; I've left the checkbox on this pull request enabled to allow maintainers such as yourself to edit it for the very reason that it is time saving for everyone for you to edit my pull request as the maintainer, so I don't understand the logic in continuing to drag this out further by asking me to make the edits myself. Its not that I mind making edits in general; on the contrary, I enjoy the opportunity to be included further. But this is a security risk and the lack of acceptance and release so far places undue burden on everyone who needs to run secure software.

All of that said, as I mentioned, I will push another update during business hours today if you haven't made the edits yourself by then.

To anyone who does read this far, thanks for your time, and I apologize for the length of this post.

Thank you for your feedback. I've made some incremental improvements (e.g. added a security policy) as a result of your feedback. It'd also be nice to have GitHub issue templates for security disclosures (full and coordinated) but I don't have the bandwidth to create those right now.

@@ -1,6 +1,6 @@
module github.com/golang-migrate/migrate/v4

go 1.17
Copy link
Member

Choose a reason for hiding this comment

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

Could you revert this change or use go 1.18 since that's the oldest version we support?

@dhui dhui merged commit c367ed2 into golang-migrate:master Oct 19, 2022
@tspearconquest
Copy link
Contributor Author

tspearconquest commented Oct 19, 2022 via email

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

Successfully merging this pull request may close these issues.

Multiple vulnerabilities reported by image scan
4 participants