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.

Sign up for GitHub

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

Jump to bottom

New implementation for signing releases #274

Closed
wants to merge 1 commit into from
Closed

New implementation for signing releases #274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions boot/core/src/boot/task/built_in.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,9 +693,6 @@
[f file PATH str "The jar file to deploy."
F file-regex MATCH #{regex} "The set of regexes of paths to deploy."
g gpg-sign bool "Sign jar using GPG private key."
k gpg-user-id NAME str "The name used to find the GPG key."
K gpg-keyring PATH str "The path to secring.gpg file to use for signing."
p gpg-passphrase PASS str "The passphrase to unlock GPG signing key."
r repo ALIAS str "The alias of the deploy repository."
t tag bool "Create git tag for this version."
B ensure-branch BRANCH str "The required current git branch."
Expand Down Expand Up @@ -726,7 +723,7 @@
snapshot? (.endsWith v "-SNAPSHOT")
artifact-map (when gpg-sign
(util/info "Signing %s...\n" (.getName f))
(helpers/sign-jar tgt f gpg-passphrase gpg-keyring gpg-user-id))]
(helpers/sign-jar tgt f))]
(assert (or (not ensure-branch) (= b ensure-branch))
(format "current git branch is %s but must be %s" b ensure-branch))
(assert (or (not ensure-clean) clean?)
Expand Down
20 changes: 5 additions & 15 deletions boot/core/src/boot/task_helpers.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,11 @@
(->> sym ns-aliases (into base) (mapcat pubs)))
(filter task?) (sort-by :name) (group-by :ns*) (into (sorted-map)))))

(defn read-pass
[prompt]
(String/valueOf (.readPassword (System/console) prompt nil)))

(defn sign-jar [out jar pass keyring user-id]
(let [prompt (pod/with-call-worker
(boot.pgp/prompt-for ~keyring ~user-id))
pass (or pass (read-pass prompt))]
(pod/with-call-worker
(boot.pgp/sign-jar
~(.getPath out)
~(.getPath jar)
~pass
:keyring ~keyring
:user-id ~user-id))))
(defn sign-jar [out jar]
(pod/with-call-worker
(boot.pgp/sign-jar
~(.getPath out)
Copy link
Contributor

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.

Copy link
Member

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.

~(.getPath jar))))

(defn print-fileset
[fileset]
Expand Down
2 changes: 1 addition & 1 deletion boot/worker/project.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
[clj-jgit "0.8.0"]
[clj-yaml "0.4.0"]
[javazoom/jlayer "1.0.1"]
[mvxcvi/clj-pgp "0.5.4"]
[mvxcvi/clj-pgp "0.8.2"]
[net.java.dev.jna/jna "4.1.0"]
[alandipert/desiderata "1.0.2"]
[org.clojure/data.xml "0.0.8"]
Expand Down
96 changes: 57 additions & 39 deletions boot/worker/src/boot/pgp.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,56 +2,74 @@
(:require
[clojure.java.io :as io]
[boot.pod :as pod]
[mvxcvi.crypto.pgp :as pgp])
(clj-pgp
[core :as pgp]
[keyring :as keyring]
[signature :as pgp-sig]))
(:import
[java.util.regex Pattern]
[java.io StringBufferInputStream]))

(def default-secring
(-> (System/getProperty "user.home")
(io/file ".gnupg" "secring.gpg")))

(defn secring
[& [file]]
(pgp/load-secret-keyring (io/file (or file default-secring))))

(defn get-pubkey
[secring & [user]]
(let [ks (pgp/list-public-keys secring)
re-user #(re-find (re-pattern (Pattern/quote (str user))) %)
match? #(some re-user (-> % pgp/key-info :user-ids))]
(let [ks' (if-not user ks (filter match? ks))]
(if (or (not user) (<= (count ks') 1))
(first ks')
(throw (ex-info "multiple keys match"
{:keys (mapv pgp/key-info ks')}))))))

(defn secret-key-for
[secring pubkey]
(pgp/get-secret-key secring (pgp/key-id pubkey)))

(defn prompt-for
[keyring user-id]
(if-let [pk (-> keyring secring (get-pubkey user-id))]
(-> pk pgp/key-info :user-ids first (str "\nGPG passphrase: "))
(throw (Exception. (format "public key not found (%s)" user-id)))))

(defn sign
[content passphrase & {:keys [keyring user-id]}]
(let [sr (secring keyring)
pk (get-pubkey sr user-id)
sk (secret-key-for sr pk)
pr (pgp/unlock-key sk passphrase)]
(-> content (pgp/sign :sha1 pr) pgp/encode-ascii)))
(defn find-secring []
(let [home (System/getenv "HOME")
locations [(System/getenv "GNUPGHOME") (System/getenv "BOOT_GPG_SECRING") (str home "/.gnupg/secring.gpg") (str home "/.gpg/secring.gpg")]
exists-fn #(when (and % (.exists (io/as-file %))) %)
secring (some exists-fn locations)]
(or secring (throw (Exception. "Can't find secring file. Please set 'BOOT_GPG_SECRING'")))))

(defn get-keyring []
(keyring/load-secret-keyring (io/file (find-secring))))

(defn find-master-key-by-user [user]
(let [keyring (get-keyring)
pub-keys (keyring/list-public-keys keyring)
re-user #(re-find (re-pattern user) %)
match? #(some re-user (-> % pgp/key-info :user-ids))
key (some #(when (match? %) %) pub-keys)]
(or key (throw (Exception. (format "User %s not found" user))))))

(defn find-master-key []
(if-let [user (System/getenv "BOOT_GPG_USER")]
(find-master-key-by-user user)
(let [keyring (get-keyring)
master-keys (filter (comp :master-key? pgp/key-info) (keyring/list-public-keys keyring))]
(cond
(= 0 (count master-keys)) (throw (Exception. "No master key found"))
(= 1 (count master-keys)) (first master-keys)
:else (throw (Exception. "Multiple master keys found. Please set 'BOOT_GPG_USER'"))))))

(defn get-private-key-by-id [hex-id passphrase]
(let [keyring (get-keyring)
seckey (keyring/get-secret-key keyring hex-id)]
(pgp/unlock-key seckey passphrase)))

(defn get-private-key [passphrase]
(let [keyring (get-keyring)]
(if-let [hex-id (System/getenv "BOOT_GPG_SIGNING_KEY_ID")]
(get-private-key-by-id hex-id passphrase)
(-> (find-master-key)
pgp/key-info
:key-id
(#(keyring/get-secret-key keyring %))
(pgp/unlock-key passphrase)))))

(defn read-pass
Copy link

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link

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.

Copy link

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.

Copy link
Contributor Author

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.

Copy link

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.

Copy link
Contributor Author

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.

[prompt]
(String/valueOf (.readPassword (System/console) prompt nil)))

(defn sign [content]
(let [passphrase (read-pass (str "\nGPG passphrase: "))
pr (get-private-key passphrase)]
(-> content (pgp-sig/sign pr) pgp/encode-ascii)))

(defn sign-jar
[outpath jarpath passphrase & {:keys [keyring user-id]}]
[outpath jarpath]
(let [outdir (doto (io/file outpath) .mkdirs)
jarname (.getName (io/file jarpath))
pomxml (StringBufferInputStream. (pod/pom-xml jarpath))
jarout (io/file outdir (str jarname ".asc"))
pomout (io/file outdir (.replaceAll jarname "\\.jar$" ".pom.asc"))
sign-it #(sign % passphrase :keyring keyring :user-id user-id)]
sign-it #(sign %)]
(spit pomout (sign-it pomxml))
(spit jarout (sign-it (io/input-stream jarpath)))
{[:extension "jar.asc"] (.getPath jarout)
Expand Down
2 changes: 1 addition & 1 deletion version.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
version=2.2.0
version=2.2.1
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change to the API, no? We've removed options from tasks, etc. Perhaps we bump to 3.0.0?