-
-
Notifications
You must be signed in to change notification settings - Fork 180
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
New implementation for signing releases #274
Conversation
danielsz
commented
Aug 18, 2015
- Client libraries like bootlaces should not have to configure anything.
- Signing and encrypting in Boot-clj works out of the box, with no configuration. This can be made to work because we are going to make assumptions about the end user's environment. In turn, those defaults will be capable of being overridden with environment variables.
- Secret keyring location is the default one ($HOME/.gnupg/secring.pgp). Can be overridden with BOOT_GPG_SECRING.
- Name/Email is the one we find. Provided the secret keyring has only one user, we can be sure that we have the correct one. If more than one are found, we throw an error. Name/Email can be set with BOOT_GPG_USER.
- Default signing key is the first one returned by (keyring/list-public-keys keyring). This will work on a straightforward GPG setup. If the user follows best practices and has derived subkeys from his master keypair, he can set those with BOOT_GPG_SIGNING_KEY_ID and BOOT_GPG_ENCRYPTING_KEY_ID.
* Client libraries like bootlaces should not have to configure anything. * Signing and encrypting in Boot-clj works out of the box, with no configuration. This can be made to work because we are going to make assumptions about the end user's environment. In turn, those defaults will be capable of being overridden with environment variables. * Secret keyring location is the default one ($HOME/.gnupg/secring.pgp). Can be overridden with BOOT_GPG_SECRING. * Name/Email is the one we find. Provided the secret keyring has only one user, we can be sure that we have the correct one. If more than one are found, we throw an error. Name/Email can be set with BOOT_GPG_USER. * Default signing key is the first one returned by (keyring/list-public-keys keyring). This will work on a straightforward GPG setup. If the user follows best practices and has derived subkeys from his master keypair, he can set those with BOOT_GPG_SIGNING_KEY_ID and BOOT_GPG_ENCRYPTING_KEY_ID.
(defn read-pass | ||
[prompt] | ||
(String/valueOf (.readPassword (System/console) prompt nil))) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to either keep this or move it to boot.util
? It seems like a handy function to have hanging around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@micha it's still in boot.pgp
but I agree that a fn reading user input/passwords could be in boot.util
as well.
(#(keyring/get-secret-key keyring %)) | ||
(pgp/unlock-key passphrase))))) | ||
|
||
(defn read-pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about having a prompt as the only way to pass a passphrase around. Unattended builds (CIs) need another option to be able to run. Maybe it's possible to use the .gpgconf? I know you can specify passphrase=..
in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current approach was to use a Java API as opposed to shelling out to the gpg
binary, so .gpgconf
does not apply.
Generally speaking, using the passphrase
switch should be avoided. Passing the passphrase automatically can be achieved with gpg-agent
. But this also relies on the gpg
binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A straightforward solution to your concern would be to add a BOOT_GPG_PASSPHRASE environment variable that, if set, will be used instead of the prompt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, lein uses gpg-agent
which I quite like as an approach because it shifts complexity on the tool itself that you can configure using .gpgconf
. I'd rather have push
accept optionally the passphrase so in my CI machines I can have a profile.boot
that does (task-options! push {:passphrase "My pass"})
. Environmental variable might be overkill in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: environmental variables adding another one might be more in line with the implementation of this pull request, but I'm actually arguing it would be better to remove all of them and pass them explicitly as optional arguments (making the configuration explicit when you call the function rather than relying on the global environment state). Just my 2 cents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current PR, GPG configuration is driven by environment variables. They are documented here: https://github.com/boot-clj/boot/wiki/Deploying-with-Boot
For consistency's sake, the passphrase feature would be just an additional environment variable. Furthermore, the advantage of an environment variable is that the user can use a third-party library like bootlaces
to deploy artefacts. He doesn't need to deal directly with the push
task, and yet he still retains control over configuration options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think He doesn't need to deal directly with the push task, and yet he still retains control over configuration options.
is a non-argument. Rather than having my configuration nicely in a profile.boot now I need to export an environment variable.
Each environment variable you add to the project adds global complexity and clutter. If it's only used by one task, then why not it be a parameter to the task itself? You get automatic documentation for it and you can set it programmatically rather than shelling out.
I mean, fine by all means if you want to give the option to drive the implementation via env vars, but don't take away the task options: they are much more easier to use and repl-friendly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problem with whatever configuration mechanism we are going to adopt. This PR is a proof of concept that can be refined and finalized by others. @micha has started this https://github.com/adzerk-oss/env which will likely address this problem. Please feel free to contribute to this PR.
@micha @alandipert Bumping this PR. Is there something I can do to help get this accepted. I would love to hear your comments or reservations. If I remember correctly, this PR doesn't break anything. |
The implementation looks good but I feel strongly that |
Is there a reason for not using |
I didn't use the |
Leiningen docs mention GPG4Win: https://github.com/technomancy/leiningen/blob/master/doc/GPG.md#windows |
I'm on board with using gpg, especially gpg-agent. Since it uses unix domain sockets we can't do it from java, unfortunately, and will have to shell out to gpg. |
I did quick test with In addition to signing, it supports reading repository credentials from |
Awesome. Any implementation solving the limitations of the current one is a big win. Leveraging |
I merged the alternative implementation using GPG binary. |