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 syscall.Stdin for input handle. Fixes #1153 #1657

Merged
merged 1 commit into from Mar 25, 2022

Conversation

mdp
Copy link
Contributor

@mdp mdp commented Mar 25, 2022

Summary

Cosign currently passes in the int '0' for the Stdin handle when reading from term. This unfortunately doesn't work on Windows, and should actually use syscall.Stdin to get the platform specific Stdin handle.

Ticket Link

Fixes #1153

Release Note

  • Bugfix to handle input on Windows platforms without crashing.

@dlorenc
Copy link
Member

dlorenc commented Mar 25, 2022

Thanks for the fix!

@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2022

Codecov Report

Merging #1657 (ec355ee) into main (340b6c6) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1657      +/-   ##
==========================================
- Coverage   28.81%   28.79%   -0.02%     
==========================================
  Files         139      139              
  Lines        8139     8143       +4     
==========================================
  Hits         2345     2345              
- Misses       5542     5546       +4     
  Partials      252      252              
Impacted Files Coverage Δ
pkg/cosign/common.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 340b6c6...ec355ee. Read the comment docs.

@mdp
Copy link
Contributor Author

mdp commented Mar 25, 2022

Sorry about spamming the CI/PR, let me figure out this lint issue on my own fork and I'll push when it's ready (it has to do with unconvert detecting an uneccessary conversion of 'int' on *nix but it's a uintptr on windows)

While is is 0 on Unix, it's not on Windows. Golang handles this with the
syscall.Stdin targeting Windows.

Signed-off-by: Mark Percival <m@mdp.im>
@mdp
Copy link
Contributor Author

mdp commented Mar 25, 2022

This should be working now and rebased onto the latest main branch. I "nolint'd" the int conversion, but the alternative is to build a platform specific ReadPassword to handle the int vs uintptr divergence. Happy to quickly build that out if you think it's the right direction.

@dlorenc dlorenc merged commit 25b1f36 into sigstore:main Mar 25, 2022
@github-actions github-actions bot added this to the v1.7.0 milestone Mar 25, 2022
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
While is is 0 on Unix, it's not on Windows. Golang handles this with the
syscall.Stdin targeting Windows.

Signed-off-by: Mark Percival <m@mdp.im>
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.

Cosign crashes when password input is requested from terminal in Windows
3 participants