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

Fix endless loop in x/crypto/openpgp func ReadMessage #690

Merged
merged 4 commits into from Jul 14, 2020

Conversation

uwehdaub
Copy link
Contributor

@uwehdaub uwehdaub commented Jul 8, 2020

I wasn't able to write tests for it, because for this problem to occur, I had to produce a slightly inconsistent state
with private keys. I guess this only occurs if some old private key (GPG 1?) are migrated to GPG 2.
But this is only guessing.

Nevertheless I was able to test it locally with my GPG key, which produced this problem, before I fixed it.

This fixes getsops#665
See also golang/go#28786

In some strange situations it can happen, that openpgp.ReadMessage()
runs into a endless loop. This seems to be triggered by a slightly
inconsistency in key settings.
It happened to me, but I wasn't able to reproduce it with a fresh key.
A proposed solution from the x/crypto community was, to break this loop
in the callback passphrasePrompt.
@uwehdaub
Copy link
Contributor Author

uwehdaub commented Jul 8, 2020

I fixed the test, so that they run locally, but this now breaks TravisCI.
Is this a well-known problem with the tests?

Copy link
Contributor

@autrilla autrilla left a comment

Choose a reason for hiding this comment

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

LGTM. The lack of tests is acceptable in this case.

pgp/keysource.go Outdated
maxCalls := 3
return func(keys []openpgp.Key, symmetric bool) ([]byte, error) {
if callCounter >= maxCalls {
return nil, fmt.Errorf("function passphrasePrompt called more than once")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("function passphrasePrompt called more than once")
return nil, fmt.Errorf("function passphrasePrompt called too many times")

I have no idea what the cause of the issue is, but if it's known, we could hint at the user what it is.

@autrilla
Copy link
Contributor

autrilla commented Jul 8, 2020

I fixed the test, so that they run locally, but this now breaks TravisCI.
Is this a well-known problem with the tests?

It is. #576 had a fix for it, but we never ended up merging it.

@codecov-commenter
Copy link

Codecov Report

Merging #690 into develop will increase coverage by 0.02%.
The diff coverage is 12.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #690      +/-   ##
===========================================
+ Coverage    38.23%   38.26%   +0.02%     
===========================================
  Files           23       23              
  Lines         3329     3335       +6     
===========================================
+ Hits          1273     1276       +3     
- Misses        1927     1929       +2     
- Partials       129      130       +1     
Impacted Files Coverage Δ
pgp/keysource.go 32.89% <12.50%> (+0.46%) ⬆️

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 7ae1968...5ff1968. Read the comment docs.

@autrilla
Copy link
Contributor

Thanks!

@autrilla autrilla merged commit 4f06780 into getsops:develop Jul 14, 2020
clelange pushed a commit to clelange/sops that referenced this pull request Jul 19, 2020
* Fix tests

* Fix endless loop in x/crypto/openpgp func ReadMessage

This fixes getsops#665
See also golang/go#28786

In some strange situations it can happen, that openpgp.ReadMessage()
runs into a endless loop. This seems to be triggered by a slightly
inconsistency in key settings.
It happened to me, but I wasn't able to reproduce it with a fresh key.
A proposed solution from the x/crypto community was, to break this loop
in the callback passphrasePrompt.

* Revert "Fix tests"

This reverts commit 285f4dc.

* Improve error description

getsops#690 (comment)
Ph0tonic added a commit to Ph0tonic/sops that referenced this pull request Nov 14, 2023
Signed-off-by: Bastien <bastien.wermeille@gmail.com>
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.

None yet

4 participants