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

don't bypass the web of trust (#378) #379

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

Lightning-
Copy link
Contributor

the choice whether to use a web of trust and on which trust level is up to the user of PGP/GPG and must not be overriden by tools that are set on top

users can decide to ignore this safety net by setting their gpg.conf adequately, defining an alias for gpg --trust-model=always or passing the env GPG to blackbox in this way but we should not override their preferences hardcoded

tlimoncelli
tlimoncelli previously approved these changes Nov 13, 2023
@fraenki
Copy link

fraenki commented Nov 14, 2023

users can decide to ignore this safety net by setting their gpg.conf adequately, defining an alias for gpg --trust-model=always or passing the env GPG to blackbox in this way but we should not override their preferences hardcoded

@Lightning-, maybe you could add this information to the README? I'd figure some people may run into issues and prefer the old way (for whatever reason).

@Lightning-
Copy link
Contributor Author

@tlimoncelli I'm trying to analyze why the test fails but somehow I'm a little bit lost in the mixture of go and bash (I'm not a golang guy but think I can read the code; maybe I made a mistake somewhere ...).
I do have a basic idea but haven't found the code yet.

Could you drop me a verbose/debug run of the check somehow?

@Lightning-
Copy link
Contributor Author

hmmm, actually this doesn't print all those logDebug.Printf() calls, which I wanted to look at to find the right passage(s) :(

@Lightning-
Copy link
Contributor Author

@tlimoncelli I think we gotta fix several other things here first, because the current go-based test is not (no longer? gpg2 etc.) solid enough. This would be beneficial for my other upcoming patches as well.

  • At first, this assumption is no longer correct: https://github.com/StackExchange/blackbox/blob/master/integrationTest/ithelpers.go#L320-L327
    gpg-agent, when using gpg2, doesn't return the GPG_AGENT_INFO anymore as the sockets are created in the gpg home directory (defaulting to ~/.gnupg/ if not passed/configured otherwise).
    In that case this isn't the reason for the breakdown of the test, but the agent IS running here although the test states it's not!
    gpg-agent[5489]: gpg-agent (GnuPG) 2.2.27 started
    WARNING: gpg-agent didn't output what we expected. Assumed dead.
    
    So here you want to test for the existence of the socket files or running process instead. Here I can't help: my go is weak ;).
  • Next is, that --dry-run actually is not a dry run in every situation and this has side effects! Especially here it's not: https://github.com/StackExchange/blackbox/blob/master/integrationTest/ithelpers.go#L422-L442
    to demonstrate this (and here the ERROR: AdminAdd failed AddNewKey: RunBash cmd="/usr/bin/gpg2" err=exit status 2 comes from):
    $ rm -r .gnupg
    
    $ gpg-agent --daemon -s
    gpg-agent[8377]: directory '/user/.gnupg' created
    gpg-agent[8377]: directory '/user/.gnupg/private-keys-v1.d' created
    gpg-agent[8378]: gpg-agent (GnuPG) 2.4.3 started
    
    $ tree .gnupg
    .gnupg
    ├── S.gpg-agent
    ├── S.gpg-agent.browser
    ├── S.gpg-agent.extra
    ├── S.gpg-agent.ssh
    └── private-keys-v1.d
    
    2 directories, 4 files
    
    $ gpg2 --list-pub
    gpg: keybox '/user/.gnupg/pubring.kbx' created
    gpg: /user/.gnupg/trustdb.gpg: trustdb created
    
    $ printf '%s\n' $?
    0
    
    $ gpg2 --list-pub
    
    $ printf '%s\n' $?
    0
    
    $ gpg2 --dry-run --quick-generate-key --batch --passphrase "" foo rsa encr
    gpg: directory '/user/.gnupg/openpgp-revocs.d' created
    gpg: revocation certificate stored as '/user/.gnupg/openpgp-revocs.d/55868E410B9D5085164493DB5CC57225FE78D262.rev'
    
    $ printf '%s\n' $?
    0
    
    $ tree .gnupg
    .gnupg
    ├── S.gpg-agent
    ├── S.gpg-agent.browser
    ├── S.gpg-agent.extra
    ├── S.gpg-agent.ssh
    ├── openpgp-revocs.d
    ├── private-keys-v1.d
    │   └── 601BCFE009210F13F66DDBFC2A6EE43E635F3777.key
    ├── pubring.kbx
    └── trustdb.gpg
    
    $ gpg2 --list-pub
    gpg: checking the trustdb
    gpg: public key of ultimately trusted key 5CC57225FE78D262 not found
    gpg: marginals needed: 3  completes needed: 1  trust model: pgp
    gpg: depth: 0  valid:   1  signed:   0  trust: 0-, 0q, 0n, 0m, 0f, 1u
    
    $ printf '%s\n' $?
    2
    
    $ rm .gnupg/private-keys-v1.d/601BCFE009210F13F66DDBFC2A6EE43E635F3777.key 
    
    $ gpg2 --list-pub
    
    $ printf '%s\n' $?
    0
    
    So if you wanna stick with the hasQuick() method you have to remove this generated keyfile afterwards. But I'd recommend to simply test for gpg2 --version and assume that gpg2 has --quick-generate-key.

the choice whether to use a web of trust and on which trust level is up
to the user of PGP/GPG and must not be overriden by tools that are set
on top

users can decide to ignore this safety net by setting their gpg.conf
adequately, defining an alias for `gpg --trust-model=always` or passing
the env GPG to blackbox in this way but we should not override their
preferences hardcoded
add note about the web of trust
assume that we have `--quick-generate-key` if we run gpg2 instead of
doing a dry run for that (which has side effects that break the test)
@Lightning-
Copy link
Contributor Author

@tlimoncelli can we try to proceed here?

@tlimoncelli
Copy link
Contributor

Thanks for the reminder. I was out for a bit.

@tlimoncelli
Copy link
Contributor

Thanks for tightening the security of Blackbox!

@tlimoncelli tlimoncelli merged commit 3a137a4 into StackExchange:master Jan 17, 2024
2 checks passed
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.

3 participants