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 gpg signing during sonatype publishing to work with gpg versions: 2.1+ #753

Closed
wants to merge 1 commit into from

Conversation

pmellati
Copy link
Contributor

@pmellati pmellati commented Jan 1, 2020

Sets flag --pinentry-mode to loopback, to prevent gpg from interactively asking for
the gpg passphrase. This flag is necessary for newer versions of gpg.

The flag has been available in gpg since version 1.4.0 , so this patch should be compatible with gpg versions 1.4.0 and up.

See: https://www.gnupg.org/%28de%29/documentation/manuals/gpgme/Pinentry-Mode.html

… 2.1+

Sets flag '--pinentry-mode' to 'loopback', to prevent gpg from interactively asking for
the gpg passphrase.

See: https://www.gnupg.org/%28de%29/documentation/manuals/gpgme/Pinentry-Mode.html
@lefou
Copy link
Member

lefou commented Jan 5, 2020

We shouldn't hard code that at all. It could be a parameter to a sign command. I also think, that it should be possible to leave some defaults in a gnupg config file. Actually, I use the interactive default, when publishing non-automatically.

@lihaoyi
Copy link
Member

lihaoyi commented Jan 10, 2020

So this is why I haven't managed to get non-interactive gpg signing to work. Looking forward to getting this merged

@pmellati
Copy link
Contributor Author

@lefou As an immediate fix, I think it makes sense to hardcode this new parameter. It accomplishes the same purpose as the other pre-existing hardcoded parameter (i.e. --batch --yes).

If that's not an option, I can close this PR and work on another PR to make the gpg params configurable, but it will take more time.

@jodersky
Copy link
Member

jodersky commented Feb 9, 2020

We shouldn't hard code that at all. It could be a parameter to a sign command. I also think, that it should be possible to leave some defaults in a gnupg config file. Actually, I use the interactive default, when publishing non-automatically.

I absolutely agree with @lefou here: gpg's config should only be handled by gpg, and never by the build tool. See this discussion in an issue in sbt-gpg. The gist of it is that key material may be provided in many different ways and that assuming the existence of a passphrase is already too restrictive (I'm using a yubikey for example).

As noted in the discussion, in case non-interactive signing is required (for example in CI), the recommended approach is to store the keychain itself in an encrypted format and remove the password from the GPG private key. See this section of sbt-gpg's readme on how this can be done.

@joan38
Copy link
Collaborator

joan38 commented Apr 29, 2020

We are basically not able to publish to sonatype from CI. I'm actually wondering how people have been doing so far.
How can we move forward on this? How can we set --pinentry-mode=loopback in config of gpg?

@jodersky
Copy link
Member

We are basically not able to publish to sonatype from CI.

That hasn't been my experience. The way to do it is to not set a passphrase on the gpg key[1]. Then the pinentry flag is not even required.

[1] the reasons for not assuming passphrases are detailed in the discussion above and int linked sbt-gpg project. It essentially boils down to supporting to passphrase protection being a too strong assumption and not something that should be handled by mill

@jodersky
Copy link
Member

Look at this how-to from sbt-gpg, it applies to mill as well. Here's an example ci build as well.

@joan38
Copy link
Collaborator

joan38 commented Apr 29, 2020

Ok I just saw the recommendation to remove the password from the key.

What makes --batch --yes hardcoded more legit over --pinentry-mode=loopback?
This publish target takes a --gpgPassphrase in argument we are obviously no looking for gpg to ask the passphrase on the standard input but rather take that passed from Mill. Isn't that right?
In what circumstances are we going to set --pinentry-mode to another mode?

@jodersky
Copy link
Member

Hmm, didn't know there was this argument actually. Let me see

@joan38
Copy link
Collaborator

joan38 commented May 2, 2020

Any more opinions on this?

@jodersky
Copy link
Member

jodersky commented May 3, 2020

The gpgPassphrase options seem to predate much of the discussion. I would actually suggest to remove them entirely.

What makes --batch --yes hardcoded more legit over --pinentry-mode=loopback?

It's a matter of assumptions and generalization. Who is to say that the key is a file protected with a passphrase in the first place? It could very well be in an HSM or obtained through some other mechanism.

The important thing to note is that we are using gpg to sign artifacts with a key. The passphrase is just an implementation detail about which mill should not concern itself. That is up to gpg.

@jodersky
Copy link
Member

jodersky commented May 3, 2020

Wrt to removing the gpgPassphrase options, I actually think it would make sense to generalize them. Instead of having a "passphrase" option, why not create a parameter "gpg options" which gets passed verbatim to gpg? That should allow enough flexibility to accommodate all use cases.

@joan38
Copy link
Collaborator

joan38 commented May 5, 2020

Here is the implementation: #851

@lefou lefou added the solved The issue was fixed/resolved label May 6, 2020
@lefou lefou added this to the 0.7.0 milestone May 6, 2020
@lefou
Copy link
Member

lefou commented May 6, 2020

Fixed by #851

@lefou lefou closed this May 6, 2020
@Ammonite-Bot
Copy link
Collaborator

Ammonite-Bot commented May 6, 2020 via email

lefou pushed a commit that referenced this pull request Sep 23, 2023
…#2782)

A few years ago how to handle `gpg`'s changing requirements were
extensively discussed here, and very reasonably resolved. See
#753

Nevertheless, having just upgraded from a very ancient `gpg`, I hit this
issue and it took a small storm of googling to figure it out. Maybe it's
worth adding a note to the documentation?

Pull request: #2782
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solved The issue was fixed/resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants