Skip to content
This repository was archived by the owner on Aug 2, 2026. It is now read-only.

Fix: openpgp unsupported feature #46 - #51

Closed
nast90210 wants to merge 3 commits into
emersion:masterfrom
nast90210:0.2.2
Closed

Fix: openpgp unsupported feature #46#51
nast90210 wants to merge 3 commits into
emersion:masterfrom
nast90210:0.2.2

Conversation

@nast90210

@nast90210 nast90210 commented Apr 5, 2019

Copy link
Copy Markdown

Hi! I change golang.org/x/crypto to github.com/keybase/go-crypto. And also disable signature key check, because it throw an error

Fixes #46

@emersion emersion left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for your pull request!

Comment thread .gitignore Outdated
.glide/

auth.json
/.DS_Store

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'd prefer not to add platform-specific things (such as .DS_Store) here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@nast90210 one great way to avoid adding this to a particular project is to use a global gitignore file just for your system (macOS in this case): https://help.github.com/en/articles/ignoring-files#create-a-global-gitignore

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@powellc thx man - you advise is great!! I will try it!!

@nast90210 nast90210 Apr 6, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@emersion done

Comment thread protonmail/srp.go Outdated
if err != nil && err != openpgperrors.ErrUnknownIssuer {
return nil, err
}
// _, err := openpgp.CheckDetachedSignature(nil, bytes.NewReader(block.Plaintext), block.ArmoredSignature.Body)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hmm, I'm a little bit sad to see this go away. What is the exact error you're hitting?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@emersion look the second commit

@jeroenev

jeroenev commented Apr 5, 2019

Copy link
Copy Markdown

Disabling signature check, do you really have too? Seems to me like disabling antivirus

@emersion

emersion commented Apr 5, 2019

Copy link
Copy Markdown
Owner

Disabling signature check, do you really have too?

Yeah, I'd prefer not to. This particular signature checks happens when logging in to make sure we're talking to the ProtonMail servers and we've not been MITM'ed (this is an additional layer of security on top of HTTPS). Tat said, the current code doesn't check for the signer (see the TODO), so disabling it doesn't really weaken security.

However I'd like to fix this TODO. Disabling the verification completely goes one step backwards, so I'd like to avoid doing that.

@nast90210

Copy link
Copy Markdown
Author

@emersion Before this pull o have never write code on go. So, when I stuck with the error and see that code is not make security weaker, I just commit it. May be on Monday I can try to finished TODO.

@emersion

emersion commented Apr 5, 2019

Copy link
Copy Markdown
Owner

May be on Monday I can try to finished TODO.

No need to finish the TODO in this PR, this can be done separately and is unrelated. However if you could post here the error message you're getting and try to make the verification work that would be helpful.

@nast90210

Copy link
Copy Markdown
Author

@emersion Hi! I understand what throw error in protonmail/srp.go. Look at second commit. I don't know what openpgperrors.ErrUnknownIssuer error check. IMHO check that error is not NULL is quite enough.

@emersion

emersion commented Apr 6, 2019

Copy link
Copy Markdown
Owner

IMHO check that error is not NULL is quite enough.

Are you able to login (hydroxide auth) after your change?

@nast90210

Copy link
Copy Markdown
Author

@emersion yeap

@nast90210

Copy link
Copy Markdown
Author

@emersion mb someone can check my PR

@emersion

emersion commented Apr 7, 2019

Copy link
Copy Markdown
Owner

hydroxide auth fails with this:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x72312b]

goroutine 1 [running]:
github.com/keybase/go-crypto/openpgp.checkDetachedSignature(0x0, 0x0, 0x94d0c0, 0xc0003162d0, 0x94d760, 0xc00011e1d8, 0x3e, 0xc0000cda98, 0x42c3ef, 0x8)
	/home/simon/go/pkg/mod/github.com/keybase/go-crypto@v0.0.0-20181127160227-255a5089e85a/openpgp/read.go:453 +0x1eb
github.com/keybase/go-crypto/openpgp.CheckDetachedSignature(...)
	/home/simon/go/pkg/mod/github.com/keybase/go-crypto@v0.0.0-20181127160227-255a5089e85a/openpgp/read.go:413
github.com/emersion/hydroxide/protonmail.decodeModulus(0xc0003e82c0, 0x282, 0xc0000cdb18, 0xc0000cdb40, 0x66c0e6, 0xc00013a190, 0x0)
	/home/simon/src/hydroxide/protonmail/srp.go:27 +0x138
github.com/emersion/hydroxide/protonmail.srp(0xc0000cdc08, 0x20, 0x20, 0xc0000c62d0, 0x20, 0x20, 0xc000092fc0)
	/home/simon/src/hydroxide/protonmail/srp.go:154 +0x4d
github.com/emersion/hydroxide/protonmail.(*Client).Auth(0xc0000cdf00, 0x7ffe70925ad9, 0x8, 0xc0000b41e0, 0x20, 0x0, 0x0, 0xc0000c62d0, 0x20, 0x20, ...)
	/home/simon/src/hydroxide/protonmail/auth.go:130 +0x9f
main.main()
	/home/simon/src/hydroxide/cmd/hydroxide/hydroxide.go:98 +0xdb8
exit status 2

@nast90210

Copy link
Copy Markdown
Author

@emersion it's strange, because I get this error without removing && err != openpgperrors.ErrUnknownIssuer, but after fix from commit https://github.com/emersion/hydroxide/pull/51/commits/8b637cb02b4049a32f878df8854576003a638d80 now I don't have error

@emersion

Copy link
Copy Markdown
Owner

Thanks for your PR, but it doesn't seem like that Keybase fork supports the required algorithm.

@emersion emersion closed this Apr 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openpgp unsupported feature

4 participants