Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
many: introduce a tool to sign assertions #1340
Conversation
pedronis
added some commits
May 27, 2016
atomatt
commented on cmd/snap-assert/main.go in 97f3958
Jun 15, 2016
|
It would be nice to mention GNUPGHOME works here too. |
atomatt
reviewed
Jun 16, 2016
| + YAMLInput = "application/x-yaml" | ||
| +) | ||
| + | ||
| +// SignRequest lets specify the complete input for signing an assertion. |
atomatt
reviewed
Jun 16, 2016
| + // or passing the key id | ||
| + KeyID string | ||
| + // and an optional account-id of the signer (if left out headers value are consulted) | ||
| + AuthorityID string |
atomatt
Jun 16, 2016
Contributor
[optional] it might be nicer to group these fields with one doc comment.
atomatt
reviewed
Jun 16, 2016
| + // or containting exactly two entries | ||
| + // "headers": mapping with the header fields | ||
| + // "content-body": used as the content body of the assertion | ||
| + Statement []byte |
atomatt
Jun 16, 2016
Contributor
Maybe group StatementMediaType and Statement with one doc comment too?
atomatt
reviewed
Jun 16, 2016
| +type nestedStatement struct { | ||
| + Headers map[string]interface{} `yaml:"headers" json:"headers"` | ||
| + // XXX/TODO: decide how this should be called, part of larger naming discussion! | ||
| + ContentBody string `yaml:"content-body" json:"content-body"` |
atomatt
Jun 16, 2016
Contributor
The more I see "content-body" the less I like it. I suspect it will never be called "content-body", so I would rename it to "body" now (the sensible choice ;)) in the hope it never needs to be renamed.
atomatt
reviewed
Jun 16, 2016
| +// let the invoker use a limited amount of structured input without having | ||
| +// to convert everything obvious to strings upfront on their side, | ||
| +// we convert integers, bool (to yes|no), list of strings (to comma separated) and nil (to empty) | ||
| + |
atomatt
Jun 16, 2016
Contributor
remove this blank line so it's more obvious it applies to the func?
atomatt
reviewed
Jun 16, 2016
| + c.Check(a.Body(), IsNil) | ||
| +} | ||
| + | ||
| +func (s *signSuite) TestSignKeyIDNestedYAMLWithBodyAndRevision(c *C) { |
atomatt
Jun 16, 2016
Contributor
The "AndRevision" part of this (and TestSignKeyIDNestedJSONWithBodyAndRevision below) feels like it should be a separate test, e.g. TestRequestRevisionOverridesHeaderReevision or similar.
atomatt
reviewed
Jun 16, 2016
| + c.Check(a.Body(), DeepEquals, []byte("CONTENT")) | ||
| +} | ||
| + | ||
| +func (s *signSuite) TestSignAccountKeyHandleNestedYAML(c *C) { |
atomatt
Jun 16, 2016
Contributor
Is the "NestedYAML" part of the name relevant? It's seems like an implementation detail of the test to me.
atomatt
reviewed
Jun 16, 2016
| + KeyID string `long:"key-id" description:"long key id of the GnuPG key to use (otherwise taken from account-key)"` | ||
| + AccountKey string `long:"account-key" description:"file with the account-key assertion of the key to use"` | ||
| + | ||
| + Revision int `long:"revision" description:"revision to set for the assertion (starts and defaults to 0)"` |
atomatt
Jun 16, 2016
Contributor
Might be nice to mention that it overrides anything in the statement.
pedronis
Jun 16, 2016
Contributor
it's a bit true for the lot of them, I think we'll want some actual document about this at some point soon
pedronis
added some commits
Jun 16, 2016
|
+1, thanks for the changes [edit] I've been using this quite a bit over the last few hours to generate account-key and account asserts, signed by keys in a local gnupg keyring. It's nice to use and works really well :) |
kyrofa
reviewed
Jun 27, 2016
| + return asserts.Encode(a), nil | ||
| +} | ||
| + | ||
| +// strigify lets the invoker use a limited amount of structured input |
|
This seems fairly straight-forward, though regarding the usage within snapcraft it would probably be best to have @sergiusens take a look-- he'll have a better idea of how he's planning on using it. |
|
What component has to handle the wizard for setting up? Is the user facing side of this only ever going to be snapcraft? |
|
Hm, it seems the model create, snapcraft and probably others to come will use this. It makes sense to do the gpg setup in a central location as well ( |
|
@sergiusens setting up is a bit of a complex thing and also orthogonal and is mostly a UX problem, but has nothing that is quite specific to the assert library, you should look at Celso's "RFC: User-signed assertions" thread for some sense of the problems there |
pedronis
added
the
Reviewed
label
Jun 29, 2016
pedronis
added some commits
Jul 1, 2016
pedronis
removed
the
Reviewed
label
Jul 4, 2016
atomatt
reviewed
Jul 5, 2016
| + | ||
| +The signer identifier (the `authority-id` header value in the resulting assertion) can be specified with `--authority-id` while the key can be with `--key-id` giving a long key id. Otherwise the same can be achieved with`--account-key` to specify a file with the account key assertion for the intended key from which all the key and signer identifying information can be extracted. | ||
| + | ||
| +## Input (aka statement) for The Assertion |
atomatt
Jul 5, 2016
Contributor
"The" should be lower case here, and "statement" should be title case I think.
atomatt
reviewed
Jul 5, 2016
| +# snap-assert: A Low-level Tool for Assertion Signing | ||
| + | ||
| +`snap-assert` is a low-level tool intended to be called by higher-level and more task specific tools when they need to create | ||
| +and sign specific assertions on behalf of the user: |
atomatt
Jul 5, 2016
Contributor
It would be nice to make this one line, like the other paragraphs in the file.
atomatt
reviewed
Jul 5, 2016
| + | ||
| +## Selecting Keys and Identifying the Signer | ||
| + | ||
| +The signer identifier (the `authority-id` header value in the resulting assertion) can be specified with `--authority-id` while the key can be with `--key-id` giving a long key id. Otherwise the same can be achieved with`--account-key` to specify a file with the account key assertion for the intended key from which all the key and signer identifying information can be extracted. |
atomatt
Jul 5, 2016
Contributor
Maybe something like, "The signer identifier (the authority-id header value in the resulting assertion) can be specified with --authority-id together with --key-id to select the GnuPG key by long key id.", to better explain the two go together?
And I think s/account key assertion/account-key assertion.
atomatt
reviewed
Jul 5, 2016
| + | ||
| +## Input (aka statement) for The Assertion | ||
| + | ||
| +For clarity the type of assertion to create must be given as the first positional argument on the command line. After that optionally an input file called here the _statement_; if it is omitted or `-` is passed stdin will be used. |
atomatt
Jul 5, 2016
Contributor
I sort-of see what you mean by "For clarity" here, but it feels a bit redundant. I also find the rest a little bit hard to read. What about something like:
"The type of assertion to create must be given as the first positional argument on the command line, followed by an optional input file called here the statement. If the statement is omitted or - is passed stdin will be used."
atomatt
reviewed
Jul 5, 2016
| + | ||
| +The remaining values for the headers and body of the assertion will come from the _statement_. It can be formatted either as YAML or JSON (YAML is the default, otherwise `--format yaml|json` helps selecting this). | ||
| + | ||
| +For assertions without body the _statement_ can just be a flat mapping of header names to header values. For assertions with a body, _statement_ can have two top level entries: |
atomatt
reviewed
Jul 5, 2016
| +* `headers` containing again a mapping from header names to values | ||
| +* `body` with the assertion body text | ||
| + | ||
| +In the end in the assertion header values will be text, but abstractely the value of some headers have specific simple types, in these cases it is possible and recommended to use those types (as supported by JSON and YAML) for the header values in _statement_, `snap-assert` will convert to string appropriately: |
atomatt
Jul 5, 2016
Contributor
I think I would change the start of this to:
"In the end, assertion header values will be text but, abstractly, the value of some headers have specific simple types. In these cases, it is possible ..."
|
lgtm, +1. I added a few suggestions to try to improve the long form doc. |
pedronis
added some commits
Jul 5, 2016
atomatt
reviewed
Jul 5, 2016
| +* `headers` containing again a mapping from header names to values | ||
| +* `body` with the assertion body text | ||
| + | ||
| +In the end, the assertion header values will be text, but, abstractely, the values of some headers have specific simple types. In these cases it is possible and recommended to use those types (as supported by JSON and YAML) for the header values in _statement_, `snap-assert` will convert to string appropriately: |
atomatt
Jul 5, 2016
Contributor
sorry, should have pointed this out more explicitly ... s/abstractely/abstractly
niemeyer
reviewed
Jul 11, 2016
| + return nil, err | ||
| + } | ||
| + if nestedStatement.Headers == nil { | ||
| + // flat headers, reparse |
niemeyer
Jul 11, 2016
Contributor
I'm picking this point to comment because it's a good example to highlight, but this is not specific to it: the initial gut reaction while reading this is that the tool is quite loose, with excessive flexibility and fiddling. This is not a good property for something dealing with assertions and signatures.
This ought to be much more tight: take a set of key/value pairs, and that's it. Fields are either there, with their proper names, or they're not. The body will need special handling, but that's trivial to get right by simply saying we have a "body" field, which we should never have as an actual header anyway as it would be very confusing.
That means this interface ends up looking a lot like Database.Sign, which is a good indication and makes me wonder if we need this package at all. Let's see how much we can take out of it.
niemeyer
reviewed
Jul 11, 2016
| + } | ||
| + if req.AccountKey == nil && req.KeyID == "" { | ||
| + return nil, fmt.Errorf("both account-key and key id were not specified") | ||
| + } |
pedronis
Jul 11, 2016
Contributor
they all are, it's mostly about failing errors/having more contextual messages
niemeyer
Jul 11, 2016
Contributor
If the error message is not good, let's fix it, and drop the redundancy. We have exactly the same context inside and outside, after all they're both specifically designed for this job.
niemeyer
reviewed
Jul 11, 2016
| + gotFpr := pk.PublicKey().Fingerprint() | ||
| + if gotFpr != expFpr { | ||
| + return nil, fmt.Errorf("cannot use found private key, fingerprint does not match account-key, expected %q, got: %s", expFpr, gotFpr) | ||
| + } |
niemeyer
Jul 11, 2016
Contributor
Checks above also ought to be done by Database.Sign, and per our conversation today we shouldn't ask the user for the account key, but should rather have it at hand in our database (fetching it if necessary, at some point).
niemeyer
Jul 11, 2016
Contributor
This doesn't look very controversial. Let me know what you need a decision upon more specifically.
niemeyer
reviewed
Jul 11, 2016
| + } | ||
| + | ||
| + if headers["authority-id"] == "" { | ||
| + return nil, fmt.Errorf("cannot sign assertion with unspecified signer identifier (aka authority-id)") |
niemeyer
Jul 11, 2016
Contributor
If the error message is not good, let's fix it, and drop the redundancy. We have exactly the same context inside and outside, after all they're both specifically designed for this job.
niemeyer
reviewed
Jul 11, 2016
| +// without having to convert everything obvious to strings upfront on | ||
| +// their side, we convert integers, bool (to yes|no), list of strings | ||
| +// (to comma separated) and nil (to empty). | ||
| +func stringify(m map[string]interface{}) (map[string]string, error) { |
niemeyer
Jul 11, 2016
Contributor
Very concerned about this one. Sounds completely obvious and innocent, yet it's implicitly defining critical properties about the format of headers. Back then we've constrained the headers to map[string]string precisely so we could not worry by consciously keeping it to a minimal set of types until a further time. This one function is now opening up to nested structures with no discussion about how structured documents should be put in place.
My suggestion is this: to avoid the discussion today, let's keep the tool as taking a map[string]string type. Then, on a follow up let's open the header format itself to be map[string]interface{}, and let's design how structured documents look like both in memory and when serialized.
pedronis
Jul 11, 2016
Contributor
happy to drop this, again any changes to asserts need to fit in the roadmap
niemeyer
Jul 11, 2016
Contributor
Of course, it's always a fine dance. Most of my recommendations here go towards simplification.
niemeyer
reviewed
Jul 11, 2016
| + s = "" | ||
| + case bool: | ||
| + if v { | ||
| + s = "yes" |
niemeyer
Jul 11, 2016
Contributor
"true" and "false" are the default terms in Go, Python, Javascript/JSON, etc, so I wouldn't do yes/no.
niemeyer
reviewed
Jul 11, 2016
| + s = v.String() | ||
| + case int: | ||
| + s = strconv.Itoa(v) | ||
| + case []interface{}: |
niemeyer
Jul 11, 2016
Contributor
Uh.. let's please consider this by itself rather than inside the signing tool.
niemeyer
reviewed
Jul 11, 2016
| + * | ||
| + */ | ||
| + | ||
| +package main |
niemeyer
Jul 11, 2016
Contributor
For the reasons we discussed today, this should probably be inside the "snap" tool itself, as it will talk to snapd for ensuring the content signed is reasonable. "snap sign" sounds nice (even despite that). Then, per above points, let's please simplify its interface to be a bit more tight.
As a strawman, how about cat input.yaml | snap sign, where the input looks like
type: ...
authority-id: ...
...
revision: ...
body:
We don't have to worry about JSON for now.
With that, we can just decode into a map[string]string, take "body" out, and hand onto Database.Sign using the data we can learn directly from it (assertion type, etc).
pedronis
Jul 11, 2016
Contributor
I have mixed feelings about exposing as a top command a tool that needs a working gpg setup and maybe snapcraft interactions (to register keys) to work
niemeyer
added
the
Reviewed
label
Jul 11, 2016
|
the new direction needs a clearer idea about the flow to enable keys so closing for now |
pedronis commentedJun 15, 2016
•
Edited 1 time
-
pedronis
Jun 20, 2016
The branch introduces a tool to sign assertions in the form of stand-alone snap-assert executable,
I think once we understand the needs better it may be moved to a snap subcommand if that is what makes sense. It is mainly intended to be used by other tools (snapcraft, model-creator etc.)
The tool piggy backs to a local GPG setup for signing (through a GPGKeypairManager), and allows to give the input for the assertion as either YAML or JSON, and as flat header values (if the assertion doesn't have a body) or specifying headers and body if not.
Private keys can be identified by long key id or using an account-key assertion as a opaque handle.
Here is a walk through for it:
http://pastebin.ubuntu.com/17367550/
Still to-do: